Removing PasswordStore::GetMatchingInsecureCredentials

This change removes GetMatchingInsecureCredentials and related methods. Also in this change FormFetcher reads insecure credentials directly from PasswordForms Change-Id: I9a7150997062ea67a7102af0fcee786c49b72a74 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3067061 Commit-Queue: Ioana Pandele <ioanap@chromium.org> Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org> Cr-Commit-Position: refs/heads/master@{#908889}
chromium · Aug 5, 2021 · ee6b410 · ee6b410
1 parent e659880
commit ee6b410
Show file tree

Hide file tree

Showing 15 changed files with 60 additions and 307 deletions.
diff --git a/components/password_manager/core/browser/form_fetcher_impl.cc b/components/password_manager/core/browser/form_fetcher_impl.cc
@@ -126,16 +126,6 @@ void FormFetcherImpl::Fetch() {
   // `stats_store` can be null in tests.
   if (stats_store)
     stats_store->GetSiteStats(form_digest_.url.GetOrigin(), this);
-
-  // The desktop bubble needs this information.
-  password_store->GetMatchingInsecureCredentials(form_digest_.signon_realm,
-                                                 this);
-#else
-  if (base::FeatureList::IsEnabled(features::kMutingCompromisedCredentials)) {
-    // We need this information to mute leak detection warming.
-    password_store->GetMatchingInsecureCredentials(form_digest_.signon_realm,
-                                                   this);
-  }
 #endif
 }
 
@@ -215,6 +205,15 @@ std::unique_ptr<FormFetcher> FormFetcherImpl::Clone() {
 
 void FormFetcherImpl::ProcessPasswordStoreResults(
     std::vector<std::unique_ptr<PasswordForm>> results) {
+  insecure_credentials_.clear();
+  for (const auto& form : results) {
+    for (const auto& issue : form->password_issues) {
+      insecure_credentials_.emplace_back(
+          form->signon_realm, form->username_value, issue.second.create_time,
+          issue.first, issue.second.is_muted);
+      insecure_credentials_.back().in_store = form->in_store;
+    }
+  }
   if (client_->GetProfilePasswordStore()->affiliated_match_helper()) {
     client_->GetProfilePasswordStore()
         ->affiliated_match_helper()
@@ -303,9 +302,4 @@ void FormFetcherImpl::ProcessMigratedForms(
   ProcessPasswordStoreResults(std::move(forms));
 }
 
-void FormFetcherImpl::OnGetInsecureCredentials(
-    std::vector<InsecureCredential> insecure_credentials) {
-  insecure_credentials_ = std::move(insecure_credentials);
-}
-
 }  // namespace password_manager
diff --git a/components/password_manager/core/browser/form_fetcher_impl.h b/components/password_manager/core/browser/form_fetcher_impl.h
@@ -13,7 +13,6 @@
 #include "base/observer_list.h"
 #include "components/password_manager/core/browser/form_fetcher.h"
 #include "components/password_manager/core/browser/http_password_store_migrator.h"
-#include "components/password_manager/core/browser/insecure_credentials_consumer.h"
 #include "components/password_manager/core/browser/insecure_credentials_table.h"
 #include "components/password_manager/core/browser/password_store.h"
 #include "components/password_manager/core/browser/password_store_consumer.h"
@@ -27,7 +26,6 @@ class PasswordManagerClient;
 // update the Clone() method accordingly.
 class FormFetcherImpl : public FormFetcher,
                         public PasswordStoreConsumer,
-                        public InsecureCredentialsConsumer,
                         public HttpPasswordStoreMigrator::Consumer {
  public:
   // |form_digest| describes what credentials need to be retrieved and
@@ -116,10 +114,6 @@ class FormFetcherImpl : public FormFetcher,
   void ProcessMigratedForms(
       std::vector<std::unique_ptr<PasswordForm>> forms) override;
 
-  // InsecureCredentialsConsumer:
-  void OnGetInsecureCredentials(
-      std::vector<InsecureCredential> insecure_credentials) override;
-
   // Does the actual migration.
   std::unique_ptr<HttpPasswordStoreMigrator> http_migrator_;
 

diff --git a/components/password_manager/core/browser/form_fetcher_impl_unittest.cc b/components/password_manager/core/browser/form_fetcher_impl_unittest.cc
@@ -447,11 +447,15 @@ TEST_P(FormFetcherImplTest, Stats) {
 TEST_P(FormFetcherImplTest, InsecureCredentials) {
   Fetch();
   form_fetcher_->AddConsumer(&consumer_);
-  const std::vector<InsecureCredential> credentials = {InsecureCredential(
-      form_digest_.signon_realm, u"username_value", base::Time::FromTimeT(1),
-      InsecureType::kLeaked, IsMuted(false))};
-  static_cast<InsecureCredentialsConsumer*>(form_fetcher_.get())
-      ->OnGetInsecureCredentials(credentials);
+  PasswordForm form = CreateNonFederated();
+  form.password_issues.insert({InsecureType::kLeaked, InsecurityMetadata()});
+  std::vector<std::unique_ptr<PasswordForm>> results;
+  results.push_back(std::make_unique<PasswordForm>(form));
+  const std::vector<InsecureCredential> credentials = {
+      InsecureCredential(form.signon_realm, form.username_value, base::Time(),
+                         InsecureType::kLeaked, IsMuted(false))};
+  static_cast<PasswordStoreConsumer*>(form_fetcher_.get())
+      ->OnGetPasswordStoreResultsFrom(mock_store_.get(), std::move(results));
   EXPECT_THAT(form_fetcher_->GetInsecureCredentials(),
               UnorderedElementsAreArray(credentials));
 }
@@ -526,53 +530,13 @@ TEST_P(FormFetcherImplTest, FetchStatistics) {
   EXPECT_THAT(form_fetcher_->GetInteractionsStats(),
               UnorderedElementsAre(stats));
 }
-
-TEST_P(FormFetcherImplTest, FetchInsecure) {
-  std::vector<InsecureCredential> list = {InsecureCredential(
-      form_digest_.signon_realm, u"username_value", base::Time::FromTimeT(1),
-      InsecureType::kLeaked, IsMuted(false))};
-  EXPECT_CALL(*mock_store_,
-              GetMatchingInsecureCredentialsImpl(form_digest_.signon_realm))
-      .WillOnce(Return(list));
-  form_fetcher_->Fetch();
-  task_environment_.RunUntilIdle();
-
-  EXPECT_THAT(form_fetcher_->GetInsecureCredentials(),
-              UnorderedElementsAreArray(list));
-}
 #else
 TEST_P(FormFetcherImplTest, DontFetchStatistics) {
   EXPECT_CALL(*mock_store_, GetLogins(form_digest_, form_fetcher_.get()));
   EXPECT_CALL(mock_smart_bubble_stats_store_, GetSiteStats).Times(0);
   form_fetcher_->Fetch();
   task_environment_.RunUntilIdle();
 }
-
-TEST_P(FormFetcherImplTest, DontFetchInsecure) {
-  base::test::ScopedFeatureList feature_list;
-  feature_list.InitWithFeatureState(features::kMutingCompromisedCredentials,
-                                    false);
-  EXPECT_CALL(*mock_store_, GetMatchingInsecureCredentialsImpl).Times(0);
-  form_fetcher_->Fetch();
-  task_environment_.RunUntilIdle();
-}
-
-TEST_P(FormFetcherImplTest, FetchInsecure) {
-  base::test::ScopedFeatureList feature_list;
-  feature_list.InitWithFeatureState(features::kMutingCompromisedCredentials,
-                                    true);
-  std::vector<InsecureCredential> list = {InsecureCredential(
-      form_digest_.signon_realm, u"username_value", base::Time::FromTimeT(1),
-      InsecureType::kLeaked, IsMuted(false))};
-  EXPECT_CALL(*mock_store_,
-              GetMatchingInsecureCredentialsImpl(form_digest_.signon_realm))
-      .WillOnce(Return(list));
-  form_fetcher_->Fetch();
-  task_environment_.RunUntilIdle();
-
-  EXPECT_THAT(form_fetcher_->GetInsecureCredentials(),
-              UnorderedElementsAreArray(list));
-}
 #endif
 
 // Test that ensures HTTP passwords are not migrated on HTTP sites.
@@ -866,13 +830,15 @@ TEST_P(FormFetcherImplTest, Clone_Stats) {
 TEST_P(FormFetcherImplTest, Clone_Insecure) {
   Fetch();
   // Pass empty results to make the state NOT_WAITING.
-  store_consumer()->OnGetPasswordStoreResultsFrom(
-      mock_store_.get(), std::vector<std::unique_ptr<PasswordForm>>());
-  const std::vector<InsecureCredential> credentials = {InsecureCredential(
-      form_digest_.signon_realm, u"username_value", base::Time::FromTimeT(1),
-      InsecureType::kLeaked, IsMuted(false))};
-  static_cast<InsecureCredentialsConsumer*>(form_fetcher_.get())
-      ->OnGetInsecureCredentials(credentials);
+  PasswordForm form = CreateNonFederated();
+  form.password_issues.insert({InsecureType::kLeaked, InsecurityMetadata()});
+  std::vector<std::unique_ptr<PasswordForm>> results;
+  results.push_back(std::make_unique<PasswordForm>(form));
+  const std::vector<InsecureCredential> credentials = {
+      InsecureCredential(form.signon_realm, form.username_value, base::Time(),
+                         InsecureType::kLeaked, IsMuted(false))};
+  static_cast<PasswordStoreConsumer*>(form_fetcher_.get())
+      ->OnGetPasswordStoreResultsFrom(mock_store_.get(), std::move(results));
 
   auto clone = form_fetcher_->Clone();
   EXPECT_THAT(clone->GetInsecureCredentials(),

diff --git a/components/password_manager/core/browser/mock_password_store.h b/components/password_manager/core/browser/mock_password_store.h
@@ -75,10 +75,6 @@ class MockPasswordStore : public PasswordStore {
               GetAllInsecureCredentialsImpl,
               (),
               (override));
-  MOCK_METHOD(std::vector<InsecureCredential>,
-              GetMatchingInsecureCredentialsImpl,
-              (const std::string&),
-              (override));
   MOCK_METHOD(void,
               GetAllLoginsWithAffiliationAndBrandingInformation,
               (PasswordStoreConsumer*),

diff --git a/components/password_manager/core/browser/multi_store_form_fetcher.cc b/components/password_manager/core/browser/multi_store_form_fetcher.cc
@@ -55,11 +55,6 @@ void MultiStoreFormFetcher::Fetch() {
   if (account_password_store) {
     state_ = State::WAITING;
     account_password_store->GetLogins(form_digest_, this);
-#if !defined(OS_IOS) && !defined(OS_ANDROID)
-    // The desktop bubble needs this information.
-    account_password_store->GetMatchingInsecureCredentials(
-        form_digest_.signon_realm, this);
-#endif
   }
 }
 
@@ -163,14 +158,6 @@ void MultiStoreFormFetcher::ProcessMigratedForms(
   AggregatePasswordStoreResults(std::move(forms));
 }
 
-void MultiStoreFormFetcher::OnGetInsecureCredentials(
-    std::vector<InsecureCredential> insecure_credentials) {
-  // Both the profile and account store has been queried. Therefore, append the
-  // received credentials to the existing ones.
-  base::ranges::move(insecure_credentials,
-                     std::back_inserter(insecure_credentials_));
-}
-
 void MultiStoreFormFetcher::SplitResults(
     std::vector<std::unique_ptr<PasswordForm>> results) {
   // Compute the |is_blocklisted_in_profile_store_| and

diff --git a/components/password_manager/core/browser/multi_store_form_fetcher.h b/components/password_manager/core/browser/multi_store_form_fetcher.h
@@ -39,10 +39,6 @@ class MultiStoreFormFetcher : public FormFetcherImpl {
   void ProcessMigratedForms(
       std::vector<std::unique_ptr<PasswordForm>> forms) override;
 
-  // InsecureCredentialsConsumer:
-  void OnGetInsecureCredentials(
-      std::vector<InsecureCredential> insecure_credentials) override;
-
  private:
   void AggregatePasswordStoreResults(
       std::vector<std::unique_ptr<PasswordForm>> results);

diff --git a/components/password_manager/core/browser/multi_store_form_fetcher_unittest.cc b/components/password_manager/core/browser/multi_store_form_fetcher_unittest.cc
@@ -445,23 +445,39 @@ TEST_F(MultiStoreFormFetcherTest, MovingToAccountStoreIsBlocked) {
 
 TEST_F(MultiStoreFormFetcherTest, InsecureCredentials) {
   Fetch();
+  PasswordForm profile_form =
+      CreateHTMLForm("www.url.com", "username1", "pass");
+  profile_form.password_issues.insert(
+      {InsecureType::kLeaked, InsecurityMetadata()});
+  std::vector<std::unique_ptr<PasswordForm>> profile_results;
+  profile_results.push_back(std::make_unique<PasswordForm>(profile_form));
+
+  PasswordForm account_form =
+      CreateHTMLForm("www.url.com", "username1", "pass");
+  account_form.password_issues.insert(
+      {InsecureType::kLeaked, InsecurityMetadata()});
+  std::vector<std::unique_ptr<PasswordForm>> account_results;
+  account_results.push_back(std::make_unique<PasswordForm>(account_form));
+
   InsecureCredential profile_store_insecure_credentials(
-      form_digest_.signon_realm, u"profile_username", base::Time::FromTimeT(1),
+      profile_form.signon_realm, profile_form.username_value, base::Time(),
       InsecureType::kLeaked, IsMuted(false));
   profile_store_insecure_credentials.in_store =
       PasswordForm::Store::kProfileStore;
 
   InsecureCredential account_store_insecure_credentials(
-      form_digest_.signon_realm, u"account_username", base::Time::FromTimeT(1),
+      account_form.signon_realm, account_form.username_value, base::Time(),
       InsecureType::kLeaked, IsMuted(false));
   account_store_insecure_credentials.in_store =
       PasswordForm::Store::kAccountStore;
 
-  static_cast<InsecureCredentialsConsumer*>(form_fetcher_.get())
-      ->OnGetInsecureCredentials({profile_store_insecure_credentials});
+  static_cast<PasswordStoreConsumer*>(form_fetcher_.get())
+      ->OnGetPasswordStoreResultsFrom(profile_mock_store_.get(),
+                                      std::move(profile_results));
 
-  static_cast<InsecureCredentialsConsumer*>(form_fetcher_.get())
-      ->OnGetInsecureCredentials({account_store_insecure_credentials});
+  static_cast<PasswordStoreConsumer*>(form_fetcher_.get())
+      ->OnGetPasswordStoreResultsFrom(account_mock_store_.get(),
+                                      std::move(account_results));
 
   EXPECT_THAT(
       form_fetcher_->GetInsecureCredentials(),

diff --git a/components/password_manager/core/browser/password_manager_unittest.cc b/components/password_manager/core/browser/password_manager_unittest.cc
@@ -4277,19 +4277,15 @@ TEST_P(PasswordManagerTest, DontStartLeakDetectionWhenMuted) {
       std::make_unique<testing::StrictMock<MockLeakDetectionCheckFactory>>();
   manager()->set_leak_factory(std::move(mock_factory));
 
-  const PasswordForm form = MakeSimpleForm();
+  PasswordForm form = MakeSimpleForm();
+  form.password_issues.insert(
+      {InsecureType::kLeaked, InsecurityMetadata(base::Time(), IsMuted(true))});
   std::vector<FormData> observed = {form.form_data};
   EXPECT_CALL(*store_, GetLogins)
-      .WillRepeatedly(WithArg<1>(InvokeEmptyConsumerWithForms(store_.get())));
+      .WillRepeatedly(WithArg<1>(InvokeConsumer(store_.get(), form)));
   manager()->OnPasswordFormsParsed(&driver_, observed);
   manager()->OnPasswordFormsRendered(&driver_, observed, true);
 
-  // Add muted insecure credentials.
-  std::vector<InsecureCredential> insecure_credentials = {InsecureCredential(
-      form.signon_realm, form.username_value, base::Time::FromTimeT(1),
-      InsecureType::kLeaked, IsMuted(true))};
-  EXPECT_CALL(*store_, GetMatchingInsecureCredentialsImpl(form.signon_realm))
-      .WillOnce(Return(insecure_credentials));
   task_environment_.RunUntilIdle();
 
   EXPECT_CALL(client_, IsSavingAndFillingEnabled).WillRepeatedly(Return(true));
@@ -4316,19 +4312,18 @@ TEST_P(PasswordManagerTest, StartLeakCheckWhenForUsernameNotMuted) {
   MockLeakDetectionCheckFactory* weak_factory = mock_factory.get();
   manager()->set_leak_factory(std::move(mock_factory));
 
-  const PasswordForm form = MakeSimpleForm();
+  PasswordForm form = MakeSimpleForm();
   std::vector<FormData> observed = {form.form_data};
+
+  form.username_value = u"different_username";
+  form.password_issues.insert(
+      {InsecureType::kLeaked, InsecurityMetadata(base::Time(), IsMuted(true))});
+
   EXPECT_CALL(*store_, GetLogins)
-      .WillRepeatedly(WithArg<1>(InvokeEmptyConsumerWithForms(store_.get())));
+      .WillRepeatedly(WithArg<1>(InvokeConsumer(store_.get(), form)));
   manager()->OnPasswordFormsParsed(&driver_, observed);
   manager()->OnPasswordFormsRendered(&driver_, observed, true);
 
-  // Add muted insecure credentials.
-  std::vector<InsecureCredential> insecure_credentials = {InsecureCredential(
-      form.signon_realm, u"different_username", base::Time::FromTimeT(1),
-      InsecureType::kLeaked, IsMuted(true))};
-  EXPECT_CALL(*store_, GetMatchingInsecureCredentialsImpl(form.signon_realm))
-      .WillOnce(Return(insecure_credentials));
   task_environment_.RunUntilIdle();
 
   EXPECT_CALL(client_, IsSavingAndFillingEnabled).WillRepeatedly(Return(true));

diff --git a/components/password_manager/core/browser/password_store.cc b/components/password_manager/core/browser/password_store.cc
@@ -332,25 +332,6 @@ void PasswordStore::GetAllInsecureCredentials(
       base::BindOnce(&PasswordStore::GetAllInsecureCredentialsImpl, this));
 }
 
-void PasswordStore::GetMatchingInsecureCredentials(
-    const std::string& signon_realm,
-    InsecureCredentialsConsumer* consumer) {
-  if (affiliated_match_helper_) {
-    PasswordFormDigest form(PasswordForm::Scheme::kHtml, signon_realm,
-                            GURL(signon_realm));
-    affiliated_match_helper_->GetAffiliatedAndroidAndWebRealms(
-        form,
-        base::BindOnce(
-            &PasswordStore::ScheduleGetInsecureCredentialsWithAffiliations,
-            this, consumer->GetWeakPtr(), signon_realm));
-  } else {
-    PostInsecureCredentialsTaskAndReplyToConsumerWithResult(
-        consumer,
-        base::BindOnce(&PasswordStore::GetMatchingInsecureCredentialsImpl, this,
-                       signon_realm));
-  }
-}
-
 void PasswordStore::AddObserver(Observer* observer) {
   observers_.AddObserver(observer);
 }
@@ -403,14 +384,6 @@ std::vector<InsecureCredential> PasswordStore::GetAllInsecureCredentialsImpl() {
   return std::vector<InsecureCredential>();
 }
 
-std::vector<InsecureCredential>
-PasswordStore::GetMatchingInsecureCredentialsImpl(
-    const std::string& signon_realm) {
-  // TODO(crbug.com/1217070): Move as implementation detail into backend.
-  LOG(ERROR) << "Called function without implementation: " << __func__;
-  return std::vector<InsecureCredential>();
-}
-
 void PasswordStore::InvokeAndNotifyAboutInsecureCredentialsChange(
     base::OnceCallback<PasswordStoreChangeList()> callback) {
   DCHECK(background_task_runner_->RunsTasksInCurrentSequence());
@@ -486,23 +459,6 @@ void PasswordStore::UnblocklistInternal(
   handler->InvokeOnCompletion(std::move(notify_callback));
 }
 
-std::vector<InsecureCredential>
-PasswordStore::GetInsecureCredentialsWithAffiliationsImpl(
-    const std::string& signon_realm,
-    const std::vector<std::string>& additional_affiliated_realms) {
-  DCHECK(background_task_runner_->RunsTasksInCurrentSequence());
-  std::vector<InsecureCredential> results(
-      GetMatchingInsecureCredentialsImpl(signon_realm));
-  for (const std::string& realm : additional_affiliated_realms) {
-    std::vector<InsecureCredential> more_results(
-        GetMatchingInsecureCredentialsImpl(realm));
-    results.insert(results.end(), std::make_move_iterator(more_results.begin()),
-                   std::make_move_iterator(more_results.end()));
-  }
-
-  return results;
-}
-
 void PasswordStore::InjectAffiliationAndBrandingInformation(
     LoginsReply callback,
     LoginsResult forms) {
@@ -515,17 +471,4 @@ void PasswordStore::InjectAffiliationAndBrandingInformation(
   }
 }
 
-void PasswordStore::ScheduleGetInsecureCredentialsWithAffiliations(
-    base::WeakPtr<InsecureCredentialsConsumer> consumer,
-    const std::string& signon_realm,
-    const std::vector<std::string>& additional_affiliated_realms) {
-  if (consumer) {
-    PostInsecureCredentialsTaskAndReplyToConsumerWithResult(
-        consumer.get(),
-        base::BindOnce(
-            &PasswordStore::GetInsecureCredentialsWithAffiliationsImpl, this,
-            signon_realm, additional_affiliated_realms));
-  }
-}
-
 }  // namespace password_manager