Bug 983270 (part 2) - _findCluster() should return null on authentication errors. r=ckarlof

services/sync/modules/browserid_identity.js

@@ -634,7 +634,28 @@ BrowserIDClusterManager.prototype = {
     let cb = Async.makeSpinningCallback();
     promiseClusterURL().then(function (clusterURL) {
       cb(null, clusterURL);
-    }).then(null, cb);
+    }).then(
+      null, err => {
+      // service.js's verifyLogin() method will attempt to fetch a cluster
+      // URL when it sees a 401.  If it gets null, it treats it as a "real"
+      // auth error and sets Status.login to LOGIN_FAILED_LOGIN_REJECTED, which
+      // in turn causes a notification bar to appear informing the user they
+      // need to re-authenticate.
+      // On the other hand, if fetching the cluster URL fails with an exception,
+      // verifyLogin() assumes it is a transient error, and thus doesn't show
+      // the notification bar under the assumption the issue will resolve
+      // itself.
+      // Thus:
+      // * On a real 401, we must return null.
+      // * On any other problem we must let an exception bubble up.
+      if (err instanceof AuthenticationError) {
+        // callback with no error and a null result - cb.wait() returns null.
+        cb(null, null);
+      } else {
+        // callback with an error - cb.wait() completes by raising an exception.
+        cb(err);
+      }
+    });
     return cb.wait();
   },
 

services/sync/tests/unit/test_fxa_service_cluster.js

@@ -0,0 +1,68 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+Cu.import("resource://services-sync/service.js");
+Cu.import("resource://services-sync/util.js");
+Cu.import("resource://testing-common/services/sync/fxa_utils.js");
+Cu.import("resource://testing-common/services/sync/utils.js");
+
+add_task(function test_findCluster() {
+  _("Test FxA _findCluster()");
+
+  _("_findCluster() throws on 500 errors.");
+  initializeIdentityWithTokenServerResponse({
+    status: 500,
+    headers: [],
+    body: "",
+  });
+
+  yield Service.identity.initializeWithCurrentIdentity();
+  yield Assert_rejects(Service.identity.whenReadyToAuthenticate.promise,
+                       "should reject due to 500");
+
+  Assert.throws(function() {
+    Service._clusterManager._findCluster();
+  });
+
+  _("_findCluster() returns null on authentication errors.");
+  initializeIdentityWithTokenServerResponse({
+    status: 401,
+    headers: {"content-type": "application/json"},
+    body: "{}",
+  });
+
+  yield Service.identity.initializeWithCurrentIdentity();
+  yield Assert_rejects(Service.identity.whenReadyToAuthenticate.promise,
+                       "should reject due to 401");
+
+  cluster = Service._clusterManager._findCluster();
+  Assert.strictEqual(cluster, null);
+
+  _("_findCluster() works with correct tokenserver response.");
+  let endpoint = "http://example.com/something";
+  initializeIdentityWithTokenServerResponse({
+    status: 200,
+    headers: {"content-type": "application/json"},
+    body:
+      JSON.stringify({
+        api_endpoint: endpoint,
+        duration: 300,
+        id: "id",
+        key: "key",
+        uid: "uid",
+      })
+  });
+
+  yield Service.identity.initializeWithCurrentIdentity();
+  yield Service.identity.whenReadyToAuthenticate.promise;
+  cluster = Service._clusterManager._findCluster();
+  // The cluster manager ensures a trailing "/"
+  Assert.strictEqual(cluster, endpoint + "/");
+
+  Svc.Prefs.resetBranch("");
+});
+
+function run_test() {
+  initTestLogging();
+  run_next_test();
+}

services/sync/tests/unit/xpcshell.ini

@@ -124,6 +124,7 @@ skip-if = os == "android"
 
 # Firefox Accounts specific tests
 [test_fxa_startOver.js]
+[test_fxa_service_cluster.js]
 
 # Finally, we test each engine.
 [test_addons_engine.js]