Don't re-store deleted passwords on form submit on the Mac

Adds test coverage for the various update cases to make sure they are all handled correctly.
Also fixes a regression during the recent PasswordStore refactoring that caused the Mac implementation to run queries on the wrong thread (found by the new unit tests).

BUG=35603
TEST=See bug; other password update cases should continue to work as before.

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

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

4 files changed, 224 insertions, 31 deletions

diff --git a/chrome/browser/password_manager/password_store.h b/chrome/browser/password_manager/password_store.h
index 90ce7f3..d315e5b 100644
--- a/chrome/browser/password_manager/password_store.h
+++ b/chrome/browser/password_manager/password_store.h
@@ -104,7 +104,7 @@ class PasswordStore : public base::RefCountedThreadSafe<PasswordStore> {
   };
 
   // Schedule the given task to be run in the PasswordStore's own thread.
-  void ScheduleTask(Task* task);
+  virtual void ScheduleTask(Task* task);
 
   // These will be run in PasswordStore's own thread.
   // Synchronous implementation that reports usage metrics.
diff --git a/chrome/browser/password_manager/password_store_mac.cc b/chrome/browser/password_manager/password_store_mac.cc
index 1804386..2b26c5a 100644
--- a/chrome/browser/password_manager/password_store_mac.cc
+++ b/chrome/browser/password_manager/password_store_mac.cc
@@ -504,6 +504,20 @@ PasswordForm* MacKeychainPasswordFormAdapter::PasswordExactlyMatchingForm(
   return NULL;
 }
 
