Don't ever attempt to upload credit cards whose numbers match.

Previously, we were using the logic derived from local card handling where if the name or expiration date was different we would offer to save a card even if we already had an autofill entry with that exact card number. But in the case of upload, this will never succeed.

BUG=535784

Review URL: https://codereview.chromium.org/1808613004

Cr-Commit-Position: refs/heads/master@{#381974}

4 files changed, 53 insertions, 4 deletions

diff --git a/components/autofill/core/browser/credit_card.cc b/components/autofill/core/browser/credit_card.cc
index 9ea2ee7..2d3129c 100644
--- a/components/autofill/core/browser/credit_card.cc
+++ b/components/autofill/core/browser/credit_card.cc
@@ -606,11 +606,17 @@ bool CreditCard::IsLocalDuplicateOfServerCard(const CreditCard& other) const {
   if (number_.empty())
     return true;
 
-  if (other.record_type() == FULL_SERVER_CARD)
-    return StripSeparators(number_) == StripSeparators(other.number_);
+  return HasSameNumberAs(other);
+}
 
+bool CreditCard::HasSameNumberAs(const CreditCard& other) const {
   // For masked cards, this is the best we can do to compare card numbers.
-  return TypeAndLastFourDigits() == other.TypeAndLastFourDigits();
+  if (record_type() == MASKED_SERVER_CARD ||
+      other.record_type() == MASKED_SERVER_CARD) {
+    return TypeAndLastFourDigits() == other.TypeAndLastFourDigits();
+  }
+
+  return StripSeparators(number_) == StripSeparators(other.number_);
 }
 
 bool CreditCard::operator==(const CreditCard& credit_card) const {
diff --git a/components/autofill/core/browser/credit_card.h b/components/autofill/core/browser/credit_card.h
index 73f138c..e15c79f 100644
--- a/components/autofill/core/browser/credit_card.h
+++ b/components/autofill/core/browser/credit_card.h
@@ -155,6 +155,10 @@ class CreditCard : public AutofillDataModel {
   // Determines if |this| is a local version of the server card |other|.
   bool IsLocalDuplicateOfServerCard(const CreditCard& other) const;
 
+  // Determines if |this| has the same number as |other|. If either is a masked
+  // server card, compares the last four digits only.
+  bool HasSameNumberAs(const CreditCard& other) const;
+
   // Equality operators compare GUIDs, origins, and the contents.
   // Usage metadata (use count, use date, modification date) are NOT compared.
   bool operator==(const CreditCard& credit_card) const;
diff --git a/components/autofill/core/browser/credit_card_unittest.cc b/components/autofill/core/browser/credit_card_unittest.cc
index 10fa35f..f24726e 100644
--- a/components/autofill/core/browser/credit_card_unittest.cc
+++ b/components/autofill/core/browser/credit_card_unittest.cc
@@ -246,6 +246,41 @@ TEST(CreditCardTest, IsLocalDuplicateOfServerCard) {
   }
 }
 
+TEST(CreditCardTest, HasSameNumberAs) {
+  CreditCard a(base::GenerateGUID(), std::string());
+  CreditCard b(base::GenerateGUID(), std::string());
+
+  // Empty cards have the same empty number.
+  EXPECT_TRUE(a.HasSameNumberAs(b));
+  EXPECT_TRUE(b.HasSameNumberAs(a));
+
+  // Same number.
+  a.set_record_type(CreditCard::LOCAL_CARD);
+  a.SetRawInfo(CREDIT_CARD_NUMBER, ASCIIToUTF16("4111111111111111"));
+  a.set_record_type(CreditCard::LOCAL_CARD);
+  b.SetRawInfo(CREDIT_CARD_NUMBER, ASCIIToUTF16("4111111111111111"));
+  EXPECT_TRUE(a.HasSameNumberAs(b));
+  EXPECT_TRUE(b.HasSameNumberAs(a));
+
+  // Local cards shouldn't match even if the last 4 are the same.
+  a.set_record_type(CreditCard::LOCAL_CARD);
+  a.SetRawInfo(CREDIT_CARD_NUMBER, ASCIIToUTF16("4111111111111111"));
+  a.set_record_type(CreditCard::LOCAL_CARD);
+  b.SetRawInfo(CREDIT_CARD_NUMBER, ASCIIToUTF16("4111222222221111"));
+  EXPECT_FALSE(a.HasSameNumberAs(b));
+  EXPECT_FALSE(b.HasSameNumberAs(a));
+
+  // Likewise if one is an unmasked server card.
+  a.set_record_type(CreditCard::FULL_SERVER_CARD);
+  EXPECT_FALSE(a.HasSameNumberAs(b));
+  EXPECT_FALSE(b.HasSameNumberAs(a));
+
+  // But if one is a masked card, then they should.
+  b.set_record_type(CreditCard::MASKED_SERVER_CARD);
+  EXPECT_TRUE(a.HasSameNumberAs(b));
+  EXPECT_TRUE(b.HasSameNumberAs(a));
+}
+
 TEST(CreditCardTest, Compare) {
   CreditCard a(base::GenerateGUID(), std::string());
   CreditCard b(base::GenerateGUID(), std::string());
diff --git a/components/autofill/core/browser/personal_data_manager.cc b/components/autofill/core/browser/personal_data_manager.cc
index c7ec6db..73d19e1 100644
--- a/components/autofill/core/browser/personal_data_manager.cc
+++ b/components/autofill/core/browser/personal_data_manager.cc
@@ -1425,8 +1425,12 @@ bool PersonalDataManager::ImportCreditCard(
   }
 
   // Also don't offer to save if we already have this stored as a server card.
+  // We only check the number because if the new card has the same number as the
+  // server card, upload is guaranteed to fail. There's no mechanism for entries
+  // with the same number but different names or expiration dates as there is
+  // for local cards.
   for (const CreditCard* card : server_credit_cards_) {
-    if (candidate_credit_card.IsLocalDuplicateOfServerCard(*card))
+    if (candidate_credit_card.HasSameNumberAs(*card))
       return false;
   }