Code

Opened 6 years ago

Closed 4 years ago

Last modified 2 years ago

#6552 closed Bug (fixed)

django.core.context_processors.auth causes "Vary: Cookie" header no matter what

Reported by: olau@… Owned by: lukeplant
Component: contrib.auth Version: master
Severity: Normal Keywords:
Cc: suor@…, django@…, lpiatek Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The session framework is pretty cool in that it will automatically add a "Vary: Cookie" header to responses but only if it is accessed.

Unfortunately this is completely defeated when you add "django.core.context_processors.auth" as a context processor because it will always access the session object to get the user object out. Which wouldn't be a huge problem if it weren't for the fact that you need this context processor for admin to work. So I can either have wicked cool caching or I can have the admin.

I think that either the context processor should be rewritten to retrieve the attributes lazily, or admin should be rewritten to rely on the request context processor (using "request.user" instead of "user", etc.). Or something like that. If admin has it own view functions, I don't see why it would even need to rely on a context processor. What do you think?

Attachments (3)

admin-context.patch (6.3 KB) - added by olau 5 years ago.
Patch against 9820 for removing admin's dependency on auth context processor
6552.diff (9.2 KB) - added by lukeplant 5 years ago.
make the context processor return objects lazily
6552_2.diff (9.7 KB) - added by lukeplant 5 years ago.
various fixes to my last patch

Download all attachments as: .zip

Change History (29)

comment:1 Changed 6 years ago by jacob

  • Component changed from Uncategorized to Authentication
  • Needs documentation unset
  • Needs tests unset
  • Owner nobody deleted
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 6 years ago by jacob

  • milestone set to 1.0

comment:3 Changed 6 years ago by dcramer

I'm +1 for removing the requisite of a context processor and just manually including that context processor into each admin view.

comment:4 Changed 6 years ago by jdunck

FWIW, you can run a site's admin as an entirely separate project (just have a different settings file and bind it to a different host.

admin.yoursite.com running admin_settings, where yoursite.com runs settings, and has no need to include the auth context processor.

comment:5 follow-up: Changed 6 years ago by ubernostrum

  • milestone changed from 1.0 to post-1.0

This feels too much like a feature request and, as Jeremy says, if you want to force this to be admin-only behavior you can run the admin separately. Moving to post-1.0.

comment:6 Changed 6 years ago by Ole Laursen <olau@…>

Maybe cache performance is not a high priority, but there's still the thing that all sites out there using the admin are evaluating the user and messages call. The lazy loaded PermWrapper helps a bit but still. I have a crude patch for loading user and messages lazily too (I'm on a pre-queryset-refactor version).

But if the general opinion is in favour of changing admin, how do we move this forward?

comment:7 in reply to: ↑ 5 Changed 6 years ago by md@…

I think at theast there should be some documentation somewhere (besides in this ticket) that use of django.core.context_processors.auth breaks caching.

comment:8 Changed 5 years ago by olau

  • Has patch set
  • Needs documentation set

Here's a patch against 9820. It introduces a django.contrib.admin.context.AdminContext which acts like RequestContext except it includes the user/messages/perms variables that admin depends on (internally it simply calls the auth context processor). This removes admin's dependency on the auth context processor being in settings.

Rationale: while we'd want to phase out general use of the auth context processor, admin changes should remain as backwards compatible as possible and extending admin must remain easy.

I haven't written any documentation yet. I suggest that the admin documentation recommends using AdminContext instead of RequestContext for building admin pages. Also as the previous commenter said, the documentation about the auth context processor should mention its side-effects (and the cache docs should possibly also).

Then the question is how to phase out the context processor. Currently, it's included by default in global_settings.py which is backwards - it should be a deliberate choice by those who understand the trade-off. Are there any other options than taking it out and mention it in the notes about backwards incompatibility? While admin will be fine, it'll break for apps that are otherwise depending on the variables.

Just to make it clear why the context processor is a problem: If you're using it in combination with any tracking framework using cookies (such as Google Analytics), caching is by default broken on all pages using RequestContext and for *all* visitors, including those without an account (because even if you don't login, you still get a unique cookie by the tracking framework). And it's certainly not easy to spot.

Changed 5 years ago by olau

