[conversions] Clear storage data on history deletion

What:
Delete Conversion Measurement API data when an origin is completely
deleted from history.

Why:
This avoids a situation where the conversions database is the only
place where any record of an origin is kept.

There is no risk of web breakage by clearing API data, as the storage
is not directly web observable. In contrast to other site data like
cookies which stands the risk of breaking the web.

How:
Adds a new flow in BrowsingDataHistoryObserverService which clears
StoragePartition data when an origin is completely deleted from
history.

Bug: 1108973
Change-Id: I5db807d965b654aea2a166080f48f9f690d8ec28
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2493707
Commit-Queue: John Delaney <johnidel@chromium.org>
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#826412}

chrome/browser/browsing_data/browsing_data_history_observer_service.cc

@@ -13,6 +13,9 @@
 #include "chrome/common/buildflags.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "components/search_engines/template_url_service.h"
+#include "content/public/browser/browsing_data_filter_builder.h"
+#include "content/public/browser/storage_partition.h"
+#include "storage/browser/quota/special_storage_policy.h"
 
 #if BUILDFLAG(ENABLE_SESSION_SERVICE)
 #include "chrome/browser/sessions/session_service_factory.h"
@@ -77,18 +80,71 @@ void DeleteTemplateUrlsForDeletedOrigins(TemplateURLService* keywords_model,
       base::Time::Min(), base::Time::Max());
 }
 
-void DeleteTemplateUrls(TemplateURLService* keywords_model,
-                        const history::DeletionInfo& deletion_info) {
-  if (deletion_info.time_range().IsValid()) {
-    DeleteTemplateUrlsForTimeRange(keywords_model,
-                                   deletion_info.time_range().begin(),
-                                   deletion_info.time_range().end());
-  } else {
-    auto deleted_origins =
-        GetDeletedOrigins(deletion_info.deleted_urls_origin_map());
-    DeleteTemplateUrlsForDeletedOrigins(keywords_model,
-                                        std::move(deleted_origins));
+bool DoesOriginMatchPredicate(
+    base::OnceCallback<bool(const url::Origin&)> predicate,
+    const url::Origin& origin,
+    storage::SpecialStoragePolicy* policy) {
+  if (!std::move(predicate).Run(origin))
+    return false;
+
+  if (policy && policy->IsStorageProtected(origin.GetURL()))
+    return false;
+
+  return true;
+}
+
+void DeleteStoragePartitionDataWithFilter(
+    content::StoragePartition* storage_partition,
+    std::unique_ptr<content::BrowsingDataFilterBuilder> filter_builder,
+    base::Time delete_begin,
+    base::Time delete_end) {
+  content::StoragePartition::OriginMatcherFunction matcher_function =
+      filter_builder ? base::BindRepeating(&DoesOriginMatchPredicate,
+                                           filter_builder->BuildOriginFilter())
+                     : base::NullCallback();
+
+  const uint32_t removal_mask =
+      content::StoragePartition::REMOVE_DATA_MASK_CONVERSIONS;
+  const uint32_t quota_removal_mask = 0;
+  storage_partition->ClearData(
+      removal_mask, quota_removal_mask, std::move(matcher_function),
+      nullptr /* cookie_deletion_filter */, false /* perform_storage_cleanup */,
+      delete_begin, delete_end, base::DoNothing());
+}
+
+void DeleteStoragePartitionDataForTimeRange(
+    content::StoragePartition* storage_partition,
+    base::Time delete_begin,
+    base::Time delete_end,
+    const base::Optional<std::set<GURL>>& urls) {
+  std::unique_ptr<content::BrowsingDataFilterBuilder> filter_builder = nullptr;
+  if (urls) {
+    filter_builder = content::BrowsingDataFilterBuilder::Create(
+        content::BrowsingDataFilterBuilder::Mode::kDelete);
+    for (const auto& url : *urls) {
+      url::Origin origin = url::Origin::Create(url);
+      if (!origin.opaque())
+        filter_builder->AddOrigin(std::move(origin));
+    }
   }
+
+  DeleteStoragePartitionDataWithFilter(
+      storage_partition, std::move(filter_builder), delete_begin, delete_end);
+}
+
+void DeleteStoragePartitionDataForDeletedOrigins(
+    content::StoragePartition* storage_partition,
+    const base::flat_set<GURL>& deleted_origins) {
+  auto filter_builder = content::BrowsingDataFilterBuilder::Create(
+      content::BrowsingDataFilterBuilder::Mode::kDelete);
+  for (const auto& url : deleted_origins) {
+    url::Origin origin = url::Origin::Create(url);
+    if (!origin.opaque())
+      filter_builder->AddOrigin(std::move(origin));
+  }
+  DeleteStoragePartitionDataWithFilter(storage_partition,
+                                       std::move(filter_builder), base::Time(),
+                                       base::Time::Max());
 }
 
 }  // namespace
@@ -114,8 +170,44 @@ void BrowsingDataHistoryObserverService::OnURLsDeleted(
   TemplateURLService* keywords_model =
       TemplateURLServiceFactory::GetForProfile(profile_);
 
-  if (keywords_model)
-    DeleteTemplateUrls(keywords_model, deletion_info);
+  content::StoragePartition* storage_partition =
+      storage_partition_for_testing_
+          ? storage_partition_for_testing_
+          : content::BrowserContext::GetDefaultStoragePartition(profile_);
+
+  if (deletion_info.time_range().IsValid()) {
+    if (keywords_model) {
+      DeleteTemplateUrlsForTimeRange(keywords_model,
+                                     deletion_info.time_range().begin(),
+                                     deletion_info.time_range().end());
+    }
+
+    if (storage_partition) {
+      DeleteStoragePartitionDataForTimeRange(
+          storage_partition, deletion_info.time_range().begin(),
+          deletion_info.time_range().end(), deletion_info.restrict_urls());
+    }
+  } else {
+    // If the history deletion did not have a time range, delete data by origin.
+    auto deleted_origins =
+        GetDeletedOrigins(deletion_info.deleted_urls_origin_map());
+
+    if (storage_partition) {
+      DeleteStoragePartitionDataForDeletedOrigins(storage_partition,
+                                                  deleted_origins);
+    }
+
+    // Move the deleted origins to avoid an expensive copy.
+    if (keywords_model) {
+      DeleteTemplateUrlsForDeletedOrigins(keywords_model,
+                                          std::move(deleted_origins));
+    }
+  }
+}
+
+void BrowsingDataHistoryObserverService::OverrideStoragePartitionForTesting(
+    content::StoragePartition* partition) {
+  storage_partition_for_testing_ = partition;
 }
 
 // static

chrome/browser/browsing_data/browsing_data_history_observer_service.h

@@ -16,7 +16,11 @@ class Profile;
 namespace base {
 template <typename T>
 struct DefaultSingletonTraits;
-}
+}  // namespace base
+
+namespace content {
+class StoragePartition;
+}  // namespace content
 
 // BrowsingDataHistoryObserverService is listening for history deletions to
 // remove navigation, session and recent tab entries.
@@ -47,9 +51,13 @@ class BrowsingDataHistoryObserverService
     bool ServiceIsCreatedWithBrowserContext() const override;
   };
 
+  void OverrideStoragePartitionForTesting(content::StoragePartition* partition);
+
  private:
   Profile* profile_;
 
+  content::StoragePartition* storage_partition_for_testing_ = nullptr;
+
   ScopedObserver<history::HistoryService, history::HistoryServiceObserver>
       history_observer_{this};
 

