Bug 1486954 - Part III, Upgrade existing Nightly credit card records to OSKeyStore. r=MattN

For Nightly users who already have credit cards saved in their profile, we will do a one-off upgrade of their encrypted credit card number.
This only applies to users who have NO master password set, to avoid showing them the master password prompt when we migrate.
For those who did, we would quietly delete their credit card record from the store.

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

--HG--
extra : rebase_source : 2b06e3fc418d5883276850def18966203c16e240
extra : histedit_source : 9fae8d61b747113e8b3f38c2886cf7d1a7c3007f
This commit is contained in:
Matthew Noorenberghe 2018-10-22 22:57:29 -07:00
parent 001567e898
commit 566350c6ac
5 changed files with 249 additions and 59 deletions

View File

@ -327,7 +327,7 @@ let BASIC_CARDS_1 = {
methodName: "basic-card",
"cc-number": "************5461",
"guid": "53f9d009aed2",
"version": 1,
"version": 2,
"timeCreated": 1505240896213,
"timeLastModified": 1515609524588,
"timeLastUsed": 10000,
@ -345,7 +345,7 @@ let BASIC_CARDS_1 = {
methodName: "basic-card",
"cc-number": "************0954",
"guid": "9h5d4h6f4d1s",
"version": 1,
"version": 2,
"timeCreated": 1517890536491,
"timeLastModified": 1517890564518,
"timeLastUsed": 50000,
@ -363,7 +363,7 @@ let BASIC_CARDS_1 = {
methodName: "basic-card",
"cc-number": "************1234",
"guid": "123456789abc",
"version": 1,
"version": 2,
"timeCreated": 1517890536491,
"timeLastModified": 1517890564518,
"timeLastUsed": 90000,
@ -397,7 +397,7 @@ let BASIC_CARDS_1 = {
methodName: "basic-card",
"cc-number": "************8563",
"guid": "missing-cc-name",
"version": 1,
"version": 2,
"timeCreated": 1517890536491,
"timeLastModified": 1517890564518,
"timeLastUsed": 30000,

View File

@ -147,6 +147,9 @@ ChromeUtils.defineModuleGetter(this, "OSKeyStore",
ChromeUtils.defineModuleGetter(this, "PhoneNumber",
"resource://formautofill/phonenumberutils/PhoneNumber.jsm");
XPCOMUtils.defineLazyServiceGetter(this, "cryptoSDR",
"@mozilla.org/login-manager/crypto/SDR;1",
Ci.nsILoginManagerCrypto);
XPCOMUtils.defineLazyServiceGetter(this, "gUUIDGenerator",
"@mozilla.org/uuid-generator;1",
"nsIUUIDGenerator");
@ -158,7 +161,7 @@ const PROFILE_JSON_FILE_NAME = "autofill-profiles.json";
const STORAGE_SCHEMA_VERSION = 1;
const ADDRESS_SCHEMA_VERSION = 1;
const CREDIT_CARD_SCHEMA_VERSION = 1;
const CREDIT_CARD_SCHEMA_VERSION = 2;
const VALID_ADDRESS_FIELDS = [
"given-name",
@ -264,13 +267,14 @@ class AutofillRecords {
this._collectionName = collectionName;
this._schemaVersion = schemaVersion;
Promise.all(this._data.map(record => this._migrateRecord(record)))
.then(hasChangesArr => {
let dataHasChanges = hasChangesArr.find(hasChanges => hasChanges);
if (dataHasChanges) {
this._store.saveSoon();
}
});
this._initializePromise =
Promise.all(this._data.map(async (record, index) => this._migrateRecord(record, index)))
.then(hasChangesArr => {
let dataHasChanges = hasChangesArr.includes(true);
if (dataHasChanges) {
this._store.saveSoon();
}
});
}
/**
@ -303,6 +307,14 @@ class AutofillRecords {
}
}
/**
* Initialize the records in the collection, resolves when the migration completes.
* @returns {Promise}
*/
initialize() {
return this._initializePromise;
}
/**
* Adds a new record.
*
@ -1176,7 +1188,7 @@ class AutofillRecords {
});
}
async _migrateRecord(record) {
async _migrateRecord(record, index) {
let hasChanges = false;
if (record.deleted) {
@ -1192,10 +1204,21 @@ class AutofillRecords {
if (record.version < this.version) {
hasChanges = true;
record.version = this.version;
// Force to recompute fields if we upgrade the schema.
await this._stripComputedFields(record);
record = await this._computeMigratedRecord(record);
if (record.deleted) {
// record is deleted by _computeMigratedRecord(),
// go ahead and put it in the store.
this._data[index] = record;
return hasChanges;
}
// Compute the computed fields before putting it to store.
await this.computeFields(record);
this._data[index] = record;
return hasChanges;
}
hasChanges |= await this.computeFields(record);
@ -1256,6 +1279,24 @@ class AutofillRecords {
}}, "formautofill-storage-changed", "removeAll");
}
/**
* Strip the computed fields based on the record version.
* @param {Object} record The record to migrate
* @returns {Object} Migrated record.
* Record is always cloned, with version updated,
* with computed fields stripped.
* Could be a tombstone record, if the record
* should be discorded.
*/
async _computeMigratedRecord(record) {
if (!record.deleted) {
record = this._clone(record);
await this._stripComputedFields(record);
record.version = this.version;
}
return record;
}
async _stripComputedFields(record) {
this.VALID_COMPUTED_FIELDS.forEach(field => delete record[field]);
}
@ -1613,6 +1654,51 @@ class CreditCards extends AutofillRecords {
return hasNewComputedFields;
}
async _computeMigratedRecord(creditCard) {
if (creditCard["cc-number-encrypted"]) {
switch (creditCard.version) {
case 1: {
if (!cryptoSDR.isLoggedIn) {
// We cannot decrypt the data, so silently remove the record for
// the user.
if (creditCard.deleted) {
break;
}
this.log.warn("Removing version 1 credit card record to migrate to new encryption:", creditCard.guid);
// Replace the record with a tombstone record here,
// regardless of existence of sync metadata.
let existingSync = this._getSyncMetaData(creditCard);
creditCard = {
guid: creditCard.guid,
timeLastModified: Date.now(),
deleted: true,
};
if (existingSync) {
creditCard._sync = existingSync;
existingSync.changeCounter++;
}
break;
}
creditCard = this._clone(creditCard);
// Decrypt the cc-number using version 1 encryption.
let ccNumber = cryptoSDR.decrypt(creditCard["cc-number-encrypted"]);
// Re-encrypt the cc-number with version 2 encryption.
creditCard["cc-number-encrypted"] = await OSKeyStore.encrypt(ccNumber);
break;
}
default:
throw new Error("Unknown credit card version to migrate: " + creditCard.version);
}
}
return super._computeMigratedRecord(creditCard);
}
async _stripComputedFields(creditCard) {
if (creditCard["cc-number-encrypted"]) {
creditCard["cc-number"] = await OSKeyStore.decrypt(creditCard["cc-number-encrypted"]);
@ -1676,6 +1762,33 @@ class CreditCards extends AutofillRecords {
}
}
_ensureMatchingVersion(record) {
if (!record.version || isNaN(record.version) || record.version < 1) {
throw new Error(`Got invalid record version ${
record.version}; want ${this.version}`);
}
if (record.version < this.version) {
switch (record.version) {
case 1:
// The difference between version 1 and 2 is only about the encryption
// method used for the cc-number-encrypted field.
// As long as the record is already decrypted, it is safe to bump the
// version directly.
if (!record["cc-number-encrypted"]) {
record.version = this.version;
} else {
throw new Error("Unexpected record migration path.");
}
break;
default:
throw new Error("Unknown credit card version to match: " + record.version);
}
}
return super._ensureMatchingVersion(record);
}
/**
* Normalize the given record and return the first matched guid if storage has the same record.
* @param {Object} targetCreditCard
@ -1802,7 +1915,10 @@ FormAutofillStorage.prototype = {
path: this._path,
dataPostProcessor: this._dataPostProcessor.bind(this),
});
this._initializePromise = this._store.load();
this._initializePromise = this._store.load()
.then(() => Promise.all([
this.addresses.initialize(),
this.creditCards.initialize()]));
}
return this._initializePromise;
},

View File

@ -316,7 +316,7 @@ add_task(async function test_add() {
do_check_credit_card_matches(creditCards[1], TEST_CREDIT_CARD_2);
Assert.notEqual(creditCards[0].guid, undefined);
Assert.equal(creditCards[0].version, 1);
Assert.equal(creditCards[0].version, 2);
Assert.notEqual(creditCards[0].timeCreated, undefined);
Assert.equal(creditCards[0].timeLastModified, creditCards[0].timeCreated);
Assert.equal(creditCards[0].timeLastUsed, 0);

View File

@ -4,16 +4,19 @@
"use strict";
let FormAutofillStorage;
ChromeUtils.import("resource://testing-common/LoginTestUtils.jsm", this);
let FormAutofillStorage;
let OSKeyStore;
add_task(async function setup() {
({FormAutofillStorage} = ChromeUtils.import("resource://formautofill/FormAutofillStorage.jsm", {}));
({OSKeyStore} = ChromeUtils.import("resource://formautofill/OSKeyStore.jsm", {}));
});
const TEST_STORE_FILE_NAME = "test-profile.json";
const ADDRESS_SCHEMA_VERSION = 1;
const CREDIT_CARD_SCHEMA_VERSION = 1;
const CREDIT_CARD_SCHEMA_VERSION = 2;
const ADDRESS_TESTCASES = [
{
@ -246,11 +249,12 @@ add_task(async function test_migrateAddressRecords() {
let profileStorage = new FormAutofillStorage(path);
await profileStorage.initialize();
await Promise.all(ADDRESS_TESTCASES.map(async testcase => {
for (let testcase of ADDRESS_TESTCASES) {
info(testcase.description);
await profileStorage.addresses._migrateRecord(testcase.record);
do_check_record_matches(testcase.expectedResult, testcase.record);
}));
profileStorage._store.data.addresses = [testcase.record];
await profileStorage.addresses._migrateRecord(testcase.record, 0);
do_check_record_matches(testcase.expectedResult, profileStorage.addresses._data[0]);
}
});
add_task(async function test_migrateCreditCardRecords() {
@ -259,9 +263,79 @@ add_task(async function test_migrateCreditCardRecords() {
let profileStorage = new FormAutofillStorage(path);
await profileStorage.initialize();
await Promise.all(CREDIT_CARD_TESTCASES.map(async testcase => {
for (let testcase of CREDIT_CARD_TESTCASES) {
info(testcase.description);
await profileStorage.creditCards._migrateRecord(testcase.record);
do_check_record_matches(testcase.expectedResult, testcase.record);
}));
profileStorage._store.data.creditCards = [testcase.record];
await profileStorage.creditCards._migrateRecord(testcase.record, 0);
do_check_record_matches(testcase.expectedResult, profileStorage.creditCards._data[0]);
}
});
add_task(async function test_migrateEncryptedCreditCardNumber() {
let path = getTempFile(TEST_STORE_FILE_NAME).path;
let profileStorage = new FormAutofillStorage(path);
await profileStorage.initialize();
const ccNumber = "4111111111111111";
let cryptoSDR = Cc["@mozilla.org/login-manager/crypto/SDR;1"]
.createInstance(Ci.nsILoginManagerCrypto);
info("Encrypted credit card should be migrated from v1 to v2");
let record = {
guid: "test-guid",
version: 1,
"cc-name": "Timothy",
"cc-number-encrypted": cryptoSDR.encrypt(ccNumber),
};
let expectedRecord = {
guid: "test-guid",
version: CREDIT_CARD_SCHEMA_VERSION,
"cc-name": "Timothy",
"cc-given-name": "Timothy",
};
profileStorage._store.data.creditCards = [record];
await profileStorage.creditCards._migrateRecord(record, 0);
record = profileStorage.creditCards._data[0];
Assert.equal(expectedRecord.guid, record.guid);
Assert.equal(expectedRecord.version, record.version);
Assert.equal(expectedRecord["cc-name"], record["cc-name"]);
Assert.equal(expectedRecord["cc-given-name"], record["cc-given-name"]);
// Ciphertext of OS Key Store is not stable, must compare decrypted text here.
Assert.equal(ccNumber, await OSKeyStore.decrypt(record["cc-number-encrypted"]));
});
add_task(async function test_migrateEncryptedCreditCardNumberWithMP() {
LoginTestUtils.masterPassword.enable();
let path = getTempFile(TEST_STORE_FILE_NAME).path;
let profileStorage = new FormAutofillStorage(path);
await profileStorage.initialize();
info("Encrypted credit card should be migrated a tombstone if MP is enabled");
let record = {
guid: "test-guid",
version: 1,
"cc-name": "Timothy",
"cc-number-encrypted": "(encrypted to be discarded)",
};
profileStorage._store.data.creditCards = [record];
await profileStorage.creditCards._migrateRecord(record, 0);
record = profileStorage.creditCards._data[0];
Assert.equal(record.guid, "test-guid");
Assert.equal(record.deleted, true);
Assert.equal(typeof record.version, "undefined");
Assert.equal(typeof record["cc-name"], "undefined");
Assert.equal(typeof record["cc-number-encrypted"], "undefined");
LoginTestUtils.masterPassword.disable();
});

View File

@ -470,7 +470,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
parent: {
// So when we last wrote the record to the server, it had these values.
"guid": "2bbd2d8fbc6b",
"version": 1,
"version": 2,
"cc-name": "John Doe",
"cc-number": "4111111111111111",
},
@ -485,7 +485,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
// we can deduce the record hasn't actually been changed remotely so we
// can safely ignore the incoming record and write our local changes.
"guid": "2bbd2d8fbc6b",
"version": 1,
"version": 2,
"cc-name": "John Doe",
"cc-number": "4111111111111111",
},
@ -499,7 +499,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
description: "Remote change",
parent: {
"guid": "e3680e9f890d",
"version": 1,
"version": 2,
"cc-name": "John Doe",
"cc-number": "4111111111111111",
},
@ -509,7 +509,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
}],
remote: {
"guid": "e3680e9f890d",
"version": 1,
"version": 2,
"cc-name": "John Doe",
"cc-number": "4929001587121045",
},
@ -524,7 +524,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
description: "New local field",
parent: {
"guid": "0cba738b1be0",
"version": 1,
"version": 2,
"cc-name": "John Doe",
"cc-number": "4111111111111111",
},
@ -535,7 +535,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
}],
remote: {
"guid": "0cba738b1be0",
"version": 1,
"version": 2,
"cc-name": "John Doe",
"cc-number": "4111111111111111",
},
@ -550,7 +550,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
description: "New remote field",
parent: {
"guid": "be3ef97f8285",
"version": 1,
"version": 2,
"cc-name": "John Doe",
"cc-number": "4111111111111111",
},
@ -560,7 +560,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
}],
remote: {
"guid": "be3ef97f8285",
"version": 1,
"version": 2,
"cc-name": "John Doe",
"cc-number": "4111111111111111",
"cc-exp-month": 12,
@ -576,7 +576,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
description: "Deleted field locally",
parent: {
"guid": "9627322248ec",
"version": 1,
"version": 2,
"cc-name": "John Doe",
"cc-number": "4111111111111111",
"cc-exp-month": 12,
@ -587,7 +587,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
}],
remote: {
"guid": "9627322248ec",
"version": 1,
"version": 2,
"cc-name": "John Doe",
"cc-number": "4111111111111111",
"cc-exp-month": 12,
@ -602,7 +602,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
description: "Deleted field remotely",
parent: {
"guid": "7d7509f3eeb2",
"version": 1,
"version": 2,
"cc-name": "John Doe",
"cc-number": "4111111111111111",
"cc-exp-month": 12,
@ -614,7 +614,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
}],
remote: {
"guid": "7d7509f3eeb2",
"version": 1,
"version": 2,
"cc-name": "John Doe",
"cc-number": "4111111111111111",
},
@ -629,7 +629,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
parent: {
// The last time we wrote this to the server, "cc-exp-month" was 12.
"guid": "e087a06dfc57",
"version": 1,
"version": 2,
"cc-name": "John Doe",
"cc-number": "4111111111111111",
"cc-exp-month": 12,
@ -643,7 +643,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
remote: {
// Remotely, we've changed "cc-exp-month" to 1.
"guid": "e087a06dfc57",
"version": 1,
"version": 2,
"cc-name": "John Doe",
"cc-number": "4111111111111111",
"cc-exp-month": 1,
@ -659,7 +659,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
description: "Multiple local changes",
parent: {
"guid": "340a078c596f",
"version": 1,
"version": 2,
"cc-name": "John Doe",
"cc-number": "4111111111111111",
},
@ -673,7 +673,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
}],
remote: {
"guid": "340a078c596f",
"version": 1,
"version": 2,
"cc-name": "John Doe",
"cc-number": "4111111111111111",
"cc-exp-year": 2000,
@ -692,7 +692,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
description: "Same change to local and remote",
parent: {
"guid": "0b3a72a1bea2",
"version": 1,
"version": 2,
"cc-name": "John Doe",
"cc-number": "4111111111111111",
},
@ -702,7 +702,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
}],
remote: {
"guid": "0b3a72a1bea2",
"version": 1,
"version": 2,
"cc-name": "John Doe",
"cc-number": "4929001587121045",
},
@ -717,7 +717,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
parent: {
// This is what we last wrote to the sync server.
"guid": "62068784d089",
"version": 1,
"version": 2,
"cc-name": "John Doe",
"cc-number": "4111111111111111",
},
@ -729,7 +729,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
remote: {
// An incoming record has a different cc-number than any of the above!
"guid": "62068784d089",
"version": 1,
"version": 2,
"cc-name": "John Doe",
"cc-number": "4929001587121045",
},
@ -750,7 +750,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
description: "Conflicting changes to multiple fields",
parent: {
"guid": "244dbb692e94",
"version": 1,
"version": 2,
"cc-name": "John Doe",
"cc-number": "4111111111111111",
"cc-exp-month": 12,
@ -762,7 +762,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
}],
remote: {
"guid": "244dbb692e94",
"version": 1,
"version": 2,
"cc-name": "John Doe",
"cc-number": "4929001587121045",
"cc-exp-month": 3,
@ -783,7 +783,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
description: "Field deleted locally, changed remotely",
parent: {
"guid": "6fc45e03d19a",
"version": 1,
"version": 2,
"cc-name": "John Doe",
"cc-number": "4111111111111111",
"cc-exp-month": 12,
@ -794,7 +794,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
}],
remote: {
"guid": "6fc45e03d19a",
"version": 1,
"version": 2,
"cc-name": "John Doe",
"cc-number": "4111111111111111",
"cc-exp-month": 3,
@ -814,7 +814,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
description: "Field changed locally, deleted remotely",
parent: {
"guid": "fff9fa27fa18",
"version": 1,
"version": 2,
"cc-name": "John Doe",
"cc-number": "4111111111111111",
"cc-exp-month": 12,
@ -826,7 +826,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
}],
remote: {
"guid": "fff9fa27fa18",
"version": 1,
"version": 2,
"cc-name": "John Doe",
"cc-number": "4111111111111111",
},
@ -848,7 +848,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
description: "Created, last modified time reconciliation without local changes",
parent: {
"guid": "5113f329c42f",
"version": 1,
"version": 2,
"cc-name": "John Doe",
"cc-number": "4111111111111111",
"timeCreated": 1234,
@ -859,7 +859,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
local: [],
remote: {
"guid": "5113f329c42f",
"version": 1,
"version": 2,
"cc-name": "John Doe",
"cc-number": "4111111111111111",
"timeCreated": 1200,
@ -883,7 +883,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
description: "Created, last modified time reconciliation with local changes",
parent: {
"guid": "791e5608b80a",
"version": 1,
"version": 2,
"cc-name": "John Doe",
"cc-number": "4111111111111111",
"timeCreated": 1234,
@ -897,7 +897,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
}],
remote: {
"guid": "791e5608b80a",
"version": 1,
"version": 2,
"cc-name": "John Doe",
"cc-number": "4111111111111111",
"timeCreated": 1300,
@ -922,7 +922,7 @@ add_task(async function test_reconcile_unknown_version() {
// Cross-version reconciliation isn't supported yet. See bug 1377204.
await Assert.rejects(profileStorage.addresses.reconcile({
"guid": "31d83d2725ec",
"version": 2,
"version": 3,
"given-name": "Mark",
"family-name": "Hammond",
}), /Got unknown record version/);