Bug 967015 - Have bid_identity call Service.startOver on logout. r=rnewman,ckarlof

services/sync/modules-testing/utils.js

@@ -117,6 +117,9 @@ this.configureFxAccountIdentity = function(authService,
   let MockInternal = {};
   let fxa = new FxAccounts(MockInternal);
 
+  // until we get better test infrastructure for bid_identity, we set the
+  // signedin user's "email" to the username, simply as many tests rely on this.
+  config.fxaccount.user.email = config.username;
   fxa.internal.currentAccountState.signedInUser = {
     version: DATA_FORMAT_VERSION,
     accountData: config.fxaccount.user
@@ -139,6 +142,7 @@ this.configureFxAccountIdentity = function(authService,
   authService._tokenServerClient = mockTSC;
   // Set the "account" of the browserId manager to be the "email" of the
   // logged in user of the mockFXA service.
+  authService._signedInUser = fxa.internal.currentAccountState.signedInUser.accountData;
   authService._account = config.fxaccount.user.email;
 }
 

services/sync/modules/browserid_identity.js

@@ -32,12 +32,6 @@ XPCOMUtils.defineLazyModuleGetter(this, "BulkKeyBundle",
 XPCOMUtils.defineLazyModuleGetter(this, "fxAccounts",
                                   "resource://gre/modules/FxAccounts.jsm");
 
-XPCOMUtils.defineLazyGetter(this, 'fxAccountsCommon', function() {
-  let ob = {};
-  Cu.import("resource://gre/modules/FxAccountsCommon.js", ob);
-  return ob;
-});
-
 XPCOMUtils.defineLazyGetter(this, 'log', function() {
   let log = Log.repository.getLogger("Sync.BrowserIDManager");
   log.addAppender(new Log.DumpAppender());
@@ -45,6 +39,15 @@ XPCOMUtils.defineLazyGetter(this, 'log', function() {
   return log;
 });
 
+// FxAccountsCommon.js doesn't use a "namespace", so create one here.
+let fxAccountsCommon = {};
+Cu.import("resource://gre/modules/FxAccountsCommon.js", fxAccountsCommon);
+
+const OBSERVER_TOPICS = [
+  fxAccountsCommon.ONLOGIN_NOTIFICATION,
+  fxAccountsCommon.ONLOGOUT_NOTIFICATION,
+];
+
 const PREF_SYNC_SHOW_CUSTOMIZATION = "services.sync.ui.showCustomizationDialog";
 
 function deriveKeyBundle(kB) {
@@ -90,7 +93,7 @@ this.BrowserIDManager.prototype = {
   _tokenServerClient: null,
   // https://docs.services.mozilla.com/token/apis.html
   _token: null,
-  _account: null,
+  _signedInUser: null, // the signedinuser we got from FxAccounts.
 
   // null if no error, otherwise a LOGIN_FAILED_* value that indicates why
   // we failed to authenticate (but note it might not be an actual
@@ -116,9 +119,9 @@ this.BrowserIDManager.prototype = {
   },
 
   initialize: function() {
-    Services.obs.addObserver(this, fxAccountsCommon.ONLOGIN_NOTIFICATION, false);
-    Services.obs.addObserver(this, fxAccountsCommon.ONLOGOUT_NOTIFICATION, false);
-    Services.obs.addObserver(this, "weave:service:logout:finish", false);
+    for (let topic of OBSERVER_TOPICS) {
+      Services.obs.addObserver(this, topic, false);
+    }
     return this.initializeWithCurrentIdentity();
   },
 
@@ -150,6 +153,16 @@ this.BrowserIDManager.prototype = {
     return this.whenReadyToAuthenticate.promise;
   },
 
+  finalize: function() {
+    // After this is called, we can expect Service.identity != this.
+    for (let topic of OBSERVER_TOPICS) {
+      Services.obs.removeObserver(this, topic);
+    }
+    this.resetCredentials();
+    this._signedInUser = null;
+    return Promise.resolve();
+  },
+
   initializeWithCurrentIdentity: function(isInitialSync=false) {
     // While this function returns a promise that resolves once we've started
     // the auth process, that process is complete when
@@ -164,19 +177,21 @@ this.BrowserIDManager.prototype = {
     return this._fxaService.getSignedInUser().then(accountData => {
       if (!accountData) {
         this._log.info("initializeWithCurrentIdentity has no user logged in");
-        this._account = null;
+        this.account = null;
+        // and we are as ready as we can ever be for auth.
         this._shouldHaveSyncKeyBundle = true;
         this.whenReadyToAuthenticate.reject("no user is logged in");
         return;
       }
 
-      this._account = accountData.email;
+      this.account = accountData.email;
+      this._updateSignedInUser(accountData);
       // The user must be verified before we can do anything at all; we kick
       // this and the rest of initialization off in the background (ie, we
       // don't return the promise)
       this._log.info("Waiting for user to be verified.");
       this._fxaService.whenVerified(accountData).then(accountData => {
-        // We do the background keybundle fetch...
+        this._updateSignedInUser(accountData);
         this._log.info("Starting fetch for key bundle.");
         if (this.needsCustomization) {
           // If the user chose to "Customize sync options" when signing
@@ -220,32 +235,50 @@ this.BrowserIDManager.prototype = {
     });
   },
 
+  _updateSignedInUser: function(userData) {
+    // This object should only ever be used for a single user.  It is an
+    // error to update the data if the user changes (but updates are still
+    // necessary, as each call may add more attributes to the user).
+    // We start with no user, so an initial update is always ok.
+    if (this._signedInUser && this._signedInUser.email != userData.email) {
+      throw new Error("Attempting to update to a different user.")
+    }
+    this._signedInUser = userData;
+  },
+
+  logout: function() {
+    // This will be called when sync fails (or when the account is being
+    // unlinked etc).  It may have failed because we got a 401 from a sync
+    // server, so we nuke the token.  Next time sync runs and wants an
+    // authentication header, we will notice the lack of the token and fetch a
+    // new one.
+    this._token = null;
+  },
+
   observe: function (subject, topic, data) {
     this._log.debug("observed " + topic);
     switch (topic) {
     case fxAccountsCommon.ONLOGIN_NOTIFICATION:
+      // This should only happen if we've been initialized without a current
+      // user - otherwise we'd have seen the LOGOUT notification and been
+      // thrown away.
+      // The exception is when we've initialized with a user that needs to
+      // reauth with the server - in that case we will also get here, but
+      // should have the same identity.
+      // initializeWithCurrentIdentity will throw and log if these contraints
+      // aren't met, so just go ahead and do the init.
       this.initializeWithCurrentIdentity(true);
       break;
 
     case fxAccountsCommon.ONLOGOUT_NOTIFICATION:
-      // Setting .username calls resetCredentials which drops the key bundle
-      // and resets _shouldHaveSyncKeyBundle.
-      this.username = "";
-      this._account = null;
-      Weave.Service.logout();
-      break;
-
-    case "weave:service:logout:finish":
-      // This signals an auth error with the storage server,
-      // or that the user unlinked her account from the browser.
-      // Either way, we clear our auth token. In the case of an
-      // auth error, this will force the fetch of a new one.
-      this._token = null;
+      Weave.Service.startOver();
+      // startOver will cause this instance to be thrown away, so there's
+      // nothing else to do.
       break;
     }
   },
 
-   /**
+  /**
    * Compute the sha256 of the message bytes.  Return bytes.
    */
   _sha256: function(message) {
@@ -275,23 +308,9 @@ this.BrowserIDManager.prototype = {
     return this._fxaService.localtimeOffsetMsec;
   },
 
-  get account() {
-    return this._account;
-  },
-
-  /**
-   * Sets the active account name.
-   *
-   * This should almost always be called in favor of setting username, as
-   * username is derived from account.
-   *
-   * Changing the account name has the side-effect of wiping out stored
-   * credentials.
-   *
-   * Set this value to null to clear out identity information.
-   */
-  set account(value) {
-    throw "account setter should be not used in BrowserIDManager";
+  usernameFromAccount: function(val) {
+    // we don't differentiate between "username" and "account"
+    return val;
   },
 
   /**
@@ -349,8 +368,8 @@ this.BrowserIDManager.prototype = {
    * Resets/Drops all credentials we hold for the current user.
    */
   resetCredentials: function() {
-    // the only credentials we hold are the sync key.
     this.resetSyncKey();
+    this._token = null;
   },
 
   /**
@@ -393,8 +412,8 @@ this.BrowserIDManager.prototype = {
   },
 
   /**
-   * Do we have a non-null, not yet expired token whose email field
-   * matches (when normalized) our account field?
+   * Do we have a non-null, not yet expired token for the user currently
+   * signed in?
    */
   hasValidToken: function() {
     if (!this._token) {
@@ -403,56 +422,15 @@ this.BrowserIDManager.prototype = {
     if (this._token.expiration < this._now()) {
       return false;
     }
-    let signedInUser = this._getSignedInUser();
-    if (!signedInUser) {
-      return false;
-    }
-    // Does the signed in user match the user we retrieved the token for?
-    if (signedInUser.email !== this.account) {
-      return false;
-    }
     return true;
   },
 
-  /**
-   * Wrap and synchronize FxAccounts.getSignedInUser().
-   *
-   * @return credentials per wrapped.
-   */
-  _getSignedInUser: function() {
-    let userData;
-    let cb = Async.makeSpinningCallback();
-
-    this._fxaService.getSignedInUser().then(function (result) {
-        cb(null, result);
-    },
-    function (err) {
-        cb(err);
-    });
-
-    try {
-      userData = cb.wait();
-    } catch (err) {
-      this._log.error("FxAccounts.getSignedInUser() failed with: " + err);
-      return null;
-    }
-    return userData;
-  },
-
   _fetchSyncKeyBundle: function() {
     // Fetch a sync token for the logged in user from the token server.
     return this._fxaService.getKeys().then(userData => {
-      // Unlikely, but if the logged in user somehow changed between these
-      // calls we better fail. TODO: add tests for these
-      if (!userData) {
-        throw new AuthenticationError("No userData in _fetchSyncKeyBundle");
-      } else if (userData.email !== this.account) {
-        throw new AuthenticationError("Unexpected user change in _fetchSyncKeyBundle");
-      }
-      return this._fetchTokenForUser(userData).then(token => {
+      this._updateSignedInUser(userData); // throws if the user changed.
+      return this._fetchTokenForUser().then(token => {
         this._token = token;
-        // Set the username to be the uid returned by the token server.
-        this.username = this._token.uid.toString();
         // both Jelly and FxAccounts give us kA/kB as hex.
         let kB = Utils.hexToBytes(userData.kB);
         this._syncKeyBundle = deriveKeyBundle(kB);
@@ -461,12 +439,13 @@ this.BrowserIDManager.prototype = {
     });
   },
 
-  // Refresh the sync token for the specified Firefox Accounts user.
-  _fetchTokenForUser: function(userData) {
+  // 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);
@@ -508,7 +487,7 @@ this.BrowserIDManager.prototype = {
 
     // wait until the account email is verified and we know that
     // getAssertion() will return a real assertion (not null).
-    return fxa.whenVerified(userData)
+    return fxa.whenVerified(this._signedInUser)
       .then(() => getAssertion())
       .then(assertion => getToken(tokenServerURI, assertion))
       .then(token => {
@@ -541,22 +520,17 @@ this.BrowserIDManager.prototype = {
       });
   },
 
-  _fetchTokenForLoggedInUserSync: function() {
-    let cb = Async.makeSpinningCallback();
-
-    this._fxaService.getSignedInUser().then(userData => {
-      this._fetchTokenForUser(userData).then(token => {
-        cb(null, token);
-      }, err => {
-        cb(err);
-      });
-    });
-    try {
-      return cb.wait();
-    } catch (err) {
-      this._log.info("_fetchTokenForLoggedInUserSync: " + err.message);
-      return null;
+  // Returns a promise that is resolved when we have a valid token for the
+  // current user stored in this._token.  When resolved, this._token is valid.
+  _ensureValidToken: function() {
+    if (this.hasValidToken()) {
+      return Promise.resolve();
     }
+    return this._fetchTokenForUser().then(
+      token => {
+        this._token = token;
+      }
+    );
   },
 
   getResourceAuthenticator: function () {
@@ -575,12 +549,16 @@ this.BrowserIDManager.prototype = {
    * of a RESTRequest or AsyncResponse object.
    */
   _getAuthenticationHeader: function(httpObject, method) {
-    if (!this.hasValidToken()) {
-      // Refresh token for the currently logged in FxA user
-      this._token = this._fetchTokenForLoggedInUserSync();
-      if (!this._token) {
-        return null;
-      }
+    let cb = Async.makeSpinningCallback();
+    this._ensureValidToken().then(cb, cb);
+    try {
+      cb.wait();
+    } catch (ex) {
+      this._log.error("Failed to fetch a token for authentication: " + ex);
+      return null;
+    }
+    if (!this._token) {
+      return null;
     }
     let credentials = {algorithm: "sha256",
                        id: this._token.id,
@@ -626,29 +604,30 @@ BrowserIDClusterManager.prototype = {
   __proto__: ClusterManager.prototype,
 
   _findCluster: function() {
-    let fxa = this.identity._fxaService; // will be mocked for tests.
+    let endPointFromIdentityToken = function() {
+      let endpoint = this.identity._token.endpoint;
+      // For Sync 1.5 storage endpoints, we use the base endpoint verbatim.
+      // However, it should end in "/" because we will extend it with
+      // well known path components. So we add a "/" if it's missing.
+      if (!endpoint.endsWith("/")) {
+        endpoint += "/";
+      }
+      return endpoint;
+    }.bind(this);
+
+    // Spinningly ensure we are ready to authenticate and have a valid token.
     let promiseClusterURL = function() {
-      return fxa.getSignedInUser().then(userData => {
-        return this.identity._fetchTokenForUser(userData).then(token => {
-          let endpoint = token.endpoint;
-          // For Sync 1.5 storage endpoints, we use the base endpoint verbatim.
-          // However, it should end in "/" because we will extend it with
-          // well known path components. So we add a "/" if it's missing.
-          if (!endpoint.endsWith("/")) {
-            endpoint += "/";
-          }
-          return endpoint;
-        });
-      });
+      return this.identity.whenReadyToAuthenticate.promise.then(
+        () => this.identity._ensureValidToken()
+      ).then(
+        () => endPointFromIdentityToken()
+      );
     }.bind(this);
 
     let cb = Async.makeSpinningCallback();
     promiseClusterURL().then(function (clusterURL) {
-        cb(null, clusterURL);
-    },
-    function (err) {
-        cb(err);
-    });
+      cb(null, clusterURL);
+    }).then(null, cb);
     return cb.wait();
   },
 

services/sync/modules/identity.js

@@ -89,10 +89,22 @@ IdentityManager.prototype = {
    * its state
    */
   initialize: function() {
-    // nothing to do for this identity provider
+    // Nothing to do for this identity provider.
     return Promise.resolve();
   },
 
+  finalize: function() {
+    // Nothing to do for this identity provider.
+    return Promise.resolve();
+  },
+
+  /**
+   * Called whenever Service.logout() is called.
+   */
+  logout: function() {
+    // nothing to do for this identity provider.
+  },
+
   /**
    * Ensure the user is logged in.  Returns a promise that resolves when
    * the user is logged in, or is rejected if the login attempt has failed.

services/sync/modules/service.js

@@ -164,7 +164,7 @@ Sync11Service.prototype = {
 
   _updateCachedURLs: function _updateCachedURLs() {
     // Nothing to cache yet if we don't have the building blocks
-    if (this.clusterURL == "" || this.identity.username == "")
+    if (!this.clusterURL || !this.identity.username)
       return;
 
     this._log.debug("Caching URLs under storage user base: " + this.userBaseURL);
@@ -852,14 +852,6 @@ Sync11Service.prototype = {
     Svc.Obs.notify("weave:engine:stop-tracking");
     this.status.resetSync();
 
-    // We want let UI consumers of the following notification know as soon as
-    // possible, so let's fake for the CLIENT_NOT_CONFIGURED status for now
-    // by emptying the passphrase (we still need the password).
-    this.identity.resetSyncKey();
-    this.status.login = LOGIN_FAILED_NO_PASSPHRASE;
-    this.logout();
-    Svc.Obs.notify("weave:service:start-over");
-
     // Deletion doesn't make sense if we aren't set up yet!
     if (this.clusterURL != "") {
       // Clear client-specific data from the server, including disabled engines.
@@ -871,10 +863,20 @@ Sync11Service.prototype = {
                          + Utils.exceptionStr(ex));
         }
       }
+      this._log.debug("Finished deleting client data.");
     } else {
       this._log.debug("Skipping client data removal: no cluster URL.");
     }
 
+    // We want let UI consumers of the following notification know as soon as
+    // possible, so let's fake for the CLIENT_NOT_CONFIGURED status for now
+    // by emptying the passphrase (we still need the password).
+    this._log.info("Service.startOver dropping sync key and logging out.");
+    this.identity.resetSyncKey();
+    this.status.login = LOGIN_FAILED_NO_PASSPHRASE;
+    this.logout();
+    Svc.Obs.notify("weave:service:start-over");
+
     // Reset all engines and clear keys.
     this.resetClient();
     this.collectionKeys.clear();
@@ -900,21 +902,23 @@ Sync11Service.prototype = {
       return;
     }
 
-    this.identity.username = "";
-    Services.prefs.clearUserPref("services.sync.fxaccounts.enabled");
-    this.status.__authManager = null;
-    this.identity = Status._authManager;
-    this._clusterManager = this.identity.createClusterManager(this);
-
-    // Tell the new identity manager to initialize itself
-    this.identity.initialize().then(() => {
-      Svc.Obs.notify("weave:service:start-over:finish");
-    }).then(null, err => {
-      this._log.error("startOver failed to re-initialize the identity manager: " + err);
-      // Still send the observer notification so the current state is
-      // reflected in the UI.
-      Svc.Obs.notify("weave:service:start-over:finish");
-    });
+    this.identity.finalize().then(
+      () => {
+        this.identity.username = "";
+        Services.prefs.clearUserPref("services.sync.fxaccounts.enabled");
+        this.status.__authManager = null;
+        this.identity = Status._authManager;
+        this._clusterManager = this.identity.createClusterManager(this);
+        Svc.Obs.notify("weave:service:start-over:finish");
+      }
+    ).then(null,
+      err => {
+        this._log.error("startOver failed to re-initialize the identity manager: " + err);
+        // Still send the observer notification so the current state is
+        // reflected in the UI.
+        Svc.Obs.notify("weave:service:start-over:finish");
+      }
+    );
   },
 
   persistLogin: function persistLogin() {
@@ -985,6 +989,7 @@ Sync11Service.prototype = {
       return;
 
     this._log.info("Logging out");
+    this.identity.logout();
     this._loggedIn = false;
 
     Svc.Obs.notify("weave:service:logout:finish");

services/sync/tests/unit/test_browserid_identity.js

@@ -72,6 +72,17 @@ add_test(function test_initial_state() {
   }
 );
 
+add_task(function test_initialializeWithCurrentIdentity() {
+    _("Verify start after initializeWithCurrentIdentity");
+    browseridManager.initializeWithCurrentIdentity();
+    yield browseridManager.whenReadyToAuthenticate.promise;
+    do_check_true(!!browseridManager._token);
+    do_check_true(browseridManager.hasValidToken());
+    do_check_eq(browseridManager.account, identityConfig.fxaccount.user.email);
+  }
+);
+
+
 add_test(function test_getResourceAuthenticator() {
     _("BrowserIDManager supplies a Resource Authenticator callback which returns a Hawk header.");
     let authenticator = browseridManager.getResourceAuthenticator();
@@ -85,7 +96,6 @@ add_test(function test_getResourceAuthenticator() {
     do_check_true(output.headers.authorization.startsWith('Hawk'));
     _("Expected internal state after successful call.");
     do_check_eq(browseridManager._token.uid, identityConfig.fxaccount.token.uid);
-    do_check_eq(browseridManager.account, identityConfig.fxaccount.user.email);
     run_next_test();
   }
 );
@@ -291,24 +301,6 @@ add_test(function test_tokenExpiration() {
   }
 );
 
-add_test(function test_userChangeAndLogOut() {
-    _("BrowserIDManager notices when the FxAccounts.getSignedInUser().email changes.");
-    let bidUser = new BrowserIDManager();
-    configureFxAccountIdentity(bidUser, identityConfig);
-    let request = new SyncStorageRequest(
-      "https://example.net/somewhere/over/the/rainbow");
-    let authenticator = bidUser.getRESTRequestAuthenticator();
-    do_check_true(!!authenticator);
-    let output = authenticator(request, 'GET');
-    do_check_true(!!output);
-    do_check_eq(bidUser.account, identityConfig.fxaccount.user.email);
-    do_check_true(bidUser.hasValidToken());
-    identityConfig.fxaccount.user.email = "something@new";
-    do_check_false(bidUser.hasValidToken());
-    run_next_test();
-  }
-);
-
 add_test(function test_sha256() {
   // Test vectors from http://www.bichlmeier.info/sha256test.html
   let vectors = [
@@ -475,6 +467,7 @@ function* initializeIdentityWithHAWKFailure(response) {
   };
 
   browseridManager._fxaService = fxa;
+  browseridManager._signedInUser = null;
   yield browseridManager.initializeWithCurrentIdentity();
   try {
     yield browseridManager.whenReadyToAuthenticate.promise;

services/sync/tests/unit/test_errorhandler.js

@@ -126,13 +126,14 @@ function sync_httpd_setup() {
 }
 
 function setUp(server) {
-  let deferred = Promise.defer();
-  configureIdentity({username: "johndoe"}).then(() => {
-    deferred.resolve(generateAndUploadKeys());
-  });
-  Service.serverURL  = server.baseURI + "/";
-  Service.clusterURL = server.baseURI + "/";
-  return deferred.promise;
+  return configureIdentity({username: "johndoe"}).then(
+    () => {
+      Service.serverURL  = server.baseURI + "/";
+      Service.clusterURL = server.baseURI + "/";
+    }
+  ).then(
+    () => generateAndUploadKeys()
+  );
 }
 
 function generateAndUploadKeys() {

services/sync/tests/unit/test_fxa_startOver.js

@@ -32,7 +32,7 @@ add_task(function* test_startover() {
   do_check_true(Service.clusterURL.length > 0);
 
   // remember some stuff so we can reset it after.
-  let oldIdentidy = Service.identity;
+  let oldIdentity = Service.identity;
   let oldClusterManager = Service._clusterManager;
   let deferred = Promise.defer();
   Services.obs.addObserver(function observeStartOverFinished() {
@@ -41,7 +41,7 @@ add_task(function* test_startover() {
   }, "weave:service:start-over:finish", false);
 
   Service.startOver();
-  yield deferred; // wait for the observer to fire.
+  yield deferred.promise; // wait for the observer to fire.
 
   // should have reset the pref that indicates if FxA is enabled.
   do_check_true(Services.prefs.getBoolPref("services.sync.fxaccounts.enabled"));
@@ -52,10 +52,11 @@ add_task(function* test_startover() {
   // should have clobbered the cluster URL
   do_check_eq(Service.clusterURL, "");
 
-  // reset the world.
-  Service.identity = oldIdentity = Service.identity;
-  Service._clusterManager = Service._clusterManager;
-  Services.prefs.setBoolPref("services.sync.fxaccounts.enabled", false);
+  // we should have thrown away the old identity provider and cluster manager.
+  do_check_neq(oldIdentity, Service.identity);
+  do_check_neq(oldClusterManager, Service._clusterManager);
 
+  // reset the world.
+  Services.prefs.setBoolPref("services.sync.fxaccounts.enabled", false);
   Services.prefs.setBoolPref("services.sync-testing.startOverKeepIdentity", oldValue);
 });

services/sync/tests/unit/test_service_wipeClient.js

@@ -82,9 +82,9 @@ add_test(function test_credentials_preserved() {
   _("Ensure that credentials are preserved if client is wiped.");
 
   // Required for wipeClient().
-  Service.clusterURL = "http://dummy:9000/";
   Service.identity.account = "testaccount";
   Service.identity.basicPassword = "testpassword";
+  Service.clusterURL = "http://dummy:9000/";
   let key = Utils.generatePassphrase();
   Service.identity.syncKey = key;
   Service.identity.persistCredentials();