1004319 - Handle server-side account changes in Gecko

Status: RESOLVED FIXED

(Firefox OS Graveyard :: FxA, defect, P2)

Description

[Sam Penrose]

STR:

1) Login to FxA on FxOS
2) Reset password on the web
3) Trigger a call to FxAccountsClient.signCertificate, which (appropriately) returns 401/110
4) nsDomIdentity.request() is now useless; you can no longer get an assertion or a sign in dialog. Going to FxA's Settings UI will not tell you anything, though I assume choosing to sign out there, then sign back in, will work around the problem

I propose that if the client receives a server authorization failure, it should sign out. Please let me know if I misunderstand.

Comment 1

[Chris Karlof [:ckarlof]]

I think there is room for improvement in these APIs, e.g., a more explicit error condition saying "someone is signed in, but there are in an error state". That's what a 401/110 signals, but it's not ideal. I suspect handling this error state will be client specific, and the response to it should not be in our common libraries. 

When Desktop Sync receives a 401/110, it displays a "Reconnect to Sync" message everywhere where we signal the logged in state to the user.

If silently logging out the user in response to a password reset on another device is the appropriate UX for FxOS, then I suggest it be implemented somewhere outside of our shared codebase.

Comment 2

[Sam Penrose]

Attachment: 1004319-signout-on-reset.patch

This patch:
  - forces checking with the server on each getAssertion(), to make testing feasible (I'll take that out)
  - for B2G only, detects errno 110 on failure and signs out using the cross-platform code
  - does not yet send an appropriate event through toolkit/identity->dom/identity to fire the RP's onlogout()

Jed let me know if you see problems with the general approach. It's another I-wish-the-current-code-were-structured-differently effort.

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Sam Penrose from comment #0)
 > I propose that if the client receives a server authorization failure, it
> should sign out. Please let me know if I misunderstand.

Shouldn't we force authentication instead of silently sign out?

Comment 4

[Sam Penrose]

That's a great question. I can see it being a better user experience because they don't have to enter the email address again. But it gets into tricky flow issues -- when do we surface the dialog? With the current code we can maybe interrupt what they're doing with a modal system flow (actually I don't know if this would work), but that feels terrible -- what if they are sending an important text message? If instead we want to convert the next request() to a forceAuth, then we have to store state somewhere, presumably in FxAccountsManager.jsm. Do you see a clean way to do that? I will think about the problem some more -- maybe there is a simple solution.

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #3)
> (In reply to Sam Penrose from comment #0)
>  > I propose that if the client receives a server authorization failure, it
> > should sign out. Please let me know if I misunderstand.
> 
> Shouldn't we force authentication instead of silently sign out?

Comment 5

[Fernando Jiménez Moreno [:ferjm]]

I believe the situation should be similar to a mozId.request() call when there is no logged in account. We need to launch the authentication UI when trying to get an assertion fails because the password changed. RPs will simply not get the assertion until the authentication flow is completed, so we don't need to store any state. If the authentication flow does not complete, we just logged the user out as you did with refreshAuthentication and we trigger the RP onerror callback with the "wrong password" or whatever reason. The next mozId.request() will trigger a regular sign in flow.

Comment 6

[Sam Penrose]

Thanks very much for the clarification -- I see what you are saying now. You're right, there is no need to store state, so refreshAuth should work cleanly. Thanks for the insight!

Comment 7

[Chris Karlof [:ckarlof]]

FWIW, on Desktop we detect invalid session tokens and trigger a "force auth". The corner case is distinguishing between password reset and delete account, which can be done with the new /account/status API endpoint.

Comment 8

[Sam Penrose]

Attachment: 1004319-handle-server-state.patch

With the diff at 220 lines, it feels like time to touch base. So far the patch does these things (bottom to top as you read):

  1) Extensively comments and refactors getAssertion() for clarity. I'd like to look at having most of it move to a request() method for all the RP-API-only paths, and make getAssertion() a simple IAC-focused method.
  2) Breaks out _refreshAuthentication(), which is now called from two places.
  3) Breaks out _handleGetAssertionError() to implement the core logic that motivated this issue. I have not yet tested account deletion functionality.

I have manually tested the use case from comment 0, and the patch fixes it. I have not yet looked at what needs unit tests and what is meaningfully testable. Thanks for your thoughts, Jed.

Comment 9

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

Comment on attachment 8424105 [details] [diff] [review]
1004319-handle-server-state.patch

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

This makes sense to me, Sam.  The refactoring you've done makes the code much clearer, too, so it's doubly good.

I'm also not sure about whether sign-in flow should be surfaced automatically, or whether the caller should do it.  I'm tending towards believing that, if your account changes, it would make sense to surface the UI on the phone, because it's an account-level thing, and not merely scoped to a particular caller.  This might be a good question for our UX folks.

::: services/fxaccounts/FxAccountsManager.jsm
@@ +156,5 @@
> +   * Determine whether the incoming error means that the current account
> +   * has new server-side state via deletion or password change, and if so,
> +   * spawn the appropriate UI (sign in or refresh); otherwise re-reject.
> +   *
> +   * TODO: review all error states for possible inclusion here.

Maybe create a follow-up bug, so this doesn't get lost in the shuffle?

@@ +170,5 @@
> +              (user) => {
> +                return this._refreshAuthentication(aAudience, user.email);
> +              }
> +            );
> +          } else { // XXX kick off signin here, or does caller do it?

Would that depend on whether the caller wanted a "silent" assertion or not?  Or perhaps UI should always be surfaced; otherwise how would the user ever get this feedback?

@@ +180,5 @@
> +              }
> +            );
> +          }
> +        }
> +        

whitespace

