Implement batch deletion in PasswordStoreMac.

Add a new keychain helper search and refactor a bit to support the deletion.

BUG=15070
TEST=Deleting passwords in Clear Browsing Data... will now work per the design doc (which means we clear passwords we created, and our metadata, but leave passwords created by other browsers alone).

Review URL: http://codereview.chromium.org/149759

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@20900 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 132 insertions, 29 deletions

diff --git a/chrome/browser/password_manager/password_store_mac.cc b/chrome/browser/password_manager/password_store_mac.cc
index 97f9aa0..d5327be 100644
--- a/chrome/browser/password_manager/password_store_mac.cc
+++ b/chrome/browser/password_manager/password_store_mac.cc
@@ -472,13 +472,7 @@ std::vector<PasswordForm*>
       MatchingKeychainItems(query_form.signon_realm, query_form.scheme,
                             NULL, NULL);
 
-  std::vector<PasswordForm*> keychain_forms =
-      CreateFormsFromKeychainItems(keychain_items);
-  for (std::vector<SecKeychainItemRef>::iterator i = keychain_items.begin();
-       i != keychain_items.end(); ++i) {
-    keychain_->Free(*i);
-  }
-  return keychain_forms;
+  return ConvertKeychainItemsToForms(&keychain_items);
 }
 
 std::vector<PasswordForm*>
@@ -489,13 +483,7 @@ std::vector<PasswordForm*>
       MatchingKeychainItems(query_form.signon_realm, query_form.scheme,
                             NULL, username.c_str());
 
