- From: Johann Hofmann <notifications@github.com>
- Date: Thu, 24 Nov 2022 11:37:42 -0800
- To: w3c/permissions <permissions@noreply.github.com>
- Cc: Subscribed <subscribed@noreply.github.com>
- Message-ID: <w3c/permissions/pull/390/review/1193056842@github.com>
@johannhof commented on this pull request.
@annevk and I went through this draft again today and discussed some improvements, which I noted down in the review here. Anne also filed a couple of bugs where we found issues that weren't caused by this patch. I'll finish up the remaining points in this review and then I think we're ready to move out of WIP state and up to general review (and merge, hopefully).
> + The user agent maintains a single <dfn class="export">permission store</dfn> which is a [=/list=] of [=permission store entries=].
+ </p>
+ <p>
+ The user agent MAY remove [=entries=] from the [=permission store=] when their respective [=permission=]'s [=permission/lifetime=] has expired.
+ </p>
+ <p>
+ The user agent MAY maintain additional permission stores with [=implementation-defined=] scope and eviction rules. Which permission store is adressed in a given moment is [=implementation-defined=].
+ <aside class="note">
+ This is intended to allow a user agent to experiment with user-friendly permission concepts such as per-tab grants.
+ </aside>
+ </p>
+ <p>
+ A <dfn class="export" data-local-lt="entry">permission store entry</dfn> is a [=tuple=] of [=powerful feature/name=] <dfn>name</dfn>, [=permission store key=] <dfn>key</dfn>, {{PermissionDescriptor}} <dfn>descriptor</dfn>, and [=permission/state=] <dfn>state</dfn>.
+ </p>
+ <p>
+ To <dfn class="export">get a permission store entry</dfn> from the user agent's permission store given a |name|, [=permission store key=] key and descriptor, run these steps:
Remove `from the user agent's permission store`
> + The user agent MAY remove [=entries=] from the [=permission store=] when their respective [=permission=]'s [=permission/lifetime=] has expired.
+ </p>
+ <p>
+ The user agent MAY maintain additional permission stores with [=implementation-defined=] scope and eviction rules. Which permission store is adressed in a given moment is [=implementation-defined=].
+ <aside class="note">
+ This is intended to allow a user agent to experiment with user-friendly permission concepts such as per-tab grants.
+ </aside>
+ </p>
+ <p>
+ A <dfn class="export" data-local-lt="entry">permission store entry</dfn> is a [=tuple=] of [=powerful feature/name=] <dfn>name</dfn>, [=permission store key=] <dfn>key</dfn>, {{PermissionDescriptor}} <dfn>descriptor</dfn>, and [=permission/state=] <dfn>state</dfn>.
+ </p>
+ <p>
+ To <dfn class="export">get a permission store entry</dfn> from the user agent's permission store given a |name|, [=permission store key=] key and descriptor, run these steps:
+ <ol class="algorithm">
+ <li>
+ If the permission store [=list/contains=] an [=entry=] with the [=name=] |name|, [=key=] |key| and [=descriptor=] |descriptor|, return that entry.
Add "user agent's permission store" here, reference permission store from above
> + <p>
+ To <dfn class="export">get a permission store entry</dfn> from the user agent's permission store given a |name|, [=permission store key=] key and descriptor, run these steps:
+ <ol class="algorithm">
+ <li>
+ If the permission store [=list/contains=] an [=entry=] with the [=name=] |name|, [=key=] |key| and [=descriptor=] |descriptor|, return that entry.
+ </li>
+ <li>
+ Return null.
+ </li>
+ </ol>
+ </p>
+ <p>
+ To <dfn class="export">set a permission store entry</dfn> in the user agent's permission store given a [=powerful feature/name=] |name|, a [=permission store key=] |key|, a {{PermissionDescriptor}} |descriptor|, and a [=permission/state=] |state|, run these steps:
+ <ol class="algorithm">
+ <li>
+ Let |newEntry| be a new [=permission store entry=] with the [=name=] |name|, [=key=] |key|, [=descriptor=] |descriptor|, and [=state=] |state|.
Instead of "with the", we should say "whose name is name"
> + The user agent maintains a single <dfn class="export">permission store</dfn> which is a [=/list=] of [=permission store entries=].
+ </p>
+ <p>
+ The user agent MAY remove [=entries=] from the [=permission store=] when their respective [=permission=]'s [=permission/lifetime=] has expired.
+ </p>
+ <p>
+ The user agent MAY maintain additional permission stores with [=implementation-defined=] scope and eviction rules. Which permission store is adressed in a given moment is [=implementation-defined=].
+ <aside class="note">
+ This is intended to allow a user agent to experiment with user-friendly permission concepts such as per-tab grants.
+ </aside>
+ </p>
+ <p>
+ A <dfn class="export" data-local-lt="entry">permission store entry</dfn> is a [=tuple=] of [=powerful feature/name=] <dfn>name</dfn>, [=permission store key=] <dfn>key</dfn>, {{PermissionDescriptor}} <dfn>descriptor</dfn>, and [=permission/state=] <dfn>state</dfn>.
+ </p>
+ <p>
+ To <dfn class="export">get a permission store entry</dfn> from the user agent's permission store given a |name|, [=permission store key=] key and descriptor, run these steps:
Some missing vars here
> + <p>
+ To <dfn class="export">set a permission store entry</dfn> in the user agent's permission store given a [=powerful feature/name=] |name|, a [=permission store key=] |key|, a {{PermissionDescriptor}} |descriptor|, and a [=permission/state=] |state|, run these steps:
+ <ol class="algorithm">
+ <li>
+ Let |newEntry| be a new [=permission store entry=] with the [=name=] |name|, [=key=] |key|, [=descriptor=] |descriptor|, and [=state=] |state|.
+ </li>
+ <li>
+ If the permission store [=list/contains=] an [=entry=] with the [=name=] |name|, the [=key=] |key| and the [=descriptor=] |descriptor|, [=list/replace=] that entry with |newEntry| and abort these steps.
+ </li>
+ <li>
+ [=list/append=] |newEntry| to the permission store.
+ </li>
+ </ol>
+ </p>
+ <p>
+ To <dfn class="export">remove a permission store entry</dfn> from the permission store given a name, key and descriptor, run these steps:
vars missing here
> + <li>
+ If the permission store [=list/contains=] an [=entry=] with the [=name=] |name|, [=key=] |key| and [=descriptor=] |descriptor|, return that entry.
+ </li>
+ <li>
+ Return null.
+ </li>
+ </ol>
+ </p>
+ <p>
+ To <dfn class="export">set a permission store entry</dfn> in the user agent's permission store given a [=powerful feature/name=] |name|, a [=permission store key=] |key|, a {{PermissionDescriptor}} |descriptor|, and a [=permission/state=] |state|, run these steps:
+ <ol class="algorithm">
+ <li>
+ Let |newEntry| be a new [=permission store entry=] with the [=name=] |name|, [=key=] |key|, [=descriptor=] |descriptor|, and [=state=] |state|.
+ </li>
+ <li>
+ If the permission store [=list/contains=] an [=entry=] with the [=name=] |name|, the [=key=] |key| and the [=descriptor=] |descriptor|, [=list/replace=] that entry with |newEntry| and abort these steps.
Need to compare keys more explicitly here
> + </li>
+ <li>
+ [=list/append=] |newEntry| to the permission store.
+ </li>
+ </ol>
+ </p>
+ <p>
+ To <dfn class="export">remove a permission store entry</dfn> from the permission store given a name, key and descriptor, run these steps:
+ <ol class="algorithm">
+ <li>
+ [=list/Remove=] the [=entry=] with the [=name=] |name|, [=key=] |key| and [=descriptor=] |descriptor| from the permission store.
+ </li>
+ </ol>
+ </p>
+ <p>
+ A <dfn class="export">permission store key</dfn> has the type returned by the feature's [=powerful feature/permission key generation algorithm=].
Could add a section to this that says something like:
A permission store key key1 compares to key2 given feature name by running the following steps:
> @@ -278,6 +278,69 @@ <h3>
reset because its [=permission/lifetime=] has expired.
</p>
</section>
+ <section>
+ <h3>
+ Permission Store
+ </h3>
+ <p>
+ The user agent maintains a single <dfn class="export">permission store</dfn> which is a [=/list=] of [=permission store entries=].
+ </p>
+ <p>
+ The user agent MAY remove [=entries=] from the [=permission store=] when their respective [=permission=]'s [=permission/lifetime=] has expired.
+ </p>
+ <p>
+ The user agent MAY maintain additional permission stores with [=implementation-defined=] scope and eviction rules. Which permission store is adressed in a given moment is [=implementation-defined=].
It's probably fine to remove this given that we have the lifetime concept. We could consider following up with replacing lifetime with an implementation-defined field on store keys, see #391.
> + <li>
+ Return null.
+ </li>
+ </ol>
+ </p>
+ <p>
+ To <dfn class="export">set a permission store entry</dfn> in the user agent's permission store given a [=powerful feature/name=] |name|, a [=permission store key=] |key|, a {{PermissionDescriptor}} |descriptor|, and a [=permission/state=] |state|, run these steps:
+ <ol class="algorithm">
+ <li>
+ Let |newEntry| be a new [=permission store entry=] with the [=name=] |name|, [=key=] |key|, [=descriptor=] |descriptor|, and [=state=] |state|.
+ </li>
+ <li>
+ If the permission store [=list/contains=] an [=entry=] with the [=name=] |name|, the [=key=] |key| and the [=descriptor=] |descriptor|, [=list/replace=] that entry with |newEntry| and abort these steps.
+ </li>
+ <li>
+ [=list/append=] |newEntry| to the permission store.
Append
> + <li>
+ Let |newEntry| be a new [=permission store entry=] with the [=name=] |name|, [=key=] |key|, [=descriptor=] |descriptor|, and [=state=] |state|.
+ </li>
+ <li>
+ If the permission store [=list/contains=] an [=entry=] with the [=name=] |name|, the [=key=] |key| and the [=descriptor=] |descriptor|, [=list/replace=] that entry with |newEntry| and abort these steps.
+ </li>
+ <li>
+ [=list/append=] |newEntry| to the permission store.
+ </li>
+ </ol>
+ </p>
+ <p>
+ To <dfn class="export">remove a permission store entry</dfn> from the permission store given a name, key and descriptor, run these steps:
+ <ol class="algorithm">
+ <li>
+ [=list/Remove=] the [=entry=] with the [=name=] |name|, [=key=] |key| and [=descriptor=] |descriptor| from the permission store.
Should be updated as per above comments
> @@ -629,9 +737,11 @@ <h3 id="reading-current-states">
</li>
</ol>
</li>
- <li>If there was a previous invocation of this algorithm with the same |descriptor| and
- |settings|, returning |previousResult|, and the user agent has not received <a>new
- information about the user's intent</a> since that invocation, return |previousResult|.
+ <li>Let |key| be the result of [=powerful feature/permission key generation algorithm|generating a permission store key=] with |settings|.
+ </li>
+ <li>Let |entry| be the result of [=get a permission store entry|getting a permission store entry=] with |feature|, |descriptor| and |key|.
+ </li>
+ <li>If |entry| is not null, return a {{PermissionState}} enum value from |entry|'s state and name.
Link to state and name here
> + A <dfn data-dfn-for="powerful feature" class="export">permission key generation algorithm</dfn>:
+ </dt>
+ <dd>
+ <p>
+ Takes an [=environment settings object=], and returns a new [=permission store key=].
+ If unspecified, this defaults to the <a>default permission key generation algorithm</a>.
+ A feature that specifies a custom [=powerful feature/permission key generation algorithm=] MUST also specify a
+ [=powerful feature/permission key comparison algorithm=].
+ </p>
+ <p>
+ The <dfn data-export="">default permission key generation algorithm</dfn>,
+ given an [=environment settings object=] |settings|, runs the following steps:
+ </p>
+ <ol class="algorithm">
+ <li>
+ Return |settings|' [=environment/top-level origin=].
Add 's
> + </dd>
+ <dt>
+ A <dfn data-dfn-for="powerful feature" class="export">permission key comparison algorithm</dfn>:
+ </dt>
+ <dd>
+ <p>
+ Takes two [=permission store keys=] and returns a boolean that shows whether the two keys are equal.
+ If unspecified, this defaults to the <a>default permission key comparison algorithm</a>.
+ </p>
+ <p>
+ The <dfn data-export="">default permission key comparison algorithm</dfn>,
+ given [=permission store keys=] |key1| and |key2| (both [=origins=]), runs the following steps:
+ </p>
+ <ol class="algorithm">
+ <li>
+ If |key1| is not [=same origin=] with |key2| return false.
Assert that both are origins here.
> @@ -502,6 +565,51 @@ <h2>
</li>
</ol>
</dd>
+ <dt>
+ A <dfn data-dfn-for="powerful feature" class="export">permission key generation algorithm</dfn>:
We should call this "permission key" consistently (or permission store key)
> + <li>
+ If the permission store [=list/contains=] an [=entry=] with the [=name=] |name|, [=key=] |key| and [=descriptor=] |descriptor|, return that entry.
+ </li>
+ <li>
+ Return null.
+ </li>
+ </ol>
+ </p>
+ <p>
+ To <dfn class="export">set a permission store entry</dfn> in the user agent's permission store given a [=powerful feature/name=] |name|, a [=permission store key=] |key|, a {{PermissionDescriptor}} |descriptor|, and a [=permission/state=] |state|, run these steps:
+ <ol class="algorithm">
+ <li>
+ Let |newEntry| be a new [=permission store entry=] with the [=name=] |name|, [=key=] |key|, [=descriptor=] |descriptor|, and [=state=] |state|.
+ </li>
+ <li>
+ If the permission store [=list/contains=] an [=entry=] with the [=name=] |name|, the [=key=] |key| and the [=descriptor=] |descriptor|, [=list/replace=] that entry with |newEntry| and abort these steps.
We noted that we don't have a comparison algorithm for descriptors, see #396, but Anne and I are okay with solving this as a follow-up to this PR (if others are as well).
--
Reply to this email directly or view it on GitHub:
https://github.com/w3c/permissions/pull/390#pullrequestreview-1193056842
You are receiving this because you are subscribed to this thread.
Message ID: <w3c/permissions/pull/390/review/1193056842@github.com>
Received on Thursday, 24 November 2022 19:37:56 UTC