Opened 7 years ago

Closed 4 years ago

Last modified 4 years ago

#27686 closed Bug (invalid)

calls to request.user.is_authenticated returns vary by cookie header for all users

Reported by: Jeff Willette Owned by: Jeff Willette
Component: contrib.sessions Version: 1.10
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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

Change History (9)

comment:1 by Jeff Willette, 7 years ago

Component: Uncategorizedcontrib.auth
Type: UncategorizedBug

comment:2 by Tim Graham, 7 years ago

Isn't the Vary: Cookie header 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.

comment:3 by Jeff Willette, 7 years ago

Component: contrib.authcontrib.sessions

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)

comment:4 by Jeff Willette, 7 years ago

Has patch: set
Owner: changed from nobody to Jeff Willette
Status: newassigned

comment:5 by Carl Meyer, 7 years ago

Resolution: invalid
Status: assignedclosed

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.

comment:6 by Carl Meyer, 7 years ago

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.

comment:7 by David Szotten, 4 years ago

Resolution: invalid
Status: closednew

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?

comment:8 by Carlton Gibson, 4 years ago

Resolution: invalid
Status: newclosed

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

comment:9 by David Szotten, 4 years ago

yes, i was just reaching that conclusion myself. thanks, and apologies for the noise

Note: See TracTickets for help on using tickets.
Back to Top