chrome/browser/browsing_data/browsing_data_history_observer_service_unittest.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/browsing_data/browsing_data_history_observer_service.h"
+
+#include <set>
+#include <utility>
+
+#include "base/optional.h"
+#include "base/time/time.h"
+#include "chrome/test/base/testing_profile.h"
+#include "components/history/core/browser/history_types.h"
+#include "content/public/browser/storage_partition.h"
+#include "content/public/test/browser_task_environment.h"
+#include "content/public/test/test_storage_partition.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+#include "url/origin.h"
+
+namespace {
+
+struct RemovalData {
+  uint32_t removal_mask = 0;
+  uint32_t quota_storage_removal_mask = 0;
+  content::StoragePartition::OriginMatcherFunction origin_matcher;
+  base::Time begin;
+  base::Time end;
+};
+
+class RemovalDataTestStoragePartition : public content::TestStoragePartition {
+ public:
+  RemovalDataTestStoragePartition() = default;
+  ~RemovalDataTestStoragePartition() override = default;
+
+  void ClearData(uint32_t removal_mask,
+                 uint32_t quota_storage_removal_mask,
+                 OriginMatcherFunction origin_matcher,
+                 network::mojom::CookieDeletionFilterPtr cookie_deletion_filter,
+                 bool perform_storage_cleanup,
+                 const base::Time begin,
+                 const base::Time end,
+                 base::OnceClosure callback) override {
+    RemovalData removal_data;
+    removal_data.removal_mask = removal_mask;
+    removal_data.quota_storage_removal_mask = quota_storage_removal_mask;
+    removal_data.origin_matcher = std::move(origin_matcher);
+    removal_data.begin = begin;
+    removal_data.end = end;
+    removal_data_ = removal_data;
+
+    std::move(callback).Run();
+  }
+
+  const base::Optional<RemovalData>& GetRemovalData() { return removal_data_; }
+
+ private:
+  base::Optional<RemovalData> removal_data_;
+};
+
+}  // namespace
+
+class BrowsingDataHistoryObserverServiceTest : public testing::Test {
+ public:
+  BrowsingDataHistoryObserverServiceTest() = default;
+
+ protected:
+  content::BrowserTaskEnvironment task_environment_;
+};
+
+TEST_F(BrowsingDataHistoryObserverServiceTest, AllHistoryDeleted_DataCleared) {
+  TestingProfile profile;
+  BrowsingDataHistoryObserverService service(&profile);
+  RemovalDataTestStoragePartition partition;
+  service.OverrideStoragePartitionForTesting(&partition);
+
+  service.OnURLsDeleted(nullptr /* history_service */,
+                        history::DeletionInfo::ForAllHistory());
+
+  const base::Optional<RemovalData>& removal_data = partition.GetRemovalData();
+  EXPECT_TRUE(removal_data.has_value());
+  EXPECT_EQ(content::StoragePartition::REMOVE_DATA_MASK_CONVERSIONS,
+            removal_data->removal_mask);
+  EXPECT_EQ(0u, removal_data->quota_storage_removal_mask);
+  EXPECT_EQ(base::Time(), removal_data->begin);
+  EXPECT_EQ(base::Time::Max(), removal_data->end);
+
+  // A null origin matcher indicates to remove all origins.
+  EXPECT_TRUE(removal_data->origin_matcher.is_null());
+}
+
+TEST_F(BrowsingDataHistoryObserverServiceTest,
+       OriginCompletelyDeleted_OriginDataCleared) {
+  TestingProfile profile;
+  BrowsingDataHistoryObserverService service(&profile);
+  RemovalDataTestStoragePartition partition;
+  service.OverrideStoragePartitionForTesting(&partition);
+
+  GURL origin_a = GURL("https://a.test");
+  GURL origin_b = GURL("https://b.test");
+
+  // Create a map which shows that `origin_a` is completely deleted and
+  // `origin_b` is not.
+  history::OriginCountAndLastVisitMap origin_map;
+  origin_map[origin_a] = std::make_pair(0, base::Time());
+  origin_map[origin_b] = std::make_pair(1, base::Time());
+
+  history::DeletionInfo deletion_info = history::DeletionInfo::ForUrls(
+      {} /* deleted_rows */, {} /* favicon_urls */);
+  deletion_info.set_deleted_urls_origin_map(std::move(origin_map));
+
+  service.OnURLsDeleted(nullptr /* history_service */, deletion_info);
+
+  const base::Optional<RemovalData>& removal_data = partition.GetRemovalData();
+  EXPECT_TRUE(removal_data.has_value());
+
+  EXPECT_EQ(base::Time(), removal_data->begin);
+  EXPECT_EQ(base::Time::Max(), removal_data->end);
+
+  // Data for `origin_a` should be cleared, but not for `origin_b`.
+  EXPECT_TRUE(removal_data->origin_matcher.Run(
+      url::Origin::Create(origin_a), nullptr /* special_storage_policy */));
+  EXPECT_FALSE(removal_data->origin_matcher.Run(
+      url::Origin::Create(origin_b), nullptr /* special_storage_policy */));
+}
+
+TEST_F(BrowsingDataHistoryObserverServiceTest,
+       TimeRangeHistoryDeleted_DataCleared) {
+  TestingProfile profile;
+  BrowsingDataHistoryObserverService service(&profile);
+  RemovalDataTestStoragePartition partition;
+  service.OverrideStoragePartitionForTesting(&partition);
+
+  base::Time begin = base::Time::Now();
+  base::Time end = begin + base::TimeDelta::FromDays(1);
+  history::DeletionInfo deletion_info(
+      history::DeletionTimeRange(begin, end), false /* is_from_expiration */,
+      {} /* deleted_rows */, {} /* favicon_urls */,
+      base::nullopt /* restrict_urls */);
+
+  service.OnURLsDeleted(nullptr /* history_service */, deletion_info);
+
+  const base::Optional<RemovalData>& removal_data = partition.GetRemovalData();
+  EXPECT_TRUE(removal_data.has_value());
+
+  EXPECT_EQ(begin, removal_data->begin);
+  EXPECT_EQ(end, removal_data->end);
+  EXPECT_TRUE(removal_data->origin_matcher.is_null());
+}
+
+TEST_F(BrowsingDataHistoryObserverServiceTest,
+       TimeRangeHistoryWithRestrictions_DataCleared) {
+  TestingProfile profile;
+  BrowsingDataHistoryObserverService service(&profile);
+  RemovalDataTestStoragePartition partition;
+  service.OverrideStoragePartitionForTesting(&partition);
+
+  GURL origin_a = GURL("https://a.test");
+  GURL origin_b = GURL("https://b.test");
+
+  std::set<GURL> restrict_urls = {origin_a};
+
+  base::Time begin = base::Time::Now();
+  base::Time end = begin + base::TimeDelta::FromDays(1);
+  history::DeletionInfo deletion_info(
+      history::DeletionTimeRange(begin, end), false /* is_from_expiration */,
+      {} /* deleted_rows */, {} /* favicon_urls */,
+      restrict_urls /* restrict_urls */);
+
+  service.OnURLsDeleted(nullptr /* history_service */, deletion_info);
+
+  const base::Optional<RemovalData>& removal_data = partition.GetRemovalData();
+  EXPECT_TRUE(removal_data.has_value());
+
+  EXPECT_EQ(begin, removal_data->begin);
+  EXPECT_EQ(end, removal_data->end);
+  EXPECT_FALSE(removal_data->origin_matcher.is_null());
+
+  // Data for `origin_a` should be cleared, but not for `origin_b`.
+  EXPECT_TRUE(removal_data->origin_matcher.Run(
+      url::Origin::Create(origin_a), nullptr /* special_storage_policy */));
+  EXPECT_FALSE(removal_data->origin_matcher.Run(
+      url::Origin::Create(origin_b), nullptr /* special_storage_policy */));
+}

