[Passwords] Update UI after each checked credential

This change implements updating the Password Check UI after each
credential was checked. It makes sure to account for duplicates with
regard to canonicalization of usernames and passwords and updates the
progress accordingly.

Bug: 1061500
Change-Id: I46d13e7e007f31596b6c67f9f3e56535cbc5e8a2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2106580
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#751128}

chrome/browser/extensions/api/passwords_private/password_check_delegate.cc

@@ -4,12 +4,17 @@
 
 #include "chrome/browser/extensions/api/passwords_private/password_check_delegate.h"
 
+#include <stddef.h>
+
 #include <algorithm>
 #include <memory>
 
+#include "base/containers/flat_map.h"
 #include "base/containers/flat_set.h"
+#include "base/memory/ref_counted.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/optional.h"
+#include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "chrome/browser/extensions/api/passwords_private/passwords_private_event_router.h"
@@ -25,6 +30,7 @@
 #include "components/password_manager/core/browser/android_affiliation/affiliation_utils.h"
 #include "components/password_manager/core/browser/bulk_leak_check_service.h"
 #include "components/password_manager/core/browser/compromised_credentials_table.h"
+#include "components/password_manager/core/browser/leak_detection/bulk_leak_check.h"
 #include "components/password_manager/core/browser/leak_detection/encryption_utils.h"
 #include "components/password_manager/core/browser/ui/compromised_credentials_provider.h"
 #include "components/password_manager/core/browser/ui/credential_utils.h"
