Safety check: added the retrieval of the actual number of compromised passwords

Bug: 1015841
Change-Id: I5c32614984a2398d83001d5b4e48c7689f7991d4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2087585
Commit-Queue: Andrey Zaytsev <andzaytsev@google.com>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#749664}

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

@@ -10,6 +10,7 @@
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
+#include "chrome/browser/extensions/api/passwords_private/passwords_private_delegate_factory.h"
 #include "chrome/browser/password_manager/bulk_leak_check_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/url_constants.h"
@@ -81,6 +82,7 @@ void SafetyCheckHandler::PerformSafetyCheck() {
   if (!version_updater_) {
     version_updater_.reset(VersionUpdater::Create(web_ui()->GetWebContents()));
   }
+  DCHECK(version_updater_);
   CheckUpdates();
 
   CheckSafeBrowsing();
@@ -90,6 +92,12 @@ void SafetyCheckHandler::PerformSafetyCheck() {
         Profile::FromWebUI(web_ui()));
   }
   DCHECK(leak_service_);
+  if (!passwords_delegate_) {
+    passwords_delegate_ =
+        extensions::PasswordsPrivateDelegateFactory::GetForBrowserContext(
+            Profile::FromWebUI(web_ui()), true);
+  }
+  DCHECK(passwords_delegate_);
   CheckPasswords();
 
   if (!extension_prefs_) {
@@ -109,10 +117,12 @@ void SafetyCheckHandler::PerformSafetyCheck() {
 SafetyCheckHandler::SafetyCheckHandler(
     std::unique_ptr<VersionUpdater> version_updater,
     password_manager::BulkLeakCheckService* leak_service,
+    extensions::PasswordsPrivateDelegate* passwords_delegate,
     extensions::ExtensionPrefs* extension_prefs,
     extensions::ExtensionServiceInterface* extension_service)
     : version_updater_(std::move(version_updater)),
       leak_service_(leak_service),
+      passwords_delegate_(passwords_delegate),
       extension_prefs_(extension_prefs),
       extension_service_(extension_service) {}
 
@@ -391,13 +401,20 @@ void SafetyCheckHandler::OnStateChanged(
   using password_manager::BulkLeakCheckService;
   switch (state) {
     case BulkLeakCheckService::State::kIdle:
-    case BulkLeakCheckService::State::kCanceled:
-      // 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).
+    case BulkLeakCheckService::State::kCanceled: {
+      size_t num_compromised =
+          passwords_delegate_->GetCompromisedCredentials().size();
+      if (num_compromised == 0) {
+        OnPasswordsCheckResult(PasswordsStatus::kSafe, 0);
+      } else {
+        OnPasswordsCheckResult(PasswordsStatus::kCompromisedExist,
+                               num_compromised);
+      }
       break;
+    }
     case BulkLeakCheckService::State::kRunning:
       OnPasswordsCheckResult(PasswordsStatus::kChecking, 0);
+      // Non-terminal state, so nothing else needs to be done.
       return;
     case BulkLeakCheckService::State::kSignedOut:
       OnPasswordsCheckResult(PasswordsStatus::kSignedOut, 0);

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

@@ -15,6 +15,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/scoped_observer.h"
 #include "base/util/type_safety/strong_alias.h"
+#include "chrome/browser/extensions/api/passwords_private/passwords_private_delegate.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/ui/webui/help/version_updater.h"
 #include "chrome/browser/ui/webui/settings/settings_page_ui_handler.h"
@@ -81,6 +82,7 @@ class SafetyCheckHandler
  protected:
   SafetyCheckHandler(std::unique_ptr<VersionUpdater> version_updater,
                      password_manager::BulkLeakCheckService* leak_service,
+                     extensions::PasswordsPrivateDelegate* passwords_delegate,
                      extensions::ExtensionPrefs* extension_prefs,
                      extensions::ExtensionServiceInterface* extension_service);
 
@@ -151,6 +153,7 @@ class SafetyCheckHandler
 
   std::unique_ptr<VersionUpdater> version_updater_;
   password_manager::BulkLeakCheckService* leak_service_ = nullptr;
+  extensions::PasswordsPrivateDelegate* passwords_delegate_ = nullptr;
   extensions::ExtensionPrefs* extension_prefs_ = nullptr;
   extensions::ExtensionServiceInterface* extension_service_ = nullptr;
   ScopedObserver<password_manager::BulkLeakCheckService,

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

@@ -9,13 +9,17 @@
 
 #include "base/bind.h"
 #include "base/optional.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/metrics/user_action_tester.h"
 #include "base/util/type_safety/strong_alias.h"
 #include "build/build_config.h"
+#include "chrome/browser/extensions/api/passwords_private/passwords_private_delegate.h"
+#include "chrome/browser/extensions/api/passwords_private/test_passwords_private_delegate.h"
 #include "chrome/browser/extensions/test_extension_service.h"
 #include "chrome/browser/ui/webui/help/test_version_updater.h"
+#include "chrome/common/extensions/api/passwords_private.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "components/crx_file/id_util.h"
 #include "components/password_manager/core/browser/bulk_leak_check_service.h"
@@ -52,14 +56,36 @@ class TestingSafetyCheckHandler : public SafetyCheckHandler {
   TestingSafetyCheckHandler(
       std::unique_ptr<VersionUpdater> version_updater,
       password_manager::BulkLeakCheckService* leak_service,
+      extensions::PasswordsPrivateDelegate* passwords_delegate,
       extensions::ExtensionPrefs* extension_prefs,
       extensions::ExtensionServiceInterface* extension_service)
       : SafetyCheckHandler(std::move(version_updater),
                            leak_service,
+                           passwords_delegate,
                            extension_prefs,
                            extension_service) {}
 };
 
+class TestPasswordsDelegate : public extensions::TestPasswordsPrivateDelegate {
+ public:
+  void SetNumCompromisedCredentials(int compromised_password_count) {
+    compromised_password_count_ = compromised_password_count;
+  }
+
+  std::vector<extensions::api::passwords_private::CompromisedCredential>
+  GetCompromisedCredentials() override {
+    std::vector<extensions::api::passwords_private::CompromisedCredential>
+        compromised(compromised_password_count_);
+    for (int i = 0; i < compromised_password_count_; ++i) {
+      compromised[i].username = "test" + base::NumberToString(i);
+    }
+    return compromised;
+  }
+
+ private:
+  int compromised_password_count_ = 0;
+};
+
 class TestSafetyCheckExtensionService : public TestExtensionService {
  public:
   void AddExtensionState(const std::string& extension_id,
@@ -118,14 +144,16 @@ class SafetyCheckHandlerTest : public ChromeRenderViewHostTestHarness {
  protected:
   TestVersionUpdater* version_updater_ = nullptr;
   std::unique_ptr<password_manager::BulkLeakCheckService> test_leak_service_;
+  TestPasswordsDelegate test_passwords_delegate_;
   extensions::ExtensionPrefs* test_extension_prefs_ = nullptr;
   TestSafetyCheckExtensionService test_extension_service_;
   content::TestWebUI test_web_ui_;
   std::unique_ptr<TestingSafetyCheckHandler> safety_check_;
 
  private:
-  // Replaces any instances of browser name (e.g. Google Chrome, Chromium, etc)
-  // with "browser" to make sure tests work both on Chromium and Google Chrome.
+  // Replaces any instances of browser name (e.g. Google Chrome, Chromium,
+  // etc) with "browser" to make sure tests work both on Chromium and
+  // Google Chrome.
   void ReplaceBrowserName(base::string16* s);
 };
 
@@ -143,7 +171,8 @@ void SafetyCheckHandlerTest::SetUp() {
   test_extension_prefs_ = extensions::ExtensionPrefs::Get(profile());
   safety_check_ = std::make_unique<TestingSafetyCheckHandler>(
       std::move(version_updater), test_leak_service_.get(),
-      test_extension_prefs_, &test_extension_service_);
+      &test_passwords_delegate_, test_extension_prefs_,
+      &test_extension_service_);
   test_web_ui_.ClearTrackedCalls();
   safety_check_->set_web_ui(&test_web_ui_);
   safety_check_->AllowJavascript();
@@ -186,9 +215,9 @@ void SafetyCheckHandlerTest::VerifyDisplayString(
   base::string16 display;
   ASSERT_TRUE(event->GetString("displayString", &display));
   ReplaceBrowserName(&display);
-  // Need to also replace any instances of Chrome and Chromium in the expected
-  // string due to an edge case on ChromeOS, where a device name is "Chrome",
-  // which gets replaced in the display string.
+  // Need to also replace any instances of Chrome and Chromium in the
+  // expected string due to an edge case on ChromeOS, where a device name
+  // is "Chrome", which gets replaced in the display string.
   base::string16 expected_replaced = expected;
   ReplaceBrowserName(&expected_replaced);
   EXPECT_EQ(expected_replaced, display);
@@ -317,7 +346,8 @@ TEST_F(SafetyCheckHandlerTest, CheckUpdates_Failed) {
   VerifyDisplayString(
       event,
       "Browser didn't update, something went wrong. <a target=\"_blank\" "
-      "href=\"https://support.google.com/chrome/answer/111996\">Fix Browser "
+      "href=\"https://support.google.com/chrome/answer/111996\">Fix "
+      "Browser "
       "update problems and failed updates.</a>");
 }
 
@@ -408,8 +438,8 @@ TEST_F(SafetyCheckHandlerTest, CheckPasswords_ObserverRemovedAfterError) {
   VerifyDisplayString(event2,
                       "Browser can't check your passwords. Try checking your "
                       "internet connection.");
-  // Another error, but since the previous state is terminal, the handler should
-  // no longer be observing the BulkLeakCheckService state.
+  // Another error, but since the previous state is terminal, the handler
+  // should no longer be observing the BulkLeakCheckService state.
   test_leak_service_->set_state_and_notify(
       password_manager::BulkLeakCheckService::State::kServiceError);
   const base::DictionaryValue* event3 =
@@ -465,16 +495,59 @@ TEST_F(SafetyCheckHandlerTest, CheckPasswords_StartedTwice) {
           kPasswords,
           static_cast<int>(SafetyCheckHandler::PasswordsStatus::kChecking));
   ASSERT_TRUE(event);
-  // Second, an "offline" state.
+  // Then, a network error.
   test_leak_service_->set_state_and_notify(
-      password_manager::BulkLeakCheckService::State::kServiceError);
+      password_manager::BulkLeakCheckService::State::kNetworkError);
   const base::DictionaryValue* event2 =
       GetSafetyCheckStatusChangedWithDataIfExists(
           kPasswords,
-          static_cast<int>(SafetyCheckHandler::PasswordsStatus::kError));
-  ASSERT_TRUE(event2);
+          static_cast<int>(SafetyCheckHandler::PasswordsStatus::kOffline));
+  EXPECT_TRUE(event2);
   VerifyDisplayString(event2,
-                      "Browser can't check your passwords. Try again later.");
+                      "Browser can't check your passwords. Try checking your "
+                      "internet connection.");
+}
+
+TEST_F(SafetyCheckHandlerTest, CheckPasswords_Safe) {
+  safety_check_->PerformSafetyCheck();
+  // First, a "running" change of state.
+  test_leak_service_->set_state_and_notify(
+      password_manager::BulkLeakCheckService::State::kRunning);
+  EXPECT_TRUE(GetSafetyCheckStatusChangedWithDataIfExists(
+      kPasswords,
+      static_cast<int>(SafetyCheckHandler::PasswordsStatus::kChecking)));
+  // Second, a "safe" state.
+  test_leak_service_->set_state_and_notify(
+      password_manager::BulkLeakCheckService::State::kIdle);
+  const base::DictionaryValue* event =
+      GetSafetyCheckStatusChangedWithDataIfExists(
+          kPasswords,
+          static_cast<int>(SafetyCheckHandler::PasswordsStatus::kSafe));
+  EXPECT_TRUE(event);
+  VerifyDisplayString(event, "No compromised passwords found");
+}
+
+TEST_F(SafetyCheckHandlerTest, CheckPasswords_CompromisedExist) {
+  constexpr int kCompromised = 7;
+  test_passwords_delegate_.SetNumCompromisedCredentials(kCompromised);
+  safety_check_->PerformSafetyCheck();
+  // First, a "running" change of state.
+  test_leak_service_->set_state_and_notify(
+      password_manager::BulkLeakCheckService::State::kRunning);
+  EXPECT_TRUE(GetSafetyCheckStatusChangedWithDataIfExists(
+      kPasswords,
+      static_cast<int>(SafetyCheckHandler::PasswordsStatus::kChecking)));
+  // Compromised passwords found state.
+  test_leak_service_->set_state_and_notify(
+      password_manager::BulkLeakCheckService::State::kIdle);
+  const base::DictionaryValue* event2 =
+      GetSafetyCheckStatusChangedWithDataIfExists(
+          kPasswords,
+          static_cast<int>(
+              SafetyCheckHandler::PasswordsStatus::kCompromisedExist));
+  ASSERT_TRUE(event2);
+  VerifyDisplayString(
+      event2, base::NumberToString(kCompromised) + " compromised passwords");
 }
 
 TEST_F(SafetyCheckHandlerTest, CheckExtensions_NoExtensions) {
@@ -564,9 +637,9 @@ TEST_F(SafetyCheckHandlerTest, CheckExtensions_BlocklistedReenabledAllByAdmin) {
       GetSafetyCheckStatusChangedWithDataIfExists(
           kExtensions, static_cast<int>(SafetyCheckHandler::ExtensionsStatus::
                                             kBlocklistedReenabledAllByAdmin));
-  VerifyDisplayString(
-      event,
-      "Your administrator turned 1 potentially harmful extension back on");
+  VerifyDisplayString(event,
+                      "Your administrator turned 1 potentially harmful "
+                      "extension back on");
 }
 
 TEST_F(SafetyCheckHandlerTest, CheckExtensions_BlocklistedReenabledSomeByUser) {
@@ -598,9 +671,9 @@ TEST_F(SafetyCheckHandlerTest, CheckExtensions_BlocklistedReenabledSomeByUser) {
           kExtensions, static_cast<int>(SafetyCheckHandler::ExtensionsStatus::
                                             kBlocklistedReenabledSomeByUser));
   EXPECT_TRUE(event);
-  VerifyDisplayString(
-      event,
-      "You turned 1 potentially harmful extension back on. Your administrator "
+  VerifyDisplayString(event,
+                      "You turned 1 potentially harmful extension back "
+                      "on. Your administrator "
                       "turned 1 potentially harmful extension back on.");
 }