Bug 1013064 (part 4) - browserid_identity and sync changes to support FxA credentials in the login manager. r=ckarlof,rnewman
authorMark Hammond <mhammond@skippinet.com.au>
Sat, 14 Jun 2014 14:33:56 +1000
changeset 197857 da30483ac566e29299ecffab2f7db8c4dfcd3e65
parent 197856 746fb5986153b1d696fae080a4dbfb87d125d5d8
child 197858 0ce8cf754d327957015d9f9cc6a8bd033d7c8f28
push id47247
push userneil@mozilla.com
push dateTue, 05 Aug 2014 14:08:09 +0000
treeherdermozilla-inbound@0ce8cf754d32 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersckarlof, rnewman
bugs1013064
milestone34.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 1013064 (part 4) - browserid_identity and sync changes to support FxA credentials in the login manager. r=ckarlof,rnewman From 9717484083e66b78eedfa14e539d51382aba760f Mon Sep 17 00:00:00 2001 --- services/sync/modules/browserid_identity.js | 61 ++++++++++++++++++++-- services/sync/modules/identity.js | 19 +++++++ services/sync/modules/service.js | 20 ++++--- .../sync/tests/unit/test_browserid_identity.js | 15 ++++++ 4 files changed, 102 insertions(+), 13 deletions(-)
services/sync/modules/browserid_identity.js
services/sync/modules/identity.js
services/sync/modules/service.js
services/sync/tests/unit/test_browserid_identity.js
--- a/services/sync/modules/browserid_identity.js
+++ b/services/sync/modules/browserid_identity.js
@@ -394,41 +394,83 @@ this.BrowserIDManager.prototype = {
     this._syncKeyUpdated = true;
     this._shouldHaveSyncKeyBundle = false;
   },
 
   /**
    * The current state of the auth credentials.
    *
    * This essentially validates that enough credentials are available to use
-   * Sync.
+   * 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.
    */
   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.
-    if (this._shouldHaveSyncKeyBundle && !this.syncKeyBundle) {
-      return LOGIN_FAILED_NO_PASSPHRASE;
+    // 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.
+    // kA and kB means we already have them.
+    return userData && (userData.keyFetchToken || (userData.kA && userData.kB));
+  },
+
+  /**
+   * 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()) {
+      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.
+      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;
+      }
+    );
+  },
+
   /**
    * Do we have a non-null, not yet expired token for the user currently
    * signed in?
    */
   hasValidToken: function() {
     if (!this._token) {
       return false;
     }
@@ -444,16 +486,24 @@ this.BrowserIDManager.prototype = {
     if (tokenServerURI.endsWith("/")) { // trailing slashes cause problems...
       tokenServerURI = tokenServerURI.slice(0, -1);
     }
     let log = this._log;
     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("_fetchTokenForUser has no keys to use.");
+      return null;
+    }
+
     log.info("Fetching assertion and token from: " + tokenServerURI);
 
     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;
       }
@@ -519,17 +569,18 @@ this.BrowserIDManager.prototype = {
         // TODO: write tests to make sure that different auth error cases are handled here
         // properly: auth error getting assertion, auth error getting token (invalid generation
         // and client-state error)
         if (err instanceof AuthenticationError) {
           this._log.error("Authentication error in _fetchTokenForUser: " + err);
           // set it to the "fatal" LOGIN_FAILED_LOGIN_REJECTED reason.
           this._authFailureReason = LOGIN_FAILED_LOGIN_REJECTED;
         } else {
-          this._log.error("Non-authentication error in _fetchTokenForUser: " + err.message);
+          this._log.error("Non-authentication error in _fetchTokenForUser: "
+                          + (err.message || err));
           // for now assume it is just a transient network related problem.
           this._authFailureReason = LOGIN_FAILED_NETWORK_ERROR;
         }
         // Drop the sync key bundle, but still expect to have one.
         // This will arrange for us to be in the right 'currentAuthState'
         // such that UI will show the right error.
         this._shouldHaveSyncKeyBundle = true;
         Weave.Status.login = this._authFailureReason;
--- a/services/sync/modules/identity.js
+++ b/services/sync/modules/identity.js
@@ -373,16 +373,35 @@ IdentityManager.prototype = {
     if (!this.syncKeyBundle) {
       return LOGIN_FAILED_INVALID_PASSPHRASE;
     }
 
     return STATUS_OK;
   },
 
   /**
+   * 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() {
+    // Try to fetch the passphrase - this will prompt for MP unlock as a
+    // side-effect...
+    try {
+      this.syncKey;
+    } catch (ex) {
+      this._log.debug("Fetching passphrase threw " + ex +
+                      "; assuming master password locked.");
+      return Promise.resolve(MASTER_PASSWORD_LOCKED);
+    }
+    return Promise.resolve(STATUS_OK);
+  },
+
+  /**
    * Persist credentials to password store.
    *
    * When credentials are updated, they are changed in memory only. This will
    * need to be called to save them to the underlying password store.
    *
    * If the password store is locked (e.g. if the master password hasn't been
    * entered), this could throw an exception.
    */
--- a/services/sync/modules/service.js
+++ b/services/sync/modules/service.js
@@ -679,27 +679,31 @@ Sync11Service.prototype = {
     }
 
     if (!this.identity.username) {
       this._log.warn("No username in verifyLogin.");
       this.status.login = LOGIN_FAILED_NO_USERNAME;
       return false;
     }
 
-    // Unlock master password, or return.
     // Attaching auth credentials to a request requires access to
     // passwords, which means that Resource.get can throw MP-related
     // exceptions!
-    // Try to fetch the passphrase first, while we still have control.
-    try {
-      this.identity.syncKey;
-    } catch (ex) {
-      this._log.debug("Fetching passphrase threw " + ex +
-                      "; assuming master password locked.");
-      this.status.login = MASTER_PASSWORD_LOCKED;
+    // So we ask the identity to verify the login state after unlocking the
+    // master password (ie, this call is expected to prompt for MP unlock
+    // if necessary) while we still have control.
+    let cb = Async.makeSpinningCallback();
+    this.identity.unlockAndVerifyAuthState().then(
+      result => cb(null, result),
+      cb
+    );
+    let unlockedState = cb.wait();
+    this._log.debug("Fetching unlocked auth state returned " + unlockedState);
+    if (unlockedState != STATUS_OK) {
+      this.status.login = unlockedState;
       return false;
     }
 
     try {
       // Make sure we have a cluster to verify against.
       // This is a little weird, if we don't get a node we pretend
       // to succeed, since that probably means we just don't have storage.
       if (this.clusterURL == "" && !this._clusterManager.setCluster()) {
--- a/services/sync/tests/unit/test_browserid_identity.js
+++ b/services/sync/tests/unit/test_browserid_identity.js
@@ -78,19 +78,33 @@ add_task(function test_initialializeWith
     browseridManager.initializeWithCurrentIdentity();
     yield browseridManager.whenReadyToAuthenticate.promise;
     do_check_true(!!browseridManager._token);
     do_check_true(browseridManager.hasValidToken());
     do_check_eq(browseridManager.account, identityConfig.fxaccount.user.email);
   }
 );
 
+add_task(function test_initialializeWithNoKeys() {
+    _("Verify start after initializeWithCurrentIdentity without kA, kB or keyFetchToken");
+    let identityConfig = makeIdentityConfig();
+    delete identityConfig.fxaccount.user.kA;
+    delete identityConfig.fxaccount.user.kB;
+    // there's no keyFetchToken by default, so the initialize should fail.
+    configureFxAccountIdentity(browseridManager, identityConfig);
+
+    yield browseridManager.initializeWithCurrentIdentity();
+    yield browseridManager.whenReadyToAuthenticate.promise;
+    do_check_eq(Status.login, LOGIN_SUCCEEDED, "login succeeded even without keys");
+    do_check_false(browseridManager._canFetchKeys(), "_canFetchKeys reflects lack of keys");
+});
 
 add_test(function test_getResourceAuthenticator() {
     _("BrowserIDManager supplies a Resource Authenticator callback which returns a Hawk header.");
+    configureFxAccountIdentity(browseridManager);
     let authenticator = browseridManager.getResourceAuthenticator();
     do_check_true(!!authenticator);
     let req = {uri: CommonUtils.makeURI(
       "https://example.net/somewhere/over/the/rainbow"),
                method: 'GET'};
     let output = authenticator(req, 'GET');
     do_check_true('headers' in output);
     do_check_true('authorization' in output.headers);
@@ -235,16 +249,17 @@ add_test(function test_RESTResourceAuthe
       (getTimestampDelta(authHeader, now) - 12 * HOUR_MS) < 2 * MINUTE_MS);
 
   run_next_test();
 });
 
 add_task(function test_ensureLoggedIn() {
   configureFxAccountIdentity(browseridManager);
   yield browseridManager.initializeWithCurrentIdentity();
+  yield browseridManager.whenReadyToAuthenticate.promise;
   Assert.equal(Status.login, LOGIN_SUCCEEDED, "original initialize worked");
   yield browseridManager.ensureLoggedIn();
   Assert.equal(Status.login, LOGIN_SUCCEEDED, "original ensureLoggedIn worked");
   Assert.ok(browseridManager._shouldHaveSyncKeyBundle,
             "_shouldHaveSyncKeyBundle should always be true after ensureLogin completes.");
 
   // arrange for no logged in user.
   let fxa = browseridManager._fxaService