Safety check page: implemented an Observer for the password leak check API

Also implemented all of the states for the password check (matches the JS frontend ones) and added tests for the Observer logic.

Bug: 1015841
Change-Id: Ie2485cdb406ea0ac44b79d4f3456bf0e7644bbe6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2076477
Commit-Queue: Andrey Zaytsev <andzaytsev@google.com>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#746741}

chrome/browser/ui/BUILD.gn

@@ -1365,7 +1365,6 @@ jumbo_static_library("ui") {
       "webui/settings/reset_settings_handler.h",
       "webui/settings/safety_check_handler.cc",
       "webui/settings/safety_check_handler.h",
-      "webui/settings/safety_check_handler_observer.h",
       "webui/settings/search_engines_handler.cc",
       "webui/settings/search_engines_handler.h",
       "webui/settings/settings_clear_browsing_data_handler.cc",

chrome/browser/ui/webui/settings/safety_check_handler.cc

@@ -5,18 +5,19 @@
 #include "chrome/browser/ui/webui/settings/safety_check_handler.h"
 
 #include "base/bind.h"
+#include "chrome/browser/password_manager/bulk_leak_check_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/ui/webui/settings/safety_check_handler_observer.h"
 #include "components/prefs/pref_service.h"
 #include "components/safe_browsing/core/common/safe_browsing_prefs.h"
 
 namespace {
 
 // Constants for communication with JS.
-static constexpr char kStatusChanged[] = "safety-check-status-changed";
-static constexpr char kPerformSafetyCheck[] = "performSafetyCheck";
-static constexpr char kSafetyCheckComponent[] = "safetyCheckComponent";
-static constexpr char kNewState[] = "newState";
+constexpr char kStatusChanged[] = "safety-check-status-changed";
+constexpr char kPerformSafetyCheck[] = "performSafetyCheck";
+constexpr char kSafetyCheckComponent[] = "safetyCheckComponent";
+constexpr char kNewState[] = "newState";
+constexpr char kPasswordsCompromised[] = "passwordsCompromised";
 
 // Converts the VersionUpdater::Status to the UpdateStatus enum to be passed
 // to the safety check frontend. Note: if the VersionUpdater::Status gets
@@ -47,8 +48,7 @@ SafetyCheckHandler::UpdateStatus ConvertToUpdateStatus(
 }
 }  // namespace
 
-SafetyCheckHandler::SafetyCheckHandler()
-    : SafetyCheckHandler(nullptr, nullptr) {}
+SafetyCheckHandler::SafetyCheckHandler() = default;
 
 SafetyCheckHandler::~SafetyCheckHandler() = default;
 
@@ -59,12 +59,19 @@ void SafetyCheckHandler::PerformSafetyCheck() {
   }
   CheckUpdates();
   CheckSafeBrowsing();
+  if (!leak_service_) {
+    leak_service_ = BulkLeakCheckServiceFactory::GetForProfile(
+        Profile::FromWebUI(web_ui()));
+  }
+  DCHECK(leak_service_);
+  CheckPasswords();
 }
 
 SafetyCheckHandler::SafetyCheckHandler(
     std::unique_ptr<VersionUpdater> version_updater,
-    SafetyCheckHandlerObserver* observer)
-    : version_updater_(std::move(version_updater)), observer_(observer) {}
+    password_manager::BulkLeakCheckService* leak_service)
+    : version_updater_(std::move(version_updater)),
+      leak_service_(leak_service) {}
 
 void SafetyCheckHandler::HandlePerformSafetyCheck(
     const base::ListValue* /*args*/) {
@@ -72,9 +79,6 @@ void SafetyCheckHandler::HandlePerformSafetyCheck(
 }
 
 void SafetyCheckHandler::CheckUpdates() {
-  if (observer_) {
-    observer_->OnUpdateCheckStart();
-  }
   version_updater_->CheckForUpdate(
       base::Bind(&SafetyCheckHandler::OnUpdateCheckResult,
                  base::Unretained(this)),
@@ -82,9 +86,6 @@ void SafetyCheckHandler::CheckUpdates() {
 }
 
 void SafetyCheckHandler::CheckSafeBrowsing() {
-  if (observer_) {
-    observer_->OnSafeBrowsingCheckStart();
-  }
   PrefService* pref_service = Profile::FromWebUI(web_ui())->GetPrefs();
   const PrefService::Preference* pref =
       pref_service->FindPreference(prefs::kSafeBrowsingEnabled);
@@ -101,39 +102,100 @@ void SafetyCheckHandler::CheckSafeBrowsing() {
   OnSafeBrowsingCheckResult(status);
 }
 
-void SafetyCheckHandler::OnUpdateCheckResult(
-    VersionUpdater::Status status,
-    int /*progress*/,
-    bool /*rollback*/,
-    const std::string& /*version*/,
-    int64_t /*update_size*/,
-    const base::string16& /*message*/) {
+void SafetyCheckHandler::CheckPasswords() {
+  // Remove |this| as an existing observer for BulkLeakCheck if it is
+  // registered. This takes care of an edge case when safety check starts twice
+  // on the same page. Normally this should not happen, but if it does, the
+  // browser should not crash.
+  observed_leak_check_.RemoveAll();
+  observed_leak_check_.Add(leak_service_);
+  // TODO(crbug.com/1015841): Implement starting a leak check if one is not
+  // running already once the API for it becomes available (see
+  // crrev.com/c/2072742 and follow up CLs).
+}
+
+void SafetyCheckHandler::OnUpdateCheckResult(VersionUpdater::Status status,
+                                             int progress,
+                                             bool rollback,
+                                             const std::string& version,
+                                             int64_t update_size,
+                                             const base::string16& message) {
   UpdateStatus update_status = ConvertToUpdateStatus(status);
-  if (observer_) {
-    observer_->OnUpdateCheckResult(update_status);
-  }
-  auto event = std::make_unique<base::DictionaryValue>();
-  event->SetInteger(kSafetyCheckComponent,
-                    static_cast<int>(SafetyCheckComponent::kUpdates));
-  event->SetInteger(kNewState, static_cast<int>(update_status));
-  FireWebUIListener(kStatusChanged, *event);
+  base::DictionaryValue event;
+  event.SetIntKey(kSafetyCheckComponent,
+                  static_cast<int>(SafetyCheckComponent::kUpdates));
+  event.SetIntKey(kNewState, static_cast<int>(update_status));
+  FireWebUIListener(kStatusChanged, event);
 }
 
 void SafetyCheckHandler::OnSafeBrowsingCheckResult(
     SafetyCheckHandler::SafeBrowsingStatus status) {
-  if (observer_) {
-    observer_->OnSafeBrowsingCheckResult(status);
+  base::DictionaryValue event;
+  event.SetIntKey(kSafetyCheckComponent,
+                  static_cast<int>(SafetyCheckComponent::kSafeBrowsing));
+  event.SetIntKey(kNewState, static_cast<int>(status));
+  FireWebUIListener(kStatusChanged, event);
+}
+
+void SafetyCheckHandler::OnPasswordsCheckResult(PasswordsStatus status,
+                                                int num_compromised) {
+  base::DictionaryValue event;
+  event.SetIntKey(kSafetyCheckComponent,
+                  static_cast<int>(SafetyCheckComponent::kPasswords));
+  event.SetIntKey(kNewState, static_cast<int>(status));
+  if (status == PasswordsStatus::kCompromisedExist) {
+    event.SetIntKey(kPasswordsCompromised, num_compromised);
   }
-  auto event = std::make_unique<base::DictionaryValue>();
-  event->SetInteger(kSafetyCheckComponent,
-                    static_cast<int>(SafetyCheckComponent::kSafeBrowsing));
-  event->SetInteger(kNewState, static_cast<int>(status));
-  FireWebUIListener(kStatusChanged, *event);
+  FireWebUIListener(kStatusChanged, event);
+}
+
+void SafetyCheckHandler::OnStateChanged(
+    password_manager::BulkLeakCheckService::State state,
+    size_t pending_credentials) {
+  using password_manager::BulkLeakCheckService;
+  switch (state) {
+    case BulkLeakCheckService::State::kIdle:
+      // TODO(crbug.com/1015841): Implement retrieving the number
+      // of leaked passwords (if any) once PasswordsPrivateDelegate provides an
+      // API for that (see crrev.com/c/2072742).
+      break;
+    case BulkLeakCheckService::State::kRunning:
+      OnPasswordsCheckResult(PasswordsStatus::kChecking, 0);
+      return;
+    case BulkLeakCheckService::State::kSignedOut:
+      OnPasswordsCheckResult(PasswordsStatus::kSignedOut, 0);
+      break;
+    case BulkLeakCheckService::State::kNetworkError:
+      OnPasswordsCheckResult(PasswordsStatus::kOffline, 0);
+      break;
+    case BulkLeakCheckService::State::kTokenRequestFailure:
+    case BulkLeakCheckService::State::kHashingFailure:
+    case BulkLeakCheckService::State::kServiceError:
+      OnPasswordsCheckResult(PasswordsStatus::kError, 0);
+      break;
+  }
+  // TODO(crbug.com/1015841): implement detecting the following states if it is
+  // possible: kNoPasswords, kQuotaLimit, and kTooManyPasswords.
+
+  // Stop observing the leak service in all terminal states.
+  observed_leak_check_.Remove(leak_service_);
+}
+
+void SafetyCheckHandler::OnLeakFound(
+    const password_manager::LeakCheckCredential& credential) {
+  // Do nothing because we only want to know the total number of compromised
+  // credentials at the end of the bulk leak check.
 }
 
 void SafetyCheckHandler::OnJavascriptAllowed() {}
 
-void SafetyCheckHandler::OnJavascriptDisallowed() {}
+void SafetyCheckHandler::OnJavascriptDisallowed() {
+  // Remove |this| as an observer for BulkLeakCheck. This takes care of an edge
+  // case when the page is reloaded while the password check is in progress and
+  // another safety check is started. Otherwise |observed_leak_check_|
+  // automatically calls RemoveAll() on destruction.
+  observed_leak_check_.RemoveAll();
+}
 
 void SafetyCheckHandler::RegisterMessages() {
   web_ui()->RegisterMessageCallback(

chrome/browser/ui/webui/settings/safety_check_handler.h

@@ -13,15 +13,17 @@
 #include "base/callback_forward.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "base/scoped_observer.h"
 #include "chrome/browser/ui/webui/help/version_updater.h"
 #include "chrome/browser/ui/webui/settings/settings_page_ui_handler.h"
-
-class SafetyCheckHandlerObserver;
+#include "components/password_manager/core/browser/bulk_leak_check_service.h"
 
 // Settings page UI handler that checks four areas of browser safety:
 // browser updates, password leaks, malicious extensions, and unwanted
 // software.
-class SafetyCheckHandler : public settings::SettingsPageUIHandler {
+class SafetyCheckHandler
+    : public settings::SettingsPageUIHandler,
+      public password_manager::BulkLeakCheckService::Observer {
  public:
   // Used in communication with the frontend to indicate which component the
   // communicated status update applies to.
@@ -31,6 +33,9 @@ class SafetyCheckHandler : public settings::SettingsPageUIHandler {
     kSafeBrowsing,
     kExtensions,
   };
+  // The following enums represent the state of each component of the safety
+  // check and should be kept in sync with the JS frontend
+  // (safety_check_browser_proxy.js).
   enum class UpdateStatus {
     kChecking,
     kUpdated,
@@ -47,6 +52,17 @@ class SafetyCheckHandler : public settings::SettingsPageUIHandler {
     kDisabledByAdmin,
     kDisabledByExtension,
   };
+  enum class PasswordsStatus {
+    kChecking,
+    kSafe,
+    kCompromisedExist,
+    kOffline,
+    kNoPasswords,
+    kSignedOut,
+    kQuotaLimit,
+    kTooManyPasswords,
+    kError,
+  };
 
   SafetyCheckHandler();
   ~SafetyCheckHandler() override;
@@ -58,14 +74,14 @@ class SafetyCheckHandler : public settings::SettingsPageUIHandler {
 
  protected:
   SafetyCheckHandler(std::unique_ptr<VersionUpdater> version_updater,
-                     SafetyCheckHandlerObserver* observer);
+                     password_manager::BulkLeakCheckService* leak_service);
 
  private:
   // Handles triggering the safety check from the frontend (by user pressing a
   // button).
   void HandlePerformSafetyCheck(const base::ListValue* args);
 
-  // Triggers an update check and invokes the provided callback once results
+  // Triggers an update check and invokes OnUpdateCheckResult once results
   // are available.
   void CheckUpdates();
 
@@ -73,6 +89,10 @@ class SafetyCheckHandler : public settings::SettingsPageUIHandler {
   // OnSafeBrowsingCheckResult with results.
   void CheckSafeBrowsing();
 
+  // Triggers a bulk password leak check and invokes OnPasswordsCheckResult once
+  // results are available.
+  void CheckPasswords();
+
   // Callbacks that get triggered when each check completes.
   void OnUpdateCheckResult(VersionUpdater::Status status,
                            int progress,
@@ -83,6 +103,14 @@ class SafetyCheckHandler : public settings::SettingsPageUIHandler {
 
   void OnSafeBrowsingCheckResult(SafeBrowsingStatus status);
 
+  void OnPasswordsCheckResult(PasswordsStatus status, int num_compromised);
+
+  // BulkLeakCheckService::Observer implementation.
+  void OnStateChanged(password_manager::BulkLeakCheckService::State state,
+                      size_t pending_credentials) override;
+  void OnLeakFound(
+      const password_manager::LeakCheckCredential& credential) override;
+
   // SettingsPageUIHandler implementation.
   void OnJavascriptAllowed() override;
   void OnJavascriptDisallowed() override;
@@ -91,7 +119,10 @@ class SafetyCheckHandler : public settings::SettingsPageUIHandler {
   void RegisterMessages() override;
 
   std::unique_ptr<VersionUpdater> version_updater_;
-  SafetyCheckHandlerObserver* const observer_;
+  password_manager::BulkLeakCheckService* leak_service_ = nullptr;
+  ScopedObserver<password_manager::BulkLeakCheckService,
+                 password_manager::BulkLeakCheckService::Observer>
+      observed_leak_check_{this};
 };
 
 #endif  // CHROME_BROWSER_UI_WEBUI_SETTINGS_SAFETY_CHECK_HANDLER_H_

chrome/browser/ui/webui/settings/safety_check_handler_observer.h

-// Copyright 2020 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.
-
-#ifndef CHROME_BROWSER_UI_WEBUI_SETTINGS_SAFETY_CHECK_HANDLER_OBSERVER_H_
-#define CHROME_BROWSER_UI_WEBUI_SETTINGS_SAFETY_CHECK_HANDLER_OBSERVER_H_
-
-#include "chrome/browser/ui/webui/help/version_updater.h"
-#include "chrome/browser/ui/webui/settings/safety_check_handler.h"
-
-// Observer for SafetyCheckHandler events. Currently only used for testing.
-class SafetyCheckHandlerObserver {
- public:
-  SafetyCheckHandlerObserver() = default;
-  SafetyCheckHandlerObserver(const SafetyCheckHandlerObserver&) = delete;
-  SafetyCheckHandlerObserver& operator=(const SafetyCheckHandlerObserver&) =
-      delete;
-  virtual ~SafetyCheckHandlerObserver() = default;
-
-  virtual void OnUpdateCheckStart() = 0;
-  virtual void OnUpdateCheckResult(SafetyCheckHandler::UpdateStatus status) = 0;
-  virtual void OnSafeBrowsingCheckStart() = 0;
-  virtual void OnSafeBrowsingCheckResult(
-      SafetyCheckHandler::SafeBrowsingStatus status) = 0;
-};
-
-#endif  // CHROME_BROWSER_UI_WEBUI_SETTINGS_SAFETY_CHECK_HANDLER_OBSERVER_H_

components/password_manager/core/browser/bulk_leak_check_service.h

@@ -92,6 +92,11 @@ class BulkLeakCheckService : public KeyedService,
   void set_leak_factory(std::unique_ptr<LeakDetectionCheckFactory> factory) {
     leak_check_factory_ = std::move(factory);
   }
+
+  void set_state_and_notify(State state) {
+    state_ = state;
+    NotifyStateChanged();
+  }
 #endif  // defined(UNIT_TEST)
 
  private: