Skip to content

Commit

Permalink
iwa: StoragePartition::OnClearSiteData to pass StoragePartition along
Browse files Browse the repository at this point in the history 
When StoragePartition::OnClearSiteData() is called, the call
takes the follow route:
> StoragePartition::OnClearSiteData()
> ClearSiteDataHandler::HandleHeader()
> ClearSiteData()
> BrowsingDataRemoverImpl::RemoveWithFilterAndReply()
> StoragePartition::ClearData()

However, any StoragePartition calls OnClearSiteData() will eventually
result in default StoragePartition calling ClearData().

This chain of changes attempts to fix this by passing the
StoragePartition along to have the final ClearData() call called on the
correct StoragePartition.

We are also updating the definition of DATA_TYPE_ON_STORAGE_PARTITION
because only subsets of DATA_TYPE_ON_STORAGE_PARTITION are allowed by
BrowsingDataRemover to be the mask when a StoragePartition is specified
in the filter.

Bug: 1449362
Change-Id: I766d02a89ee636321625e30b80b1f38f15885e74
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4770335
Reviewed-by: Robbie McElrath <rmcelrath@chromium.org>
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Commit-Queue: Zelin Liu <zelin@chromium.org>
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1188471}
  • Loading branch information
Zelin Liu authored and Chromium LUCI CQ committed Aug 25, 2023
1 parent de38d36 commit fb01fe6
Show file tree
Hide file tree
Showing 7 changed files with 175 additions and 74 deletions.
26 changes: 15 additions & 11 deletions content/browser/browsing_data/clear_site_data_handler.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,12 @@
#include "base/strings/stringprintf.h"
#include "content/browser/buckets/bucket_utils.h"
#include "content/public/browser/browser_context.h"
#include "content/public/browser/storage_partition_config.h"
#include "content/public/browser/web_contents.h"
#include "net/base/load_flags.h"
#include "net/url_request/clear_site_data.h"
#include "services/network/public/cpp/is_potentially_trustworthy.h"
#include "third_party/abseil-cpp/absl/types/optional.h"
#include "third_party/blink/public/common/features_generated.h"

