Add test extension acting as a basic dictation provider [chromium/src : main]
Kevin McNee (Gerrit)
Kevin McNee added 1 comment![]( "Open in Gerrit")
Related details
- David Bokan
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bokan (Gerrit)
David Bokan voted and added 3 comments![]( "Open in Gerrit")
Votes added by David Bokan
| Code-Review | +1 |
3 comments
// This is a short-term solution to grant camera and/or microphone access to_laughs in git blame_
stopRecognition();Should we sendResponse here as well so that we can stop the UI only when the reply comes? e.g. if there's an exception handling the end message we shouldn't close the UI / show it as stopped
Related details
- Kevin McNee
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 added 2 comments![]( "Open in Gerrit")
let creatingDocument;Kevin McNeenit: initialize to `null`
Done
Should we sendResponse here as well so that we can stop the UI only when the reply comes? e.g. if there's an exception handling the end message we shouldn't close the UI / show it as stopped
For a production impl, sure. But I don't think this stop function can fail.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin McNee (Gerrit)
Kevin McNee added 1 comment![]( "Open in Gerrit")
Adding the following for allowlisting this test extension for audioCapture:
tjudkins@ for _permission_features.json and extension_constants.h
liberato@ for extension_media_access_handler.cc
PTAL.
CC andreaorru@. FYI.
Related details
- Frank Liberato
- Tim
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Frank Liberato (Gerrit)
Frank Liberato voted Code-Review+1![]( "Open in Gerrit")
- Kevin McNee
- Tim
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tim (Gerrit)
Tim added 2 comments![]( "Open in Gerrit")
Thanks Kevin, one main question about how this test extension is actually intended to be used and how we can avoid adding the allowlist for the test extension.
extension->id() == extension_misc::kDictationTestExtensionId;I don't like having to have an entry in here just for a test-only extension, that's generally an anti-pattern we try to avoid. Depending on how this test extension is actually used in tests, there's probably a better test-specific way to ensure it gets the permission grants it requires.
Which brings up the other main point: This CL is adding the test extension and related permission grants, but no actual usage of the extension in tests, so it's hard to gauge correctness. Could you add the actual tests that will use the test extension into this CL?
Related details
- Kevin McNee
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 added 1 comment![]( "Open in Gerrit")
extension->id() == extension_misc::kDictationTestExtensionId;I don't like having to have an entry in here just for a test-only extension, that's generally an anti-pattern we try to avoid. Depending on how this test extension is actually used in tests, there's probably a better test-specific way to ensure it gets the permission grants it requires.
Which brings up the other main point: This CL is adding the test extension and related permission grants, but no actual usage of the extension in tests, so it's hard to gauge correctness. Could you add the actual tests that will use the test extension into this CL?
Okay, this function appears to essentially be re-implementing the allowlist property from _permission_features.json. I've rewritten this to check the manifest availability from the extensions code. This also means it'll respect `--allowlisted-extension-id`. PTAL.
Were you just referring to hard coding the id for this function? Or do you have a concern with its inclusion in the _permission_features.json allowlist? I'd like to have it there for manual testing convenience. But I suppose using `--allowlisted-extension-id` would be fine. PLMK.
As for the purpose of this extension: At this stage, it's basically just a prototype showing we can go from voice to having a transcription in the extension's sw, subsequently to be sent to the browser. So it's just manual at this point. It'll do something more useful once more of the dictation code is built out.
Related details
- David Bokan
- Frank Liberato
- Tim
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. |
Tim (Gerrit)
Tim added 3 comments![]( "Open in Gerrit")
Thanks Kevin, a couple more comments. As mentioned in the reply comment, I think it'd be better to hold off on adding this till we've got tests that require (and exercise) it. WDYT?
extension->id() == extension_misc::kDictationTestExtensionId;Kevin McNeeI don't like having to have an entry in here just for a test-only extension, that's generally an anti-pattern we try to avoid. Depending on how this test extension is actually used in tests, there's probably a better test-specific way to ensure it gets the permission grants it requires.
Which brings up the other main point: This CL is adding the test extension and related permission grants, but no actual usage of the extension in tests, so it's hard to gauge correctness. Could you add the actual tests that will use the test extension into this CL?
Okay, this function appears to essentially be re-implementing the allowlist property from _permission_features.json. I've rewritten this to check the manifest availability from the extensions code. This also means it'll respect `--allowlisted-extension-id`. PTAL.
Were you just referring to hard coding the id for this function? Or do you have a concern with its inclusion in the _permission_features.json allowlist? I'd like to have it there for manual testing convenience. But I suppose using `--allowlisted-extension-id` would be fine. PLMK.
As for the purpose of this extension: At this stage, it's basically just a prototype showing we can go from voice to having a transcription in the extension's sw, subsequently to be sent to the browser. So it's just manual at this point. It'll do something more useful once more of the dictation code is built out.
If this extension is a manual prototype and is not actually being used in any tests at the moment, I don't think it makes sense to go into the codebase like this yet. I appreciate trying to split changes into smaller more reviewable pieces, but I think it would be better to hold off on adding this until there's an actual test consuming it in some way. I imagine in practice this will probably end up being more like a test delegate for the dictation provider?
Once there's actually something exercising this I think the permission side of this will be a lot clearer also. If this acts as a component extension the permission feature allowlist [wont be required](https://source.chromium.org/chromium/chromium/src/+/main:extensions/common/features/simple_feature.cc;l=709;drc=fcc8e2e643f4beab78d87a7a8566f0ed50332666). If it's just an extension installed in a test it can be granted the permission `--allowlisted-extension-id` like you said (or using SimpleFeature::ScopedThreadUnsafeAllowlistForTest in unittests). `--allowlisted-extension-id` also bypasses the [allowlist check](https://source.chromium.org/chromium/chromium/src/+/main:extensions/common/features/simple_feature.cc;l=713;drc=fcc8e2e643f4beab78d87a7a8566f0ed50332666) and [manifest location check](https://source.chromium.org/chromium/chromium/src/+/main:extensions/common/features/simple_feature.cc;l=718;drc=fcc8e2e643f4beab78d87a7a8566f0ed50332666), so as long as there's a feature block that matches all the other fields (e.g. extension type, manifest version) it is made available.
If this still needs to bypass the audio capture permission dialog (which it probably will if it's being triggered from the offscreen document), it'll probably make more sense for it to use the ID of the actual component extension and have that entry added to the list in this file, akin to the virtual keyboard in here which has a duplicate test extension for some testing.
for (const std::string& capture_permission_name :
{"audioCapture", "videoCapture"}) {
const extensions::Feature* capture_permission =
extensions::FeatureProvider::GetPermissionFeature(
capture_permission_name);
if (capture_permission) {
extensions::Feature::Availability availability =
capture_permission->IsAvailableToExtension(extension);
if (availability.is_available()) {
return true;
}
}
}Going this route might seem innocuous at first, because in theory extensions should be allowlisted for `audioCapture` or `videoCapture` to be able to get here anyway. But as mentioned in the other comment component extensions bypass the allowlists (as do `--allowlisted-extension-id` loaded extensions). Since this class is about allowing an extension to bypass the associated user dialog to grant permission, this change would allow any of those extensions to now be able to silently turn on microphone and video capture.
Related details
- David Bokan
- Frank Liberato
- Kevin McNee
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 added 1 comment![]( "Open in Gerrit")
for (const std::string& capture_permission_name :
{"audioCapture", "videoCapture"}) {
const extensions::Feature* capture_permission =
extensions::FeatureProvider::GetPermissionFeature(
capture_permission_name);
if (capture_permission) {
extensions::Feature::Availability availability =
capture_permission->IsAvailableToExtension(extension);
if (availability.is_available()) {
return true;
}
}
}Going this route might seem innocuous at first, because in theory extensions should be allowlisted for `audioCapture` or `videoCapture` to be able to get here anyway. But as mentioned in the other comment component extensions bypass the allowlists (as do `--allowlisted-extension-id` loaded extensions). Since this class is about allowing an extension to bypass the associated user dialog to grant permission, this change would allow any of those extensions to now be able to silently turn on microphone and video capture.
The name of this helper function might be misleading. This is only called from SupportsStreamType. This does not actually grant the permission. For example, if I don't declare audioCapture in c/t/d/e/dictation/manifest.json, but still have it in the allowlist in _permission_features.json. Then the attempt to access the mic is rejected.
Related details
- David Bokan
- Frank Liberato
- Tim
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. |
Tim (Gerrit)
Tim added 1 comment![]( "Open in Gerrit")
for (const std::string& capture_permission_name :
{"audioCapture", "videoCapture"}) {
const extensions::Feature* capture_permission =
extensions::FeatureProvider::GetPermissionFeature(
capture_permission_name);
if (capture_permission) {
extensions::Feature::Availability availability =
capture_permission->IsAvailableToExtension(extension);
if (availability.is_available()) {
return true;
}
}
}Kevin McNeeGoing this route might seem innocuous at first, because in theory extensions should be allowlisted for `audioCapture` or `videoCapture` to be able to get here anyway. But as mentioned in the other comment component extensions bypass the allowlists (as do `--allowlisted-extension-id` loaded extensions). Since this class is about allowing an extension to bypass the associated user dialog to grant permission, this change would allow any of those extensions to now be able to silently turn on microphone and video capture.
The name of this helper function might be misleading. This is only called from SupportsStreamType. This does not actually grant the permission. For example, if I don't declare audioCapture in c/t/d/e/dictation/manifest.json, but still have it in the allowlist in _permission_features.json. Then the attempt to access the mic is rejected.
Fair, they still need to declare the permission name in the manifest, but I still feel this is a big change to what requesting the permission actually grants. If any of the existing extensions using these APIs (calling them from a user visible page) were relying on the permission dialog as part of their privacy/security model, this is a big change to that model.
But maybe this is all moot and all the allowlisted entries in _permission_features.json (and any component/external component extensions declaring the permission) that are still relevant already have an entry in here to bypass the dialog anyway? When you went through the _permission_features.json allowlist for the comment below did you see any that are still active but not listed in here?
Related details
- David Bokan
- Frank Liberato
- Kevin McNee
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 added 1 comment![]( "Open in Gerrit")
for (const std::string& capture_permission_name :
{"audioCapture", "videoCapture"}) {
const extensions::Feature* capture_permission =
extensions::FeatureProvider::GetPermissionFeature(
capture_permission_name);
if (capture_permission) {
extensions::Feature::Availability availability =
capture_permission->IsAvailableToExtension(extension);
if (availability.is_available()) {
return true;
}
}
}Kevin McNeeGoing this route might seem innocuous at first, because in theory extensions should be allowlisted for `audioCapture` or `videoCapture` to be able to get here anyway. But as mentioned in the other comment component extensions bypass the allowlists (as do `--allowlisted-extension-id` loaded extensions). Since this class is about allowing an extension to bypass the associated user dialog to grant permission, this change would allow any of those extensions to now be able to silently turn on microphone and video capture.
TimThe name of this helper function might be misleading. This is only called from SupportsStreamType. This does not actually grant the permission. For example, if I don't declare audioCapture in c/t/d/e/dictation/manifest.json, but still have it in the allowlist in _permission_features.json. Then the attempt to access the mic is rejected.
Fair, they still need to declare the permission name in the manifest, but I still feel this is a big change to what requesting the permission actually grants. If any of the existing extensions using these APIs (calling them from a user visible page) were relying on the permission dialog as part of their privacy/security model, this is a big change to that model.
But maybe this is all moot and all the allowlisted entries in _permission_features.json (and any component/external component extensions declaring the permission) that are still relevant already have an entry in here to bypass the dialog anyway? When you went through the _permission_features.json allowlist for the comment below did you see any that are still active but not listed in here?
There are some entries in the audioCapture allowlist that aren't here, but they're either test extensions (associated with http://crbug.com/496954 ) or what appears to be a disused google internal extension ( http://crbug.com/431978 ).
Related details
- David Bokan
- Frank Liberato
- Tim
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 added 1 comment![]( "Open in Gerrit")
for (const std::string& capture_permission_name :
{"audioCapture", "videoCapture"}) {
const extensions::Feature* capture_permission =
extensions::FeatureProvider::GetPermissionFeature(
capture_permission_name);
if (capture_permission) {
extensions::Feature::Availability availability =
capture_permission->IsAvailableToExtension(extension);
if (availability.is_available()) {
return true;
}
}
}Kevin McNeeGoing this route might seem innocuous at first, because in theory extensions should be allowlisted for `audioCapture` or `videoCapture` to be able to get here anyway. But as mentioned in the other comment component extensions bypass the allowlists (as do `--allowlisted-extension-id` loaded extensions). Since this class is about allowing an extension to bypass the associated user dialog to grant permission, this change would allow any of those extensions to now be able to silently turn on microphone and video capture.
TimThe name of this helper function might be misleading. This is only called from SupportsStreamType. This does not actually grant the permission. For example, if I don't declare audioCapture in c/t/d/e/dictation/manifest.json, but still have it in the allowlist in _permission_features.json. Then the attempt to access the mic is rejected.
Kevin McNeeFair, they still need to declare the permission name in the manifest, but I still feel this is a big change to what requesting the permission actually grants. If any of the existing extensions using these APIs (calling them from a user visible page) were relying on the permission dialog as part of their privacy/security model, this is a big change to that model.
But maybe this is all moot and all the allowlisted entries in _permission_features.json (and any component/external component extensions declaring the permission) that are still relevant already have an entry in here to bypass the dialog anyway? When you went through the _permission_features.json allowlist for the comment below did you see any that are still active but not listed in here?
There are some entries in the audioCapture allowlist that aren't here, but they're either test extensions (associated with http://crbug.com/496954 ) or what appears to be a disused google internal extension ( http://crbug.com/431978 ).
I suppose a minimal change here would be to check for component extensions or the use of allowlisted-extension-id. But it would seem unfortunate to me to have ad-hoc extension permission logic outside of extension code.