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

chrome/browser/keychain_mock_mac.cc

@@ -17,7 +17,8 @@ MockKeychain::MockKeychain(unsigned int item_capacity)
                     kSecAuthenticationTypeItemAttr,
                     kSecSecurityDomainItemAttr,
                     kSecCreationDateItemAttr,
-                    kSecNegativeItemAttr };
+                    kSecNegativeItemAttr,
+                    kSecCreatorItemAttr };
 
   // Create the test keychain data storage.
   keychain_attr_list_ = static_cast<SecKeychainAttributeList*>(
@@ -44,6 +45,9 @@ MockKeychain::MockKeychain(unsigned int item_capacity)
         case kSecNegativeItemAttr:
           data_size = sizeof(Boolean);
           break;
+        case kSecCreatorItemAttr:
+          data_size = sizeof(OSType);
+          break;
       }
       if (data_size > 0) {
         keychain_attr_list_[i].attr[j].length = data_size;
@@ -69,30 +73,37 @@ MockKeychain::~MockKeychain() {
   free(keychain_data_);
 }
 
-int MockKeychain::IndexForTag(const SecKeychainAttributeList& attribute_list,
-                              UInt32 tag) {
+
+SecKeychainAttribute* MockKeychain::AttributeWithTag(
+    const SecKeychainAttributeList& attribute_list, UInt32 tag) {
+  int attribute_index = -1;
   for (unsigned int i = 0; i < attribute_list.count; ++i) {
     if (attribute_list.attr[i].tag == tag) {
-      return i;
+      attribute_index = i;
+      break;
     }
   }
-  DCHECK(false);
-  return -1;
+  if (attribute_index == -1) {
+    NOTREACHED() << "Unsupported attribute: " << tag;
+    return NULL;
+  }
+  return &(attribute_list.attr[attribute_index]);
 }
 
 void MockKeychain::SetTestDataBytes(int item, UInt32 tag, const void* data,
                                     size_t length) {
-  int attribute_index = IndexForTag(keychain_attr_list_[item], tag);
-  keychain_attr_list_[item].attr[attribute_index].length = length;
+  SecKeychainAttribute* attribute = AttributeWithTag(keychain_attr_list_[item],
+                                                     tag);
+  attribute->length = length;
   if (length > 0) {
-    if (keychain_attr_list_[item].attr[attribute_index].data) {
-      free(keychain_attr_list_[item].attr[attribute_index].data);
+    if (attribute->data) {
+      free(attribute->data);
     }
-    keychain_attr_list_[item].attr[attribute_index].data = malloc(length);
-    CHECK(keychain_attr_list_[item].attr[attribute_index].data);
-    memcpy(keychain_attr_list_[item].attr[attribute_index].data, data, length);
+    attribute->data = malloc(length);
+    CHECK(attribute->data);
+    memcpy(attribute->data, data, length);
   } else {
-    keychain_attr_list_[item].attr[attribute_index].data = NULL;
+    attribute->data = NULL;
   }
 }
 
@@ -101,31 +112,39 @@ void MockKeychain::SetTestDataString(int item, UInt32 tag, const char* value) {
 }
 
 void MockKeychain::SetTestDataPort(int item, UInt32 value) {
-  int attribute_index = IndexForTag(keychain_attr_list_[item],
-                                    kSecPortItemAttr);
-  void* data = keychain_attr_list_[item].attr[attribute_index].data;
-  *(static_cast<UInt32*>(data)) = value;
+  SecKeychainAttribute* attribute = AttributeWithTag(keychain_attr_list_[item],
+                                                     kSecPortItemAttr);
+  UInt32* data = static_cast<UInt32*>(attribute->data);
+  *data = value;
 }
 
 void MockKeychain::SetTestDataProtocol(int item, SecProtocolType value) {
-  int attribute_index = IndexForTag(keychain_attr_list_[item],
-                                    kSecProtocolItemAttr);
-  void* data = keychain_attr_list_[item].attr[attribute_index].data;
-  *(static_cast<SecProtocolType*>(data)) = value;
+  SecKeychainAttribute* attribute = AttributeWithTag(keychain_attr_list_[item],
+                                                     kSecProtocolItemAttr);
+  SecProtocolType* data = static_cast<SecProtocolType*>(attribute->data);
+  *data = value;
 }
 
 void MockKeychain::SetTestDataAuthType(int item, SecAuthenticationType value) {
-  int attribute_index = IndexForTag(keychain_attr_list_[item],
-                                    kSecAuthenticationTypeItemAttr);
-  void* data = keychain_attr_list_[item].attr[attribute_index].data;
-  *(static_cast<SecAuthenticationType*>(data)) = value;
+  SecKeychainAttribute* attribute = AttributeWithTag(
+      keychain_attr_list_[item], kSecAuthenticationTypeItemAttr);
+  SecAuthenticationType* data = static_cast<SecAuthenticationType*>(
+      attribute->data);
+  *data = value;
 }
 
 void MockKeychain::SetTestDataNegativeItem(int item, Boolean value) {
-  int attribute_index = IndexForTag(keychain_attr_list_[item],
-                                    kSecNegativeItemAttr);
-  void* data = keychain_attr_list_[item].attr[attribute_index].data;
-  *(static_cast<Boolean*>(data)) = value;
+  SecKeychainAttribute* attribute = AttributeWithTag(keychain_attr_list_[item],
+                                                     kSecNegativeItemAttr);
+  Boolean* data = static_cast<Boolean*>(attribute->data);
+  *data = value;
+}
+
+void MockKeychain::SetTestDataCreator(int item, OSType value) {
+  SecKeychainAttribute* attribute = AttributeWithTag(keychain_attr_list_[item],
+                                                     kSecCreatorItemAttr);
+  OSType* data = static_cast<OSType*>(attribute->data);
+  *data = value;
 }
 
 void MockKeychain::SetTestDataPasswordBytes(int item, const void* data,
@@ -185,11 +204,19 @@ OSStatus MockKeychain::ItemModifyAttributesAndData(
     return errSecInvalidItemRef;
   }
 
+  MockKeychain* mutable_this = const_cast<MockKeychain*>(this);
   if (attrList) {
-    NOTIMPLEMENTED();
+    for (UInt32 change_attr = 0; change_attr < attrList->count; ++change_attr) {
+      if (attrList->attr[change_attr].tag == kSecCreatorItemAttr) {
+        void* data = attrList->attr[change_attr].data;
+        mutable_this->SetTestDataCreator(item_index,
+                                         *(static_cast<OSType*>(data)));
+      } else {
+        NOTIMPLEMENTED();
+      }
+    }
   }
   if (data) {
-    MockKeychain* mutable_this = const_cast<MockKeychain*>(this);
     mutable_this->SetTestDataPasswordBytes(item_index, data, length);
   }
   return noErr;
@@ -212,10 +239,9 @@ OSStatus MockKeychain::SearchCreateFromAttributes(
   for (unsigned int mock_item = 0; mock_item < item_count_; ++mock_item) {
     bool mock_item_matches = true;
     for (UInt32 search_attr = 0; search_attr < attrList->count; ++search_attr) {
-      int mock_attr = IndexForTag(keychain_attr_list_[mock_item],
-                                  attrList->attr[search_attr].tag);
       SecKeychainAttribute* mock_attribute =
-          &(keychain_attr_list_[mock_item].attr[mock_attr]);
+          AttributeWithTag(keychain_attr_list_[mock_item],
+                           attrList->attr[search_attr].tag);
       if (mock_attribute->length != attrList->attr[search_attr].length ||
           memcmp(mock_attribute->data, attrList->attr[search_attr].data,
                  attrList->attr[search_attr].length) != 0) {
@@ -277,8 +303,11 @@ OSStatus MockKeychain::AddInternetPassword(
   mutable_this->SetTestDataString(target_item, kSecCreationDateItemAttr,
                                   time_string);
 
+  added_via_api_.insert(target_item);
+
   if (itemRef) {
     *itemRef = reinterpret_cast<SecKeychainItemRef>(target_item + 1);
+    ++keychain_item_copy_count_;
   }
   return noErr;
 }
@@ -319,6 +348,19 @@ int MockKeychain::UnfreedAttributeDataCount() const {
   return attribute_data_copy_count_;
 }
 
+bool MockKeychain::CreatorCodesSetForAddedItems() const {
+  for (std::set<unsigned int>::const_iterator i = added_via_api_.begin();
+       i != added_via_api_.end(); ++i) {
+    SecKeychainAttribute* attribute = AttributeWithTag(keychain_attr_list_[*i],
+                                                       kSecCreatorItemAttr);
+    OSType* data = static_cast<OSType*>(attribute->data);
+    if (*data == 0) {
+      return false;
+    }
+  }
+  return true;
+}
+
 void MockKeychain::AddTestItem(const KeychainTestData& item_data) {
   unsigned int index = item_count_++;
   CHECK(index < item_capacity_);

chrome/browser/keychain_mock_mac.h

@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_KEYCHAIN_MOCK_MAC_H_
 #define CHROME_BROWSER_KEYCHAIN_MOCK_MAC_H_
 
+#include <set>
 #include <vector>
 
 #include "chrome/browser/keychain_mac.h"
@@ -62,6 +63,10 @@ class MockKeychain : public MacKeychain {
   int UnfreedKeychainItemCount() const;
   int UnfreedAttributeDataCount() const;
 
+  // Returns true if all items added with AddInternetPassword have a creator
+  // code set.
+  bool CreatorCodesSetForAddedItems() const;
+
   struct KeychainTestData {
     const SecAuthenticationType auth_type;
     const char* server;
@@ -92,15 +97,16 @@ class MockKeychain : public MacKeychain {
   void SetTestDataProtocol(int item, SecProtocolType value);
   void SetTestDataAuthType(int item, SecAuthenticationType value);
   void SetTestDataNegativeItem(int item, Boolean value);
+  void SetTestDataCreator(int item, OSType value);
   // Sets the password data and length for the item-th test item.
   void SetTestDataPasswordBytes(int item, const void* data, size_t length);
   // Sets the password for the item-th test item. As with SetTestDataString,
   // the data will not be null-terminated.
   void SetTestDataPasswordString(int item, const char* value);
 
-  // Returns the index of |tag| in |attribute_list|, or -1 if it's not found.
-  static int IndexForTag(const SecKeychainAttributeList& attribute_list,
-                         UInt32 tag);
+  // Returns the address of the attribute in attribute_list with tag |tag|.
+  static SecKeychainAttribute* AttributeWithTag(
+      const SecKeychainAttributeList& attribute_list, UInt32 tag);
 
   static const int kDummySearchRef = 1000;
 
@@ -126,6 +132,9 @@ class MockKeychain : public MacKeychain {
   mutable int search_copy_count_;
   mutable int keychain_item_copy_count_;
   mutable int attribute_data_copy_count_;
+
+  // Tracks which items (by index) were added with AddInternetPassword.
+  mutable std::set<unsigned int> added_via_api_;
 };
 
 #endif  // CHROME_BROWSER_KEYCHAIN_MOCK_MAC_H_

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;
 }
 

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);

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());

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) {

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);

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_;
 };
 

chrome/common/sqlite_utils.cc

@@ -221,6 +221,11 @@ sqlite_int64 SQLStatement::last_insert_rowid() {
   return sqlite3_last_insert_rowid(db_handle());
 }
 
+int SQLStatement::changes() {
+  DCHECK(stmt_);
+  return sqlite3_changes(db_handle());
+}
+
 sqlite3* SQLStatement::db_handle() {
   DCHECK(stmt_);
   return sqlite3_db_handle(stmt_);

chrome/common/sqlite_utils.h

@@ -215,6 +215,7 @@ class SQLStatement : public scoped_sqlite3_stmt_ptr {
   int step();
   int reset();
   sqlite_int64 last_insert_rowid();
+  int changes();
   sqlite3* db_handle();
 
   //