Bug 990834 (part 3) - Fix handling of hawk errors. r=ckarlof, a=sylvestre
authorMark Hammond <mhammond@skippinet.com.au>
Mon, 14 Apr 2014 11:53:29 +1000
changeset 183734 b074e386a410372f41afd3f7bbe9ab134db5442b
parent 183733 329a2a180a8b7d627dd2d42cd45b500aa707b8e1
child 183735 679aa869f39fc4e1ee84f856cc6ba51cfcf80936
push id3463
push usermhammond@skippinet.com.au
push dateMon, 14 Apr 2014 03:15:06 +0000
treeherdermozilla-beta@b074e386a410 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersckarlof, sylvestre
bugs990834
milestone29.0
Bug 990834 (part 3) - Fix handling of hawk errors. r=ckarlof, a=sylvestre Conflicts: services/fxaccounts/FxAccounts.jsm services/sync/tests/unit/test_browserid_identity.js
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
@@ -498,19 +498,22 @@ 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.whenKeysReadyPromise) {
         currentState.whenKeysReadyPromise = Promise.defer();
-        this.fetchAndUnwrapKeys(data.keyFetchToken).then(data => {
-          currentState.whenKeysReadyPromise.resolve(data);
-        });
+        this.fetchAndUnwrapKeys(data.keyFetchToken).then(
+          data => {
+            currentState.whenKeysReadyPromise.resolve(data);
+          },
+          err => currentState.whenKeysReadyPromise.reject(err)
+        );
       }
       return currentState.whenKeysReadyPromise.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,16 +504,58 @@ 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_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");
+});
+
 // End of tests
 // Utility functions follow
 
 // Create a new browserid_identity object and initialize it with a
 // hawk mock that simulates HTTP responses.
 // The callback function will be called each time the mocked hawk server wants
 // to make a request.  The result of the callback should be the mock response
 // object that will be returned to hawk.