Use the Permissions API (closes #121, #32) #138

johannhof · 2022-11-18T14:00:26Z

This is building on top of w3c/permissions#390 to integrate SAA with permissions. It's deleting a lot of old manual state management but doesn't get rid of the (global) storage access map altogether, since that is done in the per-frame change being worked on by @cfredric.

This is a bit WIP just by virtue of the other change not having merged yet. I removed some of the references to the other spec to avoid the build from breaking.

At least two implementers are interested (and none opposed)
Tests are written and can be reviewed and commented upon at:
- This is already tested as much as possible through using test_driver.set_permission in tests. We could follow up with tests that confirm that the storage-access feature is exposed via permissions APIs but that may need a separate spec change.
Implementation bugs are filed:
- Chromium: https://bugs.chromium.org/p/chromium/issues/detail?id=1401266
- Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=1805860
- WebKit: https://bugs.webkit.org/show_bug.cgi?id=249381

Preview | Diff

storage-access.bs

annevk · 2022-11-22T11:14:32Z

How would this not expose "storage-access" to permissions.query()? As I mentioned before we shouldn't regress on the goals of #120 so that ought to be tackled at the same time.

storage-access.bs

johannhof · 2022-11-22T11:19:51Z

How would this not expose "storage-access" to permissions.query()? As I mentioned before we shouldn't regress on the goals of #120 so that ought to be tackled at the same time.

I had hoped to tackle this problem as a separate follow-up but yeah, I can make a PR for permissions to add support for that and then we can add it to this PR.

storage-access.bs

johannhof · 2022-12-12T12:06:21Z

@annevk if you're okay with this PR I think it would be good to merge given that the Permissions changes merged.

Thanks!

annevk

This mostly seems okay, but there's a couple things that are somewhat questionable which I tried to call out in inline comments.

storage-access.bs

annevk · 2022-12-13T15:44:54Z

storage-access.bs

-1. [=Queue a global task=] on the [=permission task source=] given |global| to [=/reject=] |p| with a "{{NotAllowedError}}" {{DOMException}}.
-
-ISSUE: [since this is UA-defined, does it make sense to follow-up separately with a user prompt?](https://github.com/privacycg/storage-access/pull/24#discussion_r408784492)
+1. If |doc|'s {{Window}} object has [=transient activation=], [=consume user activation=] with it.


This should be consumed as part of the task in the next step. (Although maybe we should also point to the open issue regarding this.)

Yeah, let's do this separately from this PR, it's not really related.

Did you file an issue for this?

Ah, what I meant was that #141 is moving consumption anyway and that we should do that in that PR. I'll add a note there.

storage-access.bs

annevk

I'd just like to note on the record that there are still many issues with this Permissions integration, which actually makes it hard to tell if any of them are critical in some manner.

storage-access.bs

johannhof · 2022-12-14T15:17:36Z

I'd just like to note on the record that there are still many issues with this Permissions integration, which actually makes it hard to tell if any of them are critical in some manner.

Ok, can we list them out for a moment (also to figure out if this PR needs to address any of them)? I'm aware of #144 and #143 (missing WPTs for observing permissions). What else were you thinking of?

annevk · 2022-12-14T15:24:39Z

The things I notice are mainly editorial, but given there's many it makes me worried I'm overlooking other things. It's not just this PR though.

There's a lot of state changes in "in parallel" sections that should be inside tasks.
We're inconsistent on linking terms such as reject/resolve.
We sometimes return early but then still use "Otherwise".

I should make time to do a pass at one point.

Looking at the PR again I wonder if we were firmly settled on using origin for the embedded website?

johannhof · 2022-12-14T15:35:11Z

Looking at the PR again I wonder if we were firmly settled on using origin for the embedded website?

So, I think we said that we could use origin for now but note that user agents have the liberty to auto-grant requests that have existing permissions with same-site embedees. Thinking about this now I think I actually prefer the (site, site) boundary now, as that would allow observers from adjacent same-site iframes to see the permission grant.

Depending on what @bvandersloot-mozilla thinks we could move to (site, site), but maybe we should do this in a follow-up to have the decision recorded explicitly?

bvandersloot-mozilla · 2022-12-14T19:37:34Z

I'm okay with setting (site, site) now, given the other discussions on preserving origin boundaries for security.

johannhof · 2022-12-15T09:06:43Z

Ok, filed #147 for (site, site) and happy to follow-up with that.

johannhof · 2022-12-15T09:31:45Z

I filed https://bugs.chromium.org/p/chromium/issues/detail?id=1401266 for Chromium supporting and WPTesting the Permissions API for storage-access.

johannhof · 2022-12-15T10:36:00Z

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1805860 and https://bugs.webkit.org/show_bug.cgi?id=249381 as well!

annevk

I'm okay with merging this, modulo follow-ups and renaming of the permission key tuple items.

annevk · 2022-12-20T14:22:48Z

storage-access.bs

-1. [=Queue a global task=] on the [=permission task source=] given |global| to [=/reject=] |p| with a "{{NotAllowedError}}" {{DOMException}}.
-
-ISSUE: [since this is UA-defined, does it make sense to follow-up separately with a user prompt?](https://github.com/privacycg/storage-access/pull/24#discussion_r408784492)
+1. If |doc|'s {{Window}} object has [=transient activation=], [=consume user activation=] with it.


Did you file an issue for this?

annevk · 2022-12-20T14:24:44Z

storage-access.bs

+    1. Set |status|'s {{PermissionStatus/state}} to |permissionDesc|'s [=permission state=].
+    1. If |status|'s {{PermissionStatus/state}} is [=permission/denied=], set |status|'s {{PermissionStatus/state}} to [=permission/prompt=].
+
+        Note: The "denied" permission state is not revealed to avoid exposing the user's decision to developers. This is done to prevent retaliation against the user and repeated prompting to the detriment of the user experience.


Let's have a follow-up issue on this as we will to some extent expose denied to websites anyway (but they won't know whether it's persisted).

You were thinking of #149, right?

storage-access.bs

annevk · 2022-12-20T14:29:42Z

(I seem to not have sufficient access to unresolve an issue. It's the first of the three threads above.)

Co-authored-by: Anne van Kesteren <annevk@annevk.nl>

johannhof · 2023-01-05T12:38:52Z

I'll merge this now but will leave the follow-up bits unresolved in case you had something else in mind. :)

This is an attempt to migrate to a "per-frame" model, as discussed in #122. This is built on top of #138 as a starting point. The approach is to define a flag that lives on environment, and is set by document.requestStorageAccess and read by document.hasStorageAccess. In order to propagate storage access during self-initiated, same-origin navigations, we also add a flag to the source snapshot params used during create navigation params by fetching, and conditionally copy the sourceDocument's relevant settings object's flag over to the new environment that will be created. This should let us achieve the benefits of the BrowsingContext approach discussed in #122, without having to add and clear state in BrowsingContext. Co-authored-by: Johann Hofmann <johannhof@chromium.org>

johannhof requested a review from annevk November 18, 2022 14:00

miketaylr reviewed Nov 18, 2022

View reviewed changes

storage-access.bs Outdated Show resolved Hide resolved

annevk reviewed Nov 22, 2022

View reviewed changes

storage-access.bs Outdated Show resolved Hide resolved

johannhof mentioned this pull request Nov 23, 2022

Define a permission store (closes #384) w3c/permissions#390

Merged

cfredric mentioned this pull request Dec 2, 2022

Modify Storage Access to use a "per-frame" model #141

Merged

3 tasks

This was linked to issues Dec 6, 2022

Integrate with the Permissions API or its model? #32

Closed

Consider building on top of Permissions instead of using the "storage access map" #121

Closed

johannhof commented Dec 12, 2022

View reviewed changes

storage-access.bs Show resolved Hide resolved

johannhof requested a review from annevk December 12, 2022 12:05

annevk mentioned this pull request Dec 13, 2022

permissions.query() tests #143

Closed

annevk reviewed Dec 13, 2022

View reviewed changes

johannhof requested a review from annevk December 14, 2022 14:01

annevk reviewed Dec 14, 2022

View reviewed changes

storage-access.bs Outdated Show resolved Hide resolved

johannhof mentioned this pull request Dec 15, 2022

Synchronous hasStorageAccess? #146

Closed

johannhof requested a review from annevk December 15, 2022 09:23

annevk approved these changes Dec 20, 2022

View reviewed changes

johannhof mentioned this pull request Dec 20, 2022

Should we expose "denied" storage access permissions? #149

Open

johannhof added 2 commits January 5, 2023 12:25

Use the Permissions API

a8db407

Better permissions reference, fix other references

e4c2e6a

johannhof and others added 8 commits January 5, 2023 12:25

Forgot another reference

c5cb2d1

Define as algorithms on the feature instead

47ddecf

Add permission key type

3d7532a

Updates for latest permission spec

eba4482

Update storage-access.bs

258d2cf

Co-authored-by: Anne van Kesteren <annevk@annevk.nl>

Address review feedback

499f244

Compat for hSA, add note about (site,site)

2411f8d

Rebase, rename variables

6a6a93a

johannhof force-pushed the permissions-api branch from 54d4e09 to 6a6a93a Compare January 5, 2023 12:36

johannhof merged commit a20c3cc into privacycg:main Jan 5, 2023

This was referenced Jan 18, 2023

Storage Access API w3ctag/design-reviews#807

Closed

Means to observe availability of storage access #55

Open

tunetheweb mentioned this pull request Nov 25, 2023

Update Storage Access API data mdn/browser-compat-data#21336

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the Permissions API (closes #121, #32) #138

Use the Permissions API (closes #121, #32) #138

johannhof commented Nov 18, 2022 •

edited by pr-preview bot

Loading

annevk commented Nov 22, 2022

johannhof commented Nov 22, 2022

johannhof commented Dec 12, 2022

annevk left a comment

annevk Dec 13, 2022

johannhof Dec 14, 2022

annevk Dec 20, 2022

johannhof Jan 5, 2023

annevk left a comment

johannhof commented Dec 14, 2022

annevk commented Dec 14, 2022

johannhof commented Dec 14, 2022

bvandersloot-mozilla commented Dec 14, 2022

johannhof commented Dec 15, 2022

johannhof commented Dec 15, 2022

johannhof commented Dec 15, 2022

annevk left a comment

annevk Dec 20, 2022

annevk Dec 20, 2022

johannhof Jan 5, 2023

annevk commented Dec 20, 2022

johannhof commented Jan 5, 2023

Use the Permissions API (closes #121, #32) #138

Use the Permissions API (closes #121, #32) #138

Conversation

johannhof commented Nov 18, 2022 • edited by pr-preview bot Loading

annevk commented Nov 22, 2022

johannhof commented Nov 22, 2022

johannhof commented Dec 12, 2022

annevk left a comment

Choose a reason for hiding this comment

annevk Dec 13, 2022

Choose a reason for hiding this comment

johannhof Dec 14, 2022

Choose a reason for hiding this comment

annevk Dec 20, 2022

Choose a reason for hiding this comment

johannhof Jan 5, 2023

Choose a reason for hiding this comment

annevk left a comment

Choose a reason for hiding this comment

johannhof commented Dec 14, 2022

annevk commented Dec 14, 2022

johannhof commented Dec 14, 2022

bvandersloot-mozilla commented Dec 14, 2022

johannhof commented Dec 15, 2022

johannhof commented Dec 15, 2022

johannhof commented Dec 15, 2022

annevk left a comment

Choose a reason for hiding this comment

annevk Dec 20, 2022

Choose a reason for hiding this comment

annevk Dec 20, 2022

Choose a reason for hiding this comment

johannhof Jan 5, 2023

Choose a reason for hiding this comment

annevk commented Dec 20, 2022

johannhof commented Jan 5, 2023

johannhof commented Nov 18, 2022 •

edited by pr-preview bot

Loading