Bug 1205111 - return a transient error on 401 fetching info/collections using FxA. r=rnewman

services/common/tokenserverclient.js

@@ -32,6 +32,9 @@ const PREF_LOG_LEVEL = "services.common.log.logger.tokenserverclient";
 this.TokenServerClientError = function TokenServerClientError(message) {
   this.name = "TokenServerClientError";
   this.message = message || "Client error.";
+  // Without explicitly setting .stack, all stacks from these errors will point
+  // to the "new Error()" call a few lines down, which isn't helpful.
+  this.stack = Error().stack;
 }
 TokenServerClientError.prototype = new Error();
 TokenServerClientError.prototype.constructor = TokenServerClientError;
@@ -52,6 +55,7 @@ this.TokenServerClientNetworkError =
  function TokenServerClientNetworkError(error) {
   this.name = "TokenServerClientNetworkError";
   this.error = error;
+  this.stack = Error().stack;
 }
 TokenServerClientNetworkError.prototype = new TokenServerClientError();
 TokenServerClientNetworkError.prototype.constructor =
@@ -96,6 +100,7 @@ this.TokenServerClientServerError =
   this.name = "TokenServerClientServerError";
   this.message = message || "Server error.";
   this.cause = cause;
+  this.stack = Error().stack;
 }
 TokenServerClientServerError.prototype = new TokenServerClientError();
 TokenServerClientServerError.prototype.constructor =

services/sync/modules-testing/utils.js

