Bug 1647652 - Treat a new record as a duplicate of an existing one if the cc-number matches. r=abr

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

browser/extensions/formautofill/FormAutofillParent.jsm

@@ -593,9 +593,7 @@ class FormAutofillParent extends JSWindowActorParent {
       creditCard.record["cc-type"] = "";
     }
 
-    // We'll show the credit card doorhanger if:
-    //   - User applys autofill and changed
-    //   - User fills form manually and the filling data is not duplicated to storage
+    // If `guid` is present, the form has been autofilled.
     if (creditCard.guid) {
       // Indicate that the user has used Credit Card Autofill to fill in a form.
       setUsedStatus(3);
@@ -643,26 +641,48 @@ class FormAutofillParent extends JSWindowActorParent {
         timeStartedFillingMS
       );
     } else {
-      // Indicate that the user neither sees the doorhanger nor uses Autofill
-      // but somehow has a duplicate record in the storage. Will be reset to 2
-      // if the doorhanger actually shows below.
-      setUsedStatus(1);
-
       // Add the probe to record credit card manual filling.
       Services.telemetry.scalarAdd(
         "formautofill.creditCards.fill_type_manual",
         1
       );
       this._recordFormFillingTime("creditCard", "manual", timeStartedFillingMS);
-    }
 
-    // Early return if it's a duplicate data
-    let dupGuid = await gFormAutofillStorage.creditCards.getDuplicateGuid(
-      creditCard.record
-    );
-    if (dupGuid) {
-      gFormAutofillStorage.creditCards.notifyUsed(dupGuid);
-      return false;
+      let existingGuid = await gFormAutofillStorage.creditCards.getDuplicateGuid(
+        creditCard.record
+      );
+
+      if (existingGuid) {
+        creditCard.guid = existingGuid;
+
+        let originalCCData = await gFormAutofillStorage.creditCards.get(
+          creditCard.guid
+        );
+
+        gFormAutofillStorage.creditCards._normalizeRecord(creditCard.record);
+
+        // If the credit card record is a duplicate, check if the fields match the
+        // record.
+        let recordUnchanged = true;
+        for (let field in creditCard.record) {
+          if (field == "cc-number") {
+            continue;
+          }
+          if (creditCard.record[field] != originalCCData[field]) {
+            recordUnchanged = false;
+            break;
+          }
+        }
+
+        if (recordUnchanged) {
+          // Indicate that the user neither sees the doorhanger nor uses Autofill
+          // but somehow has a duplicate record in the storage. Will be reset to 2
+          // if the doorhanger actually shows below.
+          setUsedStatus(1);
+          gFormAutofillStorage.creditCards.notifyUsed(creditCard.guid);
+          return false;
+        }
+      }
     }
 
     // Indicate that the user has seen the doorhanger.

browser/extensions/formautofill/FormAutofillStorage.jsm

