Bug 1582633 - allow an FxA user to be signed in without sync being enabled. r=eoger,lina

Differential Revision: https://phabricator.services.mozilla.com/D46572

--HG--
extra : moz-landing-system : lando

browser/base/content/browser.js

@@ -649,7 +649,7 @@ var gNavigatorBundle = {
   },
 };
 
-function updateFxaToolbarMenu(enable) {
+function updateFxaToolbarMenu(enable, isInitialUpdate = false) {
   // We only show the Firefox Account toolbar menu if the feature is enabled and
   // if sync is enabled.
   const syncEnabled = Services.prefs.getBoolPref(
@@ -670,7 +670,9 @@ function updateFxaToolbarMenu(enable) {
     // We have to manually update the sync state UI when toggling the FxA toolbar
     // because it could show an invalid icon if the user is logged in and no sync
     // event was performed yet.
-    gSync.maybeUpdateUIState();
+    if (!isInitialUpdate) {
+      gSync.maybeUpdateUIState();
+    }
 
     Services.telemetry.setEventRecordingEnabled("fxa_avatar_menu", true);
 
@@ -1811,7 +1813,7 @@ var gBrowserInit = {
 
     this._setInitialFocus();
 
-    updateFxaToolbarMenu(gFxaToolbarEnabled);
+    updateFxaToolbarMenu(gFxaToolbarEnabled, true);
   },
 
   onLoad() {

services/fxaccounts/FxAccounts.jsm

@@ -580,22 +580,7 @@ class FxAccounts {
         await this.signOut();
         return null;
       }
-      if (this._internal.isUserEmailVerified(data)) {
-        // This is a work-around for preferences being reset (bug 1550967).
-        // Many things check this preference as a flag for "is sync configured",
-        // and if not, we try and avoid loading these modules at all. So if a user
-        // is signed in but this pref isn't set, things go weird.
-        // However, some thing do unconditionally load fxaccounts, such as
-        // about:prefs. When that happens we can detect the state and re-add the
-        // pref. Note that we only do this for verified users as that's what sync
-        // does (ie, if the user is unverified, sync will set it on verification)
-        if (
-          !Services.prefs.prefHasUserValue("services.sync.username") &&
-          data.email
-        ) {
-          Services.prefs.setStringPref("services.sync.username", data.email);
-        }
-      } else {
+      if (!this._internal.isUserEmailVerified(data)) {
         // If the email is not verified, start polling for verification,
         // but return null right away.  We don't want to return a promise
         // that might not be fulfilled for a long time.

services/fxaccounts/FxAccountsKeys.jsm

@@ -33,6 +33,10 @@ class FxAccountsKeys {
       if (!userData) {
         throw new Error("Can't possibly get keys; User is not signed in");
       }
+      if (!userData.verified) {
+        log.info("Can't get keys; user is not verified");
+        return false;
+      }
       // - keyFetchToken means we can almost certainly grab them.
       // - kSync, kXCS, kExtSync and kExtKbHash means we already have them.
       // - kB is deprecated but |getKeys| will help us migrate to kSync and friends.

services/fxaccounts/FxAccountsWebChannel.jsm

@@ -398,6 +398,7 @@ this.FxAccountsWebChannelHelpers = function(options) {
   options = options || {};
 
   this._fxAccounts = options.fxAccounts || fxAccounts;
+  this._weaveXPCOM = options.weaveXPCOM || null;
   this._privateBrowsingUtils =
     options.privateBrowsingUtils || PrivateBrowsingUtils;
 };
@@ -419,9 +420,10 @@ this.FxAccountsWebChannelHelpers.prototype = {
    *
    * @param accountData the user's account data and credentials
    */
-  login(accountData) {
+  async login(accountData) {
     // We don't act on customizeSync anymore, it used to open a dialog inside
     // the browser to selecte the engines to sync but we do it on the web now.
+    log.debug("Webchannel is logging a user in.");
     delete accountData.customizeSync;
 
     if (accountData.offeredSyncEngines) {
@@ -456,11 +458,19 @@ this.FxAccountsWebChannelHelpers.prototype = {
 
     // A sync-specific hack - we want to ensure sync has been initialized
     // before we set the signed-in user.
-    let xps = Cc["@mozilla.org/weave/service;1"].getService(Ci.nsISupports)
-      .wrappedJSObject;
-    return xps.whenLoaded().then(() => {
-      return this._fxAccounts._internal.setSignedInUser(accountData);
-    });
+    // XXX - probably not true any more, especially now we have observerPreloads
+    // in FxAccounts.jsm?
+    let xps =
+      this._weaveXPCOM ||
+      Cc["@mozilla.org/weave/service;1"].getService(Ci.nsISupports)
+        .wrappedJSObject;
+    await xps.whenLoaded();
+    await this._fxAccounts._internal.setSignedInUser(accountData);
+    // Configure sync itself if necessary, but after signing in the user.
+    // (Soon we'll have a way of making the sync configuration optional, based
+    // on the content of the message, but for now, sync is unconditionally
+    // configured)
+    await xps.Weave.Service.configure();
   },
 
   /**

services/fxaccounts/tests/xpcshell/test_web_channel.js

@@ -389,6 +389,14 @@ add_task(async function test_helpers_login_without_customize_sync() {
         },
       },
     },
+    weaveXPCOM: {
+      whenLoaded() {},
+      Weave: {
+        Service: {
+          configure() {},
+        },
+      },
+    },
   });
 
   // ensure the previous account pref is overwritten.
@@ -418,6 +426,14 @@ add_task(async function test_helpers_login_with_customize_sync() {
         },
       },
     },
+    weaveXPCOM: {
+      whenLoaded() {},
+      Weave: {
+        Service: {
+          configure() {},
+        },
+      },
+    },
   });
 
   await helpers.login({
@@ -469,6 +485,14 @@ add_task(
           },
         },
       },
+      weaveXPCOM: {
+        whenLoaded() {},
+        Weave: {
+          Service: {
+            configure() {},
+          },
+        },
+      },
     });
 
     Assert.equal(
@@ -512,6 +536,14 @@ add_task(async function test_helpers_login_with_offered_sync_engines() {
           },
         },
       },
+      weaveXPCOM: {
+        whenLoaded() {},
+        Weave: {
+          Service: {
+            configure() {},
+          },
+        },
+      },
     });
   });
 

services/sync/Weave.jsm

@@ -76,11 +76,14 @@ WeaveService.prototype = {
     Ci.nsISupportsWeakReference,
   ]),
 
-  ensureLoaded() {
+  get Weave() {
     const { Weave } = ChromeUtils.import("resource://services-sync/main.js");
+    return Weave;
+  },
 
+  ensureLoaded() {
     // Side-effect of accessing the service is that it is instantiated.
-    Weave.Service;
+    this.Weave.Service;
   },
 
   whenLoaded() {

services/sync/modules-testing/utils.js

@@ -243,8 +243,7 @@ var configureIdentity = async function(identityOverrides, server) {
   }
 
   configureFxAccountIdentity(ns.Service.identity, config);
-  // because we didn't send any FxA LOGIN notifications we must set the username.
-  ns.Service.identity.username = config.username;
+  Services.prefs.setStringPref("services.sync.username", config.username);
   // many of these tests assume all the auth stuff is setup and don't hit
   // a path which causes that auth to magically happen - so do it now.
   await ns.Service.identity._ensureValidToken();

services/sync/modules/UIState.jsm

@@ -74,13 +74,14 @@ const UIStateInternal = {
 
   init() {
     this._initialized = true;
-    if (!Services.prefs.prefHasUserValue("services.sync.username")) {
-      return;
-    }
-    // Refresh the state in the background.
-    this.refreshState().catch(e => {
-      Cu.reportError(e);
-    });
+    // Because the FxA toolbar is usually visible, this module gets loaded at
+    // browser startup, and we want to avoid pulling in all of FxA or Sync at
+    // that time, so we refresh the state after the browser has settled.
+    Services.tm.idleDispatchToMainThread(() => {
+      this.refreshState().catch(e => {
+        Cu.reportError(e);
+      });
+    }, 2000);
   },
 
   // Used for testing.
@@ -111,6 +112,12 @@ const UIStateInternal = {
   async refreshState() {
     const newState = {};
     await this._refreshFxAState(newState);
+    // Optimize the "not signed in" case to avoid refreshing twice just after
+    // startup - if there's currently no _state, and we still aren't configured,
+    // just early exit.
+    if (this._state == null && newState.status == DEFAULT_STATE.status) {
+      return this.state;
+    }
     if (newState.syncEnabled) {
       this._setLastSyncTime(newState); // We want this in case we change accounts.
     }

services/sync/modules/browserid_identity.js

@@ -74,6 +74,7 @@ const OBSERVER_TOPICS = [
   fxAccountsCommon.ONVERIFIED_NOTIFICATION,
   fxAccountsCommon.ONLOGOUT_NOTIFICATION,
   fxAccountsCommon.ON_ACCOUNT_STATE_CHANGE_NOTIFICATION,
+  "weave:connected",
 ];
 
 /*
@@ -118,7 +119,9 @@ this.BrowserIDManager.prototype = {
   _tokenServerClient: null,
   // https://docs.services.mozilla.com/token/apis.html
   _token: null,
-  _signedInUser: null, // the signedinuser we got from FxAccounts.
+  // protection against the user changing underneath us - the uid
+  // of the current user.
+  _userUid: null,
 
   hashedUID() {
     if (!this._hashedUID) {
@@ -142,18 +145,21 @@ this.BrowserIDManager.prototype = {
       Services.obs.removeObserver(this.asyncObserver, topic);
     }
     this.resetCredentials();
-    this._signedInUser = null;
+    this._userUid = null;
   },
 
-  _updateSignedInUser(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.uid != userData.uid) {
-      throw new Error("Attempting to update to a different user.");
+  async getSignedInUser() {
+    let data = await this._fxaService.getSignedInUser();
+    if (!data) {
+      this._userUid = null;
+      return null;
     }
-    this._signedInUser = userData;
+    if (this._userUid == null) {
+      this._userUid = data.uid;
+    } else if (this._userUid != data.uid) {
+      throw new Error("The signed in user has changed");
+    }
+    return data;
   },
 
   logout() {
@@ -167,12 +173,16 @@ this.BrowserIDManager.prototype = {
 
   async observe(subject, topic, data) {
     this._log.debug("observed " + topic);
+    if (!this.username) {
+      this._log.info("Sync is not configured, so ignoring the notification");
+      return;
+    }
     switch (topic) {
+      case "weave:connected":
       case fxAccountsCommon.ONLOGIN_NOTIFICATION: {
-        this._log.info("A user has logged in");
+        this._log.info("Sync has been connected to a logged in user");
         this.resetCredentials();
-        let accountData = await this._fxaService.getSignedInUser();
-        this._updateSignedInUser(accountData);
+        let accountData = await this.getSignedInUser();
 
         if (!accountData.verified) {
           // wait for a verified notification before we kick sync off.
@@ -184,10 +194,6 @@ this.BrowserIDManager.prototype = {
       // intentional fall-through - the user is verified.
       case fxAccountsCommon.ONVERIFIED_NOTIFICATION: {
         this._log.info("The user became verified");
-
-        // Set the username now - that will cause Sync to know it is configured
-        let accountData = await this._fxaService.getSignedInUser();
-        this.username = accountData.email;
         Weave.Status.login = LOGIN_SUCCEEDED;
 
         // And actually sync. If we've never synced before, we force a full sync.
@@ -253,22 +259,8 @@ this.BrowserIDManager.prototype = {
    * Changing the username has the side-effect of wiping credentials.
    */
   set username(value) {
-    if (value) {
-      value = value.toLowerCase();
-
-      if (value == this.username) {
-        return;
-      }
-
-      Svc.Prefs.set("username", value);
-    } else {
-      Svc.Prefs.reset("username");
-    }
-
-    // If we change the username, we interpret this as a major change event
-    // and wipe out the credentials.
-    this._log.info("Username changed. Removing stored credentials.");
-    this.resetCredentials();
+    // setting .username is an old throwback, but it should no longer happen.
+    throw new Error("don't set the username");
   },
 
   /**
@@ -295,18 +287,6 @@ this.BrowserIDManager.prototype = {
     // nothing to do here until we decide to migrate away from FxA.
   },
 
-  /**
-   * Deletes Sync credentials from the password manager.
-   */
-  deleteSyncCredentials() {
-    for (let host of Utils.getSyncCredentialsHosts()) {
-      let logins = Services.logins.findLogins(host, "", "");
-      for (let login of logins) {
-        Services.logins.removeLogin(login);
-      }
-    }
-  },
-
   /**
    * Verify the current auth state, unlocking the master-password if necessary.
    *
@@ -314,9 +294,13 @@ this.BrowserIDManager.prototype = {
    * attempting to unlock.
    */
   async unlockAndVerifyAuthState() {
-    let data = await this._fxaService.getSignedInUser();
+    let data = await this.getSignedInUser();
     if (!data) {
-      log.debug("unlockAndVerifyAuthState has no user");
+      log.debug("unlockAndVerifyAuthState has no FxA user");
+      return LOGIN_FAILED_NO_USERNAME;
+    }
+    if (!this.username) {
+      log.debug("unlockAndVerifyAuthState finds that sync isn't configured");
       return LOGIN_FAILED_NO_USERNAME;
     }
     if (!data.verified) {
@@ -325,7 +309,6 @@ this.BrowserIDManager.prototype = {
       log.debug("unlockAndVerifyAuthState has an unverified user");
       return LOGIN_FAILED_LOGIN_REJECTED;
     }
-    this._updateSignedInUser(data);
     if (await this._fxaService.keys.canGetKeys()) {
       log.debug(
         "unlockAndVerifyAuthState already has (or can fetch) sync keys"
@@ -340,9 +323,6 @@ this.BrowserIDManager.prototype = {
       );
       return MASTER_PASSWORD_LOCKED;
     }
-    // now we are unlocked we must re-fetch the user data as we may now have
-    // the details that were previously locked away.
-    this._updateSignedInUser(await this._fxaService.getSignedInUser());
     // If we still can't get keys it probably means the user authenticated
     // without unlocking the MP or cleared the saved logins, so we've now
     // lost them - the user will need to reauth before continuing.
@@ -397,11 +377,6 @@ this.BrowserIDManager.prototype = {
   // Refresh the sync token for our user. Returns a promise that resolves
   // with a token, or rejects with an error.
   async _fetchTokenForUser() {
-    // gotta be verified to fetch a token.
-    if (!this._signedInUser.verified) {
-      throw new Error("User is not verified");
-    }
-
     // We need keys for things to work.  If we don't have them, just
     // return null for the token - sync calling unlockAndVerifyAuthState()
     // before actually syncing will setup the error states if necessary.
@@ -419,7 +394,7 @@ this.BrowserIDManager.prototype = {
       const assertion = await this._fxaService._internal.getAssertion(audience);
 
       this._log.debug("Getting a token");
-      const headers = { "X-Client-State": this._signedInUser.kXCS };
+      const headers = { "X-Client-State": (await this.getSignedInUser()).kXCS };
       const token = await this._tokenServerClient.getTokenFromBrowserIDAssertion(
         this._tokenServerUrl,
         assertion,
@@ -433,8 +408,7 @@ this.BrowserIDManager.prototype = {
     try {
       try {
         this._log.info("Getting keys");
-        this._updateSignedInUser(await this._fxaService.keys.getKeys()); // throws if the user changed.
-
+        await this._fxaService.keys.getKeys(); // throws if the user changed.
         token = await getToken();
       } catch (err) {
         // If we get a 401 fetching the token it may be that our certificate
@@ -455,7 +429,7 @@ this.BrowserIDManager.prototype = {
       token.expiration = this._now() + token.duration * 1000 * 0.8;
       if (!this._syncKeyBundle) {
         this._syncKeyBundle = BulkKeyBundle.fromHexKey(
-          this._signedInUser.kSync
+          (await this.getSignedInUser()).kSync
         );
       }
       Weave.Status.login = LOGIN_SUCCEEDED;
@@ -500,10 +474,11 @@ this.BrowserIDManager.prototype = {
   // concepts could be decoupled, but there doesn't seem any value in that
   // currently.
   async _ensureValidToken(forceNewToken = false) {
-    if (!this._signedInUser) {
+    let signedInUser = await this.getSignedInUser();
+    if (!signedInUser) {
       throw new Error("no user is logged in");
     }
-    if (!this._signedInUser.verified) {
+    if (!signedInUser.verified) {
       throw new Error("user is not verified");
     }
 

services/sync/modules/service.js

@@ -76,6 +76,9 @@ const { DeclinedEngines } = ChromeUtils.import(
 const { Status } = ChromeUtils.import("resource://services-sync/status.js");
 ChromeUtils.import("resource://services-sync/telemetry.js");
 const { Svc, Utils } = ChromeUtils.import("resource://services-sync/util.js");
+const { fxAccounts } = ChromeUtils.import(
+  "resource://gre/modules/FxAccounts.jsm"
+);
 
 function getEngineModules() {
   let result = {
@@ -909,6 +912,22 @@ Sync11Service.prototype = {
     }
   },
 
+  // configures/enabled/turns-on sync. There must be an FxA user signed in.
+  async configure() {
+    // We don't, and must not, throw if sync is already configured, because we
+    // might end up being called as part of a "reconnect" flow. We also want to
+    // avoid checking the FxA user is the same as the pref because the email
+    // address for the FxA account can change - we'd need to use the uid.
+    let user = await fxAccounts.getSignedInUser();
+    if (!user) {
+      throw new Error("No FxA user is signed in");
+    }
+    this._log.info("Configuring sync with current FxA user");
+    Svc.Prefs.set("username", user.email);
+    Svc.Obs.notify("weave:connected");
+  },
+
+  // resets/turns-off sync.
   async startOver() {
     this._log.trace("Invoking Service.startOver.");
     await this._stopTracking();
@@ -930,10 +949,6 @@ Sync11Service.prototype = {
       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.resetCredentials();
     this.status.login = LOGIN_FAILED_NO_USERNAME;
     this.logout();
@@ -952,8 +967,6 @@ Sync11Service.prototype = {
 
     Svc.Prefs.set("lastversion", WEAVE_VERSION);
 
-    this.identity.deleteSyncCredentials();
-
     try {
       this.identity.finalize();
       this.status.__authManager = null;

services/sync/tests/unit/test_browserid_identity.js

@@ -295,13 +295,13 @@ add_task(async function test_ensureLoggedIn() {
   fxa._internal.currentAccountState.storageManager.accountData = null;
   await Assert.rejects(
     globalBrowseridManager._ensureValidToken(true),
-    /Can't possibly get keys; User is not signed in/,
+    /no user is logged in/,
     "expecting rejection due to no user"
   );
   // Restore the logged in user to what it was.
   fxa._internal.currentAccountState.storageManager.accountData = signedInUser;
   Status.login = LOGIN_FAILED_LOGIN_REJECTED;
-  await globalBrowseridManager._ensureValidToken();
+  await globalBrowseridManager._ensureValidToken(true);
   Assert.equal(Status.login, LOGIN_SUCCEEDED, "final ensureLoggedIn worked");
 });
 
@@ -323,10 +323,11 @@ add_task(async function test_syncState() {
   fxa._internal.currentAccountState.storageManager.accountData = null;
   await Assert.rejects(
     globalBrowseridManager._ensureValidToken(true),
-    /Can't possibly get keys; User is not signed in/,
+    /no user is logged in/,
     "expecting rejection due to no user"
   );
   // Restore to an unverified user.
+  Services.prefs.setStringPref("services.sync.username", signedInUser.email);
   signedInUser.verified = false;
   fxa._internal.currentAccountState.storageManager.accountData = signedInUser;
   Status.login = LOGIN_FAILED_LOGIN_REJECTED;
@@ -806,7 +807,6 @@ add_task(async function test_signedInUserMissing() {
   });
 
   browseridManager._fxaService = fxa;
-  browseridManager._signedInUser = await fxa.getSignedInUser();
 
   let status = await browseridManager.unlockAndVerifyAuthState();
   Assert.equal(status, LOGIN_FAILED_LOGIN_REJECTED);
@@ -899,7 +899,6 @@ async function initializeIdentityWithHAWKResponseFactory(
   let fxa = new FxAccounts(internal);
 
   globalBrowseridManager._fxaService = fxa;
-  globalBrowseridManager._signedInUser = await fxa.getSignedInUser();
   await Assert.rejects(
     globalBrowseridManager._ensureValidToken(true),
     // TODO: Ideally this should have a specific check for an error.

services/sync/tests/unit/test_service_login.js

@@ -99,18 +99,6 @@ add_task(async function test_login_logout() {
     Assert.equal(Service.status.login, LOGIN_SUCCEEDED);
     Assert.ok(Service.isLoggedIn);
 
-    _("Profile refresh edge case: FxA configured but prefs reset");
-    await Service.startOver();
-    let config = makeIdentityConfig({ username: "johndoe" }, server);
-    config.fxaccount.token.endpoint =
-      server.baseURI + "/1.1/" + config.username + "/";
-    configureFxAccountIdentity(Service.identity, config);
-
-    await Service.login();
-    Assert.equal(Service.status.service, STATUS_OK);
-    Assert.equal(Service.status.login, LOGIN_SUCCEEDED);
-    Assert.ok(Service.isLoggedIn);
-
     _("Logout.");
     Service.logout();
     Assert.ok(!Service.isLoggedIn);

services/sync/tests/unit/test_service_startup.js

@@ -13,7 +13,7 @@ add_task(async function run_test() {
 
   // Test fixtures
   let { Service } = ChromeUtils.import("resource://services-sync/service.js");
-  Service.identity.username = "johndoe";
+  Services.prefs.setStringPref("services.sync.username", "johndoe");
   Assert.ok(xps.enabled);
 
   _("Service is enabled.");

services/sync/tests/unit/test_status_checkSetup.js

@@ -5,9 +5,6 @@ const { Status } = ChromeUtils.import("resource://services-sync/status.js");
 
 add_task(async function test_status_checkSetup() {
   try {
-    _("Ensure fresh config.");
-    Status._authManager.deleteSyncCredentials();
-
     _("Fresh setup, we're not configured.");
     Assert.equal(Status.checkSetup(), CLIENT_NOT_CONFIGURED);
     Assert.equal(Status.login, LOGIN_FAILED_NO_USERNAME);

services/sync/tests/unit/test_syncengine_sync.js

@@ -1316,7 +1316,7 @@ add_task(async function test_uploadOutgoing_failed() {
 });
 
 async function createRecordFailTelemetry(allowSkippedRecord) {
-  Service.identity.username = "foo";
+  Services.prefs.setStringPref("services.sync.username", "foo");
   let collection = new ServerCollection();
   collection._wbos.flying = new ServerWBO("flying");
   collection._wbos.scotsman = new ServerWBO("scotsman");

services/sync/tests/unit/test_uistate.js

@@ -13,9 +13,14 @@ add_task(async function test_isReady_unconfigured() {
   let refreshState = sinon.spy(UIStateInternal, "refreshState");
 
   // On the first call, returns false
-  // Does not trigger a refresh of the state since services.sync.username is undefined
+  // Does trigger a refresh of the state - even though services.sync.username
+  // is undefined we still need to check the account state.
   ok(!UIState.isReady());
-  ok(!refreshState.called);
+  // resfreshState is called when idle - so only check after idle.
+  await new Promise(resolve => {
+    Services.tm.idleDispatchToMainThread(resolve);
+  });
+  ok(refreshState.called);
   refreshState.resetHistory();
 
   // On subsequent calls, only return true
@@ -33,6 +38,9 @@ add_task(async function test_isReady_signedin() {
 
   // On the first call, returns false and triggers a refresh of the state
   ok(!UIState.isReady());
+  await new Promise(resolve => {
+    Services.tm.idleDispatchToMainThread(resolve);
+  });
   ok(refreshState.calledOnce);
   refreshState.resetHistory();