Fix <webview> media permission requests with PEPC [chromium/src : main]
Giovanni Pezzino (Gerrit)
Giovanni Pezzino voted and added 7 comments![]( "Open in Gerrit")
Votes added by Giovanni Pezzino
| Auto-Submit | +1 |
| Commit-Queue | +1 |
7 comments
base::DictValue request_info;Giovanni PezzinoExisting media requests only set a url field [1].
Done
#if BUILDFLAG(ENABLE_EXTENSIONS)Giovanni PezzinoStyle nit: ifdef'd includes should be in their own section.
Done
Profile* profile = Profile::FromBrowserContext(browser_context());Giovanni Pezzinonit: The getters that use this don't need the downcast.
Done
if (extensions::IsExtensionWithPermissionOrSuggestInConsole(Giovanni PezzinoOnly platform apps can use audioCapture/videoCapture [1], so we should probably only make the suggestion for those.
[1] extensions/common/api/_permission_features.json
Done
"app": {
"background": {
"scripts": ["test.js"]
}
}Kevin McNeeas you're very familiar, apps are deprecated on non-chromeos : ) But it seems like this is a feature that should apply to webviews broadly on other operating systems. Since this is a new test, would it make sense to write it with a webview embedded by e.g. webui so it can run everywhere?
Kevin McNeeSince this CL is also covering controlled frame, I'd recommend adding a test for that case.
While having the test run for other platforms under webui would be nice, it looks like this CL also needs to test the audioCapture for the app, so a webui test would be in addition to this one. Plus we don't have an existing test fixture for webui webview tests AFAIK, so I wouldn't block you on it.
Giovanni PezzinoOh sorry, we do have some webui tests: chrome/test/data/webui/webview/webui_webview_browsertest.cc
I have added a new test with the hosting page using a controlled frame (see controlled_frame_permission_request_browsertest.cc). AFAIUI, the new test will run on all platforms.
Does it satisfy your concerns?
const GURL& requesting_frame,Giovanni Pezzinonit: The caller is providing what appears to be the GURL representation of an origin, so consider naming this something like requesting_frame_origin.
Done
// Requests Media Permission from the embedder (for PEPC).Giovanni Pezzinonit: Write out what this is (not just the acronym).
Done
Related details
- Devlin Cronin
- Kevin McNee
- Ravjit Uppal
- Simon Hangl
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin McNee (Gerrit)
Kevin McNee voted Code-Review+1![]( "Open in Gerrit")
- Devlin Cronin
- Giovanni Pezzino
- Ravjit Uppal
- Simon Hangl
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
Devlin Cronin (Gerrit)
Devlin Cronin added 14 comments![]( "Open in Gerrit")
2. Auto-approving the request on the embedder App side if it has theJust sanity checking: has this gone through security review? Are there any cases in which the embedding app would *not* want to auto-grant the site permission, just because the app had permission?
const stream = await navigator.mediaDevices.getUserMedia({video: true});nit: wrap at 80 char; here and below
await new Promise((resolve) => setTimeout(resolve, 100));why do we do this? (Add a comment)
: network::mojom::PermissionsPolicyFeature::kCamera;if this ever gets called for a new type, this will miscategorize the request as one for kCamera.
Can we add a CHECK() to ensure the type is one of these two?
blink::PermissionType permission_type =ditto
#if BUILDFLAG(ENABLE_EXTENSIONS)generally, these should either be ENABLE_EXTENSIONS_CORE or something more specific. Does that work here?
if (extension_registry) {ExtensionRegistry should never be null while there are active renderers for a BrowserContext
#if BUILDFLAG(ENABLE_EXTENSIONS)ditto re ENABLE_EXTENSIONS
extensions::ExtensionBuilder("Test App", extensions::ExtensionBuilder::Type::PLATFORM_APP)git cl format
JSON.stringify(['check-media-permission', '' + testName]), '*');in lots of places here, please prefer template literals to avoid string concatenation.
"app": {
"background": {
"scripts": ["test.js"]
}
}Kevin McNeeas you're very familiar, apps are deprecated on non-chromeos : ) But it seems like this is a feature that should apply to webviews broadly on other operating systems. Since this is a new test, would it make sense to write it with a webview embedded by e.g. webui so it can run everywhere?
Kevin McNeeSince this CL is also covering controlled frame, I'd recommend adding a test for that case.
While having the test run for other platforms under webui would be nice, it looks like this CL also needs to test the audioCapture for the app, so a webui test would be in addition to this one. Plus we don't have an existing test fixture for webui webview tests AFAIK, so I wouldn't block you on it.
Giovanni PezzinoOh sorry, we do have some webui tests: chrome/test/data/webui/webview/webui_webview_browsertest.cc
I have added a new test with the hosting page using a controlled frame (see controlled_frame_permission_request_browsertest.cc). AFAIUI, the new test will run on all platforms.
Does it satisfy your concerns?
I'd still prefer a webui one, since controlled frame is kinda-sorta chromeos only (even though it's compiled on all platforms). But I'll leave it up to you and Kevin (I'm not an owner of webview)
// Requests Media Permission from the embedder (for Page Embedded Permission Control).wrap at 80 char
// Requests Media Permission from the embedder (for Page Embedded Permission Control).wrap at 80 char
Related details
Giovanni Pezzino (Gerrit)
Giovanni Pezzino voted and added 12 comments![]( "Open in Gerrit")
Votes added by Giovanni Pezzino
| Auto-Submit | +1 |
| Commit-Queue | +0 |
12 comments
2. Auto-approving the request on the embedder App side if it has theJust sanity checking: has this gone through security review? Are there any cases in which the embedding app would *not* want to auto-grant the site permission, just because the app had permission?
The implementation is not actually auto-approving the request: it is merely forwarding the request to the embedding page/app using a JS permissionrequest event, and the embedding page/app handles the event with any logic the developers deems appropriate.
If the event handler approves the request, then DevicePermission in media_stream_device_permission_context.cc checks if the top-level app has the permission in the manifest.
The description is indeed misleading, so I have updated it.
const stream = await navigator.mediaDevices.getUserMedia({video: true});nit: wrap at 80 char; here and below
Done
await new Promise((resolve) => setTimeout(resolve, 100));why do we do this? (Add a comment)
Done
if this ever gets called for a new type, this will miscategorize the request as one for kCamera.
Can we add a CHECK() to ensure the type is one of these two?
Done
blink::PermissionType permission_type =Giovanni Pezzinoditto
Done
generally, these should either be ENABLE_EXTENSIONS_CORE or something more specific. Does that work here?
ENABLE_EXTENSION_CORE is defined as `enable_extensions || enable_desktop_android_extensions`, so it will work on all desktop platforms.
ExtensionRegistry should never be null while there are active renderers for a BrowserContext
Done
#if BUILDFLAG(ENABLE_EXTENSIONS)Giovanni Pezzinoditto re ENABLE_EXTENSIONS
Done
extensions::ExtensionBuilder("Test App", extensions::ExtensionBuilder::Type::PLATFORM_APP)Giovanni Pezzinogit cl format
Done
JSON.stringify(['check-media-permission', '' + testName]), '*');in lots of places here, please prefer template literals to avoid string concatenation.
Done
// Requests Media Permission from the embedder (for Page Embedded Permission Control).Giovanni Pezzinowrap at 80 char
Done
// Requests Media Permission from the embedder (for Page Embedded Permission Control).Giovanni Pezzinowrap at 80 char
Done
Related details
- Devlin Cronin
- Ravjit Uppal
- Simon Hangl
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Simon Hangl (Gerrit)
Simon Hangl voted and added 3 comments![]( "Open in Gerrit")
Votes added by Simon Hangl
| Code-Review | +1 |
3 comments
network::mojom::PermissionsPolicyFeature feature =nit: `const` or inline
blink::PermissionType permission_type =nit: `const` or inline
Related details
- Devlin Cronin
- Giovanni Pezzino
- Ravjit Uppal
Ravjit Uppal (Gerrit)
Ravjit Uppal voted![]( "Open in Gerrit")
| Code-Review | +1 |
| Commit-Queue | +2 |
Giovanni Pezzino (Gerrit)
Giovanni Pezzino voted and added 2 comments![]( "Open in Gerrit")
Votes added by Giovanni Pezzino
| Auto-Submit | +1 |
| Commit-Queue | +1 |
2 comments
network::mojom::PermissionsPolicyFeature feature =Giovanni Pezzinonit: `const` or inline
Done
blink::PermissionType permission_type =Giovanni Pezzinonit: `const` or inline
Done
Related details
- Devlin Cronin
- Ravjit Uppal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Giovanni Pezzino (Gerrit)
Giovanni Pezzino voted and added 1 comment![]( "Open in Gerrit")
Votes added by Giovanni Pezzino
| Auto-Submit | +1 |
1 comment
"app": {
"background": {
"scripts": ["test.js"]
}
}Kevin McNeeas you're very familiar, apps are deprecated on non-chromeos : ) But it seems like this is a feature that should apply to webviews broadly on other operating systems. Since this is a new test, would it make sense to write it with a webview embedded by e.g. webui so it can run everywhere?
Kevin McNeeSince this CL is also covering controlled frame, I'd recommend adding a test for that case.
While having the test run for other platforms under webui would be nice, it looks like this CL also needs to test the audioCapture for the app, so a webui test would be in addition to this one. Plus we don't have an existing test fixture for webui webview tests AFAIK, so I wouldn't block you on it.
Giovanni PezzinoOh sorry, we do have some webui tests: chrome/test/data/webui/webview/webui_webview_browsertest.cc
Devlin CroninI have added a new test with the hosting page using a controlled frame (see controlled_frame_permission_request_browsertest.cc). AFAIUI, the new test will run on all platforms.
Does it satisfy your concerns?
I'd still prefer a webui one, since controlled frame is kinda-sorta chromeos only (even though it's compiled on all platforms). But I'll leave it up to you and Kevin (I'm not an owner of webview)
I ended up adding the webui test: easier done than discussing it further 😜
Related details
- Devlin Cronin
- Kevin McNee
- Ravjit Uppal
- Simon Hangl
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Giovanni Pezzino (Gerrit)
Giovanni Pezzino voted and added 1 comment![]( "Open in Gerrit")
Votes added by Giovanni Pezzino
| Commit-Queue | +0 |
1 comment
GTEST_SKIP() << "Glic media tests are disabled on ChromeOS.";I went with GTEST_SKIP instead of the DISABLED_ prefix because the WebUI presubmit check failed.
Please, let me know if you have any questions.
Related details
- Devlin Cronin
- Kevin McNee
- Ravjit Uppal
- Simon Hangl
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Kevin McNee (Gerrit)
Kevin McNee voted and added 2 comments![]( "Open in Gerrit")
Votes added by Kevin McNee
| Code-Review | +1 |
2 comments
// unrendered elements are ignored by Blink's security engine.nit: I wonder if we can do something that doesn't rely on picking timeout values. Would something like double requestAnimationFrame work here? Ditto for the timeouts in the other tests.
Related details
- Devlin Cronin
- Giovanni Pezzino
- Ravjit Uppal
- Simon Hangl
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From chrome/enterprise/gwsq/commercial-readability-review.gwsq:
A C++ readability reviewer was assigned to this CL. (http://go/cros-readability)
For the author: Feel free to remove the reviewer.
For the reviewer:
Please wait until normal review is finished and then follow http://go/cros-readability-guidelines.
Fill out http://go/cros-readability-poll?entry.704301348=giovax&entry.1703826705=7868189 after you finished reviewing.
Reviewer source(s):
greengrape is from group(chromeos-commercial-r...@google.com)
Related details
- Andrew Rayskiy
- Devlin Cronin
- Giovanni Pezzino
- Ravjit Uppal
- Simon Hangl
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Ravjit Uppal (Gerrit)
Ravjit Uppal voted Code-Review+1![]( "Open in Gerrit")
- Andrew Rayskiy
- Devlin Cronin
- Giovanni Pezzino
- Simon Hangl
Thomas Nguyen (Gerrit)
Thomas Nguyen added 1 comment![]( "Open in Gerrit")
Drive by comment. The routing logic makes sense to me. However, we are seeing that PEPC is actually disabled under <webview>.
If it's disabled, does this code path get hit in production right now, or is this laying the plumbing for an upcoming CL that will enable PEPC for guests? Or anything I might be misunderstanding between the guest_view/web_view and <webview>?
Thomas Nguyen (Gerrit)
Thomas Nguyen added 1 comment![]( "Open in Gerrit")
Drive by comment. The routing logic makes sense to me. However, we are seeing that PEPC is actually disabled under <webview>.
If it's disabled, does this code path get hit in production right now, or is this laying the plumbing for an upcoming CL that will enable PEPC for guests? Or anything I might be misunderstanding between the guest_view/web_view and <webview>?
.
Devlin Cronin (Gerrit)
Devlin Cronin voted and added 5 comments![]( "Open in Gerrit")
Votes added by Devlin Cronin
| Code-Review | +1 |
5 comments
Thanks, Gio! LGTM % nits.
#include "extensions/buildflags/buildflags.h"nit: this should go in the main block of includes
"chrome-extension://llkfhpcadmdealbbjgclngjoffihfkmo/index.html");nit: prefer `app->GetResourceURL("index.html")`
Bonus: Then you probably don't need the SetID() call on ExtensionBuilder
function waitForSource() {indentation blocks should be +2, not +4
// If the permission state is already 'granted' or 'denied', the PEPCas mentioned before in this CL, please wrap lines at 80 char when possible
Related details
- Andrew Rayskiy
- Giovanni Pezzino
- Simon Hangl
Code-Coverage
Code-Owners