namespace content {
Expand Down Expand Up @@ -121,18 +123,19 @@ void ClearSiteDataHandler::ConsoleMessagesDelegate::
void ClearSiteDataHandler::HandleHeader(
base::RepeatingCallback<BrowserContext*()> browser_context_getter,
base::RepeatingCallback<WebContents*()> web_contents_getter,
const StoragePartitionConfig& storage_partition_config,
const GURL& url,
const std::string& header_value,
int load_flags,
const absl::optional<net::CookiePartitionKey>& cookie_partition_key,
const absl::optional<blink::StorageKey>& storage_key,
const absl::optional<net::CookiePartitionKey> cookie_partition_key,
const absl::optional<blink::StorageKey> storage_key,
bool partitioned_state_allowed_only,
base::OnceClosure callback) {
ClearSiteDataHandler handler(browser_context_getter, web_contents_getter, url,
header_value, load_flags, cookie_partition_key,
storage_key, partitioned_state_allowed_only,
std::move(callback),
std::make_unique<ConsoleMessagesDelegate>());
ClearSiteDataHandler handler(
browser_context_getter, web_contents_getter, storage_partition_config,
url, header_value, load_flags, cookie_partition_key, storage_key,
partitioned_state_allowed_only, std::move(callback),
std::make_unique<ConsoleMessagesDelegate>());
handler.HandleHeaderAndOutputConsoleMessages();
}

Expand All @@ -151,16 +154,18 @@ bool ClearSiteDataHandler::ParseHeaderForTesting(
ClearSiteDataHandler::ClearSiteDataHandler(
base::RepeatingCallback<BrowserContext*()> browser_context_getter,
base::RepeatingCallback<WebContents*()> web_contents_getter,
const StoragePartitionConfig& storage_partition_config,
const GURL& url,
const std::string& header_value,
int load_flags,
const absl::optional<net::CookiePartitionKey>& cookie_partition_key,
const absl::optional<blink::StorageKey>& storage_key,
const absl::optional<net::CookiePartitionKey> cookie_partition_key,
const absl::optional<blink::StorageKey> storage_key,
bool partitioned_state_allowed_only,
base::OnceClosure callback,
std::unique_ptr<ConsoleMessagesDelegate> delegate)
: browser_context_getter_(browser_context_getter),
web_contents_getter_(web_contents_getter),
storage_partition_config_(storage_partition_config),
url_(url),
header_value_(header_value),
load_flags_(load_flags),
Expand Down Expand Up @@ -364,8 +369,7 @@ void ClearSiteDataHandler::ExecuteClearingTask(
const ClearSiteDataTypeSet clear_site_data_types,
const std::set<std::string>& storage_buckets_to_remove,
base::OnceClosure callback) {
ClearSiteData(browser_context_getter_,
/*storage_partition_config=*/absl::nullopt, origin,
ClearSiteData(browser_context_getter_, storage_partition_config_, origin,
clear_site_data_types, storage_buckets_to_remove,
/*avoid_closing_connections=*/true, cookie_partition_key_,
storage_key_, partitioned_state_allowed_only_,
Expand Down
19 changes: 15 additions & 4 deletions content/browser/browsing_data/clear_site_data_handler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@
#include "base/time/time.h"
#include "content/common/content_export.h"
#include "content/public/browser/clear_site_data_utils.h"
#include "content/public/browser/storage_partition_config.h"
#include "net/cookies/cookie_partition_key.h"
#include "third_party/abseil-cpp/absl/types/optional.h"
#include "third_party/blink/public/common/storage_key/storage_key.h"
#include "third_party/blink/public/mojom/devtools/console_message.mojom.h"
#include "url/gurl.h"
Expand Down Expand Up @@ -79,11 +81,12 @@ class CONTENT_EXPORT ClearSiteDataHandler {
static void HandleHeader(
base::RepeatingCallback<BrowserContext*()> browser_context_getter,
base::RepeatingCallback<WebContents*()> web_contents_getter,
const StoragePartitionConfig& storage_partition_config,
const GURL& url,
const std::string& header_value,
int load_flags,
const absl::optional<net::CookiePartitionKey>& cookie_partition_key,
const absl::optional<blink::StorageKey>& storage_key,
const absl::optional<net::CookiePartitionKey> cookie_partition_key,
const absl::optional<blink::StorageKey> storage_key,
bool partitioned_state_allowed_only,
base::OnceClosure callback);

Expand All @@ -99,11 +102,12 @@ class CONTENT_EXPORT ClearSiteDataHandler {
ClearSiteDataHandler(
base::RepeatingCallback<BrowserContext*()> browser_context_getter,
base::RepeatingCallback<WebContents*()> web_contents_getter,
const StoragePartitionConfig& storage_partition_config,
const GURL& url,
const std::string& header_value,
int load_flags,
const absl::optional<net::CookiePartitionKey>& cookie_partition_key,
const absl::optional<blink::StorageKey>& storage_key,
const absl::optional<net::CookiePartitionKey> cookie_partition_key,
const absl::optional<blink::StorageKey> storage_key,
bool partitioned_state_allowed_only,
base::OnceClosure callback,
std::unique_ptr<ConsoleMessagesDelegate> delegate);
Expand Down Expand Up @@ -149,6 +153,10 @@ class CONTENT_EXPORT ClearSiteDataHandler {
// Run the callback to resume loading. No clearing actions were conducted.
void RunCallbackNotDeferred();

const StoragePartitionConfig& StoragePartitionConfigForTesting() const {
return storage_partition_config_;
}

const GURL& GetURLForTesting();

const absl::optional<net::CookiePartitionKey> CookiePartitionKeyForTesting()
Expand All @@ -169,6 +177,9 @@ class CONTENT_EXPORT ClearSiteDataHandler {
base::RepeatingCallback<BrowserContext*()> browser_context_getter_;
base::RepeatingCallback<WebContents*()> web_contents_getter_;

// The config for the target storage partition which stores the data.
const StoragePartitionConfig storage_partition_config_;

// Target URL whose data will be cleared.
const GURL url_;

Expand Down
71 changes: 48 additions & 23 deletions content/browser/browsing_data/clear_site_data_handler_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include "content/public/browser/content_browser_client.h"
#include "content/public/browser/network_service_util.h"
#include "content/public/browser/storage_partition.h"
#include "content/public/browser/storage_partition_config.h"
#include "content/public/browser/storage_usage_info.h"
#include "content/public/browser/web_contents.h"
#include "content/public/test/browser_test.h"
Expand All @@ -48,6 +49,7 @@
#include "net/test/embedded_test_server/http_response.h"
#include "storage/browser/quota/quota_settings.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "third_party/abseil-cpp/absl/types/optional.h"
#include "third_party/blink/public/common/features.h"
#include "third_party/blink/public/common/features_generated.h"
#include "third_party/blink/public/common/storage_key/storage_key.h"
Expand Down Expand Up @@ -86,10 +88,12 @@ class TestBrowsingDataRemoverDelegate : public MockBrowsingDataRemoverDelegate {
// Sets a test expectation that a Clear-Site-Data header call from |origin|,
// instructing to delete |cookies|, |storage|, and |cache|, will schedule
// the corresponding BrowsingDataRemover deletion tasks.
void ExpectClearSiteDataCall(const url::Origin& origin,
bool cookies,
bool storage,
bool cache) {
void ExpectClearSiteDataCall(
const StoragePartitionConfig& storage_partition_config,
const url::Origin& origin,
bool cookies,
bool storage,
bool cache) {
const uint64_t kOriginTypeMask =
BrowsingDataRemover::ORIGIN_TYPE_UNPROTECTED_WEB |
BrowsingDataRemover::ORIGIN_TYPE_PROTECTED_WEB;
Expand All @@ -102,6 +106,8 @@ class TestBrowsingDataRemoverDelegate : public MockBrowsingDataRemoverDelegate {
BrowsingDataFilterBuilderImpl filter_builder(
BrowsingDataFilterBuilder::Mode::kDelete);
filter_builder.AddRegisterableDomain(origin.host());
filter_builder.SetStoragePartitionConfig(storage_partition_config);

ExpectCall(base::Time(), base::Time::Max(), data_type_mask,
kOriginTypeMask, &filter_builder);
}
Expand All @@ -117,15 +123,20 @@ class TestBrowsingDataRemoverDelegate : public MockBrowsingDataRemoverDelegate {
BrowsingDataFilterBuilderImpl filter_builder(
BrowsingDataFilterBuilder::Mode::kDelete);
filter_builder.AddOrigin(origin);
filter_builder.SetStoragePartitionConfig(storage_partition_config);

ExpectCall(base::Time(), base::Time::Max(), data_type_mask,
kOriginTypeMask, &filter_builder);
}
}

// A shortcut for the above method, but with only cookies deleted. This is
// useful for most tests that use |kClearCookiesHeader|.
void ExpectClearSiteDataCookiesCall(const url::Origin& origin) {
ExpectClearSiteDataCall(origin, true, false, false);
void ExpectClearSiteDataCookiesCall(
const StoragePartitionConfig& storage_partition_config,
const url::Origin& origin) {
ExpectClearSiteDataCall(storage_partition_config, origin, true, false,
false);
}
};

Expand Down Expand Up @@ -173,6 +184,10 @@ class ClearSiteDataHandlerBrowserTest : public ContentBrowserTest {
return browser_context()->GetDefaultStoragePartition();
}

const StoragePartitionConfig& storage_partition_config() {
return storage_partition()->GetConfig();
}

// Adds a cookie for the |url|. Used in the cookie integration tests.
void AddCookie(const GURL& url,
const absl::optional<net::CookiePartitionKey>&
Expand Down Expand Up @@ -376,7 +391,7 @@ IN_PROC_BROWSER_TEST_F(ClearSiteDataHandlerBrowserTest,

if (mask & (1 << i))
delegate()->ExpectClearSiteDataCookiesCall(
url::Origin::Create(urls[i]));
storage_partition_config(), url::Origin::Create(urls[i]));
}

// Set up redirects between urls 0 --> 1 --> 2.
Expand Down Expand Up @@ -425,7 +440,7 @@ IN_PROC_BROWSER_TEST_F(ClearSiteDataHandlerBrowserTest,

if (mask & (1 << i))
delegate()->ExpectClearSiteDataCookiesCall(
url::Origin::Create(urls[i]));
storage_partition_config(), url::Origin::Create(urls[i]));
}

// Set up redirects between urls 0 --> 1 --> 2.
Expand Down Expand Up @@ -526,13 +541,15 @@ IN_PROC_BROWSER_TEST_F(ClearSiteDataHandlerBrowserTestWithAutoupgradesDisabled,
AddQuery(&secure_page, "html", content_with_secure_image);

// Secure resource on an insecure page does execute Clear-Site-Data.
delegate()->ExpectClearSiteDataCookiesCall(url::Origin::Create(secure_image));
delegate()->ExpectClearSiteDataCookiesCall(storage_partition_config(),
url::Origin::Create(secure_image));

EXPECT_TRUE(NavigateToURL(shell(), secure_page));
delegate()->VerifyAndClearExpectations();

// Secure resource on a secure page does execute Clear-Site-Data.
delegate()->ExpectClearSiteDataCookiesCall(url::Origin::Create(secure_image));
delegate()->ExpectClearSiteDataCookiesCall(storage_partition_config(),
url::Origin::Create(secure_image));

EXPECT_TRUE(NavigateToURL(shell(), secure_page));
delegate()->VerifyAndClearExpectations();
Expand Down Expand Up @@ -574,10 +591,14 @@ IN_PROC_BROWSER_TEST_F(ClearSiteDataHandlerBrowserTest, ServiceWorker) {
// but not by the "/resource_from_sw" fetch. |origin3| and |origin4| prove
// that the number of calls is dependent on the number of network responses,
// i.e. that it isn't always 1 as in the case of |origin1| and |origin2|.
delegate()->ExpectClearSiteDataCookiesCall(url::Origin::Create(origin1));
delegate()->ExpectClearSiteDataCookiesCall(url::Origin::Create(origin4));
delegate()->ExpectClearSiteDataCookiesCall(url::Origin::Create(origin2));
delegate()->ExpectClearSiteDataCookiesCall(url::Origin::Create(origin4));
delegate()->ExpectClearSiteDataCookiesCall(storage_partition_config(),
url::Origin::Create(origin1));
delegate()->ExpectClearSiteDataCookiesCall(storage_partition_config(),
url::Origin::Create(origin4));
delegate()->ExpectClearSiteDataCookiesCall(storage_partition_config(),
url::Origin::Create(origin2));
delegate()->ExpectClearSiteDataCookiesCall(storage_partition_config(),
url::Origin::Create(origin4));

url = https_server()->GetURL("origin1.com", "/anything-in-workers-scope");
AddQuery(&url, "origin1", origin1.spec());
Expand Down Expand Up @@ -647,7 +668,8 @@ IN_PROC_BROWSER_TEST_F(ClearSiteDataHandlerBrowserTest, MAYBE_Credentials) {
AddQuery(&page, "html", content);

if (test_case.should_run)
delegate()->ExpectClearSiteDataCookiesCall(url::Origin::Create(resource));
delegate()->ExpectClearSiteDataCookiesCall(storage_partition_config(),
url::Origin::Create(resource));

EXPECT_TRUE(NavigateToURL(shell(), page));
WaitForTitle(shell(), "done");
Expand Down Expand Up @@ -687,7 +709,8 @@ IN_PROC_BROWSER_TEST_F(ClearSiteDataHandlerBrowserTest, CredentialsOnRedirect) {
"</script></body></html>",
urls[0].spec().c_str());

delegate()->ExpectClearSiteDataCookiesCall(url::Origin::Create(urls[0]));
delegate()->ExpectClearSiteDataCookiesCall(storage_partition_config(),
url::Origin::Create(urls[0]));

GURL page = https_server()->GetURL("origin1.com", "/");
AddQuery(&page, "html", content);
Expand Down Expand Up @@ -721,8 +744,9 @@ IN_PROC_BROWSER_TEST_F(ClearSiteDataHandlerBrowserTest, Types) {
AddQuery(&url, "header", test_case.value);

delegate()->ExpectClearSiteDataCall(
url::Origin::Create(url), test_case.remove_cookies,
test_case.remove_storage, test_case.remove_cache);
storage_partition_config(), url::Origin::Create(url),
test_case.remove_cookies, test_case.remove_storage,
test_case.remove_cache);

EXPECT_TRUE(NavigateToURL(shell(), url));

Expand Down Expand Up @@ -917,8 +941,8 @@ IN_PROC_BROWSER_TEST_F(ClearSiteDataHandlerBrowserTest,
GURL url = embedded_test_server()->GetURL("127.0.0.1", "/");
AddQuery(&url, "file", "worker_test.html");
EXPECT_TRUE(NavigateToURL(shell(), url));
delegate()->ExpectClearSiteDataCall(url::Origin::Create(url), false, true,
false);
delegate()->ExpectClearSiteDataCall(
storage_partition_config(), url::Origin::Create(url), false, true, false);
SetClearSiteDataHeader("\"storage\"");
EXPECT_FALSE(RunScriptAndGetBool("installServiceWorker()"));
delegate()->VerifyAndClearExpectations();
Expand All @@ -944,8 +968,8 @@ IN_PROC_BROWSER_TEST_F(ClearSiteDataHandlerBrowserTest,
EXPECT_TRUE(RunScriptAndGetBool("installServiceWorker()"));
delegate()->VerifyAndClearExpectations();
// Update the service worker and send C-S-D during update.
delegate()->ExpectClearSiteDataCall(url::Origin::Create(url), false, true,
false);
delegate()->ExpectClearSiteDataCall(
storage_partition_config(), url::Origin::Create(url), false, true, false);

base::RunLoop loop;
auto* remover = browser_context()->GetBrowsingDataRemover();
Expand Down Expand Up @@ -1087,7 +1111,8 @@ IN_PROC_BROWSER_TEST_F(ClearSiteDataHandlerSharedStorageBrowserTest,
EXPECT_EQ(15, tester.GetSharedStorageTotalEntries());

// Let Clear-Site-Data delete the shared storage of "origin1.com".
delegate()->ExpectClearSiteDataCall(kOrigin1, /*cookies=*/false,
delegate()->ExpectClearSiteDataCall(storage_partition_config(), kOrigin1,
/*cookies=*/false,
/*storage=*/true, /*cache=*/false);
AddQuery(&url1, "header", "\"storage\"");
EXPECT_TRUE(NavigateToURL(shell(), url1));
Expand Down

0 comments on commit fb01fe6

Please sign in to comment.