1254810 - Changing password doesn't cause browser to enter reauth state until browser is restarted.

Status: RESOLVED WORKSFORME

(Firefox :: Sync, defect, P1)

Description

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

STR:
* Create a bool pref |services.sync.debug.ignoreCachedAuthCredentials| set to true so the repro happens immediately.
* Configure Sync, perform a first sync.
* In the same browser, change your password via "manage account"
* Perform a Sync

Expected:
* Hamburger menu enters the "please reconnect to Sync" state.

Actual:
* Sync fails but the state isn't reflected in the hamburger menu (but it *is* reflected in the prefs pane)

If you restart the browser it correctly enters that state.

The relevant log entries:
> 1457493320592	Sync.BrowserIDManager	ERROR	Authentication error in _fetchTokenForUser: {"details":{"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"}}
> 1457493320592	Sync.Status	DEBUG	Status.login: success.login => error.login.reason.account
> 1457493320592	Sync.Status	DEBUG	Status.service: success.status_ok => error.login.failed
> 1457493320592	Sync.BrowserIDManager	INFO	Failed to fetch the cluster URL: {"details":{"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"}}
> 1457493320592	Sync.Service	DEBUG	Cluster value = null
> 1457493320592	Sync.Status	DEBUG	Status.sync: success.sync => error.sync.reason.no_node_found
> 1457493320592	Sync.Status	DEBUG	Status.service: error.login.failed => error.sync.failed

So Sync tried to update its token and failed. Sync treated this as a "no node available" error and changed the service state away from the error.login.failed state to error.sync.failed - it's that initial state that the hamburger menu expects to see before showing its reauth state.

(aside - the logs also show:

> 1457493309567	FirefoxAccounts	WARN	Unrecognized FxAccountsWebChannel command: fxaccounts:change_password

as the password is changed, which we should consider using so we don't rely on the token expiring before the device notices - but that's somewhat orthogonal to this bug)

(aside 2 - this is very difficult to write a test for - it might make sense to spend some time with tps to add checks for these states - tps will let you restart the browser etc - but that's probably a few days work I'd think, so probably shouldn't block this being fixed)

Comment 1

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

I think the correct fix here depends on bug 1044530. For this bug, we are entering unlockAndVerifyAuthState() which is returning STATUS_OK, when it should be returning LOGIN_FAILED_LOGIN_REJECTED. The reason it returns STATUS_OK is because the condition |return userData && (userData.keyFetchToken || (userData.kA && userData.kB));| is returning true (ie, we have either a keyFetchToken or kA and kB - in this case I believe it will be that we have the actual keys).

Note that this check happens *after* we've failed to fetch a new certificate - so that check isn't catching that. Bug 1044530 will cause us to drop ["sessionToken", "keyPair", "cert"] on failure, but not kA or kB - However, I'm thinking it should at least drop kB. But even if that's not the right thing to do, checking if we have a session token would still make sense - so a patch like below should solve the problem.

Kit, does it make sense to drop kB on on failure to get a certificate? If so, I think the patch below is unnecessary and this bug would be fixed by bug 1044530. If not, the patch below would work after that bug lands.

diff --git a/services/sync/modules/browserid_identity.js b/services/sync/modules/browserid_identity.
js
index 4ec08cc..22db792 100644
--- a/services/sync/modules/browserid_identity.js
+++ b/services/sync/modules/browserid_identity.js
@@ -461,9 +461,12 @@ this.BrowserIDManager.prototype = {
   // to successfully fetch them?
   _canFetchKeys: function() {
     let userData = this._signedInUser;
+    // A missing sessionToken means our state is invalid and we need to
+    // log back in.
     // a keyFetchToken means we can almost certainly grab them.
     // kA and kB means we already have them.
-    return userData && (userData.keyFetchToken || (userData.kA && userData.kB));
+    return userData && userData.sessionToken &&
+           (userData.keyFetchToken || (userData.kA && userData.kB));
   },

Comment 2

[Chris Karlof [:ckarlof]]

> -    return userData && (userData.keyFetchToken || (userData.kA && userData.kB));
> +    return userData && userData.sessionToken &&
> +           (userData.keyFetchToken || (userData.kA && userData.kB));

As an aside, this reminds that there's possible value in a future refactoring that captures the FxA client states more explicitly (https://wiki.mozilla.org/User_Services/Sync/FxA_Client_States).

Mark or Kit, perhaps we should open a bug on that? I feel it'll make bugs like this less likely.

Comment 3

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

(In reply to Chris Karlof [:ckarlof] from comment #2)
> Mark or Kit, perhaps we should open a bug on that? I feel it'll make bugs
> like this less likely.

Yeah, we should! I opened bug 1256459

Comment 4

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

(In reply to Mark Hammond [:markh] from comment #3)
> Yeah, we should! I opened bug 1256459

(Oops - hit "send" too soon) - but I think we should still fix these in the meantime

Comment 5

[Chris Karlof [:ckarlof]]

> (Oops - hit "send" too soon) - but I think we should still fix these in the meantime

Yep, I agree!

Comment 6

[Lina Butler [:lina]]

I somehow missed your ni? on this, Mark. My apologies. The blacklist approach that you suggested for bug 1044530 should take care of clearing out the old keys.

Comment 7

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

(In reply to Kit Cambridge [:kitcambridge] from comment #6)
> I somehow missed your ni? on this, Mark. My apologies. The blacklist
> approach that you suggested for bug 1044530 should take care of clearing out
> the old keys.

Yep - that bug has made this irrelevant...