diff options
Diffstat (limited to 'services/fxaccounts')
15 files changed, 115 insertions, 62 deletions
diff --git a/services/fxaccounts/FxAccounts.sys.mjs b/services/fxaccounts/FxAccounts.sys.mjs index 18169c6b2d..790a6195f8 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.<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 diff --git a/services/fxaccounts/FxAccountsClient.sys.mjs b/services/fxaccounts/FxAccountsClient.sys.mjs index 9dc80ff419..a62da40fd8 100644 --- a/services/fxaccounts/FxAccountsClient.sys.mjs +++ b/services/fxaccounts/FxAccountsClient.sys.mjs @@ -59,7 +59,7 @@ FxAccountsClient.prototype = { /* * Return current time in milliseconds * - * Not used by this module, but made available to the FxAccounts.jsm + * Not used by this module, but made available to the FxAccounts.sys.mjs * that uses this client. */ now() { @@ -498,7 +498,7 @@ FxAccountsClient.prototype = { */ accountExists(email) { return this.signIn(email, "").then( - cantHappen => { + () => { throw new Error("How did I sign in with an empty password?"); }, expectedError => { diff --git a/services/fxaccounts/FxAccountsProfile.sys.mjs b/services/fxaccounts/FxAccountsProfile.sys.mjs index de8bdb2f0e..8022a6d8a8 100644 --- a/services/fxaccounts/FxAccountsProfile.sys.mjs +++ b/services/fxaccounts/FxAccountsProfile.sys.mjs @@ -52,7 +52,7 @@ FxAccountsProfile.prototype = { // making another request to determine if it is fresh or not. PROFILE_FRESHNESS_THRESHOLD: 120000, // 2 minutes - observe(subject, topic, data) { + observe(subject, topic) { // If we get a profile change notification from our webchannel it means // the user has just changed their profile via the web, so we want to // ignore our "freshness threshold" diff --git a/services/fxaccounts/FxAccountsWebChannel.sys.mjs b/services/fxaccounts/FxAccountsWebChannel.sys.mjs index fdd0b75e93..f8d7a3362d 100644 --- a/services/fxaccounts/FxAccountsWebChannel.sys.mjs +++ b/services/fxaccounts/FxAccountsWebChannel.sys.mjs @@ -431,7 +431,7 @@ FxAccountsWebChannelHelpers.prototype = { // A sync-specific hack - we want to ensure sync has been initialized // before we set the signed-in user. // XXX - probably not true any more, especially now we have observerPreloads - // in FxAccounts.jsm? + // in FxAccounts.sys.mjs? let xps = this._weaveXPCOM || Cc["@mozilla.org/weave/service;1"].getService(Ci.nsISupports) diff --git a/services/fxaccounts/tests/mochitest/test_invalidEmailCase.html b/services/fxaccounts/tests/mochitest/test_invalidEmailCase.html index 4b5e943591..24353afbc1 100644 --- a/services/fxaccounts/tests/mochitest/test_invalidEmailCase.html +++ b/services/fxaccounts/tests/mochitest/test_invalidEmailCase.html @@ -49,7 +49,7 @@ MockStorage.prototype = Object.freeze({ getOAuthTokens() { return Promise.resolve(null); }, - setOAuthTokens(contents) { + setOAuthTokens() { return Promise.resolve(); }, }); diff --git a/services/fxaccounts/tests/xpcshell/test_accounts.js b/services/fxaccounts/tests/xpcshell/test_accounts.js index c4aec73a03..239adb206f 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( @@ -120,7 +120,7 @@ function MockFxAccountsClient() { // mock calls up to the auth server to determine whether the // user account has been verified - this.recoveryEmailStatus = async function (sessionToken) { + this.recoveryEmailStatus = async function () { // simulate a call to /recovery_email/status return { email: this._email, @@ -139,7 +139,7 @@ function MockFxAccountsClient() { return !this._deletedOnServer; }; - this.accountKeys = function (keyFetchToken) { + this.accountKeys = function () { return new Promise(resolve => { do_timeout(50, () => { resolve({ @@ -188,7 +188,7 @@ Object.setPrototypeOf( * mock the now() method, so that we can simulate the passing of * time and verify that signatures expire correctly. */ -function MockFxAccounts(credentials = null) { +function MockFxAccounts() { let result = new FxAccounts({ VERIFICATION_POLL_TIMEOUT_INITIAL: 100, // 100ms @@ -453,10 +453,10 @@ add_test(function test_polling_timeout() { fxa.setSignedInUser(test_user).then(() => { p.then( - success => { + () => { do_throw("this should not succeed"); }, - fail => { + () => { removeObserver(); fxa.signOut().then(run_next_test); } @@ -471,7 +471,7 @@ add_task(async function test_onverified_once() { let numNotifications = 0; - function observe(aSubject, aTopic, aData) { + function observe() { numNotifications += 1; } Services.obs.addObserver(observe, ONVERIFIED_NOTIFICATION); @@ -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; @@ -972,17 +971,17 @@ add_test(function test_fetchAndUnwrapAndDeriveKeys_no_token() { makeObserver(ONLOGOUT_NOTIFICATION, function () { log.debug("test_fetchAndUnwrapKeys_no_token observed logout"); - fxa._internal.getUserAccountData().then(user2 => { + fxa._internal.getUserAccountData().then(() => { fxa._internal.abortExistingFlow().then(run_next_test); }); }); fxa .setSignedInUser(user) - .then(user2 => { + .then(() => { return fxa.keys._fetchAndUnwrapAndDeriveKeys(); }) - .catch(error => { + .catch(() => { log.info("setSignedInUser correctly rejected"); }); }); @@ -1273,11 +1272,7 @@ add_task(async function test_getOAuthTokenCachedScopeNormalization() { let numOAuthTokenCalls = 0; let client = fxa._internal.fxAccountsClient; - client.accessTokenWithSessionToken = async ( - _sessionTokenHex, - _clientId, - scopeString - ) => { + client.accessTokenWithSessionToken = async (_sessionTokenHex, _clientId) => { numOAuthTokenCalls++; return MOCK_TOKEN_RESPONSE; }; @@ -1405,6 +1400,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/fxaccounts/tests/xpcshell/test_accounts_device_registration.js b/services/fxaccounts/tests/xpcshell/test_accounts_device_registration.js index 68337eb69e..4b6ac58879 100644 --- a/services/fxaccounts/tests/xpcshell/test_accounts_device_registration.js +++ b/services/fxaccounts/tests/xpcshell/test_accounts_device_registration.js @@ -80,7 +80,7 @@ function MockFxAccountsClient(device) { // mock calls up to the auth server to determine whether the // user account has been verified - this.recoveryEmailStatus = function (sessionToken) { + this.recoveryEmailStatus = function () { // simulate a call to /recovery_email/status return Promise.resolve({ email: this._email, @@ -104,8 +104,7 @@ function MockFxAccountsClient(device) { return Promise.resolve(!!uid && !this._deletedOnServer); }; - this.registerDevice = (st, name, type) => - Promise.resolve({ id: device.id, name }); + this.registerDevice = (st, name) => Promise.resolve({ id: device.id, name }); this.updateDevice = (st, id, name) => Promise.resolve({ id, name }); this.signOut = () => Promise.resolve({}); this.getDeviceList = st => @@ -655,7 +654,7 @@ add_task(async function test_verification_updates_registration() { }; }); - fxa._internal.checkEmailStatus = async function (sessionToken) { + fxa._internal.checkEmailStatus = async function () { credentials.verified = true; return credentials; }; @@ -792,7 +791,7 @@ add_task(async function test_refreshDeviceList() { }; const deviceListUpdateObserver = { count: 0, - observe(subject, topic, data) { + observe() { this.count++; }, }; diff --git a/services/fxaccounts/tests/xpcshell/test_commands.js b/services/fxaccounts/tests/xpcshell/test_commands.js index 3fa42da439..c72f76193a 100644 --- a/services/fxaccounts/tests/xpcshell/test_commands.js +++ b/services/fxaccounts/tests/xpcshell/test_commands.js @@ -174,7 +174,7 @@ add_task(async function test_sendtab_receive() { const fxai = FxaInternalMock(); const sendTab = new SendTab(commands, fxai); - sendTab._encrypt = (bytes, device) => { + sendTab._encrypt = bytes => { return bytes; }; sendTab._decrypt = bytes => { @@ -387,7 +387,7 @@ add_task(async function test_commands_handleCommands() { }, }; const commands = new FxAccountsCommands(fxAccounts); - commands.sendTab.handle = (sender, data, reason) => { + commands.sendTab.handle = () => { return { title: "testTitle", uri: "https://testURI", @@ -436,7 +436,7 @@ add_task(async function test_commands_handleCommands_invalid_tab() { }, }; const commands = new FxAccountsCommands(fxAccounts); - commands.sendTab.handle = (sender, data, reason) => { + commands.sendTab.handle = () => { return { title: "badUriTab", uri: "file://path/to/pdf", diff --git a/services/fxaccounts/tests/xpcshell/test_oauth_token_storage.js b/services/fxaccounts/tests/xpcshell/test_oauth_token_storage.js index 798c439212..8575f7065c 100644 --- a/services/fxaccounts/tests/xpcshell/test_oauth_token_storage.js +++ b/services/fxaccounts/tests/xpcshell/test_oauth_token_storage.js @@ -94,7 +94,7 @@ Object.setPrototypeOf( FxAccountsClient.prototype ); -function MockFxAccounts(device = {}) { +function MockFxAccounts() { return new FxAccounts({ fxAccountsClient: new MockFxAccountsClient(), newAccountState(credentials) { diff --git a/services/fxaccounts/tests/xpcshell/test_pairing.js b/services/fxaccounts/tests/xpcshell/test_pairing.js index eac3112242..245db66f66 100644 --- a/services/fxaccounts/tests/xpcshell/test_pairing.js +++ b/services/fxaccounts/tests/xpcshell/test_pairing.js @@ -61,7 +61,7 @@ const fxAccounts = { }, _internal: { keys: { - getKeyForScope(scope) { + getKeyForScope() { return { kid: "123456", k: KSYNC, diff --git a/services/fxaccounts/tests/xpcshell/test_profile.js b/services/fxaccounts/tests/xpcshell/test_profile.js index f8137b5691..39acf653d4 100644 --- a/services/fxaccounts/tests/xpcshell/test_profile.js +++ b/services/fxaccounts/tests/xpcshell/test_profile.js @@ -142,10 +142,10 @@ add_test(function fetchAndCacheProfile_always_bumps_cachedAt() { profile._cachedAt = 12345; return profile._fetchAndCacheProfile().then( - result => { + () => { do_throw("Should not succeed"); }, - err => { + () => { Assert.notEqual(profile._cachedAt, 12345, "cachedAt has been bumped"); run_next_test(); } @@ -164,7 +164,7 @@ add_test(function fetchAndCacheProfile_sendsETag() { }; let profile = CreateFxAccountsProfile(fxa, client); - return profile._fetchAndCacheProfile().then(result => { + return profile._fetchAndCacheProfile().then(() => { run_next_test(); }); }); @@ -282,7 +282,7 @@ add_test(function fetchAndCacheProfile_alreadyCached() { }; let profile = CreateFxAccountsProfile(fxa, client); - profile._cacheProfile = function (toCache) { + profile._cacheProfile = function () { do_throw("This method should not be called."); }; @@ -614,7 +614,7 @@ add_test(function getProfile_has_cached_fetch_deleted() { // instead of checking this in a mocked "save" function, just check after the // observer - makeObserver(ON_PROFILE_CHANGE_NOTIFICATION, function (subject, topic, data) { + makeObserver(ON_PROFILE_CHANGE_NOTIFICATION, function () { profile.getProfile().then(profileData => { Assert.equal(null, profileData.avatar); run_next_test(); diff --git a/services/fxaccounts/tests/xpcshell/test_profile_client.js b/services/fxaccounts/tests/xpcshell/test_profile_client.js index 22fcc293f8..5b6e4ffdff 100644 --- a/services/fxaccounts/tests/xpcshell/test_profile_client.js +++ b/services/fxaccounts/tests/xpcshell/test_profile_client.js @@ -39,7 +39,7 @@ let mockResponse = function (response) { Request.ifNoneMatchSet = true; } }, - async dispatch(method, payload) { + async dispatch() { this.response = response; return this.response; }, @@ -74,7 +74,7 @@ let mockResponseError = function (error) { return function () { return { setHeader() {}, - async dispatch(method, payload) { + async dispatch() { throw error; }, }; @@ -221,7 +221,7 @@ add_test(function server401ResponseThenSuccess() { let numRequests = 0; let numAuthHeaders = 0; // Like mockResponse but we want access to headers etc. - client._Request = function (requestUri) { + client._Request = function () { return { setHeader(name, value) { if (name == "Authorization") { @@ -229,7 +229,7 @@ add_test(function server401ResponseThenSuccess() { Assert.equal(value, "Bearer " + lastToken); } }, - async dispatch(method, payload) { + async dispatch() { this.response = responses[numRequests]; ++numRequests; return this.response; @@ -283,7 +283,7 @@ add_test(function server401ResponsePersists() { let numRequests = 0; let numAuthHeaders = 0; - client._Request = function (requestUri) { + client._Request = function () { return { setHeader(name, value) { if (name == "Authorization") { @@ -291,7 +291,7 @@ add_test(function server401ResponsePersists() { Assert.equal(value, "Bearer " + lastToken); } }, - async dispatch(method, payload) { + async dispatch() { this.response = response; ++numRequests; return this.response; diff --git a/services/fxaccounts/tests/xpcshell/test_push_service.js b/services/fxaccounts/tests/xpcshell/test_push_service.js index 0441888847..216f5d8cc8 100644 --- a/services/fxaccounts/tests/xpcshell/test_push_service.js +++ b/services/fxaccounts/tests/xpcshell/test_push_service.js @@ -179,7 +179,7 @@ add_test(function observePushTopicDeviceConnected() { return this; }, }; - let obs = (subject, topic, data) => { + let obs = (subject, topic) => { Services.obs.removeObserver(obs, topic); run_next_test(); }; @@ -392,7 +392,7 @@ add_test(function observePushTopicProfileUpdated() { return this; }, }; - let obs = (subject, topic, data) => { + let obs = (subject, topic) => { Services.obs.removeObserver(obs, topic); run_next_test(); }; diff --git a/services/fxaccounts/tests/xpcshell/test_storage_manager.js b/services/fxaccounts/tests/xpcshell/test_storage_manager.js index 05c565d2f4..df29ca881d 100644 --- a/services/fxaccounts/tests/xpcshell/test_storage_manager.js +++ b/services/fxaccounts/tests/xpcshell/test_storage_manager.js @@ -62,7 +62,7 @@ MockedSecureStorage.prototype = { // "TypeError: this.STORAGE_LOCKED is not a constructor" STORAGE_LOCKED: function () {}, /* eslint-enable object-shorthand */ - async get(uid, email) { + async get() { this.fetchCount++; if (this.locked) { throw new this.STORAGE_LOCKED(); diff --git a/services/fxaccounts/tests/xpcshell/test_web_channel.js b/services/fxaccounts/tests/xpcshell/test_web_channel.js index 48f043d0b9..020ac4d905 100644 --- a/services/fxaccounts/tests/xpcshell/test_web_channel.js +++ b/services/fxaccounts/tests/xpcshell/test_web_channel.js @@ -202,7 +202,7 @@ add_test(function test_error_message_remove_profile_path() { const toTest = Object.keys(errors).length; for (const key in errors) { let error = errors[key]; - channel._channel.send = (message, context) => { + channel._channel.send = message => { equal( message.data.error.message, error.expected, @@ -403,7 +403,7 @@ add_test(function test_fxa_status_message() { }); channel._channel = { - send(response, sendingContext) { + send(response) { Assert.equal(response.command, "fxaccounts:fxa_status"); Assert.equal(response.messageId, 123); @@ -513,7 +513,7 @@ add_task(async function test_helpers_login_set_previous_account_name_hash() { let helpers = new FxAccountsWebChannelHelpers({ fxAccounts: { _internal: { - setSignedInUser(accountData) { + setSignedInUser() { return new Promise(resolve => { // previously signed in user preference is updated. Assert.equal( @@ -554,7 +554,7 @@ add_task( let helpers = new FxAccountsWebChannelHelpers({ fxAccounts: { _internal: { - setSignedInUser(accountData) { + setSignedInUser() { return new Promise(resolve => { // previously signed in user preference should not be updated. Assert.equal( |