Introduce leak history flow in leak detection delegate

Saves credentials in the leaked credentials table when the leak
detection confirms that they are leaked, removes them otherwise.
It is currently hidden behind a feature.

Bug: 1005746
Change-Id: I23277b3cd395a9464e0dd0f4f1e4976b901d2a65
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1826998
Commit-Queue: Rafał Godlewski <rgod@google.com>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#701501}

components/password_manager/core/browser/leak_detection_delegate.cc

@@ -11,9 +11,11 @@
 #include "components/password_manager/core/browser/leak_detection/leak_detection_check_factory_impl.h"
 #include "components/password_manager/core/browser/leak_detection_delegate_helper.h"
 #include "components/password_manager/core/browser/leak_detection_dialog_utils.h"
+#include "components/password_manager/core/browser/leaked_credentials_table.h"
 #include "components/password_manager/core/browser/password_feature_manager.h"
 #include "components/password_manager/core/browser/password_manager_client.h"
 #include "components/password_manager/core/browser/password_manager_util.h"
+#include "components/password_manager/core/common/password_manager_features.h"
 #include "components/password_manager/core/common/password_manager_pref_names.h"
 #include "components/prefs/pref_service.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
@@ -58,10 +60,24 @@ void LeakDetectionDelegate::OnLeakDetectionDone(bool is_leaked,
     BrowserSavePasswordProgressLogger logger(client_->GetLogManager());
     logger.LogBoolean(Logger::STRING_LEAK_DETECTION_FINISHED, is_leaked);
   }
