945363 - Ensure FXA callbacks fire in correct sequence

Status: RESOLVED FIXED

(Core Graveyard :: Identity, defect)

Description

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

We may need to remove 'logout' from the list of required callbacks to watch.

Comment 1

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #0)
> We may need to remove 'logout' from the list of required callbacks to watch.

How will RPs be notified about a logout if there is no 'logout' callback in watch?

Comment 2

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Fernando, I agree.  When we were contemplating this, we were desirous of keeping the fxa api in line with the browserid api used by persona.  The persona team, in response to desires by RPs, had decided to remove login state management from persona, and so remove onlogout().

In our case, though, I don't think this is possible.  Like you, I cannot see how we can create an effective SSO system in fxa if we don't have logout notifications.

I think it is also fair to say that clients who get to use firefox accounts should be expected to step up to a different set of rules from RPs who simply want sign-in with persona.  To participate in fxa will require a stronger state-management contract with apps.

Comment 3

[Sam Penrose]

Jed, can you correct my thinking about what we need here:

1) RPs with wantIssuer: firefox-accounts either must have onlogout or will break without it.

2) The system app, and only the system app, can send a "logout of FxA" message to dom/identity (probably nsDomIdentity, but ?).

3) Whoever receives the message will iterate over the RP structures to fire onlogouts, and send a message down to services/fxaccounts .

Comment 4

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Sam Penrose from comment #3)
> Jed, can you correct my thinking about what we need here:
> 
> 1) RPs with wantIssuer: firefox-accounts either must have onlogout or will
> break without it.

We need onlogout because, unlike with the "goldilocks" persona contract and api, we *are* responsible for login state management for apps.  We need a way to tell them when credentials have expired or been revoked and the user is no longer signed in.

So I think part of the contract for playing with fxa must be to have onlogout.

> 2) The system app, and only the system app, can send a "logout of FxA"
> message to dom/identity (probably nsDomIdentity, but ?).

I think either the system app or any certified app (which are basically extensions of the system) could send the logout message.  For instance, if the user failed to enter the right PIN code into marketplace some number of times, the marketplace app could say, "Hey, I don't think this device is currently in the hands of the user who signed in."  It could tell fxa to sign the user out.  This would broadcast the fxa logout message to all RPs.  (The Wheres-My-Fox app would have special behavior in this case, preventing sign-out without a re-auth, because this is the kind of situation where you want your WMF working.)

(strictly speaking, the message would land in the lap of DOMIdentity, which would then repeat it via async message to each nsDOMIdentity listener.  There's one nsDOMIdentity instance per RP (per "window"), but only one DOMIdentity.

> 3) Whoever receives the message will iterate over the RP structures to fire
> onlogouts, and send a message down to services/fxaccounts .

Yes.

Comment 5

[Sam Penrose]

(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #4)

Awesome, thanks for working through this:

> (In reply to Sam Penrose from comment #3)
> > Jed, can you correct my thinking about what we need here:
> > 
> > 1) RPs with wantIssuer: firefox-accounts either must have onlogout or will
> > break without it.
> 
> We need onlogout because, unlike with the "goldilocks" persona contract and
> api, we *are* responsible for login state management for apps.  We need a
> way to tell them when credentials have expired or been revoked and the user
> is no longer signed in.
> 
> So I think part of the contract for playing with fxa must be to have
> onlogout.

Cool. So we have to decide how to implement this requirement. I want to be very careful not to create additional work for the 1.4 deadline. Maybe simply follow an existing failure mode and log.error('mozId API call including wantIssuer must also include onlogout handler').

> 
> > 2) The system app, and only the system app, can send a "logout of FxA"
> > message to dom/identity (probably nsDomIdentity, but ?).
> 
> I think either the system app or any certified app (which are basically
> extensions of the system) could send the logout message.  For instance, if
> the user failed to enter the right PIN code into marketplace some number of
> times, the marketplace app could say, "Hey, I don't think this device is
> currently in the hands of the user who signed in."  It could tell fxa to
> sign the user out.  This would broadcast the fxa logout message to all RPs. 
> (The Wheres-My-Fox app would have special behavior in this case, preventing
> sign-out without a re-auth, because this is the kind of situation where you
> want your WMF working.)
> 
> (strictly speaking, the message would land in the lap of DOMIdentity, which
> would then repeat it via async message to each nsDOMIdentity listener. 
> There's one nsDOMIdentity instance per RP (per "window"), but only one
> DOMIdentity.

For 1.4 logout from an RP is not a deliverable, so let's table that. The supported path will be:

  Settings Logout UI ---[mechanism]-->DOMIdentity.js, which iterates over this._serviceContexts and calls RP.logout() on them.

> 
> > 3) Whoever receives the message will iterate over the RP structures to fire
> > onlogouts, and send a message down to services/fxaccounts .
> 
> Yes.