@@ -40,24 +46,94 @@
 
 namespace extensions {
 
-namespace {
-
 using autofill::PasswordForm;
 using password_manager::BulkLeakCheckService;
+using password_manager::CanonicalizedCredential;
 using password_manager::CanonicalizeUsername;
+using password_manager::CompromiseType;
 using password_manager::CredentialWithPassword;
+using password_manager::LeakCheckCredential;
 using ui::TimeFormat;
 using CompromisedCredentialsView =
     password_manager::CompromisedCredentialsProvider::CredentialsView;
 using SavedPasswordsView =
     password_manager::SavedPasswordsPresenter::SavedPasswordsView;
 
+// Key used to attach UserData to a LeakCheckCredential.
+constexpr char kPasswordCheckDataKey[] = "password-check-data-key";
+
+// Class remembering the state required to update the progress of an ongoing
+// Password Check.
+class PasswordCheckProgress : public base::RefCounted<PasswordCheckProgress> {
+ public:
+  base::WeakPtr<PasswordCheckProgress> GetWeakPtr() {
+    return weak_ptr_factory_.GetWeakPtr();
+  }
+
+  size_t remaining_in_queue() const { return remaining_in_queue_; }
+  size_t already_processed() const { return already_processed_; }
+
+  // Increments the counts corresponding to |password|. Intended to be called
+  // for each credential that is passed to the bulk check.
+  void IncrementCounts(const PasswordForm& password) {
+    ++remaining_in_queue_;
+    ++counts_[password];
+  }
+
+  // Updates the counts after a |credential| has been processed by the bulk
+  // check.
+  void OnProcessed(const LeakCheckCredential& credential) {
+    auto it = counts_.find(credential);
+    const int num_matching = it != counts_.end() ? it->second : 0;
+    already_processed_ += num_matching;
+    remaining_in_queue_ -= num_matching;
+  }
+
+ private:
+  friend class base::RefCounted<PasswordCheckProgress>;
+  ~PasswordCheckProgress() = default;
+
+  // Count variables needed to correctly show the progress of the check to the
+  // user. |already_processed_| contains the number of credentials that have
+  // been checked already, while |remaining_in_queue_| remembers how many
+  // passwords still need to be checked.
+  // Since the bulk leak check tries to be as efficient as possible, it performs
+  // a deduplication step before starting to check passwords. In this step it
+  // canonicalizes each credential, and only processes the combinations that are
+  // unique. Since this number likely does not match the total number of saved
+  // passwords, we remember in |counts_| how many saved passwords a given
+  // canonicalized credential corresponds to.
+  size_t already_processed_ = 0;
+  size_t remaining_in_queue_ = 0;
+  base::flat_map<CanonicalizedCredential, size_t> counts_;
+
+  base::WeakPtrFactory<PasswordCheckProgress> weak_ptr_factory_{this};
+};
+
+namespace {
+
+// A class attached to each LeakCheckCredential that holds a shared handle to
+// the PasswordCheckProgress and is able to update the progress accordingly.
+class PasswordCheckData : public LeakCheckCredential::Data {
+ public:
+  explicit PasswordCheckData(scoped_refptr<PasswordCheckProgress> progress)
+      : progress_(std::move(progress)) {}
+  ~PasswordCheckData() override = default;
+
+  std::unique_ptr<Data> Clone() override {
+    return std::make_unique<PasswordCheckData>(progress_);
+  }
+
+ private:
+  scoped_refptr<PasswordCheckProgress> progress_;
+};
+
 api::passwords_private::CompromiseType ConvertCompromiseType(
-    password_manager::CompromiseType type) {
+    CompromiseType type) {
   switch (type) {
-    case password_manager::CompromiseType::kLeaked:
+    case CompromiseType::kLeaked:
       return api::passwords_private::COMPROMISE_TYPE_LEAKED;
-    case password_manager::CompromiseType::kPhished:
+    case CompromiseType::kPhished:
       return api::passwords_private::COMPROMISE_TYPE_PHISHED;
   }
 
@@ -303,8 +379,21 @@ bool PasswordCheckDelegate::RemoveCompromisedCredential(
 }
 
 bool PasswordCheckDelegate::StartPasswordCheck() {
-  is_bulk_check_running_ = true;
-  return bulk_leak_check_service_adapter_.StartBulkLeakCheck();
+  if (bulk_leak_check_service_adapter_.GetBulkLeakCheckState() ==
+      BulkLeakCheckService::State::kRunning) {
+    return false;
+  }
+
+  auto progress = base::MakeRefCounted<PasswordCheckProgress>();
+  for (const auto& password : saved_passwords_presenter_.GetSavedPasswords())
+    progress->IncrementCounts(password);
+
+  password_check_progress_ = progress->GetWeakPtr();
+  PasswordCheckData data(std::move(progress));
+  is_check_running_ = bulk_leak_check_service_adapter_.StartBulkLeakCheck(
+      kPasswordCheckDataKey, &data);
+  DCHECK(is_check_running_);
+  return is_check_running_;
 }
 
 void PasswordCheckDelegate::StopPasswordCheck() {
@@ -313,7 +402,6 @@ void PasswordCheckDelegate::StopPasswordCheck() {
 
 api::passwords_private::PasswordCheckStatus
 PasswordCheckDelegate::GetPasswordCheckStatus() const {
-  // TODO(crbug.com/1047726): Add support for QUOTA_LIMIT state.
   api::passwords_private::PasswordCheckStatus result;
 
   // Obtain the timestamp of the last completed check. This is 0.0 in case the
@@ -333,12 +421,17 @@ PasswordCheckDelegate::GetPasswordCheckStatus() const {
   // Handle the currently running case first, only then consider errors.
   if (state == BulkLeakCheckService::State::kRunning) {
     result.state = api::passwords_private::PASSWORD_CHECK_STATE_RUNNING;
-    result.remaining_in_queue = std::make_unique<int>(
-        bulk_leak_check_service_adapter_.GetPendingChecksCount());
-    // Make sure we'll never surface a negative number in the UI.
-    result.already_processed = std::make_unique<int>(
-        std::max(0, base::checked_cast<int>(saved_passwords.size()) -
-                        *result.remaining_in_queue));
+
+    if (password_check_progress_) {
+      result.already_processed =
+          std::make_unique<int>(password_check_progress_->already_processed());
+      result.remaining_in_queue =
+          std::make_unique<int>(password_check_progress_->remaining_in_queue());
+    } else {
+      result.already_processed = std::make_unique<int>(0);
+      result.remaining_in_queue = std::make_unique<int>(0);
+    }
+
     return result;
   }
 
@@ -375,10 +468,10 @@ void PasswordCheckDelegate::OnCompromisedCredentialsChanged(
 }
 
 void PasswordCheckDelegate::OnStateChanged(BulkLeakCheckService::State state) {
-  if (is_bulk_check_running_ && state == BulkLeakCheckService::State::kIdle) {
+  if (is_check_running_ && state == BulkLeakCheckService::State::kIdle) {
     // When the service transitions from running into idle it has finished a
     // check.
-    is_bulk_check_running_ = false;
+    is_check_running_ = false;
     profile_->GetPrefs()->SetDouble(
         password_manager::prefs::kLastTimePasswordCheckCompleted,
         base::Time::Now().ToDoubleT());
@@ -391,11 +484,12 @@ void PasswordCheckDelegate::OnStateChanged(BulkLeakCheckService::State state) {
 }
 
 void PasswordCheckDelegate::OnCredentialDone(
-    const password_manager::LeakCheckCredential& credential,
+    const LeakCheckCredential& credential,
     password_manager::IsLeaked is_leaked) {
-  if (!is_leaked)
-    return;
-
+  if (is_leaked) {
+    // In case the credential is leaked, iterate over all currently saved
+    // credentials and mark those as compromised that have the same
+    // canonicalized username and password.
     const base::string16 canocalized_username =
         CanonicalizeUsername(credential.username());
     for (const PasswordForm& saved_password :
@@ -407,10 +501,19 @@ void PasswordCheckDelegate::OnCredentialDone(
             .signon_realm = saved_password.signon_realm,
             .username = saved_password.username_value,
             .create_time = base::Time::Now(),
-          .compromise_type = password_manager::CompromiseType::kLeaked,
+            .compromise_type = CompromiseType::kLeaked,
         });
       }
     }
+  }
+
+  // Update the progress in case there is one.
+  if (password_check_progress_)
+    password_check_progress_->OnProcessed(credential);
+
+  // Trigger an update of the check status, considering that the progress has
+  // changed.
+  NotifyPasswordCheckStatusChanged();
 }
 
 const CredentialWithPassword*

chrome/browser/extensions/api/passwords_private/password_check_delegate.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_EXTENSIONS_API_PASSWORDS_PRIVATE_PASSWORD_CHECK_DELEGATE_H_
 
 #include "base/memory/scoped_refptr.h"
+#include "base/memory/weak_ptr.h"
 #include "base/scoped_observer.h"
 #include "chrome/browser/extensions/api/passwords_private/passwords_private_utils.h"
 #include "chrome/common/extensions/api/passwords_private.h"
@@ -26,6 +27,10 @@ class PasswordStore;
 
 namespace extensions {
 
+extern const char kPasswordCheckDataKey[];
+
+class PasswordCheckProgress;
+
 // This class handles the part of the passwordsPrivate extension API that deals
 // with the bulk password check feature.
 class PasswordCheckDelegate
@@ -131,8 +136,12 @@ class PasswordCheckDelegate
   password_manager::BulkLeakCheckServiceAdapter
       bulk_leak_check_service_adapter_;
 
-  // Remembers whether the bulk check is running due to explicit user action.
-  bool is_bulk_check_running_ = false;
+  // Remembers the progress of the ongoing check. Null if no check is currently
+  // running.
+  base::WeakPtr<PasswordCheckProgress> password_check_progress_;
+
+  // Remembers whether a password check is running right now.
+  bool is_check_running_ = false;
 
   // A scoped observer for |saved_passwords_presenter_|.
   ScopedObserver<password_manager::SavedPasswordsPresenter,

chrome/browser/extensions/api/passwords_private/password_check_delegate_unittest.cc

@@ -862,7 +862,7 @@ TEST_F(PasswordCheckDelegateTest, LastTimePasswordCheckCompletedIsSet) {
               Pointee(std::string("5 minutes ago")));
 }
 
-// Checks that a tranistion into the idle state after starting a check results
+// Checks that a transition into the idle state after starting a check results
 // in resetting the kLastTimePasswordCheckCompleted pref to the current time.
 TEST_F(PasswordCheckDelegateTest, LastTimePasswordCheckCompletedReset) {
   delegate().StartPasswordCheck();
@@ -873,4 +873,53 @@ TEST_F(PasswordCheckDelegateTest, LastTimePasswordCheckCompletedReset) {
               Pointee(std::string("Just now")));
 }
 
+// Checks that processing a credential by the leak check updates the progress
+// correctly and raises the expected event.
+TEST_F(PasswordCheckDelegateTest, OnCredentialDoneUpdatesProgress) {
+  const char* const kEventName =
+      api::passwords_private::OnPasswordCheckStatusChanged::kEventName;
+
+  identity_test_env().MakeAccountAvailable(kTestEmail);
+  store().AddLogin(MakeSavedPassword(kExampleCom, kUsername1, kPassword1));
+  store().AddLogin(MakeSavedPassword(kExampleCom, kUsername2, kPassword2));
+  store().AddLogin(MakeSavedPassword(kExampleOrg, kUsername1, kPassword1));
+  store().AddLogin(MakeSavedPassword(kExampleOrg, kUsername2, kPassword2));
+  RunUntilIdle();
+
+  const auto event_iter = event_router_observer().events().find(kEventName);
+  delegate().StartPasswordCheck();
+  EXPECT_EQ(events::PASSWORDS_PRIVATE_ON_PASSWORD_CHECK_STATUS_CHANGED,
+            event_iter->second->histogram_value);
+  auto status = PasswordCheckStatus::FromValue(
+      event_iter->second->event_args->GetList().front());
+  EXPECT_EQ(api::passwords_private::PASSWORD_CHECK_STATE_RUNNING,
+            status->state);
+  EXPECT_EQ(0, *status->already_processed);
+  EXPECT_EQ(4, *status->remaining_in_queue);
+
+  static_cast<BulkLeakCheckDelegateInterface*>(service())->OnFinishedCredential(
+      LeakCheckCredential(base::ASCIIToUTF16(kUsername1),
+                          base::ASCIIToUTF16(kPassword1)),
+      IsLeaked(false));
+
+  status = PasswordCheckStatus::FromValue(
+      event_iter->second->event_args->GetList().front());
+  EXPECT_EQ(api::passwords_private::PASSWORD_CHECK_STATE_RUNNING,
+            status->state);
+  EXPECT_EQ(2, *status->already_processed);
+  EXPECT_EQ(2, *status->remaining_in_queue);
+
+  static_cast<BulkLeakCheckDelegateInterface*>(service())->OnFinishedCredential(
+      LeakCheckCredential(base::ASCIIToUTF16(kUsername2),
+                          base::ASCIIToUTF16(kPassword2)),
+      IsLeaked(false));
+
+  status = PasswordCheckStatus::FromValue(
+      event_iter->second->event_args->GetList().front());
+  EXPECT_EQ(api::passwords_private::PASSWORD_CHECK_STATE_RUNNING,
+            status->state);
+  EXPECT_EQ(4, *status->already_processed);
+  EXPECT_EQ(0, *status->remaining_in_queue);
+}
+
 }  // namespace extensions

components/password_manager/core/browser/ui/bulk_leak_check_service_adapter.cc

@@ -13,34 +13,12 @@
 #include "components/password_manager/core/browser/leak_detection/bulk_leak_check.h"
 #include "components/password_manager/core/browser/leak_detection/encryption_utils.h"
 #include "components/password_manager/core/browser/leak_detection_delegate.h"
+#include "components/password_manager/core/browser/ui/credential_utils.h"
 #include "components/password_manager/core/browser/ui/saved_passwords_presenter.h"
 #include "components/prefs/pref_service.h"
 
 namespace password_manager {
 
-namespace {
-
-using autofill::PasswordForm;
-
-// Simple struct that stores a canonicalized credential. Allows implicit
-// constructon from PasswordForm for convenience.
-struct CanonicalizedCredential {
-  CanonicalizedCredential(const PasswordForm& form)
-      : canonicalized_username(CanonicalizeUsername(form.username_value)),
-        password(form.password_value) {}
-
-  base::string16 canonicalized_username;
-  base::string16 password;
-};
-
-bool operator<(const CanonicalizedCredential& lhs,
-               const CanonicalizedCredential& rhs) {
-  return std::tie(lhs.canonicalized_username, lhs.password) <
-         std::tie(rhs.canonicalized_username, rhs.password);
-}
-
-}  // namespace
-
 BulkLeakCheckServiceAdapter::BulkLeakCheckServiceAdapter(
     SavedPasswordsPresenter* presenter,
     BulkLeakCheckService* service,
@@ -56,7 +34,9 @@ BulkLeakCheckServiceAdapter::~BulkLeakCheckServiceAdapter() {
   presenter_->RemoveObserver(this);
 }
 
-bool BulkLeakCheckServiceAdapter::StartBulkLeakCheck() {
+bool BulkLeakCheckServiceAdapter::StartBulkLeakCheck(
+    const void* key,
+    LeakCheckCredential::Data* data) {
   if (service_->state() == BulkLeakCheckService::State::kRunning)
     return false;
 
@@ -75,6 +55,10 @@ bool BulkLeakCheckServiceAdapter::StartBulkLeakCheck() {
   for (const auto& credential : canonicalized) {
     credentials.emplace_back(credential.canonicalized_username,
                              credential.password);
+    if (key) {
+      DCHECK(data);
+      credentials.back().SetUserData(key, data->Clone());
+    }
   }
 
   service_->CheckUsernamePasswordPairs(std::move(credentials));
@@ -94,7 +78,7 @@ size_t BulkLeakCheckServiceAdapter::GetPendingChecksCount() const {
   return service_->GetPendingChecksCount();
 }
 
-void BulkLeakCheckServiceAdapter::OnEdited(const PasswordForm& form) {
+void BulkLeakCheckServiceAdapter::OnEdited(const autofill::PasswordForm& form) {
   if (CanStartLeakCheck(*prefs_)) {
     // Here no extra canonicalization is needed, as there are no other forms we
     // could de-dupe before we pass it on to the service.

components/password_manager/core/browser/ui/bulk_leak_check_service_adapter.h

@@ -29,9 +29,11 @@ class BulkLeakCheckServiceAdapter : public SavedPasswordsPresenter::Observer {
   // Instructs the adapter to start a check. This is a no-op in case a check is
   // already running. Otherwise, this will obtain the list of saved passwords
   // from |presenter_|, perform de-duplication of username and password pairs
-  // and then feed it to the |service_| for checking.
+  // and then feed it to the |service_| for checking. If |key| is present, it
+  // will append |data->Clone()| to each created LeakCheckCredential.
   // Returns whether new check was started.
-  bool StartBulkLeakCheck();
+  bool StartBulkLeakCheck(const void* key = nullptr,
+                          LeakCheckCredential::Data* data = nullptr);
 
   // This asks |service_| to stop an ongoing check.
   void StopBulkLeakCheck();

components/password_manager/core/browser/ui/bulk_leak_check_service_adapter_unittest.cc

@@ -4,6 +4,7 @@
 
 #include "components/password_manager/core/browser/ui/bulk_leak_check_service_adapter.h"
 
+#include <memory>
 #include <tuple>
 #include <vector>
 
@@ -164,6 +165,27 @@ TEST_F(BulkLeakCheckServiceAdapterTest, StartBulkLeakCheck) {
   EXPECT_THAT(credentials, CredentialsAre(std::cref(expected)));
 }
 
+TEST_F(BulkLeakCheckServiceAdapterTest, StartBulkLeakCheckAttachesData) {
+  constexpr char kKey[] = "key";
+  struct UserData : LeakCheckCredential::Data {
+    std::unique_ptr<Data> Clone() override { return std::make_unique<Data>(); }
+  } data;
+
+  std::vector<PasswordForm> passwords = {
+      MakeSavedPassword(kExampleCom, kUsername1, kPassword1)};
+  store().AddLogin(passwords[0]);
+  RunUntilIdle();
+
+  auto leak_check = std::make_unique<NiceMockBulkLeakCheck>();
+  std::vector<LeakCheckCredential> credentials;
+  EXPECT_CALL(*leak_check, CheckCredentials).WillOnce(MoveArg(&credentials));
+  EXPECT_CALL(factory(), TryCreateBulkLeakCheck)
+      .WillOnce(Return(ByMove(std::move(leak_check))));
+  adapter().StartBulkLeakCheck(kKey, &data);
+
+  EXPECT_NE(nullptr, credentials.at(0).GetUserData(kKey));
+}
+
 // Tests that multiple credentials with effectively the same username are
 // correctly deduped before starting the leak check.
 TEST_F(BulkLeakCheckServiceAdapterTest, StartBulkLeakCheckDedupes) {

components/password_manager/core/browser/ui/credential_utils.h

@@ -13,6 +13,8 @@
 #include "base/strings/string16.h"
 #include "base/template_util.h"
 #include "components/autofill/core/common/password_form.h"
+#include "components/password_manager/core/browser/leak_detection/bulk_leak_check.h"
+#include "components/password_manager/core/browser/leak_detection/encryption_utils.h"
 
 namespace password_manager {
 
@@ -29,6 +31,27 @@ struct CredentialView {
   base::string16 password;
 };
 
+// Simple struct that stores a canonicalized credential. Allows implicit
+// constructon from PasswordForm and LeakCheckCredentail for convenience.
+struct CanonicalizedCredential {
+  CanonicalizedCredential(const autofill::PasswordForm& form)
+      : canonicalized_username(CanonicalizeUsername(form.username_value)),
+        password(form.password_value) {}
+
+  CanonicalizedCredential(const LeakCheckCredential& credential)
+      : canonicalized_username(CanonicalizeUsername(credential.username())),
+        password(credential.password()) {}
+
+  base::string16 canonicalized_username;
+  base::string16 password;
+};
+
+inline bool operator<(const CanonicalizedCredential& lhs,
+                      const CanonicalizedCredential& rhs) {
+  return std::tie(lhs.canonicalized_username, lhs.password) <
+         std::tie(rhs.canonicalized_username, rhs.password);
+}
+
 // Transparent comparator that can compare various types like CredentialView or
 // CredentialsWithPasswords.
 struct PasswordCredentialLess {