+
+  password_manager::PasswordStore* password_store =
+      client_->GetProfilePasswordStore();
+  if (base::FeatureList::IsEnabled(password_manager::features::kLeakHistory)) {
+    if (is_leaked) {
+      password_store->AddLeakedCredentials(
+          LeakedCredentials(url, username, base::Time::Now()));
+    } else {
+      // If the credentials are not saved as leaked in the database, this call
+      // will just get ignored.
+      password_store->RemoveLeakedCredentials(url, username);
+    }
+  }
+
   if (is_leaked) {
     if (!client_->GetPasswordFeatureManager()
              ->ShouldCheckReuseOnLeakDetection()) {
-      // If we should not check leaked password reuse, then the
+      // If leaked password reuse should not be checked, then the
       // |CredentialLeakType| needed to show the correct notification is already
       // determined.
       OnShowLeakDetectionNotification(
@@ -73,9 +89,8 @@ void LeakDetectionDelegate::OnLeakDetectionDone(bool is_leaked,
       helper_ = std::make_unique<LeakDetectionDelegateHelper>(base::BindOnce(
           &LeakDetectionDelegate::OnShowLeakDetectionNotification,
           base::Unretained(this)));
-      helper_->GetCredentialLeakType(client_->GetProfilePasswordStore(),
-                                     std::move(url), std::move(username),
-                                     std::move(password));
+      helper_->GetCredentialLeakType(password_store, std::move(url),
+                                     std::move(username), std::move(password));
     }
   }
 }

components/password_manager/core/browser/leak_detection_delegate_unittest.cc

@@ -8,8 +8,12 @@
 
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/metrics/histogram_tester.h"
+#include "base/test/scoped_feature_list.h"
+#include "base/test/task_environment.h"
 #include "components/password_manager/core/browser/leak_detection/leak_detection_check.h"
+#include "components/password_manager/core/browser/mock_password_store.h"
 #include "components/password_manager/core/browser/stub_password_manager_client.h"
+#include "components/password_manager/core/common/password_manager_features.h"
 #include "components/password_manager/core/common/password_manager_pref_names.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
@@ -45,6 +49,7 @@ class MockPasswordManagerClient : public StubPasswordManagerClient {
   MOCK_CONST_METHOD0(GetPrefs, PrefService*());
   MOCK_METHOD2(NotifyUserCredentialsWereLeaked,
                void(password_manager::CredentialLeakType, const GURL&));
+  MOCK_CONST_METHOD0(GetProfilePasswordStore, PasswordStore*());
 };
 
 class MockLeakDetectionCheck : public LeakDetectionCheck {
@@ -65,29 +70,37 @@ class MockLeakDetectionCheckFactory : public LeakDetectionCheckFactory {
 
 class LeakDetectionDelegateTest : public testing::Test {
  public:
-  LeakDetectionDelegateTest() : delegate_(&client_) {
+  LeakDetectionDelegateTest() {
+    mock_store_->Init(syncer::SyncableService::StartSyncFlare(), nullptr);
     auto mock_factory =
         std::make_unique<testing::StrictMock<MockLeakDetectionCheckFactory>>();
     mock_factory_ = mock_factory.get();
     delegate_.set_leak_factory(std::move(mock_factory));
-    prefs_ = std::make_unique<TestingPrefServiceSimple>();
-    prefs_->registry()->RegisterBooleanPref(
+    pref_service_->registry()->RegisterBooleanPref(
         password_manager::prefs::kPasswordLeakDetectionEnabled, true);
-    ON_CALL(client_, GetPrefs()).WillByDefault(Return(prefs_.get()));
+    ON_CALL(client_, GetPrefs()).WillByDefault(Return(pref_service()));
   }
-  ~LeakDetectionDelegateTest() override = default;
+
+  ~LeakDetectionDelegateTest() override { mock_store_->ShutdownOnUIThread(); }
 
   MockPasswordManagerClient& client() { return client_; }
   MockLeakDetectionCheckFactory& factory() { return *mock_factory_; }
   LeakDetectionDelegate& delegate() { return delegate_; }
+  MockPasswordStore* store() { return mock_store_.get(); }
+  PrefService* pref_service() { return pref_service_.get(); }
 
- protected:
-  std::unique_ptr<TestingPrefServiceSimple> prefs_;
+  void WaitForPasswordStore() { task_environment_.RunUntilIdle(); }
 
  private:
+  base::test::TaskEnvironment task_environment_{
+      base::test::TaskEnvironment::TimeSource::MOCK_TIME};
   testing::NiceMock<MockPasswordManagerClient> client_;
   MockLeakDetectionCheckFactory* mock_factory_ = nullptr;
-  LeakDetectionDelegate delegate_;
+  scoped_refptr<MockPasswordStore> mock_store_ =
+      base::MakeRefCounted<testing::StrictMock<MockPasswordStore>>();
+  LeakDetectionDelegate delegate_{&client_};
+  std::unique_ptr<TestingPrefServiceSimple> pref_service_ =
+      std::make_unique<TestingPrefServiceSimple>();
 };
 
 TEST_F(LeakDetectionDelegateTest, InIncognito) {
@@ -101,8 +114,8 @@ TEST_F(LeakDetectionDelegateTest, InIncognito) {
 
 TEST_F(LeakDetectionDelegateTest, PrefIsFalse) {
   const autofill::PasswordForm form = CreateTestForm();
-  prefs_->SetBoolean(password_manager::prefs::kPasswordLeakDetectionEnabled,
-                     false);
+  pref_service()->SetBoolean(
+      password_manager::prefs::kPasswordLeakDetectionEnabled, false);
 
   EXPECT_CALL(factory(), TryCreateLeakCheck).Times(0);
   delegate().StartLeakCheck(form);
@@ -133,7 +146,7 @@ TEST_F(LeakDetectionDelegateTest, StartCheck) {
   EXPECT_TRUE(delegate().leak_check());
 }
 
-TEST_F(LeakDetectionDelegateTest, LeakDetectionDone) {
+TEST_F(LeakDetectionDelegateTest, LeakDetectionDoneWithFalseResult) {
   base::HistogramTester histogram_tester;
   LeakDetectionDelegateInterface* delegate_interface = &delegate();
   const autofill::PasswordForm form = CreateTestForm();
@@ -144,9 +157,20 @@ TEST_F(LeakDetectionDelegateTest, LeakDetectionDone) {
 
   EXPECT_CALL(client(), NotifyUserCredentialsWereLeaked).Times(0);
   delegate_interface->OnLeakDetectionDone(
-      false, form.origin, form.username_value, form.password_value);
+      /*is_leaked=*/false, form.origin, form.username_value,
+      form.password_value);
   histogram_tester.ExpectTotalCount(
       "PasswordManager.LeakDetection.NotifyIsLeakedTime", 0);
+}
+
+TEST_F(LeakDetectionDelegateTest, LeakDetectionDoneWithTrueResult) {
+  base::HistogramTester histogram_tester;
+  LeakDetectionDelegateInterface* delegate_interface = &delegate();
+  const autofill::PasswordForm form = CreateTestForm();
+
+  EXPECT_CALL(factory(), TryCreateLeakCheck)
+      .WillOnce(Return(ByMove(std::make_unique<MockLeakDetectionCheck>())));
+  delegate().StartLeakCheck(form);
 
   EXPECT_CALL(client(),
               NotifyUserCredentialsWereLeaked(
@@ -154,9 +178,55 @@ TEST_F(LeakDetectionDelegateTest, LeakDetectionDone) {
                       IsSaved(false), IsReused(false), IsSyncing(false)),
                   form.origin));
   delegate_interface->OnLeakDetectionDone(
-      true, form.origin, form.username_value, form.password_value);
+      /*is_leaked=*/true, form.origin, form.username_value,
+      form.password_value);
   histogram_tester.ExpectTotalCount(
       "PasswordManager.LeakDetection.NotifyIsLeakedTime", 1);
 }
 
+TEST_F(LeakDetectionDelegateTest, LeakHistoryRemoveCredentials) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(features::kLeakHistory);
+  LeakDetectionDelegateInterface* delegate_interface = &delegate();
+  const autofill::PasswordForm form = CreateTestForm();
+
+  EXPECT_CALL(client(), GetProfilePasswordStore())
+      .WillRepeatedly(testing::Return(store()));
+  EXPECT_CALL(factory(), TryCreateLeakCheck)
+      .WillOnce(Return(ByMove(std::make_unique<MockLeakDetectionCheck>())));
+  delegate().StartLeakCheck(form);
+
+  EXPECT_CALL(client(), NotifyUserCredentialsWereLeaked).Times(0);
+  delegate_interface->OnLeakDetectionDone(
+      /*is_leaked=*/false, form.origin, form.username_value,
+      form.password_value);
+
+  EXPECT_CALL(*store(),
+              RemoveLeakedCredentialsImpl(form.origin, form.username_value));
+  WaitForPasswordStore();
+}
+
+TEST_F(LeakDetectionDelegateTest, LeakHistoryAddCredentials) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(features::kLeakHistory);
+  LeakDetectionDelegateInterface* delegate_interface = &delegate();
+  const autofill::PasswordForm form = CreateTestForm();
+
+  EXPECT_CALL(client(), GetProfilePasswordStore())
+      .WillRepeatedly(testing::Return(store()));
+  EXPECT_CALL(factory(), TryCreateLeakCheck)
+      .WillOnce(Return(ByMove(std::make_unique<MockLeakDetectionCheck>())));
+  delegate().StartLeakCheck(form);
+
+  EXPECT_CALL(client(), NotifyUserCredentialsWereLeaked(_, form.origin));
+  delegate_interface->OnLeakDetectionDone(
+      /*is_leaked=*/true, form.origin, form.username_value,
+      form.password_value);
+
+  const LeakedCredentials leaked_credentials(form.origin, form.username_value,
+                                             base::Time::Now());
+  EXPECT_CALL(*store(), AddLeakedCredentialsImpl(leaked_credentials));
+  WaitForPasswordStore();
+}
+
 }  // namespace password_manager

components/password_manager/core/browser/leaked_credentials_table.cc

@@ -41,14 +41,15 @@ void InitializeLeakedCredentialsBuilder(SQLTableBuilder* builder) {
 std::vector<LeakedCredentials> StatementToLeakedCredentials(sql::Statement* s) {
   std::vector<LeakedCredentials> results;
   while (s->Step()) {
-    results.push_back(LeakedCredentials());
-    results.back().url = GURL(
+    GURL url(
         s->ColumnString(GetColumnNumber(LeakedCredentialsTableColumn::kUrl)));
-    results.back().username = s->ColumnString16(
+    base::string16 username = s->ColumnString16(
         GetColumnNumber(LeakedCredentialsTableColumn::kUsername));
-    results.back().create_time = base::Time::FromDeltaSinceWindowsEpoch(
+    base::Time create_time = base::Time::FromDeltaSinceWindowsEpoch(
         (base::TimeDelta::FromMicroseconds(s->ColumnInt64(
             GetColumnNumber(LeakedCredentialsTableColumn::kCreateTime)))));
+    results.push_back(
+        LeakedCredentials(std::move(url), std::move(username), create_time));
   }
   return results;
 }

components/password_manager/core/browser/leaked_credentials_table.h

@@ -16,9 +16,12 @@ class Database;
 namespace password_manager {
 
 // Represents information about the particular credentials leak.
-// TODO(crbug.com/1005746): Discuss the design whether we should have additional
-// |update_time| field or store multiple records with different |create_time|.
 struct LeakedCredentials {
+  LeakedCredentials(GURL url, base::string16 username, base::Time create_time)
+      : url(std::move(url)),
+        username(std::move(username)),
+        create_time(create_time) {}
+
   // The url of the website of the leak.
   GURL url;
   // The value of the leaked username.

components/password_manager/core/browser/leaked_credentials_table_unittest.cc

@@ -26,10 +26,6 @@ class LeakedCredentialsTableTest : public testing::Test {
   void SetUp() override {
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
     ReloadDatabase();
-
-    test_data_.url = GURL(kTestDomain);
-    test_data_.username = base::ASCIIToUTF16(kUsername);
-    test_data_.create_time = base::Time::FromTimeT(1);
   }
 
   void ReloadDatabase() {
@@ -49,7 +45,8 @@ class LeakedCredentialsTableTest : public testing::Test {
   base::ScopedTempDir temp_dir_;
   std::unique_ptr<sql::Database> connection_;
   std::unique_ptr<LeakedCredentialsTable> db_;
-  LeakedCredentials test_data_;
+  LeakedCredentials test_data_{GURL(kTestDomain), base::ASCIIToUTF16(kUsername),
+                               base::Time::FromTimeT(1)};
 };
 
 TEST_F(LeakedCredentialsTableTest, Sanity) {

components/password_manager/core/browser/password_store_unittest.cc

@@ -1248,15 +1248,12 @@ TEST_F(PasswordStoreTest, ReportMetricsForNonSyncPassword) {
 }
 
 TEST_F(PasswordStoreTest, GetAllLeakedCredentials) {
-  LeakedCredentials leaked_credentials;
-  leaked_credentials.url = GURL("https://example.com");
-  leaked_credentials.username = base::ASCIIToUTF16("username");
-  leaked_credentials.create_time = base::Time::FromTimeT(1);
-
-  LeakedCredentials leaked_credentials2;
-  leaked_credentials2.url = GURL("https://example2.com");
-  leaked_credentials2.username = base::ASCIIToUTF16("username2");
-  leaked_credentials2.create_time = base::Time::FromTimeT(2);
+  LeakedCredentials leaked_credentials(GURL("https://example.com"),
+                                       base::ASCIIToUTF16("username"),
+                                       base::Time::FromTimeT(1));
+  LeakedCredentials leaked_credentials2(GURL("https://example2.com"),
+                                        base::ASCIIToUTF16("username2"),
+                                        base::Time::FromTimeT(2));
 
   scoped_refptr<PasswordStoreDefault> store = CreatePasswordStore();
   store->Init(syncer::SyncableService::StartSyncFlare(), nullptr);