mozilla / django-browserid

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

Add getAssertion and verifyAssertion to JavaScript API. #177

Closed Osmose closed 11 years ago

Osmose commented 11 years ago

On Flicks, we ran into an issue where we wanted to perform an action that required a POST and a csrf token and prompt the user for login first if they aren't logged in. The problem is that if we attempt to log the user in prior to performing the action, django-browserid will redirect the user to a new page. If that new page automatically does the action, it will be vulnerable to a CSRF attack.

The alternative was to perform the action, store the intent in the user's session, and then prompt them for login, and perform the action post-login if the intent is found in their session. The issue with this is that the action requires an AJAX request, and calling the login code from within the AJAX handler causes popup warnings because navigator.id.request is being called outside of a click handler.

The next option was to use navigator.id.get to get an assertion, and then perform the action, and lastly submit the assertion for verification once the action was logged in their session. This would've worked, except include.js doesn't allow you to use navigator.id.get at the same time as navigator.id.watch.

Thus, this patch. It emulates the functionality of navigator.id.get using the watch API, and also adds a function to trigger the verification form submission in case anyone wants to trigger login from within the callback.

Long term I think we should change verifyAssertion to use an iframe so that we can finally support verification and login without leaving the page, but this should work in the short term and let me get on with my Flicks work. :D

peterbe commented 11 years ago

I'm not convinced. There's got to be a better way to add this important functionality.

My beef is that django_browserid.getAssertion() does almost the exact thing as django_browserid.login but with a weird name. To begin with, instead of calling it getAssertion perhaps it should be loginWithCallback instead to fit in better.

Could we instead just do something like this:

django_browserid.login(function(assertion) {
    ...
})

then, in django_browserid.login we assign a callable loginRedirect to use that isFunction to call the loginRedirect instead. Or, in fact, that's not a good choice of name either. Perhaps we can just call it next. If it's a callable function, we call it, if it's a string we use it as argument to redirect. E.g.

var next = null;  // for redirecting or callback after login and logout
...
function: login(_next, requestArgs) {
    next = _next; // set the global variable for use with the navigator API later
}
peterbe commented 11 years ago

Why isn't verifyAssertion just a private function? It looks like the point of it is to act as a default "callback" that force-submits the log in form if there isn't a callback to manually handle this.

Or? Am I just not seeing the bigger picture?

Osmose commented 11 years ago

To begin with, instead of calling it getAssertion perhaps it should be loginWithCallback instead to fit in better.

IMO getAssertion describes what it does better; it literally gets you an assertion from Persona. Note that this is also structured similarly to [navigator.id.get](https://developer.mozilla.org/en-US/docs/Web/API/navigator.id.get) from the Persona API as it's meant to be a version of that that works with the Watch API since we can't use that method natively.

Could we instead just do something like this:

We could, and it might be a bit cleaner on our side in terms of code, but I'm not a fan of using that as an API. My idea is that login doesn't mean "Log into Persona", it means "Log the user into my site, a process which may or may not require them to log into Persona", whereas getAssertion means "Get an assertion from Persona for this user, which may require them to log into Persona". Both steps may not require them to log into Persona if they've already authed with Persona and don't have multiple emails.

One thing that we could do that makes sense is to have login use getAssertion internally to reduce code duplication, and if you agree with my argument about the API then I'd be happy to make that change.

Why isn't verifyAssertion just a private function?

Because external code (such as Flicks) may want to use it. Right now it's actually not doing exactly what I want it to, but it'd take too much time to make it work just the right way and I need this to be working for Flicks now.

What Flicks will do is get an assertion from Persona, submit an AJAX request to vote on a video, and then verify the assertion it retrieved and log the user in. That last step requires a method like verifyAssertion that lets use continue the login process when we have an assertion.

Long-term I think verifyAssertion won't submit the form but instead will make an AJAX request so that devs can make sites that log a user in without leaving the page, but that's down the road. For now I think it's a decent fit to solve a problem that requires help from django-browserid. :D

Does that make sense?

peterbe commented 11 years ago

One thing that we could do that makes sense is to have login use getAssertion internally to reduce code duplication, and if you agree with my argument about the API then I'd be happy to make that change.

Yes I think that would be a good idea.

Generally, I feel like I can't see this clearly. Why can't getAssertion and login "merge"? The only difference is that one of them preps for a redirect later, the other for a callback. That seems sufficiently similar that this could all go under one name.

I feel like I'm nitpicking. If you think this change is going to work for Flicks then I'm perfectly OK with it. I guess I'm just the necessary evil inertia to make you reconsider it one last time if it can be improved even more.

Go forth!

fmarier commented 11 years ago

The next option was to use navigator.id.get to get an assertion, and then perform the action, and lastly submit the assertion for verification once the action was logged in their session. This would've worked, except include.js doesn't allow you to use navigator.id.get at the same time as navigator.id.watch.

That sounds like a bug in the Persona API to me. We've discussed it before, but given that you're running into this too, we should really fix it.

Osmose commented 11 years ago

The only difference is that one of them preps for a redirect later, the other for a callback.

Actually that's not the only difference. login does the redirect, but it does it after logging the user into your site. That's the key difference; getAssertion does not log the user into your site, it only deals with Persona. It's use is in complex situations like Flicks that our "Submit the assertion via a form and log the user in, then redirect to a URL" doesn't fit.

If we add the callback functionality to login, we're saying that a function called login doesn't always log you in, which doesn't make sense. We could rename it, but I think that the functionality of logging in and then redirecting is unique enough to deserve a separate function.

Given time constraints and the fact that you don't have any problems with the code itself I'll merge, but the greater discussion of "What should our JavaScript API look like" is an important one that I think we should continue. :D

That sounds like a bug in the Persona API to me. We've discussed it before, but given that you're running into this too, we should really fix it.

Yes! :D

Osmose commented 11 years ago

PS +162 Saronite prutot to @peterbe for being a great conscience. <3

fmarier commented 11 years ago

We've opened https://github.com/mozilla/browserid/issues/3637 to track this bug in the Persona API.

coveralls commented 10 years ago

Coverage Status

Changes Unknown when pulling f1ab8018e4197a21f9262aa041d0f537a3c55cb7 on Osmose:js-add-get-assertion into \ on mozilla:master**.