-  std::vector<PasswordForm*> keychain_forms =
-      CreateFormsFromKeychainItems(keychain_items);
-  for (std::vector<SecKeychainItemRef>::iterator i = keychain_items.begin();
-       i != keychain_items.end(); ++i) {
-    keychain_->Free(*i);
-  }
-  return keychain_forms;
+  return ConvertKeychainItemsToForms(&keychain_items);
 }
 
 PasswordForm* MacKeychainPasswordFormAdapter::PasswordExactlyMatchingForm(
@@ -512,6 +500,26 @@ PasswordForm* MacKeychainPasswordFormAdapter::PasswordExactlyMatchingForm(
   return NULL;
 }
 
+std::vector<PasswordForm*>
+    MacKeychainPasswordFormAdapter::GetAllPasswordFormPasswords() {
+  SecAuthenticationType supported_auth_types[] = {
+    kSecAuthenticationTypeHTMLForm,
+    kSecAuthenticationTypeHTTPBasic,
+    kSecAuthenticationTypeHTTPDigest,
+  };
+  OSType creator = finds_only_owned_ ? kChromeKeychainCreatorCode : 0;
+
+  std::vector<SecKeychainItemRef> matches;
+  for (unsigned int i = 0; i < arraysize(supported_auth_types); ++i) {
+    KeychainSearch keychain_search(*keychain_);
+    keychain_search.Init(NULL, 0, kSecProtocolTypeAny, supported_auth_types[i],
+                         NULL, NULL, NULL, creator);
+    keychain_search.FindMatchingItems(&matches);
+  }
+
+  return ConvertKeychainItemsToForms(&matches);
+}
+
 bool MacKeychainPasswordFormAdapter::AddPassword(const PasswordForm& form) {
   // We should never be trying to store a blacklist in the keychain.
   DCHECK(!form.blacklisted_by_user);
@@ -570,17 +578,19 @@ void MacKeychainPasswordFormAdapter::SetFindsOnlyOwnedItems(
 }
 
 std::vector<PasswordForm*>
-    MacKeychainPasswordFormAdapter::CreateFormsFromKeychainItems(
-        const std::vector<SecKeychainItemRef>& items) {
+    MacKeychainPasswordFormAdapter::ConvertKeychainItemsToForms(
+        std::vector<SecKeychainItemRef>* items) {
   std::vector<PasswordForm*> keychain_forms;
-  for (std::vector<SecKeychainItemRef>::const_iterator i = items.begin();
-       i != items.end(); ++i) {
+  for (std::vector<SecKeychainItemRef>::const_iterator i = items->begin();
+       i != items->end(); ++i) {
     PasswordForm* form = new PasswordForm();
     if (internal_keychain_helpers::FillPasswordFormFromKeychainItem(*keychain_,
                                                                     *i, form)) {
       keychain_forms.push_back(form);
     }
+    keychain_->Free(*i);
   }
+  items->clear();
   return keychain_forms;
 }
 
@@ -750,7 +760,18 @@ void PasswordStoreMac::RemoveLoginImpl(const PasswordForm& form) {
 
 void PasswordStoreMac::RemoveLoginsCreatedBetweenImpl(
     const base::Time& delete_begin, const base::Time& delete_end) {
-  NOTIMPLEMENTED();
+  login_metadata_db_->RemoveLoginsCreatedBetween(delete_begin, delete_end);
+
+  // We can't delete from the Keychain by date because we may be sharing items
+  // with database entries that weren't in the delete range. Instead, we find
+  // all the Keychain items we own but aren't using any more and delete those.
+  std::vector<PasswordForm*> orphan_keychain_forms = GetUnusedKeychainForms();
+  // This is inefficient, since we have to re-look-up each keychain item one at
+  // a time to delete it even though the search step already had a list of
+  // Keychain item references. If this turns out to be noticeably slow we'll
+  // need to rearchitect to allow the search and deletion steps to share.
+  RemoveKeychainForms(orphan_keychain_forms);
+  STLDeleteElements(&orphan_keychain_forms);
 }
 
 void PasswordStoreMac::GetLoginsImpl(GetLoginsRequest* request,
@@ -838,6 +859,27 @@ bool PasswordStoreMac::DatabaseHasFormMatchingKeychainForm(
   return has_match;
 }
 
+std::vector<PasswordForm*> PasswordStoreMac::GetUnusedKeychainForms() {
+  std::vector<PasswordForm*> database_forms;
+  login_metadata_db_->GetAllLogins(&database_forms, false);
+  
+  MacKeychainPasswordFormAdapter owned_keychain_adapter(keychain_.get());
+  owned_keychain_adapter.SetFindsOnlyOwnedItems(true);
+  std::vector<PasswordForm*> owned_keychain_forms =
+      owned_keychain_adapter.GetAllPasswordFormPasswords();
+  
+  // Run a merge; anything left in owned_keychain_forms when we are done no
+  // longer has a matching database entry.
+  std::vector<PasswordForm*> merged_forms;
+  internal_keychain_helpers::MergePasswordForms(&owned_keychain_forms,
+                                                &database_forms,
+                                                &merged_forms);
+  STLDeleteElements(&merged_forms);
+  STLDeleteElements(&database_forms);
+
+  return owned_keychain_forms;
+}
+
 void PasswordStoreMac::RemoveDatabaseForms(
     const std::vector<PasswordForm*>& forms) {
   for (std::vector<PasswordForm*>::const_iterator i = forms.begin();
@@ -845,3 +887,13 @@ void PasswordStoreMac::RemoveDatabaseForms(
     login_metadata_db_->RemoveLogin(**i);
   }
 }
+
+void PasswordStoreMac::RemoveKeychainForms(
+    const std::vector<PasswordForm*>& forms) {
+  MacKeychainPasswordFormAdapter owned_keychain_adapter(keychain_.get());
+  owned_keychain_adapter.SetFindsOnlyOwnedItems(true);
+  for (std::vector<PasswordForm*>::const_iterator i = forms.begin();
+       i != forms.end(); ++i) {
+    owned_keychain_adapter.RemovePassword(**i);
+  }
+}
diff --git a/chrome/browser/password_manager/password_store_mac.h b/chrome/browser/password_manager/password_store_mac.h
index 1ea24e9..7874c24 100644
--- a/chrome/browser/password_manager/password_store_mac.h
+++ b/chrome/browser/password_manager/password_store_mac.h
@@ -13,6 +13,11 @@
 class LoginDatabaseMac;
 class MacKeychain;
 
+// Implements PasswordStore on top of the OS X Keychain, with an internal
+// database for extra metadata. For an overview of the interactions with the
+// Keychain, as well as the rationale for some of the behaviors, see the
+// Keychain integration design doc:
+// http://dev.chromium.org/developers/design-documents/os-x-password-manager-keychain-integration
 class PasswordStoreMac : public PasswordStore {
  public:
   // Takes ownership of |keychain| and |login_db|, both of which must be
@@ -41,10 +46,19 @@ class PasswordStoreMac : public PasswordStore {
   bool DatabaseHasFormMatchingKeychainForm(
       const webkit_glue::PasswordForm& form);
 
+  // Returns all the Keychain entries that we own but no longer have
+  // corresponding metadata for in our database.
+  // Caller is responsible for deleting the forms.
+  std::vector<webkit_glue::PasswordForm*> GetUnusedKeychainForms();
+
   // Removes the given forms from the database.
   void RemoveDatabaseForms(
       const std::vector<webkit_glue::PasswordForm*>& forms);
 
+  // Removes the given forms from the Keychain.
+  void RemoveKeychainForms(
+      const std::vector<webkit_glue::PasswordForm*>& forms);
+
   scoped_ptr<MacKeychain> keychain_;
   scoped_ptr<LoginDatabaseMac> login_metadata_db_;
 
diff --git a/chrome/browser/password_manager/password_store_mac_internal.h b/chrome/browser/password_manager/password_store_mac_internal.h
index ba6e07b..59dd2f2 100644
--- a/chrome/browser/password_manager/password_store_mac_internal.h
+++ b/chrome/browser/password_manager/password_store_mac_internal.h
@@ -40,6 +40,9 @@ class MacKeychainPasswordFormAdapter {
   webkit_glue::PasswordForm* PasswordExactlyMatchingForm(
       const webkit_glue::PasswordForm& query_form);
 
+  // Returns all keychain items of types corresponding to password forms.
+  std::vector<webkit_glue::PasswordForm*> GetAllPasswordFormPasswords();
+
   // Creates a new keychain entry from |form|, or updates the password of an
   // existing keychain entry if there is a collision. Returns true if a keychain
   // entry was successfully added/updated.
@@ -54,10 +57,11 @@ class MacKeychainPasswordFormAdapter {
   void SetFindsOnlyOwnedItems(bool finds_only_owned);
 
  private:
-  // Returns PasswordForms constructed from the given Keychain items.
+  // Returns PasswordForms constructed from the given Keychain items, calling
+  // MacKeychain::Free on all of the keychain items and clearing the vector.
   // Caller is responsible for deleting the returned forms.
-  std::vector<webkit_glue::PasswordForm*> CreateFormsFromKeychainItems(
-      const std::vector<SecKeychainItemRef>& items);
+  std::vector<webkit_glue::PasswordForm*> ConvertKeychainItemsToForms(
+      std::vector<SecKeychainItemRef>* items);
 
   // Searches |keychain| for the specific keychain entry that corresponds to the
   // given form, and returns it (or NULL if no match is found). The caller is
diff --git a/chrome/browser/password_manager/password_store_mac_unittest.cc b/chrome/browser/password_manager/password_store_mac_unittest.cc
index 41d5d1e..c96a1a4 100644
--- a/chrome/browser/password_manager/password_store_mac_unittest.cc
+++ b/chrome/browser/password_manager/password_store_mac_unittest.cc
@@ -33,7 +33,7 @@ class PasswordStoreMacTest : public testing::Test {
         kSecProtocolTypeHTTP, NULL, 0, NULL, "20000101000000Z",
         "", "", true },
       // De-facto negative item, type one.
-      {kSecAuthenticationTypeHTMLForm, "dont.remember.com",
+      { kSecAuthenticationTypeHTMLForm, "dont.remember.com",
         kSecProtocolTypeHTTP, NULL, 0, NULL, "20000101000000Z",
         "Password Not Stored", "", false },
       // De-facto negative item, type two.
@@ -55,8 +55,8 @@ class PasswordStoreMacTest : public testing::Test {
         "abc", "123", false },
     };
 
-    // Save one slot for use by AddInternetPassword.
-    unsigned int capacity = arraysize(test_data) + 1;
+    // Save some extra slots for use by AddInternetPassword.
+    unsigned int capacity = arraysize(test_data) + 3;
     keychain_ = new MockKeychain(capacity);
 
     for (unsigned int i = 0; i < arraysize(test_data); ++i) {
@@ -485,7 +485,8 @@ TEST_F(PasswordStoreMacTest, TestKeychainAdd) {
   owned_keychain_adapter.SetFindsOnlyOwnedItems(true);
 
   for (unsigned int i = 0; i < ARRAYSIZE_UNSAFE(test_data); ++i) {
-    PasswordForm* in_form = CreatePasswordFormFromData(test_data[i].data);
+    scoped_ptr<PasswordForm> in_form(
+        CreatePasswordFormFromData(test_data[i].data));
     bool add_succeeded = owned_keychain_adapter.AddPassword(*in_form);
     EXPECT_EQ(test_data[i].should_succeed, add_succeeded);
     if (add_succeeded) {
@@ -498,7 +499,6 @@ TEST_F(PasswordStoreMacTest, TestKeychainAdd) {
       EXPECT_EQ(out_form->username_value, in_form->username_value);
       EXPECT_EQ(out_form->password_value, in_form->password_value);
     }
-    delete in_form;
   }
 
   // Test that adding duplicate item updates the existing item.
@@ -508,7 +508,7 @@ TEST_F(PasswordStoreMacTest, TestKeychainAdd) {
       "http://some.domain.com/insecure.html", NULL,
       NULL, NULL, NULL, L"joe_user", L"updated_password", false, false, 0
     };
-    PasswordForm* update_form = CreatePasswordFormFromData(data);
+    scoped_ptr<PasswordForm> update_form(CreatePasswordFormFromData(data));
     MacKeychainPasswordFormAdapter keychain_adapter(keychain_);
     EXPECT_TRUE(keychain_adapter.AddPassword(*update_form));
     SecKeychainItemRef keychain_item = reinterpret_cast<SecKeychainItemRef>(2);
@@ -517,7 +517,6 @@ TEST_F(PasswordStoreMacTest, TestKeychainAdd) {
                                                                 keychain_item,
                                                                 &stored_form);
     EXPECT_EQ(update_form->password_value, stored_form.password_value);
-    delete update_form;
   }
 }
 
@@ -819,3 +818,37 @@ TEST_F(PasswordStoreMacTest, TestPasswordBulkLookup) {
   STLDeleteElements(&database_forms);
   STLDeleteElements(&merged_forms);
 }
+
+TEST_F(PasswordStoreMacTest, TestPasswordGetAll) {
+  MacKeychainPasswordFormAdapter keychain_adapter(keychain_);
+  MacKeychainPasswordFormAdapter owned_keychain_adapter(keychain_);
+  owned_keychain_adapter.SetFindsOnlyOwnedItems(true);
+
+  // Add a few passwords of various types so that we own some.
+  PasswordFormData owned_password_data[] = {
+    { PasswordForm::SCHEME_HTML, "http://web.site.com/",
+      "http://web.site.com/path/to/page.html", NULL, NULL, NULL, NULL,
+      L"anonymous", L"knock-knock", false, false, 0 },
+    { PasswordForm::SCHEME_BASIC, "http://a.site.com:2222/therealm",
+      "http://a.site.com:2222/", NULL, NULL, NULL, NULL,
+      L"username", L"password", false, false, 0 },
+    { PasswordForm::SCHEME_DIGEST, "https://digest.site.com/differentrealm",
+      "https://digest.site.com/secure.html", NULL, NULL, NULL, NULL,
+      L"testname", L"testpass", false, false, 0 },
+  };
+  for (unsigned int i = 0; i < arraysize(owned_password_data); ++i) {
+    scoped_ptr<PasswordForm> form(CreatePasswordFormFromData(
+        owned_password_data[i]));
+    owned_keychain_adapter.AddPassword(*form);
+  }
+
+  std::vector<PasswordForm*> all_passwords =
+      keychain_adapter.GetAllPasswordFormPasswords();
+  EXPECT_EQ(8 + arraysize(owned_password_data), all_passwords.size());
+  STLDeleteElements(&all_passwords);
+
+  std::vector<PasswordForm*> owned_passwords =
+      owned_keychain_adapter.GetAllPasswordFormPasswords();
+  EXPECT_EQ(arraysize(owned_password_data), owned_passwords.size());
+  STLDeleteElements(&owned_passwords);
+}