diff --git a/services/fxaccounts/FxAccounts.sys.mjs b/services/fxaccounts/FxAccounts.sys.mjs index 18169c6b2d83..790a6195f833 100644 --- a/services/fxaccounts/FxAccounts.sys.mjs +++ b/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. + * 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 diff --git a/services/fxaccounts/tests/xpcshell/test_accounts.js b/services/fxaccounts/tests/xpcshell/test_accounts.js index c4aec73a0395..49ee12cb2344 100644 --- a/services/fxaccounts/tests/xpcshell/test_accounts.js +++ b/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; diff --git a/services/sync/modules/sync_auth.sys.mjs b/services/sync/modules/sync_auth.sys.mjs index 6b8da4061ced..9508154ad58a 100644 --- a/services/sync/modules/sync_auth.sys.mjs +++ b/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, }; diff --git a/services/sync/tests/unit/test_sync_auth_manager.js b/services/sync/tests/unit/test_sync_auth_manager.js index 9af40d26c612..97f551252eb3 100644 --- a/services/sync/tests/unit/test_sync_auth_manager.js +++ b/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);