Add an optional topLevelOrigin parameter to the permissions.setPermission command

[jalonthomas]

Updates the permissions.setPermission command and the permission key generation algorithm. The command now accepts an optional topLevelOrigin parameter to support permissions that require double-keying, such as the Storage Access API. The permission key generation algorithm has been updated to accept origin and top-level origin as direct inputs, instead of an Environment Settings Objects (ESO). This allows WebDriver to properly set a permission, since it doesn't necessarily have an ESO to pass.

closes #450

The following tasks have been completed:

• Modified Web platform tests (link)

Implementation commitment:

• WebKit (link to issue)
• Blink (link to issue)
• Gecko (link to issue)

---

Preview | Diff

[cfredric]

I think this looks pretty good now, thanks!

[cfredric]

It occurs to me that there's a type mismatch here - top level origin is defined to be a text value here, but later on we use it as if it were an [=origin=] (without parsing it or anything). This is an existing problem with how the spec handles the origin variable, too. Technically, the spec probably ought to run a URL parser on the text to produce a URL and then access the URL's origin. Maybe there's a shorthand for doing all of that?

[miketaylr]

Hey thanks! A few questions/comments while I try to page this all back into my head.

Dumb question: why not just add an additional optional top-level origin argument into "set a permission" and plumb that in?

The permission key generation algorithm has been updated to accept origin and top-level origin as direct inputs, instead of an Environment Settings Objects (ESO). This allows WebDriver to properly set a permission, since it doesn't necessarily have an ESO to pass.

I don't follow here - in https://pr-preview.s3.amazonaws.com/w3c/permissions/468/4f2ccb0...jalonthomas:4841693.html#automation you're calling the permission key generation algorithm with a current settings object. So why do we need to modify the permission key generation algorithm? When does WebDriver have a CSO (which is a type of ESO) but not an ESO?

[jalonthomas]

I don't follow here - in https://pr-preview.s3.amazonaws.com/w3c/permissions/468/4f2ccb0...jalonthomas:4841693.html#automation you're calling the permission key generation algorithm with a current settings object. So why do we need to modify the permission key generation algorithm? When does WebDriver have a CSO (which is a type of ESO) but not an ESO?

My understanding is that WebDriver doesn’t have an ESO at all. By modifying the permission key generation algorithm to take an origin and a top-level origin directly, it's now possible for WebDriver to call the algorithm and set permissions without needing an ESO..

Dumb question: why not just add an additional optional top-level origin argument into "set a permission" and plumb that in?

This is definitely a possibility, but the optional origin before functionally was the key. So rather than pass both origins, I think it makes sense to consolidate under a single permission key input, and have the callers generate that key themselves.

[miketaylr]

Thanks - I'm still trying to wrap my head around how WebDriver-bidi implements all of this, there's quite a bit of indirection.

https://github.com/GoogleChromeLabs/chromium-bidi/blob/0a6af6579fd89754d8e7e397dfb44d905e40e1df/src/bidiMapper/modules/permissions/PermissionsProcessor.ts#L25

etc

My understanding is that WebDriver doesn’t have an ESO at all. By modifying the permission key generation algorithm to take an origin and a top-level origin directly, it's now possible for WebDriver to call the algorithm and set permissions without needing an ESO..

How do steps 4 or 6 work in your changes with WebDriver(-bidi) if ESOs don't exist?

Dumb question: why not just add an additional optional top-level origin argument into "set a permission" and plumb that in?

This is definitely a possibility, but the optional origin before functionally was the key. So rather than pass both origins, I think it makes sense to consolidate under a single permission key input, and have the callers generate that key themselves.

One upside is that you don't have to touch any of the algorithms outside of the Automated testing section, I think.

[jalonthomas]

How do steps 4 or 6 work in your changes with WebDriver(-bidi) if ESOs don't exist?

Those steps happen on the browser side, so it should have access to all ESOs. WebDriver only sends the command to the browser.

[jalonthomas]

Thanks - I'm still trying to wrap my head around how WebDriver-bidi implements all of this, there's quite a bit of indirection.

Right so the only change that we're realistically making is adding an optional topLevelOrigin to SetPermissionParameters here. That's the input to the link that you sent. We'll then parse that value as well and send it using Broser.setPermission.

[miketaylr]

My feeling here is that we should just add the argument to the setPermission command and not do any additional refactoring (the fact that WebDriver/BiDi does or does not have an ESO at various parts of the algorithm it consumes feels like an implementation detail for WebDriver to worry about). But, maybe I wrong and happy to convinced otherwise. Curious if @marcoscaceres has any thoughts here.

[marcoscaceres]

Yeah, I'm reaching the same conclusion as @miketaylr. The fact that we can set permissions at all via Web Driver implies that there is an ESO in Web Driver.

In all cases, we should be passing both the origin and top level origin regardless... if we can pass the origin already.

[cfredric]

If I may chime in - the issue that Jalon's trying to fix is that step 2 of set a permission grabs all of the ESOs that are same-origin with target origin.

The usage of same-origin was fine up until #390, when it became possible for permission "scopes" to deviate from an origin, via the concept of permission key. But importantly, #390 didn't update the Set Permission command to use a permission key instead of an origin. (I haven't dug through the whole PR, but I suspect this was an unintentional omission.)

This means it's no longer possible for WebDriver to precisely override the permissions states for permissions whose key type is not the default, i.e. aren't origins. For example, WebDriver cannot request that the storage-access permission state should be overridden for siteA iframes that are embedded in siteB (but not for any other iframes).

Jalon's PR is meant to fix this by allowing WebDriver remote end to receive two origins, compute a permission key from them, and then use that key to choose which ESOs get the new permission state.

This is why the PR modifies the key generation algorithm: so that the WebDriver remote end can construct a permission key from the command's pair of origins and then use the key to affect specific ESOs.

---

(Side note -- I don't really understand step 4.1 of set a permission in the current spec. Is it possible this step should have been changed when the permission store concept was introduced?)

[miketaylr]

I met with @cfredric and @jalonthomas to discuss the PR again. My initial preference would be to keep changes local for the WebDriver BiDi changes, and saw the other changes as ~refactoring. But also, this change looks good and will make it possible to test the "storage access" permission, which is currently untestable via WebDriver bidi.