#6552 closed Bug (fixed)
django.core.context_processors.auth causes "Vary: Cookie" header no matter what
Reported by: | 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)
Change History (29)
comment:1 by , 17 years ago
Component: | Uncategorized → Authentication |
---|---|
Owner: | removed |
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 17 years ago
milestone: | → 1.0 |
---|
comment:3 by , 16 years ago
comment:4 by , 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.
follow-up: 7 comment:5 by , 16 years ago
milestone: | 1.0 → 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 by , 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?
comment:7 by , 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 , 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 , 16 years ago
Attachment: | admin-context.patch added |
---|
Patch against 9820 for removing admin's dependency on auth context processor
comment:11 by , 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 , 15 years ago
django.core.context_processors.auth should return lazy user and messages fields.
It would almost solve this ticket
comment:13 by , 15 years ago
Cc: | added |
---|
comment:14 by , 15 years ago
Owner: | set to |
---|---|
Status: | new → 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.
comment:15 by , 15 years ago
I realise part of this overlaps slightly with #4604. But this is really a separate issue.
comment:16 by , 15 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:17 by , 15 years ago
comment:18 by , 15 years ago
Resolution: | fixed |
---|---|
Status: | closed → 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 by , 15 years ago
Resolution: | → fixed |
---|---|
Status: | reopened → 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 by , 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 , 14 years ago
Resolution: | fixed |
---|---|
Status: | closed → 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 by , 14 years ago
Resolution: | → fixed |
---|---|
Status: | reopened → 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 by , 13 years ago
Cc: | 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 , 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 , 13 years ago
Cc: | added |
---|---|
Type: | Uncategorized → 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?
comment:26 by , 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.
I'm +1 for removing the requisite of a context processor and just manually including that context processor into each admin view.