Let `document.hasStorageAccess` check whether the Document already has unpartitioned data access #174

shuranhuang · 2023-06-13T18:24:22Z

This commits tries to make hSA match the description in the spec that “This specification defines a method to query whether or not a Document currently has access to its unpartitioned data (hasStorageAccess()) …” by including a check of whether the user agent allows the document to access unpartitioned data based on user settings.

At least two implementers are interested (and none opposed)
Tests are written and can be reviewed and commented upon at:
- https://chromium-review.googlesource.com/c/chromium/src/+/4611289
Implementation bugs are filed:
- Chromium: https://bugs.chromium.org/p/chromium/issues/detail?id=1433013
- Gecko: N/A
- WebKit: N/A

Fixes #171

(See WHATWG Working Mode: Changes for more details.)

Preview | Diff

This change is based on spec PR privacycg/storage-access#174. Bug: 1433013 Change-Id: I6c29b2a2afddb288d40d946040dc73fbe76b6fcb

annevk

Thanks for tackling this, but I think this needs a bit more work.

storage-access.bs

…d data access"

This change is based on spec PR privacycg/storage-access#174. Bug: 1433013 Change-Id: I6c29b2a2afddb288d40d946040dc73fbe76b6fcb

annevk

This looks better, but:

The state you grab from the global you could grab directly in the steps of the task. With what you do now you look at a cached value, which I guess theoretically could have changed. If you meant to do that, a note would help.
The user agent settings shouldn't be grabbed as part of the task steps. You want to grab them while in parallel.
Is "unpartitioned data" correct? We're just dealing with cookies here, right?

shuranhuang · 2023-06-15T15:18:38Z

Thanks!
I used "unpartitioned data" here to align with the description of the API definition:

storage-access/storage-access.bs

Line 114 in 69042e6

    
           This specification defines a method to query whether or not a {{Document}} currently has access to its [=unpartitioned data=] ({{Document/hasStorageAccess()}}), and a method that can be used to request access to its [=unpartitioned data=] ({{Document/requestStorageAccess()}}).

. But you are right that we are just dealing with cookies for this change. Changed it to use "unpartitioned cookie" in the latest commit. PTAL!

storage-access.bs

…n hasStorageAccess() This change is based on spec PR privacycg/storage-access#174. Bug: 1433013 Change-Id: I6c29b2a2afddb288d40d946040dc73fbe76b6fcb

storage-access.bs

…n hasStorageAccess() This change is based on spec PR privacycg/storage-access#174. Bug: 1433013 Change-Id: I6c29b2a2afddb288d40d946040dc73fbe76b6fcb

This change is based on spec PR privacycg/storage-access#174. Bug: 1433013 Change-Id: I6c29b2a2afddb288d40d946040dc73fbe76b6fcb

This change is based on spec PR privacycg/storage-access#174. Bug: 1433013 Change-Id: I6c29b2a2afddb288d40d946040dc73fbe76b6fcb Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4611289 Reviewed-by: Mason Freed <[email protected]> Commit-Queue: Shuran Huang <[email protected]> Reviewed-by: Chris Fredrickson <[email protected]> Cr-Commit-Position: refs/heads/main@{#1161766}

storage-access.bs

Co-authored-by: Johann Hofmann <[email protected]>

johannhof

LGTM but would like to wait on @annevk's review before merging.

storage-access.bs

johannhof · 2023-07-06T06:27:16Z

storage-access.bs

+
+        1. If the user agent does not have explicit settings for unpartitioned cookie access for |tuple|, return "none".
+        1. If the user agent's settings explicitly allow unpartitioned cookie access for |tuple|, return "allow".
+        1. If the user agent's settings explicitly disallow unpartitioned cookie access for |tuple|, return "disallow".


I guess "disallow" is unused here now, but we might use it if we re-use this algorithm for rSA later?

That's right. "disallow" is not checked here explicitly, but we can add

If |explicitSetting| is "disallow", [=/resolve=] |p| with false.

if that's clearer/preferred.

Hmm not sure, it seems unnecessary to add there. It's probably fine like this? Maybe @annevk has a preference...

I'm somewhat surprised this has explicit "allow". I thought that was always subject to requestStorageAccess() for child frames?

There are user agent settings that disable 3PC blocking (e.g. in Safari IIRC) or allow-list specific sites (e.g. in Firefox and Chrome), how else would we represent those?

…y check in hasStorageAccess(), a=testonly Automatic update from web-platform-tests Include unpartitioned cookie availability check in hasStorageAccess() This change is based on spec PR privacycg/storage-access#174. Bug: 1433013 Change-Id: I6c29b2a2afddb288d40d946040dc73fbe76b6fcb Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4611289 Reviewed-by: Mason Freed <[email protected]> Commit-Queue: Shuran Huang <[email protected]> Reviewed-by: Chris Fredrickson <[email protected]> Cr-Commit-Position: refs/heads/main@{#1161766} -- wpt-commits: 2cf035c769f58ead31c5c11c737b4b5c6e6aed88 wpt-pr: 40531

