diff --git a/browser/extensions/formautofill/test/unit/test_migrateRecords.js b/browser/extensions/formautofill/test/unit/test_migrateRecords.js index b3107e6ffa2d..c67ea1dcb0e8 100644 --- a/browser/extensions/formautofill/test/unit/test_migrateRecords.js +++ b/browser/extensions/formautofill/test/unit/test_migrateRecords.js @@ -322,3 +322,53 @@ add_task(async function test_migrateEncryptedCreditCardNumber() { Assert.ok(v1record.deleted); Assert.ok(v2record.deleted); }); + +add_task(async function test_migrateDeprecatedCreditCardV4() { + let path = getTempFile(TEST_STORE_FILE_NAME).path; + + let profileStorage = new FormAutofillStorage(path); + await profileStorage.initialize(); + + let records = [ + { + guid: "test-guid1", + version: CREDIT_CARD_SCHEMA_VERSION, + "cc-name": "Alice", + _sync: { + changeCounter: 0, + lastSyncedFields: {}, + }, + }, + { + guid: "test-guid2", + version: 4, + "cc-name": "Timothy", + _sync: { + changeCounter: 0, + lastSyncedFields: {}, + }, + }, + { + guid: "test-guid3", + version: 4, + "cc-name": "Bob", + }, + ]; + + profileStorage._store.data.creditCards = records; + for (let idx = 0; idx < records.length; idx++) { + await profileStorage.creditCards._migrateRecord(records[idx], idx); + } + + profileStorage.creditCards.pullSyncChanges(); + + // Record that has already synced before, do not sync again + equal(getSyncChangeCounter(profileStorage.creditCards, records[0].guid), 0); + + // alaways force sync v4 record + equal(records[1].version, CREDIT_CARD_SCHEMA_VERSION); + equal(getSyncChangeCounter(profileStorage.creditCards, records[1].guid), 1); + + equal(records[2].version, CREDIT_CARD_SCHEMA_VERSION); + equal(getSyncChangeCounter(profileStorage.creditCards, records[2].guid), 1); +}); diff --git a/browser/extensions/formautofill/test/unit/test_sync_deprecate_credit_card_v4.js b/browser/extensions/formautofill/test/unit/test_sync_deprecate_credit_card_v4.js new file mode 100644 index 000000000000..9f23310bbb54 --- /dev/null +++ b/browser/extensions/formautofill/test/unit/test_sync_deprecate_credit_card_v4.js @@ -0,0 +1,246 @@ +/** + * Tests sync functionality. + */ + +/* import-globals-from ../../../../../services/sync/tests/unit/head_appinfo.js */ +/* import-globals-from ../../../../../services/common/tests/unit/head_helpers.js */ +/* import-globals-from ../../../../../services/sync/tests/unit/head_helpers.js */ +/* import-globals-from ../../../../../services/sync/tests/unit/head_http_server.js */ + +"use strict"; + +const { Service } = ChromeUtils.import("resource://services-sync/service.js"); + +const { CreditCardsEngine } = ChromeUtils.import( + "resource://autofill/FormAutofillSync.jsm" +); + +Services.prefs.setCharPref("extensions.formautofill.loglevel", "Trace"); +initTestLogging("Trace"); + +const TEST_STORE_FILE_NAME = "test-profile.json"; + +const TEST_CREDIT_CARD_1 = { + guid: "86d961c7717a", + "cc-name": "John Doe", + "cc-number": "4111111111111111", + "cc-exp-month": 4, + "cc-exp-year": new Date().getFullYear(), +}; + +const TEST_CREDIT_CARD_2 = { + guid: "cf57a7ac3539", + "cc-name": "Timothy Berners-Lee", + "cc-number": "4929001587121045", + "cc-exp-month": 12, + "cc-exp-year": new Date().getFullYear() + 10, +}; + +function expectProfiles(profiles, expected) { + expected.sort((a, b) => a.guid.localeCompare(b.guid)); + profiles.sort((a, b) => a.guid.localeCompare(b.guid)); + try { + deepEqual( + profiles.map(p => p.guid), + expected.map(p => p.guid) + ); + for (let i = 0; i < expected.length; i++) { + let thisExpected = expected[i]; + let thisGot = profiles[i]; + // always check "deleted". + equal(thisExpected.deleted, thisGot.deleted); + ok(objectMatches(thisGot, thisExpected)); + } + } catch (ex) { + info("Comparing expected profiles:"); + info(JSON.stringify(expected, undefined, 2)); + info("against actual profiles:"); + info(JSON.stringify(profiles, undefined, 2)); + throw ex; + } +} + +async function expectServerProfiles(collection, expected) { + const profiles = collection + .payloads() + .map(payload => Object.assign({ guid: payload.id }, payload.entry)); + expectProfiles(profiles, expected); +} + +async function expectLocalProfiles(profileStorage, expected) { + const profiles = await profileStorage.creditCards.getAll({ + rawData: true, + includeDeleted: true, + }); + expectProfiles(profiles, expected); +} + +async function setup() { + let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME); + // should always start with no profiles. + Assert.equal( + (await profileStorage.creditCards.getAll({ includeDeleted: true })).length, + 0 + ); + + Services.prefs.setCharPref( + "services.sync.log.logger.engine.CreditCards", + "Trace" + ); + let engine = new CreditCardsEngine(Service); + await engine.initialize(); + // Avoid accidental automatic sync due to our own changes + Service.scheduler.syncThreshold = 10000000; + let syncID = await engine.resetLocalSyncID(); + let server = serverForUsers( + { foo: "password" }, + { + meta: { + global: { + engines: { creditcards: { version: engine.version, syncID } }, + }, + }, + creditcards: {}, + } + ); + + Service.engineManager._engines.creditcards = engine; + engine.enabled = true; + engine._store._storage = profileStorage.creditCards; + + generateNewKeys(Service.collectionKeys); + + await SyncTestingInfrastructure(server); + + let collection = server.user("foo").collection("creditcards"); + + return { profileStorage, server, collection, engine }; +} + +async function cleanup(server) { + let promiseStartOver = promiseOneObserver("weave:service:start-over:finish"); + await Service.startOver(); + await promiseStartOver; + await promiseStopServer(server); +} + +function getTestRecords(profileStorage, version) { + return [ + Object.assign({ version }, TEST_CREDIT_CARD_1), + Object.assign({ version }, TEST_CREDIT_CARD_2), + ]; +} + +function setupServerRecords(server, records) { + for (const record of records) { + server.insertWBO( + "foo", + "creditcards", + new ServerWBO( + record.guid, + encryptPayload({ + id: record.guid, + entry: Object.assign({}, record), + }), + Date.now() / 1000 + ) + ); + } +} + +/** + * We want to setup old records and run init() to migrate records. + * However, We don't have an easy way to setup an older version record with + * init() function now. + * So as a workaround, we simulate the behavior by directly setting data and then + * run migration. + */ +async function setupLocalProfilesAndRunMigration(profileStorage, records) { + for (const record of records) { + profileStorage._store.data.creditCards.push(Object.assign({}, record)); + } + await Promise.all( + profileStorage.creditCards._data.map(async (record, index) => + profileStorage.creditCards._migrateRecord(record, index) + ) + ); +} + +// local v3, server v4 +add_task(async function test_local_v3_server_v4() { + let { collection, profileStorage, server, engine } = await setup(); + + const V3_RECORDS = getTestRecords(profileStorage, 3); + const V4_RECORDS = getTestRecords(profileStorage, 4); + + await setupLocalProfilesAndRunMigration(profileStorage, V3_RECORDS); + setupServerRecords(server, V4_RECORDS); + + await engine.setLastSync(0); + await engine.sync(); + + await expectServerProfiles(collection, V3_RECORDS); + await expectLocalProfiles(profileStorage, V3_RECORDS); + + await cleanup(server); +}); + +// local v4, server empty +add_task(async function test_local_v4_server_empty() { + let { collection, profileStorage, server, engine } = await setup(); + const V3_RECORDS = getTestRecords(profileStorage, 3); + const V4_RECORDS = getTestRecords(profileStorage, 4); + + await setupLocalProfilesAndRunMigration(profileStorage, V4_RECORDS); + + await engine.setLastSync(0); + await engine.sync(); + + await expectServerProfiles(collection, V3_RECORDS); + await expectLocalProfiles(profileStorage, V3_RECORDS); + + await cleanup(server); +}); + +// local v4, server v3 +add_task(async function test_local_v4_server_v3() { + let { collection, profileStorage, server, engine } = await setup(); + const V3_RECORDS = getTestRecords(profileStorage, 3); + const V4_RECORDS = getTestRecords(profileStorage, 4); + + await setupLocalProfilesAndRunMigration(profileStorage, V4_RECORDS); + setupServerRecords(server, V3_RECORDS); + + // local should be v3 before syncing. + await expectLocalProfiles(profileStorage, V3_RECORDS); + + await engine.setLastSync(0); + await engine.sync(); + + await expectServerProfiles(collection, V3_RECORDS); + await expectLocalProfiles(profileStorage, V3_RECORDS); + + await cleanup(server); +}); + +// local v4, server v4 +add_task(async function test_local_v4_server_v4() { + let { collection, profileStorage, server, engine } = await setup(); + const V3_RECORDS = getTestRecords(profileStorage, 3); + const V4_RECORDS = getTestRecords(profileStorage, 4); + + await setupLocalProfilesAndRunMigration(profileStorage, V4_RECORDS); + setupServerRecords(server, V4_RECORDS); + + // local should be v3 before syncing and then we ignore + // incoming v4 from server + await expectLocalProfiles(profileStorage, V3_RECORDS); + + await engine.setLastSync(0); + await engine.sync(); + + await expectServerProfiles(collection, V3_RECORDS); + await expectLocalProfiles(profileStorage, V3_RECORDS); + + await cleanup(server); +}); diff --git a/browser/extensions/formautofill/test/unit/xpcshell.ini b/browser/extensions/formautofill/test/unit/xpcshell.ini index 0bdcbe893ebb..1c3333f43d65 100644 --- a/browser/extensions/formautofill/test/unit/xpcshell.ini +++ b/browser/extensions/formautofill/test/unit/xpcshell.ini @@ -69,3 +69,8 @@ skip-if = [test_sync.js] head = head.js ../../../../../services/sync/tests/unit/head_appinfo.js ../../../../../services/common/tests/unit/head_helpers.js ../../../../../services/sync/tests/unit/head_helpers.js ../../../../../services/sync/tests/unit/head_http_server.js skip-if = tsan # Times out, bug 1612707 +[test_sync_deprecate_credit_card_v4.js] +head = head.js ../../../../../services/sync/tests/unit/head_appinfo.js ../../../../../services/common/tests/unit/head_helpers.js ../../../../../services/sync/tests/unit/head_helpers.js ../../../../../services/sync/tests/unit/head_http_server.js +skip-if = + tsan # Times out, bug 1612707 + apple_silicon # bug 1729554 diff --git a/toolkit/components/formautofill/FormAutofillStorageBase.jsm b/toolkit/components/formautofill/FormAutofillStorageBase.jsm index 080bc5deb4bd..244cb71a7bf5 100644 --- a/toolkit/components/formautofill/FormAutofillStorageBase.jsm +++ b/toolkit/components/formautofill/FormAutofillStorageBase.jsm @@ -67,10 +67,10 @@ * cc-exp-month, * cc-exp-year, // 2-digit year will be converted to 4 digits * // upon saving + * cc-type, // Optional card network id (instrument type) * * // computed fields (These fields are computed based on the above fields * // and are not allowed to be modified directly.) - * cc-type, // Optional card network id (instrument type) * cc-given-name, * cc-additional-name, * cc-family-name, @@ -160,12 +160,20 @@ const CryptoHash = Components.Constructor( ); const STORAGE_SCHEMA_VERSION = 1; + +// NOTE: It's likely this number can never change. +// Please talk to the sync team before changing this! +// (And if it did ever change, it must never be "4" due to the reconcile hacks +// below which repairs credit-cards with version=4) const ADDRESS_SCHEMA_VERSION = 1; // Version 2: Bug 1486954 - Encrypt `cc-number` // Version 3: Bug 1639795 - Update keystore name -// Version 4: Bug 1667257 - Do not store `cc-type` field -const CREDIT_CARD_SCHEMA_VERSION = 4; +// Version 4: (deprecated!!! See Bug 1812235): Bug 1667257 - Do not store `cc-type` field +// Next version should be 5 +// NOTE: It's likely this number can never change. +// Please talk to the sync team before changing this! +const CREDIT_CARD_SCHEMA_VERSION = 3; const VALID_ADDRESS_FIELDS = [ "given-name", @@ -208,10 +216,10 @@ const VALID_CREDIT_CARD_FIELDS = [ "cc-number", "cc-exp-month", "cc-exp-year", + "cc-type", ]; const VALID_CREDIT_CARD_COMPUTED_FIELDS = [ - "cc-type", "cc-given-name", "cc-additional-name", "cc-family-name", @@ -971,6 +979,30 @@ class AutofillRecords { let forkedGUID = null; + // NOTE: This implies a credit-card - so it's critical ADDRESS_SCHEMA_VERSION + // never equals 4 while this code exists! + let requiresForceUpdate = + localRecord.version != remoteRecord.version && remoteRecord.version == 4; + + if (requiresForceUpdate) { + // Another desktop device that is still using version=4 has created or + // modified a remote record. Here we downgrade it to version=3 so we can + // treat it normally, then cause it to be re-uploaded so other desktop + // or mobile devices can still see it. + // That device still using version=4 *will* again see it, and again + // upgrade it, but thankfully that 3->4 migration doesn't force a reupload + // of all records, or we'd be going back and forward on every sync. + // Once that version=4 device gets updated to roll back to version=3, it + // will then yet again re-upload it, this time with version=3, but the + // content will be the same here, so everything should work out in the end. + // + // If we just ignored this incoming record, it would remain on the server + // with version=4. If the device that wrote that went away (ie, never + // synced again) nothing would ever repair it back to 3, which would + // be bad because mobile would remain broken until the user edited the + // card somewhere. + remoteRecord = await this._computeMigratedRecord(remoteRecord); + } if (sync.changeCounter === 0) { // Local not modified. Replace local with remote. await this._replaceRecordAt(localIndex, remoteRecord, { @@ -1003,6 +1035,18 @@ class AutofillRecords { } } + if (requiresForceUpdate) { + // The incoming record was version=4 and we want to re-upload it as version=3. + // We need to reach directly into self._data[] so we can poke at the + // sync metadata directly. + let indexToUpdate = this._findIndexByGUID(remoteRecord.guid); + let toUpdate = this._data[indexToUpdate]; + this._getSyncMetaData(toUpdate, true).changeCounter += 1; + this.log.info( + `Flagging record ${toUpdate.guid} for re-upload after record version downgrade` + ); + } + this._store.saveSoon(); Services.obs.notifyObservers( { @@ -1314,7 +1358,7 @@ class AutofillRecords { record.version = 0; } - if (record.version < this.version) { + if (this._isMigrationNeeded(record.version)) { hasChanges = true; record = await this._computeMigratedRecord(record); @@ -1405,6 +1449,10 @@ class AutofillRecords { ); } + _isMigrationNeeded(recordVersion) { + return recordVersion < this.version; + } + /** * Strip the computed fields based on the record version. * @@ -1759,6 +1807,13 @@ class CreditCardsBase extends AutofillRecords { throw Components.Exception("", Cr.NS_ERROR_NOT_IMPLEMENTED); } + _isMigrationNeeded(recordVersion) { + return ( + // version 4 is deprecated and is rolled back to version 3 + recordVersion == 4 || recordVersion < this.version + ); + } + async _computeMigratedRecord(creditCard) { if (creditCard.version <= 2) { if (creditCard["cc-number-encrypted"]) { @@ -1789,9 +1844,15 @@ class CreditCardsBase extends AutofillRecords { } } - if (creditCard.version <= 3) { - if (creditCard["cc-type"]) { - delete creditCard["cc-type"]; + // Do not remove the migration code until we're sure no users have version 4 + // credit card records (created in Fx110 or Fx111) + if (creditCard.version == 4) { + // Version 4 is deprecated, so downgrade or upgrade to the current version + // Since the only change made in version 4 is deleting `cc-type` field, so + // nothing else need to be done here expect flagging sync needed + let existingSync = this._getSyncMetaData(creditCard); + if (existingSync) { + existingSync.changeCounter++; } } @@ -1885,7 +1946,11 @@ class CreditCardsBase extends AutofillRecords { ); } - if (record.version < this.version) { + if (record.version == 4) { + // Version 4 is deprecated, we need to force downloading it from sync + // and let migration do the work to downgrade it back to the current version. + return true; + } else if (record.version < this.version) { switch (record.version) { case 1: case 2: