Access Context Auditing: Prevent recording repeated cookie accesses

To reduce the number of records pending update in the access context
audit database, an in memory set of previously seen cookie accesses is
added. This set is consulted before attempting database insertion and
is cleared on navigation.

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

chrome/browser/browsing_data/access_context_audit_browsertest.cc

@@ -6,6 +6,7 @@
 
 #include "base/path_service.h"
 #include "base/test/bind_test_util.h"
+#include "base/test/simple_test_clock.h"
 #include "base/test/test_timeouts.h"
 #include "chrome/browser/browsing_data/access_context_audit_service.h"
 #include "chrome/browser/browsing_data/access_context_audit_service_factory.h"
@@ -469,6 +470,34 @@ IN_PROC_BROWSER_TEST_F(AccessContextAuditBrowserTest, TreeModelDeletion) {
   EXPECT_EQ(cookies.size(), 0u);
 }
 
+IN_PROC_BROWSER_TEST_F(AccessContextAuditBrowserTest, MultipleAccesses) {
+  // Ensure that renavigating to a page in the same tab correctly re-records
+  // accesses.
+  base::SimpleTestClock clock;
+  clock.SetNow(base::Time::Now());
+  AccessContextAuditServiceFactory::GetForProfile(browser()->profile())
+      ->SetClockForTesting(&clock);
+
+  NavigateToTopLevelPage();
+  NavigateToEmbeddedPage();
+
+  // Check all records have the initial access time.
+  auto records = GetAllAccessRecords();
+  for (const auto& record : records)
+    EXPECT_EQ(record.last_access_time, clock.Now());
+
+  // Renavigate to the same pages, this should update the access times on all
+  // records.
+  clock.Advance(base::TimeDelta::FromHours(1));
+  NavigateToTopLevelPage();
+  NavigateToEmbeddedPage();
+
+  // All records should now have the updated time.
+  records = GetAllAccessRecords();
+  for (const auto& record : records)
+    EXPECT_EQ(record.last_access_time, clock.Now());
+}
+
 class AccessContextAuditSessionRestoreBrowserTest
     : public AccessContextAuditBrowserTest {
  public:

chrome/browser/browsing_data/access_context_audit_service.cc

@@ -16,6 +16,37 @@
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "content/public/browser/storage_partition.h"
 
+AccessContextAuditService::CookieAccessHelper::CookieAccessHelper(
+    AccessContextAuditService* service)
+    : service_(service) {
+  DCHECK(service);
+  deletion_observer_.Add(service);
+}
+AccessContextAuditService::CookieAccessHelper::~CookieAccessHelper() = default;
+
+void AccessContextAuditService::CookieAccessHelper::OnCookieDeleted(
+    const net::CanonicalCookie& cookie) {
+  seen_cookies_.erase(cookie);
+}
+
+void AccessContextAuditService::CookieAccessHelper::RecordCookieAccess(
+    const net::CookieList& accessed_cookies,
+    const url::Origin& top_frame_origin) {
+  net::CookieList new_cookies;
+  for (const auto& cookie : accessed_cookies) {
+    if (!seen_cookies_.count(cookie)) {
+      new_cookies.push_back(cookie);
+      seen_cookies_.insert(cookie);
+    }
+  }
+  if (!new_cookies.empty())
+    service_->RecordCookieAccess(new_cookies, top_frame_origin);
+}
+
+void AccessContextAuditService::CookieAccessHelper::ClearSeenCookies() {
+  seen_cookies_.clear();
+}
+
 AccessContextAuditService::AccessContextAuditService(Profile* profile)
     : clock_(base::DefaultClock::GetInstance()), profile_(profile) {}
 AccessContextAuditService::~AccessContextAuditService() = default;
@@ -68,8 +99,8 @@ void AccessContextAuditService::RecordCookieAccess(
       continue;
 
     access_records.emplace_back(top_frame_origin, cookie.Name(),
-                                cookie.Domain(), cookie.Path(),
-                                cookie.LastAccessDate(), cookie.IsPersistent());
+                                cookie.Domain(), cookie.Path(), now,
+                                cookie.IsPersistent());
   }
   database_task_runner_->PostTask(
       FROM_HERE, base::BindOnce(&AccessContextAuditDatabase::AddRecords,
@@ -198,6 +229,10 @@ void AccessContextAuditService::OnCookieChange(
     case net::CookieChangeCause::EXPIRED:
     case net::CookieChangeCause::EVICTED:
     case net::CookieChangeCause::EXPIRED_OVERWRITE: {
+      // Notify helpers so that future accesses to this cookie are reported.
+      for (auto& helper : cookie_access_helpers_) {
+        helper.OnCookieDeleted(change.cookie);
+      }
       // Remove records of deleted cookie from database.
       database_task_runner_->PostTask(
           FROM_HERE,
@@ -251,6 +286,14 @@ void AccessContextAuditService::OnURLsDeleted(
   }
 }
 
+void AccessContextAuditService::AddObserver(CookieAccessHelper* helper) {
+  cookie_access_helpers_.AddObserver(helper);
+}
+
+void AccessContextAuditService::RemoveObserver(CookieAccessHelper* helper) {
+  cookie_access_helpers_.RemoveObserver(helper);
+}
+
 void AccessContextAuditService::SetClockForTesting(base::Clock* clock) {
   clock_ = clock;
 }

chrome/browser/browsing_data/access_context_audit_service.h

@@ -8,6 +8,7 @@
 #include "base/updateable_sequenced_task_runner.h"
 #include "chrome/browser/browsing_data/access_context_audit_database.h"
 #include "chrome/browser/profiles/profile.h"
+#include "components/browsing_data/content/canonical_cookie_hash.h"
 #include "components/browsing_data/content/local_shared_objects_container.h"
 #include "components/history/core/browser/history_service.h"
 #include "components/history/core/browser/history_service_observer.h"
@@ -27,6 +28,43 @@ class AccessContextAuditService
       public history::HistoryServiceObserver,
       public content::StoragePartition::DataRemovalObserver {
  public:
+  class CookieAccessHelper;
+  void AddObserver(CookieAccessHelper* helper);
+  void RemoveObserver(CookieAccessHelper* helper);
+
+  // A helper class used to report cookie accesses to the audit service. Keeps
+  // an internal record of cookie accesses which have already been seen.
+  // Repeated calls to RecordCookieAccess are ignored until the cookie is
+  // observed as deleted, or the set of seen cookies is cleared via
+  // ClearSeenCookies.
+  class CookieAccessHelper : public base::CheckedObserver {
+   public:
+    explicit CookieAccessHelper(AccessContextAuditService* service);
+    ~CookieAccessHelper() override;
+
+    // Selectively forwards cookie accesses to the audit service based on
+    // whether this helper has previously seen the cookie.
+    void RecordCookieAccess(const net::CookieList& accessed_cookies,
+                            const url::Origin& top_frame_origin);
+
+    // Observer method called by the audit service when a cookie has been
+    // deleted and future accesses should be reported.
+    void OnCookieDeleted(const net::CanonicalCookie& cookie);
+
+    // Resets the internal set of seen cookies, resulting in future reported
+    // accesses to those cookies being forwarded to the service for recording.
+    // This should be called at least prior to every top-frame navigation,
+    // calling more frequently increases accuracy of access timestamps but also
+    // increases performance overhead.
+    void ClearSeenCookies();
+
+   private:
+    AccessContextAuditService* service_;
+    canonical_cookie::CookieHashSet seen_cookies_;
+    ScopedObserver<AccessContextAuditService, CookieAccessHelper>
+        deletion_observer_{this};
+  };
+
   explicit AccessContextAuditService(Profile* profile);
   ~AccessContextAuditService() override;
 
@@ -37,10 +75,6 @@ class AccessContextAuditService
             history::HistoryService* history_service,
             content::StoragePartition* storage_partition);
 
-  // Records accesses for all cookies in |details| against |top_frame_origin|.
-  void RecordCookieAccess(const net::CookieList& accessed_cookies,
-                          const url::Origin& top_frame_origin);
-
   // Records access for |storage_origin|'s storage of |type| against
   // |top_frame_origin|.
   void RecordStorageAPIAccess(const url::Origin& storage_origin,
@@ -89,8 +123,20 @@ class AccessContextAuditService
 
  private:
   friend class AccessContextAuditServiceTest;
+  FRIEND_TEST_ALL_PREFIXES(AccessContextAuditServiceTest, CookieRecords);
+  FRIEND_TEST_ALL_PREFIXES(AccessContextAuditServiceTest, ExpiredCookies);
+  FRIEND_TEST_ALL_PREFIXES(AccessContextAuditServiceTest, HistoryDeletion);
+  FRIEND_TEST_ALL_PREFIXES(AccessContextAuditServiceTest, AllHistoryDeletion);
+  FRIEND_TEST_ALL_PREFIXES(AccessContextAuditServiceTest,
+                           TimeRangeHistoryDeletion);
+  FRIEND_TEST_ALL_PREFIXES(AccessContextAuditServiceTest, OpaqueOrigins);
   FRIEND_TEST_ALL_PREFIXES(AccessContextAuditServiceTest, SessionOnlyRecords);
 
+  // Records accesses for all cookies in |details| against |top_frame_origin|.
+  // Should only be accessed via the CookieAccessHelper.
+  void RecordCookieAccess(const net::CookieList& accessed_cookies,
+                          const url::Origin& top_frame_origin);
+
   // Removes any records which are session only from the database.
   void ClearSessionOnlyRecords();
 
@@ -102,6 +148,8 @@ class AccessContextAuditService
   base::Clock* clock_;
   Profile* profile_;
 
+  base::ObserverList<CookieAccessHelper> cookie_access_helpers_;
+
   mojo::Receiver<network::mojom::CookieChangeListener>
       cookie_listener_receiver_{this};
   ScopedObserver<history::HistoryService, history::HistoryServiceObserver>

chrome/browser/content_settings/page_specific_content_settings_delegate.cc

@@ -52,7 +52,18 @@ namespace chrome {
 
 PageSpecificContentSettingsDelegate::PageSpecificContentSettingsDelegate(
     content::WebContents* web_contents)
-    : WebContentsObserver(web_contents) {}
+    : WebContentsObserver(web_contents) {
+#if !defined(OS_ANDROID)
+  auto* access_context_audit_service =
+      AccessContextAuditServiceFactory::GetForProfile(
+          Profile::FromBrowserContext(web_contents->GetBrowserContext()));
+  if (access_context_audit_service) {
+    cookie_access_helper_ =
+        std::make_unique<AccessContextAuditService::CookieAccessHelper>(
+            access_context_audit_service);
+  }
+#endif  // !defined(OS_ANDROID)
+}
 
 PageSpecificContentSettingsDelegate::~PageSpecificContentSettingsDelegate() =
     default;
@@ -202,13 +213,11 @@ void PageSpecificContentSettingsDelegate::OnCacheStorageAccessAllowed(
 void PageSpecificContentSettingsDelegate::OnCookieAccessAllowed(
     const net::CookieList& accessed_cookies) {
 #if !defined(OS_ANDROID)
-  auto* access_context_audit_service =
-      AccessContextAuditServiceFactory::GetForProfile(
-          Profile::FromBrowserContext(web_contents()->GetBrowserContext()));
-  if (access_context_audit_service)
-    access_context_audit_service->RecordCookieAccess(
+  if (cookie_access_helper_) {
+    cookie_access_helper_->RecordCookieAccess(
         accessed_cookies,
         url::Origin::Create(web_contents()->GetLastCommittedURL()));
+  }
 #endif  // !defined(OS_ANDROID)
 }
 
@@ -256,6 +265,15 @@ void PageSpecificContentSettingsDelegate::OnWebDatabaseAccessAllowed(
       web_contents());
 #endif  // !defined(OS_ANDROID)
 }
+void PageSpecificContentSettingsDelegate::DidStartNavigation(
+    content::NavigationHandle* navigation_handle) {
+#if !defined(OS_ANDROID)
+  if (cookie_access_helper_ && navigation_handle->IsInMainFrame() &&
+      !navigation_handle->IsSameDocument()) {
+    cookie_access_helper_->ClearSeenCookies();
+  }
+#endif  // !defined(OS_ANDROID)
+}
 
 void PageSpecificContentSettingsDelegate::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {

chrome/browser/content_settings/page_specific_content_settings_delegate.h

@@ -5,9 +5,14 @@
 #ifndef CHROME_BROWSER_CONTENT_SETTINGS_PAGE_SPECIFIC_CONTENT_SETTINGS_DELEGATE_H_
 #define CHROME_BROWSER_CONTENT_SETTINGS_PAGE_SPECIFIC_CONTENT_SETTINGS_DELEGATE_H_
 
+#include "build/build_config.h"
 #include "chrome/common/custom_handlers/protocol_handler.h"
 #include "components/content_settings/browser/page_specific_content_settings.h"
 
+#if !defined(OS_ANDROID)
+#include "chrome/browser/browsing_data/access_context_audit_service.h"
+#endif  // !defined(OS_ANDROID)
+
 namespace chrome {
 
 class PageSpecificContentSettingsDelegate
@@ -89,6 +94,8 @@ class PageSpecificContentSettingsDelegate
   void OnWebDatabaseAccessAllowed(const url::Origin& origin) override;
 
   // content::WebContentsObserver:
+  void DidStartNavigation(
+      content::NavigationHandle* navigation_handle) override;
   void DidFinishNavigation(
       content::NavigationHandle* navigation_handle) override;
 
@@ -108,6 +115,11 @@ class PageSpecificContentSettingsDelegate
   // The setting on the pending protocol handler registration. Persisted in case
   // the user opens the bubble and makes changes multiple times.
   ContentSetting pending_protocol_handler_setting_ = CONTENT_SETTING_DEFAULT;
+
+#if !defined(OS_ANDROID)
+  std::unique_ptr<AccessContextAuditService::CookieAccessHelper>
+      cookie_access_helper_;
+#endif  // !defined(OS_ANDROID)
 };
 
 }  // namespace chrome