1044530 - Auth errors on /certificate/sign should cause us to discard the fxa session token

Status: RESOLVED FIXED

(Firefox :: Firefox Accounts, enhancement, P2)

Description

[Ryan Kelly [:rfkelly]]

In Bug 1042109 Comment 12, we have some sync logs from a desktop machine attempting to sync after a password reset.  Its FxA session token has been invalidated by the password reset, so its attempts to get a freshly signed certificate fail with the expected:

"""
1406318884269	FirefoxAccounts	ERROR	error POSTing /certificate/sign: {"code":401,"errno":110,"error":"Unauthorized","message":"Invalid authentication token in request signature","info":"https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#response-format"}
1406318884269	FirefoxAccounts	ERROR	HAWK.signCertificate error: {"code":401,"errno":110,"error":"Unauthorized","message":"Invalid authentication token in request signature","info":"https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#response-format"}
1406318884270	Sync.BrowserIDManager	ERROR	Authentication error in _fetchTokenForUser: AuthenticationError([object Object])
"""

But it keeps trying to refresh its certificate using the same session token, getting the same error every 10 minutes, until the user eventually intervenes to re-authenticate with FxA.

Ideally the {"code":401,"errno":110} response would cause us to discard the now-known-to-be-invalid session token and avoid these doomed-to-fail network requests.

Comment 1

[Sam Penrose]

This is a Sync bug, not a Core bug. I cope with this case in B2G-only code here:

  https://github.com/mozilla/gecko-dev/blob/master/services/fxaccounts/FxAccountsManager.jsm#L171

I agree that it should be fixed in FxAccounts.jsm. Maybe it can be done cleanly. If not, that's one more reason for a complete rewrite of that module.

Comment 2

[Ryan Kelly [:rfkelly]]

I've no idea if this is still valid, /cc :markh

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Yeah, I think this remains current and worthwhile.

Comment 4

[Mark Hammond [:markh] [:mhammond]]

According to http://mzl.la/1SY9JAF, beta-43 has seen 10k "invalid session
token" errors querying /certificate/sign. However, this bug prevents us from reading any meaning into those figures - we can't tell how many of these failures are due to this automatic retry. We should fix this bug and see what new results we get.

Comment 5

[Lina Butler [:lina]]

Should we clear out the session token for all `{"code":401,"errno":110}` responses, or specifically `/certificate/sign`? I'm happy to handle the general case, though, for expediency, it might make sense to start with just the latter.

Comment 6

[Ryan Kelly [:rfkelly]]

> Should we clear out the session token for all `{"code":401,"errno":110}` responses

Yes, but starting with just the /certificate/sign ones would still add a lot of value.

Comment 7

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1044530 - Remove invalid session and key fetch tokens from account storage. r?markh

Review commit: https://reviewboard.mozilla.org/r/31119/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31119/

Comment 8

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/31119/#review27933

