Implement origin-based deletion for passwords in PasswordDefaultMac.

chrome/browser/password_manager/password_store_mac.cc

 // Copyright (c) 2012 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 "chrome/browser/password_manager/password_store_mac.h"
 #include "chrome/browser/password_manager/password_store_mac_internal.h"
 
 #include <CoreServices/CoreServices.h>
 #include <set>
 #include <string>
 #include <utility>
 #include <vector>
 
 #include "base/callback.h"
 #include "base/logging.h"
 #include "base/mac/foundation_util.h"
 #include "base/mac/mac_logging.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/mac/security_wrappers.h"
 #include "components/os_crypt/os_crypt.h"
 #include "components/password_manager/core/browser/affiliation_utils.h"
 #include "components/password_manager/core/browser/login_database.h"
 #include "components/password_manager/core/browser/password_store_change.h"
 #include "content/public/browser/browser_thread.h"
 #include "crypto/apple_keychain.h"
+#include "url/origin.h"
 
 using autofill::PasswordForm;
 using crypto::AppleKeychain;
 using password_manager::PasswordStoreChange;
 using password_manager::PasswordStoreChangeList;
 
 namespace {
 
 // Utility class to handle the details of constructing and running a keychain
 // search from a set of attributes.
@@ skipping 1080 matching lines @@
       if (!DatabaseHasFormMatchingKeychainForm(form)) {
         owned_keychain_adapter.RemovePassword(form);
       }
     }
 
     changes.push_back(PasswordStoreChange(PasswordStoreChange::REMOVE, form));
   }
   return changes;
 }
 
+PasswordStoreChangeList PasswordStoreMac::RemoveLoginsByOriginAndTimeImpl(
+    const url::Origin& origin,
+    base::Time delete_begin,
+    base::Time delete_end) {
+  PasswordStoreChangeList changes;
+  ScopedVector<PasswordForm> forms_to_consider;
+  ScopedVector<PasswordForm> forms_to_remove;
+  if (login_metadata_db_ &&
+      login_metadata_db_->GetLoginsCreatedBetween(delete_begin, delete_end,
+                                                  &forms_to_consider)) {
+    MoveAllFormsOut(
+        &forms_to_consider,
+        [this, &origin, &forms_to_remove](
+            scoped_ptr<autofill::PasswordForm> form_to_consider) {
+          if (origin.IsSameOriginWith(url::Origin(form_to_consider->origin)) &&
+              login_metadata_db_->RemoveLogin(*form_to_consider))
+            forms_to_remove.push_back(form_to_consider.Pass());
+        });
+    if (!forms_to_remove.empty()) {
+      RemoveKeychainForms(forms_to_remove.get());
+      CleanOrphanedForms(&forms_to_remove);  // Add the orphaned forms.

[Timo Reimann, 2015/10/20 20:47:09]

Note that I did not implement tests to verify that keychain forms are removed /
orphaned forms cleaned during the execution of this method as I wasn't
completely sure about the right approach. Here are the available options I see:

1. Extend PasswordStoreMacTest.CheckRemoveLoginsBetween to cover origin-based
deletion too. The downside is that this would duplicate some functionality
(though not all) that PasswordStoreCommonTest.* (the new type-parameterized
tests) already cares about in a more specific way. Consequently, in the worst
case this could lead to two tests failing for the same regression, which is bad
from a test design perspective.
2. Extract the keychain and orphaned forms parts from
PasswordStoreMacTest.CheckRemoveLoginsBetween into a new test and share it by
all methods dealing with the two form types.
3. Make the keychain and orphaned forms injectable so that we can use mocks to
verify the behavior on.
4. Having read crbug.com/466638, I understand that we are dropping Mac keychain
support soon. This might turn it acceptable to skip writing tests for the
functionality at hand altogether and wait for the test gap to close naturally.

Happy to hear your thoughts.

[vasilii, 2015/10/21 16:47:56]

You can proceed with the option 4. I don't think we ship your feature soon
enough.

+      changes = FormsToRemoveChangeList(forms_to_remove.get());
+      LogStatsForBulkDeletion(changes.size());
+    }
+  }
+  return changes;
+}
+
 PasswordStoreChangeList PasswordStoreMac::RemoveLoginsCreatedBetweenImpl(
     base::Time delete_begin,
     base::Time delete_end) {
   PasswordStoreChangeList changes;
   ScopedVector<PasswordForm> forms_to_remove;
   if (login_metadata_db_ &&
       login_metadata_db_->GetLoginsCreatedBetween(delete_begin, delete_end,
                                                   &forms_to_remove) &&
       login_metadata_db_->RemoveLoginsCreatedBetween(delete_begin,
                                                      delete_end)) {
@@ skipping 183 matching lines @@
   ScopedVector<PasswordForm> forms_with_keychain_entry;
   internal_keychain_helpers::GetPasswordsForForms(*keychain_, &database_forms,
                                                   &forms_with_keychain_entry);
 
   // Clean up any orphaned database entries.
   RemoveDatabaseForms(&database_forms);
 
   // Move the orphaned DB forms to the output parameter.
   AppendSecondToFirst(orphaned_forms, &database_forms);
 }