-
Notifications
You must be signed in to change notification settings - Fork 74
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
Reference Permissions Policy for Cross-Origin iframe Support #233
Conversation
I think there's a misconception in the explainer that PP would only be useful for per-page control. It actually supports both iframe and header controls. Assuming that the header feature is not harmful to FedCM, PP is the recommended way of adding new `allow` features, makes it easier for implementers to support and more versatile for web developers to work with.
@samuelgoto @dj2 wdyt? :) |
Oh, and, assuming this change is accepted I'm happy to update the spec as well. Maybe I should have filed an issue for this instead, apologies :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually was going to ask about this (I thought the right way would be Document Policy, but this seems correct if PP actually supports iframes). I agree with this PR :)
@@ -377,31 +377,24 @@ tokens = await cred.refresh({ | |||
|
|||
The IDP calls for `refresh` have not been flushed out yet. | |||
|
|||
#### iframe Support | |||
#### Cross-Origin iframe Support |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Cross-origin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, interesting. So the Policy turns on FedCM for a given page by setting it to 'self'. That doesn't inherit down to iframes. The iframe is then marked up as "allow=fedcm" which enables the policy in that iframe. That makes sense, and this lgtm.
Looks great to me, thanks for the clarification! Accepting and merging (and yes, would love to accept a PR for the spec too if you can get to it!!)!! |
SHA: 33a1bac Reason: push, by @samuelgoto Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
See rationale here: w3c-fedid#233 (comment)
See rationale here: w3c-fedid#233 (comment)
See rationale here: w3c-fedid#233 (comment)
See rationale here: w3c-fedid#233 (comment)
* Use Permissions Policy instead of sameOriginWithAncestors See rationale here: #233 (comment) * Update feature name based on feedback
…id#233) I think there's a misconception in the explainer that PP would only be useful for per-page control. It actually supports both iframe and header controls. Assuming that the header feature is not harmful to FedCM, PP is the recommended way of adding new `allow` features, makes it easier for implementers to support and more versatile for web developers to work with.
) * Use Permissions Policy instead of sameOriginWithAncestors See rationale here: w3c-fedid#233 (comment) * Update feature name based on feedback
I think there's a misconception in the explainer that PP would only be useful for per-page control. It actually supports both iframe and header controls. Assuming that the header feature is not harmful to FedCM, PP is the recommended way of adding new
allow
features, makes it easier for implementers to support and more versatile for web developers to work with.