…rageAccess() This change is based on spec PR privacycg/storage-access#174. (cherry picked from commit 120b35b) Bug: 1433013 Change-Id: I6c29b2a2afddb288d40d946040dc73fbe76b6fcb Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4611289 Reviewed-by: Mason Freed <[email protected]> Commit-Queue: Shuran Huang <[email protected]> Reviewed-by: Chris Fredrickson <[email protected]> Cr-Original-Commit-Position: refs/heads/main@{#1161766} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4665808 Auto-Submit: Shuran Huang <[email protected]> Reviewed-by: Dominic Farolino <[email protected]> Commit-Queue: Dominic Farolino <[email protected]> Cr-Commit-Position: refs/branch-heads/5845@{#322} Cr-Branched-From: 5a5dff6-refs/heads/main@{#1160321}

…y check in hasStorageAccess(), a=testonly Automatic update from web-platform-tests Include unpartitioned cookie availability check in hasStorageAccess() This change is based on spec PR privacycg/storage-access#174. Bug: 1433013 Change-Id: I6c29b2a2afddb288d40d946040dc73fbe76b6fcb Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4611289 Reviewed-by: Mason Freed <[email protected]> Commit-Queue: Shuran Huang <[email protected]> Reviewed-by: Chris Fredrickson <[email protected]> Cr-Commit-Position: refs/heads/main@{#1161766} -- wpt-commits: 2cf035c769f58ead31c5c11c737b4b5c6e6aed88 wpt-pr: 40531

shuranhuang · 2023-07-10T15:15:55Z

Friendly ping on this @annevk :)

annevk

Didn't we want to make hasStorageAccess() reflect deny states for the top-level document as well? It seems earlier steps of hasStorageAccess() end up returning early with true.

storage-access.bs

annevk · 2023-07-12T13:33:34Z

storage-access.bs

+
+        1. If the user agent does not have explicit settings for unpartitioned cookie access for |tuple|, return "none".
+        1. If the user agent's settings explicitly allow unpartitioned cookie access for |tuple|, return "allow".
+        1. If the user agent's settings explicitly disallow unpartitioned cookie access for |tuple|, return "disallow".


I'm somewhat surprised this has explicit "allow". I thought that was always subject to requestStorageAccess() for child frames?

johannhof · 2023-07-12T14:29:12Z

Didn't we want to make hasStorageAccess() reflect deny states for the top-level document as well? It seems earlier steps of hasStorageAccess() end up returning early with true.

Hmm I don't think that was an expectation on my end. I do see SAA primarily as a mechanism governing cross-site data access, not same-site (or same authority). FWIW I'd be okay with punting on a follow-up.

shuranhuang · 2023-07-21T20:03:21Z

@annevk PTAL the latest version.

…hecked only if user settings do not explicitly allow/disallow

Since we should always take user settings into account. This change is based on spec PR privacycg/storage-access#174. Bug: 1433013 Change-Id: I76a4fe5841c6ac1c566cbb2fa34298832a6f7da0 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4705048 Reviewed-by: Johann Hofmann <[email protected]> Reviewed-by: Chris Fredrickson <[email protected]> Commit-Queue: Shuran Huang <[email protected]> Reviewed-by: Dominic Farolino <[email protected]> Cr-Commit-Position: refs/heads/main@{#1174378}

storage-access.bs

annevk · 2023-07-25T07:30:11Z

storage-access.bs

+        1. If |explicitSetting| is "`disallow`", [=/resolve=] |p| with false.
+        1. If |explicitSetting| is "`allow`", [=/resolve=] |p| with true.
+        1. If |explicitSetting| is "`none`":
+            1. If |browsingContext| is a [=top-level browsing context=], [=/resolve=] |p| with true.


I like this better. Does @johannhof agree with this change as well?

Yes, this still LGTM :)

shuranhuang · 2023-07-27T14:15:40Z

@annevk does it look good with your suggestions applied?

shuranhuang · 2023-08-08T15:07:29Z

@annevk Friendly ping:)

johannhof · 2023-08-21T09:54:12Z

In the interest of moving things along on our graduation goals, I'll go ahead and merge this without @annevk's explicit sign-off, as there's been plenty of review (and a final positive note). I hope that works for you, Anne. I think @shuranhuang is happy to correct any remaining concerns you may have in another PR.

Check whether the context already has unpartitioned data access in hSA

5555beb

shuranhuang marked this pull request as ready for review June 13, 2023 19:36

chromium-wpt-export-bot mentioned this pull request Jun 13, 2023

Include unpartitioned cookie availability check in hasStorageAccess() web-platform-tests/wpt#40531

Merged

annevk reviewed Jun 14, 2023

View reviewed changes

storage-access.bs Outdated Show resolved Hide resolved

storage-access.bs Outdated Show resolved Hide resolved

storage-access.bs Outdated Show resolved Hide resolved

storage-access.bs Outdated Show resolved Hide resolved

storage-access.bs Outdated Show resolved Hide resolved

shuranhuang added 2 commits June 14, 2023 20:07

Add implementation dfn of "whether the user agent allows unpartitione…

d5efb4d

…d data access"

