Access Context Auditing: Respect time range history deletions

Extends Access Context service and database to delete access records
with timestamps that fall within an observed history deletion time
range.

Bug: 1101675
Change-Id: I49e0fdfc4d638c6d065d63a188da57ae5ab9ffca
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2312699
Commit-Queue: Theodore Olsauskas-Warren <sauski@google.com>
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#791736}

chrome/browser/browsing_data/access_context_audit_database.cc

@@ -306,6 +306,38 @@ void AccessContextAuditDatabase::RemoveAllRecords() {
   transaction.Commit();
 }
 
+void AccessContextAuditDatabase::RemoveAllRecordsForTimeRange(base::Time begin,
+                                                              base::Time end) {
+  sql::Transaction transaction(&db_);
+  if (!transaction.Begin())
+    return;
+
+  std::string remove = "DELETE FROM ";
+  remove.append(kCookieTableName);
+  remove.append(" WHERE access_utc BETWEEN ? AND ?");
+  sql::Statement remove_cookies(
+      db_.GetCachedStatement(SQL_FROM_HERE, remove.c_str()));
+  remove_cookies.BindInt64(0,
+                           begin.ToDeltaSinceWindowsEpoch().InMicroseconds());
+  remove_cookies.BindInt64(1, end.ToDeltaSinceWindowsEpoch().InMicroseconds());
+  if (!remove_cookies.Run())
+    return;
+
+  remove = "DELETE FROM ";
+  remove.append(kStorageAPITableName);
+  remove.append(" WHERE access_utc BETWEEN ? AND ?");
+  sql::Statement remove_storage_apis(
+      db_.GetCachedStatement(SQL_FROM_HERE, remove.c_str()));
+  remove_storage_apis.BindInt64(
+      0, begin.ToDeltaSinceWindowsEpoch().InMicroseconds());
+  remove_storage_apis.BindInt64(
+      1, end.ToDeltaSinceWindowsEpoch().InMicroseconds());
+  if (!remove_storage_apis.Run())
+    return;
+
+  transaction.Commit();
+}
+
 void AccessContextAuditDatabase::RemoveSessionOnlyRecords(
     scoped_refptr<content_settings::CookieSettings> cookie_settings,
     const ContentSettingsForOneType& content_settings) {

chrome/browser/browsing_data/access_context_audit_database.h

@@ -88,6 +88,9 @@ class AccessContextAuditDatabase
   // Removes all records from the the database.
   void RemoveAllRecords();
 
+  // Removes all records where |begin| <= record.last_access_time <= |end|.
+  void RemoveAllRecordsForTimeRange(base::Time begin, base::Time end);
+
   // Removes all records that match the provided cookie details.
   void RemoveAllRecordsForCookie(const std::string& name,
                                  const std::string& domain,

chrome/browser/browsing_data/access_context_audit_database_unittest.cc

@@ -320,6 +320,32 @@ TEST_F(AccessContextAuditDatabaseTest, RemoveAllRecords) {
   EXPECT_EQ(0u, storage_api_rows);
 }
 
+TEST_F(AccessContextAuditDatabaseTest, RemoveAllRecordsForTimeRange) {
+  // Check that records within the specified time range are removed.
+  auto test_records = GetTestRecords();
+  OpenDatabase();
+  database()->AddRecords(test_records);
+  ValidateDatabaseRecords(database(), test_records);
+
+  auto begin_time =
+      base::Time::FromDeltaSinceWindowsEpoch(base::TimeDelta::FromHours(4));
+  auto end_time =
+      base::Time::FromDeltaSinceWindowsEpoch(base::TimeDelta::FromHours(6));
+
+  database()->RemoveAllRecordsForTimeRange(begin_time, end_time);
+
+  test_records.erase(
+      std::remove_if(
+          test_records.begin(), test_records.end(),
+          [=](const AccessContextAuditDatabase::AccessRecord& record) {
+            return record.last_access_time >= begin_time &&
+                   record.last_access_time <= end_time;
+          }),
+      test_records.end());
+
+  ValidateDatabaseRecords(database(), test_records);
+}
+
 TEST_F(AccessContextAuditDatabaseTest, RemoveAllCookieRecords) {
   // Check that all matching cookie records are removed from the database.
   auto test_records = GetTestRecords();

chrome/browser/browsing_data/access_context_audit_service.cc

@@ -7,6 +7,7 @@
 #include "base/sequenced_task_runner.h"
 #include "base/task/post_task.h"
 #include "base/task/thread_pool.h"
+#include "base/time/default_clock.h"
 #include "base/time/time.h"
 #include "chrome/browser/browsing_data/access_context_audit_database.h"
 #include "chrome/browser/content_settings/cookie_settings_factory.h"
@@ -16,7 +17,7 @@
 #include "content/public/browser/storage_partition.h"
 
 AccessContextAuditService::AccessContextAuditService(Profile* profile)
-    : profile_(profile) {}
+    : clock_(base::DefaultClock::GetInstance()), profile_(profile) {}
 AccessContextAuditService::~AccessContextAuditService() = default;
 
 bool AccessContextAuditService::Init(
@@ -49,7 +50,7 @@ bool AccessContextAuditService::Init(
 void AccessContextAuditService::RecordCookieAccess(
     const net::CookieList& accessed_cookies,
     const url::Origin& top_frame_origin) {
-  auto now = base::Time::Now();
+  auto now = clock_->Now();
   std::vector<AccessContextAuditDatabase::AccessRecord> access_records;
   for (const auto& cookie : accessed_cookies) {
     // Do not record accesses to already expired cookies. This service is
@@ -71,8 +72,8 @@ void AccessContextAuditService::RecordStorageAPIAccess(
     AccessContextAuditDatabase::StorageAPIType type,
     const url::Origin& top_frame_origin) {
   std::vector<AccessContextAuditDatabase::AccessRecord> access_record = {
-      AccessContextAuditDatabase::AccessRecord(
-          top_frame_origin, type, storage_origin, base::Time::Now())};
+      AccessContextAuditDatabase::AccessRecord(top_frame_origin, type,
+                                               storage_origin, clock_->Now())};
   database_task_runner_->PostTask(
       FROM_HERE, base::BindOnce(&AccessContextAuditDatabase::AddRecords,
                                 database_, std::move(access_record)));
@@ -122,6 +123,20 @@ void AccessContextAuditService::OnURLsDeleted(
     return;
   }
 
+  if (deletion_info.time_range().IsValid()) {
+    // If a time range is specified, a time based deletion is performed as a
+    // first pass before origins without history entries are removed. A second
+    // pass based on origins is required as access record timestamps are not
+    // directly comparable to history timestamps. Only deleting based on
+    // timestamp may persist origins on disk for which no other trace exists.
+    database_task_runner_->PostTask(
+        FROM_HERE,
+        base::BindOnce(
+            &AccessContextAuditDatabase::RemoveAllRecordsForTimeRange,
+            database_, deletion_info.time_range().begin(),
+            deletion_info.time_range().end()));
+  }
+
   std::vector<url::Origin> deleted_origins;
   // Map is of type {Origin -> {Count, LastVisitTime}}.
   for (const auto& origin_urls_remaining :
@@ -141,6 +156,10 @@ void AccessContextAuditService::OnURLsDeleted(
   }
 }
 
+void AccessContextAuditService::SetClockForTesting(base::Clock* clock) {
+  clock_ = clock;
+}
+
 void AccessContextAuditService::SetTaskRunnerForTesting(
     scoped_refptr<base::SequencedTaskRunner> task_runner) {
   DCHECK(!database_task_runner_);

chrome/browser/browsing_data/access_context_audit_service.h

@@ -56,6 +56,10 @@ class AccessContextAuditService : public KeyedService,
   void OnURLsDeleted(history::HistoryService* history_service,
                      const history::DeletionInfo& deletion_info) override;
 
+  // Override the internal clock used to record storage API access timestamps
+  // and check for expired cookies.
+  void SetClockForTesting(base::Clock* clock);
+
   // Override internal task runner with provided task runner. Must be called
   // before Init().
   void SetTaskRunnerForTesting(
@@ -71,6 +75,7 @@ class AccessContextAuditService : public KeyedService,
   scoped_refptr<AccessContextAuditDatabase> database_;
   scoped_refptr<base::SequencedTaskRunner> database_task_runner_;
 
+  base::Clock* clock_;
   Profile* profile_;
 
   mojo::Receiver<network::mojom::CookieChangeListener>

chrome/browser/browsing_data/access_context_audit_service_unittest.cc

@@ -7,6 +7,7 @@
 #include "base/files/scoped_temp_dir.h"
 #include "base/i18n/time_formatting.h"
 #include "base/test/scoped_feature_list.h"
+#include "base/test/simple_test_clock.h"
 #include "base/test/test_simple_task_runner.h"
 #include "chrome/browser/browsing_data/access_context_audit_database.h"
 #include "chrome/browser/browsing_data/access_context_audit_service_factory.h"
@@ -146,6 +147,7 @@ class AccessContextAuditServiceTest : public testing::Test {
   void ClearReturnedRecords() { records_.clear(); }
 
   TestCookieManager* cookie_manager() { return &cookie_manager_; }
+  base::SimpleTestClock* clock() { return &clock_; }
   TestingProfile* profile() { return profile_.get(); }
   history::HistoryService* history_service() { return history_service_; }
   AccessContextAuditService* service() {
@@ -155,6 +157,7 @@ class AccessContextAuditServiceTest : public testing::Test {
  protected:
   content::BrowserTaskEnvironment browser_task_environment_;
   std::unique_ptr<TestingProfile> profile_;
+  base::SimpleTestClock clock_;
   base::ScopedTempDir temp_directory_;
   TestCookieManager cookie_manager_;
   history::HistoryService* history_service_;
@@ -394,6 +397,93 @@ TEST_F(AccessContextAuditServiceTest, AllHistoryDeletion) {
   EXPECT_EQ(0u, GetReturnedRecords().size());
 }
 
+TEST_F(AccessContextAuditServiceTest, TimeRangeHistoryDeletion) {
+  // Test that deleting a time range of history records correctly removes
+  // records within the time range, as well as records for which no history
+  // entry for the top frame origin remains.
+
+  // Create a situation where origin https://foo.com has history entries and
+  // access records with timestamps both inside and outside the deleted range.
+  // Additionally create a single history entry for origin https://bar.com
+  // inside the deleted range, and multiple access records outside the range.
+  // After deletion, the access records for https://foo.com outside the deletion
+  // range should still be present, while all access records https://bar.com
+  // should have been removed.
+
+  const GURL kURL1 = GURL("https://foo.com/example.html");
+  const GURL kURL2 = GURL("https://bar.com/another.html");
+  const url::Origin kOrigin1 = url::Origin::Create(kURL1);
+  const url::Origin kOrigin2 = url::Origin::Create(kURL2);
+  const GURL kTestCookieURL = GURL("https://test.com");
+  const auto kTestStorageType1 =
+      AccessContextAuditDatabase::StorageAPIType::kWebDatabase;
+  const auto kTestStorageType2 =
+      AccessContextAuditDatabase::StorageAPIType::kAppCache;
+
+  clock()->SetNow(base::Time::Now());
+  service()->SetClockForTesting(clock());
+  const base::Time kInsideTimeRange =
+      clock()->Now() + base::TimeDelta::FromHours(1);
+  const base::Time kOutsideTimeRange =
+      clock()->Now() + base::TimeDelta::FromHours(3);
+
+  history_service()->AddPageWithDetails(kURL1, base::ASCIIToUTF16("Test1"), 1,
+                                        1, kInsideTimeRange, false,
+                                        history::SOURCE_BROWSED);
+  history_service()->AddPageWithDetails(kURL2, base::ASCIIToUTF16("Test2"), 1,
+                                        1, kInsideTimeRange, false,
+                                        history::SOURCE_BROWSED);
+  history_service()->AddPageWithDetails(kURL1, base::ASCIIToUTF16("Test3"), 1,
+                                        1, kOutsideTimeRange, false,
+                                        history::SOURCE_BROWSED);
+
+  // Record accesses to cookies both inside and outside the deletion range.
+  auto cookie_accessed_in_range = net::CanonicalCookie::Create(
+      kTestCookieURL, "inside=1; max-age=3600", kInsideTimeRange,
+      base::nullopt /* server_time */);
+  auto cookie_accessed_outside_range = net::CanonicalCookie::Create(
+      kTestCookieURL, "outside=1; max-age=3600", kOutsideTimeRange,
+      base::nullopt /* server_time */);
+
+  service()->RecordCookieAccess({*cookie_accessed_in_range}, kOrigin1);
+  service()->RecordCookieAccess({*cookie_accessed_outside_range}, kOrigin1);
+  service()->RecordCookieAccess({*cookie_accessed_outside_range}, kOrigin2);
+
+  // Record accesses to storage APIs both inside and outside the deletion range.
+  clock()->SetNow(kInsideTimeRange);
+  service()->RecordStorageAPIAccess(kOrigin1, kTestStorageType1, kOrigin1);
+  clock()->SetNow(kOutsideTimeRange);
+  service()->RecordStorageAPIAccess(kOrigin1, kTestStorageType2, kOrigin1);
+  service()->RecordStorageAPIAccess(kOrigin2, kTestStorageType1, kOrigin2);
+
+  // Ensure all records have been initially recorded.
+  service()->GetAllAccessRecords(
+      base::BindOnce(&AccessContextAuditServiceTest::AccessRecordCallback,
+                     base::Unretained(this)));
+  browser_task_environment_.RunUntilIdle();
+  EXPECT_EQ(6u, GetReturnedRecords().size());
+
+  // Expire history in target time range.
+  base::RunLoop run_loop;
+  base::CancelableTaskTracker task_tracker;
+  history_service()->ExpireHistoryBetween(
+      std::set<GURL>(), kInsideTimeRange - base::TimeDelta::FromMinutes(10),
+      kInsideTimeRange + base::TimeDelta::FromMinutes(10),
+      /*user_initiated*/ true, run_loop.QuitClosure(), &task_tracker);
+  run_loop.Run();
+
+  // Ensure records have been removed as expected.
+  service()->GetAllAccessRecords(
+      base::BindOnce(&AccessContextAuditServiceTest::AccessRecordCallback,
+                     base::Unretained(this)));
+  browser_task_environment_.RunUntilIdle();
+  EXPECT_EQ(2u, GetReturnedRecords().size());
+  CheckContainsCookieRecord(cookie_accessed_outside_range.get(), kOrigin1,
+                            GetReturnedRecords());
+  CheckContainsStorageAPIRecord(kOrigin1, kTestStorageType2, kOrigin1,
+                                GetReturnedRecords());
+}
+
 TEST_F(AccessContextAuditServiceTest, SessionOnlyRecords) {
   // Check that data for cookie domains and storage origins are cleared on
   // service shutdown when the associated content settings indicate they should.