Skip to content

Commit

Permalink
Show no SAA prompt when 3p cookies are blocked by user explicitly
Browse files Browse the repository at this point in the history
Bug: 1441133
Change-Id: I1457737288c2fca5fb17d875fc5fea031b4d891a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4658836
Reviewed-by: Dominic Farolino <dom@chromium.org>
Reviewed-by: Johann Hofmann <johannhof@chromium.org>
Commit-Queue: Shuran Huang <shuuran@chromium.org>
Auto-Submit: Shuran Huang <shuuran@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1165965}
  • Loading branch information
shuranhuang authored and Chromium LUCI CQ committed Jul 5, 2023
1 parent 473c4c7 commit b41ce75
Show file tree
Hide file tree
Showing 6 changed files with 49 additions and 15 deletions.
15 changes: 3 additions & 12 deletions chrome/browser/storage_access_api/api_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
Expand All @@ -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());
}
Expand All @@ -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());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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:
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
4 changes: 4 additions & 0 deletions third_party/blink/renderer/core/dom/document.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions tools/metrics/histograms/enums.xml
Original file line number Diff line number Diff line change
Expand Up @@ -100944,13 +100944,13 @@ https://www.dmtf.org/sites/default/files/standards/documents/DSP0134_2.7.1.pdf
<int value="3" label="Denied by First-Party Set"/>
<int value="4" label="Denied by user"/>
<int value="5"
label="Denied by missing prerequisite (user gesture, valid requesting
and top-level origins, feature enabled)"/>
label="Denied by missing prerequisite (user gesture, feature enabled)"/>
<int value="6" label="Dismissed by user"/>
<int value="7" label="Reused previous decision (made by user)"/>
<int value="8" label="Denied by top-level user interaction heuristic"/>
<int value="9" label="Access was allowed through cookie settings"/>
<int value="10" label="Reused implicit grant (e.g. from First-Party-Sets)"/>
<int value="11" label="Access was denied through cookie settings"/>
</enum>

<enum name="StorageAccessInputState">
Expand Down

0 comments on commit b41ce75

Please sign in to comment.