::: services/fxaccounts/FxAccounts.jsm:519
(Diff revision 1)
> -    return currentState.getUserAccountData().then(data => {
> +    return this.withValidUserAccountData(data => {

I'm not fond of this name. `ensureValidTokenWithUserAccountData` is more accurate, but tedious.

Comment 9

[Lina Butler [:lina]]

Comment on attachment 8708605 [details]
MozReview Request: Bug 1044530 - Remove invalid session and key fetch tokens from account storage. r?markh

Looking at the auth server docs, I see a few more `FxAccountsClient` calls that can return invalid token errors.

* accountKeys: We should sign out if the key fetch token is invalid, since we can't get a new one unless the user signs back in.
* resendVerificationEmail: Requires a valid session token. We should probably sign the user out if it's is invalid.
* recoveryEmailStatus: We can probably leave this one, since it'll already stop polling, and attempting to operate on the account will sign the user out in any case.
* signOut: Destroys the session token, so we don't care if it's valid.

Comment 10

[Chris Karlof [:ckarlof]]

(In reply to Kit Cambridge [:kitcambridge] from comment #9)
> Comment on attachment 8708605 [details]
> MozReview Request: Bug 1044530 - Sign out if the FxA session or key fetch
> tokens are invalid. r?markh
> 
> Looking at the auth server docs, I see a few more `FxAccountsClient` calls
> that can return invalid token errors.
> 
> * accountKeys: We should sign out if the key fetch token is invalid, since
> we can't get a new one unless the user signs back in.
> * resendVerificationEmail: Requires a valid session token. We should
> probably sign the user out if it's is invalid.
> * recoveryEmailStatus: We can probably leave this one, since it'll already
> stop polling, and attempting to operate on the account will sign the user
> out in any case.
> * signOut: Destroys the session token, so we don't care if it's valid.

Signing out is different from "deleting the token and not making subsequent requests". The "signed out" state is different from the "needs reauth" state, which prompts the user to log back in.

However, in many (all?) cases when we get a 401, we'll likely want to also immediately check to see if the user's account still exists (they might have deleted it or used an invalid email to sign up), in which case we *do* want to sign them completely out instead of prompting them to log back in. 

Here's a code path showing an example of that: https://dxr.mozilla.org/mozilla-central/source/services/sync/modules/browserid_identity.js#248

Ideally, this patch could possibly generalize that policy a bit more.

Comment 11

[Ryan Kelly [:rfkelly]]

Comment on attachment 8708605 [details]
MozReview Request: Bug 1044530 - Remove invalid session and key fetch tokens from account storage. r?markh

Your proposes API coverage sounds right, but +1 to what Chris said - the appropriate response here is for the user to be asked for their password to re-connect to sync, not to be disconnected from sync entirely.

Comment 12

[Lina Butler [:lina]]

Comment on attachment 8708605 [details]
MozReview Request: Bug 1044530 - Remove invalid session and key fetch tokens from account storage. r?markh

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31119/diff/1-2/

Comment 13

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8708605 [details]
MozReview Request: Bug 1044530 - Remove invalid session and key fetch tokens from account storage. r?markh

https://reviewboard.mozilla.org/r/31119/#review28617

::: services/fxaccounts/FxAccounts.jsm:635
(Diff revision 2)
> +  withValidSignedInUser(func, options) {

I'm not that keen on the changes here - instead of a nice clean promise chain, we've moved to a promise-chain + a callback pattern that itself is a promise chain, which I find really difficult to get my head around in terms of control flow and return values.

Is there a reason we can't just have a final .catch at the end of the existing chains that does the removal of the data?

::: services/fxaccounts/FxAccounts.jsm:651
(Diff revision 2)
> +          return this.signOut(true);

While this is probably the right thing to do in general, I fear it will have undesirable UX for the case of bug 1226449, and may cause the code in browserid_identity that Chris pointed at to do the wrong thing (or possibly just be redundant)

::: services/fxaccounts/FxAccounts.jsm:662
(Diff revision 2)
> +            this.notifyObservers(ONLOGIN_NOTIFICATION)

Why do we notify here? In most cases we'd seem to already by doing a login when we hit this.

Comment 14

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1044530 - Remove invalid session and key fetch tokens from account storage. r=markh

Review commit: https://reviewboard.mozilla.org/r/36487/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36487/

Comment 15

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/31119/#review28617

> I'm not that keen on the changes here - instead of a nice clean promise chain, we've moved to a promise-chain + a callback pattern that itself is a promise chain, which I find really difficult to get my head around in terms of control flow and return values.
> 
> Is there a reason we can't just have a final .catch at the end of the existing chains that does the removal of the data?

Done.

> While this is probably the right thing to do in general, I fear it will have undesirable UX for the case of bug 1226449, and may cause the code in browserid_identity that Chris pointed at to do the wrong thing (or possibly just be redundant)

You're totally right. With this change, the additional check in `browserid_identity.js` is redundant. I updated the test to make sure the account is still cleared out in that case.

> Why do we notify here? In most cases we'd seem to already by doing a login when we hit this.

I thought it would trigger the "reconnect to Sync" UI, but that was a flawed assumption. With this patch, the UI will be shown on the next sync attempt.

Comment 16

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8723327 [details]
MozReview Request: Bug 1044530 - Remove invalid session and key fetch tokens from account storage. r=markh

https://reviewboard.mozilla.org/r/36487/#review33883

I think this is looking good, but the calls should be closer to the function that actually causes the error. ISTM a good rule-of-thumb might be that any function accepting the session token as an input param could handle it internally, but that might get messy or unclear, so chaning directly off the function that passes the session token in would also make sense.

Please let me know if I'm missing anything, and sorry for the delay in getting to this!

::: services/fxaccounts/FxAccounts.jsm:586
(Diff revision 1)
> -    });
> +    }).catch(error => this._handleTokenError(error, {

IIUC this is handing the error from resendVerificationMail(), so I think we should explicitly chain it off that call (or possibly directly in that function)

::: services/fxaccounts/FxAccounts.jsm:759
(Diff revision 1)
> -    }).then(result => currentState.resolve(result));
> +    }).catch(error => this._handleTokenError(error, {

similarly here - I think it's the fetchAndUnwrapKeys() call that could fail here, so we should chain from that call - or just move it into the body of that function.

::: services/fxaccounts/FxAccounts.jsm:1449
(Diff revision 1)
> -      return this._logErrorAndSetStaleDeviceFlag(error);
> +      // `_handleTokenError` re-throws the error.

and here - it's _recoverFromDeviceSessionConflict that might fail here.

::: services/sync/modules/browserid_identity.js:619
(Diff revision 1)
> +        // An FxAccounts.jsm error.

Seeing every block of this multi-if is identical I'd be inclined to make it a single statement with a multi-line "||" clause - but that's stylistic, so feel free to ignore if you think it reads better like this.

Comment 17

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/36487/#review33883

> similarly here - I think it's the fetchAndUnwrapKeys() call that could fail here, so we should chain from that call - or just move it into the body of that function.

Agreed. I kept it in this file to match `_handleDeviceError`, but I don't have a strong opinion on whether it belongs here or in the body of the function.

> and here - it's _recoverFromDeviceSessionConflict that might fail here.

Not quite. This just falls through for `_handleDeviceError`, so that I don't need to handle both errors for the device manager methods. But I can trade a little duplication if you feel it would make the intent clearer.

> Seeing every block of this multi-if is identical I'd be inclined to make it a single statement with a multi-line "||" clause - but that's stylistic, so feel free to ignore if you think it reads better like this.

I think it reads a bit better with the comments, but happy to make the change if you'd like.

Comment 18

[Lina Butler [:lina]]

Comment on attachment 8723327 [details]
MozReview Request: Bug 1044530 - Remove invalid session and key fetch tokens from account storage. r=markh

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36487/diff/1-2/

Comment 19

[Lina Butler [:lina]]

(In reply to Mark Hammond [:markh] from comment #16)
> I think this is looking good, but the calls should be closer to the function
> that actually causes the error. ISTM a good rule-of-thumb might be that any
> function accepting the session token as an input param could handle it
> internally, but that might get messy or unclear, so chaning directly off the
> function that passes the session token in would also make sense.

Per our discussion today, I moved us over to a whitelist of fields that we want to keep when we ask the user to reauth. I think that addresses your principal concern of remembering to clean up all the relevant fields. I decided to keep the `_handleTokenError` calls on the end because it's possible for us to invalidate the account state if the account no longer exists, and recovering earlier might cause us to operate on a state that's no longer current.

Comment 20

[Lina Butler [:lina]]

Comment on attachment 8723327 [details]
MozReview Request: Bug 1044530 - Remove invalid session and key fetch tokens from account storage. r=markh

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36487/diff/2-3/

Comment 21

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8723327 [details]
MozReview Request: Bug 1044530 - Remove invalid session and key fetch tokens from account storage. r=markh

https://reviewboard.mozilla.org/r/36487/#review41111

::: services/fxaccounts/FxAccounts.jsm:1137
(Diff revision 3)
>          if (!error || !error.code || error.code != 401) {
>            this.pollEmailStatusAgain(currentState, sessionToken, timeoutMs);
> +        } else {
> +          currentState.whenVerifiedDeferred.reject(new Error(
> +            "Verification status check failed"));
>          }

it looks like we should delete currentState.whenVerifiedDeferred after the rejection like is done a few lines below after a timeout (presumably so another caller of this starts the process again instead of immediately rejecting)

::: services/fxaccounts/FxAccounts.jsm:1596
(Diff revision 3)
>      }).then(() => {});
> -  }
> +  },
> +
> +  _handleTokenError(err) {
> +    if (!err || err.code != 401 || err.errno != ERRNO_INVALID_AUTH_TOKEN) {
> +      throw err;

I think we might as well have lots of logging in this function - here, let's log.error("unexpected error handling a token error", err);

::: services/fxaccounts/FxAccounts.jsm:1602
(Diff revision 3)
> +    }
> +    return this.accountStatus().then(exists => {
> +      if (!exists) {
> +        // Delete all local account data. Since the account no longer
> +        // exists, we can skip the remote calls.
> +        return this.signOut(true);

here: log.info("handleTokenError found the account no longer exists");

::: services/fxaccounts/FxAccounts.jsm:1607
(Diff revision 3)
> +        return this.signOut(true);
> +      }
> +
> +      // Delete all fields except those required for the user to
> +      // reauthenticate.
> +      let updateData = {};

then here: log.info("handleTokenError found an authentication error - clearing credentials");

(obviously feel free to tweak the wording and/or levels as you feel appropriate)

Comment 22

[Lina Butler [:lina]]

Comment on attachment 8723327 [details]
MozReview Request: Bug 1044530 - Remove invalid session and key fetch tokens from account storage. r=markh

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36487/diff/3-4/

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3e64199755c5

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3e64199755c5