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

This reverts commit 2d2185d8.

Reason for revert: This caused ios build failures
https://ci.chromium.org/p/chromium/builders/ci/ios-device/154217?

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,javierrobles@chromium.org

Change-Id: I769316c34e40c248a245553b1e5f78c834eee1aa
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1066803
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2156786Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#760486}

ios/chrome/browser/credential_provider/BUILD.gn

@@ -56,7 +56,6 @@ 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
-// blacklisted by the user, with an empty origin or Android forms.
+// 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() || passwordForm.blacklisted_by_user ||
+  if (passwordForm.origin.is_empty() ||
       password_manager::IsValidAndroidFacetURI(passwordForm.signon_realm)) {
     return nil;
   }

ios/chrome/browser/credential_provider/credential_provider_service.h

@@ -16,43 +16,34 @@
 // Extension data up to date.
 class CredentialProviderService
     : public KeyedService,
-      public password_manager::PasswordStoreConsumer,
-      public password_manager::PasswordStore::Observer {
+      public password_manager::PasswordStoreConsumer {
  public:
   // Initializes the service.
   CredentialProviderService(
       scoped_refptr<password_manager::PasswordStore> password_store,
-      ArchivableCredentialStore* credential_store);
+      NSURL* file_url);
   ~CredentialProviderService() override;
 
   // KeyedService:
   void Shutdown() override;
 
  private:
-  // Request all the credentials to sync them. Before adding the fresh ones,
+  friend class CredentialProviderServiceTest;
+
+  // Request all the credentials to sync them. B efore adding the fresh ones,
   // the old ones are deleted.
-  void RequestSyncAllCredentials();
+  void SyncAllCredentials();
 
-  // Adds a credential with |form| to the store.
+  // Adds a credential with the passed args to the store.
   void SaveCredential(const autofill::PasswordForm& form) 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;
+  // Syncs the store to disk.
+  void SyncStore() 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,7 +8,6 @@
 #include "base/scoped_observer.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "build/build_config.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"
@@ -19,16 +18,10 @@
 #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();
@@ -37,34 +30,19 @@ 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,
-    ArchivableCredentialStore* credential_store)
-    : password_store_(password_store),
-      archivable_credential_store_(credential_store) {
+    NSURL* file_url)
+    : password_store_(password_store) {
   DCHECK(password_store_);
-  password_store_->AddObserver(this);
+  archivable_credential_store_ =
+      [[ArchivableCredentialStore alloc] initWithFileURL:file_url];
   // 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()) {
-    RequestSyncAllCredentials();
+    SyncAllCredentials();
   }
 }
 
@@ -72,7 +50,7 @@ CredentialProviderService::~CredentialProviderService() {}
 
 void CredentialProviderService::Shutdown() {}
 
-void CredentialProviderService::RequestSyncAllCredentials() {
+void CredentialProviderService::SyncAllCredentials() {
   password_store_->GetAutofillableLogins(this);
 }
 
@@ -82,65 +60,32 @@ void CredentialProviderService::OnGetPasswordStoreResults(
   for (const auto& form : results) {
     SaveCredential(*form);
   }
-  SyncStore(^(NSError* error) {
-    if (!error) {
-      [app_group::GetGroupUserDefaults()
-          setBool:YES
-           forKey:kUserDefaultsCredentialProviderFirstTimeSyncCompleted];
-    }
-  });
+  SyncStore();
 }
 
 void CredentialProviderService::SaveCredential(const PasswordForm& form) const {
-  ArchivableCredential* credential = CredentialFromForm(form);
+  ArchivableCredential* credential =
+      [[ArchivableCredential alloc] initWithPasswordForm:form
+                                                 favicon:nil
+                                    validationIdentifier:nil];
   if (credential) {
     [archivable_credential_store_ addCredential:credential];
   }
-}
-
-void CredentialProviderService::UpdateCredential(
-    const PasswordForm& form) const {
-  ArchivableCredential* credential = CredentialFromForm(form);
-  if (credential) {
-    [archivable_credential_store_ updateCredential:credential];
-  }
-}
-
-void CredentialProviderService::RemoveCredential(
-    const PasswordForm& form) const {
-  ArchivableCredential* credential = CredentialFromForm(form);
-  if (credential) {
-    [archivable_credential_store_ removeCredential: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));
   }
+#endif  // !defined(NDEBUG)
 }
 
-void CredentialProviderService::SyncStore(void (^completion)(NSError*)) const {
+void CredentialProviderService::SyncStore() const {
   [archivable_credential_store_ saveDataWithCompletion:^(NSError* error) {
     DCHECK(!error) << "An error occurred while saving to disk";
-    if (completion) {
-      completion(error);
+    if (!error) {
+      [app_group::GetGroupUserDefaults()
+          setBool:YES
+           forKey:kUserDefaultsCredentialProviderFirstTimeSyncCompleted];
     }
   }];
 }
-
-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,7 +9,6 @@
 #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)
@@ -47,9 +46,7 @@ CredentialProviderServiceFactory::BuildServiceInstanceFor(
   scoped_refptr<password_manager::PasswordStore> password_store =
       IOSChromePasswordStoreFactory::GetForBrowserState(
           browser_state, ServiceAccessType::IMPLICIT_ACCESS);
-  ArchivableCredentialStore* credential_store =
-      [[ArchivableCredentialStore alloc]
-          initWithFileURL:CredentialProviderSharedArchivableStoreURL()];
-  return std::make_unique<CredentialProviderService>(password_store,
-                                                     credential_store);
+
+  return std::make_unique<CredentialProviderService>(
+      password_store, CredentialProviderSharedArchivableStoreURL());
 }

ios/chrome/browser/credential_provider/credential_provider_service_unittest.mm

@@ -5,16 +5,11 @@
 #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)
@@ -23,7 +18,6 @@
 
 namespace {
 
-using autofill::PasswordForm;
 using base::test::ios::WaitUntilConditionOrTimeout;
 using base::test::ios::kWaitForFileOperationTimeout;
 using password_manager::PasswordStoreDefault;
@@ -42,9 +36,8 @@ class CredentialProviderServiceTest : public PlatformTest {
     NSUserDefaults* shared_defaults = app_group::GetGroupUserDefaults();
     EXPECT_FALSE([shared_defaults
         boolForKey:kUserDefaultsCredentialProviderFirstTimeSyncCompleted]);
-    credential_store_ = [[ArchivableCredentialStore alloc] initWithFileURL:nil];
-    credential_provider_service_ = std::make_unique<CredentialProviderService>(
-        password_store_, credential_store_);
+    credential_provider_service_ =
+        std::make_unique<CredentialProviderService>(password_store_, nil);
   }
 
   void TearDown() override {
@@ -72,7 +65,6 @@ 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);
 };
@@ -92,41 +84,4 @@ 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