Verify.success_url never used with {% browserid_login %}

[alanbriolat]

The expectation set by the documentation is that you should be able to override Verify.success_url in your own view (used with BROWSERID_VERIFY_CLASS) with some complex behaviour and control where a successful login redirects to. After finding the method was never called, I dug into the django_browserid code and found the following:

• The next POST parameter always takes precedence in Verify.login_success().
• The {% browserid_login %} template tag always results in a next, whether you supply one or not, defaulting to LOGIN_REDIRECT_URL.
• Therefore Verify.success_url is rendered inert, with LOGIN_REDIRECT_URL being the only real point of control.
• (I also couldn't help but notice that the code is obsessed with using / as a default value when getting LOGIN_REDIRECT_URL, even though Django already has a default of /accounts/profile/ and therefore such "defensiveness" is unnecessary.)

As far as I can tell, the correct behaviour would be to not assume a default next parameter, instead leaving it up to Verify.success_url to decide in the absence of one. Of course, I could be completely breaking some other workflow with that assumption, so I'd appreciate any input before committing time to a fix.

[bobsilverberg]

@alanbriolat I have been able to successfully override Verify.success_url [1] in a project I have which uses django-browserid and browserid_login [2]. Perhaps my case is different from yours, but maybe looking at my code could help.

[1] https://github.com/mozilla/oneanddone/blob/master/oneanddone/users/views.py#L158
[2] https://github.com/mozilla/oneanddone/blob/master/oneanddone/base/jinja2/base/base.html#L78

[alanbriolat]

@bobsilverberg I found that hard to believe, having read the corresponding logic in django_browserid, so I actually went to the effort of running mozilla/oneanddone locally to investigate.

Setting a breakpoint in your Verify.success_url, it definitely is never called on login, and for the exact reason I specified: the browserid_login generated button has data-next="/" in it.

In fact, the reason you get the behaviour you expect is because the logic in your Verify.success_url is duplicated in oneanddone.base.views.HomeView.dispatch(...). The access pattern in the server log confirms this:

[15/Dec/2015 04:45:33] "GET /browserid/csrf/ HTTP/1.1" 200 32
[15/Dec/2015 04:45:34] "POST /browserid/login/ HTTP/1.1" 200 52
[15/Dec/2015 04:45:34] "GET / HTTP/1.1" 302 0
[15/Dec/2015 04:45:35] "GET /profile/new/ HTTP/1.1" 200 8381

Note the 302 redirect to /profile/new/ after hitting /. The response from /browserid/login/ is {"redirect": "/", "email": "<redacted>"}.

[Osmose]

Thanks for the report!

I think you're right about success_url never being used in practices with the {% browserid_login %} tag. I'd probably accept a fix that avoided adding a next parameter when one wasn't specified, so that the view will fall back to success_url. You sounded like you were willing to work on one?

I imagine this will constitute a patch release given that the code is documented to work this way and just isn't, making this a backwards-compatible bugfix.

[alanbriolat]

I'm not so sure about it being a backwards-compatible fix. The browserid_login template tag is definitely documented as falling back to LOGIN_REDIRECT_URL for the next parameter, and the tests explicitly confirm this. The default behaviour would remain the same, because Verify.success_url defaults to LOGIN_REDIRECT_URL. However, any custom JS that (mistakenly?) relies on the data-next attribute of the button, instead of the returned redirect URL, would encounter new behaviour.

I'm work on a fix, yes. For consistency it seems the duplicate behaviour should also be removed from logout as well as login.

[Osmose]

Fixed by #297. I'll look into cutting a release on Monday and figure out if I should do a major version change or not. Thanks again!

[Osmose]

Aaaaaaaand it's out! https://github.com/mozilla/django-browserid/releases/tag/v2.0.0