Reland "[iOS][Credential-Provider] Observe changes in the password store"

This is a reland of 2d2185d8

The fix is to remove the ifdef check. I incorrectly assumed that DCHECK
would strip the code on release configurations.

Original change's description:
> [iOS][Credential-Provider] Observe changes in the password store
>
> Updates the credential store with changes from the password store.
> Renames SyncAllCredentials to RequestSyncAllCredentials to make clearer
> the async nature of it.
> Creates entry functions for adding, updating, and removing passwords
> which will be used to also keep up to date the OS metadata in a follow
> up CL.
> Injects the credential store for easier testing.
>
> Bug: 1066803
> Change-Id: Id79c3b276517c7bb223b3988db4d1a8c25e00025
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2152814
> Commit-Queue: Javier Ernesto Flores Robles <javierrobles@chromium.org>
> Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#760483}

TBR=vasilii@chromium.org

Bug: 1066803
Change-Id: Ie50f790c6cc8715c13017fc930327019f137fb3e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2157045Reviewed-by: Javier Ernesto Flores Robles <javierrobles@chromium.org>
Commit-Queue: Javier Ernesto Flores Robles <javierrobles@chromium.org>
Cr-Commit-Position: refs/heads/master@{#760564}

ios/chrome/browser/credential_provider/BUILD.gn

@@ -56,6 +56,7 @@ source_set("unit_tests") {
     "//components/password_manager/core/browser:test_support",
     "//ios/chrome/common/app_group",
     "//ios/chrome/common/credential_provider",
+    "//ios/chrome/common/credential_provider:ui",
     "//testing/gtest",
     "//url",
   ]

ios/chrome/browser/credential_provider/archivable_credential+password_form.h

@@ -15,7 +15,7 @@ struct PasswordForm;
 @interface ArchivableCredential (PasswordForm)
 
 // Convenience initializer from a PasswordForm. Will return nil for forms
-// with an empty origin or Android forms.
+// blacklisted by the user, with an empty origin or Android forms.
 - (instancetype)initWithPasswordForm:(const autofill::PasswordForm&)passwordForm
                              favicon:(NSString*)favicon
                 validationIdentifier:(NSString*)validationIdentifier;

ios/chrome/browser/credential_provider/archivable_credential+password_form.mm

@@ -38,7 +38,7 @@ NSString* recordIdentifierForPasswordForm(const autofill::PasswordForm& form) {
 - (instancetype)initWithPasswordForm:(const autofill::PasswordForm&)passwordForm
                              favicon:(NSString*)favicon
                 validationIdentifier:(NSString*)validationIdentifier {
-  if (passwordForm.origin.is_empty() ||
+  if (passwordForm.origin.is_empty() || passwordForm.blacklisted_by_user ||
       password_manager::IsValidAndroidFacetURI(passwordForm.signon_realm)) {
     return nil;
   }

ios/chrome/browser/credential_provider/credential_provider_service.h

@@ -16,34 +16,43 @@
 // Extension data up to date.
 class CredentialProviderService
     : public KeyedService,
-      public password_manager::PasswordStoreConsumer {
+      public password_manager::PasswordStoreConsumer,
+      public password_manager::PasswordStore::Observer {
  public:
   // Initializes the service.
   CredentialProviderService(
       scoped_refptr<password_manager::PasswordStore> password_store,
-      NSURL* file_url);
+      ArchivableCredentialStore* credential_store);
   ~CredentialProviderService() override;
 
   // KeyedService:
   void Shutdown() override;
 
  private:
-  friend class CredentialProviderServiceTest;
-
-  // Request all the credentials to sync them. B efore adding the fresh ones,
+  // Request all the credentials to sync them. Before adding the fresh ones,
   // the old ones are deleted.
-  void SyncAllCredentials();
+  void RequestSyncAllCredentials();
 
-  // Adds a credential with the passed args to the store.
+  // Adds a credential with |form| to the store.
   void SaveCredential(const autofill::PasswordForm& form) const;
 
-  // Syncs the store to disk.
-  void SyncStore() const;
+  // Updates a credential with |form| in the store.
+  void UpdateCredential(const autofill::PasswordForm& form) const;
+
+  // Removes a credential with |form| from the store.
+  void RemoveCredential(const autofill::PasswordForm& form) const;
+
+  // Syncs the credential store to disk.
+  void SyncStore(void (^completion)(NSError*)) const;
 
   // PasswordStoreConsumer:
   void OnGetPasswordStoreResults(
       std::vector<std::unique_ptr<autofill::PasswordForm>> results) override;
 
+  // PasswordStore::Observer:
+  void OnLoginsChanged(
+      const password_manager::PasswordStoreChangeList& changes) override;
+
   // The interface for getting and manipulating a user's saved passwords.
   scoped_refptr<password_manager::PasswordStore> password_store_;
 

ios/chrome/browser/credential_provider/credential_provider_service.mm

@@ -8,20 +8,24 @@
 #include "base/scoped_observer.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "build/build_config.h"
+#include "components/password_manager/core/browser/android_affiliation/affiliation_utils.h"
+#include "components/password_manager/core/browser/password_store_change.h"
 #include "ios/chrome/browser/credential_provider/archivable_credential+password_form.h"
 #include "ios/chrome/common/app_group/app_group_constants.h"
 #import "ios/chrome/common/credential_provider/archivable_credential.h"
 #import "ios/chrome/common/credential_provider/archivable_credential_store.h"
 #import "ios/chrome/common/credential_provider/constants.h"
 
-#if !defined(NDEBUG)
-#include "components/password_manager/core/browser/android_affiliation/affiliation_utils.h"
-#endif  // !defined(NDEBUG)
+#if !defined(__has_feature) || !__has_feature(objc_arc)
+#error "This file requires ARC support."
+#endif
 
 namespace {
 
 using autofill::PasswordForm;
 using password_manager::PasswordStore;
+using password_manager::PasswordStoreChange;
+using password_manager::PasswordStoreChangeList;
 
 BOOL ShouldSyncAllCredentials() {
   NSUserDefaults* shared_defaults = app_group::GetGroupUserDefaults();
@@ -30,19 +34,34 @@ BOOL ShouldSyncAllCredentials() {
       boolForKey:kUserDefaultsCredentialProviderFirstTimeSyncCompleted];
 }
 
+ArchivableCredential* CredentialFromForm(const PasswordForm& form) {
+  DCHECK(!form.blacklisted_by_user);
+  ArchivableCredential* credential =
+      [[ArchivableCredential alloc] initWithPasswordForm:form
+                                                 favicon:nil
+                                    validationIdentifier:nil];
+  if (!credential) {
+    // Verify that the credential is nil because it's an Android one or
+    // blacklisted.
+    DCHECK(password_manager::IsValidAndroidFacetURI(form.signon_realm) ||
+           form.blacklisted_by_user);
+  }
+  return credential;
+}
+
 }  // namespace
 
 CredentialProviderService::CredentialProviderService(
     scoped_refptr<PasswordStore> password_store,
-    NSURL* file_url)
-    : password_store_(password_store) {
+    ArchivableCredentialStore* credential_store)
+    : password_store_(password_store),
+      archivable_credential_store_(credential_store) {
   DCHECK(password_store_);
-  archivable_credential_store_ =
-      [[ArchivableCredentialStore alloc] initWithFileURL:file_url];
+  password_store_->AddObserver(this);
   // TODO(crbug.com/1066803): Wait for things to settle down before sync, and
   // sync credentials after Sync finishes or some seconds in the future.
   if (ShouldSyncAllCredentials()) {
-    SyncAllCredentials();
+    RequestSyncAllCredentials();
   }
 }
 
@@ -50,7 +69,7 @@ CredentialProviderService::~CredentialProviderService() {}
 
 void CredentialProviderService::Shutdown() {}
 
-void CredentialProviderService::SyncAllCredentials() {
+void CredentialProviderService::RequestSyncAllCredentials() {
   password_store_->GetAutofillableLogins(this);
 }
 
@@ -60,32 +79,65 @@ void CredentialProviderService::OnGetPasswordStoreResults(
   for (const auto& form : results) {
     SaveCredential(*form);
   }
-  SyncStore();
+  SyncStore(^(NSError* error) {
+    if (!error) {
+      [app_group::GetGroupUserDefaults()
+          setBool:YES
+           forKey:kUserDefaultsCredentialProviderFirstTimeSyncCompleted];
+    }
+  });
 }
 
 void CredentialProviderService::SaveCredential(const PasswordForm& form) const {
-  ArchivableCredential* credential =
-      [[ArchivableCredential alloc] initWithPasswordForm:form
-                                                 favicon:nil
-                                    validationIdentifier:nil];
+  ArchivableCredential* credential = CredentialFromForm(form);
   if (credential) {
     [archivable_credential_store_ addCredential:credential];
   }
-#if !defined(NDEBUG)
-  // Double check that the credential is nil because it is an Android one.
-  else {
-    DCHECK(password_manager::IsValidAndroidFacetURI(form.signon_realm));
+}
+
+void CredentialProviderService::UpdateCredential(
+    const PasswordForm& form) const {
+  ArchivableCredential* credential = CredentialFromForm(form);
+  if (credential) {
+    [archivable_credential_store_ updateCredential:credential];
   }
-#endif  // !defined(NDEBUG)
 }
 
-void CredentialProviderService::SyncStore() const {
+void CredentialProviderService::RemoveCredential(
+    const PasswordForm& form) const {
+  ArchivableCredential* credential = CredentialFromForm(form);
+  if (credential) {
+    [archivable_credential_store_ removeCredential:credential];
+  }
+}
+
+void CredentialProviderService::SyncStore(void (^completion)(NSError*)) const {
   [archivable_credential_store_ saveDataWithCompletion:^(NSError* error) {
     DCHECK(!error) << "An error occurred while saving to disk";
-    if (!error) {
-      [app_group::GetGroupUserDefaults()
-          setBool:YES
-           forKey:kUserDefaultsCredentialProviderFirstTimeSyncCompleted];
+    if (completion) {
+      completion(error);
     }
   }];
 }
+
+void CredentialProviderService::OnLoginsChanged(
+    const PasswordStoreChangeList& changes) {
+  for (const PasswordStoreChange& change : changes) {
+    switch (change.type()) {
+      case PasswordStoreChange::ADD:
+        SaveCredential(change.form());
+        break;
+      case PasswordStoreChange::UPDATE:
+        UpdateCredential(change.form());
+        break;
+      case PasswordStoreChange::REMOVE:
+        RemoveCredential(change.form());
+        break;
+
+      default:
+        NOTREACHED();
+        break;
+    }
+  }
+  SyncStore(nil);
+}

ios/chrome/browser/credential_provider/credential_provider_service_factory.mm

@@ -9,6 +9,7 @@
 #include "ios/chrome/browser/browser_state/chrome_browser_state.h"
 #include "ios/chrome/browser/credential_provider/credential_provider_service.h"
 #include "ios/chrome/browser/passwords/ios_chrome_password_store_factory.h"
+#import "ios/chrome/common/credential_provider/archivable_credential_store.h"
 #import "ios/chrome/common/credential_provider/constants.h"
 
 #if !defined(__has_feature) || !__has_feature(objc_arc)
@@ -46,7 +47,9 @@ CredentialProviderServiceFactory::BuildServiceInstanceFor(
   scoped_refptr<password_manager::PasswordStore> password_store =
       IOSChromePasswordStoreFactory::GetForBrowserState(
           browser_state, ServiceAccessType::IMPLICIT_ACCESS);
-
-  return std::make_unique<CredentialProviderService>(
-      password_store, CredentialProviderSharedArchivableStoreURL());
+  ArchivableCredentialStore* credential_store =
+      [[ArchivableCredentialStore alloc]
+          initWithFileURL:CredentialProviderSharedArchivableStoreURL()];
+  return std::make_unique<CredentialProviderService>(password_store,
+                                                     credential_store);
 }

ios/chrome/browser/credential_provider/credential_provider_service_unittest.mm

@@ -5,11 +5,16 @@
 #include "ios/chrome/browser/credential_provider/credential_provider_service.h"
 
 #include "base/files/scoped_temp_dir.h"
+#include "base/strings/utf_string_conversions.h"
 #import "base/test/ios/wait_util.h"
 #include "base/test/task_environment.h"
+#include "components/autofill/core/common/password_form.h"
 #include "components/password_manager/core/browser/password_store_default.h"
 #include "ios/chrome/common/app_group/app_group_constants.h"
+#import "ios/chrome/common/credential_provider/archivable_credential_store.h"
 #import "ios/chrome/common/credential_provider/constants.h"
+#import "ios/chrome/common/credential_provider/credential.h"
+#import "testing/gtest_mac.h"
 #include "testing/platform_test.h"
 
 #if !defined(__has_feature) || !__has_feature(objc_arc)
@@ -18,6 +23,7 @@
 
 namespace {
 
+using autofill::PasswordForm;
 using base::test::ios::WaitUntilConditionOrTimeout;
 using base::test::ios::kWaitForFileOperationTimeout;
 using password_manager::PasswordStoreDefault;
@@ -36,8 +42,9 @@ class CredentialProviderServiceTest : public PlatformTest {
     NSUserDefaults* shared_defaults = app_group::GetGroupUserDefaults();
     EXPECT_FALSE([shared_defaults
         boolForKey:kUserDefaultsCredentialProviderFirstTimeSyncCompleted]);
-    credential_provider_service_ =
-        std::make_unique<CredentialProviderService>(password_store_, nil);
+    credential_store_ = [[ArchivableCredentialStore alloc] initWithFileURL:nil];
+    credential_provider_service_ = std::make_unique<CredentialProviderService>(
+        password_store_, credential_store_);
   }
 
   void TearDown() override {
@@ -65,6 +72,7 @@ class CredentialProviderServiceTest : public PlatformTest {
   base::test::TaskEnvironment task_environment_;
   std::unique_ptr<CredentialProviderService> credential_provider_service_;
   scoped_refptr<PasswordStoreDefault> password_store_;
+  ArchivableCredentialStore* credential_store_;
 
   DISALLOW_COPY_AND_ASSIGN(CredentialProviderServiceTest);
 };
@@ -84,4 +92,41 @@ TEST_F(CredentialProviderServiceTest, FirstSync) {
   }));
 }
 
+// Test that CredentialProviderService observes changes in the password store.
+TEST_F(CredentialProviderServiceTest, PasswordChanges) {
+  EXPECT_EQ(0u, credential_store_.credentials.count);
+
+  PasswordForm form;
+  form.origin = GURL("http://0.com");
+  form.signon_realm = "http://www.example.com/";
+  form.action = GURL("http://www.example.com/action");
+  form.password_element = base::ASCIIToUTF16("pwd");
+  form.password_value = base::ASCIIToUTF16("example");
+
+  password_store_->AddLogin(form);
+  task_environment_.RunUntilIdle();
+
+  // Expect the store to be populated with 1 credential.
+  ASSERT_EQ(1u, credential_store_.credentials.count);
+
+  NSString* keychainIdentifier =
+      credential_store_.credentials.firstObject.keychainIdentifier;
+  form.password_value = base::ASCIIToUTF16("secret");
+
+  password_store_->UpdateLogin(form);
+  task_environment_.RunUntilIdle();
+
+  // Expect that the credential in the store now has a different keychain
+  // identifier.
+  id<Credential> credential = credential_store_.credentials.firstObject;
+  EXPECT_NSNE(keychainIdentifier, credential.keychainIdentifier);
+  ASSERT_EQ(1u, credential_store_.credentials.count);
+
+  password_store_->RemoveLogin(form);
+  task_environment_.RunUntilIdle();
+
+  // Expect the store to be empty.
+  ASSERT_EQ(0u, credential_store_.credentials.count);
+}
+
 }  // namespace