Patch against 9820 for removing admin's dependency on auth context processor

comment:9 Changed 5 years ago by lau

Any progress on this?

comment:10 Changed 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:11 Changed 5 years ago by olau

I'm waiting for feedback on the approach. If it's OK, I'll see if I can rip out an hour and do the documentation changes. I'm getting tired of having to hack this on all Django projects I set up. :)

By the way, here's a context processor that can be used to disable the effect of the auth context processor without setting up two sites or patching Django:

def auth_fixer(request):
    """Replace request.user with AnonymousUser unless we're in admin."""
    if not request.path.startswith("/admin"):
        from django.contrib.auth.models import AnonymousUser
        request.user = AnonymousUser()
    
    return {}

Reference it in TEMPLATE_CONTEXT_PROCESSORS in settings.py before the auth context processor. An important gotcha with this hack is that request.user is gone after you process a template with RequestContext, so be careful.

comment:12 Changed 5 years ago by Suor

django.core.context_processors.auth should return lazy user and messages fields.
It would almost solve this ticket

comment:13 Changed 5 years ago by Suor

  • Cc suor@… added

comment:14 Changed 5 years ago by lukeplant

  • Owner set to lukeplant
  • Status changed from new to assigned

I think by far the best way to fix this is to make the context processor return user/messages/perms lazily. This is more tricky than it sounds, but I have a patch that I think works, with tests, I'll attach shortly.

Changed 5 years ago by lukeplant

make the context processor return objects lazily

Changed 5 years ago by lukeplant

various fixes to my last patch

comment:15 Changed 5 years ago by lukeplant

I realise part of this overlaps slightly with #4604. But this is really a separate issue.

comment:16 Changed 5 years ago by lukeplant

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [11623]) Fixed #6552, #12031 - Make django.core.context_processors.auth lazy to avoid "Vary: Cookie"

Thanks to olau@…, Suor for the report

comment:17 Changed 5 years ago by lukeplant

(In [11624]) [1.1.X] Fixed #6552, #12031 - Make django.core.context_processors.auth lazy to avoid "Vary: Cookie"

Thanks to olau@…, Suor for the report

Backport of r11623 from trunk

comment:18 Changed 4 years ago by harm

  • Resolution fixed deleted
  • Status changed from closed to reopened

It seems its not fixed, or reintroduced...

I see Vary: Cookie headers appearing on views that don't touch the session at all.
Which makes it impossible in practice to cache these, either by (reverse) proxies nor by (most) browsers.

version: Django-1.2-beta-1

steps to reproduce

  • enable session framework
  • use django.core.context_processors.auth

write a simple view:

def some_view(request):
    return HttpResponse("Hello, world. You're at the poll index.")

result

This view always get a Vary: Cookie header set by django
this page then never gets cached by most browsers (even firerfox), not even when adding a max-age=14400 cache-control header.

expected

any view, not touching the session, should not get a Vary: Cookie http header.

comment:19 Changed 4 years ago by lukeplant

  • Resolution set to fixed
  • Status changed from reopened to closed

I cannot reproduce. I started a new project, which has the session framework and middleware turned on by default, and I modified the view so that it actually uses RequestContext:

from django.template import Template, RequestContext
from django.http import HttpResponse

def some_view(request):
    return HttpResponse(Template("Hello, world.").render(RequestContext(request)))

With this view I get:

$ curl -i http://localhost:8000/
HTTP/1.0 200 OK
Date: Sat, 27 Feb 2010 17:40:23 GMT
Server: WSGIServer/0.1 Python/2.5.4
Content-Type: text/html; charset=utf-8

Hello, world.