Queue a global task for the algo

de02b69

annevk reviewed Jun 15, 2023

View reviewed changes

Address comments

9191bee

annevk reviewed Jun 15, 2023

View reviewed changes

storage-access.bs Outdated Show resolved Hide resolved

storage-access.bs Outdated Show resolved Hide resolved

Update inline algo definition

88d169e

johannhof reviewed Jun 16, 2023

View reviewed changes

storage-access.bs Outdated Show resolved Hide resolved

Update the "user agent explicitly allows..." part

95261cc

This comment was marked as spam.

Sign in to view

shuranhuang requested review from annevk and johannhof June 21, 2023 14:26

Use (topLevelSite, embeddedSite) tuple for user agent's setting checks.

a8a08cc

johannhof reviewed Jun 29, 2023

View reviewed changes

storage-access.bs Outdated Show resolved Hide resolved

storage-access.bs Outdated Show resolved Hide resolved

storage-access.bs Outdated Show resolved Hide resolved

storage-access.bs Outdated Show resolved Hide resolved

storage-access.bs Outdated Show resolved Hide resolved

annevk mentioned this pull request Jun 29, 2023

hasStorageAccess() deals with promises in parallel #151

Closed

shuranhuang and others added 2 commits June 29, 2023 10:22

Update storage-access.bs

259ede3

Co-authored-by: Johann Hofmann <[email protected]>

Update storage-access.bs

2c68690

Co-authored-by: Johann Hofmann <[email protected]>

shuranhuang and others added 5 commits July 5, 2023 22:22

Update storage-access.bs

7a88f3e

Co-authored-by: Johann Hofmann <[email protected]>

Update storage-access.bs

bd288a1

Co-authored-by: Johann Hofmann <[email protected]>

Update storage-access.bs

9637c1c

Co-authored-by: Johann Hofmann <[email protected]>

Update storage-access.bs

a390040

Co-authored-by: Johann Hofmann <[email protected]>

Update storage-access.bs

f3aaccb

shuranhuang requested a review from johannhof July 6, 2023 02:28

johannhof approved these changes Jul 6, 2023

View reviewed changes

Inline the sub step

8f5dda8

annevk reviewed Jul 12, 2023

View reviewed changes

format string

4ad7a32

shuranhuang requested a review from annevk July 12, 2023 14:48

Remove early return for top-level document and same authority cases

10030f4

Add back top-level and same authority checks and make sure they are c…

3ef5060

…hecked only if user settings do not explicitly allow/disallow

annevk reviewed Jul 25, 2023

View reviewed changes

Resolve comments

024f4b8

shuranhuang requested a review from annevk July 25, 2023 14:31

cfredric mentioned this pull request Aug 1, 2023

Inconsistent request storage Access behavior cfredric/chrome-storage-access-api#4

Closed

johannhof merged commit 139ef58 into privacycg:main Aug 21, 2023

annevk mentioned this pull request Sep 4, 2023

Regression: consulting permission state from a task's steps #182

Closed

shuranhuang mentioned this pull request Oct 13, 2023

document.requestStorageAccess should consider user explicit settings for unpartitioned data access #186

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Let `document.hasStorageAccess` check whether the Document already has unpartitioned data access #174

Let `document.hasStorageAccess` check whether the Document already has unpartitioned data access #174

shuranhuang commented Jun 13, 2023 •

edited by pr-preview bot

Loading

annevk left a comment

annevk left a comment •

edited

Loading

shuranhuang commented Jun 15, 2023

This comment was marked as spam.

johannhof left a comment

johannhof Jul 6, 2023

shuranhuang Jul 6, 2023

johannhof Jul 7, 2023

annevk Jul 12, 2023

johannhof Jul 12, 2023

shuranhuang commented Jul 10, 2023

annevk left a comment

annevk Jul 12, 2023

johannhof commented Jul 12, 2023

shuranhuang commented Jul 21, 2023

annevk Jul 25, 2023

johannhof Jul 25, 2023

shuranhuang commented Jul 27, 2023

shuranhuang commented Aug 8, 2023

johannhof commented Aug 21, 2023

Let document.hasStorageAccess check whether the Document already has unpartitioned data access #174

Let document.hasStorageAccess check whether the Document already has unpartitioned data access #174

Conversation

shuranhuang commented Jun 13, 2023 • edited by pr-preview bot Loading

annevk left a comment

Choose a reason for hiding this comment

annevk left a comment • edited Loading

Choose a reason for hiding this comment

shuranhuang commented Jun 15, 2023

This comment was marked as spam.

johannhof left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shuranhuang commented Jul 10, 2023

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johannhof commented Jul 12, 2023

shuranhuang commented Jul 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shuranhuang commented Jul 27, 2023

shuranhuang commented Aug 8, 2023

johannhof commented Aug 21, 2023

Let `document.hasStorageAccess` check whether the Document already has unpartitioned data access #174

Let `document.hasStorageAccess` check whether the Document already has unpartitioned data access #174

shuranhuang commented Jun 13, 2023 •

edited by pr-preview bot

Loading

annevk left a comment •

edited

Loading