Bug 1855264: Prevents access token and scoped key race condition. r=m…

…arkh,sync-reviewers

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

services/fxaccounts/FxAccounts.sys.mjs

@@ -65,6 +65,8 @@ XPCOMUtils.defineLazyPreferenceGetter(
   true
 );
 
+export const ERROR_INVALID_ACCOUNT_STATE = "ERROR_INVALID_ACCOUNT_STATE";
+
 // An AccountState object holds all state related to one specific account.
 // It is considered "private" to the FxAccounts modules.
 // Only one AccountState is ever "current" in the FxAccountsInternal object -
@@ -170,7 +172,7 @@ AccountState.prototype = {
       delete updatedFields.uid;
     }
     if (!this.isCurrent) {
-      return Promise.reject(new Error("Another user has signed in"));
+      return Promise.reject(new Error(ERROR_INVALID_ACCOUNT_STATE));
     }
     return this.storageManager.updateAccountData(updatedFields);
   },
@@ -179,11 +181,11 @@ AccountState.prototype = {
     if (!this.isCurrent) {
       log.info(
         "An accountState promise was resolved, but was actually rejected" +
-          " due to a different user being signed in. Originally resolved" +
-          " with",
+          " due to the account state changing. This can happen if a new account signed in, or" +
+          " the account was signed out. Originally resolved with, ",
         result
       );
-      return Promise.reject(new Error("A different user signed in"));
+      return Promise.reject(new Error(ERROR_INVALID_ACCOUNT_STATE));
     }
     return Promise.resolve(result);
   },
@@ -195,12 +197,13 @@ AccountState.prototype = {
     // problems.
     if (!this.isCurrent) {
       log.info(
-        "An accountState promise was rejected, but we are ignoring that " +
-          "reason and rejecting it due to a different user being signed in. " +
-          "Originally rejected with",
+        "An accountState promise was rejected, but we are ignoring that" +
+          " reason and rejecting it due to the account state changing. This can happen if" +
+          " a different account signed in or the account was signed out" +
+          " originally resolved with, ",
         error
       );
-      return Promise.reject(new Error("A different user signed in"));
+      return Promise.reject(new Error(ERROR_INVALID_ACCOUNT_STATE));
     }
     return Promise.reject(error);
   },
@@ -215,7 +218,7 @@ AccountState.prototype = {
   // A preamble for the cache helpers...
   _cachePreamble() {
     if (!this.isCurrent) {
-      throw new Error("Another user has signed in");
+      throw new Error(ERROR_INVALID_ACCOUNT_STATE);
     }
   },
 
@@ -466,6 +469,37 @@ export class FxAccounts {
     }
   }
 
