Delete unmasked credit cards when clearing data.

components/autofill/core/browser/webdata/autofill_table.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/autofill/core/browser/webdata/autofill_table.h"
 
 #include <algorithm>
 #include <cmath>
 #include <limits>
 #include <map>
@@ skipping 454 matching lines @@
       return MigrateToVersion60AddServerCards();
     case 61:
       *update_compatible_version = false;
       return MigrateToVersion61AddUsageStats();
     case 62:
       *update_compatible_version = false;
       return MigrateToVersion62AddUsageStatsForUnmaskedCards();
     case 63:
       *update_compatible_version = false;
       return MigrateToVersion63AddServerRecipientName();
+    case 64:
+      *update_compatible_version = false;
+      return MigrateToVersion64AddUnmaskDate();
   }
   return true;
 }
 
 bool AutofillTable::AddFormFieldValues(
     const std::vector<FormFieldData>& elements,
     std::vector<AutofillChange>* changes) {
   return AddFormFieldValuesTime(elements, changes, Time::Now());
 }
 
@@ skipping 700 matching lines @@
     card->SetServerStatus(ServerStatusStringToEnum(s.ColumnString(index++)));
     card->SetRawInfo(CREDIT_CARD_NAME, s.ColumnString16(index++));
     card->SetRawInfo(CREDIT_CARD_EXP_MONTH, s.ColumnString16(index++));
     card->SetRawInfo(CREDIT_CARD_EXP_4_DIGIT_YEAR, s.ColumnString16(index++));
     credit_cards->push_back(card);
   }
 
   return s.Succeeded();
 }
 
 void AutofillTable::SetServerCreditCards(

[Evan Stade, 2015/03/04 23:12:25]

Why does this function support an input vector of both masked and unmasked
cards? Isn't it only used with masked cards? Unmasking should always happen via
UnmaskServerCreditCard, right?

[brettw, 2015/03/04 23:21:45]

It was convenient for testing.

[Evan Stade, 2015/03/05 01:54:24]

I don't think it makes tests that much easier to write, but if you find it does
then I'd rather there be a test util function that calls SetServerCreditCards
then UnmaskServerCreditCard as appropriate, rather than putting test-only code
in this file. Then we don't have to maintain as much code and we get better test
coverage of real code.

[brettw, 2015/03/05 20:52:19]

I spent a while changing this but it's not worthwhile. Almost all of the server
card tests, including the ones you wrote, depend on this behavior. It also means
that you can't make a vector of cards, set them to the DB, and compare results
you get out the other end since you have to maintain both a masked card for
adding, and an unmasked card for doing the comparison out the other end. This
definitely makes the tests harder.

     const std::vector<CreditCard>& credit_cards) {
   // Delete all old values.
   sql::Statement masked_delete(db_->GetUniqueStatement(
       "DELETE FROM masked_credit_cards"));
   masked_delete.Run();
 
   // Delete all items in the unmasked table that aren't in the new set.
   sql::Statement get_unmasked(db_->GetUniqueStatement(
       "SELECT id FROM unmasked_credit_cards"));
+  std::set<std::string> kept_unmasked_entries;
   while (get_unmasked.Step()) {
     // We expect relatively few cards, just do brute-force.
     std::string server_id = get_unmasked.ColumnString(0);
     bool found_card = false;
     for (const CreditCard& cur_card : credit_cards) {
       if (cur_card.server_id() == server_id) {
         found_card = true;
         break;
       }
     }
-    if (!found_card) {
+    if (found_card) {
+      kept_unmasked_entries.insert(server_id);
+    } else {
       // This unmasked card in the DB isn't present in the input. The statement
       // is compiled every time because it's much more likely that this is never
       // executed than it runs more than once.
       sql::Statement unmasked_delete(db_->GetUniqueStatement(
           "DELETE FROM unmasked_credit_cards WHERE id = ?"));
       unmasked_delete.BindString(0, server_id);
       unmasked_delete.Run();
       DCHECK_EQ(1, db_->GetLastChangeCount());
     }
   }
 
   sql::Statement masked_insert(db_->GetUniqueStatement(

[Evan Stade, 2015/03/04 23:12:25]

there are multiple inserts in this function --- wrap in a transaction?

[brettw, 2015/03/04 23:21:45]

Sure. I added one. I think this will actually be in a bigger outer transaction
anyway. The WebDB transaction model is pretty messed up so it's "hard to say"
and I have a post-it on my desk to fix it.

       "INSERT INTO masked_credit_cards("
       "id,"            // 0
       "type,"          // 1
       "status,"        // 2
       "name_on_card,"  // 3
       "last_four,"     // 4
       "exp_month,"     // 4
       "exp_year) "     // 5
       "VALUES (?,?,?,?,?,?,?)"));
   sql::Statement unmasked_insert(db_->GetUniqueStatement(
       "INSERT INTO unmasked_credit_cards("
-      "id,"                     // 0
-      "card_number_encrypted)"  // 1
-      "VALUES (?,?)"));
+      "id,"                      // 0
+      "card_number_encrypted, "  // 1
+      "unmask_date)"             // 2
+      "VALUES (?,?,?)"));
   for (const CreditCard& card : credit_cards) {
     DCHECK(card.record_type() != CreditCard::LOCAL_CARD);
 
     masked_insert.BindString(0, card.server_id());
     masked_insert.BindString(1, card.type());
     masked_insert.BindString(2,
                              ServerStatusEnumToString(card.GetServerStatus()));
     masked_insert.BindString16(3, card.GetRawInfo(CREDIT_CARD_NAME));
     masked_insert.BindString16(4, card.LastFourDigits());
     masked_insert.BindString16(5, card.GetRawInfo(CREDIT_CARD_EXP_MONTH));
     masked_insert.BindString16(6,
                                card.GetRawInfo(CREDIT_CARD_EXP_4_DIGIT_YEAR));
 
     masked_insert.Run();
     masked_insert.Reset(true);
 
-    if (card.record_type() == CreditCard::FULL_SERVER_CARD) {
-      // Unmasked cards also get an entry in the unmasked table. Note that the
-      // input card could be MASKED but if we have an UNMASKED entry for that
-      // card already, it will be preserved.
+    if (kept_unmasked_entries.find(card.server_id()) ==
+            kept_unmasked_entries.end() &&
+        card.record_type() == CreditCard::FULL_SERVER_CARD) {
+      // New unmasked cards also get an entry in the unmasked table. In
+      // practice this will only happen for tests since the server cards will
+      // all be marked masked (if a server card is manually unmasked, we will
+      // have kept the existing entry in the table and won't get here).
       unmasked_insert.BindString(0, card.server_id());
       BindEncryptedCardToColumn(&unmasked_insert, 1,
                                 card.GetRawInfo(CREDIT_CARD_NUMBER));
+      // Unmask time for this card is now.
+      unmasked_insert.BindInt64(2, Time::Now().ToInternalValue());
       unmasked_insert.Run();
       unmasked_insert.Reset(true);
     }
   }
 }
 
 bool AutofillTable::UnmaskServerCreditCard(const std::string& id,
                                            const base::string16& full_number) {
   // Make sure there aren't duplicates for this card.
   MaskServerCreditCard(id);
   sql::Statement s(db_->GetUniqueStatement(
-      "INSERT INTO unmasked_credit_cards(id, card_number_encrypted,"
-      "            use_count, use_date) "
-      "VALUES (?,?,?,?)"));
+      "INSERT INTO unmasked_credit_cards("
+          "id,"
+          "card_number_encrypted,"
+          "use_count,"
+          "use_date,"
+          "unmask_date)"
+      "VALUES (?,?,?,?,?)"));
   s.BindString(0, id);
 
   std::string encrypted_data;
   OSCrypt::EncryptString16(full_number, &encrypted_data);
   s.BindBlob(1, encrypted_data.data(),
              static_cast<int>(encrypted_data.length()));
 
   // Unmasking counts as a usage, so set the stats accordingly.
