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.

Sign up for GitHub

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

Remove modification of document.requestStorageAccess algorithm #34

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Remove modification of document.requestStorageAccess algorithm #34

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e125112
Remove modification of `document.requestStorageAccess` algorithm
cfredric Apr 12, 2023
54d5d1b
Swap "may" note for proper permissions integration
cfredric Apr 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 2 additions & 11 deletions index.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ When invoked on {{Document}} |doc| with {{USVString}} |requestedOrigin|, the <df
1. Return.
1. Let |permissionState| be the result of [=requesting permission to use=] "<a permission><code>top-level-storage-access</code></a>" with |descriptor|.

NOTE: Note that when requesting permissions and deciding whether to show a prompt, user agents apply implementation-defined behavior to shape the end user experience. Particularly for `top-level-storage-access`, user agents are known to apply custom rules that will grant or deny a permission without showing a prompt.
NOTE: Note that when requesting permissions and deciding whether to show a prompt, user agents apply implementation-defined behavior to shape the end user experience. Particularly for `top-level-storage-access`, user agents are known to apply custom rules that will grant or deny a permission without showing a prompt. User agents may also treat the `top-level-storage-access` permission as a "superset" of other permissions, and may grant those other permissions when `top-level-storage-access` permission is granted.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few thoughts:

  • This doesn't seem placed right. In this step we check for permission for top-level-storage-access, we're not trying to determine other permissions.
  • It's generally discouraged (sometimes disallowed) to use RFC 2119 terms ("may", in this case) in informative notes.
  • I can't really wrap my head around the "superset" concept for the relationship between these two permissions, but maybe that's just me :)

Would it make sense to just add another normative step here with a MAY, something like:

1. The user agent may (or should?) grant the "storage-access" permission for origin and doc's origin.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about this more maybe this should be just a regular step, it's not clear why it has to be may/should :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These steps are written in the context of a "client" that is interacting with permissions via request permission to use, so I don't think we can just say "if permissionState is "granted", grant permission for storage-access", because we don't have access to those internals here (IIUC). I think we would have to patch the request permission to use algorithm instead, such that it grants both storage-access and top-level-storage-access whenever top-level-storage-access is requested and granted.

That was the reason I put this here as a note, rather than as a regular step. And this note already talks about user-agent-defined behavior in request permission to use, which is why I expected that "may" would be fine. WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I defer to @johannhof on this, but for what my opinion is worth:

  • dual write or check both permissions accomplishes the same thing in the rSA steps. I'm aware there are other side effects with permissions.query, etc., but we already anticipated wanting those side effects and could specify them separately vs mandating dual-write.
  • if I'm not mistaken, switching to may would mean we'd need to delete WPTs that verify the autogrant of rSA, and that's a pretty important part of what this API does. It seems like it should be an explicit requirement, not implementation defined behavior (implementation defined behavior seems fine when deciding to grant the permission, since developers would already need to contend with humans being shown a prompt, but less fine in specifying what the permission does once it's granted).

Basically, it makes sense to me to dual-write, but I worry that having it be implementation-defined behavior is not what we want (but again, I defer to Johann).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTAL - I've updated the PR to still mandate the top-level-storage-access/storage-access dual write, but do so by properly integrating into the Permissions spec.


1. If |permissionState| is [=permission/granted=]:
1. [=Queue a global task=] on the [=permissions task source=] given |global| to [=resolve=] |p|.
Expand Down Expand Up @@ -229,18 +229,9 @@ In [=http network or cache fetch=], when determining whether to block cookies, r
<h2 id="storage-access-api-integration">Storage Access API Integration</h2>

Note: even after a successful {{Document/requestStorageAccessFor(requestedOrigin)}} call, frames have to explicitly invoke {{Document/requestStorageAccess()}} for cookie access.
This modification allows {{Document/requestStorageAccessFor(requestedOrigin)}} to allow resolution of {{Document/requestStorageAccess()}} calls similarly to a prior successful {{Document/requestStorageAccess()}} grant.

<div algorithm='storage-access-policy-modification'>
Modify {{Document/requestStorageAccess()}} to insert the following steps before step 13.4 (i.e. before checking transient activation):
As noted above, user agents may choose to grant the corresponding `storage-access` permission when granting `top-level-storage-access` permission.

1. Let |settings| be <var ignore>doc</var>'s [=relevant settings object=].
1. Let |origin| be |settings|' [=environment settings object/origin=].
1. Let |descriptor| be a newly created {{TopLevelStorageAccessPermissionDescriptor}} with {{PermissionDescriptor/name}} set to "<a permission><code>top-level-storage-access</code></a>" and with {{TopLevelStorageAccessPermissionDescriptor/requestedOrigin}} set to |origin|.
1. If |descriptor|'s [=permission state=] is [=permission/granted=], [=queue a global task=] on the [=permissions task source=] given |global| to [=resolve=] |p|, and return.
1. If |descriptor|'s [=permission state=] is [=permission/denied=], [=queue a global task=] on the [=permissions task source=] given |global| to [=reject=] |p| with a "{{NotAllowedError}}" {{DOMException}}, and return.

</div>

<h2 id="privacy">Privacy considerations</h2>

Expand Down