@@ -1996,24 +1996,21 @@ class CreditCards extends AutofillRecords {
   async getDuplicateGuid(targetCreditCard) {
     let clonedTargetCreditCard = this._clone(targetCreditCard);
     this._normalizeRecord(clonedTargetCreditCard);
+    if (!clonedTargetCreditCard["cc-number"]) {
+      return null;
+    }
+
     for (let creditCard of this._data) {
-      let isDuplicate = await Promise.all(
-        this.VALID_FIELDS.map(async field => {
-          if (!clonedTargetCreditCard[field]) {
-            return !creditCard[field];
-          }
-          if (field == "cc-number" && creditCard[field]) {
-            // Compare the masked numbers instead when decryption requires a password
-            // because we don't want to leak the credit card number.
-            return (
-              CreditCard.getLongMaskedNumber(clonedTargetCreditCard[field]) ==
-              creditCard[field]
-            );
-          }
-          return clonedTargetCreditCard[field] == creditCard[field];
-        })
-      ).then(fieldResults => fieldResults.every(result => result));
-      if (isDuplicate) {
+      if (creditCard.deleted) {
+        continue;
+      }
+
+      let decrypted = await OSKeyStore.decrypt(
+        creditCard["cc-number-encrypted"],
+        false
+      );
+
+      if (decrypted == clonedTargetCreditCard["cc-number"]) {
         return creditCard.guid;
       }
     }

browser/extensions/formautofill/test/browser/creditCard/browser_creditCard_doorhanger.js

@@ -302,11 +302,7 @@ add_task(async function test_submit_changed_subset_creditCard_form() {
 
   creditCards = await getCreditCards();
   is(creditCards.length, 1, "Still 1 credit card in storage");
-  is(
-    creditCards[0]["cc-name"],
-    TEST_CREDIT_CARD_1["cc-name"],
-    "name field still exists"
-  );
+  is(creditCards[0]["cc-name"], undefined, "name field got cleared");
   is(
     SpecialPowers.getIntPref(CREDITCARDS_USED_STATUS_PREF),
     2,

browser/extensions/formautofill/test/unit/test_creditCardRecords.js

@@ -785,15 +785,15 @@ add_task(async function test_getDuplicateGuid() {
     null
   );
 
-  // Subset shouldn't be treated as a duplicate.
+  // Subset with the same number is a duplicate.
   let record = Object.assign({}, TEST_CREDIT_CARD_3);
   delete record["cc-exp-month"];
-  Assert.equal(await profileStorage.creditCards.getDuplicateGuid(record), null);
+  Assert.equal(await profileStorage.creditCards.getDuplicateGuid(record), guid);
 
-  // Superset shouldn't be treated as a duplicate.
+  // Superset with the same number is a duplicate.
   record = Object.assign({}, TEST_CREDIT_CARD_3);
   record["cc-name"] = "John Doe";
-  Assert.equal(await profileStorage.creditCards.getDuplicateGuid(record), null);
+  Assert.equal(await profileStorage.creditCards.getDuplicateGuid(record), guid);
 
   // Numbers with the same last 4 digits shouldn't be treated as a duplicate.
   record = Object.assign({}, TEST_CREDIT_CARD_3);
@@ -802,13 +802,44 @@ add_task(async function test_getDuplicateGuid() {
   // 09 and 90 adjacent digits, which is still a valid credit card number.
   record["cc-number"] = "358999378390" + last4Digits;
 
-  // We treat numbers with the same last 4 digits as a duplicate.
+  // We don't treat numbers with the same last 4 digits as a duplicate.
+  Assert.equal(await profileStorage.creditCards.getDuplicateGuid(record), null);
+});
+
+add_task(async function test_getDuplicateGuidMatch() {
+  let profileStorage = await initProfileStorage(
+    TEST_STORE_FILE_NAME,
+    [TEST_CREDIT_CARD_2],
+    "creditCards"
+  );
+  let guid = profileStorage.creditCards._data[0].guid;
+
+  // Absolutely a duplicate.
+  Assert.equal(
+    await profileStorage.creditCards.getDuplicateGuid(TEST_CREDIT_CARD_2),
+    guid
+  );
+
+  // Absolutely not a duplicate.
+  Assert.equal(
+    await profileStorage.creditCards.getDuplicateGuid(TEST_CREDIT_CARD_1),
+    null
+  );
+
+  // Numbers with the same last 4 digits shouldn't be treated as a duplicate.
+  record = Object.assign({}, TEST_CREDIT_CARD_2);
+
+  // We change month from `1` to `2`
+  record["cc-exp-month"] = 2;
   Assert.equal(await profileStorage.creditCards.getDuplicateGuid(record), guid);
 
-  // Even though the last 4 digits are the same, an invalid credit card number
-  // should never be treated as a duplicate.
-  record["cc-number"] = "************" + last4Digits;
-  Assert.equal(await profileStorage.creditCards.getDuplicateGuid(record), null);
+  // We change year from `2000` to `2001`
+  record["cc-exp-year"] = 2001;
+  Assert.equal(await profileStorage.creditCards.getDuplicateGuid(record), guid);
+
+  // New name, same card
+  record["cc-name"] = "John Doe";
+  Assert.equal(await profileStorage.creditCards.getDuplicateGuid(record), guid);
 });
 
 add_task(async function test_creditCardFillDisabled() {