mozilla / persona

Persona is a secure, distributed, and easy to use identification system.
https://login.persona.org
Other
1.83k stars 264 forks source link

[b2g] navigator.id.logout() should always call the onLogout handler. #2997

Closed wraithan closed 11 years ago

wraithan commented 11 years ago

Currently if you are logged in, you can call navigator.id.logout() an infinite number of times and it will always call onLogout. If you aren't logged in at page load time then hitting navigator.id.logout() does not call onLogout.

What we'd like for webpay is to always call onLogout regardless of the logged in status of the user.

Here are some bugs being caused by this: https://bugzilla.mozilla.org/show_bug.cgi?id=838812 https://bugzilla.mozilla.org/show_bug.cgi?id=836860

seanmonstar commented 11 years ago

The onlogout handler purposefully only triggers if the state goes from logged in to logged out. If the user isn't logged in originally, then it's not true that there has been an onlogout event.

What's the problem you're trying to solve? Is it not possible to know if the user was logged in previously in some specific way?

wraithan commented 11 years ago

It actually doesn't though, since it will trigger an infinite number of times once a person is logged in. The only case when onlogout isn't triggered is when the user was never logged in.

We need to definitely clear the user's cookies then redirect them. We purposefully do the redirect in the onlogout handler so we know that their cookies have been removed. AFAIK browserid doesn't give us a way to query if the user is logged in or not. We considered doing something janky like setting a global variable in onlogin then checking for it when our logout button is clicked, but thought we'd raise the issue to browserid.

Currently onlogin always works regardless of the number of times you call navigator.id.request() I'd argue the onlogout should work the same way.

callahad commented 11 years ago

Setting to 4 stars for needing review / consideration.

@seanmonstar and @jedp -- can you take a look at this? Added [b2g] to title since this impacts marketplace, but please also consider if it would make sense to change our spec / implementation in general.

jedp commented 11 years ago

It makes sense to me, and I don't think it's marketplace- or b2g-specific in the end. It seems to me that if an RP assigns an onlogout handler, the RP can reasonably expect that if there's a logout button exposed, and the user clicks it, it should call the handler.

seanmonstar commented 11 years ago

This isn't b2g-specific.

I'll be the contrarian: The onlogin should only be triggered if the user completes the login process from nav.id.request. If they don't, such as canceling and closing the dialog, it should not call the onlogin process. (If it does, that's a separate bug.)

Likewise, onlogout should only be called if a user was actually logged out.

@wraithan The root of the problem is not having an easy way of knowing if the user is logged in?

callahad commented 11 years ago

This needs to make it into MDN, but here's how the API works.

You tell Persona what you believe about the user's state by including a loggedInUser parameter in your call to id.watch. Persona will always believe that the user wants to be logged in as someone, or wants to be logged out. Persona will then invoke the appropriate callback to harmonize that state.

You BelievePersona BelievesCallback
foo@example.comfoo@example.comonmatch
nullfoo@example.comonlogin
undefinedfoo@example.comonlogin
foo@example.comnullonlogout
nullnullonmatch
undefinednullonlogout

Caveats:

  1. The onmatch callback doesn't actually exist, yet. So instead you'd get nothing in those cases.
  2. If both you and Persona think someone is logged in, but disagree on who, then you'll get an onlogin callback for the user that Persona believes wants to be logged in.
wraithan commented 11 years ago

The disagree case where they both believe someone is logged in should be in the table, not listed as a caveat.

The onmatch not existing means it should be documented as doing nothing otherwise you confuse users.

seanmonstar commented 11 years ago

@callahad :+1: * 1000

callahad commented 11 years ago

MDN (https://developer.mozilla.org/en-US/docs/DOM/navigator.id.watch) has been updated -- closing this bug. @wraithan, let me know if you guys run into any more issues related to this.