TemplateResponse and decorator_from_middleware
Luke Plant
decorator_from_middleware was not updated to match. This produces at
least this problem:
The csrf_protect decorator, which is just
decorator_from_middleware(CsrfViewMiddleware), does not work correctly
on its own. Because the template isn't rendered until after the
decorator has fully run, get_token() is not called before
CsrfViewMiddleware.process_response, which means the latter does not
realise it has to send the cookie, and doesn't do so.
An example view which demonstrates the problem is the login view, which
now uses TemplateResponse (since April 22nd - had it been earlier, I
would have found this sooner).
I imagine there are others, especially with caching decorators, but I'm
surprised this hasn't been caught earlier.
However, I also imagine there will be problems if we change this, or at
least we could end up removing some of the usefulness of
TemplateResponse, since any decorator based on decorator_from_middleware
will cause the template to be rendered.
I've written a patch (attached), and running the test suite with it
produces no errors, but I'm still a bit nervous about it. Any input
would be appreciated.
Regards,
Luke
--
Parenthetical remarks (however relevant) are unnecessary
Luke Plant || http://lukeplant.me.uk/
Luke Plant
problems.
First, existing decorators:
1) It looks like the cache_page decorator will actually work with
TemplateResponse, but only because the cache middleware special-cases
responses with a 'render' method.
This is cheating really - if we had to patch this middleware to get it
to handle TemplateResponse, then there will be many other 3rd party
pieces of middleware that will need similar fixes, yet we gave no-one
any warning of this in the 1.3 release of this fairly major backwards
incompatibility.
2) crsf_protect, and almost certainly conditional_page and gzip_page,
are broken for TemplateResponse objects (to a greater or lesser degree),
due to decorator_from_middleware not rendering TemplateResponse objects
before passing them to the process_response method.
So, we need to fix decorator_from_middleware. I think that is unavoidable.
Once we do that, and even before we do it in the case of cache_page, we
have to face the fact that these decorators, and possibly others like
them, cause TemplateResponse objects to be rendered. This means that the
usefulness of TemplateResponse and 'template response middleware' are
seriously reduced:
These features have the stated purpose of allowing a response to be
altered by middleware or decorators. However, if a decorator causes the
template to be rendered, then such alterations will not happen.
This is a serious issue, because decorators like these are often applied
on an ad-hoc basis, without thinking that they are going to change
things like the body of the response.
So, if we need to cache one page, on a site without global caching, the
obvious thing to do is to add the cache_page decorator. Unfortunately,
if you were actually using a template response middleware, you'll almost
certainly have breakage. It is really bad that adding cache_page can
change what would actually be rendered, and this could be extremely hard
to debug. Similarly with the other affected decorators.
I can see 2 ways that TemplateResponses are still useful:
1) if you have 'sub views', which are private and are not going to be
decorated with any of these things, and want to return responses to the
main view for finalising, and you only put decorators on the main view.
2) if you provide re-usable views in a re-usable app, which have
deliberately have no decorators applied, but come with instructions
about the decorators that have to be applied to them.
However, the 2nd of these could make things pretty icky.
A case in point is django.contrib.auth.views.login. Once we fix
csrf_protect, the TemplateResponse returned will be immediately baked,
and so it is pointless using it in the first place. If we don't put
csrf_protect on it, we've got a security problem - we agreed that with
re-usable apps, we don't want to rely on people using
CsrfViewMiddleware, but we also don't want to rely on people reading
instructions like "make sure you apply csrf_protect to this view".
The only solution I can see that isn't awful is providing two views - a
default and documented one that has all the right decorators applied,
and one that is obviously 'use at own risk', called something like
'login_unsafe', for which we document the extra things you need.
I can see even less room for process_template_response - it just isn't
useful to add a middleware for the sole purpose of messing with the body
of any response, only to find that in many cases you can't mess with
them after all, since the developer had to do something like add caching
to a particular view etc.
So where does this leave us? I don't know. I did have some bad feelings
about TemplateResponse and the idea of lazy responses, but didn't voice
them at the time because I didn't have anything but a vague feeling of
unease (which is not something you can either defend or argue against),
and I wasn't involved in reviewing that feature much.
Carl Meyer
On 05/11/2011 05:37 PM, Luke Plant wrote:
> The only solution I can see that isn't awful is providing two views - a
> default and documented one that has all the right decorators applied,
> and one that is obviously 'use at own risk', called something like
> 'login_unsafe', for which we document the extra things you need.
>
> I can see even less room for process_template_response - it just isn't
> useful to add a middleware for the sole purpose of messing with the body
> of any response, only to find that in many cases you can't mess with
> them after all, since the developer had to do something like add caching
> to a particular view etc.
>
> So where does this leave us? I don't know. I did have some bad feelings
> about TemplateResponse and the idea of lazy responses, but didn't voice
> them at the time because I didn't have anything but a vague feeling of
> unease (which is not something you can either defend or argue against),
> and I wasn't involved in reviewing that feature much.
Yeah, this is bad. I don't have a lot to add at the moment, except to
also point out #15855 - cache_page as a decorator breaks proper setting
of Vary headers.
The reason I bring it up here is just to point out another case,
unrelated to TemplateResponse, where we promote the idea that you can
use a decorator as a view-specific equivalent to a middleware, but doing
so breaks the semantics of the original middleware in subtle but
significant ways (because it changes the order things happen in).
Not sure what that implies, except that maybe decorator_from_middleware
and the pattern it encourages is ultimately at least as problematic as
TemplateResponse...
Carl
Luke Plant
> Yeah, this is bad. I don't have a lot to add at the moment, except to
> also point out #15855 - cache_page as a decorator breaks proper setting
> of Vary headers.
>
> The reason I bring it up here is just to point out another case,
> unrelated to TemplateResponse, where we promote the idea that you can
> use a decorator as a view-specific equivalent to a middleware, but doing
> so breaks the semantics of the original middleware in subtle but
> significant ways (because it changes the order things happen in).
>
> Not sure what that implies, except that maybe decorator_from_middleware
> and the pattern it encourages is ultimately at least as problematic as
> TemplateResponse...
Yeah, I guess I was seeing TemplateResponse as the culprit, on the basis
of Last In First Out, but maybe it's decorators we need to be worrying
about.
I've actually documented a specific instance of #15855 in the CRSF docs
And while we're on the subject, another problem with decorators vs.
middleware:
A stack of decorators can behave differently from the corresponding
stack of middleware, due to subtle differences in the ordering of method
calls e.g. [m1.process_request, m2.process_request, m1.process_view,
m2.process_view] vs [m1.process_request, m1.process_view,
m2.process_request, m2.process_view]
Regards,
legutierr
I actually faced a problem similar to this when I ported CBV from 1.3
to 1.2, and then tried to use the TemplateResponse with a couple of
middleware decorators that I created from some custom middleware
classes. If I recall properly, I was also having backwards
incompatibility problems with some other middleware classes that I had
installed.
The solution for me was actually very simple: instead of raising a
ContentNotRenderedError on line 110 of django/template/response.py, I
modified the code of _get_content() to call self.render(), that way
making a TemplateResponse look just like a standard response with a
static "content" field. Doing it this way still allows
TemplateResponse to be useful, it just requires you to be careful how
you order middleware classes so that the template context is updated
before request.content is accessed (i.e. at the very front). It's a
small race condition, but not inconsistent with the other ordering
issues that go along with using middleware.
That being said, I don't know if the bug I encountered was exactly the
same as what you are describing, I can't quite recall the details, but
I just wanted to throw this out there anyway. This may not be a
complete or ideal solution, but if it works, it seems like a better
option than changing the middleware API--and better than making
backwards-incompatible changes to decorator_from_middleware.
Regards,
Ed Gutierrez
legutierr
> These features have the stated purpose of allowing a response to be
> altered by middleware or decorators. However, if a decorator causes the
> template to be rendered, then such alterations will not happen.
>
> This is a serious issue, because decorators like these are often applied
> on an ad-hoc basis, without thinking that they are going to change
> things like the body of the response.
>
> So, if we need to cache one page, on a site without global caching, the
> obvious thing to do is to add the cache_page decorator. Unfortunately,
> if you were actually using a template response middleware, you'll almost
> certainly have breakage. It is really bad that adding cache_page can
> change what would actually be rendered, and this could be extremely hard
> to debug. Similarly with the other affected decorators.
"small race condition" (is it a race condition? maybe that's the wrong
term as concurrency doesn't come into play), I hadn't thought this
through. Yeah, the usefulness of the TemplateResponse is much reduced
when you have to worry about decorators accessing the content of the
response. But that should be obvious, right, at least for caching?
Certainly if you cache a response before you modify it either the
cache will have to be wrong or the modifications have to will fail.
Carl Meyer
On May 11, 7:00 pm, Luke Plant <L.Plant...@cantab.net> wrote:
> Yeah, I guess I was seeing TemplateResponse as the culprit, on the basis
> of Last In First Out, but maybe it's decorators we need to be worrying
> about.
>
> I've actually documented a specific instance of #15855 in the CRSF docs
morning. What if, instead of running process_response immediately, it
annotated the response with metadata saying "please run this
process_response middleware on me!" And then the running of that
middleware actually occurred later.
There'd be a couple possible variants of "later": right before your
actual middleware runs, right after, or somewhere in the middle (you
pick when). This latter option would have to be controlled by
something like a "ViewDecoratorMiddleware" which you put into your
MIDDLEWARE_CLASSES. I think this control might be necessary to get the
ordering right in some cases; there are other middlewares that should
always be first/last, so I don't think either "before all" or "after
all" would suffice. This ViewDecoratorMiddleware would also give us a
backwards-compatible way to introduce the change: if you've got this
ViewDecoratorMiddleware in your MIDDLEWARE_CLASSES,
decorator_from_middleware annotates. If you don't, it sticks with
current behavior (and raises a PendingDeprecationWarning).
I think we could even do something similar for process_view, if the
decorator annotates the actual view function with which additional
process_view middlewares should be run on it. Process_request can't be
helped; since request middleware can change the urlconf, thus
resolving to a different view, there's no way we can run request
middleware when it ought to have been run, if its applied as a
decorator. In any case, though, I think the most serious problems here
have to do with when process_response runs; process_request and
decorator_from_middleware doesn't seem as problematic.
It seems to me that this approach could potentially fix the cache_page/
Vary headers problem and the CSRF issues, make both of them work
nicely with TemplateResponse, and generally make
decorator_from_middleware less problematic.
Let me get a bit crazy now: If we want to fix the problem even more
fully, get rid of ViewDecoratorMiddleware, and allow full ordering
control of when the various middlewares-as-decorators are applied,
another option would be a way to include a specific middleware in your
MIDDLEWARE_CLASSES, but annotated as "decorator-only". So you'd go
ahead and include e.g. CsrfViewMiddleware in MIDDLEWARE_CLASSES but
marked decorator-only, and then it would only apply to requests/
responses where the resolved view has been annotated with the
csrf_protect decorator. Again, for backwards-compatibility,
decorator_from_middleware would check to see if you have the given
middleware in your MIDDLEWARE_CLASSES, marked decorator-only: if you
do, it would annotate, if you don't, it would run eagerly as it does
now.
Thoughts? Does something along these lines seem worth pursing as a
fix? Or am I off my rocker?
Carl
Luke Plant
I think your core idea is quite interesting. I haven't had time to think
through all the implications.
It does make the result of what is returned from view functions rather
obscure, and this could affect things like testing.
For example, the new tests I wrote for decorator_from_middleware, which
pass with my first patch on #16004, will not pass with this
implementation. This means the semantics are quite difference. It also
means that tests using RequestFactory could be broken, because they
won't pass through the machinery of request handling.
However, with one modification, we can get something quite a bit better.
Basically, if it is a TemplateResponse, we defer the process_response
handling as you say. Otherwise, we do it the way we currently do it.
In fact, we *almost* have machinery in place to do this -
'TemplateResponse.add_post_render_callback' - and I've got a patch based
on that which works. The only problem is that currently
'process_response' is allowed to return a completely new response
object, whereas that is not supported with add_post_render_callback.
That would be easy to change, however - or we could add a similar hook
which does support it. Either way, it would require changing code like this:
response.render()
to
response = response.render()
This way, the only change people have to cope with in tests is adding
'response = response.render()' to things that have 'render', rather than
having to invoke some other machinery to get the expected result -
TemplateResponse has all the machinery itself. Since they already have
to invoke 'render' for anything that has changed to TemplateResponse,
this change adds no extra burden.
(BTW, we do need to document the change introduced by [16087] in the 1.4
release notes as a breaking change - any tests against customized admin
views that use RequestFactory will currently break if they don't add
'.render()' calls.)
Putting all these things together, I've uploaded a new patch to #16004:
http://code.djangoproject.com/attachment/ticket/16004/16004.fix.alternative.diff
It is not nearly as invasive as the other changes you suggested.
However, it does fix the failing tests in my project (i.e. fixes
csrf_protect), and leaves TemplateResponse objects unrendered as long as
possible, without having to change one line of my own code/tests. And
the implementation doesn't make me puke either.
I agree that we ought to be doing something about middleware ordering
and things like that, but perhaps that is something for Django 2000 -
when we can sit down and work out how everything should fit together.
One little note while I remember - I'm really not happy about decorator
functions looking at settings to see how they should work - this is
fixing fragility by adding more fragility. We want to be getting rid of
this kind of thing, not adding more of it. There was also the general
assumption of just one view being involved, one response object. We
don't want to break code that is slightly more inventive, like this:
http://docs.djangoproject.com/en/dev/ref/contrib/csrf/#view-needs-protection-for-one-path
(Sorry for vague criticisms, I don't have more time to spend on this today).
Regards,
Luke
--
"The first ten million years were the worst. And the second ten
million, they were the worst too. The third ten million, I didn't
enjoy at all. After that I went into a bit of a decline." (Marvin
the paranoid android)
Luke Plant || http://lukeplant.me.uk/
Carl Meyer
On 05/23/2011 11:31 AM, Luke Plant wrote:
> I think your core idea is quite interesting. I haven't had time to think
> through all the implications.
>
> It does make the result of what is returned from view functions rather
> obscure, and this could affect things like testing.
I don't think it's any more obscure - an HttpResponse is still an
HttpResponse - just different. It changes the semantics of
decorator_from_middleware so you can't expect the middlewares to be
applied immediately without further response handling (more like actual
middleware).
> For example, the new tests I wrote for decorator_from_middleware, which
> pass with my first patch on #16004, will not pass with this
> implementation. This means the semantics are quite difference. It also
> means that tests using RequestFactory could be broken, because they
> won't pass through the machinery of request handling.
Yes, the semantics would be different, and this would break some current
tests that assume the current semantic of immediate application as part
of the view function call.
The entire issue, though, is that the current semantic of
decorator_from_middleware is broken, because middlewares can't reliably
be converted into something that runs immediately rather than in the
middleware phase, and still work properly.
One other option, I suppose, would be to introduce this as a new
function, delayed_decorator_from_middleware, and convert cache_page and
the csrf stuff to use it, to un-break them, without changing the current
behavior of decorator_from_middleware.
This does seem like a relatively nice solution for TemplateResponse and
decorator_from_middleware. Isn't the patch missing a modification to
BaseHandler to do "response = response.render()" rather than just
"response.render()"?
Unfortunately, this doesn't do anything for the problem with cache_page
and Vary headers, which isn't related to TemplateResponse. I'm not sure
we can wait for Django 2000 to find a fix for that.
> One little note while I remember - I'm really not happy about decorator
> functions looking at settings to see how they should work - this is
> fixing fragility by adding more fragility. We want to be getting rid of
> this kind of thing, not adding more of it.
I agree, of course. I only proposed that as a temporary deprecation-path
check, not a permanent behavior.
There was also the general
> assumption of just one view being involved, one response object. We
> don't want to break code that is slightly more inventive, like this:
>
> http://docs.djangoproject.com/en/dev/ref/contrib/csrf/#view-needs-protection-for-one-path
I don't like seeing that recommended in the documentation. For one
thing, it's highly dependent on detailed knowledge of the implementation
of csrf_protect and csrf_exempt. The response is still going to pass
through both of those decorators; a naive reader would be as likely to
assume "last-one-wins" as anything else. And I find it quite hard to
read, compared to just having a function available that takes the
request as argument and performs the CSRF check directly and immediately.
> (Sorry for vague criticisms, I don't have more time to spend on this today).
No problem, critiques appreciated. And - nor do I ;-)
Carl
Luke Plant
> On 05/23/2011 11:31 AM, Luke Plant wrote:
>> Putting all these things together, I've uploaded a new patch to #16004:
>>
>> http://code.djangoproject.com/attachment/ticket/16004/16004.fix.alternative.diff
>>
>> It is not nearly as invasive as the other changes you suggested.
>> However, it does fix the failing tests in my project (i.e. fixes
>> csrf_protect), and leaves TemplateResponse objects unrendered as long as
>> possible, without having to change one line of my own code/tests. And
>> the implementation doesn't make me puke either.
Just re-read this, and realised it might come over as suggesting that
your idea *did* make me puke, which isn't what I was saying at all - I
was saying that this (limited) implementation of what was essentially
your idea turns out quite nicely.
>> I agree that we ought to be doing something about middleware ordering
>> and things like that, but perhaps that is something for Django 2000 -
>> when we can sit down and work out how everything should fit together.
>
> This does seem like a relatively nice solution for TemplateResponse and
> decorator_from_middleware. Isn't the patch missing a modification to
> BaseHandler to do "response = response.render()" rather than just
> "response.render()"?
Yep, good catch.
> Unfortunately, this doesn't do anything for the problem with cache_page
> and Vary headers, which isn't related to TemplateResponse. I'm not sure
> we can wait for Django 2000 to find a fix for that.
OK, but I think it is less pressing. #16004 is significant regression
introduced in 1.3, which is now causing more bugs in trunk due to actual
use of TemplateResponse. #15855 has existed since the dawn of time (0.91
at least, as far as I can tell from eye-balling the code).
I'm happy to wait around for a fuller solution to be thrashed out, but I
worried that the pursuit of an ideal might make it actually more fragile
than it is. I think long term we should move towards a single processing
phase (e.g. decorators always just mark for action later). But to
attempt that in a backwards compatible way requires making functions
that are eager now lazy (in effect). That addition of laziness is very
likely to add more bugs - as the addition of TemplateResponse highlighted.
decorator_from_middleware is broken, but it has been broken for a long
time, and its very likely that a lot is depending on its eager behaviour.
>> There was also the general
>> assumption of just one view being involved, one response object. We
>> don't want to break code that is slightly more inventive, like this:
>>
>> http://docs.djangoproject.com/en/dev/ref/contrib/csrf/#view-needs-protection-for-one-path
>
> I don't like seeing that recommended in the documentation. For one
> thing, it's highly dependent on detailed knowledge of the implementation
> of csrf_protect and csrf_exempt. The response is still going to pass
> through both of those decorators; a naive reader would be as likely to
> assume "last-one-wins" as anything else. And I find it quite hard to
> read, compared to just having a function available that takes the
> request as argument and performs the CSRF check directly and immediately.
It doesn't depend on anything in the implementation, as far as I can
see. csrf_protect protects a view from CSRF attacks. csrf_exempt exempts
a view from the checks the CSRF middleware performs. That is how they
are documented, and that's all you need to know. That section of the
documentation is pointing out a solution that you could deduce simply
from the documentation of the parts.
I guess whether it is obvious depends on what your model is of how these
work. I'd argue that by far the most obvious model is that the
csrf_protect does what it says eagerly i.e. immediately, especially
given that is supposed to work when the middleware is absent.
Carl Meyer
On 05/23/2011 05:14 PM, Luke Plant wrote:
> On 23/05/11 18:33, Carl Meyer wrote:
>>> It is not nearly as invasive as the other changes you suggested.
>>> However, it does fix the failing tests in my project (i.e. fixes
>>> csrf_protect), and leaves TemplateResponse objects unrendered as long as
>>> possible, without having to change one line of my own code/tests. And
>>> the implementation doesn't make me puke either.
>
> Just re-read this, and realised it might come over as suggesting that
> your idea *did* make me puke, which isn't what I was saying at all - I
> was saying that this (limited) implementation of what was essentially
> your idea turns out quite nicely.
I didn't read it that way, but thanks for the clarification ;-)
>> Unfortunately, this doesn't do anything for the problem with cache_page
>> and Vary headers, which isn't related to TemplateResponse. I'm not sure
>> we can wait for Django 2000 to find a fix for that.
>
> OK, but I think it is less pressing. #16004 is significant regression
> introduced in 1.3, which is now causing more bugs in trunk due to actual
> use of TemplateResponse. #15855 has existed since the dawn of time (0.91
> at least, as far as I can tell from eye-balling the code).
>
> I'm happy to wait around for a fuller solution to be thrashed out, but I
> worried that the pursuit of an ideal might make it actually more fragile
> than it is. I think long term we should move towards a single processing
> phase (e.g. decorators always just mark for action later). But to
> attempt that in a backwards compatible way requires making functions
> that are eager now lazy (in effect). That addition of laziness is very
> likely to add more bugs - as the addition of TemplateResponse highlighted.
>
> decorator_from_middleware is broken, but it has been broken for a long
> time, and its very likely that a lot is depending on its eager behaviour.
Yes, fair enough. I think your current patch is the best plan we have
for now, and it wouldn't make sense to hold it; I'll keep noodling on
the longer-term options.
>>> There was also the general
>>> assumption of just one view being involved, one response object. We
>>> don't want to break code that is slightly more inventive, like this:
>>>
>>> http://docs.djangoproject.com/en/dev/ref/contrib/csrf/#view-needs-protection-for-one-path
>>
>> I don't like seeing that recommended in the documentation. For one
>> thing, it's highly dependent on detailed knowledge of the implementation
>> of csrf_protect and csrf_exempt. The response is still going to pass
>> through both of those decorators; a naive reader would be as likely to
>> assume "last-one-wins" as anything else. And I find it quite hard to
>> read, compared to just having a function available that takes the
>> request as argument and performs the CSRF check directly and immediately.
>
> It doesn't depend on anything in the implementation, as far as I can
> see. csrf_protect protects a view from CSRF attacks. csrf_exempt exempts
> a view from the checks the CSRF middleware performs. That is how they
> are documented, and that's all you need to know. That section of the
> documentation is pointing out a solution that you could deduce simply
> from the documentation of the parts.
>
> I guess whether it is obvious depends on what your model is of how these
> work. I'd argue that by far the most obvious model is that the
> csrf_protect does what it says eagerly i.e. immediately, especially
> given that is supposed to work when the middleware is absent.
Yes, you're right here as well. I still think that pattern is ugly and
hard to read, but given the documented behavior of those decorators, its
function is clear.
Carl