[Django] #27686: calls to request.user.is_authenticated returns vary by cookie header for all users
Django
all users
-----------------------------------------+------------------------
Reporter: Jeff Willette | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.10
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
If request.user.is_authenticated() is called in a view, a `Vary: "Cookie"`
Http header is returned, even if the user is an anonymous user. The
anonymous user has no `sessionid` and no other cookies set. This means
that any other inconsequential cookies that are in the request (such as
google analytics) will cause downstream caches to cache separate pages for
each user.
I think that is the user is not_authenticated, then there should be no
`Vary` header sent back from django.
You can recreate this problem by creating a new django project and
creating a view that returns an HttpResponse after calling to
is_authenticated with an anonymous user, and also calling another view
that does not call to is_authenticated and comparing the HttpHeaders. I
have done so here: https://github.com/deltaskelta/django-is-authenticated-
vary-headers.git
--
Ticket URL: <https://code.djangoproject.com/ticket/27686>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
all users
Reporter: Jeff Willette | Owner: nobody
Component: contrib.auth | Version: 1.10
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
Changes (by Jeff Willette):
* type: Uncategorized => Bug
* component: Uncategorized => contrib.auth
--
Ticket URL: <https://code.djangoproject.com/ticket/27686#comment:1>
Django
all users
Reporter: Jeff Willette | Owner: nobody
Component: contrib.auth | Version: 1.10
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
Isn't the `Vary: Cookie` header
[https://github.com/django/django/blob/944610a86c81f90c3c79e5440a5e2c706aa0ef62/django/contrib/sessions/middleware.py#L44-L45
added] the `SessionMiddleware` if the session is accessed? Anonymous users
can have sessions. Feel free to propose a patch, otherwise I'm not sure
what to do.
--
Ticket URL: <https://code.djangoproject.com/ticket/27686#comment:2>
Django
all users
Reporter: Jeff Willette | Owner: nobody
Component: contrib.sessions | Version: 1.10
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
Changes (by Jeff Willette):
* component: contrib.auth => contrib.sessions
Comment:
I think I have a working fix here:
https://github.com/django/django/pull/7802/commits/1777eac1da084577fb10020c685d21f0dfcf71a8
I changed one other test that tested for the vary cookie to be sent when I
don't think it should be (on an empty session)
--
Ticket URL: <https://code.djangoproject.com/ticket/27686#comment:3>
Django
all users
Reporter: Jeff Willette | Owner: Jeff
| Willette
Type: Bug | Status: assigned
Component: contrib.sessions | Version: 1.10
Severity: Normal | Resolution:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jeff Willette):
* owner: nobody => Jeff Willette
* status: new => assigned
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/27686#comment:4>
Django
all users
Reporter: Jeff Willette | Owner: Jeff
| Willette
Component: contrib.sessions | Version: 1.10
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carl Meyer):
* status: assigned => closed
* resolution: => invalid
Comment:
I don't think the analysis in this ticket is correct. If we leave out the
`Vary: Cookie` for responses to unauthenticated users, when the view
checked the session (e.g. even just by calling
`request.is_authenticated()`), then a cache could incorrectly serve the
unauthed version of the response to an authenticated user.
Fundamentally, if your view ever checks the session and modifies the
response according to its contents (or lack thereof), then that response
is cookie-dependent and should have `Vary: Cookie` set. Whether the
session was empty or not is irrelevant; if a different session cookie
could have resulted in a different response, we need `Vary: Cookie`.
--
Ticket URL: <https://code.djangoproject.com/ticket/27686#comment:5>
Django
all users
Reporter: Jeff Willette | Owner: Jeff
| Willette
Type: Bug | Status: closed
Component: contrib.sessions | Version: 1.10
Severity: Normal | Resolution: invalid
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carl Meyer):
If you need a shared-cacheable response, then you simply need to avoid
ever checking the session at all: the response must be the same for ALL
users, regardless of their session or lack thereof. If you need a small
user-specific portion (e.g. a logged-in notice in the top right or
whatnot) in an otherwise shared-cacheable e.g. landing page, one technique
is to populate that user-specific portion with a separate Ajax request.
--
Ticket URL: <https://code.djangoproject.com/ticket/27686#comment:6>
Django
all users
Reporter: Jeff Willette | Owner: Jeff
| Willette
Component: contrib.sessions | Version: 1.10
Severity: Normal | Resolution:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by David Szotten):
* status: closed => new
* resolution: invalid =>
Comment:
I'm running into a similar issue but have a different idea for a fix. I
may be missing some reason why this also doesn't work but thought worth
checking:
what if {{{get_user}}} ( {{{django/contrib/auth/__init__.py}}}) checks if
{{{request.session.session_key is None}}} (and if so just returns
{{{AnonymousUser()}}}) before accessing the session?
--
Ticket URL: <https://code.djangoproject.com/ticket/27686#comment:7>
Django
all users
Reporter: Jeff Willette | Owner: Jeff
| Willette
Component: contrib.sessions | Version: 1.10
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* status: new => closed
* resolution: => invalid
Comment:
Hi David.
As I read the discussion, the issue is that the `is_authenticated()` check
could return `True`, i.e. if there's any point in checking it at all, then
we really **should** set the `Vary` header.
(Hence the ticket being closed as invalid.)
This is independent of whether there's a way we could detect this without
triggering it being set, which would be to introduce a regression on this
understanding.
Please see the
[https://docs.djangoproject.com/en/dev/internals/contributing/triaging-
tickets/ Triage Flow guidelines] and
TicketClosingReasons/DontReopenTickets — if there's doubt about a
resolution, much better that it's addressed on the DevelopersMailingList
where it can be properly discussed. Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/27686#comment:8>
Django
all users
Reporter: Jeff Willette | Owner: Jeff
| Willette
Type: Bug | Status: closed
Component: contrib.sessions | Version: 1.10
Severity: Normal | Resolution: invalid
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by David Szotten):
yes, i was just reaching that conclusion myself. thanks, and apologies for
the noise
--
Ticket URL: <https://code.djangoproject.com/ticket/27686#comment:9>