chrome/test/BUILD.gn

@@ -3370,6 +3370,7 @@ test("unit_tests") {
     "../browser/bluetooth/bluetooth_chooser_context_unittest.cc",
     "../browser/bookmarks/managed_bookmark_service_unittest.cc",
     "../browser/browser_about_handler_unittest.cc",
+    "../browser/browsing_data/browsing_data_history_observer_service_unittest.cc",
     "../browser/browsing_data/browsing_data_media_license_helper_unittest.cc",
     "../browser/browsing_data/browsing_data_quota_helper_unittest.cc",
     "../browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc",

content/browser/browsing_data/browsing_data_filter_builder_impl.cc

@@ -93,10 +93,6 @@ BrowsingDataFilterBuilderImpl::BrowsingDataFilterBuilderImpl(Mode mode)
 BrowsingDataFilterBuilderImpl::~BrowsingDataFilterBuilderImpl() {}
 
 void BrowsingDataFilterBuilderImpl::AddOrigin(const url::Origin& origin) {
-  // TODO(msramek): Optimize OriginFilterBuilder for larger filters if needed.
-  DCHECK_LE(origins_.size(), 10U) << "OriginFilterBuilder is only suitable "
-                                     "for creating small filters.";
-
   // By limiting the filter to non-unique origins, we can guarantee that
   // origin1 < origin2 && origin1 > origin2 <=> origin1.isSameOrigin(origin2).
   // This means that std::set::find() will use the same semantics for
@@ -139,6 +135,12 @@ BrowsingDataFilterBuilderImpl::BuildOriginFilter() {
 
 network::mojom::ClearDataFilterPtr
 BrowsingDataFilterBuilderImpl::BuildNetworkServiceFilter() {
+  // TODO(msramek): Optimize BrowsingDataFilterBuilder for larger filters
+  // if needed.
+  DCHECK_LE(origins_.size(), 10U)
+      << "BrowsingDataFilterBuilder is only suitable for creating "
+         "small network service filters.";
+
   if (MatchesAllOriginsAndDomains())
     return nullptr;
   network::mojom::ClearDataFilterPtr filter =

content/browser/browsing_data/browsing_data_filter_builder_impl.h

@@ -19,15 +19,13 @@ class CONTENT_EXPORT BrowsingDataFilterBuilderImpl
   explicit BrowsingDataFilterBuilderImpl(Mode mode);
   ~BrowsingDataFilterBuilderImpl() override;
 
-  // TODO(csharrison): consider exposing / using this in the //content public
-  // API.
-  base::RepeatingCallback<bool(const url::Origin&)> BuildOriginFilter();
-
   // BrowsingDataFilterBuilder implementation:
   void AddOrigin(const url::Origin& origin) override;
   void AddRegisterableDomain(const std::string& registrable_domain) override;
   bool MatchesAllOriginsAndDomains() override;
   base::RepeatingCallback<bool(const GURL&)> BuildUrlFilter() override;
+  base::RepeatingCallback<bool(const url::Origin&)> BuildOriginFilter()
+      override;
   network::mojom::ClearDataFilterPtr BuildNetworkServiceFilter() override;
   network::mojom::CookieDeletionFilterPtr BuildCookieDeletionFilter() override;
   base::RepeatingCallback<bool(const std::string& site)> BuildPluginFilter()

content/public/browser/browsing_data_filter_builder.h

@@ -23,13 +23,13 @@ class Origin;
 
 namespace content {
 
-// An class that builds GURL->bool predicates to filter browsing data. These
+// A class that builds Origin->bool predicates to filter browsing data. These
 // filters can be of two modes - a list of items to delete or a list of items to
 // preserve, deleting everything else. The filter entries can be origins or
 // registrable domains.
 //
 // This class defines interface to build filters for various kinds of browsing
-// data. |BuildUrlFilter()| is useful for most browsing data storage backends,
+// data. |BuildOriginFilter()| is useful for most browsing data storage backends,
 // but some backends, such as website settings and cookies, use other formats of
 // filter.
 class CONTENT_EXPORT BrowsingDataFilterBuilder {
@@ -62,10 +62,20 @@ class CONTENT_EXPORT BrowsingDataFilterBuilder {
   // Returns true if we're an empty preserve list, where we delete everything.
   virtual bool MatchesAllOriginsAndDomains() = 0;
 
+  // Deprecated: Prefer `BuildOriginFilter()` instead.
   // Builds a filter that matches URLs that are in the list to delete, or aren't
   // in the list to preserve.
+  //
+  // TODO(https://crbug.com/1145270): Migrate usage to BuildOriginFilter() and
+  // remove this function.
   virtual base::RepeatingCallback<bool(const GURL&)> BuildUrlFilter() = 0;
 
+  // Builds a filter that matches origins that are in the list to delete, or
+  // aren't in the list to preserve. This is preferred to BuildUrlFilter() as
+  // it does not inherently perform GURL->Origin conversions.
+  virtual base::RepeatingCallback<bool(const url::Origin&)>
+  BuildOriginFilter() = 0;
+
   // Builds a filter that can be used with the network service. This uses a Mojo
   // struct rather than a predicate function (as used by the rest of the filters
   // built by this class) because we need to be able to pass the filter to the