From 86c67f9b9081ce905442c86b38575b3422c8dce3 Mon Sep 17 00:00:00 2001 From: Mark Hammond Date: Wed, 18 Jun 2014 15:07:41 +1000 Subject: Bug 1013064 (part 3) - only migrate data into the loginmgr when it is unlocked. r=ckarlof --- services/fxaccounts/FxAccounts.jsm | 25 ++++- .../tests/xpcshell/test_loginmgr_storage.js | 113 +++++++++++++++++++++ 2 files changed, 137 insertions(+), 1 deletion(-) diff --git a/services/fxaccounts/FxAccounts.jsm b/services/fxaccounts/FxAccounts.jsm index e9328dd..14d71fc 100644 --- a/services/fxaccounts/FxAccounts.jsm +++ b/services/fxaccounts/FxAccounts.jsm @@ -1025,17 +1025,40 @@ LoginManagerStorage.prototype = { // 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) { - log.info("account data needs migration to the login manager."); + // 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) { diff --git a/services/fxaccounts/tests/xpcshell/test_loginmgr_storage.js b/services/fxaccounts/tests/xpcshell/test_loginmgr_storage.js index 297b256..a9cf5f4 100644 --- a/services/fxaccounts/tests/xpcshell/test_loginmgr_storage.js +++ b/services/fxaccounts/tests/xpcshell/test_loginmgr_storage.js @@ -98,16 +98,129 @@ add_task(function test_MPLocked() { Assert.ok(!("kA" in data.accountData), "kA not stored in clear text"); Assert.ok(!("kB" in data.accountData), "kB not stored in clear text"); Assert.strictEqual(getLoginMgrData(), null, "login mgr data doesn't exist"); 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. + let 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", function() 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", function() 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() { let fxa = new FxAccounts({}); let creds1 = { email: "test@example.com", sessionToken: "sessionToken", kA: "the kA value", kB: "the kB value", -- 1.8.3.msysgit.0