[Django] #32885: CsrfViewMiddlewareTestMixin contains some logic specific to the CSRF_USE_SESSIONS=False case
Django
CSRF_USE_SESSIONS=False case
-------------------------------------+-------------------------------------
Reporter: Chris | Owner: Chris Jerdonek
Jerdonek |
Type: | Status: assigned
Cleanup/optimization |
Component: CSRF | Version: dev
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32885>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
CSRF_USE_SESSIONS=False case
-------------------------------------+-------------------------------------
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
-------------------------------------+-------------------------------------
Comment (by Chris Jerdonek):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32885#comment:1>
Django
CSRF_USE_SESSIONS=False case
-------------------------------------+-------------------------------------
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
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
--
--
Ticket URL: <https://code.djangoproject.com/ticket/32885#comment:2>
Django
CSRF_USE_SESSIONS=False case
-------------------------------------+-------------------------------------
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/32885#comment:3>
Django
CSRF_USE_SESSIONS=False case
-------------------------------------+-------------------------------------
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
PR: https://github.com/django/django/pull/14570
--
Ticket URL: <https://code.djangoproject.com/ticket/32885#comment:4>
Django
CSRF_USE_SESSIONS=False case
-------------------------------------+-------------------------------------
Type: | Jerdonek
Component: CSRF | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"43d1ea6e2ff1982d52faf6b04d613390220e5d7a" 43d1ea6]:
{{{
#!CommitTicketReference repository=""
revision="43d1ea6e2ff1982d52faf6b04d613390220e5d7a"
Refs #32885 -- Used _read_csrf_cookie()/_set_csrf_cookie() in more CSRF
tests.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32885#comment:6>
Django
CSRF_USE_SESSIONS=False case
-------------------------------------+-------------------------------------
Type: | Jerdonek
Cleanup/optimization | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"abc8795632e423974a628f4725702f015b2d1ed8" abc87956]:
{{{
#!CommitTicketReference repository=""
revision="abc8795632e423974a628f4725702f015b2d1ed8"
Fixed #32885 -- Removed cookie-based token specific logic from
CsrfViewMiddlewareTestMixin.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32885#comment:5>