If I change the template to `"Hello, {{ user }}", I get:

$ curl -i http://localhost:8000/
HTTP/1.0 200 OK
Date: Sat, 27 Feb 2010 17:42:22 GMT
Server: WSGIServer/0.1 Python/2.5.4
Vary: Cookie
Content-Type: text/html; charset=utf-8

Hello, AnonymousUser

So it seems to be working correctly. I see this both with current trunk and with the 1.2 beta 1 (revision 12394) . Please re-open if you can provide more details that will demonstrate your bug.

comment:20 Changed 4 years ago by harm

You're right. There is no bug.
I had a custom context processor, that was touching the session.
Without that all is OK.

comment:21 Changed 4 years ago by intrepidweb

  • Resolution fixed deleted
  • Status changed from closed to reopened

Hi -- I am not sure if this is related, but I want to report it in case. I recently had a problem with the Vary:Cookie header unexpectedly being added to my responses. I scoured my code for unintentional accessing of the session. I couldn't find anything. Eventually through testing I pinpointed the auth context processor (django.contrib.auth.context_processors.auth) as the source of the problem. My temporary fix was to comment out the 'messages' assignment in the dict returned by the processor. That did the trick; now the Vary:Cookie header is only being added when the session is indeed accessed. Any ideas of what's going on? Should get_messages perhaps be lazily accessed, just like get_user?

comment:22 Changed 4 years ago by lukeplant

  • Resolution set to fixed
  • Status changed from reopened to closed

From my experiments, it is still is behaving correctly, using any backend including the SessionStorage backend. The get_messages call is effectively lazy because the object it returns only accesses the session if the contents are accessed by iterating over it. Perhaps you have a template somewhere that is iterating over the 'messages' value.

If you can attach a minimal test project with views and templates that show the problem, please re-open.

comment:23 Changed 2 years ago by mat

  • Cc django@… added
  • Easy pickings unset
  • Severity set to Normal
  • Type set to Uncategorized
  • UI/UX unset

Just reproduced this with django 1.3, using the simple view+template mentionned in [(comment 19).

The trick to reproduce this is simply to remove django.contrib.messages.middleware.MessageMiddleware from MIDDLEWARE_CLASSES. If you do this, messages continue to work as far as I've tested (provided you kept the auth context_processor), but they are no longer lazy. The auth context_processor calls messages.get_messages() which looks like this:

def get_messages(request):
    if hasattr(request, '_messages'):
        return request._messages

    def get_user():
        if hasattr(request, 'user'):
            return request.user
        else:
            from django.contrib.auth.models import AnonymousUser
            return AnonymousUser()

    return lazy(memoize(get_user().get_and_delete_messages, {}, 0), list)()

The get_and_delete_messages is lazy, but the get_user() call just before isn't: it's evaluated directly, and causes request.user to be evaluated, which in turn causes the Vary: Cookie header to appear.

Because get_and_delete_messages have been removed in django trunk following the deprecation timeline, this isn't maybe very relevant any more, but considering django 1.3 is still the stable version, maybe it is... We didn't need django.contrib.messages on our website, so we thought it would be a good idea to remove the middleware, and certainly didn't expect this to have such repercussions.

comment:24 Changed 2 years ago by lukeplant

We don't backport normal bug fixes to the stable branch, only critical ones, so unless you can reproduce this with trunk, there is no point filing a bug because it won't be fixed. Sorry!

comment:25 Changed 2 years ago by lpiatek

  • Cc lpiatek added
  • Type changed from Uncategorized to Bug

+1 for mat, I crossed this issue today. When we remove django.contrib.messages.middleware.MessageMiddleware and keep the django.contrib.auth.middleware.AuthenticationMiddleware we automatically get session wide cache instead of full cache (django adds 'Vary: Cookie' to every response, so each unique user is 'under' new cache!).

In my setup, daily avarage CPU LOAD, caused by this issue went to 2.52 from 0.3 ...

I didn't get so deep like mat, but problem is still there, I see two ways of 'fixing' this:

  • just add info/note to cache documentation it is necessary to kepp django.contrib.messages.middleware.MessageMiddleware when django.contrib.auth.middleware.AuthenticationMiddleware is on and CACHE_MIDDLEWARE_ANONYMOUS_ONLY = True
  • make get_messages() lazy like mat suggested

I would preffer mat's version but I assume we need new bug and new patch?

Last edited 2 years ago by lpiatek (previous) (diff)

comment:26 Changed 2 years ago by lukeplant

As stated before: if you cannot reproduce this bug using trunk, there is no point either re-opening or filing a new bug.

If you can reproduce it, I would open a new bug, unless the original issue of this ticket has re-surfaced, which seems unlikely since the tests are passing. (The tests could have bugs in them, of course).

Thanks.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.