Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 18 commits into from
Jan 18, 2023

Conversation

cfredric
Copy link
Contributor

@cfredric cfredric commented Dec 2, 2022

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.

Leaving the checklist for now, though I'd like to get some feedback before I try to write WPTs:

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


Preview | Diff

@johannhof johannhof linked an issue Dec 6, 2022 that may be closed by this pull request
Copy link
Member

@johannhof johannhof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me, though I'd love to get @annevk's thoughts as well before merging.

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
Copy link
Collaborator

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of this feedback might be duplicative of the Permissions PR as it looks there's a lot of overlapping text.

storage-access.bs Outdated Show resolved Hide resolved
storage-access.bs Outdated Show resolved Hide resolved
storage-access.bs 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
1. Return the [=agent cluster/storage access map=] of |doc|'s [=relevant agent=]'s [=agent cluster=].
Modify the definition of [=source snapshot params=] in the following manner:
1. Add a new member called <dfn for="source snapshot params">has storage access flag</dfn> of type [=boolean=].
1. Add a new member called <dfn for="source snapshot params">environment id</dfn> of type opaque [=string=].
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should just be string or you should define what opaque means for the type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I included "opaque" since this member will contain values from environment/id, which is defined as an opaque string.

storage-access.bs Outdated Show resolved Hide resolved
storage-access.bs Outdated Show resolved Hide resolved
storage-access.bs 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
@johannhof
Copy link
Member

(Edited the checklist to indicate multi-browser support + the Chromium bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1401089)

@johannhof
Copy link
Member

@annevk
Copy link
Collaborator

annevk commented Dec 20, 2022

I think I would have a much easier time reviewing this once #138 lands and this is rebased given the overlap. Given that tests are not ready yet either I think it's okay that this potentially has to wait until early January. I will try to reply to all the questions above though.

storage-access.bs Outdated Show resolved Hide resolved
johannhof added a commit that referenced this pull request Jan 5, 2023
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 #141.

Co-authored-by: Anne van Kesteren <[email protected]>
Copy link
Collaborator

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johannhof and I went over this. It generally looks good, but there's a few loose ends to tie up still.

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
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
@johannhof
Copy link
Member

WPTs coming up in https://crrev.com/c/4117243

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
storage-access.bs Outdated Show resolved Hide resolved
@annevk
Copy link
Collaborator

annevk commented Jan 17, 2023

I was trying to be careful about the particular task source in use.

But that means this task would en up running after the rejection has already happened, right? So in the rejection task the activation wouldn't have been consumed.

I think this looks good now, but maybe @bvandersloot-mozilla can do a final read as there's been quite a bit of back-and-forth.

Copy link
Collaborator

@bvandersloot-mozilla bvandersloot-mozilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dug through the resolved conversations and re-read the diff.
This generally matches the consensus we have on what per-frame means and accomplishes it in simple ways that are consistent with conversations. I'm not super familiar with the new "navigator" stuff still, but reading that spec makes the environment id and cross origin navigation protections look good.

Just one inline comment.

storage-access.bs Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider restricting storage access scope back to per-frame
4 participants