Opened 3 years ago

Last modified 3 years ago

#32885 closed Cleanup/optimization

CsrfViewMiddlewareTestMixin contains some logic specific to the CSRF_USE_SESSIONS=False case — at Version 2

Reported by: Chris Jerdonek Owned by: Chris Jerdonek
Component: CSRF Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Chris Jerdonek)

In tests/csrf_tests/tests.py, CsrfViewMiddlewareTestMixin is only supposed to contain logic common to both the CSRF_USE_SESSIONS=True and CSRF_USE_SESSIONS=False cases (since both CsrfViewMiddlewareTests and CsrfViewMiddlewareUseSessionsTests inherit from it). However, I noticed that it contains some logic specific to the CSRF_USE_SESSIONS=False case.

Specifically, CsrfViewMiddlewareTestMixin's test_process_response_get_token_not_used(), test_token_node_with_new_csrf_cookie(), test_cookie_not_reset_on_accepted_request() all check resp.cookies, even though that attribute is specific to CSRF_USE_SESSIONS=False.

test_process_response_get_token_not_used() "accidentally" passes for CsrfViewMiddlewareUseSessionsTests on this line: https://github.com/django/django/blob/e9fbd7348013bce753c0f4e0e492007f50a87095/tests/csrf_tests/tests.py#L106
because the cookie is never set with CSRF_USE_SESSIONS=True.

test_token_node_with_new_csrf_cookie() would fail for CsrfViewMiddlewareUseSessionsTests, but it is (accidentally?) masked by CsrfViewMiddlewareUseSessionsTests.test_token_node_with_new_csrf_cookie().

And test_cookie_not_reset_on_accepted_request() would normally fail for CsrfViewMiddlewareUseSessionsTests, but the if check in this line causes the main assertion to be skipped. (Looking into why this if check is necessary is what caused me to discover this issue.)

These tests should be modified to work for both CsrfViewMiddlewareTests and CsrfViewMiddlewareUseSessionsTests, by accessing the cookie token from the proper store (using a method overridden in the concrete class), similar to how it's done for setting the cookie in the store.

Change History (2)

comment:1 by Chris Jerdonek, 3 years ago

I'm planning to work on this ticket after #32843 is addressed, since I will be doing some minor refactoring in the resolution of that ticket.

comment:2 by Chris Jerdonek, 3 years ago

Description: modified (diff)
Note: See TracTickets for help on using tickets.
Back to Top