Add Permissions Policy integration with "allowed to use" check #59
Conversation
|
This still assumes a single permission model and will be modifed once #17 is resolved. |
christhompson
left a comment
christhompson
left a comment
There was a problem hiding this comment.
Thanks for fleshing out this TODO! Overall this LGTM with a couple comments.
index.bs
Outdated
| 3. Otherwise, prompt the user: | ||
| 1. If the user grants permission, return null. | ||
| 2. If the user denies the permission, return |error|. | ||
| 4. If |request|'s [=request/client=] is null, then return |
There was a problem hiding this comment.
I think this is a strict subset of step 2 above and can be cut.
index.bs
Outdated
| 7. Let |document| be |global|'s [=global object/associated | ||
| Document=]. | ||
|
|
||
| 8. If |document| is null, then return |error|. |
There was a problem hiding this comment.
We'll want to specify the behavior for Workers here at some point, but I think okay to punt.
IIRC for Dedicated Workers and Shared Workers, the associated Document should be well defined (although we might need to traverse up the tree of global objects to get to the owning Window for the case of nested workers?), but for Service Workers the associated Document is only defined if we have a window client. It might be worth flagging this as a TODO here (and that permissions policy isn't supported in workers w3c/webappsec-permissions-policy#207).
(This gives rise to the current behavior in Chromium where we don't even try to get at the service worker's window client, since no such window client may exist if the service worker is running in the background, and instead rely on checking whether the LNA permission has been granted to the service worker's script URL.)
There was a problem hiding this comment.
That's also technically the case for shared workers. We also don't let shared/service workers look up an owning document as it is inherently racy.
There was a problem hiding this comment.
That's also technically the case for shared workers. We also don't let shared/service workers look up an owning document as it is inherently racy.
Right, and I realize that matches Chromium's implementation currently (we get the document for Dedicated Workers but Shared Workers are treated the same as Service Workers).
There's some precedent for doing this on a best-effort basis in Chromium -- we do it for client cert selection for example (looking up by window ID from the worker context, if we have that information).
It seems like there is some desire for this from developers for LNA, even if it doesn't always work. I do feel like we're maybe treading new ground for permissions-in-workers in many ways though, and for now the "must pre-grant the permission to the worker origin" seems "good enough" maybe? Opinions very welcome on this area.
There was a problem hiding this comment.
I would prefer not doing anything magical/new for shared/service workers. We probably have to go through some scenarios to see how to best tackle them.
Resolves issue WICG#37 by adding proper Permissions Policy integration to the Local Network Access check algorithm. Changes: - Replace TODO with normative algorithm steps for permission checking in the Local Network Access check algorithm - Add "allowed to use" check to verify the "local-network-access" policy-controlled feature is allowed before checking permission state - Add anchor definition for HTML spec's "associated Document" term This ensures that documents must be allowed to use the feature by Permissions Policy before local network access requests can proceed, addressing the concern raised in issue WICG#37 about requiring explicit Permissions Policy delegation.
| ISSUE: Define local network access behavior for Service Workers. | ||
|
|
||
| 8. If |document| is not [=allowed to use=] | ||
| "local-network-access", then return |error|. |
There was a problem hiding this comment.
This does seem to further cement using a single permission btw. Given the revived interest in #17 perhaps it's a bit weird to merge this as-is?
|
@christhompson @annevk, do you want me to re-write this patch with fine-grained permissions (with names we all agreed to i.e., local-network and loopback-network) or we merge this patch as is and visit this after #17 is resolved? |
3 participants
Resolves issue #37 by adding proper Permissions Policy integration to the Local Network Access check algorithm
Changes:
Preview | Diff