Bug 983270 (part 2) - _findCluster() should return null on authentication errors. r=ckarlof
authorMark Hammond <mhammond@skippinet.com.au>
Mon, 17 Mar 2014 09:39:31 +1100
changeset 191040 bf2baa6d94aa7662b1ce9952543e4827648aab1a
parent 191039 aa19dcc93745d362451e5a2665f73fd66b9f74bf
child 191041 46355c54bd1f5866e764a548c3ab2a6c6b0db66e
push id3503
push userraliiev@mozilla.com
push dateMon, 28 Apr 2014 18:51:11 +0000
treeherdermozilla-beta@c95ac01e332e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersckarlof
bugs983270
milestone30.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 983270 (part 2) - _findCluster() should return null on authentication errors. r=ckarlof
services/sync/modules/browserid_identity.js
services/sync/tests/unit/test_fxa_service_cluster.js
services/sync/tests/unit/xpcshell.ini
--- a/services/sync/modules/browserid_identity.js
+++ b/services/sync/modules/browserid_identity.js
@@ -629,17 +629,38 @@ BrowserIDClusterManager.prototype = {
       ).then(
         () => endPointFromIdentityToken()
       );
     }.bind(this);
 
     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();
   },
 
   getUserBaseURL: function() {
     // Legacy Sync and FxA Sync construct the userBaseURL differently. Legacy
     // Sync appends path components onto an empty path, and in FxA Sync the
     // token server constructs this for us in an opaque manner. Since the
     // cluster manager already sets the clusterURL on Service and also has
new file mode 100644
--- /dev/null
+++ b/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();
+}
--- a/services/sync/tests/unit/xpcshell.ini
+++ b/services/sync/tests/unit/xpcshell.ini
@@ -119,16 +119,17 @@ skip-if = os == "android"
 [test_sendcredentials_controller.js]
 [test_status.js]
 [test_status_checkSetup.js]
 [test_syncscheduler.js]
 [test_upgrade_old_sync_key.js]
 
 # Firefox Accounts specific tests
 [test_fxa_startOver.js]
+[test_fxa_service_cluster.js]
 
 # Finally, we test each engine.
 [test_addons_engine.js]
 run-sequentially = Hardcoded port in static files.
 [test_addons_reconciler.js]
 [test_addons_store.js]
 run-sequentially = Hardcoded port in static files.
 [test_addons_tracker.js]