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

Feature Policy JS introspection API #292

Closed
3 of 5 tasks
clelland opened this issue Jul 4, 2018 · 8 comments
Closed
3 of 5 tasks

Feature Policy JS introspection API #292

clelland opened this issue Jul 4, 2018 · 8 comments
Assignees
Labels
Progress: pending editor update TAG is waiting for a spec/explainer update Progress: pending external feedback The TAG is waiting on response to comments/questions asked by the TAG during the review Progress: review complete Review type: later review Topic: Feature Policy Venue: WebAppSec WG

Comments

@clelland
Copy link

clelland commented Jul 4, 2018

Hello TAG!

I'm requesting a TAG review of:

  • Name: Feature Policy: JavaScript policy introspection
  • Specification URL: PR, published as draft here (See section 7, specifically).
  • Explainer, Requirements Doc, or Example code: See introduction and examples in draft
  • Tests: This API is tested in WPT/feature-policy, specifically with the feature-policy-frame-policy-*, feature-policy-header-policy-*, and feature-policy-nested-header-policy-* tests. Once available, this API can also be used to test many other policy-controlled features, as well.
  • Primary contacts: iclelland

Further details (optional):

You should also know that...

[please tell us anything you think is relevant to this review]

We'd prefer the TAG provide feedback as (please select one):

  • open issues in our Github repo for each point of feedback
  • open a single issue in our Github repo for the entire review
  • leave review feedback as a comment in this issue and @-notify [github usernames]

Please preview the issue and check that the links work before submitting

For background, some decent explainers:

https://github.com/w3c/ServiceWorker/blob/master/explainer.md
https://github.com/zkoch/paymentrequest/blob/gh-pages/docs/explainer.md
https://github.com/WICG/IntersectionObserver/blob/gh-pages/explainer.md (although this one includes IDL, which an explainer should not)

@clelland
Copy link
Author

Chrome is implementing this as an origin trial, as a means of obtaining additional feedback from the developer community. As such, I've merged the PR into the main spec, with a large disclaimer that the API should be considered unstable (pending feedback).

The up-to-date spec changes are now public at https://wicg.github.io/feature-policy/#introspection

@travisleithead travisleithead self-assigned this Jul 25, 2018
@torgo torgo changed the title TAG review requested: Feature Policy JS introspection API Feature Policy JS introspection API Oct 30, 2018
@torgo torgo added this to the 2018-10-30-f2f-paris milestone Oct 30, 2018
@clelland
Copy link
Author

I'm considering making some changes to this API, in light of developer and implementer feedback; before I do, though, I'm wondering if there has been any action from the TAG, or if I should wait a bit longer?

(I totally understand that there's a backlog, just looking to get some insight into the process)

@clelland
Copy link
Author

Friendly ping :) Any updates?
FYI, this has received API owners approval to ship in Chrome: https://groups.google.com/a/chromium.org/d/msg/blink-dev/qwLRSNFsfUQ/ZhxYYusjCAAJ

Also, the spec changes (now at https://w3c.github.io/webappsec-feature-policy/#introspection) are no longer marked as unstable.

@cynthia
Copy link
Member

cynthia commented Feb 6, 2019

Pong! Sorry this took so long. We plan to discuss this in our Tokyo F2F. (which is until tomorrow)

Reading the spec now.

@kenchris
Copy link

During the F2F, @cynthia and me looked at this issue. Apologies for the long wait, but we are in the process of speeding up our reviews.

We find the method names somewhat inconsistent. There are two getters, one starts with get - the other doesn't. There is a query method that is not clear that it is a query method from the name.

allowedFeatures() should maybe take an optional origin like the query method.

We have existing similar names on the web today like document.querySelector() and document.getElementsByTag(). Better names could be policy.queryAllowList() and policy.getAllowList(optional origin) and policy.getAllowedOrigins(feature) - in particular we find the term "allow list" being two different things (features, origins) called the same thing confusing (hence getAllowlistForFeature returns a list of origins and not an actual allow list).

Given these inconsistencies, how malleable are the APIs at the moment since it is a shipped feature? If the adoption rate isn´t high, considering that other browsers have not shipped we think it might not be too late to make adjustments to the naming.

At this F2F we are hearing that feature policy will get feature policies that support parameters - what is the interopt story for that?

@kenchris kenchris removed this from the 2019-05-21-f2f-reykjavík milestone May 22, 2019
@cynthia cynthia added Progress: pending external feedback The TAG is waiting on response to comments/questions asked by the TAG during the review Progress: pending editor update TAG is waiting for a spec/explainer update Progress: review complete Review type: later review Topic: Feature Policy Venue: WebAppSec WG labels May 22, 2019
@clelland
Copy link
Author

@kenchris:

Given these inconsistencies, how malleable are the APIs at the moment since it is a shipped feature?

Those changes are likely still possible; the feature itself only shipped in Chrome 74; We're seeing some uptick in usage, though the absolute numbers are still quite small (https://www.chromestatus.com/metrics/feature/timeline/popularity/2511). An optional origin should be trivial to add; we may need to have a short overlap period while we deprecate the old names, if we go that route.

Are there TAG guidelines for what counts as a 'query' for naming purposes? The only instances I can find in our IDL are for querySelector and querySelectorAll (Permissions and WebLocks have generically-named query() methods as well)

in particular we find the term "allow list" being two different things (features, origins) called the same thing confusing (hence getAllowlistForFeature returns a list of origins and not an actual allow list).

In the spec, an 'allowlist' is either a list of origins, or the value '*' (representing all origins). It shouldn't be a list of features (a declared policy is a map from features to allowlists). I think that it would run counter to that definition if we used getAllowList to return a list of features -- would getAllowedFeatures be as clear?

At this F2F we are hearing that feature policy will get feature policies that support parameters - what is the interopt story for that?

The initial proposal (w3c/webappsec-permissions-policy#163) extended feature policy with parameters for some new features; existing features are all considered 'boolean' in type, and so 'true' and 'false' would be the only valid values, and could be omitted from any declarations (the 'true' value is inferred from the an origin being allowed at all); in this way it was intended to be backwards-compatible with existing features and existing uses.

There is some interest from people in taking those features that would work well with parameters, and splitting them off into a different mechanism, to avoid burdening Feature Policy with additional complexity. This was proposed in https://github.com/w3c/webappsec-feature-policy/issues/282#issuecomment-486267212, and I opened an issue for further design discussion at w3c/webappsec-permissions-policy#299.

@torgo torgo added this to the 2019-06-19-telecon milestone Jun 18, 2019
@cynthia
Copy link
Member

cynthia commented Jun 26, 2019

Apologies for the delay again. New process adoption.. takes time, and discipline - the latter which at least I have failed at. :-)

Are there TAG guidelines for what counts as a 'query' for naming purposes? The only instances I can find in our IDL are for querySelector and querySelectorAll (Permissions and WebLocks have generically-named query() methods as well)

No, not that I am aware of. The only bit we touch on is the term "is", here: https://w3ctag.github.io/design-principles/#naming-is-hard

Would it be helpful if we had a guideline on this? If it feels useful we will file an issue to address this in the design principles document.

As for the remaining comments - I'll discuss this separately with @kenchris. Thanks for the patience.

@kenchris
Copy link

I hope our feedback was useful, there doesn't seem to be much more for us to do here, so thanks for flying TAG!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Progress: pending editor update TAG is waiting for a spec/explainer update Progress: pending external feedback The TAG is waiting on response to comments/questions asked by the TAG during the review Progress: review complete Review type: later review Topic: Feature Policy Venue: WebAppSec WG
Projects
None yet
Development

No branches or pull requests

8 participants