Implement Add and Update for PasswordStoreMac.

Modify LoginDatabase slightly to give PasswordStoreMac enough information to do the right thing.
Add creator code for keychain items we create, and unit tests to make sure.

BUG=11745
TEST=Visit a site for which you have a password in the Keychain. Type your username, unfocus the field, and then log in with the filled password. Log out, return to the login page, and the username and password should now autofill without user interaction.
Review URL: http://codereview.chromium.org/151176

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

6 files changed, 59 insertions, 11 deletions

diff --git a/chrome/browser/password_manager/login_database.cc b/chrome/browser/password_manager/login_database.cc
index b630867..965e9c3 100644
--- a/chrome/browser/password_manager/login_database.cc
+++ b/chrome/browser/password_manager/login_database.cc
@@ -159,7 +159,7 @@ bool LoginDatabase::AddLogin(const PasswordForm& form) {
   return true;
 }
 
-bool LoginDatabase::UpdateLogin(const PasswordForm& form) {
+bool LoginDatabase::UpdateLogin(const PasswordForm& form, int* items_changed) {
   SQLStatement s;
   if (s.prepare(db_, "UPDATE logins SET "
                 "action_url = ?, "
@@ -191,6 +191,9 @@ bool LoginDatabase::UpdateLogin(const PasswordForm& form) {
     NOTREACHED();
     return false;
   }
+  if (items_changed) {
+    *items_changed = s.changes();
+  }
   return true;
 }
 
diff --git a/chrome/browser/password_manager/login_database.h b/chrome/browser/password_manager/login_database.h
index 8b9a844..e2619ae 100644
--- a/chrome/browser/password_manager/login_database.h
+++ b/chrome/browser/password_manager/login_database.h
@@ -31,8 +31,9 @@ class LoginDatabase {
   // Adds |form| to the list of remembered password forms.
   bool AddLogin(const webkit_glue::PasswordForm& form);
 
-  // Updates remembered password form.
-  bool UpdateLogin(const webkit_glue::PasswordForm& form);
+  // Updates remembered password form. Returns true on success and sets
+  // items_changed (if non-NULL) to the number of logins updated.
+  bool UpdateLogin(const webkit_glue::PasswordForm& form, int* items_changed);
 
   // Removes |form| from the list of remembered password forms.
   bool RemoveLogin(const webkit_glue::PasswordForm& form);
diff --git a/chrome/browser/password_manager/login_database_unittest.cc b/chrome/browser/password_manager/login_database_unittest.cc
index 76dce57..8b4f6a9 100644
--- a/chrome/browser/password_manager/login_database_unittest.cc
+++ b/chrome/browser/password_manager/login_database_unittest.cc
@@ -159,7 +159,9 @@ TEST_F(LoginDatabaseTest, Logins) {
 
   // We update, and check to make sure it matches the
   // old form, and there is only one record.
-  EXPECT_TRUE(db->UpdateLogin(form6));
+  int rows_changed = 0;
+  EXPECT_TRUE(db->UpdateLogin(form6, &rows_changed));
+  EXPECT_EQ(1, rows_changed);
   // matches
   EXPECT_TRUE(db->GetLogins(form5, &result));
   EXPECT_EQ(1U, result.size());
diff --git a/chrome/browser/password_manager/password_store_mac.cc b/chrome/browser/password_manager/password_store_mac.cc
index fac6bda..56980fc 100644
--- a/chrome/browser/password_manager/password_store_mac.cc
+++ b/chrome/browser/password_manager/password_store_mac.cc
@@ -18,6 +18,8 @@
 
 using webkit_glue::PasswordForm;
 
+static const OSType kChromeKeychainCreatorCode = 'rimZ';
+
 // Utility class to handle the details of constructing and running a keychain
 // search from a set of attributes.
 class KeychainSearch {
@@ -539,16 +541,20 @@ bool MacKeychainPasswordFormAdapter::AddLogin(const PasswordForm& form) {
   std::string path = form.origin.path();
   SecProtocolType protocol = is_secure ? kSecProtocolTypeHTTPS
                                        : kSecProtocolTypeHTTP;
+  SecKeychainItemRef new_item = NULL;
   OSStatus result = keychain_->AddInternetPassword(
       NULL, server.size(), server.c_str(),
       security_domain.size(), security_domain.c_str(),
       username.size(), username.c_str(),
       path.size(), path.c_str(),
       port, protocol, internal_keychain_helpers::AuthTypeForScheme(form.scheme),
-      password.size(), password.c_str(), NULL);
+      password.size(), password.c_str(), &new_item);
 
-  // If we collide with an existing item, find and update it instead.
-  if (result == errSecDuplicateItem) {
+  if (result == noErr) {
+    SetKeychainItemCreatorCode(new_item, kChromeKeychainCreatorCode);
+    keychain_->Free(new_item);
+  } else if (result == errSecDuplicateItem) {
+    // If we collide with an existing item, find and update it instead.
     SecKeychainItemRef existing_item =
         internal_keychain_helpers::MatchingKeychainItem(*keychain_, form);
     if (!existing_item) {
@@ -559,7 +565,7 @@ bool MacKeychainPasswordFormAdapter::AddLogin(const PasswordForm& form) {
     return changed;
   }
 
-  return (result == noErr);
+  return result == noErr;
 }
 
 std::vector<PasswordForm*>
@@ -616,7 +622,17 @@ bool MacKeychainPasswordFormAdapter::SetKeychainItemPassword(
   OSStatus result = keychain_->ItemModifyAttributesAndData(keychain_item, NULL,
                                                            password.size(),
                                                            password.c_str());
-  return (result == noErr);
+  return result == noErr;
+}
+
+bool MacKeychainPasswordFormAdapter::SetKeychainItemCreatorCode(
+    const SecKeychainItemRef& keychain_item, OSType creator_code) {
+  SecKeychainAttribute attr = { kSecCreatorItemAttr, sizeof(creator_code),
+                                &creator_code };
+  SecKeychainAttributeList attrList = { 1, &attr };
+  OSStatus result = keychain_->ItemModifyAttributesAndData(keychain_item,
+                                                           &attrList, 0, NULL);
+  return result == noErr;
 }
 
 #pragma mark -
@@ -631,11 +647,25 @@ PasswordStoreMac::PasswordStoreMac(MacKeychain* keychain,
 PasswordStoreMac::~PasswordStoreMac() {}
 
 void PasswordStoreMac::AddLoginImpl(const PasswordForm& form) {
-  NOTIMPLEMENTED();
+  MacKeychainPasswordFormAdapter keychainAdapter(keychain_.get());
+  if (keychainAdapter.AddLogin(form)) {
+    login_metadata_db_->AddLogin(form);
+  }
 }
 
 void PasswordStoreMac::UpdateLoginImpl(const PasswordForm& form) {
-  NOTIMPLEMENTED();
+  MacKeychainPasswordFormAdapter keychainAdapter(keychain_.get());
+  // The keychain AddLogin will update if there is a collision and add if there
+  // isn't, which is the behavior we want, so there's no separate UpdateLogin.
+  if (keychainAdapter.AddLogin(form)) {
+    int update_count = 0;
+    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.
+    if (update_count == 0) {
+      login_metadata_db_->AddLogin(form);
+    }
+  }
 }
 
 void PasswordStoreMac::RemoveLoginImpl(const PasswordForm& form) {
diff --git a/chrome/browser/password_manager/password_store_mac_internal.h b/chrome/browser/password_manager/password_store_mac_internal.h
index 3d546e0..6b7712b 100644
--- a/chrome/browser/password_manager/password_store_mac_internal.h
+++ b/chrome/browser/password_manager/password_store_mac_internal.h
@@ -50,6 +50,11 @@ class MacKeychainPasswordFormAdapter {
   bool SetKeychainItemPassword(const SecKeychainItemRef& keychain_item,
                                const std::string& password);
 
+  // Sets the creator code of keychain_item to creator_code; returns true if the
+  // creator code was successfully set.
+  bool SetKeychainItemCreatorCode(const SecKeychainItemRef& keychain_item,
+                                  OSType creator_code);
+
   MacKeychain* keychain_;
 
   DISALLOW_COPY_AND_ASSIGN(MacKeychainPasswordFormAdapter);
diff --git a/chrome/browser/password_manager/password_store_mac_unittest.cc b/chrome/browser/password_manager/password_store_mac_unittest.cc
index 21cf79a..f26fffe 100644
--- a/chrome/browser/password_manager/password_store_mac_unittest.cc
+++ b/chrome/browser/password_manager/password_store_mac_unittest.cc
@@ -66,6 +66,7 @@ class PasswordStoreMacTest : public testing::Test {
 
   virtual void TearDown() {
     ExpectCreatesAndFreesBalanced();
+    ExpectCreatorCodesSet();
     delete keychain_;
   }
 
@@ -79,6 +80,12 @@ class PasswordStoreMacTest : public testing::Test {
     EXPECT_EQ(0, keychain_->UnfreedAttributeDataCount());
   }
 
+  // Causes a test failure unless any Keychain items added during the test have
+  // their creator code set.
+  void ExpectCreatorCodesSet() {
+    EXPECT_TRUE(keychain_->CreatorCodesSetForAddedItems());
+  }
+
   MockKeychain* keychain_;
 };