Comment 6

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Ensure RPs provide correct callbacks to watch() (WIP)

Confirms that RPs (both BrowserID and Firefox Accounts) are providing the expected callbacks to watch.

The tests are a little long-winded - I'd like to simplify the generation of the iframes (have them created by an sjs, for example).

Sam, for the purposes of this bug, do you think this approach is sufficient?  It will throw an exception if an RP doesn't provide the callbacks we expect.

Comment 7

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

spenrose - sorry - this is only a partial solution - it still needs the 'sign out everywhere' feature.  Like I said, WIP :)

Comment 8

[Sam Penrose]

This patch fulfills the first change, good stuff.

(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #6)
> Created attachment 8376622 [details] [diff] [review]
> Ensure RPs provide correct callbacks to watch() (WIP)
> 
> Confirms that RPs (both BrowserID and Firefox Accounts) are providing the
> expected callbacks to watch.
> 
> The tests are a little long-winded - I'd like to simplify the generation of
> the iframes (have them created by an sjs, for example).
> 
> Sam, for the purposes of this bug, do you think this approach is sufficient?
> It will throw an exception if an RP doesn't provide the callbacks we expect.

Comment 9

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

I ought to be able to put all the mochitests into one file, calling the privileged _unwatch between tests to reset the state of the rpWatcher for that window.

Comment 10

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(Note to self: Make sure we have a test to prove that if you call watch() and the signedInUser is null, onlogout() is fired.)

Comment 11

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: 945363-ensure-onlogout-handler.patch

Rebased over Bug 963835 and Bug 972582

Comment 12

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: FxA RPs provide required callbacks; callbacks fire in proper sequence

Sam, would you take a look at what I've done in toolkit/identity/tests/unit/test_firefox_accounts.js and make sure it's what we want for the behavior of watch()?

In this patch, watch() will first either call login(assertion) or logout(), depending on whether there's already a signed-in user, and then call ready().

I think this is the required behavior for apps that are being woken up or restarted.  They need to act according to the current login state.

I hope I've got those in the right order.  (login/out then ready, not the other way around.)  Doing it this way takes care of state management before signaling to the app that it may now take over.

(If this is correct, it will have a teensy effect on Bug 947374, because it will be necessary for certified and packaged apps to specify assertion audience when calling watch(), not request().  I've made a comment on that bug accordingly.)

Thanks!

Comment 13

[Sam Penrose]

The logic sounds exactly right to me, and test_firefox_accounts.js seems to express it. I can read the rest of the patch and/or play with it if you want me to take a deeper dive, otherwise we're f+ .

Comment 14

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Great.  Thanks, for the feedback, Sam.  No further help needed.

Comment 15

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Comment on attachment 8378687 [details] [diff] [review]
FxA RPs provide required callbacks; callbacks fire in proper sequence

Fernando, how does this approach look to you?

I think it should solve the SSO issues for RPs using the DOM API, provided they call watch() whenever they start or are re-started by the OS.  Hopefully this will be a good thing if forceAuth is used.

What do you think?

Comment 16

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8378687 [details] [diff] [review]
FxA RPs provide required callbacks; callbacks fire in proper sequence

Review of attachment 8378687 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/identity/FirefoxAccounts.jsm
@@ +89,5 @@
>      this._rpFlows.set(aRpCaller.id, aRpCaller);
>      log.debug("Current rp flows: " + this._rpFlows.size);
>  
> +    // If the user is already signed in, trigger onlogin with an assertion.  If
> +    // not, trigger onlogout.  After either case, follow up with onready.

Sorry, I don't really understand this logic. Can you elaborate more on this, please?

@@ +96,5 @@
> +        this.fxAccountsManager.getAssertion(aRpCaller.audience).then(
> +          data => {
> +            if (data) {
> +              this.doLogin(aRpCaller.id, data);
> +            } else {

IIRC this case won't ever happen.

FxAccountsManager.getAssertion() doesn't fulfill the promise with no data.

Comment 17

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #16)
> Comment on attachment 8378687 [details] [diff] [review]
> FxA RPs provide required callbacks; callbacks fire in proper sequence

Thanks for taking a look, Fernando!

> Review of attachment 8378687 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/identity/FirefoxAccounts.jsm
> @@ +89,5 @@
> >      this._rpFlows.set(aRpCaller.id, aRpCaller);
> >      log.debug("Current rp flows: " + this._rpFlows.size);
> >  
> > +    // If the user is already signed in, trigger onlogin with an assertion.  If
> > +    // not, trigger onlogout.  After either case, follow up with onready.
> 
> Sorry, I don't really understand this logic. Can you elaborate more on this,
> please?

The idea is that we handle signed-in state management first (onlogin or onlogout), and then signal to the RP that we are done and it can take over (onready).

If we call onready first, and follow with onlogout or onlogin, the RP won't yet know what state it's in, and it won't know whether to wait for onlogin or onlogout to fire.  So I think onlogin or onlogout must fire first, and then onready as a signal that all state management on our side is done and the RP can carry on.

For example, say someone has signed into Marketplace.  At some later time, the Marketplace app is closed and the user also signs out of FXA.  Then the Marketplace app is opened again.  When opened, it calls watch() with its callbacks.  We respond by calling onlogout() because there is no signed-in user; then we call onready().  With this sequence of events, Marketplace has a chance to update its state before being told to run (i.e., upon receiving onready).  If it received onready() first, it would not know that the user had signed out.

Have I got that right?

> @@ +96,5 @@
> > +        this.fxAccountsManager.getAssertion(aRpCaller.audience).then(
> > +          data => {
> > +            if (data) {
> > +              this.doLogin(aRpCaller.id, data);
> > +            } else {
> 
> IIRC this case won't ever happen.
> 
> FxAccountsManager.getAssertion() doesn't fulfill the promise with no data.

Oh nice catch.  Thank you.  Yes, FxAccounts.jsm returns null in this case.  I will fix this.

Thanks again.  Does the explanation above make sense?  Or am I thinking about this wrong?

Comment 18

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #17)
> (In reply to Fernando Jiménez Moreno [:ferjm] from comment #16)
> > Comment on attachment 8378687 [details] [diff] [review]
> > FxA RPs provide required callbacks; callbacks fire in proper sequence
> 
> Thanks for taking a look, Fernando!
> 
> > Review of attachment 8378687 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: toolkit/identity/FirefoxAccounts.jsm
> > @@ +89,5 @@
> > >      this._rpFlows.set(aRpCaller.id, aRpCaller);
> > >      log.debug("Current rp flows: " + this._rpFlows.size);
> > >  
> > > +    // If the user is already signed in, trigger onlogin with an assertion.  If
> > > +    // not, trigger onlogout.  After either case, follow up with onready.
> > 
> > Sorry, I don't really understand this logic. Can you elaborate more on this,
> > please?
> 
> The idea is that we handle signed-in state management first (onlogin or
> onlogout), and then signal to the RP that we are done and it can take over
> (onready).
> 
> If we call onready first, and follow with onlogout or onlogin, the RP won't
> yet know what state it's in, and it won't know whether to wait for onlogin
> or onlogout to fire.  So I think onlogin or onlogout must fire first, and
> then onready as a signal that all state management on our side is done and
> the RP can carry on.
> 
> For example, say someone has signed into Marketplace.  At some later time,
> the Marketplace app is closed and the user also signs out of FXA.  Then the
> Marketplace app is opened again.  When opened, it calls watch() with its
> callbacks.  We respond by calling onlogout() because there is no signed-in
> user; then we call onready().  With this sequence of events, Marketplace has
> a chance to update its state before being told to run (i.e., upon receiving
> onready).  If it received onready() first, it would not know that the user
> had signed out.
> 
> Have I got that right?

Yes, thanks for the detailed explanation :). This feels a bit strange though :\. If I understood it properly, we are forcing the RP to make distinctions if the onlogout/onlogin event has been fired before or after the onready one. The RP might want to show some UI when it receives onlogout/onlogin events.

Can we pass a 'loginState' like parameter to the onready callback instead? IMHO we should not fire onlogout/onlogin before onready.

Comment 19

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Part of my answer is simply an appeal to the spec.  I've double-checked with :callahad and :fmarier in IRC, and indeed, if we're going to fire login or logout in BrowserID, we do it before we fire onready.  (https://developer.mozilla.org/en-US/docs/Web/API/navigator.id.watch)

But specifically to your point, I don't think this would restrict the RP's actions in any way.  If it wanted to show some UI on login/logout, it could make a note and then do so when it received onready.

When you say, "we are forcing the RP to make distinctions if the onlogout/onlogin event has been fired before or after the onready one", are you referring to a scenario such as the following: The user is initially not signed in when we call watch() (so fire onlogout then onready); then request() is invoked; then the user is logged in (fire onlogin but but do not fire onready)?  If so, I still feel that's ok.  It's not that the RP has to decide what to do based on whether onlogin fired before or after onready; it's simply that the RP shouldn't take any action until after onready has been fired, which will happen exactly once.  It's the DOMContentLoaded event of BrowserID :)

One question that it does raise, however, is whether we can have an onready event in firefox accounts without a corresponding onlogin or onlogout being fired.  In BrowserID/Persona, if there is no known loggedInUser state, we just fire onready when there's no state to match against.  In our current one-handset-one-user case with FirefoxOS, I can't see how this third case would ever occur: The rightful owner is either logged in or she is logged out.  If that analysis is correct, I don't think it's a bad thing, just noteworthy.

Comment 20

[Fernando Jiménez Moreno [:ferjm]]

I might be missing some information. When a user requests a sign out (via Settings for example) are we firing an onlogout event over RPs that are opened at that point? I think we do. For example:

1. There is a logged in account in the device.
2. The user opens the Marketplace.
3. Marketplace uses the logged in account.
4. The user closes Marketplace and opens Settings (note that Marketplace keeps running and so can receive events).
5. The user logs out from the device via Settings.
6. Marketplace should receive the onlogout event.

If that is the case, my point is that we are overloading onlogout (and onlogin) with two different meanings:

1. Before 'onready': To let the RP know about the previous login state of the account in the device.
2. After 'onready': To notify the RP about a logout/login event and so about a change in the current account state (which seems the correct use for it)

Let's say that a RP shows a screen like: "You are now logged out. Come back soon!" when the user logs out from the device. With the current approach, the RP would need to check if the 'onlogout' event has been received before (it shouldn't show the "You are now logged out..." UI cause the event is only informing about the current state) or after (it should show the UI cause the event is informing about a change in the current state) the 'onready' callback.

I am not saying that it is incorrect, but it just feels to me like an strange usage of the API. IMHO it would be easier to just provide the current status of the account within the 'onready' callback. But even if I think that this API is not the best approach, I can understand that you want to keep it as much aligned as possible with the current Persona API.

In any case, the WebAPI team should be aware of any API changes.

Comment 21

[Sam Penrose]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #20)
> I might be missing some information. When a user requests a sign out (via
> Settings for example) are we firing an onlogout event over RPs that are
> opened at that point? I think we do. For example:
> 
> 1. There is a logged in account in the device.
> 2. The user opens the Marketplace.
> 3. Marketplace uses the logged in account.
> 4. The user closes Marketplace and opens Settings (note that Marketplace
> keeps running and so can receive events).

I believe our primary concern in this thread has been that we cannot assume Marketplace will keep running. Certainly ggp has been concerned about that case for WMF, and it seems generally true of possible future RPs.

> 5. The user logs out from the device via Settings.
> 6. Marketplace should receive the onlogout event.
> 
> If that is the case, my point is that we are overloading onlogout (and
> onlogin) with two different meanings:
> 
> 1. Before 'onready': To let the RP know about the previous login state of
> the account in the device.

If I understand Jed correctly, s/previous/current .

> 2. After 'onready': To notify the RP about a logout/login event and so about
> a change in the current account state (which seems the correct use for it)
> 
> Let's say that a RP shows a screen like: "You are now logged out. Come back
> soon!" when the user logs out from the device. With the current approach,
> the RP would need to check if the 'onlogout' event has been received before
> (it shouldn't show the "You are now logged out..." UI cause the event is
> only informing about the current state) or after (it should show the UI
> cause the event is informing about a change in the current state) the
> 'onready' callback.

I agree that there are potential UX issues here. But:

1) FxA is the lowest level of login state. RPs will definitely have to managed session state above FxA.
2) Hard UX cases are for UX and product people to own.

> I am not saying that it is incorrect, but it just feels to me like an
> strange usage of the API. IMHO it would be easier to just provide the
> current status of the account within the 'onready' callback. But even if I
> think that this API is not the best approach, I can understand that you want
> to keep it as much aligned as possible with the current Persona API.
> 
> In any case, the WebAPI team should be aware of any API changes.

I move that we bless Jed's solution. We need to make "good enough" decisions quickly.

Comment 22

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #20)

Hi, Fernando,

Thanks for all your comments on this question.  My arguments so far have understandably failed to impress you, as they are purely pragmatic in nature, appealing to the authority of the existing spec, and seeking to have no change in behavior for existing RPs.  Let me try to expand more.

> If that is the case, my point is that we are overloading onlogout (and
> onlogin) with two different meanings:

I think this is the crux of your argument, and it's a good point.  Presently, yes, the semantics of onlogin and onlogout depends on whether or not the RP is waiting for onready, and this is weird for the reasons you describe.  I thought your suggestion in Comment 18 was a good one for a revision of the API, namely that onready could take a loginState argument (or possibly an assertion like you would get with onlogin).
 
I don't think, though, that passing login state to ready() would necessarily be without complication.  You would, for instance, have to handle login and logout events in not one but two callbacks, and this in turn could lead to duplication of code and additional state-management complexity.

Furthermore, as it stands now, an RP that wanted to show a different UI for each of the two modes of logout could simply trigger one from a click handler on its own logout button, reserving onlogout()'s functionality purely for recording state, not triggering UI.  Done this way, I think the problem goes away.

> I am not saying that it is incorrect, but it just feels to me like an
> strange usage of the API. IMHO it would be easier to just provide the
> current status of the account within the 'onready' callback. But even if I
> think that this API is not the best approach, I can understand that you want
> to keep it as much aligned as possible with the current Persona API.

That's precisely the bind I'm in right now.  The BrowserID API was designed and deployed (for Persona) with the dual use of onlogin and onlogout, and Persona implements it this way.  One of the assurances we've given to Marketplace and WIMF is that the RP API for Firefox Accounts will be the same as Persona's.

Overall, I agree that, yes, the API is not perfect.  But at the same time, it is already in production with this behavior.  Additionally, despite the infelicities, I don't think there are any problems that cannot be solved with the existing API.  It may not be perfect, but I think it's good enough and that we should stick with it.

> In any case, the WebAPI team should be aware of any API changes.

True, though this patch does not change anything; it conforms to BrowserID.

Comment 23

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #22)
> 
> That's precisely the bind I'm in right now.  The BrowserID API was designed
> and deployed (for Persona) with the dual use of onlogin and onlogout, and
> Persona implements it this way.  One of the assurances we've given to
> Marketplace and WIMF is that the RP API for Firefox Accounts will be the
> same as Persona's.
> 
> Overall, I agree that, yes, the API is not perfect.  But at the same time,
> it is already in production with this behavior.  Additionally, despite the
> infelicities, I don't think there are any problems that cannot be solved
> with the existing API.  It may not be perfect, but I think it's good enough
> and that we should stick with it.

Fair enough. Thanks for the explanation Jed.

Comment 24

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: FxA RPs provide required callbacks; callbacks fire in proper sequence

Rebased over current m-i and Bug 978896

At this point, this patch largely provides mochitest and xpcshell cases for existing logic.

Comment 25

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Comment on attachment 8391013 [details] [diff] [review]
FxA RPs provide required callbacks; callbacks fire in proper sequence

Hi, Fernando,

This patch primarily consists of tests at this point.

There is a refactoring of the code that makes sure the right callbacks are defined, and a sanity-check that the callbacks are actually functions.

Perhaps the most useful part of it is the four lines in unwatch() that make sure we don't leak memory!

Thanks for your review.  Cheers!
j

Comment 26

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

I'm changing the title of this bug because other bugs have already addressed the SSO question; this one is really about test cases at this point, a minor refactor, and ensuring we don't leak memory in some cases on unwatch().

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cabe6efe4de8

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Backed out for xpcshell failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/29ccee717a9b

https://tbpl.mozilla.org/php/getParsedLog.php?id=36332919&tree=Mozilla-Inbound

Comment 29

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Oh no!  Looking into it now.

Comment 30

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: FxA RPs provide required callbacks; callbacks fire in proper sequence

r=ferjm

fixed xpcshell tests for toolkit/identity.  Sorry, I had made a rebasing error :(

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/3216252d1a53

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3216252d1a53