@@ -7,6 +7,7 @@
 this.EXPORTED_SYMBOLS = [
   "btoa", // It comes from a module import.
   "encryptPayload",
+  "isConfiguredWithLegacyIdentity",
   "ensureLegacyIdentityManager",
   "setBasicCredentials",
   "makeIdentityConfig",
@@ -94,6 +95,18 @@ this.waitForZeroTimer = function waitForZeroTimer(callback) {
   CommonUtils.namedTimer(wait, 150, {}, "timer");
 }
 
+/**
+ * Return true if Sync is configured with the "legacy" identity provider.
+ */
+this.isConfiguredWithLegacyIdentity = function() {
+  let ns = {};
+  Cu.import("resource://services-sync/service.js", ns);
+
+  // We can't use instanceof as BrowserIDManager (the "other" identity) inherits
+  // from IdentityManager so that would return true - so check the prototype.
+  return Object.getPrototypeOf(ns.Service.identity) === IdentityManager.prototype;
+}
+
 /**
   * Ensure Sync is configured with the "legacy" identity provider.
   */

services/sync/modules/browserid_identity.js

@@ -692,6 +692,10 @@ this.BrowserIDManager.prototype = {
   _getAuthenticationHeader: function(httpObject, method) {
     let cb = Async.makeSpinningCallback();
     this._ensureValidToken().then(cb, cb);
+    // Note that in failure states we return null, causing the request to be
+    // made without authorization headers, thereby presumably causing a 401,
+    // which causes Sync to log out. If we throw, this may not happen as
+    // expected.
     try {
       cb.wait();
     } catch (ex if !Async.isShutdownException(ex)) {
@@ -730,8 +734,17 @@ this.BrowserIDManager.prototype = {
 
   createClusterManager: function(service) {
     return new BrowserIDClusterManager(service);
-  }
+  },
 
+  // Tell Sync what the login status should be if it saw a 401 fetching
+  // info/collections as part of login verification (typically immediately
+  // after login.)
+  // In our case, it almost certainly means a transient error fetching a token
+  // (and hitting this will cause us to logout, which will correctly handle an
+  // authoritative login issue.)
+  loginStatusFromVerification404() {
+    return LOGIN_FAILED_NETWORK_ERROR;
+  },
 };
 
 /* An implementation of the ClusterManager for this identity
@@ -777,7 +790,7 @@ BrowserIDClusterManager.prototype = {
           // it's likely a 401 was received using the existing token - in which
           // case we just discard the existing token and fetch a new one.
           if (this.service.clusterURL) {
-            log.debug("_findCluster found existing clusterURL, so discarding the current token");
+            log.debug("_findCluster has a pre-existing clusterURL, so discarding the current token");
             this.identity._token = null;
           }
           return this.identity._ensureValidToken();

services/sync/modules/identity.js

@@ -590,4 +590,13 @@ IdentityManager.prototype = {
     // Do nothing for Sync 1.1.
     return {accepted: true};
   },
+
+  // Tell Sync what the login status should be if it saw a 401 fetching
+  // info/collections as part of login verification (typically immediately
+  // after login.)
+  // In our case it means an authoritative "password is incorrect".
+  loginStatusFromVerification404() {
+    return LOGIN_FAILED_LOGIN_REJECTED;
+  }
+
 };

services/sync/modules/service.js

@@ -765,8 +765,12 @@ Sync11Service.prototype = {
             return this.verifyLogin(false);
           }
 
-          // We must have the right cluster, but the server doesn't expect us
-          this.status.login = LOGIN_FAILED_LOGIN_REJECTED;
+          // We must have the right cluster, but the server doesn't expect us.
+          // The implications of this depend on the identity being used - for
+          // the legacy identity, it's an authoritatively "incorrect password",
+          // (ie, LOGIN_FAILED_LOGIN_REJECTED) but for FxA it probably means
+          // "transient error fetching auth token".
+          this.status.login = this.identity.loginStatusFromVerification404();
           return false;
 
         default:
@@ -990,6 +994,7 @@ Sync11Service.prototype = {
       }
 
       // Ask the identity manager to explicitly login now.
+      this._log.info("Logging in the user.");
       let cb = Async.makeSpinningCallback();
       this.identity.ensureLoggedIn().then(
         () => cb(null),
@@ -1005,9 +1010,9 @@ Sync11Service.prototype = {
           && (username || password || passphrase)) {
         Svc.Obs.notify("weave:service:setup-complete");
       }
-      this._log.info("Logging in the user.");
       this._updateCachedURLs();
 
+      this._log.info("User logged in successfully - verifying login.");
       if (!this.verifyLogin()) {
         // verifyLogin sets the failure states here.
         throw "Login failed: " + this.status.login;

services/sync/tests/unit/test_errorhandler.js

@@ -184,7 +184,10 @@ add_identity_test(this, function test_401_logout() {
       let errorCount = sumHistogram("WEAVE_STORAGE_AUTH_ERRORS", { key: "info/collections" });
       do_check_eq(errorCount, 2);
 
-      do_check_eq(Status.login, LOGIN_FAILED_LOGIN_REJECTED);
+      let expected = isConfiguredWithLegacyIdentity() ?
+                     LOGIN_FAILED_LOGIN_REJECTED : LOGIN_FAILED_NETWORK_ERROR;
+
+      do_check_eq(Status.login, expected);
       do_check_false(Service.isLoggedIn);
 
       // Clean up.

services/sync/tests/unit/test_syncscheduler.js

@@ -956,11 +956,22 @@ add_identity_test(this, function test_loginError_fatal_clearsTriggers() {
   Svc.Obs.add("weave:service:login:error", function onLoginError() {
     Svc.Obs.remove("weave:service:login:error", onLoginError);
     Utils.nextTick(function aLittleBitAfterLoginError() {
-      do_check_eq(Status.login, LOGIN_FAILED_LOGIN_REJECTED);
 
-      do_check_eq(scheduler.nextSync, 0);
-      do_check_eq(scheduler.syncTimer, null);
+      if (isConfiguredWithLegacyIdentity()) {
+        // for the "legacy" identity, a 401 on info/collections means the
+        // password is wrong, so we enter a "login rejected" state.
+        do_check_eq(Status.login, LOGIN_FAILED_LOGIN_REJECTED);
 
+        do_check_eq(scheduler.nextSync, 0);
+        do_check_eq(scheduler.syncTimer, null);
+      } else {
+        // For the FxA identity, a 401 on info/collections means a transient
+        // error, probably due to an inability to fetch a token.
+        do_check_eq(Status.login, LOGIN_FAILED_NETWORK_ERROR);
+        // syncs should still be scheduled.
+        do_check_true(scheduler.nextSync > Date.now());
+        do_check_true(scheduler.syncTimer.delay > 0);
+      }
       cleanUpAndGo(server).then(deferred.resolve);
     });
   });