+bool MacKeychainPasswordFormAdapter::HasPasswordsMergeableWithForm(
+    const PasswordForm& query_form) {
+  std::string username = UTF16ToUTF8(query_form.username_value);
+  std::vector<SecKeychainItemRef> matches =
+      MatchingKeychainItems(query_form.signon_realm, query_form.scheme,
+                            NULL, username.c_str());
+  for (std::vector<SecKeychainItemRef>::iterator i = matches.begin();
+       i != matches.end(); ++i) {
+    keychain_->Free(*i);
+  }
+
+  return matches.size() != 0;
+}
+
 std::vector<PasswordForm*>
     MacKeychainPasswordFormAdapter::GetAllPasswordFormPasswords() {
   SecAuthenticationType supported_auth_types[] = {
@@ -762,30 +776,36 @@ void PasswordStoreMac::AddLoginImpl(const PasswordForm& form) {
 }
 
 void PasswordStoreMac::UpdateLoginImpl(const PasswordForm& form) {
+  int update_count = 0;
+  if (!login_metadata_db_->UpdateLogin(form, &update_count))
+    return;
+
+  MacKeychainPasswordFormAdapter keychain_adapter(keychain_.get());
+  if (update_count == 0 &&
+      !keychain_adapter.HasPasswordsMergeableWithForm(form)) {
+    // If the password isn't in either the DB or the keychain, then it must have
+    // been deleted after autofill happened, and should not be re-added.
+    return;
+  }
+
   // The keychain add will update if there is a collision and add if there
   // isn't, which is the behavior we want, so there's no separate update call.
   if (AddToKeychainIfNecessary(form)) {
-    int update_count = 0;
-    if (login_metadata_db_->UpdateLogin(form, &update_count)) {
-      // Update will catch any database entries that we already had, but we
-      // could also be updating a keychain-only form, in which case we need to
-      // add.
-      PasswordStoreChangeList changes;
-      if (update_count == 0) {
-        if (login_metadata_db_->AddLogin(form)) {
-          changes.push_back(PasswordStoreChange(PasswordStoreChange::ADD,
-                                                form));
-        }
-      } else {
-        changes.push_back(PasswordStoreChange(PasswordStoreChange::UPDATE,
+    PasswordStoreChangeList changes;
+    if (update_count == 0) {
+      if (login_metadata_db_->AddLogin(form)) {
+        changes.push_back(PasswordStoreChange(PasswordStoreChange::ADD,
                                               form));
       }
-      if (!changes.empty()) {
-        NotificationService::current()->Notify(
-            NotificationType::LOGINS_CHANGED,
-            NotificationService::AllSources(),
-            Details<PasswordStoreChangeList>(&changes));
-      }
+    } else {
+      changes.push_back(PasswordStoreChange(PasswordStoreChange::UPDATE,
+                                            form));
+    }
+    if (!changes.empty()) {
+      NotificationService::current()->Notify(
+          NotificationType::LOGINS_CHANGED,
+          NotificationService::AllSources(),
+          Details<PasswordStoreChangeList>(&changes));
     }
   }
 }
diff --git a/chrome/browser/password_manager/password_store_mac_internal.h b/chrome/browser/password_manager/password_store_mac_internal.h
index 223c20e..43ae5ad 100644
--- a/chrome/browser/password_manager/password_store_mac_internal.h
+++ b/chrome/browser/password_manager/password_store_mac_internal.h
@@ -40,6 +40,13 @@ class MacKeychainPasswordFormAdapter {
   webkit_glue::PasswordForm* PasswordExactlyMatchingForm(
       const webkit_glue::PasswordForm& query_form);
 
+  // Returns true if PasswordsMergeableWithForm would return any items. This is
+  // a separate method because calling PasswordsMergeableWithForm and checking
+  // the return count would require reading the passwords from the keychain,
+  // thus potentially triggering authorizaiton UI, whereas this won't.
+  bool HasPasswordsMergeableWithForm(
+      const webkit_glue::PasswordForm& query_form);
+
   // Returns all keychain items of types corresponding to password forms.
   std::vector<webkit_glue::PasswordForm*> GetAllPasswordFormPasswords();
 
diff --git a/chrome/browser/password_manager/password_store_mac_unittest.cc b/chrome/browser/password_manager/password_store_mac_unittest.cc
index 7fefc31..3db112d 100644
--- a/chrome/browser/password_manager/password_store_mac_unittest.cc
+++ b/chrome/browser/password_manager/password_store_mac_unittest.cc
@@ -2,19 +2,49 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 #include "base/basictypes.h"
+#include "base/file_util.h"
+#include "base/path_service.h"
+#include "base/scoped_temp_dir.h"
 #include "base/stl_util-inl.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
+#include "chrome/browser/chrome_thread.h"
 #include "chrome/browser/keychain_mock_mac.h"
 #include "chrome/browser/password_manager/password_store_mac.h"
 #include "chrome/browser/password_manager/password_store_mac_internal.h"
+#include "chrome/common/chrome_paths.h"
 
 using webkit_glue::PasswordForm;
+using testing::_;
+using testing::DoAll;
+using testing::WithArg;
 
-class PasswordStoreMacTest : public testing::Test {
+namespace {
+
+class MockPasswordStoreConsumer : public PasswordStoreConsumer {
+public:
+  MOCK_METHOD2(OnPasswordStoreRequestDone,
+               void(int, const std::vector<webkit_glue::PasswordForm*>&));
+};
+
+ACTION(STLDeleteElements0) {
+  STLDeleteContainerPointers(arg0.begin(), arg0.end());
+}
+
+ACTION(QuitUIMessageLoop) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+  MessageLoop::current()->Quit();
+}
+
+}  // namespace
+
+#pragma mark -
+
+class PasswordStoreMacInternalsTest : public testing::Test {
  public:
   virtual void SetUp() {
     MockKeychain::KeychainTestData test_data[] = {
@@ -193,7 +223,7 @@ static void CheckFormsAgainstExpectations(
 
 #pragma mark -
 
-TEST_F(PasswordStoreMacTest, TestKeychainToFormTranslation) {
+TEST_F(PasswordStoreMacInternalsTest, TestKeychainToFormTranslation) {
   typedef struct {
     const PasswordForm::Scheme scheme;
     const char* signon_realm;
@@ -291,7 +321,7 @@ TEST_F(PasswordStoreMacTest, TestKeychainToFormTranslation) {
   }
 }
 
-TEST_F(PasswordStoreMacTest, TestKeychainSearch) {
+TEST_F(PasswordStoreMacInternalsTest, TestKeychainSearch) {
   struct TestDataAndExpectation {
     const PasswordFormData data;
     const size_t expected_fill_matches;
@@ -352,7 +382,9 @@ TEST_F(PasswordStoreMacTest, TestKeychainSearch) {
     EXPECT_EQ(test_data[i].expected_fill_matches, matching_items.size());
     STLDeleteElements(&matching_items);
 
-    // Check matches teating the form as a merging target.
+    // Check matches treating the form as a merging target.
+    EXPECT_EQ(test_data[i].expected_merge_matches > 0,
+              keychain_adapter.HasPasswordsMergeableWithForm(*query_form));
     matching_items = keychain_adapter.PasswordsMergeableWithForm(*query_form);
     EXPECT_EQ(test_data[i].expected_merge_matches, matching_items.size());
     STLDeleteElements(&matching_items);
@@ -391,7 +423,7 @@ static void SetPasswordFormRealm(PasswordForm* form, const char* realm) {
   form->signon_realm = signon_gurl.ReplaceComponents(replacement).spec();
 }
 
-TEST_F(PasswordStoreMacTest, TestKeychainExactSearch) {
+TEST_F(PasswordStoreMacInternalsTest, TestKeychainExactSearch) {
   MacKeychainPasswordFormAdapter keychain_adapter(keychain_);
 
   PasswordFormData base_form_data[] = {
@@ -410,6 +442,7 @@ TEST_F(PasswordStoreMacTest, TestKeychainExactSearch) {
     // Create a base form and make sure we find a match.
     scoped_ptr<PasswordForm> base_form(CreatePasswordFormFromData(
         base_form_data[i]));
+    EXPECT_TRUE(keychain_adapter.HasPasswordsMergeableWithForm(*base_form));
     PasswordForm* match =
         keychain_adapter.PasswordExactlyMatchingForm(*base_form);
     EXPECT_TRUE(match != NULL);
@@ -455,7 +488,7 @@ TEST_F(PasswordStoreMacTest, TestKeychainExactSearch) {
   }
 }
 
-TEST_F(PasswordStoreMacTest, TestKeychainAdd) {
+TEST_F(PasswordStoreMacInternalsTest, TestKeychainAdd) {
   struct TestDataAndExpectation {
     PasswordFormData data;
     bool should_succeed;
@@ -494,6 +527,8 @@ TEST_F(PasswordStoreMacTest, TestKeychainAdd) {
     bool add_succeeded = owned_keychain_adapter.AddPassword(*in_form);
     EXPECT_EQ(test_data[i].should_succeed, add_succeeded);
     if (add_succeeded) {
+      EXPECT_TRUE(owned_keychain_adapter.HasPasswordsMergeableWithForm(
+          *in_form));
       scoped_ptr<PasswordForm> out_form(
           owned_keychain_adapter.PasswordExactlyMatchingForm(*in_form));
       EXPECT_TRUE(out_form.get() != NULL);
@@ -524,7 +559,7 @@ TEST_F(PasswordStoreMacTest, TestKeychainAdd) {
   }
 }
 
-TEST_F(PasswordStoreMacTest, TestKeychainRemove) {
+TEST_F(PasswordStoreMacInternalsTest, TestKeychainRemove) {
   struct TestDataAndExpectation {
     PasswordFormData data;
     bool should_succeed;
@@ -563,7 +598,7 @@ TEST_F(PasswordStoreMacTest, TestKeychainRemove) {
   }
 }
 
-TEST_F(PasswordStoreMacTest, TestFormMatch) {
+TEST_F(PasswordStoreMacInternalsTest, TestFormMatch) {
   PasswordForm base_form;
   base_form.signon_realm = std::string("http://some.domain.com/");
   base_form.origin = GURL("http://some.domain.com/page.html");
@@ -625,7 +660,7 @@ TEST_F(PasswordStoreMacTest, TestFormMatch) {
   }
 }
 
-TEST_F(PasswordStoreMacTest, TestFormMerge) {
+TEST_F(PasswordStoreMacInternalsTest, TestFormMerge) {
   // Set up a bunch of test data to use in varying combinations.
   PasswordFormData keychain_user_1 =
       { PasswordForm::SCHEME_HTML, "http://some.domain.com/",
@@ -780,7 +815,7 @@ TEST_F(PasswordStoreMacTest, TestFormMerge) {
   }
 }
 
-TEST_F(PasswordStoreMacTest, TestPasswordBulkLookup) {
+TEST_F(PasswordStoreMacInternalsTest, TestPasswordBulkLookup) {
   PasswordFormData db_data[] = {
     { PasswordForm::SCHEME_HTML, "http://some.domain.com/",
       "http://some.domain.com/", "http://some.domain.com/action.cgi",
@@ -823,7 +858,7 @@ TEST_F(PasswordStoreMacTest, TestPasswordBulkLookup) {
   STLDeleteElements(&merged_forms);
 }
 
-TEST_F(PasswordStoreMacTest, TestPasswordGetAll) {
+TEST_F(PasswordStoreMacInternalsTest, TestPasswordGetAll) {
   MacKeychainPasswordFormAdapter keychain_adapter(keychain_);
   MacKeychainPasswordFormAdapter owned_keychain_adapter(keychain_);
   owned_keychain_adapter.SetFindsOnlyOwnedItems(true);
@@ -856,3 +891,134 @@ TEST_F(PasswordStoreMacTest, TestPasswordGetAll) {
   EXPECT_EQ(arraysize(owned_password_data), owned_passwords.size());
   STLDeleteElements(&owned_passwords);
 }
+
+#pragma mark -
+
+class PasswordStoreMacTest : public testing::Test {
+ public:
+  PasswordStoreMacTest() : ui_thread_(ChromeThread::UI, &message_loop_) {}
+
+  virtual void SetUp() {
+    login_db_ = new LoginDatabase();
+    ASSERT_TRUE(db_dir_.CreateUniqueTempDir());
+    FilePath db_file = db_dir_.path().AppendASCII("login.db");
+    ASSERT_TRUE(login_db_->Init(db_file));
+
+    keychain_ = new MockKeychain(3);
+
+    store_ = new PasswordStoreMac(keychain_, login_db_);
+    ASSERT_TRUE(store_->Init());
+  }
+
+  virtual void TearDown() {
+    MessageLoop::current()->PostTask(FROM_HERE, new MessageLoop::QuitTask);
+    MessageLoop::current()->Run();
+  }
+
+ protected:
+  MessageLoopForUI message_loop_;
+  ChromeThread ui_thread_;
+
+  MockKeychain* keychain_;  // Owned by store_.
+  LoginDatabase* login_db_;  // Owned by store_.
+  scoped_refptr<PasswordStoreMac> store_;
+  ScopedTempDir db_dir_;
+};
+
+TEST_F(PasswordStoreMacTest, TestStoreUpdate) {
+  // Insert a password into both the database and the keychain.
+  // This is done manually, rather than through store_->AddLogin, because the
+  // Mock Keychain isn't smart enough to be able to support update generically,
+  // so some.domain.com triggers special handling to test it that make inserting
+  // fail.
+  PasswordFormData joint_data = {
+    PasswordForm::SCHEME_HTML, "http://some.domain.com/",
+    "http://some.domain.com/insecure.html", "login.cgi",
+    L"username", L"password", L"submit", L"joe_user", L"sekrit", true, false, 1
+  };
+  scoped_ptr<PasswordForm> joint_form(CreatePasswordFormFromData(joint_data));
+  login_db_->AddLogin(*joint_form);
+  MockKeychain::KeychainTestData joint_keychain_data = {
+    kSecAuthenticationTypeHTMLForm, "some.domain.com",
+    kSecProtocolTypeHTTP, "/insecure.html", 0, NULL, "20020601171500Z",
+    "joe_user", "sekrit", false };
+  keychain_->AddTestItem(joint_keychain_data);
+
+  // Insert a password into the keychain only.
+  MockKeychain::KeychainTestData keychain_only_data = {
+    kSecAuthenticationTypeHTMLForm, "keychain.only.com",
+    kSecProtocolTypeHTTP, NULL, 0, NULL, "20020601171500Z",
+    "keychain", "only", false
+  };
+  keychain_->AddTestItem(keychain_only_data);
+
+  struct UpdateData {
+    PasswordFormData form_data;
+    const char* password;  // NULL indicates no entry should be present.
+  };
+
+  // Make a series of update calls.
+  UpdateData updates[] = {
+    // Update the keychain+db passwords (the normal password update case).
+    { { PasswordForm::SCHEME_HTML, "http://some.domain.com/",
+        "http://some.domain.com/insecure.html", "login.cgi",
+        L"username", L"password", L"submit", L"joe_user", L"53krit",
+        true, false, 2 },
+      "53krit",
+    },
+    // Update the keychain-only password; this simulates the initial use of a
+    // password stored by another browsers.
+    { { PasswordForm::SCHEME_HTML, "http://keychain.only.com/",
+        "http://keychain.only.com/login.html", "login.cgi",
+        L"username", L"password", L"submit", L"keychain", L"only",
+        true, false, 2 },
+      "only",
+    },
+    // Update a password that doesn't exist in either location. This tests the
+    // case where a form is filled, then the stored login is removed, then the
+    // form is submitted.
+    { { PasswordForm::SCHEME_HTML, "http://different.com/",
+        "http://different.com/index.html", "login.cgi",
+        L"username", L"password", L"submit", L"abc", L"123",
+        true, false, 2 },
+      NULL,
+    },
+  };
+  for (unsigned int i = 0; i < ARRAYSIZE_UNSAFE(updates); ++i) {
+    scoped_ptr<PasswordForm> form(CreatePasswordFormFromData(
+        updates[i].form_data));
+    store_->UpdateLogin(*form);
+  }
+
+  // Do a store-level query to wait for all the operations above to be done.
+  MockPasswordStoreConsumer consumer;
+  ON_CALL(consumer, OnPasswordStoreRequestDone(_, _)).WillByDefault(
+      QuitUIMessageLoop());
+  EXPECT_CALL(consumer, OnPasswordStoreRequestDone(_, _)).WillOnce(
+      DoAll(WithArg<1>(STLDeleteElements0()), QuitUIMessageLoop()));
+  store_->GetLogins(*joint_form, &consumer);
+  MessageLoop::current()->Run();
+
+  MacKeychainPasswordFormAdapter keychain_adapter(keychain_);
+  for (unsigned int i = 0; i < ARRAYSIZE_UNSAFE(updates); ++i) {
+    scoped_ptr<PasswordForm> query_form(
+        CreatePasswordFormFromData(updates[i].form_data));
+
+    std::vector<PasswordForm*> matching_items =
+        keychain_adapter.PasswordsFillingForm(*query_form);
+    if (updates[i].password) {
+      EXPECT_GT(matching_items.size(), 0U) << "iteration " << i;
+      if (matching_items.size() >= 1)
+        EXPECT_EQ(ASCIIToUTF16(updates[i].password),
+                  matching_items[0]->password_value) << "iteration " << i;
+    } else {
+      EXPECT_EQ(0U, matching_items.size()) << "iteration " << i;
+    }
+    STLDeleteElements(&matching_items);
+
+    login_db_->GetLogins(*query_form, &matching_items);
+    EXPECT_EQ(updates[i].password ? 1U : 0U, matching_items.size())
+        << "iteration " << i;
+    STLDeleteElements(&matching_items);
+  }
+}