Add url filtering support for removing leaked credentials

Adds url filter support to function which removes leaked credentials
from the database by time range as suggested by the privacy team.


Bug: 1005746
Change-Id: I8bb417c21592b9d72a885d23590ef4f8b41a35b3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1847297
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@{#704168}

components/password_manager/core/browser/leaked_credentials_table.cc

@@ -106,17 +106,57 @@ bool LeakedCredentialsTable::RemoveRow(const GURL& url,
   return s.Run();
 }
 
-bool LeakedCredentialsTable::RemoveRowsCreatedBetween(base::Time remove_begin,
-                                                      base::Time remove_end) {
-  sql::Statement s(
-      db_->GetCachedStatement(SQL_FROM_HERE,
-                              "DELETE FROM leaked_credentials WHERE "
-                              "create_time >= ? AND create_time < ?"));
-  s.BindInt64(0, remove_begin.ToDeltaSinceWindowsEpoch().InMicroseconds());
-  s.BindInt64(1, remove_end.is_null()
-                     ? std::numeric_limits<int64_t>::max()
-                     : remove_end.ToDeltaSinceWindowsEpoch().InMicroseconds());
-  return s.Run();
+bool LeakedCredentialsTable::RemoveRowsByUrlAndTime(
+    const base::RepeatingCallback<bool(const GURL&)>& url_filter,
+    base::Time remove_begin,
+    base::Time remove_end) {
+  if (remove_end.is_null())
+    remove_end = base::Time::Max();
+
+  const int64_t remove_begin_us =
+      remove_begin.ToDeltaSinceWindowsEpoch().InMicroseconds();
+  const int64_t remove_end_us =
+      remove_end.ToDeltaSinceWindowsEpoch().InMicroseconds();
+
+  // If |url_filter| is null, remove all records in given date range.
+  if (!url_filter) {
+    sql::Statement s(
+        db_->GetCachedStatement(SQL_FROM_HERE,
+                                "DELETE FROM leaked_credentials WHERE "
+                                "create_time >= ? AND create_time < ?"));
+    s.BindInt64(0, remove_begin_us);
+    s.BindInt64(1, remove_end_us);
+    return s.Run();
+  }
+
+  // Otherwise, filter urls.
+  sql::Statement s(db_->GetCachedStatement(
+      SQL_FROM_HERE,
+      "SELECT DISTINCT url FROM leaked_credentials WHERE "
+      "create_time >= ? AND create_time < ?"));
+  s.BindInt64(0, remove_begin_us);
+  s.BindInt64(1, remove_end_us);
+
+  std::vector<std::string> urls;
+  while (s.Step()) {
+    std::string url = s.ColumnString(0);
+    if (url_filter.Run(GURL(url))) {
+      urls.push_back(std::move(url));
+    }
+  }
+
+  bool success = true;
+  for (const std::string& url : urls) {
+    sql::Statement s(
+        db_->GetCachedStatement(SQL_FROM_HERE,
+                                "DELETE FROM leaked_credentials WHERE url = ? "
+                                "AND create_time >= ? AND create_time < ?"));
+    s.BindString(0, url);
+    s.BindInt64(1, remove_begin_us);
+    s.BindInt64(2, remove_end_us);
+    success = success && s.Run();
+  }
+  return success;
 }
 
 std::vector<LeakedCredentials> LeakedCredentialsTable::GetAllRows() {

components/password_manager/core/browser/leaked_credentials_table.h

@@ -54,8 +54,13 @@ class LeakedCredentialsTable {
 
   // Removes all leaked credentials created between |remove_begin| inclusive and
   // |remove_end| exclusive. Using a null Time value will do an unbounded delete
-  // in either direction.
-  bool RemoveRowsCreatedBetween(base::Time remove_begin, base::Time remove_end);
+  // in either direction. If |url_filter| is not null, only leaked credentials
+  // for matching urls are removed. Returns true if the SQL completed
+  // successfully.
+  bool RemoveRowsByUrlAndTime(
+      const base::RepeatingCallback<bool(const GURL&)>& url_filter,
+      base::Time remove_begin,
+      base::Time remove_end);
 
   // Returns all leaked credentials from the database.
   std::vector<LeakedCredentials> GetAllRows();

components/password_manager/core/browser/leaked_credentials_table_unittest.cc

@@ -4,6 +4,9 @@
 
 #include "components/password_manager/core/browser/leaked_credentials_table.h"
 
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/callback.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/strings/utf_string_conversions.h"
 #include "sql/database.h"
@@ -15,6 +18,7 @@ namespace {
 
 const char kTestDomain[] = "http://example.com";
 const char kTestDomain2[] = "http://test.com";
+const char kTestDomain3[] = "http://google.com";
 const char kUsername[] = "user";
 const char kUsername2[] = "user2";
 const char kUsername3[] = "user3";
@@ -116,8 +120,9 @@ TEST_F(LeakedCredentialsTableTest, RemoveRowsCreatedBetween) {
               ElementsAre(leaked_credentials1, leaked_credentials2,
                           leaked_credentials3));
 
-  EXPECT_TRUE(db()->RemoveRowsCreatedBetween(base::Time::FromTimeT(15),
-                                             base::Time::FromTimeT(25)));
+  EXPECT_TRUE(db()->RemoveRowsByUrlAndTime(base::NullCallback(),
+                                           base::Time::FromTimeT(15),
+                                           base::Time::FromTimeT(25)));
 
   EXPECT_THAT(db()->GetAllRows(),
               ElementsAre(leaked_credentials1, leaked_credentials3));
@@ -138,7 +143,8 @@ TEST_F(LeakedCredentialsTableTest, RemoveRowsCreatedBetweenEdgeCase) {
   EXPECT_THAT(db()->GetAllRows(),
               ElementsAre(leaked_credentials_begin, leaked_credentials_end));
 
-  EXPECT_TRUE(db()->RemoveRowsCreatedBetween(begin_time, end_time));
+  EXPECT_TRUE(
+      db()->RemoveRowsByUrlAndTime(base::NullCallback(), begin_time, end_time));
   // RemoveRowsCreatedBetween takes |begin_time| inclusive and |end_time|
   // exclusive, hence the credentials with |end_time| should remain in the
   // database.
@@ -163,11 +169,39 @@ TEST_F(LeakedCredentialsTableTest, RemoveRowsCreatedUpUntilNow) {
               ElementsAre(leaked_credentials1, leaked_credentials2,
                           leaked_credentials3));
 
-  EXPECT_TRUE(db()->RemoveRowsCreatedBetween(base::Time(), base::Time()));
+  EXPECT_TRUE(db()->RemoveRowsByUrlAndTime(base::NullCallback(), base::Time(),
+                                           base::Time()));
 
   EXPECT_THAT(db()->GetAllRows(), IsEmpty());
 }
 
+TEST_F(LeakedCredentialsTableTest, RemoveRowsByUrlAndTime) {
+  LeakedCredentials leaked_credentials1 = test_data();
+  LeakedCredentials leaked_credentials2 = test_data();
+  LeakedCredentials leaked_credentials3 = test_data();
+  LeakedCredentials leaked_credentials4 = test_data();
+  leaked_credentials2.username = base::ASCIIToUTF16(kUsername2);
+  leaked_credentials3.url = GURL(kTestDomain2);
+  leaked_credentials4.url = GURL(kTestDomain3);
+
+  EXPECT_TRUE(db()->AddRow(leaked_credentials1));
+  EXPECT_TRUE(db()->AddRow(leaked_credentials2));
+  EXPECT_TRUE(db()->AddRow(leaked_credentials3));
+  EXPECT_TRUE(db()->AddRow(leaked_credentials4));
+
+  EXPECT_THAT(db()->GetAllRows(),
+              ElementsAre(leaked_credentials1, leaked_credentials2,
+                          leaked_credentials3, leaked_credentials4));
+
+  EXPECT_TRUE(db()->RemoveRowsByUrlAndTime(
+      base::BindRepeating(std::not_equal_to<GURL>(), leaked_credentials1.url),
+      base::Time(), base::Time()));
+  // With unbounded time range and given url filter all rows that are not
+  // matching the |leaked_credentials1.url| should be removed.
+  EXPECT_THAT(db()->GetAllRows(),
+              ElementsAre(leaked_credentials1, leaked_credentials2));
+}
+
 TEST_F(LeakedCredentialsTableTest, BadURL) {
   test_data().url = GURL("bad");
   EXPECT_FALSE(db()->AddRow(test_data()));

components/password_manager/core/browser/mock_password_store.h

@@ -76,8 +76,10 @@ class MockPasswordStore : public PasswordStore {
   MOCK_METHOD2(RemoveLeakedCredentialsImpl,
                void(const GURL&, const base::string16&));
   MOCK_METHOD0(GetAllLeakedCredentialsImpl, std::vector<LeakedCredentials>());
-  MOCK_METHOD2(RemoveLeakedCredentialsCreatedBetweenImpl,
-               void(base::Time, base::Time));
+  MOCK_METHOD3(RemoveLeakedCredentialsByUrlAndTimeImpl,
+               void(const base::RepeatingCallback<bool(const GURL&)>&,
+                    base::Time,
+                    base::Time));
   MOCK_CONST_METHOD0(IsAbleToSavePasswords, bool());
 
 #if defined(SYNC_PASSWORD_REUSE_DETECTION_ENABLED)

components/password_manager/core/browser/password_store.cc

@@ -358,14 +358,15 @@ void PasswordStore::GetAllLeakedCredentials(
       base::BindOnce(&PasswordStore::GetAllLeakedCredentialsImpl, this));
 }
 
-void PasswordStore::RemoveLeakedCredentialsCreatedBetween(
+void PasswordStore::RemoveLeakedCredentialsByUrlAndTime(
+    base::RepeatingCallback<bool(const GURL&)> url_filter,
     base::Time remove_begin,
     base::Time remove_end,
-    const base::Closure& completion) {
+    base::OnceClosure completion) {
   DCHECK(main_task_runner_->RunsTasksInCurrentSequence());
   ScheduleTask(base::BindOnce(
-      &PasswordStore::RemoveLeakedCredentialsCreatedBetweenInternal, this,
-      remove_begin, remove_end, completion));
+      &PasswordStore::RemoveLeakedCredentialsByUrlAndTimeInternal, this,
+      std::move(url_filter), remove_begin, remove_end, std::move(completion)));
 }
 
 void PasswordStore::AddObserver(Observer* observer) {
@@ -860,14 +861,15 @@ void PasswordStore::UnblacklistInternal(
     main_task_runner_->PostTask(FROM_HERE, std::move(completion));
 }
 
-void PasswordStore::RemoveLeakedCredentialsCreatedBetweenInternal(
+void PasswordStore::RemoveLeakedCredentialsByUrlAndTimeInternal(
+    const base::RepeatingCallback<bool(const GURL&)>& url_filter,
     base::Time remove_begin,
     base::Time remove_end,
-    const base::Closure& completion) {
+    base::OnceClosure completion) {
   DCHECK(background_task_runner_->RunsTasksInCurrentSequence());
-  RemoveLeakedCredentialsCreatedBetweenImpl(remove_begin, remove_end);
+  RemoveLeakedCredentialsByUrlAndTimeImpl(url_filter, remove_begin, remove_end);
   if (!completion.is_null())
-    main_task_runner_->PostTask(FROM_HERE, completion);
+    main_task_runner_->PostTask(FROM_HERE, std::move(completion));
 }
 
 std::vector<std::unique_ptr<autofill::PasswordForm>>

components/password_manager/core/browser/password_store.h

@@ -253,12 +253,15 @@ class PasswordStore : protected PasswordStoreSync,
   // request will be cancelled if the consumer is destroyed.
   void GetAllLeakedCredentials(PasswordLeakHistoryConsumer* consumer);
 
-  // Removes all leaked credentials in the given date range. If |completion| is
-  // not null, it will be posted to the |main_task_runner_| after deletions have
-  // been completed. Should be called on the UI thread.
-  void RemoveLeakedCredentialsCreatedBetween(base::Time remove_begin,
-                                             base::Time remove_end,
-                                             const base::Closure& completion);
+  // Removes all leaked credentials in the given date range. If |url_filter| is
+  // not null, only leaked credentials for matching urls are removed. If
+  // |completion| is not null, it will be posted to the |main_task_runner_|
+  // after deletions have been completed. Should be called on the UI thread.
+  void RemoveLeakedCredentialsByUrlAndTime(
+      base::RepeatingCallback<bool(const GURL&)> url_filter,
+      base::Time remove_begin,
+      base::Time remove_end,
+      base::OnceClosure completion);
 
   // Adds an observer to be notified when the password store data changes.
   void AddObserver(Observer* observer);
@@ -460,7 +463,8 @@ class PasswordStore : protected PasswordStoreSync,
   virtual void RemoveLeakedCredentialsImpl(const GURL& url,
                                            const base::string16& username) = 0;
   virtual std::vector<LeakedCredentials> GetAllLeakedCredentialsImpl() = 0;
-  virtual void RemoveLeakedCredentialsCreatedBetweenImpl(
+  virtual void RemoveLeakedCredentialsByUrlAndTimeImpl(
+      const base::RepeatingCallback<bool(const GURL&)>& url_filter,
       base::Time remove_begin,
       base::Time remove_end) = 0;
 
@@ -602,10 +606,11 @@ class PasswordStore : protected PasswordStoreSync,
       const base::Closure& completion);
   void UnblacklistInternal(const PasswordStore::FormDigest& form_digest,
                            base::OnceClosure completion);
-  void RemoveLeakedCredentialsCreatedBetweenInternal(
+  void RemoveLeakedCredentialsByUrlAndTimeInternal(
+      const base::RepeatingCallback<bool(const GURL&)>& url_filter,
       base::Time remove_begin,
       base::Time remove_end,
-      const base::Closure& completion);
+      base::OnceClosure completion);
 
   // Finds all PasswordForms with a signon_realm that is equal to, or is a
   // PSL-match to that of |form|, and takes care of notifying the consumer with

components/password_manager/core/browser/password_store_default.cc

@@ -244,12 +244,13 @@ PasswordStoreDefault::GetAllLeakedCredentialsImpl() {
                    : std::vector<LeakedCredentials>();
 }
 
-void PasswordStoreDefault::RemoveLeakedCredentialsCreatedBetweenImpl(
+void PasswordStoreDefault::RemoveLeakedCredentialsByUrlAndTimeImpl(
+    const base::RepeatingCallback<bool(const GURL&)>& url_filter,
     base::Time remove_begin,
     base::Time remove_end) {
   if (login_db_) {
-    login_db_->leaked_credentials_table().RemoveRowsCreatedBetween(remove_begin,
-                                                                   remove_end);
+    login_db_->leaked_credentials_table().RemoveRowsByUrlAndTime(
+        url_filter, remove_begin, remove_end);
   }
 }
 

components/password_manager/core/browser/password_store_default.h

@@ -77,7 +77,8 @@ class PasswordStoreDefault : public PasswordStore {
   void RemoveLeakedCredentialsImpl(const GURL& url,
                                    const base::string16& username) override;
   std::vector<LeakedCredentials> GetAllLeakedCredentialsImpl() override;
-  void RemoveLeakedCredentialsCreatedBetweenImpl(
+  void RemoveLeakedCredentialsByUrlAndTimeImpl(
+      const base::RepeatingCallback<bool(const GURL&)>& url_filter,
       base::Time remove_begin,
       base::Time remove_end) override;
 

components/password_manager/core/browser/password_store_unittest.cc

@@ -1303,8 +1303,9 @@ TEST_F(PasswordStoreTest, RemoveLeakedCredentialsCreatedBetween) {
   WaitForPasswordStore();
   testing::Mock::VerifyAndClearExpectations(&consumer);
 
-  store->RemoveLeakedCredentialsCreatedBetween(
-      base::Time::FromTimeT(150), base::Time::FromTimeT(250), base::Closure());
+  store->RemoveLeakedCredentialsByUrlAndTime(
+      base::BindRepeating(std::not_equal_to<GURL>(), leaked_credentials3.url),
+      base::Time::FromTimeT(150), base::Time::FromTimeT(350), base::Closure());
 
   EXPECT_CALL(consumer, OnGetLeakedCredentials(UnorderedElementsAre(
                             leaked_credentials1, leaked_credentials3)));

components/password_manager/core/browser/test_password_store.cc

@@ -242,7 +242,8 @@ TestPasswordStore::GetAllLeakedCredentialsImpl() {
   return std::vector<LeakedCredentials>();
 }
 
-void TestPasswordStore::RemoveLeakedCredentialsCreatedBetweenImpl(
+void TestPasswordStore::RemoveLeakedCredentialsByUrlAndTimeImpl(
+    const base::RepeatingCallback<bool(const GURL&)>& url_filter,
     base::Time remove_begin,
     base::Time remove_end) {
   NOTIMPLEMENTED();

components/password_manager/core/browser/test_password_store.h

@@ -90,7 +90,8 @@ class TestPasswordStore : public PasswordStore {
   void RemoveLeakedCredentialsImpl(const GURL& url,
                                    const base::string16& username) override;
   std::vector<LeakedCredentials> GetAllLeakedCredentialsImpl() override;
-  void RemoveLeakedCredentialsCreatedBetweenImpl(
+  void RemoveLeakedCredentialsByUrlAndTimeImpl(
+      const base::RepeatingCallback<bool(const GURL&)>& url_filter,
       base::Time remove_begin,
       base::Time remove_end) override;