@@ +437,5 @@
> +   *   else if we were asked to refresh and the grace peroid is up:
> +   *     trigger the refresh flow.
> +   *   else ask the core code for an assertion, which might itself
> +   *   trigger either the sign in or refresh flows (if our account
> +   *   changed on the server).

yay for documentation!

@@ +474,5 @@
>                return this._error(ERROR_NO_SILENT_REFRESH_AUTH);
>              }
> +            let secondsSinceAuth = (Date.now() / 1000) - this._activeSession.authAt;
> +            if (secondsSinceAuth > gracePeriod) {
> +              return this._refreshAuthentication(aAudience, user.email);

the refactor makes this clearer

Comment 10

[Sam Penrose]

(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #9)
> Comment on attachment 8424105 [details] [diff] [review]
> 1004319-handle-server-state.patch
> 
> Review of attachment 8424105 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This makes sense to me, Sam.  The refactoring you've done makes the code
> much clearer, too, so it's doubly good.
> 
> I'm also not sure about whether sign-in flow should be surfaced
> automatically, or whether the caller should do it.  I'm tending towards
> believing that, if your account changes, it would make sense to surface the
> UI on the phone, because it's an account-level thing, and not merely scoped
> to a particular caller.  This might be a good question for our UX folks.
> 
> ::: services/fxaccounts/FxAccountsManager.jsm
> @@ +156,5 @@
> > +   * Determine whether the incoming error means that the current account
> > +   * has new server-side state via deletion or password change, and if so,
> > +   * spawn the appropriate UI (sign in or refresh); otherwise re-reject.
> > +   *
> > +   * TODO: review all error states for possible inclusion here.
> 
> Maybe create a follow-up bug, so this doesn't get lost in the shuffle?
> 
> @@ +170,5 @@
> > +              (user) => {
> > +                return this._refreshAuthentication(aAudience, user.email);
> > +              }
> > +            );
> > +          } else { // XXX kick off signin here, or does caller do it?
> 
> Would that depend on whether the caller wanted a "silent" assertion or not? 
> Or perhaps UI should always be surfaced; otherwise how would the user ever
> get this feedback?

Ah, .silent: good catch. Ignoring the (actually more important) UX question, I suppose we push this back to getAssertion(), where .silent exists. I am inclined to bite the bullet and factor IAC -> getAssertion() separately from RP's request() -> getAssertion().

Can you remind me of the product thinking, if any, behind .silent?

Comment 11

[Sam Penrose]

(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #9)
> Comment on attachment 8424105 [details] [diff] [review]
> 1004319-handle-server-state.patch
> > +   * TODO: review all error states for possible inclusion here.
> 
> Maybe create a follow-up bug, so this doesn't get lost in the shuffle?

I'm going to handle these errors as part of the current work. Currently the only HTTP call triggers by getAssertion() is to /certificate/sign, which can return a whopping 10 separate errors. Good news is that most of them indicate bad code within services/fxaccounts or other throw-up-your-hands-and-die situations:

  https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#response-12

Comment 12

[Sam Penrose]

(In reply to Sam Penrose from comment #11)
> /certificate/sign, which can
> return a whopping 10 separate errors. Good news is that most of them
> indicate bad code within services/fxaccounts or other
> throw-up-your-hands-and-die situations:
> 
> https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#response-12

I believe that all except "status code 401, errno 110: invalid authentication token" indicate code problems rather that valid user states. "invalid request signature" (sounds like HAWK) and "invalid authentication timestamp" give me modest pause, but not enough to escalate.

Comment 13

[Sam Penrose]

Attachment: 1004319-handle-server-state.patch

Updates from last patch:
  1) I verified and commented the HTTP error handling per comments.
  2) I wrote unit tests ...
  3) ... which reveal that the refresh and signin/up flows really want to be called from the same place. Re: discussion of .silent above, I went ahead and dropped them into the error handling, but I have not factored request() out. Thanks for your time, sir.

Comment 14

[Sam Penrose]

Attachment: 1004319-handle-server-state.patch

Something got unhappy about my caching rejected promises; this version has a two-line tweak to bypass that. No rush on the review.

Comment 15

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

Comment on attachment 8433721 [details] [diff] [review]
1004319-handle-server-state.patch

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

This looks good to me, Sam.  Thanks in particular for the new tests, and the improvements in comments and documentation.

r=me

::: services/fxaccounts/FxAccountsManager.jsm
@@ +189,5 @@
> +                return this._error(reason);
> +              }
> +            );
> +          }
> +        }        

Tad of trailing whitespace

@@ +226,5 @@
> +    );
> +  },
> +
> +  _localSignOut: function() {
> +    return this._fxAccounts.signOut(true);

It's a small point, but this makes me wonder whether FxAccounts.jsm should really offer its own version of _signOutLocal and -Server as public methods, and we rename the methods to signOutEverywhere(), signOutLocal(), and signOutServer().

@@ +446,5 @@
> +   *   else if we were asked to refresh and the grace period is up:
> +   *     trigger the refresh flow.
> +   *   else ask the core code for an assertion, which might itself
> +   *   trigger either the sign in or refresh flows (if our account
> +   *   changed on the server).

Thanks for the additional comment here.

@@ +464,4 @@
>      return this.getAccount().then(
>        user => {
>          if (user) {
> +          // Three have-user cases to consider. First: are we unverified?

Again, even easier to follow with your new comments.  Thanks.

Comment 16

[Sam Penrose]

https://tbpl.mozilla.org/?tree=Try&rev=97b5da75e8c1

Comment 17

[Sam Penrose]

Attachment: 1004319-handle-server-state.patch

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/4720480c606b

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4720480c606b