#32885: CsrfViewMiddlewareTestMixin contains some logic specific to the
CSRF_USE_SESSIONS=False case
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Component: CSRF | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Description changed by Chris Jerdonek:
Old 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`.
>
> [https://github.com/django/django/blob/e9fbd7348013bce753c0f4e0e492007f50a87095/tests/csrf_tests/tests.py#L87-L106
> 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`.
>
> [https://github.com/django/django/blob/e9fbd7348013bce753c0f4e0e492007f50a87095/tests/csrf_tests/tests.py#L330-L340
> test_token_node_with_new_csrf_cookie()] would fail for
> `CsrfViewMiddlewareUseSessionsTests`, but it is (accidentally?) masked by
> [https://github.com/django/django/blob/e9fbd7348013bce753c0f4e0e492007f50a87095/tests/csrf_tests/tests.py#L1050-L1060
> CsrfViewMiddlewareUseSessionsTests.test_token_node_with_new_csrf_cookie()].
>
> And
> [https://github.com/django/django/blob/e9fbd7348013bce753c0f4e0e492007f50a87095/tests/csrf_tests/tests.py#L342-L358
> test_cookie_not_reset_on_accepted_request()] would normally fail for
> `CsrfViewMiddlewareUseSessionsTests`, but the `if` check in
> [https://github.com/django/django/blob/e9fbd7348013bce753c0f4e0e492007f50a87095/tests/csrf_tests/tests.py#L354
> 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.
New description:
In
[https://github.com/django/django/blob/e9fbd7348013bce753c0f4e0e492007f50a87095/tests/csrf_tests/tests.py
tests/csrf_tests/tests.py],
[https://github.com/django/django/blob/e9fbd7348013bce753c0f4e0e492007f50a87095/tests/csrf_tests/tests.py#L34
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`.
[https://github.com/django/django/blob/e9fbd7348013bce753c0f4e0e492007f50a87095/tests/csrf_tests/tests.py#L87-L106
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`.
[https://github.com/django/django/blob/e9fbd7348013bce753c0f4e0e492007f50a87095/tests/csrf_tests/tests.py#L330-L340
test_token_node_with_new_csrf_cookie()] would fail for
`CsrfViewMiddlewareUseSessionsTests`, but it is (accidentally?) masked by
[https://github.com/django/django/blob/e9fbd7348013bce753c0f4e0e492007f50a87095/tests/csrf_tests/tests.py#L1050-L1060
CsrfViewMiddlewareUseSessionsTests.test_token_node_with_new_csrf_cookie()].
And
[https://github.com/django/django/blob/e9fbd7348013bce753c0f4e0e492007f50a87095/tests/csrf_tests/tests.py#L342-L358
test_cookie_not_reset_on_accepted_request()] would normally fail for
`CsrfViewMiddlewareUseSessionsTests`, but the `if` check in
[https://github.com/django/django/blob/e9fbd7348013bce753c0f4e0e492007f50a87095/tests/csrf_tests/tests.py#L354
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.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/32885#comment:2>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/067.ac424c79a35b139d546c2a74fe35d6f8%40djangoproject.com.
