mozilla / django-browserid

Django application for adding BrowserID support.
Mozilla Public License 2.0
179 stars 80 forks source link

CSRF token fetching does not work with Django 1.8 #282

Closed abompard closed 9 years ago

abompard commented 9 years ago

The newly released Django 1.8 changed the way context processors are run in the RequestContext object. They were previously run on instanciation, they are now run when the RequestContext is bound to a template. As a result, the CsrfToken view fails to get the token and login does not work. I see 3 ways of solving this problem, I have implemented all of them and all tests pass:

I have tested all three options on my local python2.7-django1.8 install, and they all work. I would personally go with the third, but it's up to you in the end. I'll create a pull request for the one you choose.

Osmose commented 9 years ago

First, thanks for the report / fixes!

The reason why we didn't do the second or third option before is that a lot of the sites using django-browserid (mainly the Mozilla ones since they're all on *.mozilla.org) also use django-session-csrf. Since django-session-csrf stores the token in the session, we can't rely on the token being in the cookie.

This first option seems doable, although the issue with multiple template engines is an annoying one. I'm not super-excited about it, but could we alter the current view to bind a dummy Template instance before fetching the value? That way we could just generate an empty template instance from whatever the default template engine is and avoid worrying about which engine they're using.

In a semi-related note, does 1.8's swappable engines mean our current templates might be parsed by the wrong engine? I did some searching but couldn't find any useful info on how libraries specify what template engine they want to use.

Osmose commented 9 years ago

After some discussion with @carljm it seems out best bet for the latter question will be to document how to add an extra template engine definition specifically for django-browserid if you're using non-DTL templates on your site.

I filed #283 for that specific issue. In the case of this issue I think a dummy template from the default template engine would be better.

abompard commented 9 years ago

Gotcha, that's what the comment at the top of the method meant :-) OK, I've committed a change to bind to a dummy template instead of rendering it, and submitted the pull request #284. Unfortunately I have to use a try-except block for Django 1.8+, I didn't see a way that would be API-compatible with Django 1.4 to 1.8. I tested it on my local install with Django 1.6 to 1.8, but all tests pass in tox and they include Django 1.4 too.

Osmose commented 9 years ago

This was fixed by #284.