-
Notifications
You must be signed in to change notification settings - Fork 55
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
Integrate with the Permissions spec. #36
Conversation
If that returns no answer, then do not alter <var>permission</var>. </p> | ||
<p class="note" role="note">User agents are encouraged to not let the user answer this question twice for | ||
the same <a data-link-type="dfn" href="https://html.spec.whatwg.org/multipage/browsers.html#origin">origin</a> around the same time and this algorithm is not equipped to handle such a | ||
scenario. </p> |
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.
We should keep this note I think.
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.
Done.
Also, thanks for doing this! |
e35f939
to
cb12ff0
Compare
<p class="XXX">We will eventually integrate with the Permissions API, with the identifier | ||
"<dfn export for=PermissionName type=enum-value><code>persistent-storage</code></dfn>", but since | ||
that specification is not in great shape at the moment that has not happened yet. | ||
The <dfn for="PermissionName" enum-value>"persistent-storage"</dfn> <a>powerful feature</a>'s |
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.
Powerful feature? Didn't we do away with that term?
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 reintroduced it in w3c/permissions#97, but with a more tautological definition: "A feature of a UA that some code might not be allowed to access, for example because its environment settings object doesn’t satisfy some criteria, or because the user hasn’t given permission."
@mikewest was happy with it, and because it doesn't include the "should only be accessible from HTTPS" bits, I don't expect other folks to have a problem. If they do, we could back off to just "feature". And we might do that anyway if we unify the PermissionName enum with the list in Feature Policy.
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 think we should just call them (privileged/powerful) APIs. But I guess I'm okay with this for now. (API is the term we use elsewhere, e.g., in Fetch.)
I would prefer "Let permission be the result of requesting permission to use ..." indeed. Either with an The main problem with the descriptor I think is that you're sort of hand-waving the parsing and turning it into a dictionary part. I don't think we currently have anything that implicit. |
It seems powerful feature is how Permissions describes gated APIs. I thought we had gotten rid of the term after the rename to secure contexts. Any particular reason we say feature and not just API? |
(Feel free to get annoyed with me for second guessing some Permissions stuff. I realize that's not ideal to do here.) |
Interesting. I think it's unambiguous what WebIDL object would be created by this sort of spec syntax, but I'm happy to say something else.
Not a problem. We need to come to consensus about this, and going around in a couple circles is the only way to do that. |
The main problem is that it doesn't follow from first principles. It kinda works because everyone recognizes it, but it isn't really grounded formally. |
This change defines both "Verb noun" and "verbing noun" so other specs can use "Let foo be the result of verbing noun." instead of the current "Verb noun, and let foo be the result." Motivated by whatwg/storage#36.
"<dfn export for=PermissionName type=enum-value><code>persistent-storage</code></dfn>", but since | ||
that specification is not in great shape at the moment that has not happened yet. | ||
The <dfn for="PermissionName" enum-value>"<code>persistent-storage</code>"</dfn> <a>powerful feature</a>'s | ||
permission-related flags, algorithms, and types are defaulted except for: |
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.
comma before "except for"
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.
Done.
You should also add your name to the acknowledgments. |
<dl> | ||
<dt><a>permission state</a></dt> | ||
<dd><code>{name: {{"persistent-storage"}}}</code>'s <a>permission state</a> must have the same | ||
value for all <a>environment settings objects</a> with a given <a>origin</a>.</dd> |
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 (sorry I'm going through a backlog of activity :) I wonder if we can word this in a way that makes it clear the permission state is being "returned"?
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.
That's a good point.
What we really want is to require a certain scope for permissions. And the scope for "persistent-storage" is an origin, but we'd then also need to define where that origin originates.
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.
w3c/permissions#114 adds an explicit settings object argument to "permission state", which is where the origin would come from.
I'm skeptical that we need to talk about "returning" a permission state. When we talk about a settings object's origin, we don't bother saying that it's the origin returned by some algorithm.
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.
But we do define creation of a settings object. At that point the origin is set, too. The statement we have here around "permission state" is somewhat declarative and doesn't directly fall out from something else. Does that make sense?
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.
Leaving the value up to the UA is definitely an uncomfortable point in the permissions spec, but saying "returned" here wouldn't help with it.
w3c/permissions#117 changes the "permission state" algorithm to explicitly delegate to this section. Does that help?
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.
You can see in the ordering of the PRs that I mostly agree with you. I had to be pulled into the looser description by the people implementing Chrome's and Firefox's permission storage systems. That said, I think the variety of existing stores is a good reason not to try to write up a single model that covers all of them.
It'd be fine with me if we revisit this. (Except argh more work. 😉) If you want to change it, could you start a thread on webappsec so everyone who cares can be involved?
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 think the variety of existing stores is a good reason not to try to write up a single model that covers all of them.
Isn't that what Permissions is? If Permissions is not that, what exactly is this integration supposed to be?
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'm happy to start a thread, but I'm not really sure where to start. It seems like we want a generalized model since some want to expose generalized "get" and "request" operations. It's not clear how those would work if there wasn't a generalized model.
And if there's no generalized store that means each permission either has to adopt a default store or implement its own. We could go down that route too, but I'm not sure that's more useful than just allowing the nature of the key to be changed on a per-permission-type basis.
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.
Assuming that we can't build a generalized model, I think the Permissions spec serves to explain that fact and prevent individual specs from trying to specify a store that not all browsers will be willing to implement. It says, maybe not clearly enough, that each UA has to provide the "get" and "request" operations in whatever way makes sense for that UA, and then specs can call those operations, instead of independently asserting that they exist.
The useful webappsec thread, I think, would consist of revisiting whether that first assumption is true, or whether there is a more detailed model we could put into the Permissions spec.
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.
7639c39
to
5cd6ae7
Compare
Can you click the checkbox in the sidebar that allows me to push to your branch? This looks okay now. Needs a date bump and maybe some minor tweaks, depending on the console output of bikeshed. |
5cd6ae7
to
0800e6d
Compare
…iptor. This works since w3c/permissions#115.
I've updated the date, and bikeshed shows no errors or warnings. And I've checked the box. |
Thanks! |
* Don't use an IDL block for PermissionName, so that it can link outside the local spec. * Remove the local persistent-storage definition now that it's defined in the Storage spec. (whatwg/storage#36)
Preview at https://api.csswg.org/bikeshed/?url=https://github.com/jyasskin/storage/raw/permission-integration/storage.bs#persistence