Bug 1812235 - Rollback credit card record version from 4 to 3 r=markh,sgalich

Mobile clients reject syncing when credit card record version is not 3, so this patch
reverts the credit card record version back to 3 (with `cc-type` field).

The auto-detect network type behavior implemented in bug 1667257 still applies.
The changes made in this commit are:
1. Save the cc-type to storage to comply with the expectation of mobile clients.
   (This is the behavior for v3 credit record)
2. When a v4 record is found, rollback to v3 and make sure `_sync.changeCounter` is set
   so we upload the downgraded record to the sync server

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

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);
+});

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);
+});

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

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: