Bug 990834 - Part 3: Fix handling of hawk errors. r=ckarlof, a=sledru
authorMark Hammond <mhammond@skippinet.com.au>
Thu, 10 Apr 2014 12:08:19 +1000
changeset 192737 d39a8f58016bd5e93f56330d4b3925d213404c5b
parent 192736 a84426aeb7a7f6127df691fe956769e5038af4bd
child 192738 a8575f98bb8afa52e5fa645d0950c9c1f122c7ea
push id474
push userasasaki@mozilla.com
push dateMon, 02 Jun 2014 21:01:02 +0000
treeherdermozilla-release@967f4cf1b31c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersckarlof, sledru
bugs990834
milestone30.0a2
Bug 990834 - Part 3: Fix handling of hawk errors. r=ckarlof, a=sledru
services/fxaccounts/FxAccounts.jsm
services/sync/modules/browserid_identity.js
services/sync/tests/unit/test_browserid_identity.js
--- a/services/fxaccounts/FxAccounts.jsm
+++ b/services/fxaccounts/FxAccounts.jsm
@@ -509,25 +509,28 @@ FxAccountsInternal.prototype = {
       if (!data) {
         throw new Error("Can't get keys; User is not signed in");
       }
       if (data.kA && data.kB) {
         return data;
       }
       if (!currentState.whenKeysReadyDeferred) {
         currentState.whenKeysReadyDeferred = Promise.defer();
-        this.fetchAndUnwrapKeys(data.keyFetchToken).then(data => {
-          if (!data.kA || !data.kB) {
-            currentState.whenKeysReadyDeferred.reject(
-              new Error("user data missing kA or kB")
-            );
-            return;
-          }
-          currentState.whenKeysReadyDeferred.resolve(data);
-        });
+        this.fetchAndUnwrapKeys(data.keyFetchToken).then(
+          data => {
+            if (!data.kA || !data.kB) {
+              currentState.whenKeysReadyDeferred.reject(
+                new Error("user data missing kA or kB")
+              );
+              return;
+            }
+            currentState.whenKeysReadyDeferred.resolve(data);
+          },
+          err => currentState.whenKeysReadyDeferred.reject(err)
+        );
       }
       return currentState.whenKeysReadyDeferred.promise;
     }).then(result => currentState.resolve(result));
    },
 
   fetchAndUnwrapKeys: function(keyFetchToken) {
     log.debug("fetchAndUnwrapKeys: token: " + keyFetchToken);
     let currentState = this.currentAccountState;
--- a/services/sync/modules/browserid_identity.js
+++ b/services/sync/modules/browserid_identity.js
@@ -214,18 +214,19 @@ this.BrowserIDManager.prototype = {
             // Mark any non-selected engines as declined.
             Weave.Service.engineManager.declineDisabled();
           } else {
             // Log out if the user canceled the dialog.
             return this._fxaService.signOut();
           }
         }
       }).then(() => {
-        return this._fetchSyncKeyBundle();
-      }).then(() => {
+        return this._fetchTokenForUser();
+      }).then(token => {
+        this._token = token;
         this._shouldHaveSyncKeyBundle = true; // and we should actually have one...
         this.whenReadyToAuthenticate.resolve();
         this._log.info("Background fetch for key bundle done");
         Weave.Status.login = LOGIN_SUCCEEDED;
         if (isInitialSync) {
           this._log.info("Doing initial sync actions");
           Svc.Prefs.set("firstSync", "resetClient");
           Services.obs.notifyObservers(null, "weave:service:setup-complete", null);
@@ -428,89 +429,91 @@ this.BrowserIDManager.prototype = {
       return false;
     }
     if (this._token.expiration < this._now()) {
       return false;
     }
     return true;
   },
 
-  _fetchSyncKeyBundle: function() {
-    // Fetch a sync token for the logged in user from the token server.
-    return this._fxaService.getKeys().then(userData => {
-      this._updateSignedInUser(userData); // throws if the user changed.
-      return this._fetchTokenForUser().then(token => {
-        this._token = token;
-        // both Jelly and FxAccounts give us kA/kB as hex.
-        let kB = Utils.hexToBytes(userData.kB);
-        this._syncKeyBundle = deriveKeyBundle(kB);
-        return;
-      });
-    });
-  },
-
   // Refresh the sync token for our user.
   _fetchTokenForUser: function() {
     let tokenServerURI = Svc.Prefs.get("tokenServerURI");
     let log = this._log;
     let client = this._tokenServerClient;
     let fxa = this._fxaService;
     let userData = this._signedInUser;
 
-    // Both Jelly and FxAccounts give us kB as hex
-    let kBbytes = CommonUtils.hexToBytes(userData.kB);
-    let headers = {"X-Client-State": this._computeXClientState(kBbytes)};
     log.info("Fetching assertion and token from: " + tokenServerURI);
 
-    function getToken(tokenServerURI, assertion) {
+    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;
+      }
+      return this._fxaService.getKeys().then(
+        newUserData => {
+          userData = newUserData;
+          this._updateSignedInUser(userData); // throws if the user changed.
+        }
+      );
+    }
+
+    let getToken = (tokenServerURI, assertion) => {
       log.debug("Getting a token");
       let deferred = Promise.defer();
       let cb = function (err, token) {
         if (err) {
-          log.info("TokenServerClient.getTokenFromBrowserIDAssertion() failed with: " + err);
-          if (err.response && err.response.status === 401) {
-            err = new AuthenticationError(err);
-          }
           return deferred.reject(err);
-        } else {
-          log.debug("Successfully got a sync token");
-          return deferred.resolve(token);
         }
+        log.debug("Successfully got a sync token");
+        return deferred.resolve(token);
       };
 
+      let kBbytes = CommonUtils.hexToBytes(userData.kB);
+      let headers = {"X-Client-State": this._computeXClientState(kBbytes)};
       client.getTokenFromBrowserIDAssertion(tokenServerURI, assertion, cb, headers);
       return deferred.promise;
     }
 
-    function getAssertion() {
+    let getAssertion = () => {
       log.debug("Getting an assertion");
       let audience = Services.io.newURI(tokenServerURI, null, null).prePath;
-      return fxa.getAssertion(audience).then(null, err => {
-        log.error("fxa.getAssertion() failed with: " + err.code + " - " + err.message);
-        if (err.code === 401) {
-          throw new AuthenticationError("Unable to get assertion for user");
-        } else {
-          throw err;
-        }
-      });
+      return fxa.getAssertion(audience);
     };
 
     // wait until the account email is verified and we know that
     // getAssertion() will return a real assertion (not null).
     return fxa.whenVerified(this._signedInUser)
+      .then(() => maybeFetchKeys())
       .then(() => getAssertion())
       .then(assertion => getToken(tokenServerURI, assertion))
       .then(token => {
         // TODO: Make it be only 80% of the duration, so refresh the token
         // before it actually expires. This is to avoid sync storage errors
         // otherwise, we get a nasty notification bar briefly. Bug 966568.
         token.expiration = this._now() + (token.duration * 1000) * 0.80;
+        if (!this._syncKeyBundle) {
+          // We are given kA/kB as hex.
+          this._syncKeyBundle = deriveKeyBundle(Utils.hexToBytes(userData.kB));
+        }
         return token;
       })
       .then(null, err => {
+        // TODO: unify these errors - we need to handle errors thrown by
+        // both tokenserverclient and hawkclient.
+        // A tokenserver error thrown based on a bad response.
+        if (err.response && err.response.status === 401) {
+          err = new AuthenticationError(err);
+        // A hawkclient error.
+        } else if (err.code === 401) {
+          err = new AuthenticationError(err);
+        }
+
         // 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 {
--- a/services/sync/tests/unit/test_browserid_identity.js
+++ b/services/sync/tests/unit/test_browserid_identity.js
@@ -504,18 +504,60 @@ add_task(function test_getHAWKErrors() {
       status: 200,
       headers: [],
       body: "",
     }
   });
   Assert.equal(Status.login, LOGIN_FAILED_NETWORK_ERROR, "login state is LOGIN_FAILED_NETWORK_ERROR");
 });
 
-add_task(function test_getKeysError() {
-  _("BrowserIDManager correctly handles getKeys failures.");
+add_task(function test_getGetKeysFailing401() {
+  _("BrowserIDManager correctly handles 401 responses fetching keys.");
+
+  _("Arrange for a 401 - Sync should reflect an auth error.");
+  let config = makeIdentityConfig();
+  // We want no kA or kB so we attempt to fetch them.
+  delete config.fxaccount.user.kA;
+  delete config.fxaccount.user.kB;
+  config.fxaccount.user.keyFetchToken = "keyfetchtoken";
+  yield initializeIdentityWithHAWKResponseFactory(config, function(method, data, uri) {
+    Assert.equal(method, "get");
+    Assert.equal(uri, "http://mockedserver:9999/account/keys")
+    return {
+      status: 401,
+      headers: {"content-type": "application/json"},
+      body: "{}",
+    }
+  });
+  Assert.equal(Status.login, LOGIN_FAILED_LOGIN_REJECTED, "login was rejected");
+});
+
+add_task(function test_getGetKeysFailing503() {
+  _("BrowserIDManager correctly handles 5XX responses fetching keys.");
+
+  _("Arrange for a 503 - Sync should reflect a network error.");
+  let config = makeIdentityConfig();
+  // We want no kA or kB so we attempt to fetch them.
+  delete config.fxaccount.user.kA;
+  delete config.fxaccount.user.kB;
+  config.fxaccount.user.keyFetchToken = "keyfetchtoken";
+  yield initializeIdentityWithHAWKResponseFactory(config, function(method, data, uri) {
+    Assert.equal(method, "get");
+    Assert.equal(uri, "http://mockedserver:9999/account/keys")
+    return {
+      status: 503,
+      headers: {"content-type": "application/json"},
+      body: "{}",
+    }
+  });
+  Assert.equal(Status.login, LOGIN_FAILED_NETWORK_ERROR, "state reflects network error");
+});
+
+add_task(function test_getKeysMissing() {
+  _("BrowserIDManager correctly handles getKeys succeeding but not returning keys.");
 
   let browseridManager = new BrowserIDManager();
   let identityConfig = makeIdentityConfig();
   // our mock identity config already has kA and kB - remove them or we never
   // try and fetch them.
   delete identityConfig.fxaccount.user.kA;
   delete identityConfig.fxaccount.user.kB;