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

Unverified login not working on Android #4073

Closed kumar303 closed 9 years ago

kumar303 commented 10 years ago

The Firefox Marketplace uses experimental_allowUnverified=true and experimental_forceIssuer=firefoxos.persona.org. This works fine on Firefox OS but on Android every call to id.watch() results in onlogout().

STR:

You should already be logged in but you're not. Persona is triggering our onlogout handler. Our loggedInUser appears to be correct.

Here are some things I found while debugging:

transition_to_primary seems wrong. Why would we get that for allow unverified and a forced issuer?

callahad commented 10 years ago

Parallel bug in Bugzilla: https://bugzilla.mozilla.org/show_bug.cgi?id=939328

Off the top of your head, do you know if allowUnverified and forceIssuer are working properly in other contexts and/or without that add-on? With dom.identity.enabled set to false, Fennec should be using our polyfill, which is identical for all browsers...

kumar303 commented 10 years ago

We've never really used allowUnverified + forceIssuer on the web polyfill. Up until now we've always done it natively on B2G. Does anyone else use allowUnverified + forceIssuer on a web polyfill?

callahad commented 10 years ago

@kumar303 As far as I know, you guys are the only consumers of those features, and I think we may have been under the impression that they were B2G-only when implementing them. Using them on the web is not recommended and is likely to be pretty buggy. :(

kumar303 commented 10 years ago

OK. This is what I suspected. One of our goals is to ship payments on Android and this is a blocker right now which is why I filed.

callahad commented 10 years ago

Absolutely. Is it possible for you to not use those features on Android? I'm still digging a bit. Will have a nice jsfiddle up soon.

callahad commented 10 years ago

Fiddle: http://jsfiddle.net/5GBNj/

callahad commented 10 years ago

As far as I can tell, we're not taking unverified identities into account when checking against loggedInUser. Thus, Persona thinks the user should be logged out, and onlogout fires automatically.

You can confirm this per the steps below:

Precondition: user is logged in with an unverified email.

  1. Load http://jsfiddle.net/5GBNj/
  2. Click "Configure Polyfill"
  3. Click "Request Email"
  4. Enter an email address + password

Note that the event sequence is:

  1. watch called (undefined)
  2. onlogout called
  3. onready called
  4. onlogin called ({"unverified-email":"foo@example.com"})

Also note that localStorage.siteInfo on the https://login.persona.org/ origin now contains:

{
  "http://fiddle.jshell.net": {
    "email": "foo@example.com",
    "issuer": "firefoxos.persona.org",
    "logged_in": "foo@example.com"
  }
}

and localStorage.emails contains:

{
  "default": {
    "foo@example.com": {
      "created": "2014-01-21T23:19:55.906Z"
    }
  },
  "firefoxos.persona.org": {
    "foo@example.com": {
      "created": "2014-01-21T23:19:55.906Z",
      "updated": "2014-01-21T23:19:56.036Z",
      "pub": { /* ... */ },
      "priv": { /* ... */ },
      "cert": "..."
    }
  }
}

Reproduce behavior

  1. Load http://jsfiddle.net/5GBNj/
  2. Click "Configure Polyfill"

Note that the event sequence is:

  1. watch called (undefined)
  2. onlogout called
  3. onready called

Because onlogout was called automatically, it indicates that Persona does not believe the user should be logged in.

Re-confirm hypothesis

  1. Load http://jsfiddle.net/5GBNj/
  2. Set loggedInUser to null
  3. Click "Configure Polyfill"

Note that the event sequence is:

  1. watch called (undefined)
  2. onmatch called
  3. onready called

Because onmatch was called automatically, Persona agrees that the user is not logged in.

callahad commented 10 years ago

@kumar303 Suggested workaround: Can you ignore onlogout events that fire before onready is called? This would effectively delegate session management to your own session cookie, and take Persona out of that loop.

kumar303 commented 10 years ago

I can't think of an easy way to ignore onlogout because we rely on it to know when to wipe out the previous user's saved payment tokens. Without onlogout we'd have security issues when switching users.

callahad commented 10 years ago

@kumar303 If you absolutely must have forceIssuer, can you drop allowUnverified? That should work with loggedInUser the way you want it to.

callahad commented 10 years ago

Hm... actually, that doesn't seem to work :(

kumar303 commented 10 years ago

we use allowUnverified so that someone can make a payment when first unboxing their phone. This was designed for newly purchased Firefox OS phones and since Android is a similar mobile environment we decided we wanted it there too.

callahad commented 10 years ago

@kumar303 /me nods.

I don't think we'll be able to fix this bug on our end in time for you guys to hit Q1. Moreover, I believe we'd have to change our API to fix it: the forceIssuer and allowUnverified parameters will need to move into .watch() so that they're available at initialization time.

Practically, the way we're going to solve this is to work with what's currently in production, which means ignoring automatic calls to onlogout, at least on Desktop and Android. (Unless @seanmonstar or @fmarier can think of other solutions?) Fortunately, all of the authentication parts of Persona are working correctly, so we just need to shift the session management onto Marketplace instead.

To that end, I'm here to help. How do you handle switching users right now? What are payment tokens, and how are they stored?

callahad commented 10 years ago

(I see the same behavior—Persona wants loggedInUser to be null—with just forceIssuer. Fiddle: http://jsfiddle.net/F9ABG/1/)

kumar303 commented 10 years ago

User switching: Users can log out of the payment screen in two ways:

  1. from the Marketplace (which shares the same domain name) they can log out/in via Persona
  2. when the payment screen asks for a PIN they can choose 'forgot PIN' and eventually they'll be logged out

The case no. 1 is the most common and this is what we rely on onlogout for. When a user logs out of Marketplace, we get the single-signon power of Persona to tell the payment server that they should be logged out. The Marketplace and payment server are two physically separate servers but they share the same domain which is how we achieve the single sign-on feature.

Payment tokens:

When a user authenticates their phone (via SMS challenge, etc) we store a cookie to make repeat purchases fast. We use the onlogout as a cue to delete the cookie. However, this only applies to the user switching case no. 1, from Marketplace.

@andymckay is checking on whether we can drop allowUnverifed on Android which would also allow us to drop forceIssuer (maybe). There is this edge case to worry about: https://bugzilla.mozilla.org/show_bug.cgi?id=910938 (I cc'd you)

kumar303 commented 10 years ago

Oh I just saw the thread of moving to Firefox Accounts. That might solve everything ;)

callahad commented 10 years ago

@kumar303 If FxA is the plan of record, then you're set, but just in case delays or anything else happens, here are some thoughts on a Persona solution:

  1. Ideal: Switch to FxA.
  2. Second Best: Use normal (federated, verified) Persona on Desktop/Android.
  3. Third Best: Ignore automatic login / logout.

Even under # 3, I think you can still get your two scenarios to work correctly. What I'm thinking is along the lines of:

var ready = false;
navigator.id.watch() {
  onready: function () { ready = true; },
  onlogin: function (assertion) { /* unchanged */ },
  onlogout: function () {
    // Abort early if onready hasn't been called.
    if ( !ready ) { return; }

    // Normal onlogout stuff here...
  });
}

Thus, when Persona loads, it'll automatically call its onlogout, onready sequence, but they'll be effectively ignored. The next time you call navigator.id.logout(), say, from a user clicking "Log out" on the Marketplace homepage, onlogout will fire and actually take effect, so you can nuke your cookies and do whatever else.

In this case, you can omit loggedInUser altogether, since you're circumventing its session hints anyway.

Does that make sense / work for you?

callahad commented 10 years ago

(Triaging as 1-star, as Marketplace is planning on switching to FxA instead)

kumar303 commented 10 years ago

Interesting. I didn't know we could rely on onready like this. If it comes to that option, I'll give it a try. Thanks.

callahad commented 10 years ago

You can indeed :) Discourse is actively doing that in production, so it's an OK thing to do.

callahad commented 9 years ago

Hi! To help us better focus, I'm "closing" all issues that have been open for more than six months. These have been tagged "cleanup-2014" so that we can go back and review them in the future.

For more information, check out this thread: http://thread.gmane.org/gmane.comp.mozilla.identity.devel/7394

If you believe this bug is still a major issue for you, please comment, submit a pull request, or discuss it on our mailing list: https://lists.mozilla.org/listinfo/dev-identity

Sorry for GitHub notification spam!