Closed Bug 1319639 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Sync, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 53
Tracking Status
firefox-esr45 --- wontfix
firefox50 --- wontfix
firefox51 --- fixed
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: markh, Assigned: markh)

Details

Attachments

(1 file)

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);
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.
Assignee: nobody → markh
Priority: -- → P1
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()`.
Attachment #8815085 - Flags: review?(kcambridge) → review+
Pushed by mhammond@skippinet.com.au:
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
https://hg.mozilla.org/mozilla-central/rev/553e9e8548de
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
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
Attachment #8815085 - Flags: approval-mozilla-release?
Attachment #8815085 - Flags: approval-mozilla-beta?
Attachment #8815085 - Flags: approval-mozilla-aurora?
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.
Attachment #8815085 - Flags: approval-mozilla-beta?
Attachment #8815085 - Flags: approval-mozilla-beta+
Attachment #8815085 - Flags: approval-mozilla-aurora?
Attachment #8815085 - Flags: approval-mozilla-aurora+
(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 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.
Attachment #8815085 - Flags: approval-mozilla-release? → approval-mozilla-release-
You need to log in before you can comment on or make changes to this bug.