From 2e2645d0d28feec93b305a255838e01a3e3f6676 Mon Sep 17 00:00:00 2001 From: Mark Hammond Date: Sat, 18 Jul 2015 18:47:55 +1000 Subject: [PATCH] Bug 1157529 - refactor FxA storage to be less lossy and less racey. r=ckarlof --- ...rowser_946320_tabs_from_other_computers.js | 116 ++-- services/fxaccounts/FxAccounts.jsm | 591 ++++-------------- services/fxaccounts/FxAccountsCommon.js | 4 +- services/fxaccounts/FxAccountsStorage.jsm | 540 ++++++++++++++++ services/fxaccounts/moz.build | 3 +- .../tests/xpcshell/test_accounts.js | 94 ++- .../tests/xpcshell/test_loginmgr_storage.js | 206 ++---- .../xpcshell/test_oauth_token_storage.js | 170 ++--- .../tests/xpcshell/test_storage_manager.js | 407 ++++++++++++ .../fxaccounts/tests/xpcshell/xpcshell.ini | 1 + services/sync/modules-testing/utils.js | 79 ++- .../tests/unit/test_browserid_identity.js | 35 +- 12 files changed, 1385 insertions(+), 861 deletions(-) create mode 100644 services/fxaccounts/FxAccountsStorage.jsm create mode 100644 services/fxaccounts/tests/xpcshell/test_storage_manager.js diff --git a/browser/components/customizableui/test/browser_946320_tabs_from_other_computers.js b/browser/components/customizableui/test/browser_946320_tabs_from_other_computers.js index 9ca53d655a1..ff3785519ba 100644 --- a/browser/components/customizableui/test/browser_946320_tabs_from_other_computers.js +++ b/browser/components/customizableui/test/browser_946320_tabs_from_other_computers.js @@ -6,12 +6,10 @@ let Preferences = Cu.import("resource://gre/modules/Preferences.jsm", {}).Preferences; -let tmp = {}; -Cu.import("resource://gre/modules/FxAccounts.jsm", tmp); -Cu.import("resource://gre/modules/FxAccountsCommon.js", tmp); -Cu.import("resource://services-sync/browserid_identity.js", tmp); -let {FxAccounts, BrowserIDManager, DATA_FORMAT_VERSION, CERT_LIFETIME} = tmp; -let fxaSyncIsEnabled = Weave.Service.identity instanceof BrowserIDManager; +const {FxAccounts, AccountState} = Cu.import("resource://gre/modules/FxAccounts.jsm", {}); + +// FxA logs can be gotten at via this pref which helps debugging. +Preferences.set("services.sync.log.appender.dump", "Debug"); add_task(function() { yield PanelUI.show({type: "command"}); @@ -47,35 +45,56 @@ add_task(function() { PanelUI.toggle({type: "command"}); yield hiddenPanelPromise; - if (fxaSyncIsEnabled) { - yield fxAccounts.signOut(); - } + yield fxAccounts.signOut(/*localOnly = */true); }); function configureIdentity() { - // do the FxAccounts thang... + // do the FxAccounts thang and wait for Sync to initialize the identity. configureFxAccountIdentity(); - - if (fxaSyncIsEnabled) { - return Weave.Service.identity.initializeWithCurrentIdentity().then(() => { - // need to wait until this identity manager is readyToAuthenticate. - return Weave.Service.identity.whenReadyToAuthenticate.promise; - }); - } - - Weave.Service.createAccount("john@doe.com", "mysecretpw", - "challenge", "response"); - Weave.Service.identity.account = "john@doe.com"; - Weave.Service.identity.basicPassword = "mysecretpw"; - Weave.Service.identity.syncKey = Weave.Utils.generatePassphrase(); - Weave.Svc.Prefs.set("firstSync", "newAccount"); - Weave.Service.persistLogin(); - return Promise.resolve(); + return Weave.Service.identity.initializeWithCurrentIdentity().then(() => { + // need to wait until this identity manager is readyToAuthenticate. + return Weave.Service.identity.whenReadyToAuthenticate.promise; + }); } -// Configure an instance of an FxAccount identity provider with the specified -// config (or the default config if not specified). +// Configure an instance of an FxAccount identity provider. function configureFxAccountIdentity() { + // A mock "storage manager" for FxAccounts that doesn't actually write anywhere. + function MockFxaStorageManager() { + } + + MockFxaStorageManager.prototype = { + promiseInitialized: Promise.resolve(), + + initialize(accountData) { + this.accountData = accountData; + }, + + finalize() { + return Promise.resolve(); + }, + + getAccountData() { + return Promise.resolve(this.accountData); + }, + + updateAccountData(updatedFields) { + for (let [name, value] of Iterator(updatedFields)) { + if (value == null) { + delete this.accountData[name]; + } else { + this.accountData[name] = value; + } + } + return Promise.resolve(); + }, + + deleteAccountData() { + this.accountData = null; + return Promise.resolve(); + } + } + let user = { assertion: "assertion", email: "email", @@ -94,7 +113,25 @@ function configureFxAccountIdentity() { // uid will be set to the username. }; - let MockInternal = {}; + let MockInternal = { + newAccountState(credentials) { + isnot(credentials, "not expecting credentials"); + let storageManager = new MockFxaStorageManager(); + // and init storage with our user. + storageManager.initialize(user); + return new AccountState(this, storageManager); + }, + getCertificate(data, keyPair, mustBeValidUntil) { + this.cert = { + validUntil: this.now() + 10000, + cert: "certificate", + }; + return Promise.resolve(this.cert.cert); + }, + getCertificateSigned() { + return Promise.resolve(); + }, + }; let mockTSC = { // TokenServerClient getTokenFromBrowserIDAssertion: function(uri, assertion, cb) { token.uid = "username"; @@ -102,23 +139,10 @@ function configureFxAccountIdentity() { }, }; - let authService = Weave.Service.identity; - authService._fxaService = new FxAccounts(MockInternal); - - authService._fxaService.internal.currentAccountState.signedInUser = { - version: DATA_FORMAT_VERSION, - accountData: user - } - authService._fxaService.internal.currentAccountState.getCertificate = function(data, keyPair, mustBeValidUntil) { - this.cert = { - validUntil: authService._fxaService.internal.now() + CERT_LIFETIME, - cert: "certificate", - }; - return Promise.resolve(this.cert.cert); - }; - - authService._tokenServerClient = mockTSC; + let fxa = new FxAccounts(MockInternal); + Weave.Service.identity._fxaService = fxa; + Weave.Service.identity._tokenServerClient = mockTSC; // Set the "account" of the browserId manager to be the "email" of the // logged in user of the mockFXA service. - authService._account = user.email; + Weave.Service.identity._account = user.email; } diff --git a/services/fxaccounts/FxAccounts.jsm b/services/fxaccounts/FxAccounts.jsm index 0f08102356f..29048391095 100644 --- a/services/fxaccounts/FxAccounts.jsm +++ b/services/fxaccounts/FxAccounts.jsm @@ -1,6 +1,7 @@ /* This Source Code Form is subject to the terms of the Mozilla Public * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ +"use strict"; this.EXPORTED_SYMBOLS = ["fxAccounts", "FxAccounts"]; @@ -8,13 +9,13 @@ const {classes: Cc, interfaces: Ci, utils: Cu} = Components; Cu.import("resource://gre/modules/Log.jsm"); Cu.import("resource://gre/modules/Promise.jsm"); -Cu.import("resource://gre/modules/osfile.jsm"); Cu.import("resource://services-common/utils.js"); Cu.import("resource://services-crypto/utils.js"); Cu.import("resource://gre/modules/Services.jsm"); Cu.import("resource://gre/modules/XPCOMUtils.jsm"); Cu.import("resource://gre/modules/Timer.jsm"); Cu.import("resource://gre/modules/Task.jsm"); +Cu.import("resource://gre/modules/FxAccountsStorage.jsm"); Cu.import("resource://gre/modules/FxAccountsCommon.js"); XPCOMUtils.defineLazyModuleGetter(this, "FxAccountsClient", @@ -50,7 +51,6 @@ let publicProperties = [ "resendVerificationEmail", "setSignedInUser", "signOut", - "version", "whenVerified" ]; @@ -72,28 +72,27 @@ let publicProperties = [ // } // If the state has changed between the function being called and the promise // being resolved, the .resolve() call will actually be rejected. -let AccountState = function(fxaInternal, signedInUserStorage, accountData = null) { +let AccountState = function(fxaInternal, storageManager) { this.fxaInternal = fxaInternal; - this.signedInUserStorage = signedInUserStorage; - this.signedInUser = accountData ? {version: DATA_FORMAT_VERSION, accountData} : null; - this.uid = accountData ? accountData.uid : null; - this.oauthTokens = {}; + this.storageManager = storageManager; + this.promiseInitialized = this.storageManager.getAccountData().then(data => { + this.oauthTokens = data && data.oauthTokens ? data.oauthTokens : {}; + }).catch(err => { + log.error("Failed to initialize the storage manager", err); + // Things are going to fall apart, but not much we can do about it here. + }); }; AccountState.prototype = { cert: null, keyPair: null, - signedInUser: null, oauthTokens: null, whenVerifiedDeferred: null, whenKeysReadyDeferred: null, - profile: null, - promiseInitialAccountData: null, - uid: null, get isCurrent() this.fxaInternal && this.fxaInternal.currentAccountState === this, - abort: function() { + abort() { if (this.whenVerifiedDeferred) { this.whenVerifiedDeferred.reject( new Error("Verification aborted; Another user signing in")); @@ -108,127 +107,47 @@ AccountState.prototype = { this.cert = null; this.keyPair = null; - this.signedInUser = null; - this.uid = null; + this.oauthTokens = null; this.fxaInternal = null; + // Avoid finalizing the storageManager multiple times (ie, .signOut() + // followed by .abort()) + if (!this.storageManager) { + return Promise.resolve(); + } + let storageManager = this.storageManager; + this.storageManager = null; + return storageManager.finalize(); }, // Clobber all cached data and write that empty data to storage. signOut() { this.cert = null; this.keyPair = null; - this.signedInUser = null; - this.oauthTokens = {}; - this.uid = null; - return this.persistUserData(); + this.oauthTokens = null; + let storageManager = this.storageManager; + this.storageManager = null; + return storageManager.deleteAccountData().then(() => { + return storageManager.finalize(); + }); }, getUserAccountData() { if (!this.isCurrent) { - return this.reject(new Error("Another user has signed in")); + return Promise.reject(new Error("Another user has signed in")); } - if (this.promiseInitialAccountData) { - // We are still reading the data for the first and only time. - return this.promiseInitialAccountData; - } - // We've previously read it successfully (and possibly updated it since) - if (this.signedInUser) { - return this.resolve(this.signedInUser.accountData); - } - - // We fetch the signedInUser data first, then fetch the token store and - // ensure the uid in the tokens matches our user. - let accountData = null; - let oauthTokens = {}; - return this.promiseInitialAccountData = this.signedInUserStorage.get() - .then(user => { - if (logPII) { - log.debug("getUserAccountData", user); - } - // In an ideal world we could cache the data in this.signedInUser, but - // if we do, the interaction with the login manager breaks when the - // password is locked as this read may only have obtained partial data. - // Therefore every read *must* really read incase the login manager is - // now unlocked. We could fix this with a refactor... - accountData = user ? user.accountData : null; - }, err => { - // Error reading signed in user account data. - this.promiseInitialAccountData = null; - if (err instanceof OS.File.Error && err.becauseNoSuchFile) { - // File hasn't been created yet. That will be done - // on the first call to setSignedInUser - return; - } - // something else went wrong - report the error but continue without - // user data. - log.error("Failed to read signed in user data", err); - }).then(() => { - if (!accountData) { - return null; - } - return this.signedInUserStorage.getOAuthTokens(); - }).then(tokenData => { - if (tokenData && tokenData.tokens && - tokenData.version == DATA_FORMAT_VERSION && - tokenData.uid == accountData.uid ) { - oauthTokens = tokenData.tokens; - } - }, err => { - // Error reading the OAuth tokens file. - if (err instanceof OS.File.Error && err.becauseNoSuchFile) { - // File hasn't been created yet, but will be when tokens are saved. - return; - } - log.error("Failed to read oauth tokens", err) - }).then(() => { - // We are done - clear our promise and save the data if we are still - // current. - this.promiseInitialAccountData = null; - if (this.isCurrent) { - // As above, we can not cache the data to this.signedInUser as we - // may only have partial data due to a locked MP, so the next - // request must re-read incase it is now unlocked. - // But we do save the tokens and the uid - this.oauthTokens = oauthTokens; - this.uid = accountData ? accountData.uid : null; - } - return accountData; - }); - // phew! + return this.storageManager.getAccountData().then(result => { + return this.resolve(result); + }); }, - // XXX - this should really be called "updateCurrentUserData" or similar as - // it is only ever used to add new fields to the *current* user, not to - // set a new user as current. - setUserAccountData: function(accountData) { + updateUserAccountData(updatedFields) { if (!this.isCurrent) { - return this.reject(new Error("Another user has signed in")); + return Promise.reject(new Error("Another user has signed in")); } - if (this.promiseInitialAccountData) { - throw new Error("Can't set account data before it's been read."); - } - if (!accountData) { - // see above - this should really be called "updateCurrentUserData" or similar. - throw new Error("Attempt to use setUserAccountData with null user data."); - } - if (accountData.uid != this.uid) { - // see above - this should really be called "updateCurrentUserData" or similar. - throw new Error("Attempt to use setUserAccountData with a different user."); - } - // Set our signedInUser before we start the write, so any updates to the - // data while the write completes are still captured. - this.signedInUser = {version: DATA_FORMAT_VERSION, accountData: accountData}; - return this.signedInUserStorage.set(this.signedInUser) - .then(() => this.resolve(accountData)); + return this.storageManager.updateAccountData(updatedFields); }, - getCertificate: function(data, keyPair, mustBeValidUntil) { - if (logPII) { - // don't stringify unless it will be written. We should replace this - // check with param substitutions added in bug 966674 - log.debug("getCertificate" + JSON.stringify(this.signedInUser)); - } // TODO: get the lifetime from the cert's .exp field if (this.cert && this.cert.validUntil > mustBeValidUntil) { log.debug(" getCertificate already had one"); @@ -292,7 +211,7 @@ 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: " + result); + " with", result); return Promise.reject(new Error("A different user signed in")); } return Promise.resolve(result); @@ -306,14 +225,18 @@ AccountState.prototype = { 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: " + error); + "Originally rejected with", error); return Promise.reject(new Error("A different user signed in")); } return Promise.reject(error); }, // Abstractions for storage of cached tokens - these are all sync, and don't - // handle revocation etc - it's just storage. + // handle revocation etc - it's just storage (and the storage itself is async, + // but we don't return the storage promises, so it *looks* sync) + // These functions are sync simply so we can handle "token races" - when there + // are multiple in-flight requests for the same scope, we can detect this + // and revoke the redundant token. // A preamble for the cache helpers... _cachePreamble() { @@ -340,25 +263,16 @@ AccountState.prototype = { getCachedToken(scopeArray) { this._cachePreamble(); let key = getScopeKey(scopeArray); - if (this.oauthTokens[key]) { + let result = this.oauthTokens[key]; + if (result) { // later we might want to check an expiry date - but we currently // have no such concept, so just return it. log.trace("getCachedToken returning cached token"); - return this.oauthTokens[key]; + return result; } return null; }, - // Get an array of tokenData for all cached tokens. - getAllCachedTokens() { - this._cachePreamble(); - let result = []; - for (let [key, tokenValue] in Iterator(this.oauthTokens)) { - result.push(tokenValue); - } - return result; - }, - // Remove a cached token from the cache. Does *not* revoke it from anywhere. // Returns the entire token entry if found, null otherwise. removeCachedToken(token) { @@ -380,30 +294,8 @@ AccountState.prototype = { // set of user data.) _persistCachedTokens() { this._cachePreamble(); - let record; - if (this.uid) { - record = { - version: DATA_FORMAT_VERSION, - uid: this.uid, - tokens: this.oauthTokens, - }; - } else { - record = null; - } - return this.signedInUserStorage.setOAuthTokens(record).catch( - err => { - log.error("Failed to save account data for token cache", err); - } - ); - }, - - persistUserData() { - return this._persistCachedTokens().catch(err => { - log.error("Failed to persist cached tokens", err); - }).then(() => { - return this.signedInUserStorage.set(this.signedInUser); - }).catch(err => { - log.error("Failed to persist account data", err); + return this.updateUserAccountData({ oauthTokens: this.oauthTokens }).catch(err => { + log.error("Failed to update cached tokens", err); }); }, } @@ -472,15 +364,13 @@ this.FxAccounts = function (mockInternal) { } if (mockInternal) { - // A little work-around to ensure the initial currentAccountState has - // the same mock storage the test passed in. - if (mockInternal.signedInUserStorage) { - internal.currentAccountState.signedInUserStorage = mockInternal.signedInUserStorage; - } // Exposes the internal object for testing only. external.internal = internal; } + // wait until after the mocks are setup before initializing. + internal.initialize(); + return Object.freeze(external); } @@ -488,57 +378,17 @@ this.FxAccounts = function (mockInternal) { * The internal API's constructor. */ function FxAccountsInternal() { - this.version = DATA_FORMAT_VERSION; - // Make a local copy of this constant so we can mock it in testing this.POLL_SESSION = POLL_SESSION; - // The one and only "storage" object. While this is created here, the - // FxAccountsInternal object does *not* use it directly, but instead passes - // it to AccountState objects which has sole responsibility for storage. - // Ideally we would create it in the AccountState objects, but that makes - // testing hard as AccountState objects are regularly created and thrown - // away. Doing it this way means tests can mock/replace this storage object - // and have it used by all AccountState objects, even those created before - // and after the mock has been setup. - - // We only want the fancy LoginManagerStorage on desktop. -#if defined(MOZ_B2G) - this.signedInUserStorage = new JSONStorage({ -#else - this.signedInUserStorage = new LoginManagerStorage({ -#endif - // We don't reference |profileDir| in the top-level module scope - // as we may be imported before we know where it is. - filename: DEFAULT_STORAGE_FILENAME, - oauthTokensFilename: DEFAULT_OAUTH_TOKENS_FILENAME, - baseDir: OS.Constants.Path.profileDir, - }); - - // We interact with the Firefox Accounts auth server in order to confirm that - // a user's email has been verified and also to fetch the user's keys from - // the server. We manage these processes in possibly long-lived promises - // that are internal to this object (never exposed to callers). Because - // Firefox Accounts allows for only one logged-in user, and because it's - // conceivable that while we are waiting to verify one identity, a caller - // could start verification on a second, different identity, we need to be - // able to abort all work on the first sign-in process. The currentTimer and - // currentAccountState are used for this purpose. - // (XXX - should the timer be directly on the currentAccountState?) - this.currentTimer = null; - this.currentAccountState = new AccountState(this, this.signedInUserStorage); + // All significant initialization should be done in the initialize() method + // below as it helps with testing. } /** * The internal API's prototype. */ FxAccountsInternal.prototype = { - - /** - * The current data format's version number. - */ - version: DATA_FORMAT_VERSION, - // The timeout (in ms) we use to poll for a verified mail for the first 2 mins. VERIFICATION_POLL_TIMEOUT_INITIAL: 5000, // 5 seconds // And how often we poll after the first 2 mins. @@ -546,6 +396,13 @@ FxAccountsInternal.prototype = { _fxAccountsClient: null, + // All significant initialization should be done in this initialize() method, + // as it's called after this object has been mocked for tests. + initialize() { + this.currentTimer = null; + this.currentAccountState = this.newAccountState(); + }, + get fxAccountsClient() { if (!this._fxAccountsClient) { this._fxAccountsClient = new FxAccountsClient(); @@ -566,6 +423,13 @@ FxAccountsInternal.prototype = { return this._profile; }, + // A hook-point for tests who may want a mocked AccountState or mocked storage. + newAccountState(credentials) { + let storage = new FxAccountsStorageManager(); + storage.initialize(credentials); + return new AccountState(this, storage); + }, + /** * Return the current time in milliseconds as an integer. Allows tests to * manipulate the date to simulate certificate expiration. @@ -676,24 +540,23 @@ FxAccountsInternal.prototype = { */ setSignedInUser: function setSignedInUser(credentials) { log.debug("setSignedInUser - aborting any existing flows"); - this.abortExistingFlow(); - - let currentAccountState = this.currentAccountState = new AccountState( - this, - this.signedInUserStorage, - JSON.parse(JSON.stringify(credentials)) // Pass a clone of the credentials object. - ); - - // This promise waits for storage, but not for verification. - // We're telling the caller that this is durable now. - return currentAccountState.persistUserData().then(() => { - this.notifyObservers(ONLOGIN_NOTIFICATION); - if (!this.isUserEmailVerified(credentials)) { - this.startVerifiedCheck(credentials); - } - }).then(() => { - return currentAccountState.resolve(); - }); + return this.abortExistingFlow().then(() => { + let currentAccountState = this.currentAccountState = this.newAccountState( + Cu.cloneInto(credentials, {}) // Pass a clone of the credentials object. + ); + // This promise waits for storage, but not for verification. + // We're telling the caller that this is durable now (although is that + // really something we should commit to? Why not let the write happen in + // the background? Already does for updateAccountData ;) + return currentAccountState.promiseInitialized.then(() => { + this.notifyObservers(ONLOGIN_NOTIFICATION); + if (!this.isUserEmailVerified(credentials)) { + this.startVerifiedCheck(credentials); + } + }).then(() => { + return currentAccountState.resolve(); + }); + }) }, /** @@ -749,8 +612,13 @@ FxAccountsInternal.prototype = { clearTimeout(this.currentTimer); this.currentTimer = 0; } - this.currentAccountState.abort(); - this.currentAccountState = new AccountState(this, this.signedInUserStorage); + if (this._profile) { + this._profile.tearDown(); + this._profile = null; + } + // We "abort" the accountState and assume our caller is about to throw it + // away and replace it with a new one. + return this.currentAccountState.abort(); }, accountStatus: function accountStatus() { @@ -773,7 +641,7 @@ FxAccountsInternal.prototype = { _destroyAllOAuthTokens: function(tokenInfos) { // let's just destroy them all in parallel... let promises = []; - for (let tokenInfo of tokenInfos) { + for (let [key, tokenInfo] in Iterator(tokenInfos || {})) { promises.push(this._destroyOAuthToken(tokenInfo)); } return Promise.all(promises); @@ -786,7 +654,7 @@ FxAccountsInternal.prototype = { return currentState.getUserAccountData().then(data => { // Save the session token for use in the call to signOut below. sessionToken = data && data.sessionToken; - tokensToRevoke = currentState.getAllCachedTokens(); + tokensToRevoke = data && data.oauthTokens; return this._signOutLocal(); }).then(() => { // FxAccountsManager calls here, then does its own call @@ -821,12 +689,12 @@ FxAccountsInternal.prototype = { */ _signOutLocal: function signOutLocal() { let currentAccountState = this.currentAccountState; - if (this._profile) { - this._profile.tearDown(); - this._profile = null; - } return currentAccountState.signOut().then(() => { - this.abortExistingFlow(); // this resets this.currentAccountState. + // this "aborts" this.currentAccountState but doesn't make a new one. + return this.abortExistingFlow(); + }).then(() => { + this.currentAccountState = this.newAccountState(); + return this.currentAccountState.promiseInitialized; }); }, @@ -917,23 +785,24 @@ FxAccountsInternal.prototype = { if (logPII) { log.debug("kB_hex: " + kB_hex); } - data.kA = CommonUtils.bytesAsHex(kA); - data.kB = CommonUtils.bytesAsHex(kB_hex); - - delete data.keyFetchToken; - delete data.unwrapBKey; - - log.debug("Keys Obtained: kA=" + !!data.kA + ", kB=" + !!data.kB); - if (logPII) { - log.debug("Keys Obtained: kA=" + data.kA + ", kB=" + data.kB); + let updateData = { + kA: CommonUtils.bytesAsHex(kA), + kB: CommonUtils.bytesAsHex(kB_hex), + keyFetchToken: null, // null values cause the item to be removed. + unwrapBKey: null, } - yield currentState.setUserAccountData(data); + log.debug("Keys Obtained: kA=" + !!updateData.kA + ", kB=" + !!updateData.kB); + if (logPII) { + log.debug("Keys Obtained: kA=" + updateData.kA + ", kB=" + updateData.kB); + } + + yield currentState.updateUserAccountData(updateData); // We are now ready for business. This should only be invoked once // per setSignedInUser(), regardless of whether we've rebooted since // setSignedInUser() was called. this.notifyObservers(ONVERIFIED_NOTIFICATION); - return data; + return currentState.getUserAccountData(); }.bind(this)).then(result => currentState.resolve(result)); }, @@ -1070,12 +939,11 @@ FxAccountsInternal.prototype = { .then((response) => { log.debug("checkEmailStatus -> " + JSON.stringify(response)); if (response && response.verified) { - currentState.getUserAccountData() - .then((data) => { - data.verified = true; - return currentState.setUserAccountData(data); + currentState.updateUserAccountData({ verified: true }) + .then(() => { + return currentState.getUserAccountData(); }) - .then((data) => { + .then(data => { // Now that the user is verified, we can proceed to fetch keys if (currentState.whenVerifiedDeferred) { currentState.whenVerifiedDeferred.resolve(data); @@ -1409,7 +1277,7 @@ FxAccountsInternal.prototype = { let currentState = this.currentAccountState; return this.profile.getProfile().then( profileData => { - let profile = JSON.parse(JSON.stringify(profileData)); + let profile = Cu.cloneInto(profileData, {}); return currentState.resolve(profile); }, error => { @@ -1420,241 +1288,6 @@ FxAccountsInternal.prototype = { }, }; -/** - * JSONStorage constructor that creates instances that may set/get - * to a specified file, in a directory that will be created if it - * doesn't exist. - * - * @param options { - * filename: of the file to write to - * baseDir: directory where the file resides - * } - * @return instance - */ -function JSONStorage(options) { - this.baseDir = options.baseDir; - this.path = OS.Path.join(options.baseDir, options.filename); - this.oauthTokensPath = OS.Path.join(options.baseDir, options.oauthTokensFilename); -}; - -JSONStorage.prototype = { - set: function(contents) { - return OS.File.makeDir(this.baseDir, {ignoreExisting: true}) - .then(CommonUtils.writeJSON.bind(null, contents, this.path)); - }, - - get: function() { - return CommonUtils.readJSON(this.path); - }, - - setOAuthTokens: function(contents) { - return OS.File.makeDir(this.baseDir, {ignoreExisting: true}) - .then(CommonUtils.writeJSON.bind(null, contents, this.oauthTokensPath)); - }, - - getOAuthTokens: function(contents) { - return CommonUtils.readJSON(this.oauthTokensPath); - }, - -}; - -/** - * LoginManagerStorage constructor that creates instances that may set/get - * from a combination of a clear-text JSON file and stored securely in - * the nsILoginManager. - * - * @param options { - * filename: of the plain-text file to write to - * baseDir: directory where the file resides - * } - * @return instance - */ - -function LoginManagerStorage(options) { - // we reuse the JSONStorage for writing the plain-text stuff. - this.jsonStorage = new JSONStorage(options); -} - -LoginManagerStorage.prototype = { - // The fields in the credentials JSON object that are stored in plain-text - // in the profile directory. All other fields are stored in the login manager, - // and thus are only available when the master-password is unlocked. - - // a hook point for testing. - get _isLoggedIn() { - return Services.logins.isLoggedIn; - }, - - // Clear any data from the login manager. Returns true if the login manager - // was unlocked (even if no existing logins existed) or false if it was - // locked (meaning we don't even know if it existed or not.) - _clearLoginMgrData: Task.async(function* () { - try { // Services.logins might be third-party and broken... - yield Services.logins.initializationPromise; - if (!this._isLoggedIn) { - return false; - } - let logins = Services.logins.findLogins({}, FXA_PWDMGR_HOST, null, FXA_PWDMGR_REALM); - for (let login of logins) { - Services.logins.removeLogin(login); - } - return true; - } catch (ex) { - log.error("Failed to clear login data: ${}", ex); - return false; - } - }), - - set: Task.async(function* (contents) { - if (!contents) { - // User is signing out - write the null to the json file. - yield this.jsonStorage.set(contents); - - // And nuke it from the login manager. - let cleared = yield this._clearLoginMgrData(); - if (!cleared) { - // just log a message - we verify that the email address matches when - // we reload it, so having a stale entry doesn't really hurt. - log.info("not removing credentials from login manager - not logged in"); - } - return; - } - - // We are saving actual data. - // Split the data into 2 chunks - one to go to the plain-text, and the - // other to write to the login manager. - let toWriteJSON = {version: contents.version}; - let accountDataJSON = toWriteJSON.accountData = {}; - let toWriteLoginMgr = {version: contents.version}; - let accountDataLoginMgr = toWriteLoginMgr.accountData = {}; - for (let [name, value] of Iterator(contents.accountData)) { - if (FXA_PWDMGR_PLAINTEXT_FIELDS.indexOf(name) >= 0) { - accountDataJSON[name] = value; - } else { - accountDataLoginMgr[name] = value; - } - } - yield this.jsonStorage.set(toWriteJSON); - - try { // Services.logins might be third-party and broken... - // and the stuff into the login manager. - yield Services.logins.initializationPromise; - // If MP is locked we silently fail - the user may need to re-auth - // next startup. - if (!this._isLoggedIn) { - log.info("not saving credentials to login manager - not logged in"); - return; - } - // write the rest of the data to the login manager. - let loginInfo = new Components.Constructor( - "@mozilla.org/login-manager/loginInfo;1", Ci.nsILoginInfo, "init"); - let login = new loginInfo(FXA_PWDMGR_HOST, - null, // aFormSubmitURL, - FXA_PWDMGR_REALM, // aHttpRealm, - contents.accountData.email, // aUsername - JSON.stringify(toWriteLoginMgr), // aPassword - "", // aUsernameField - "");// aPasswordField - - let existingLogins = Services.logins.findLogins({}, FXA_PWDMGR_HOST, null, - FXA_PWDMGR_REALM); - if (existingLogins.length) { - Services.logins.modifyLogin(existingLogins[0], login); - } else { - Services.logins.addLogin(login); - } - } catch (ex) { - log.error("Failed to save data to the login manager: ${}", ex); - } - }), - - get: Task.async(function* () { - // we need to suck some data from the .json file in the profile dir and - // some other from the login manager. - let data = yield this.jsonStorage.get(); - if (!data) { - // no user logged in, nuke the storage data incase we couldn't remove - // it previously and then we are done. - yield this._clearLoginMgrData(); - return null; - } - - // if we have encryption keys it must have been saved before we - // used the login manager, so re-save it. - if (data.accountData.kA || data.accountData.kB || data.keyFetchToken) { - // We need to migrate, but the MP might be locked (eg, on the first run - // with this enabled, we will get here very soon after startup, so will - // certainly be locked.) This means we can't actually store the data in - // the login manager (and thus might lose it if we migrated now) - // So if the MP is locked, we *don't* migrate, but still just return - // the subset of data we now store in the JSON. - // This will cause sync to notice the lack of keys, force an unlock then - // re-fetch the account data to see if the keys are there. At *that* - // point we will end up back here, but because the MP is now unlocked - // we can actually perform the migration. - if (!this._isLoggedIn) { - // return the "safe" subset but leave the storage alone. - log.info("account data needs migration to the login manager but the MP is locked."); - let result = { - version: data.version, - accountData: {}, - }; - for (let fieldName of FXA_PWDMGR_PLAINTEXT_FIELDS) { - result.accountData[fieldName] = data.accountData[fieldName]; - } - return result; - } - // actually migrate - just calling .set() will split everything up. - log.info("account data is being migrated to the login manager."); - yield this.set(data); - } - - try { // Services.logins might be third-party and broken... - // read the data from the login manager and merge it for return. - yield Services.logins.initializationPromise; - - if (!this._isLoggedIn) { - log.info("returning partial account data as the login manager is locked."); - return data; - } - - let logins = Services.logins.findLogins({}, FXA_PWDMGR_HOST, null, FXA_PWDMGR_REALM); - if (logins.length == 0) { - // This could happen if the MP was locked when we wrote the data. - log.info("Can't find the rest of the credentials in the login manager"); - return data; - } - let login = logins[0]; - if (login.username == data.accountData.email) { - let lmData = JSON.parse(login.password); - if (lmData.version == data.version) { - // Merge the login manager data - copyObjectProperties(lmData.accountData, data.accountData); - } else { - log.info("version field in the login manager doesn't match - ignoring it"); - yield this._clearLoginMgrData(); - } - } else { - log.info("username in the login manager doesn't match - ignoring it"); - yield this._clearLoginMgrData(); - } - } catch (ex) { - log.error("Failed to get data from the login manager: ${}", ex); - } - return data; - }), - - // OAuth tokens are always written to disk, so delegate to our JSON storage. - // (Bug 1013064 comments 23-25 explain why we save the sessionToken into the - // plain JSON file, and the same logic applies for oauthTokens being in JSON) - getOAuthTokens() { - return this.jsonStorage.getOAuthTokens(); - }, - - setOAuthTokens(contents) { - return this.jsonStorage.setOAuthTokens(contents); - }, -} // A getter for the instance to export XPCOMUtils.defineLazyGetter(this, "fxAccounts", function() { diff --git a/services/fxaccounts/FxAccountsCommon.js b/services/fxaccounts/FxAccountsCommon.js index d900afbf8a8..9d90ad38062 100644 --- a/services/fxaccounts/FxAccountsCommon.js +++ b/services/fxaccounts/FxAccountsCommon.js @@ -66,7 +66,6 @@ exports.FXACCOUNTS_PERMISSION = "firefox-accounts"; exports.DATA_FORMAT_VERSION = 1; exports.DEFAULT_STORAGE_FILENAME = "signedInUser.json"; -exports.DEFAULT_OAUTH_TOKENS_FILENAME = "signedInUserOAuthTokens.json"; // Token life times. // Having this parameter be short has limited security value and can cause @@ -217,7 +216,8 @@ exports.ERROR_MSG_METHOD_NOT_ALLOWED = "METHOD_NOT_ALLOWED"; // The fields we save in the plaintext JSON. // See bug 1013064 comments 23-25 for why the sessionToken is "safe" exports.FXA_PWDMGR_PLAINTEXT_FIELDS = ["email", "verified", "authAt", - "sessionToken", "uid"]; + "sessionToken", "uid", "oauthTokens", + "profile"]; // The pseudo-host we use in the login manager exports.FXA_PWDMGR_HOST = "chrome://FirefoxAccounts"; // The realm we use in the login manager. diff --git a/services/fxaccounts/FxAccountsStorage.jsm b/services/fxaccounts/FxAccountsStorage.jsm new file mode 100644 index 00000000000..fc69c1eda6d --- /dev/null +++ b/services/fxaccounts/FxAccountsStorage.jsm @@ -0,0 +1,540 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ +"use strict"; + +this.EXPORTED_SYMBOLS = [ + "FxAccountsStorageManager", +]; + +const {classes: Cc, interfaces: Ci, utils: Cu} = Components; + +Cu.import("resource://gre/modules/Services.jsm"); +Cu.import("resource://gre/modules/Task.jsm"); +Cu.import("resource://gre/modules/FxAccountsCommon.js"); +Cu.import("resource://gre/modules/osfile.jsm"); +Cu.import("resource://services-common/utils.js"); + +function FxAccountsStorageManager(options = {}) { + this.options = { + filename: options.filename || DEFAULT_STORAGE_FILENAME, + baseDir: options.baseDir || OS.Constants.Path.profileDir, + } + this.plainStorage = new JSONStorage(this.options); + // On b2g we have no loginManager for secure storage, and tests may want + // to pretend secure storage isn't available. + let useSecure = 'useSecure' in options ? options.useSecure : haveLoginManager; + if (useSecure) { + this.secureStorage = new LoginManagerStorage(); + } else { + this.secureStorage = null; + } + this._clearCachedData(); + // See .initialize() below - this protects against it not being called. + this._promiseInitialized = Promise.reject("initialize not called"); + // A promise to avoid storage races - see _queueStorageOperation + this._promiseStorageComplete = Promise.resolve(); +} + +FxAccountsStorageManager.prototype = { + _initialized: false, + _needToReadSecure: true, + + // An initialization routine that *looks* synchronous to the callers, but + // is actually async as everything else waits for it to complete. + initialize(accountData) { + if (this._initialized) { + throw new Error("already initialized"); + } + this._initialized = true; + // If we just throw away our pre-rejected promise it is reported as an + // unhandled exception when it is GCd - so add an empty .catch handler here + // to prevent this. + this._promiseInitialized.catch(() => {}); + this._promiseInitialized = this._initialize(accountData); + }, + + _initialize: Task.async(function* (accountData) { + log.trace("initializing new storage manager"); + try { + if (accountData) { + // If accountData is passed we don't need to read any storage. + this._needToReadSecure = false; + // split it into the 2 parts, write it and we are done. + for (let [name, val] of Iterator(accountData)) { + if (FXA_PWDMGR_PLAINTEXT_FIELDS.indexOf(name) >= 0) { + this.cachedPlain[name] = val; + } else { + this.cachedSecure[name] = val; + } + } + // write it out and we are done. + yield this._write(); + return; + } + // So we were initialized without account data - that means we need to + // read the state from storage. We try and read plain storage first and + // only attempt to read secure storage if the plain storage had a user. + this._needToReadSecure = yield this._readPlainStorage(); + if (this._needToReadSecure && this.secureStorage) { + yield this._doReadAndUpdateSecure(); + } + } finally { + log.trace("initializing of new storage manager done"); + } + }), + + finalize() { + // We can't throw this instance away while it is still writing or we may + // end up racing with the newly created one. + log.trace("StorageManager finalizing"); + return this._promiseInitialized.then(() => { + return this._promiseStorageComplete; + }).then(() => { + this._promiseStorageComplete = null; + this._promiseInitialized = null; + this._clearCachedData(); + log.trace("StorageManager finalized"); + }) + }, + + // We want to make sure we don't end up doing multiple storage requests + // concurrently - which has a small window for reads if the master-password + // is locked at initialization time and becomes unlocked later, and always + // has an opportunity for updates. + // We also want to make sure we finished writing when finalizing, so we + // can't accidentally end up with the previous user's write finishing after + // a signOut attempts to clear it. + // So all such operations "queue" themselves via this. + _queueStorageOperation(func) { + // |result| is the promise we return - it has no .catch handler, so callers + // of the storage operation still see failure as a normal rejection. + let result = this._promiseStorageComplete.then(func); + // But the promise we assign to _promiseStorageComplete *does* have a catch + // handler so that rejections in one storage operation does not prevent + // future operations from starting (ie, _promiseStorageComplete must never + // be in a rejected state) + this._promiseStorageComplete = result.catch(err => { + log.error("${func} failed: ${err}", {func, err}); + }); + return result; + }, + + // Get the account data by combining the plain and secure storage. + getAccountData: Task.async(function* () { + yield this._promiseInitialized; + // We know we are initialized - this means our .cachedPlain is accurate + // and doesn't need to be read (it was read if necessary by initialize). + // So if there's no uid, there's no user signed in. + if (!('uid' in this.cachedPlain)) { + return null; + } + let result = {}; + for (let [name, value] of Iterator(this.cachedPlain)) { + result[name] = value; + } + // But the secure data may not have been read, so try that now. + yield this._maybeReadAndUpdateSecure(); + // .cachedSecure now has as much as it possibly can (which is possibly + // nothing if (a) secure storage remains locked and (b) we've never updated + // a field to be stored in secure storage.) + for (let [name, value] of Iterator(this.cachedSecure)) { + result[name] = value; + } + return result; + }), + + + // Update just the specified fields. This DOES NOT allow you to change to + // a different user, nor to set the user as signed-out. + updateAccountData: Task.async(function* (newFields) { + yield this._promiseInitialized; + if (!('uid' in this.cachedPlain)) { + // If this storage instance shows no logged in user, then you can't + // update fields. + throw new Error("No user is logged in"); + } + if (!newFields || 'uid' in newFields || 'email' in newFields) { + // Once we support + // user changing email address this may need to change, but it's not + // clear how we would be told of such a change anyway... + throw new Error("Can't change uid or email address"); + } + log.debug("_updateAccountData with items", Object.keys(newFields)); + // work out what bucket. + for (let [name, value] of Iterator(newFields)) { + if (FXA_PWDMGR_PLAINTEXT_FIELDS.indexOf(name) >= 0) { + if (value == null) { + delete this.cachedPlain[name]; + } else { + this.cachedPlain[name] = value; + } + } else { + // don't do the "delete on null" thing here - we need to keep it until + // we have managed to read so we can nuke it on write. + this.cachedSecure[name] = value; + } + } + // If we haven't yet read the secure data, do so now, else we may write + // out partial data. + yield this._maybeReadAndUpdateSecure(); + // Now save it - but don't wait on the _write promise - it's queued up as + // a storage operation, so .finalize() will wait for completion, but no need + // for us to. + this._write(); + }), + + _clearCachedData() { + this.cachedPlain = {}; + // If we don't have secure storage available we have cachedPlain and + // cachedSecure be the same object. + this.cachedSecure = this.secureStorage == null ? this.cachedPlain : {}; + }, + + /* Reads the plain storage and caches the read values in this.cachedPlain. + Only ever called once and unlike the "secure" storage, is expected to never + fail (ie, plain storage is considered always available, whereas secure + storage may be unavailable if it is locked). + + Returns a promise that resolves with true if valid account data was found, + false otherwise. + + Note: _readPlainStorage is only called during initialize, so isn't + protected via _queueStorageOperation() nor _promiseInitialized. + */ + _readPlainStorage: Task.async(function* () { + let got; + try { + got = yield this.plainStorage.get(); + } catch(err) { + // File hasn't been created yet. That will be done + // when write is called. + if (!(err instanceof OS.File.Error) || !err.becauseNoSuchFile) { + log.error("Failed to read plain storage", err); + } + // either way, we return null. + got = null; + } + if (!got || !got.accountData || !got.accountData.uid || + got.version != DATA_FORMAT_VERSION) { + return false; + } + // We need to update our .cachedPlain, but can't just assign to it as + // it may need to be the exact same object as .cachedSecure + // As a sanity check, .cachedPlain must be empty (as we are called by init) + // XXX - this would be a good use-case for a RuntimeAssert or similar, as + // being added in bug 1080457. + if (Object.keys(this.cachedPlain).length != 0) { + throw new Error("should be impossible to have cached data already.") + } + for (let [name, value] of Iterator(got.accountData)) { + this.cachedPlain[name] = value; + } + return true; + }), + + /* If we haven't managed to read the secure storage, try now, so + we can merge our cached data with the data that's already been set. + */ + _maybeReadAndUpdateSecure: Task.async(function* () { + if (this.secureStorage == null || !this._needToReadSecure) { + return; + } + return this._queueStorageOperation(() => { + if (this._needToReadSecure) { // we might have read it by now! + return this._doReadAndUpdateSecure(); + } + }); + }), + + /* Unconditionally read the secure storage and merge our cached data (ie, data + which has already been set while the secure storage was locked) with + the read data + */ + _doReadAndUpdateSecure: Task.async(function* () { + let { uid, email } = this.cachedPlain; + try { + log.debug("reading secure storage with existing", Object.keys(this.cachedSecure)); + // If we already have anything in .cachedSecure it means something has + // updated cachedSecure before we've read it. That means that after we do + // manage to read we must write back the merged data. + let needWrite = Object.keys(this.cachedSecure).length != 0; + let readSecure = yield this.secureStorage.get(uid, email); + // and update our cached data with it - anything already in .cachedSecure + // wins (including the fact it may be null or undefined, the latter + // which means it will be removed from storage. + if (readSecure && readSecure.version != DATA_FORMAT_VERSION) { + log.warn("got secure data but the data format version doesn't match"); + readSecure = null; + } + if (readSecure && readSecure.accountData) { + log.debug("secure read fetched items", Object.keys(readSecure.accountData)); + for (let [name, value] of Iterator(readSecure.accountData)) { + if (!(name in this.cachedSecure)) { + this.cachedSecure[name] = value; + } + } + if (needWrite) { + log.debug("successfully read secure data; writing updated data back") + yield this._doWriteSecure(); + } + } + this._needToReadSecure = false; + } catch (ex if ex instanceof this.secureStorage.STORAGE_LOCKED) { + log.debug("setAccountData: secure storage is locked trying to read"); + } catch (ex) { + log.error("failed to read secure storage", ex); + throw ex; + } + }), + + _write() { + // We don't want multiple writes happening concurrently, and we also need to + // know when an "old" storage manager is done (this.finalize() waits for this) + return this._queueStorageOperation(() => this.__write()); + }, + + __write: Task.async(function* () { + // Write everything back - later we could track what's actually dirty, + // but for now we write it all. + log.debug("writing plain storage", Object.keys(this.cachedPlain)); + let toWritePlain = { + version: DATA_FORMAT_VERSION, + accountData: this.cachedPlain, + } + yield this.plainStorage.set(toWritePlain); + + // If we have no secure storage manager we are done. + if (this.secureStorage == null) { + return; + } + // and only attempt to write to secure storage if we've managed to read it, + // otherwise we might clobber data that's already there. + if (!this._needToReadSecure) { + yield this._doWriteSecure(); + } + }), + + /* Do the actual write of secure data. Caller is expected to check if we actually + need to write and to ensure we are in a queued storage operation. + */ + _doWriteSecure: Task.async(function* () { + // We need to remove null items here. + for (let [name, value] of Iterator(this.cachedSecure)) { + if (value == null) { + delete this.cachedSecure[name]; + } + } + log.debug("writing secure storage", Object.keys(this.cachedSecure)); + let toWriteSecure = { + version: DATA_FORMAT_VERSION, + accountData: this.cachedSecure, + } + try { + yield this.secureStorage.set(this.cachedPlain.email, toWriteSecure); + } catch (ex if ex instanceof this.secureStorage.STORAGE_LOCKED) { + // This shouldn't be possible as once it is unlocked it can't be + // re-locked, and we can only be here if we've previously managed to + // read. + log.error("setAccountData: secure storage is locked trying to write"); + } + }), + + // Delete the data for an account - ie, called on "sign out". + deleteAccountData() { + return this._queueStorageOperation(() => this._deleteAccountData()); + }, + + _deleteAccountData: Task.async(function() { + log.debug("removing account data"); + yield this._promiseInitialized; + yield this.plainStorage.set(null); + if (this.secureStorage) { + yield this.secureStorage.set(null); + } + this._clearCachedData(); + log.debug("account data reset"); + }), +} + +/** + * JSONStorage constructor that creates instances that may set/get + * to a specified file, in a directory that will be created if it + * doesn't exist. + * + * @param options { + * filename: of the file to write to + * baseDir: directory where the file resides + * } + * @return instance + */ +function JSONStorage(options) { + this.baseDir = options.baseDir; + this.path = OS.Path.join(options.baseDir, options.filename); +}; + +JSONStorage.prototype = { + set: function(contents) { + log.trace("starting write of json user data", contents ? Object.keys(contents.accountData) : "null"); + let start = Date.now(); + return OS.File.makeDir(this.baseDir, {ignoreExisting: true}) + .then(CommonUtils.writeJSON.bind(null, contents, this.path)) + .then(result => { + log.trace("finished write of json user data - took", Date.now()-start); + return result; + }); + }, + + get: function() { + log.trace("starting fetch of json user data"); + let start = Date.now(); + return CommonUtils.readJSON(this.path).then(result => { + log.trace("finished fetch of json user data - took", Date.now()-start); + return result; + }); + }, +}; + +function StorageLockedError() { +} +/** + * LoginManagerStorage constructor that creates instances that set/get + * data stored securely in the nsILoginManager. + * + * @return instance + */ + +function LoginManagerStorage() { +} + +LoginManagerStorage.prototype = { + STORAGE_LOCKED: StorageLockedError, + // The fields in the credentials JSON object that are stored in plain-text + // in the profile directory. All other fields are stored in the login manager, + // and thus are only available when the master-password is unlocked. + + // a hook point for testing. + get _isLoggedIn() { + return Services.logins.isLoggedIn; + }, + + // Clear any data from the login manager. Returns true if the login manager + // was unlocked (even if no existing logins existed) or false if it was + // locked (meaning we don't even know if it existed or not.) + _clearLoginMgrData: Task.async(function* () { + try { // Services.logins might be third-party and broken... + yield Services.logins.initializationPromise; + if (!this._isLoggedIn) { + return false; + } + let logins = Services.logins.findLogins({}, FXA_PWDMGR_HOST, null, FXA_PWDMGR_REALM); + for (let login of logins) { + Services.logins.removeLogin(login); + } + return true; + } catch (ex) { + log.error("Failed to clear login data: ${}", ex); + return false; + } + }), + + set: Task.async(function* (email, contents) { + if (!contents) { + // Nuke it from the login manager. + let cleared = yield this._clearLoginMgrData(); + if (!cleared) { + // just log a message - we verify that the uid matches when + // we reload it, so having a stale entry doesn't really hurt. + log.info("not removing credentials from login manager - not logged in"); + } + log.trace("storage set finished clearing account data"); + return; + } + + // We are saving actual data. + log.trace("starting write of user data to the login manager"); + try { // Services.logins might be third-party and broken... + // and the stuff into the login manager. + yield Services.logins.initializationPromise; + // If MP is locked we silently fail - the user may need to re-auth + // next startup. + if (!this._isLoggedIn) { + log.info("not saving credentials to login manager - not logged in"); + throw new this.STORAGE_LOCKED(); + } + // write the data to the login manager. + let loginInfo = new Components.Constructor( + "@mozilla.org/login-manager/loginInfo;1", Ci.nsILoginInfo, "init"); + let login = new loginInfo(FXA_PWDMGR_HOST, + null, // aFormSubmitURL, + FXA_PWDMGR_REALM, // aHttpRealm, + email, // aUsername + JSON.stringify(contents), // aPassword + "", // aUsernameField + "");// aPasswordField + + let existingLogins = Services.logins.findLogins({}, FXA_PWDMGR_HOST, null, + FXA_PWDMGR_REALM); + if (existingLogins.length) { + Services.logins.modifyLogin(existingLogins[0], login); + } else { + Services.logins.addLogin(login); + } + log.trace("finished write of user data to the login manager"); + } catch (ex if ex instanceof this.STORAGE_LOCKED) { + throw ex; + } catch (ex) { + // just log and consume the error here - it may be a 3rd party login + // manager replacement that's simply broken. + log.error("Failed to save data to the login manager", ex); + } + }), + + get: Task.async(function* (uid, email) { + log.trace("starting fetch of user data from the login manager"); + + try { // Services.logins might be third-party and broken... + // read the data from the login manager and merge it for return. + yield Services.logins.initializationPromise; + + if (!this._isLoggedIn) { + log.info("returning partial account data as the login manager is locked."); + throw new this.STORAGE_LOCKED(); + } + + let logins = Services.logins.findLogins({}, FXA_PWDMGR_HOST, null, FXA_PWDMGR_REALM); + if (logins.length == 0) { + // This could happen if the MP was locked when we wrote the data. + log.info("Can't find any credentials in the login manager"); + return null; + } + let login = logins[0]; + // Support either the uid or the email as the username - we plan to move + // to storing the uid once Fx41 hits the release channel as the code below + // that handles either first landed in 41. Bug 1183951 is to store the uid. + if (login.username == uid || login.username == email) { + return JSON.parse(login.password); + } + log.info("username in the login manager doesn't match - ignoring it"); + yield this._clearLoginMgrData(); + } catch (ex if ex instanceof this.STORAGE_LOCKED) { + throw ex; + } catch (ex) { + // just log and consume the error here - it may be a 3rd party login + // manager replacement that's simply broken. + log.error("Failed to get data from the login manager", ex); + } + return null; + }), +} + +// A global variable to indicate if the login manager is available - it doesn't +// exist on b2g. Defined here as the use of preprocessor directives skews line +// numbers in the runtime, meaning stack-traces etc end up off by a few lines. +// Doing it at the end of the file makes that less of a pita. +let haveLoginManager = +#if defined(MOZ_B2G) + false; +#else + true; +#endif diff --git a/services/fxaccounts/moz.build b/services/fxaccounts/moz.build index 63dedff4e0c..b56dd809271 100644 --- a/services/fxaccounts/moz.build +++ b/services/fxaccounts/moz.build @@ -12,6 +12,7 @@ XPCSHELL_TESTS_MANIFESTS += ['tests/xpcshell/xpcshell.ini'] EXTRA_JS_MODULES += [ 'Credentials.jsm', + 'FxAccounts.jsm', 'FxAccountsClient.jsm', 'FxAccountsCommon.js', 'FxAccountsOAuthClient.jsm', @@ -22,7 +23,7 @@ EXTRA_JS_MODULES += [ ] EXTRA_PP_JS_MODULES += [ - 'FxAccounts.jsm', + 'FxAccountsStorage.jsm', ] # For now, we will only be using the FxA manager in B2G. diff --git a/services/fxaccounts/tests/xpcshell/test_accounts.js b/services/fxaccounts/tests/xpcshell/test_accounts.js index 09a2adb1194..2b6b4c697ab 100644 --- a/services/fxaccounts/tests/xpcshell/test_accounts.js +++ b/services/fxaccounts/tests/xpcshell/test_accounts.js @@ -12,6 +12,9 @@ Cu.import("resource://gre/modules/FxAccountsOAuthGrantClient.jsm"); Cu.import("resource://gre/modules/Promise.jsm"); Cu.import("resource://gre/modules/Log.jsm"); +// We grab some additional stuff via backstage passes. +let {AccountState} = Cu.import("resource://gre/modules/FxAccounts.jsm", {}); + const ONE_HOUR_MS = 1000 * 60 * 60; const ONE_DAY_MS = ONE_HOUR_MS * 24; const TWO_MINUTES_MS = 1000 * 60 * 2; @@ -47,6 +50,42 @@ Services.prefs.setCharPref("identity.fxaccounts.settings.uri", CONTENT_URL); * We add the _verified attribute to mock the change in verification * state on the FXA server. */ + +function MockStorageManager() { +} + +MockStorageManager.prototype = { + promiseInitialized: Promise.resolve(), + + initialize(accountData) { + this.accountData = accountData; + }, + + finalize() { + return Promise.resolve(); + }, + + getAccountData() { + return Promise.resolve(this.accountData); + }, + + updateAccountData(updatedFields) { + for (let [name, value] of Iterator(updatedFields)) { + if (value == null) { + delete this.accountData[name]; + } else { + this.accountData[name] = value; + } + } + return Promise.resolve(); + }, + + deleteAccountData() { + this.accountData = null; + return Promise.resolve(); + } +} + function MockFxAccountsClient() { this._email = "nobody@example.com"; this._verified = false; @@ -96,25 +135,6 @@ MockFxAccountsClient.prototype = { __proto__: FxAccountsClient.prototype } -let MockStorage = function() { - this.data = null; -}; -MockStorage.prototype = Object.freeze({ - set: function (contents) { - this.data = contents; - return Promise.resolve(null); - }, - get: function () { - return Promise.resolve(this.data); - }, - getOAuthTokens() { - return Promise.resolve(null); - }, - setOAuthTokens(contents) { - return Promise.resolve(); - }, -}); - /* * We need to mock the FxAccounts module's interfaces to external * services, such as storage and the FxAccounts client. We also @@ -128,10 +148,15 @@ function MockFxAccounts() { _getCertificateSigned_calls: [], _d_signCertificate: Promise.defer(), _now_is: new Date(), - signedInUserStorage: new MockStorage(), now: function () { return this._now_is; }, + newAccountState(credentials) { + // we use a real accountState but mocked storage. + let storage = new MockStorageManager(); + storage.initialize(credentials); + return new AccountState(this, storage); + }, getCertificateSigned: function (sessionToken, serializedPublicKey) { _("mock getCertificateSigned\n"); this._getCertificateSigned_calls.push([sessionToken, serializedPublicKey]); @@ -172,9 +197,13 @@ add_test(function test_non_https_remote_server_uri() { add_task(function test_get_signed_in_user_initially_unset() { // This test, unlike many of the the rest, uses a (largely) un-mocked // FxAccounts instance. - // We do mock the storage to keep the test fast on b2g. let account = new FxAccounts({ - signedInUserStorage: new MockStorage(), + newAccountState(credentials) { + // we use a real accountState but mocked storage. + let storage = new MockStorageManager(); + storage.initialize(credentials); + return new AccountState(this, storage); + }, }); let credentials = { email: "foo@example.com", @@ -185,9 +214,6 @@ add_task(function test_get_signed_in_user_initially_unset() { kB: "cafe", verified: true }; - // and a sad hack to ensure the mocked storage is used for the initial reads. - account.internal.currentAccountState.signedInUserStorage = account.internal.signedInUserStorage; - let result = yield account.getSignedInUser(); do_check_eq(result, null); @@ -221,7 +247,12 @@ add_task(function* test_getCertificate() { // FxAccounts instance. // We do mock the storage to keep the test fast on b2g. let fxa = new FxAccounts({ - signedInUserStorage: new MockStorage(), + newAccountState(credentials) { + // we use a real accountState but mocked storage. + let storage = new MockStorageManager(); + storage.initialize(credentials); + return new AccountState(this, storage); + }, }); let credentials = { email: "foo@example.com", @@ -232,8 +263,6 @@ add_task(function* test_getCertificate() { kB: "cafe", verified: true }; - // and a sad hack to ensure the mocked storage is used for the initial reads. - fxa.internal.currentAccountState.signedInUserStorage = fxa.internal.signedInUserStorage; yield fxa.setSignedInUser(credentials); // Test that an expired cert throws if we're offline. @@ -814,7 +843,6 @@ add_task(function* test_getOAuthTokenCachedScopeNormalization() { do_check_eq(result, "token"); }); - Services.prefs.setCharPref("identity.fxaccounts.remote.oauth.uri", "https://example.com/v1"); add_test(function test_getOAuthToken_invalid_param() { let fxa = new MockFxAccounts(); @@ -967,13 +995,13 @@ add_test(function test_getSignedInUserProfile() { let mockProfile = { getProfile: function () { return Promise.resolve({ avatar: "image" }); - } + }, + tearDown: function() {}, }; - let fxa = new FxAccounts({ - _profile: mockProfile, - }); + let fxa = new FxAccounts({}); fxa.setSignedInUser(alice).then(() => { + fxa.internal._profile = mockProfile; fxa.getSignedInUserProfile() .then(result => { do_check_true(!!result); diff --git a/services/fxaccounts/tests/xpcshell/test_loginmgr_storage.js b/services/fxaccounts/tests/xpcshell/test_loginmgr_storage.js index 690556be01b..6204086131a 100644 --- a/services/fxaccounts/tests/xpcshell/test_loginmgr_storage.js +++ b/services/fxaccounts/tests/xpcshell/test_loginmgr_storage.js @@ -7,6 +7,8 @@ // Stop us hitting the real auth server. Services.prefs.setCharPref("identity.fxaccounts.auth.uri", "http://localhost"); +// See verbose logging from FxAccounts.jsm +Services.prefs.setCharPref("identity.fxaccounts.loglevel", "Trace"); Cu.import("resource://gre/modules/Services.jsm"); Cu.import("resource://gre/modules/FxAccounts.jsm"); @@ -16,9 +18,18 @@ Cu.import("resource://gre/modules/osfile.jsm"); Cu.import("resource://services-common/utils.js"); Cu.import("resource://gre/modules/FxAccountsCommon.js"); +// Use a backstage pass to get at our LoginManagerStorage object, so we can +// mock the prototype. +let {LoginManagerStorage} = Cu.import("resource://gre/modules/FxAccountsStorage.jsm", {}); +let isLoggedIn = true; +LoginManagerStorage.prototype.__defineGetter__("_isLoggedIn", () => isLoggedIn); + +function setLoginMgrLoggedInState(loggedIn) { + isLoggedIn = loggedIn; +} + + initTestLogging("Trace"); -// See verbose logging from FxAccounts.jsm -Services.prefs.setCharPref("identity.fxaccounts.loglevel", "DEBUG"); function run_test() { run_next_test(); @@ -37,6 +48,7 @@ add_task(function test_simple() { let fxa = new FxAccounts({}); let creds = { + uid: "abcd", email: "test@example.com", sessionToken: "sessionToken", kA: "the kA value", @@ -58,7 +70,7 @@ add_task(function test_simple() { Assert.ok(!("kB" in data.accountData), "kB not stored in clear text"); let login = getLoginMgrData(); - Assert.strictEqual(login.username, creds.email, "email matches"); + Assert.strictEqual(login.username, creds.email, "email used for username"); let loginData = JSON.parse(login.password); Assert.strictEqual(loginData.version, data.version, "same version flag in both places"); Assert.strictEqual(loginData.accountData.kA, creds.kA, "correct kA in the login mgr"); @@ -76,6 +88,7 @@ add_task(function test_MPLocked() { let fxa = new FxAccounts({}); let creds = { + uid: "abcd", email: "test@example.com", sessionToken: "sessionToken", kA: "the kA value", @@ -83,8 +96,9 @@ add_task(function test_MPLocked() { verified: true }; + Assert.strictEqual(getLoginMgrData(), null, "no login mgr at the start"); // tell the storage that the MP is locked. - fxa.internal.signedInUserStorage.__defineGetter__("_isLoggedIn", () => false); + setLoginMgrLoggedInState(false); yield fxa.setSignedInUser(creds); // This should have stored stuff in the .json, and the login manager stuff @@ -103,123 +117,14 @@ add_task(function test_MPLocked() { yield fxa.signOut(/* localOnly = */ true) }); -add_task(function test_migrationMPUnlocked() { - // first manually save a signedInUser.json to simulate a first-run with - // pre-migrated data. - let fxa = new FxAccounts({}); - - let creds = { - email: "test@example.com", - sessionToken: "sessionToken", - kA: "the kA value", - kB: "the kB value", - verified: true - }; - let toWrite = { - version: fxa.version, - accountData: creds, - } - - let path = OS.Path.join(OS.Constants.Path.profileDir, "signedInUser.json"); - yield CommonUtils.writeJSON(toWrite, path); - - // now load it - it should migrate. - let data = yield fxa.getSignedInUser(); - Assert.deepEqual(data, creds, "we got all the data back"); - - // and verify it was actually migrated - re-read signedInUser back. - data = yield CommonUtils.readJSON(path); - - Assert.strictEqual(data.accountData.email, creds.email, "correct email in the clear text"); - Assert.strictEqual(data.accountData.sessionToken, creds.sessionToken, "correct sessionToken in the clear text"); - Assert.strictEqual(data.accountData.verified, creds.verified, "correct verified flag"); - - Assert.ok(!("kA" in data.accountData), "kA not stored in clear text"); - Assert.ok(!("kB" in data.accountData), "kB not stored in clear text"); - - let login = getLoginMgrData(); - Assert.strictEqual(login.username, creds.email, "email matches"); - let loginData = JSON.parse(login.password); - Assert.strictEqual(loginData.version, data.version, "same version flag in both places"); - Assert.strictEqual(loginData.accountData.kA, creds.kA, "correct kA in the login mgr"); - Assert.strictEqual(loginData.accountData.kB, creds.kB, "correct kB in the login mgr"); - - Assert.ok(!("email" in loginData), "email not stored in the login mgr json"); - Assert.ok(!("sessionToken" in loginData), "sessionToken not stored in the login mgr json"); - Assert.ok(!("verified" in loginData), "verified not stored in the login mgr json"); - - yield fxa.signOut(/* localOnly = */ true); - Assert.strictEqual(getLoginMgrData(), null, "login mgr data deleted on logout"); -}); - -add_task(function test_migrationMPLocked() { - // first manually save a signedInUser.json to simulate a first-run with - // pre-migrated data. - let fxa = new FxAccounts({}); - - let creds = { - email: "test@example.com", - sessionToken: "sessionToken", - kA: "the kA value", - kB: "the kB value", - verified: true - }; - let toWrite = { - version: fxa.version, - accountData: creds, - } - - let path = OS.Path.join(OS.Constants.Path.profileDir, "signedInUser.json"); - yield CommonUtils.writeJSON(toWrite, path); - - // pretend the MP is locked. - fxa.internal.signedInUserStorage.__defineGetter__("_isLoggedIn", () => false); - - // now load it - it should *not* migrate, but should only give the JSON-safe - // data back. - let data = yield fxa.getSignedInUser(); - Assert.ok(!data.kA); - Assert.ok(!data.kB); - - // and verify the data on disk wan't migrated. - data = yield CommonUtils.readJSON(path); - Assert.deepEqual(data, toWrite); - - // Now "unlock" and re-ask for the signedInUser - it should migrate. - fxa.internal.signedInUserStorage.__defineGetter__("_isLoggedIn", () => true); - data = yield fxa.getSignedInUser(); - // this time we should have got all the data, not just the JSON-safe fields. - Assert.strictEqual(data.kA, creds.kA); - Assert.strictEqual(data.kB, creds.kB); - - // And verify the data in the JSON was migrated - data = yield CommonUtils.readJSON(path); - Assert.strictEqual(data.accountData.email, creds.email, "correct email in the clear text"); - Assert.strictEqual(data.accountData.sessionToken, creds.sessionToken, "correct sessionToken in the clear text"); - Assert.strictEqual(data.accountData.verified, creds.verified, "correct verified flag"); - - Assert.ok(!("kA" in data.accountData), "kA not stored in clear text"); - Assert.ok(!("kB" in data.accountData), "kB not stored in clear text"); - - let login = getLoginMgrData(); - Assert.strictEqual(login.username, creds.email, "email matches"); - let loginData = JSON.parse(login.password); - Assert.strictEqual(loginData.version, data.version, "same version flag in both places"); - Assert.strictEqual(loginData.accountData.kA, creds.kA, "correct kA in the login mgr"); - Assert.strictEqual(loginData.accountData.kB, creds.kB, "correct kB in the login mgr"); - - Assert.ok(!("email" in loginData), "email not stored in the login mgr json"); - Assert.ok(!("sessionToken" in loginData), "sessionToken not stored in the login mgr json"); - Assert.ok(!("verified" in loginData), "verified not stored in the login mgr json"); - - yield fxa.signOut(/* localOnly = */ true); - Assert.strictEqual(getLoginMgrData(), null, "login mgr data deleted on logout"); -}); add_task(function test_consistentWithMPEdgeCases() { + setLoginMgrLoggedInState(true); + let fxa = new FxAccounts({}); let creds1 = { + uid: "uid1", email: "test@example.com", sessionToken: "sessionToken", kA: "the kA value", @@ -228,6 +133,7 @@ add_task(function test_consistentWithMPEdgeCases() { }; let creds2 = { + uid: "uid2", email: "test2@example.com", sessionToken: "sessionToken2", kA: "the kA value2", @@ -240,7 +146,7 @@ add_task(function test_consistentWithMPEdgeCases() { // tell the storage that the MP is locked - this will prevent logout from // being able to clear the data. - fxa.internal.signedInUserStorage.__defineGetter__("_isLoggedIn", () => false); + setLoginMgrLoggedInState(false); // now set the second credentials. yield fxa.setSignedInUser(creds2); @@ -252,9 +158,9 @@ add_task(function test_consistentWithMPEdgeCases() { Assert.strictEqual(JSON.parse(login.password).accountData.kA, creds1.kA, "stale data still in login mgr"); - // Make a new FxA instance (otherwise the values in memory will be used.) - // Because we haven't overridden _isLoggedIn for this new instance it will - // treat the MP as unlocked. + // Make a new FxA instance (otherwise the values in memory will be used) + // and we want the login manager to be unlocked. + setLoginMgrLoggedInState(true); fxa = new FxAccounts({}); let accountData = yield fxa.getSignedInUser(); @@ -264,46 +170,28 @@ add_task(function test_consistentWithMPEdgeCases() { yield fxa.signOut(/* localOnly = */ true) }); -add_task(function test_migration() { - // manually write out the full creds data to the JSON - this will look like - // old data that needs migration. - let creds = { - email: "test@example.com", - sessionToken: "sessionToken", - kA: "the kA value", - kB: "the kB value", - verified: true - }; - let toWrite = { - version: 1, - accountData: creds, - }; +// A test for the fact we will accept either a UID or email when looking in +// the login manager. +add_task(function test_uidMigration() { + setLoginMgrLoggedInState(true); + Assert.strictEqual(getLoginMgrData(), null, "expect no logins at the start"); - let path = OS.Path.join(OS.Constants.Path.profileDir, "signedInUser.json"); - let data = yield CommonUtils.writeJSON(toWrite, path); + // create the login entry using uid as a key. + let contents = {kA: "kA"}; - // Create an FxA object - and tell it to load the data. - let fxa = new FxAccounts({}); - data = yield fxa.getSignedInUser(); + let loginInfo = new Components.Constructor( + "@mozilla.org/login-manager/loginInfo;1", Ci.nsILoginInfo, "init"); + let login = new loginInfo(FXA_PWDMGR_HOST, + null, // aFormSubmitURL, + FXA_PWDMGR_REALM, // aHttpRealm, + "uid", // aUsername + JSON.stringify(contents), // aPassword + "", // aUsernameField + "");// aPasswordField + Services.logins.addLogin(login); - Assert.deepEqual(data, creds, "we should have everything available"); - - // now sniff the data on disk - it should have been magically migrated. - data = yield CommonUtils.readJSON(path); - - Assert.strictEqual(data.accountData.email, creds.email, "correct email in the clear text"); - Assert.strictEqual(data.accountData.sessionToken, creds.sessionToken, "correct sessionToken in the clear text"); - Assert.strictEqual(data.accountData.verified, creds.verified, "correct verified flag"); - - Assert.ok(!("kA" in data.accountData), "kA not stored in clear text"); - Assert.ok(!("kB" in data.accountData), "kB not stored in clear text"); - - // and it should magically be in the login manager. - let login = getLoginMgrData(); - Assert.strictEqual(login.username, creds.email); - // and that we do have the first kA in the login manager. - Assert.strictEqual(JSON.parse(login.password).accountData.kA, creds.kA, - "kA was migrated"); - - yield fxa.signOut(/* localOnly = */ true) + // ensure we read it. + let storage = new LoginManagerStorage(); + let got = yield storage.get("uid", "foo@bar.com"); + Assert.deepEqual(got, contents); }); diff --git a/services/fxaccounts/tests/xpcshell/test_oauth_token_storage.js b/services/fxaccounts/tests/xpcshell/test_oauth_token_storage.js index 289bce806fc..0be796ed0ce 100644 --- a/services/fxaccounts/tests/xpcshell/test_oauth_token_storage.js +++ b/services/fxaccounts/tests/xpcshell/test_oauth_token_storage.js @@ -8,6 +8,9 @@ Cu.import("resource://gre/modules/FxAccountsClient.jsm"); Cu.import("resource://gre/modules/FxAccountsCommon.js"); Cu.import("resource://gre/modules/osfile.jsm"); +// We grab some additional stuff via backstage passes. +let {AccountState} = Cu.import("resource://gre/modules/FxAccounts.jsm", {}); + function promiseNotification(topic) { return new Promise(resolve => { let observe = () => { @@ -18,6 +21,43 @@ function promiseNotification(topic) { }); } +// A storage manager that doesn't actually write anywhere. +function MockStorageManager() { +} + +MockStorageManager.prototype = { + promiseInitialized: Promise.resolve(), + + initialize(accountData) { + this.accountData = accountData; + }, + + finalize() { + return Promise.resolve(); + }, + + getAccountData() { + return Promise.resolve(this.accountData); + }, + + updateAccountData(updatedFields) { + for (let [name, value] of Iterator(updatedFields)) { + if (value == null) { + delete this.accountData[name]; + } else { + this.accountData[name] = value; + } + } + return Promise.resolve(); + }, + + deleteAccountData() { + this.accountData = null; + return Promise.resolve(); + } +} + + // Just enough mocks so we can avoid hawk etc. function MockFxAccountsClient() { this._email = "nobody@example.com"; @@ -41,6 +81,12 @@ MockFxAccountsClient.prototype = { function MockFxAccounts() { return new FxAccounts({ fxAccountsClient: new MockFxAccountsClient(), + newAccountState(credentials) { + // we use a real accountState but mocked storage. + let storage = new MockStorageManager(); + storage.initialize(credentials); + return new AccountState(this, storage); + }, }); } @@ -82,132 +128,22 @@ add_task(function testCacheStorage() { cas.setCachedToken(scopeArray, tokenData); deepEqual(cas.getCachedToken(scopeArray), tokenData); - deepEqual(cas.getAllCachedTokens(), [tokenData]); + deepEqual(cas.oauthTokens, {"bar|foo": tokenData}); // wait for background write to complete. yield promiseWritten; - // Check the token cache was written to signedInUserOAuthTokens.json. - let path = OS.Path.join(OS.Constants.Path.profileDir, DEFAULT_OAUTH_TOKENS_FILENAME); - let data = yield CommonUtils.readJSON(path); - ok(data.tokens, "the data is in the json"); - equal(data.uid, "1234@lcip.org", "The user's uid is in the json"); - - // Check it's all in the json. - let expectedKey = "bar|foo"; - let entry = data.tokens[expectedKey]; - ok(entry, "our key is in the json"); - deepEqual(entry, tokenData, "correct token is in the json"); + // Check the token cache made it to our mocked storage. + deepEqual(cas.storageManager.accountData.oauthTokens, {"bar|foo": tokenData}); // Drop the token from the cache and ensure it is removed from the json. promiseWritten = promiseNotification("testhelper-fxa-cache-persist-done"); yield cas.removeCachedToken("token1"); - deepEqual(cas.getAllCachedTokens(), []); + deepEqual(cas.oauthTokens, {}); yield promiseWritten; - data = yield CommonUtils.readJSON(path); - ok(!data.tokens[expectedKey], "our key was removed from the json"); + deepEqual(cas.storageManager.accountData.oauthTokens, {}); // sign out and the token storage should end up with null. + let storageManager = cas.storageManager; // .signOut() removes the attribute. yield fxa.signOut( /* localOnly = */ true); - data = yield CommonUtils.readJSON(path); - ok(data === null, "data wiped on signout"); -}); - -// Test that the tokens are available after a full read of credentials from disk. -add_task(function testCacheAfterRead() { - let fxa = yield createMockFxA(); - // Hook what the impl calls to save to disk. - let cas = fxa.internal.currentAccountState; - let origPersistCached = cas._persistCachedTokens.bind(cas) - cas._persistCachedTokens = function() { - return origPersistCached().then(() => { - Services.obs.notifyObservers(null, "testhelper-fxa-cache-persist-done", null); - }); - }; - - let promiseWritten = promiseNotification("testhelper-fxa-cache-persist-done"); - let tokenData = {token: "token1", somethingelse: "something else"}; - let scopeArray = ["foo", "bar"]; - cas.setCachedToken(scopeArray, tokenData); - yield promiseWritten; - - // trick things so the data is re-read from disk. - cas.signedInUser = null; - cas.oauthTokens = null; - yield cas.getUserAccountData(); - ok(cas.oauthTokens, "token data was re-read"); - deepEqual(cas.getCachedToken(scopeArray), tokenData); -}); - -// Test that the tokens are saved after we read user credentials from disk. -add_task(function testCacheAfterRead() { - let fxa = yield createMockFxA(); - // Hook what the impl calls to save to disk. - let cas = fxa.internal.currentAccountState; - let origPersistCached = cas._persistCachedTokens.bind(cas) - - // trick things so that FxAccounts is in the mode where we're reading data - // from disk each time getSignedInUser() is called (ie, where .signedInUser - // remains null) - cas.signedInUser = null; - cas.oauthTokens = null; - - yield cas.getUserAccountData(); - - // hook our "persist" function. - cas._persistCachedTokens = function() { - return origPersistCached().then(() => { - Services.obs.notifyObservers(null, "testhelper-fxa-cache-persist-done", null); - }); - }; - let promiseWritten = promiseNotification("testhelper-fxa-cache-persist-done"); - - // save a new token - it should be persisted. - let tokenData = {token: "token1", somethingelse: "something else"}; - let scopeArray = ["foo", "bar"]; - cas.setCachedToken(scopeArray, tokenData); - - yield promiseWritten; - - // re-read the tokens directly from the storage to ensure they were persisted. - let got = yield cas.signedInUserStorage.getOAuthTokens(); - ok(got, "got persisted data"); - ok(got.tokens, "have tokens"); - // this is internal knowledge of how scopes get turned into "keys", but that's OK - ok(got.tokens["bar|foo"], "have our scope"); - equal(got.tokens["bar|foo"].token, "token1", "have our token"); -}); - -// Test that the tokens are ignored when the token storage has an incorrect uid. -add_task(function testCacheAfterReadBadUID() { - let fxa = yield createMockFxA(); - // Hook what the impl calls to save to disk. - let cas = fxa.internal.currentAccountState; - let origPersistCached = cas._persistCachedTokens.bind(cas) - cas._persistCachedTokens = function() { - return origPersistCached().then(() => { - Services.obs.notifyObservers(null, "testhelper-fxa-cache-persist-done", null); - }); - }; - - let promiseWritten = promiseNotification("testhelper-fxa-cache-persist-done"); - let tokenData = {token: "token1", somethingelse: "something else"}; - let scopeArray = ["foo", "bar"]; - cas.setCachedToken(scopeArray, tokenData); - yield promiseWritten; - - // trick things so the data is re-read from disk. - cas.signedInUser = null; - cas.oauthTokens = null; - - // re-write the tokens data with an invalid UID. - let path = OS.Path.join(OS.Constants.Path.profileDir, DEFAULT_OAUTH_TOKENS_FILENAME); - let data = yield CommonUtils.readJSON(path); - ok(data.tokens, "the data is in the json"); - equal(data.uid, "1234@lcip.org", "The user's uid is in the json"); - data.uid = "someone_else"; - yield CommonUtils.writeJSON(data, path); - - yield cas.getUserAccountData(); - deepEqual(cas.oauthTokens, {}, "token data ignored due to bad uid"); - equal(null, cas.getCachedToken(scopeArray), "no token available"); + deepEqual(storageManager.accountData, null); }); diff --git a/services/fxaccounts/tests/xpcshell/test_storage_manager.js b/services/fxaccounts/tests/xpcshell/test_storage_manager.js new file mode 100644 index 00000000000..5b414cf34f3 --- /dev/null +++ b/services/fxaccounts/tests/xpcshell/test_storage_manager.js @@ -0,0 +1,407 @@ +/* Any copyright is dedicated to the Public Domain. + * http://creativecommons.org/publicdomain/zero/1.0/ */ + +"use strict"; + +// Tests for the FxA storage manager. + +Cu.import("resource://gre/modules/Task.jsm"); +Cu.import("resource://gre/modules/Services.jsm"); +Cu.import("resource://gre/modules/FxAccountsStorage.jsm"); +Cu.import("resource://gre/modules/FxAccountsCommon.js"); +Cu.import("resource://gre/modules/Log.jsm"); + +initTestLogging("Trace"); +log.level = Log.Level.Trace; + +// A couple of mocks we can use. +function MockedPlainStorage(accountData) { + let data = null; + if (accountData) { + data = { + version: DATA_FORMAT_VERSION, + accountData: accountData, + } + } + this.data = data; + this.numReads = 0; +} +MockedPlainStorage.prototype = { + get: Task.async(function* () { + this.numReads++; + Assert.equal(this.numReads, 1, "should only ever be 1 read of acct data"); + return this.data; + }), + + set: Task.async(function* (data) { + this.data = data; + }), +}; + +function MockedSecureStorage(accountData) { + let data = null; + if (accountData) { + data = { + version: DATA_FORMAT_VERSION, + accountData: accountData, + } + } + this.data = data; + this.numReads = 0; +} + +MockedSecureStorage.prototype = { + locked: false, + STORAGE_LOCKED: function() {}, + get: Task.async(function* (uid, email) { + if (this.locked) { + throw new this.STORAGE_LOCKED(); + } + this.numReads++; + Assert.equal(this.numReads, 1, "should only ever be 1 read of unlocked data"); + return this.data; + }), + + set: Task.async(function* (uid, contents) { + this.data = contents; + }), +} + +function add_storage_task(testFunction) { + add_task(function* () { + print("Starting test with secure storage manager"); + yield testFunction(new FxAccountsStorageManager()); + }); + add_task(function* () { + print("Starting test with simple storage manager"); + yield testFunction(new FxAccountsStorageManager({useSecure: false})); + }); +} + +// initialized without account data and there's nothing to read. Not logged in. +add_storage_task(function* checkInitializedEmpty(sm) { + if (sm.secureStorage) { + sm.secureStorage = new MockedSecureStorage(null); + } + yield sm.initialize(); + Assert.strictEqual((yield sm.getAccountData()), null); + Assert.rejects(sm.updateAccountData({foo: "bar"}), "No user is logged in") +}); + +// Initialized with account data (ie, simulating a new user being logged in). +// Should reflect the initial data and be written to storage. +add_storage_task(function* checkNewUser(sm) { + let initialAccountData = { + uid: "uid", + email: "someone@somewhere.com", + kA: "kA", + }; + sm.plainStorage = new MockedPlainStorage() + if (sm.secureStorage) { + sm.secureStorage = new MockedSecureStorage(null); + } + yield sm.initialize(initialAccountData); + let accountData = yield sm.getAccountData(); + Assert.equal(accountData.uid, initialAccountData.uid); + Assert.equal(accountData.email, initialAccountData.email); + Assert.equal(accountData.kA, initialAccountData.kA); + + // and it should have been written to storage. + Assert.equal(sm.plainStorage.data.accountData.uid, initialAccountData.uid); + Assert.equal(sm.plainStorage.data.accountData.email, initialAccountData.email); + // check secure + if (sm.secureStorage) { + Assert.equal(sm.secureStorage.data.accountData.kA, initialAccountData.kA); + } else { + Assert.equal(sm.plainStorage.data.accountData.kA, initialAccountData.kA); + } +}); + +// Initialized without account data but storage has it available. +add_storage_task(function* checkEverythingRead(sm) { + sm.plainStorage = new MockedPlainStorage({uid: "uid", email: "someone@somewhere.com"}) + if (sm.secureStorage) { + sm.secureStorage = new MockedSecureStorage(null); + } + yield sm.initialize(); + let accountData = yield sm.getAccountData(); + Assert.ok(accountData, "read account data"); + Assert.equal(accountData.uid, "uid"); + Assert.equal(accountData.email, "someone@somewhere.com"); + // Update the data - we should be able to fetch it back and it should appear + // in our storage. + yield sm.updateAccountData({verified: true, foo: "bar", kA: "kA"}); + accountData = yield sm.getAccountData(); + Assert.equal(accountData.foo, "bar"); + Assert.equal(accountData.kA, "kA"); + // Check the new value was written to storage. + yield sm._promiseStorageComplete; // storage is written in the background. + // "verified" is a plain-text field. + Assert.equal(sm.plainStorage.data.accountData.verified, true); + // "kA" and "foo" are secure + if (sm.secureStorage) { + Assert.equal(sm.secureStorage.data.accountData.kA, "kA"); + Assert.equal(sm.secureStorage.data.accountData.foo, "bar"); + } else { + Assert.equal(sm.plainStorage.data.accountData.kA, "kA"); + Assert.equal(sm.plainStorage.data.accountData.foo, "bar"); + } +}); + +add_storage_task(function* checkInvalidUpdates(sm) { + sm.plainStorage = new MockedPlainStorage({uid: "uid", email: "someone@somewhere.com"}) + if (sm.secureStorage) { + sm.secureStorage = new MockedSecureStorage(null); + } + Assert.rejects(sm.updateAccountData({uid: "another"}), "Can't change"); + Assert.rejects(sm.updateAccountData({email: "someoneelse"}), "Can't change"); +}); + +add_storage_task(function* checkNullUpdatesRemovedUnlocked(sm) { + if (sm.secureStorage) { + sm.plainStorage = new MockedPlainStorage({uid: "uid", email: "someone@somewhere.com"}) + sm.secureStorage = new MockedSecureStorage({kA: "kA", kB: "kB"}); + } else { + sm.plainStorage = new MockedPlainStorage({uid: "uid", email: "someone@somewhere.com", + kA: "kA", kB: "kB"}); + } + yield sm.initialize(); + + yield sm.updateAccountData({kA: null}); + let accountData = yield sm.getAccountData(); + Assert.ok(!accountData.kA); + Assert.equal(accountData.kB, "kB"); +}); + +add_storage_task(function* checkDelete(sm) { + if (sm.secureStorage) { + sm.plainStorage = new MockedPlainStorage({uid: "uid", email: "someone@somewhere.com"}) + sm.secureStorage = new MockedSecureStorage({kA: "kA", kB: "kB"}); + } else { + sm.plainStorage = new MockedPlainStorage({uid: "uid", email: "someone@somewhere.com", + kA: "kA", kB: "kB"}); + } + yield sm.initialize(); + + yield sm.deleteAccountData(); + // Storage should have been reset to null. + Assert.equal(sm.plainStorage.data, null); + if (sm.secureStorage) { + Assert.equal(sm.secureStorage.data, null); + } + // And everything should reflect no user. + Assert.equal((yield sm.getAccountData()), null); +}); + +// Some tests only for the secure storage manager. +add_task(function* checkNullUpdatesRemovedLocked() { + let sm = new FxAccountsStorageManager(); + sm.plainStorage = new MockedPlainStorage({uid: "uid", email: "someone@somewhere.com"}) + sm.secureStorage = new MockedSecureStorage({kA: "kA", kB: "kB"}); + sm.secureStorage.locked = true; + yield sm.initialize(); + + yield sm.updateAccountData({kA: null}); + let accountData = yield sm.getAccountData(); + Assert.ok(!accountData.kA); + // still no kB as we are locked. + Assert.ok(!accountData.kB); + + // now unlock - should still be no kA but kB should appear. + sm.secureStorage.locked = false; + accountData = yield sm.getAccountData(); + Assert.ok(!accountData.kA); + Assert.equal(accountData.kB, "kB"); + // And secure storage should have been written with our previously-cached + // data. + Assert.strictEqual(sm.secureStorage.data.accountData.kA, undefined); + Assert.strictEqual(sm.secureStorage.data.accountData.kB, "kB"); +}); + +add_task(function* checkEverythingReadSecure() { + let sm = new FxAccountsStorageManager(); + sm.plainStorage = new MockedPlainStorage({uid: "uid", email: "someone@somewhere.com"}) + sm.secureStorage = new MockedSecureStorage({kA: "kA"}); + yield sm.initialize(); + + let accountData = yield sm.getAccountData(); + Assert.ok(accountData, "read account data"); + Assert.equal(accountData.uid, "uid"); + Assert.equal(accountData.email, "someone@somewhere.com"); + Assert.equal(accountData.kA, "kA"); +}); + +add_task(function* checkLockedUpdates() { + let sm = new FxAccountsStorageManager(); + sm.plainStorage = new MockedPlainStorage({uid: "uid", email: "someone@somewhere.com"}) + sm.secureStorage = new MockedSecureStorage({kA: "old-kA", kB: "kB"}); + sm.secureStorage.locked = true; + yield sm.initialize(); + + let accountData = yield sm.getAccountData(); + // requesting kA and kB will fail as storage is locked. + Assert.ok(!accountData.kA); + Assert.ok(!accountData.kB); + // While locked we can still update it and see the updated value. + sm.updateAccountData({kA: "new-kA"}); + accountData = yield sm.getAccountData(); + Assert.equal(accountData.kA, "new-kA"); + // unlock. + sm.secureStorage.locked = false; + accountData = yield sm.getAccountData(); + // should reflect the value we updated and the one we didn't. + Assert.equal(accountData.kA, "new-kA"); + Assert.equal(accountData.kB, "kB"); + // And storage should also reflect it. + Assert.strictEqual(sm.secureStorage.data.accountData.kA, "new-kA"); + Assert.strictEqual(sm.secureStorage.data.accountData.kB, "kB"); +}); + +// Some tests for the "storage queue" functionality. + +// A helper for our queued tests. It creates a StorageManager and then queues +// an unresolved promise. The tests then do additional setup and checks, then +// resolves or rejects the blocked promise. +let setupStorageManagerForQueueTest = Task.async(function* () { + let sm = new FxAccountsStorageManager(); + sm.plainStorage = new MockedPlainStorage({uid: "uid", email: "someone@somewhere.com"}) + sm.secureStorage = new MockedSecureStorage({kA: "kA"}); + sm.secureStorage.locked = true; + yield sm.initialize(); + + let resolveBlocked, rejectBlocked; + let blockedPromise = new Promise((resolve, reject) => { + resolveBlocked = resolve; + rejectBlocked = reject; + }); + + sm._queueStorageOperation(() => blockedPromise); + return {sm, blockedPromise, resolveBlocked, rejectBlocked} +}); + +// First the general functionality. +add_task(function* checkQueueSemantics() { + let { sm, resolveBlocked } = yield setupStorageManagerForQueueTest(); + + // We've one unresolved promise in the queue - add another promise. + let resolveSubsequent; + let subsequentPromise = new Promise(resolve => { + resolveSubsequent = resolve; + }); + let subsequentCalled = false; + + sm._queueStorageOperation(() => { + subsequentCalled = true; + resolveSubsequent(); + return subsequentPromise; + }); + + // Our "subsequent" function should not have been called yet. + Assert.ok(!subsequentCalled); + + // Release our blocked promise. + resolveBlocked(); + + // Our subsequent promise should end up resolved. + yield subsequentPromise; + Assert.ok(subsequentCalled); + yield sm.finalize(); +}); + +// Check that a queued promise being rejected works correctly. +add_task(function* checkQueueSemanticsOnError() { + let { sm, blockedPromise, rejectBlocked } = yield setupStorageManagerForQueueTest(); + + let resolveSubsequent; + let subsequentPromise = new Promise(resolve => { + resolveSubsequent = resolve; + }); + let subsequentCalled = false; + + sm._queueStorageOperation(() => { + subsequentCalled = true; + resolveSubsequent(); + return subsequentPromise; + }); + + // Our "subsequent" function should not have been called yet. + Assert.ok(!subsequentCalled); + + // Reject our blocked promise - the subsequent operations should still work + // correctly. + rejectBlocked("oh no"); + + // Our subsequent promise should end up resolved. + yield subsequentPromise; + Assert.ok(subsequentCalled); + + // But the first promise should reflect the rejection. + try { + yield blockedPromise; + Assert.ok(false, "expected this promise to reject"); + } catch (ex) { + Assert.equal(ex, "oh no"); + } + yield sm.finalize(); +}); + + +// And some tests for the specific operations that are queued. +add_task(function* checkQueuedReadAndUpdate() { + let { sm, resolveBlocked } = yield setupStorageManagerForQueueTest(); + // Mock the underlying operations + // _doReadAndUpdateSecure is queued by _maybeReadAndUpdateSecure + let _doReadCalled = false; + sm._doReadAndUpdateSecure = () => { + _doReadCalled = true; + return Promise.resolve(); + } + + let resultPromise = sm._maybeReadAndUpdateSecure(); + Assert.ok(!_doReadCalled); + + resolveBlocked(); + yield resultPromise; + Assert.ok(_doReadCalled); + yield sm.finalize(); +}); + +add_task(function* checkQueuedWrite() { + let { sm, resolveBlocked } = yield setupStorageManagerForQueueTest(); + // Mock the underlying operations + let __writeCalled = false; + sm.__write = () => { + __writeCalled = true; + return Promise.resolve(); + } + + let writePromise = sm._write(); + Assert.ok(!__writeCalled); + + resolveBlocked(); + yield writePromise; + Assert.ok(__writeCalled); + yield sm.finalize(); +}); + +add_task(function* checkQueuedDelete() { + let { sm, resolveBlocked } = yield setupStorageManagerForQueueTest(); + // Mock the underlying operations + let _deleteCalled = false; + sm._deleteAccountData = () => { + _deleteCalled = true; + return Promise.resolve(); + } + + let resultPromise = sm.deleteAccountData(); + Assert.ok(!_deleteCalled); + + resolveBlocked(); + yield resultPromise; + Assert.ok(_deleteCalled); + yield sm.finalize(); +}); + +function run_test() { + run_next_test(); +} diff --git a/services/fxaccounts/tests/xpcshell/xpcshell.ini b/services/fxaccounts/tests/xpcshell/xpcshell.ini index 22bdb7ae942..85855713073 100644 --- a/services/fxaccounts/tests/xpcshell/xpcshell.ini +++ b/services/fxaccounts/tests/xpcshell/xpcshell.ini @@ -21,3 +21,4 @@ reason = FxAccountsManager is only available for B2G for now [test_web_channel.js] skip-if = (appname == 'b2g' || appname == 'thunderbird') # fxa web channels only used on desktop [test_profile.js] +[test_storage_manager.js] diff --git a/services/sync/modules-testing/utils.js b/services/sync/modules-testing/utils.js index 58913e422cb..8a146b21559 100644 --- a/services/sync/modules-testing/utils.js +++ b/services/sync/modules-testing/utils.js @@ -16,6 +16,8 @@ this.EXPORTED_SYMBOLS = [ "waitForZeroTimer", "Promise", // from a module import "add_identity_test", + "MockFxaStorageManager", + "AccountState", // from a module import ]; const {utils: Cu} = Components; @@ -32,6 +34,45 @@ Cu.import("resource://gre/modules/FxAccounts.jsm"); Cu.import("resource://gre/modules/FxAccountsCommon.js"); Cu.import("resource://gre/modules/Promise.jsm"); +// and grab non-exported stuff via a backstage pass. +const {AccountState} = Cu.import("resource://gre/modules/FxAccounts.jsm", {}); + +// A mock "storage manager" for FxAccounts that doesn't actually write anywhere. +function MockFxaStorageManager() { +} + +MockFxaStorageManager.prototype = { + promiseInitialized: Promise.resolve(), + + initialize(accountData) { + this.accountData = accountData; + }, + + finalize() { + return Promise.resolve(); + }, + + getAccountData() { + return Promise.resolve(this.accountData); + }, + + updateAccountData(updatedFields) { + for (let [name, value] of Iterator(updatedFields)) { + if (value == null) { + delete this.accountData[name]; + } else { + this.accountData[name] = value; + } + } + return Promise.resolve(); + }, + + deleteAccountData() { + this.accountData = null; + return Promise.resolve(); + } +} + /** * First wait >100ms (nsITimers can take up to that much time to fire, so * we can account for the timer in delayedAutoconnect) and then two event @@ -126,23 +167,33 @@ this.makeIdentityConfig = function(overrides) { // config (or the default config if not specified). this.configureFxAccountIdentity = function(authService, config = makeIdentityConfig()) { - 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 - }; - fxa.internal.currentAccountState.getCertificate = function(data, keyPair, mustBeValidUntil) { - this.cert = { - validUntil: fxa.internal.now() + CERT_LIFETIME, - cert: "certificate", - }; - return Promise.resolve(this.cert.cert); + + let fxa; + let MockInternal = { + newAccountState(credentials) { + // We only expect this to be called with null indicating the (mock) + // storage should be read. + if (credentials) { + throw new Error("Not expecting to have credentials passed"); + } + let storageManager = new MockFxaStorageManager(); + storageManager.initialize(config.fxaccount.user); + let accountState = new AccountState(this, storageManager); + // mock getCertificate + accountState.getCertificate = function(data, keyPair, mustBeValidUntil) { + accountState.cert = { + validUntil: fxa.internal.now() + CERT_LIFETIME, + cert: "certificate", + }; + return Promise.resolve(this.cert.cert); + } + return accountState; + } }; + fxa = new FxAccounts(MockInternal); let mockTSC = { // TokenServerClient getTokenFromBrowserIDAssertion: function(uri, assertion, cb) { @@ -154,7 +205,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._signedInUser = config.fxaccount.user; authService._account = config.fxaccount.user.email; } diff --git a/services/sync/tests/unit/test_browserid_identity.js b/services/sync/tests/unit/test_browserid_identity.js index f3cde9f8f99..4a7661d10a6 100644 --- a/services/sync/tests/unit/test_browserid_identity.js +++ b/services/sync/tests/unit/test_browserid_identity.js @@ -264,8 +264,8 @@ add_task(function test_ensureLoggedIn() { // arrange for no logged in user. let fxa = browseridManager._fxaService - let signedInUser = fxa.internal.currentAccountState.signedInUser; - fxa.internal.currentAccountState.signedInUser = null; + let signedInUser = fxa.internal.currentAccountState.storageManager.accountData; + fxa.internal.currentAccountState.storageManager.accountData = null; browseridManager.initializeWithCurrentIdentity(); Assert.ok(!browseridManager._shouldHaveSyncKeyBundle, "_shouldHaveSyncKeyBundle should be false so we know we are testing what we think we are."); @@ -273,7 +273,8 @@ add_task(function test_ensureLoggedIn() { yield Assert.rejects(browseridManager.ensureLoggedIn(), "expecting rejection due to no user"); Assert.ok(browseridManager._shouldHaveSyncKeyBundle, "_shouldHaveSyncKeyBundle should always be true after ensureLogin completes."); - fxa.internal.currentAccountState.signedInUser = signedInUser; + // Restore the logged in user to what it was. + fxa.internal.currentAccountState.storageManager.accountData = signedInUser; Status.login = LOGIN_FAILED_LOGIN_REJECTED; yield Assert.rejects(browseridManager.ensureLoggedIn(), "LOGIN_FAILED_LOGIN_REJECTED should have caused immediate rejection"); @@ -585,7 +586,17 @@ add_task(function test_getKeysMissing() { fetchAndUnwrapKeys: function () { return Promise.resolve({}); }, - fxAccountsClient: new MockFxAccountsClient() + fxAccountsClient: new MockFxAccountsClient(), + newAccountState(credentials) { + // We only expect this to be called with null indicating the (mock) + // storage should be read. + if (credentials) { + throw new Error("Not expecting to have credentials passed"); + } + let storageManager = new MockFxaStorageManager(); + storageManager.initialize(identityConfig.fxaccount.user); + return new AccountState(this, storageManager); + }, }); // Add a mock to the currentAccountState object. @@ -597,9 +608,6 @@ add_task(function test_getKeysMissing() { return Promise.resolve(this.cert.cert); }; - // Ensure the new FxAccounts mock has a signed-in user. - fxa.internal.currentAccountState.signedInUser = browseridManager._fxaService.internal.currentAccountState.signedInUser; - browseridManager._fxaService = fxa; yield browseridManager.initializeWithCurrentIdentity(); @@ -658,11 +666,18 @@ function* initializeIdentityWithHAWKResponseFactory(config, cbGetResponse) { fxaClient.hawk = new MockedHawkClient(); let internal = { fxAccountsClient: fxaClient, + newAccountState(credentials) { + // We only expect this to be called with null indicating the (mock) + // storage should be read. + if (credentials) { + throw new Error("Not expecting to have credentials passed"); + } + let storageManager = new MockFxaStorageManager(); + storageManager.initialize(config.fxaccount.user); + return new AccountState(this, storageManager); + }, } let fxa = new FxAccounts(internal); - fxa.internal.currentAccountState.signedInUser = { - accountData: config.fxaccount.user, - }; browseridManager._fxaService = fxa; browseridManager._signedInUser = null;