Opened 17 years ago

Closed 14 years ago

Last modified 13 years ago

#6552 closed Bug (fixed)

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

Reported by: olau@… Owned by: Luke Plant
Component: contrib.auth Version: dev
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 Ole Laursen 16 years ago.
Patch against 9820 for removing admin's dependency on auth context processor
6552.diff (9.2 KB ) - added by Luke Plant 15 years ago.
make the context processor return objects lazily
6552_2.diff (9.7 KB ) - added by Luke Plant 15 years ago.
various fixes to my last patch

Download all attachments as: .zip

Change History (29)

comment:1 by Jacob, 16 years ago

Component: UncategorizedAuthentication
Owner: nobody removed
Triage Stage: UnreviewedAccepted

comment:2 by Jacob, 16 years ago

milestone: 1.0

comment:3 by David Cramer, 16 years ago

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

comment:4 by Jeremy Dunck, 16 years ago

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 by James Bennett, 16 years ago

milestone: 1.0post-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 by Ole Laursen <olau@…>, 16 years ago

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?

in reply to:  5 comment:7 by md@…, 16 years ago

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 by Ole Laursen, 16 years ago

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.

by Ole Laursen, 16 years ago

Attachment: admin-context.patch added

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

comment:9 by Lau Bech Lauritzen, 16 years ago

Any progress on this?

comment:10 by (none), 16 years ago

milestone: post-1.0

Milestone post-1.0 deleted

comment:11 by Ole Laursen, 16 years ago

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 by Alexander Schepanovski, 15 years ago

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

comment:13 by Alexander Schepanovski, 15 years ago

Cc: suor@… added

comment:14 by Luke Plant, 15 years ago

Owner: set to Luke Plant
Status: newassigned

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.

by Luke Plant, 15 years ago

Attachment: 6552.diff added

make the context processor return objects lazily

by Luke Plant, 15 years ago

Attachment: 6552_2.diff added

various fixes to my last patch

comment:15 by Luke Plant, 15 years ago

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

comment:16 by Luke Plant, 15 years ago

Resolution: fixed
Status: assignedclosed

(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 by Luke Plant, 15 years ago

(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 by harm, 15 years ago

Resolution: fixed
Status: closedreopened

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 by Luke Plant, 15 years ago

Resolution: fixed
Status: reopenedclosed

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 by harm, 15 years ago

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 by intrepidweb, 14 years ago

Resolution: fixed
Status: closedreopened

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 by Luke Plant, 14 years ago

Resolution: fixed
Status: reopenedclosed

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 by Mathieu Pillard, 13 years ago

Cc: django@… added
Easy pickings: unset
Severity: Normal
Type: 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 by Luke Plant, 13 years ago

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 by lpiatek, 13 years ago

Cc: lpiatek added
Type: UncategorizedBug

+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 new cache!).

In my setup, daily avarage CPU LOAD, caused by this issue went to somethig like 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?

Version 1, edited 13 years ago by lpiatek (previous) (next) (diff)

comment:26 by Luke Plant, 13 years ago

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.

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