1319639 - Firefox doesn't enter "needs reauthentication" state after 401 getting a certificate during a sync

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

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

If we get a 401 fetching an initial token before we sync everything acts correctly. However, subsequent 401s hitting /certificate/sign will *not* enter needs-reauthenication state until the browser is restarted. This means that in this case the browser is silently failing to sync without the user being aware.

I simulated this with a simple patch which is below. It appears that sync isn't sending the notifications browser-fxaccounts expects to see to update the state. Adding "weave:service:sync:error" to the list of observers that cause us to update the UI appears would fix it.

patch against 50, which also requires the pref services.sync.debug.ignoreCachedAuthCredentials to be set to true (the hard-coded 7 is roughly how many token fetches will be done such that sync is actually running when the failure is simulated)

--- a/services/fxaccounts/FxAccounts.jsm
+++ b/services/fxaccounts/FxAccounts.jsm
@@ -33,6 +33,7 @@ XPCOMUtils.defineLazyModuleGetter(this, "FxAccountsProfile",
 XPCOMUtils.defineLazyModuleGetter(this, "Utils",
   "resource://services-sync/util.js");

+let numCertificateRequests = 0;
 // All properties exposed by the public FxAccounts API.
 var publicProperties = [
   "accountStatus",
@@ -1005,6 +1006,11 @@ FxAccountsInternal.prototype = {

     // and generate the cert.
     let certWillBeValidUntil = this.now() + CERT_LIFETIME;
+    ++numCertificateRequests;
+    if (numCertificateRequests == 7) {
+      log.debug("simulating certificate failure");
+      throw { code: 401, errno: ERRNO_INVALID_AUTH_TOKEN };
+    }
     let certificate = yield this.getCertificateSigned(accountData.sessionToken,
                                                       keyPair.rawKeyPair.serializedPublicKey,
                                                       CERT_LIFETIME);

Comment 1

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

One line patch coming up. Sadly the browser tests aren't very thorough, so a new test is more difficult than seems worthwhile, but this patch should be safe and I tested it with the STR in comment 0 and it works as expected.

Comment 2

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

Attachment: Bug 1319639 - update the browser UI on a sync error to ensure an auth error during a sync is correctly handled.

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

Comment 3

[Lina Butler [:lina]]

Comment on attachment 8815085 [details]
Bug 1319639 - update the browser UI on a sync error to ensure an auth error during a sync is correctly handled.

https://reviewboard.mozilla.org/r/96080/#review96238

This looks good! I was surprised that the 401 didn't trigger `ONLOGOUT_NOTIFICATION`, but we only do that automatically for deleted accounts, and in `getKeys()`.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/553e9e8548de
update the browser UI on a sync error to ensure an auth error during a sync is correctly handled. r=kitcambridge

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/553e9e8548de

Comment 6

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

Comment on attachment 8815085 [details]
Bug 1319639 - update the browser UI on a sync error to ensure an auth error during a sync is correctly handled.

Approval Request Comment
[Feature/Bug causing the regression]: n/a
[User impact if declined]: If sync encounters an authentication error after the browser has been running for a while, the user will not be notified of the failure and sync will silently fail until the next restart.
[Is this code covered by automated tests?]: Unfortunately this part of the code doesn't have good automated test coverage.
[Has the fix been verified in Nightly?]: Verified by me.
[Needs manual test from QE? If yes, steps to reproduce]: This code is also very difficult to test manually, as it relies on a server deciding to throw an authentication error. 
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: This is a single line change that causes the browser to update the sync UI whenever a sync fails.
[String changes made/needed]: None

Comment 7

[Gerry Chang [:gchang]]

Comment on attachment 8815085 [details]
Bug 1319639 - update the browser UI on a sync error to ensure an auth error during a sync is correctly handled.

Fix an error handle condition during sync. Beta51+ and Aurora52+. Should be 51 beta 6.

Comment 8

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

(In reply to Kit Cambridge [:kitcambridge] from comment #3)
> I was surprised that the 401 didn't trigger ONLOGOUT_NOTIFICATION`, 

Yeah - ONLOGOUT originally meant "no one is logged in", vs a user-known-but-needs-reauth state.

> but we only do that automatically for deleted accounts

IIRC, we do reset to that "no user" state in that case, which seems sane to me.

> and in `getKeys()`.

hrm - but this seems wrong - it probably shouldn't do that and I'd expect the UI to get upset - I'll try and remember to dig into this (and obviously feel free to yourself ;)

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a065f231fb79

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b0a358343e70

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8815085 [details]
Bug 1319639 - update the browser UI on a sync error to ensure an auth error during a sync is correctly handled.

This is fixed for 51 already and we're past the beta to release merge date.