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 )
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 , 3 years ago
comment:2 by , 3 years ago
Description: | modified (diff) |
---|
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.