From b41ce75691d068ae5f7aa6289445fd6f898244fe Mon Sep 17 00:00:00 2001 From: Shuran Huang Date: Wed, 5 Jul 2023 16:11:22 +0000 Subject: [PATCH] Show no SAA prompt when 3p cookies are blocked by user explicitly Bug: 1441133 Change-Id: I1457737288c2fca5fb17d875fc5fea031b4d891a Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4658836 Reviewed-by: Dominic Farolino Reviewed-by: Johann Hofmann Commit-Queue: Shuran Huang Auto-Submit: Shuran Huang Cr-Commit-Position: refs/heads/main@{#1165965} --- .../storage_access_api/api_browsertest.cc | 15 +++---------- ...storage_access_grant_permission_context.cc | 16 ++++++++++++++ .../storage_access_grant_permission_context.h | 4 +++- ...ccess_grant_permission_context_unittest.cc | 21 +++++++++++++++++++ .../blink/renderer/core/dom/document.cc | 4 ++++ tools/metrics/histograms/enums.xml | 4 ++-- 6 files changed, 49 insertions(+), 15 deletions(-) diff --git a/chrome/browser/storage_access_api/api_browsertest.cc b/chrome/browser/storage_access_api/api_browsertest.cc index 050312f385bb15..9d96fa88df19a0 100644 --- a/chrome/browser/storage_access_api/api_browsertest.cc +++ b/chrome/browser/storage_access_api/api_browsertest.cc @@ -816,10 +816,7 @@ IN_PROC_BROWSER_TEST_F(StorageAccessAPIBrowserTest, EXPECT_FALSE(storage::test::HasStorageAccessForFrame(GetFrame())); prompt_factory()->set_response_type( permissions::PermissionRequestManager::ACCEPT_ALL); - // TODO(https://crbug.com/1441133): requestStorageAccess() should be rejected - // when 3p cookie is blocked by user explicitly. - EXPECT_TRUE(content::ExecJs(GetFrame(), "document.requestStorageAccess()")); - EXPECT_FALSE(storage::test::HasStorageAccessForFrame(GetFrame())); + EXPECT_FALSE(content::ExecJs(GetFrame(), "document.requestStorageAccess()")); EXPECT_EQ(ReadCookies(GetFrame(), kHostB), NoCookies()); } @@ -841,11 +838,8 @@ IN_PROC_BROWSER_TEST_F( prompt_factory()->set_response_type( permissions::PermissionRequestManager::ACCEPT_ALL); - // TODO(https://crbug.com/1441133): requestStorageAccess() should be rejected - // when 3p cookie is blocked by user explicitly. - EXPECT_TRUE( + EXPECT_FALSE( content::ExecJs(GetNestedFrame(), "document.requestStorageAccess()")); - EXPECT_FALSE(storage::test::HasStorageAccessForFrame(GetNestedFrame())); EXPECT_EQ(ReadCookies(GetNestedFrame(), kHostB), NoCookies()); } @@ -869,11 +863,8 @@ IN_PROC_BROWSER_TEST_F( prompt_factory()->set_response_type( permissions::PermissionRequestManager::ACCEPT_ALL); - // TODO(https://crbug.com/1441133): requestStorageAccess() should be rejected - // when 3p cookie is blocked by user explicitly. - EXPECT_TRUE( + EXPECT_FALSE( content::ExecJs(GetNestedFrame(), "document.requestStorageAccess()")); - EXPECT_FALSE(storage::test::HasStorageAccessForFrame(GetNestedFrame())); EXPECT_EQ(ReadCookies(GetNestedFrame(), kHostC), NoCookies()); } diff --git a/chrome/browser/storage_access_api/storage_access_grant_permission_context.cc b/chrome/browser/storage_access_api/storage_access_grant_permission_context.cc index ca1f3f8f1474b8..33f44e1ce837b9 100644 --- a/chrome/browser/storage_access_api/storage_access_grant_permission_context.cc +++ b/chrome/browser/storage_access_api/storage_access_grant_permission_context.cc @@ -55,6 +55,7 @@ constexpr base::TimeDelta kExplicitGrantDuration = base::Days(30); bool IsImplicitOutcome(RequestOutcome outcome) { switch (outcome) { case RequestOutcome::kAllowedByCookieSettings: + case RequestOutcome::kDeniedByCookieSettings: case RequestOutcome::kDeniedByFirstPartySet: case RequestOutcome::kDeniedByPrerequisites: case RequestOutcome::kDeniedByTopLevelInteractionHeuristic: @@ -79,6 +80,7 @@ bool ShouldDisplayOutcomeInOmnibox(RequestOutcome outcome) { case RequestOutcome::kReusedPreviousDecision: return true; case RequestOutcome::kAllowedByCookieSettings: + case RequestOutcome::kDeniedByCookieSettings: case RequestOutcome::kDeniedByFirstPartySet: case RequestOutcome::kDeniedByTopLevelInteractionHeuristic: case RequestOutcome::kGrantedByAllowance: @@ -134,6 +136,7 @@ content_settings::ContentSettingConstraints ComputeConstraints( case RequestOutcome::kReusedImplicitGrant: case RequestOutcome::kDeniedByTopLevelInteractionHeuristic: case RequestOutcome::kAllowedByCookieSettings: + case RequestOutcome::kDeniedByCookieSettings: NOTREACHED_NORETURN(); case RequestOutcome::kGrantedByUser: case RequestOutcome::kDeniedByUser: @@ -195,6 +198,19 @@ void StorageAccessGrantPermissionContext::DecidePermission( CHECK(requesting_origin.is_valid()); CHECK(embedding_origin.is_valid()); + // Return early without letting SAA override any explicit user settings to + // block 3p cookies. + HostContentSettingsMap* settings_map = + HostContentSettingsMapFactory::GetForProfile(browser_context()); + CHECK(settings_map); + ContentSetting setting = settings_map->GetContentSetting( + requesting_origin, embedding_origin, ContentSettingsType::COOKIES); + if (setting == CONTENT_SETTING_BLOCK) { + RecordOutcomeSample(RequestOutcome::kDeniedByCookieSettings); + std::move(callback).Run(CONTENT_SETTING_BLOCK); + return; + } + content::RenderFrameHost* rfh = content::RenderFrameHost::FromID(id.global_render_frame_host_id()); CHECK(rfh); diff --git a/chrome/browser/storage_access_api/storage_access_grant_permission_context.h b/chrome/browser/storage_access_api/storage_access_grant_permission_context.h index a4bff75bb84f4c..80dc9f4703dba1 100644 --- a/chrome/browser/storage_access_api/storage_access_grant_permission_context.h +++ b/chrome/browser/storage_access_api/storage_access_grant_permission_context.h @@ -51,8 +51,10 @@ enum class RequestOutcome { // The permission was previously granted without user action. E.g. through // FPS. kReusedImplicitGrant = 10, + // 3p cookies are blocked by user explicitly, so there is no need to ask. + kDeniedByCookieSettings = 11, - kMaxValue = kReusedImplicitGrant, + kMaxValue = kDeniedByCookieSettings, }; class StorageAccessGrantPermissionContext diff --git a/chrome/browser/storage_access_api/storage_access_grant_permission_context_unittest.cc b/chrome/browser/storage_access_api/storage_access_grant_permission_context_unittest.cc index 8c89f0573aaccb..02b0f8f40405bb 100644 --- a/chrome/browser/storage_access_api/storage_access_grant_permission_context_unittest.cc +++ b/chrome/browser/storage_access_api/storage_access_grant_permission_context_unittest.cc @@ -464,6 +464,27 @@ TEST_F(StorageAccessGrantPermissionContextAPIEnabledTest, IsEmpty()); } +// When 3p cookie access is blocked by user explicitly, request should be denied +// without prompting. +TEST_F(StorageAccessGrantPermissionContextAPIEnabledTest, + DeniedByCookieSettings) { + HostContentSettingsMap* settings_map = + HostContentSettingsMapFactory::GetForProfile(profile()); + settings_map->SetContentSettingDefaultScope( + GetRequesterURL(), GetTopLevelURL(), ContentSettingsType::COOKIES, + CONTENT_SETTING_BLOCK); + + // User gesture is not needed. + EXPECT_EQ(CONTENT_SETTING_BLOCK, + DecidePermissionSync(/*user_gesture=*/false)); + histogram_tester().ExpectUniqueSample( + kRequestOutcomeHistogram, RequestOutcome::kDeniedByCookieSettings, 1); + + EXPECT_THAT(page_specific_content_settings()->GetTwoSiteRequests( + ContentSettingsType::STORAGE_ACCESS), + IsEmpty()); +} + class StorageAccessGrantPermissionContextAPIWithImplicitGrantsTest : public StorageAccessGrantPermissionContextAPIEnabledTest { public: diff --git a/third_party/blink/renderer/core/dom/document.cc b/third_party/blink/renderer/core/dom/document.cc index 29baac9abd8b0b..daf39009f36a39 100644 --- a/third_party/blink/renderer/core/dom/document.cc +++ b/third_party/blink/renderer/core/dom/document.cc @@ -6451,6 +6451,8 @@ ScriptPromise Document::requestStorageAccess(ScriptState* script_state) { return promise; } + // TODO(https://crbug.com/1441133): we should not return early here since 3p + // cookies could be blocked by user explicitly. if (GetExecutionContext()->GetSecurityOrigin()->IsSameSiteWith( &*TopFrameOrigin())) { FireRequestStorageAccessHistogram( @@ -6486,6 +6488,8 @@ ScriptPromise Document::requestStorageAccess(ScriptState* script_state) { return promise; } + // TODO(https://crbug.com/1441133): we should not return early based on the + // per-frame bit since 3p cookies could be blocked by user explicitly. if (dom_window_->HasStorageAccess()) { FireRequestStorageAccessHistogram( RequestStorageResult::APPROVED_EXISTING_ACCESS); diff --git a/tools/metrics/histograms/enums.xml b/tools/metrics/histograms/enums.xml index 90a3003b21c0a4..5c80198cffc7a2 100644 --- a/tools/metrics/histograms/enums.xml +++ b/tools/metrics/histograms/enums.xml @@ -100944,13 +100944,13 @@ https://www.dmtf.org/sites/default/files/standards/documents/DSP0134_2.7.1.pdf + label="Denied by missing prerequisite (user gesture, feature enabled)"/> +