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

Add permissions patches #74

Merged
merged 29 commits into from
Apr 30, 2023
Merged

Add permissions patches #74

merged 29 commits into from
Apr 30, 2023

Conversation

blu25
Copy link
Collaborator

@blu25 blu25 commented Apr 13, 2023

@blu25 blu25 requested a review from domfarolino April 13, 2023 23:25
@domfarolino
Copy link
Collaborator

Is this ready for my review right now or is it still in draft shape?

@blu25
Copy link
Collaborator Author

blu25 commented Apr 20, 2023

Is this ready for my review right now or is it still in draft shape?

This is ready for your review, but is not ready to be taken out of draft. There are a couple TODOs still in the patches that I need your input on how to handle.

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
@domfarolino
Copy link
Collaborator

There are a couple TODOs still in the patches that I need your input on how to handle.

Can you be more specific with what you need help on?

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
@blu25
Copy link
Collaborator Author

blu25 commented Apr 24, 2023

There are a couple TODOs still in the patches that I need your input on how to handle.

Can you be more specific with what you need help on?

I'm not entirely sure how to get access to the fenced frame config. There are 2 places where algorithms will need access to a fenced frame config object to get the required permissions to load:

  1. the shared document creation infra which, when it calls the create permissions policy algorithm, will have access to the navigation params.
  2. Step 19 in the main fetch algorithm, which will call a new algorithm (specified in this PR) that has access to the request and response object.

The fenced frame config piece isn't fully spec'd out, and I want to make sure that what I'm trying to do is actually feasible/the fenced frame config object is going to be in a place where both algorithms can access them.

@domfarolino
Copy link
Collaborator

I'm not entirely sure how to get access to the fenced frame config.

Let me make sure I understand: these are cases where you are effectively "inside" the fenced frame and you need to access i.e., the "fenced frame properties" implementation-equivalent object in the spec, right? (Basically what we have in DocumentLoader in our implementation inside the FF renderer?) In that case, I think you can do what we do in https://wicg.github.io/fenced-frame/#ref-for-fenced-frame-config-instance%E2%91%A2, including with the XXX box below until I fix that :/

Does that help?

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
@domfarolino
Copy link
Collaborator

Please feel free to mark all previous discussions has "resolved" with the GitHub UI just so I'm sure which ones still need attention vs which ones are indeed addressed.

@domfarolino
Copy link
Collaborator

Let me make sure I understand: these are cases where you are effectively "inside" the fenced frame and you need to access i.e., the "fenced frame properties" implementation-equivalent object in the spec, right? (Basically what we have in DocumentLoader in our implementation inside the FF renderer?) In that case, I think you can do what we do in https://wicg.github.io/fenced-frame/#ref-for-fenced-frame-config-instance%E2%91%A2, including with the XXX box below until I fix that :/

OK this was fixed in #80. Now you can just use https://wicg.github.io/fenced-frame/#navigable-fenced-frame-config-instance as it is referenced elsewhere.

@blu25 blu25 marked this pull request as ready for review April 25, 2023 19:26
@domfarolino
Copy link
Collaborator

@gtanzer Can you take a look at specifically the changes to fenced frame config (+ instance): https://pr-preview.s3.amazonaws.com/WICG/fenced-frame/74/4686c12...7c8a3ba.html#fenced-frame-config-required-permissions-to-load.

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
blu25 and others added 2 commits April 26, 2023 17:42
-split off "create a permissions policy" algorithm into fenced and unfenced versions
- can load check calls unfenced version, keeps rest of algorithm in tact
- modify "create a permissions policy for a navigable from response" step 1 to have a fenced and unfenced if statement
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
Copy link
Collaborator

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

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

I added some more fixes and editorial content. I think this is all ready to go % one single comment!

</div>

<div algorithm=define-inherited-policy-in-container-patches>
Modify the [$Define an inherited policy for feature in container at origin$] algorithm to
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to also modify this algorithm to "fence" the container policy look-up? Right now https://w3c.github.io/webappsec-permissions-policy/#define-inherited-policy-in-container looks at the container policy (which may contain 'none', self', 'src, '*', or an origin) and runs the "matches" algorithm there. I think we'll need to ensure that 'self', 'src', and "an origin" are "fenced" and fail right? Or is there a reason we don't have to do this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Filed #82 so we can just land this PR now.

@domfarolino domfarolino merged commit cc80a57 into master Apr 30, 2023
@domfarolino domfarolino deleted the liam-permissions branch April 30, 2023 12:38
github-actions bot added a commit that referenced this pull request Apr 30, 2023
SHA: cc80a57
Reason: push, by domfarolino

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

3 participants