Bug 1146346 - fix sync login when master-password was unlocked by something other than sync. r=ckarlof
authorMark Hammond <mhammond@skippinet.com.au>
Thu, 02 Apr 2015 09:51:05 +1100
changeset 237254 283d8666043ff4bd723315be266fe3e288982008
parent 237253 122413f5b05172226436539de56395136b494c8b
child 237255 52979d819dc2ff7b5253043aeaad4955d0dd66a3
push id57898
push usercbook@mozilla.com
push dateThu, 02 Apr 2015 12:14:17 +0000
treeherdermozilla-inbound@63c87250946e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersckarlof
bugs1146346
milestone40.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1146346 - fix sync login when master-password was unlocked by something other than sync. r=ckarlof
services/sync/modules/browserid_identity.js
--- a/services/sync/modules/browserid_identity.js
+++ b/services/sync/modules/browserid_identity.js
@@ -415,43 +415,33 @@ this.BrowserIDManager.prototype = {
   _getSyncCredentialsHosts: function() {
     return Utils.getSyncCredentialsHostsFxA();
   },
 
   /**
    * The current state of the auth credentials.
    *
    * This essentially validates that enough credentials are available to use
-   * Sync, although it effectively ignores the state of the master-password -
-   * if that's locked and that's the only problem we can see, say everything
-   * is OK - unlockAndVerifyAuthState will be used to perform the unlock
-   * and re-verification if necessary.
+   * Sync. It doesn't check we have all the keys we need as the master-password
+   * may have been locked when we tried to get them - we rely on
+   * unlockAndVerifyAuthState to check that for us.
    */
   get currentAuthState() {
     if (this._authFailureReason) {
       this._log.info("currentAuthState returning " + this._authFailureReason +
                      " due to previous failure");
       return this._authFailureReason;
     }
     // TODO: need to revisit this. Currently this isn't ready to go until
     // both the username and syncKeyBundle are both configured and having no
     // username seems to make things fail fast so that's good.
     if (!this.username) {
       return LOGIN_FAILED_NO_USERNAME;
     }
 
-    // No need to check this.syncKey as our getter for that attribute
-    // uses this.syncKeyBundle
-    // If bundle creation started, but failed due to any reason other than
-    // the MP being locked...
-    if (this._shouldHaveSyncKeyBundle && !this.syncKeyBundle && !Utils.mpLocked()) {
-      // Return a state that says a re-auth is necessary so we can get keys.
-      return LOGIN_FAILED_LOGIN_REJECTED;
-    }
-
     return STATUS_OK;
   },
 
   // Do we currently have keys, or do we have enough that we should be able
   // to successfully fetch them?
   _canFetchKeys: function() {
     let userData = this._signedInUser;
     // a keyFetchToken means we can almost certainly grab them.
@@ -462,32 +452,36 @@ this.BrowserIDManager.prototype = {
   /**
    * Verify the current auth state, unlocking the master-password if necessary.
    *
    * Returns a promise that resolves with the current auth state after
    * attempting to unlock.
    */
   unlockAndVerifyAuthState: function() {
     if (this._canFetchKeys()) {
+      log.debug("unlockAndVerifyAuthState already has (or can fetch) sync keys");
       return Promise.resolve(STATUS_OK);
     }
     // so no keys - ensure MP unlocked.
     if (!Utils.ensureMPUnlocked()) {
       // user declined to unlock, so we don't know if they are stored there.
+      log.debug("unlockAndVerifyAuthState: user declined to unlock master-password");
       return Promise.resolve(MASTER_PASSWORD_LOCKED);
     }
     // now we are unlocked we must re-fetch the user data as we may now have
     // the details that were previously locked away.
     return this._fxaService.getSignedInUser().then(
       accountData => {
         this._updateSignedInUser(accountData);
         // If we still can't get keys it probably means the user authenticated
         // without unlocking the MP or cleared the saved logins, so we've now
         // lost them - the user will need to reauth before continuing.
-        return this._canFetchKeys() ? STATUS_OK : LOGIN_FAILED_LOGIN_REJECTED;
+        let result = this._canFetchKeys() ? STATUS_OK : LOGIN_FAILED_LOGIN_REJECTED;
+        log.debug("unlockAndVerifyAuthState re-fetched credentials and is returning", result);
+        return result;
       }
     );
   },
 
   /**
    * Do we have a non-null, not yet expired token for the user currently
    * signed in?
    */
@@ -524,16 +518,17 @@ this.BrowserIDManager.prototype = {
     let client = this._tokenServerClient;
     let fxa = this._fxaService;
     let userData = this._signedInUser;
 
     // We need kA and kB for things to work.  If we don't have them, just
     // return null for the token - sync calling unlockAndVerifyAuthState()
     // before actually syncing will setup the error states if necessary.
     if (!this._canFetchKeys()) {
+      log.info("Unable to fetch keys (master-password locked?), so aborting token fetch");
       return Promise.resolve(null);
     }
 
     let maybeFetchKeys = () => {
       // This is called at login time and every time we need a new token - in
       // the latter case we already have kA and kB, so optimise that case.
       if (userData.kA && userData.kB) {
         return;