+  /** Gets both the OAuth token and the users scoped keys for that token
+   * and verifies that both operations were done for the same user,
+   * preventing race conditions where a caller
+   * can get the key for one user, and the id of another if the user
+   * is rapidly switching between accounts
+   *
+   * @param options
+   *        {
+   *          scope: string the oauth scope being requested. This must
+   *          be a scope with an associated key, otherwise an error
+   *          will be thrown that the key is not available.
+   *          ttl: (number) OAuth token TTL in seconds
+   *        }
+   *
+   * @return Promise.<Object | Error>
+   * The promise resolve to both the access token being requested, and the scoped key
+   *        {
+   *         token: (string) access token
+   *         key: (object) the scoped key object
+   *        }
+   * The promise can reject, with one of the errors `getOAuthToken`, `FxAccountKeys.getKeyForScope`, or
+   * error if the user changed in-between operations
+   */
+  getOAuthTokenAndKey(options = {}) {
+    return this._withCurrentAccountState(async () => {
+      const key = await this.keys.getKeyForScope(options.scope);
+      const token = await this.getOAuthToken(options);
+      return { token, key };
+    });
+  }
+
   /**
    * Remove an OAuth token from the token cache. Callers should call this
    * after they determine a token is invalid, so a new token will be fetched

services/fxaccounts/tests/xpcshell/test_accounts.js

@@ -6,7 +6,7 @@
 const { CryptoUtils } = ChromeUtils.importESModule(
   "resource://services-crypto/utils.sys.mjs"
 );
-const { FxAccounts } = ChromeUtils.importESModule(
+const { FxAccounts, ERROR_INVALID_ACCOUNT_STATE } = ChromeUtils.importESModule(
   "resource://gre/modules/FxAccounts.sys.mjs"
 );
 const { FxAccountsClient } = ChromeUtils.importESModule(
@@ -777,11 +777,10 @@ add_task(async function test_getKeyForScope_nonexistent_account() {
     });
   });
 
-  // XXX - the exception message here isn't ideal, but doesn't really matter...
-  await Assert.rejects(
-    fxa.keys.getKeyForScope(SCOPE_OLD_SYNC),
-    /A different user signed in/
-  );
+  await Assert.rejects(fxa.keys.getKeyForScope(SCOPE_OLD_SYNC), err => {
+    Assert.equal(err.message, ERROR_INVALID_ACCOUNT_STATE);
+    return true; // expected error
+  });
 
   await promiseLogout;
 
@@ -1405,6 +1404,31 @@ add_test(function test_getOAuthToken_error() {
   });
 });
 
+add_test(async function test_getOAuthTokenAndKey_errors_if_user_change() {
+  const fxa = new MockFxAccounts();
+  const alice = getTestUser("alice");
+  const bob = getTestUser("bob");
+  alice.verified = true;
+  bob.verified = true;
+
+  fxa.getOAuthToken = async () => {
+    // We mock what would happen if the user got changed
+    // after we got the access token
+    await fxa.setSignedInUser(bob);
+    return "access token";
+  };
+  fxa.keys.getKeyForScope = () => Promise.resolve("key!");
+  await fxa.setSignedInUser(alice);
+  await Assert.rejects(
+    fxa.getOAuthTokenAndKey({ scope: "foo", ttl: 10 }),
+    err => {
+      Assert.equal(err.message, ERROR_INVALID_ACCOUNT_STATE);
+      return true; // expected error
+    }
+  );
+  run_next_test();
+});
+
 add_task(async function test_listAttachedOAuthClients() {
   const ONE_HOUR = 60 * 60 * 1000;
   const ONE_DAY = 24 * ONE_HOUR;

services/sync/modules/sync_auth.sys.mjs

@@ -387,22 +387,28 @@ SyncAuthManager.prototype = {
     // Do the token dance, with a retry in case of transient auth failure.
     // We need to prove that we know the sync key in order to get a token
     // from the tokenserver.
-    let getToken = async key => {
+    let getToken = async (key, accessToken) => {
       this._log.info("Getting a sync token from", this._tokenServerUrl);
-      let token = await this._fetchTokenUsingOAuth(key);
+      let token = await this._fetchTokenUsingOAuth(key, accessToken);
       this._log.trace("Successfully got a token");
       return token;
     };
 
+    const ttl = fxAccountsCommon.OAUTH_TOKEN_FOR_SYNC_LIFETIME_SECONDS;
     try {
       let token, key;
       try {
         this._log.info("Getting sync key");
-        key = await fxa.keys.getKeyForScope(SCOPE_OLD_SYNC);
+        const tokenAndKey = await fxa.getOAuthTokenAndKey({
+          scope: SCOPE_OLD_SYNC,
+          ttl,
+        });
+
+        key = tokenAndKey.key;
         if (!key) {
           throw new Error("browser does not have the sync key, cannot sync");
         }
-        token = await getToken(key);
+        token = await getToken(key, tokenAndKey.token);
       } catch (err) {
         // If we get a 401 fetching the token it may be that our auth tokens needed
         // to be regenerated; retry exactly once.
@@ -412,8 +418,11 @@ SyncAuthManager.prototype = {
         this._log.warn(
           "Token server returned 401, retrying token fetch with fresh credentials"
         );
-        key = await fxa.keys.getKeyForScope(SCOPE_OLD_SYNC);
-        token = await getToken(key);
+        const tokenAndKey = await fxa.getOAuthTokenAndKey({
+          scope: SCOPE_OLD_SYNC,
+          ttl,
+        });
+        token = await getToken(tokenAndKey.key, tokenAndKey.token);
       }
       // TODO: Make it be only 80% of the duration, so refresh the token
       // before it actually expires. This is to avoid sync storage errors
@@ -460,17 +469,13 @@ SyncAuthManager.prototype = {
   },
 
   /**
-   * Generates an OAuth access_token using the OLD_SYNC scope and exchanges it
-   * for a TokenServer token.
-   *
+   * Exchanges an OAuth access_token for a TokenServer token.
    * @returns {Promise}
    * @private
    */
-  async _fetchTokenUsingOAuth(key) {
+  async _fetchTokenUsingOAuth(key, accessToken) {
     this._log.debug("Getting a token using OAuth");
     const fxa = this._fxaService;
-    const ttl = fxAccountsCommon.OAUTH_TOKEN_FOR_SYNC_LIFETIME_SECONDS;
-    const accessToken = await fxa.getOAuthToken({ scope: SCOPE_OLD_SYNC, ttl });
     const headers = {
       "X-KeyId": key.kid,
     };

services/sync/tests/unit/test_sync_auth_manager.js

@@ -37,9 +37,8 @@ const { TokenServerClient, TokenServerClientServerError } =
   ChromeUtils.importESModule(
     "resource://services-common/tokenserverclient.sys.mjs"
   );
-const { AccountState } = ChromeUtils.importESModule(
-  "resource://gre/modules/FxAccounts.sys.mjs"
-);
+const { AccountState, ERROR_INVALID_ACCOUNT_STATE } =
+  ChromeUtils.importESModule("resource://gre/modules/FxAccounts.sys.mjs");
 
 const SECOND_MS = 1000;
 const MINUTE_MS = SECOND_MS * 60;
@@ -192,8 +191,11 @@ add_task(async function test_initialializeWithAuthErrorAndDeletedAccount() {
 
   await Assert.rejects(
     syncAuthManager._ensureValidToken(),
-    AuthenticationError,
-    "should reject due to an auth error"
+    err => {
+      Assert.equal(err.message, ERROR_INVALID_ACCOUNT_STATE);
+      return true; // expected error
+    },
+    "should reject because the account was deleted"
   );
 
   Assert.ok(accessTokenWithSessionTokenCalled);