privacycg / storage-access

The Storage Access API
https://privacycg.github.io/storage-access/
199 stars 26 forks source link

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

Closed johannhof closed 1 year ago

johannhof commented 1 year ago

This is building on top of https://github.com/w3c/permissions/pull/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.


Preview | Diff

annevk commented 1 year ago

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.

johannhof commented 1 year ago

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.

johannhof commented 1 year ago

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

Thanks!

johannhof commented 1 year ago

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 commented 1 year ago

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.

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 commented 1 year ago

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 commented 1 year ago

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

johannhof commented 1 year ago

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

johannhof commented 1 year ago

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

johannhof commented 1 year ago

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

annevk commented 1 year ago

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

johannhof commented 1 year ago

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