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 Initial Version
| 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
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.