Closed alanbriolat closed 8 years ago
@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
@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>"}
.
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.
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.
Fixed by https://github.com/mozilla/django-browserid/pull/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!
Aaaaaaaand it's out! https://github.com/mozilla/django-browserid/releases/tag/v2.0.0
The expectation set by the documentation is that you should be able to override
Verify.success_url
in your own view (used withBROWSERID_VERIFY_CLASS
) with some complex behaviour and control where a successful login redirects to. After finding the method was never called, I dug into thedjango_browserid
code and found the following:next
POST parameter always takes precedence inVerify.login_success()
.{% browserid_login %}
template tag always results in anext
, whether you supply one or not, defaulting toLOGIN_REDIRECT_URL
.Verify.success_url
is rendered inert, withLOGIN_REDIRECT_URL
being the only real point of control./
as a default value when gettingLOGIN_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 toVerify.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.