-  s.BindInt64(2, 1);
-  s.BindInt64(3, base::Time::Now().ToInternalValue());
+  base::Time now = base::Time::Now();
+  s.BindInt64(2, 1);                      // use_count
+  s.BindInt64(3, now.ToInternalValue());  // use_date
+
+  s.BindInt64(4, now.ToInternalValue());  // unmask_date
 
   s.Run();
   return db_->GetLastChangeCount() > 0;
 }
 
 bool AutofillTable::MaskServerCreditCard(const std::string& id) {
   sql::Statement s(db_->GetUniqueStatement(
       "DELETE FROM unmasked_credit_cards WHERE id = ?"));
   s.BindString(0, id);
   s.Run();
@@ skipping 21 matching lines @@
   CreditCard* tmp_credit_card = NULL;
   if (!GetCreditCard(credit_card.guid(), &tmp_credit_card))
     return false;
 
   scoped_ptr<CreditCard> old_credit_card(tmp_credit_card);
   bool update_modification_date = *old_credit_card != credit_card;
 
   sql::Statement s(db_->GetUniqueStatement(
       "UPDATE credit_cards "
       "SET guid=?, name_on_card=?, expiration_month=?,"
-      "    expiration_year=?, card_number_encrypted=?, use_count=?, use_date=?,"
-      "    date_modified=?, origin=?"
+          "expiration_year=?, card_number_encrypted=?, use_count=?, use_date=?,"
+          "date_modified=?, origin=?"
       "WHERE guid=?"));
   BindCreditCardToStatement(
       credit_card,
       update_modification_date ? base::Time::Now() :
                                  old_credit_card->modification_date(),
       &s);
   s.BindString(9, credit_card.guid());
 
   bool result = s.Run();
   DCHECK_GT(db_->GetLastChangeCount(), 0);
@@ skipping 58 matching lines @@
   }
   if (!s_credit_cards_get.Succeeded())
     return false;
 
   // Remove Autofill credit cards in the time range.
   sql::Statement s_credit_cards(db_->GetUniqueStatement(
       "DELETE FROM credit_cards "
       "WHERE date_modified >= ? AND date_modified < ?"));
   s_credit_cards.BindInt64(0, delete_begin_t);
   s_credit_cards.BindInt64(1, delete_end_t);
+  if (!s_credit_cards.Run())
+    return false;
 
-  return s_credit_cards.Run();
+  // Remove unmasked credit cards in the time range.
+  sql::Statement s_unmasked_cards(db_->GetUniqueStatement(
+      "DELETE FROM unmasked_credit_cards "
+      "WHERE unmask_date >= ? AND unmask_date < ?"));
+  s_unmasked_cards.BindInt64(0, delete_begin.ToInternalValue());
+  s_unmasked_cards.BindInt64(1, delete_end.ToInternalValue());
+  if (!s_unmasked_cards.Run())
+    return false;
+
+  return true;
 }
 
 bool AutofillTable::RemoveOriginURLsModifiedBetween(
     const Time& delete_begin,
     const Time& delete_end,
     ScopedVector<AutofillProfile>* profiles) {
   DCHECK(delete_end.is_null() || delete_begin < delete_end);
 
   time_t delete_begin_t = delete_begin.ToTimeT();
   time_t delete_end_t = GetEndTime(delete_end);
@@ skipping 243 matching lines @@
   }
   return true;
 }
 
 bool AutofillTable::InitUnmaskedCreditCardsTable() {
   if (!db_->DoesTableExist("unmasked_credit_cards")) {
     if (!db_->Execute("CREATE TABLE unmasked_credit_cards ("
                       "id VARCHAR,"
                       "card_number_encrypted VARCHAR, "
                       "use_count INTEGER NOT NULL DEFAULT 0, "
-                      "use_date INTEGER NOT NULL DEFAULT 0)")) {
+                      "use_date INTEGER NOT NULL DEFAULT 0, "
+                      "unmask_date INTEGER NOT NULL DEFAULT 0)")) {
       NOTREACHED();
       return false;
     }
   }
   return true;
 }
 
 bool AutofillTable::InitServerAddressesTable() {
   if (!db_->DoesTableExist("server_addresses")) {
     // The space after language_code is necessary to match what sqlite does
@@ skipping 296 matching lines @@
 
 bool AutofillTable::MigrateToVersion63AddServerRecipientName() {
   if (!db_->DoesColumnExist("server_addresses", "recipient_name") &&
       !db_->Execute("ALTER TABLE server_addresses ADD COLUMN "
                     "recipient_name VARCHAR")) {
     return false;
   }
   return true;
 }
 
+bool AutofillTable::MigrateToVersion64AddUnmaskDate() {
+  if (!db_->DoesColumnExist("unmasked_credit_cards", "unmask_date") &&
+      !db_->Execute("ALTER TABLE unmasked_credit_cards ADD COLUMN "
+                    "unmask_date INTEGER NOT NULL DEFAULT 0")) {
+    return false;
+  }
+  return true;
+}
+
 }  // namespace autofill