[Extensions] Support webRequest.onAuthRequired in MV3 [chromium/src : main]
Devlin Cronin (Gerrit)
Attention is currently required from: David Bertoni.
Devlin Cronin would like David Bertoni to review this change.
[Extensions] Support webRequest.onAuthRequired in MV3
Add support for webRequest.onAuthRequired in Manifest V3. Unlike many of
the other use cases handled in webRequest, handling onAuthRequired
doesn't lend itself to a declarative model.
Instead, introduce a new permission, `webRequestAuthProvider`, that
extensions can use if they want to handle the webRequest.onAuthRequired
event with a blocking (or asyncBlocking) listener. Like
`webRequestBlocking`, this permission does not add a new warning, as its
functionality is covered by the warning for the host permission (which
the extension requires in order to intercept the calls).
Add the new permission and wire up the functionality in the webRequest
API, adding tests for both MV3 extensions with and without the new
permission (where it is expected to fail without the new permission).
More details in the design doc here:
https://docs.google.com/document/d/1iNonK6pgl9U5BTELR3iumfOU6TbirmC--a3GkIjUbfc/edit#
Bug: 1135492
Change-Id: Ib6bbde135014e46b12e186c24197b628622d30bf
---
M chrome/browser/extensions/api/web_request/web_request_apitest.cc
M chrome/common/extensions/permissions/permission_set_unittest.cc
M extensions/browser/api/web_request/web_request_api.cc
M extensions/common/api/_permission_features.json
M extensions/common/mojom/api_permission_id.mojom
M extensions/common/permissions/extensions_api_permissions.cc
M tools/metrics/histograms/enums.xml
7 files changed, 202 insertions(+), 6 deletions(-)
To view, visit change 3923277. To unsubscribe, or for help writing mail filters, visit settings.
Devlin Cronin (Gerrit)
Attention is currently required from: David Bertoni.
2 comments:
Patchset:
Dave, please take a look when you have a chance
Dave, please take a look when you have a chance
To view, visit change 3923277. To unsubscribe, or for help writing mail filters, visit settings.
David Bertoni (Gerrit)
Attention is currently required from: Devlin Cronin.
Patch set 3:Code-Review +1
4 comments:
Patchset:
lgtm, thanks!
File chrome/browser/extensions/api/web_request/web_request_apitest.cc:
Patch Set #3, Line 5980:
{username: 'foo', password: 'secret'}Nit: Maybe just inline this on line 5982?
File extensions/browser/api/web_request/web_request_api.cc:
bool is_auth_event = event_name == keys::kOnAuthRequiredEvent;
if (is_auth_event && extension->permissions_data()->HasAPIPermission(
APIPermissionID::kWebRequestAuthProvider)) {
return true;
}
return false;Nit: Since we seem to love early returns in Chrome:
````c
if (event_name != keys::kOnAuthRequiredEvent)
return false;
return extension->permissions_data()->HasAPIPermission(
APIPermissionID::kWebRequestAuthProvider);
````
That seems more concise to me.
File extensions/common/permissions/extensions_api_permissions.cc:
Patch Set #3, Line 165:
{APIPermissionID::kWebRequestAuthProvider, "webRequestAuthProvider"},Nit: This isn't in alpha order like the rest of the permissions.
To view, visit change 3923277. To unsubscribe, or for help writing mail filters, visit settings.
Devlin Cronin (Gerrit)
4 comments:
Patchset:
Thanks, Dave!
File chrome/browser/extensions/api/web_request/web_request_apitest.cc:
Patch Set #3, Line 5980:
{username: 'foo', password: 'secret'}Nit: Maybe just inline this on line 5982?
that's actually not quite as clean IMO, because right now we're using the object initialization that sets `authCredentials` as a property + value on the object. Inlining it would make it:
```
callback(
{
authCredentials: {username: 'foo', password: 'secret'}
});
```Which I think is a bit clunkier
File extensions/browser/api/web_request/web_request_api.cc:
bool is_auth_event = event_name == keys::kOnAuthRequiredEvent;
if (is_auth_event && extension->permissions_data()->HasAPIPermission(
APIPermissionID::kWebRequestAuthProvider)) {
return true;
}
return false;
Nit: Since we seem to love early returns in Chrome: […]
if we're going for concise, we can just combine the name and permission check in the return statement; done.
File extensions/common/permissions/extensions_api_permissions.cc:
Patch Set #3, Line 165:
{APIPermissionID::kWebRequestAuthProvider, "webRequestAuthProvider"},Nit: This isn't in alpha order like the rest of the permissions.
Whoops! Fixed
To view, visit change 3923277. To unsubscribe, or for help writing mail filters, visit settings.
Devlin Cronin (Gerrit)
Attention is currently required from: Alex Gough.
Devlin Cronin would like Alex Gough to review this change.
7 files changed, 199 insertions(+), 6 deletions(-)
Devlin Cronin (Gerrit)
Patch set 4:Auto-Submit +1
1 comment:
Patchset:
Alex, mind taking a look at permission_id.mojom?
To view, visit change 3923277. To unsubscribe, or for help writing mail filters, visit settings.
Devlin Cronin (Gerrit)
Attention is currently required from: Alex Gough.
1 comment:
Patchset:
Wow, Gerrit is having issues...
Alex, mind taking a look at the mojom?
To view, visit change 3923277. To unsubscribe, or for help writing mail filters, visit settings.
Findit (Gerrit)
File extensions/browser/api/web_request/web_request_api.cc:
Robot Comment from Chromium Coverage Checker (run ID chromium/src~3923277~4):
Incremental Coverage (All Tests) for this file is below 80 %. Please add tests for uncovered lines.
To view, visit change 3923277. To unsubscribe, or for help writing mail filters, visit settings.
Devlin Cronin (Gerrit)
File extensions/browser/api/web_request/web_request_api.cc:
I don't believe you, FindIt.
The added lines are all tested in the newly added browser tests, which is supported by Gerrit not showing any un-covered lines in this section.
To view, visit change 3923277. To unsubscribe, or for help writing mail filters, visit settings.
Alex Gough (Gerrit)
Attention is currently required from: Devlin Cronin.
Patch set 4:Code-Review +1Commit-Queue +2
1 comment:
Patchset:
lgtm mojom
To view, visit change 3923277. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
[Extensions] Support webRequest.onAuthRequired in MV3
Add support for webRequest.onAuthRequired in Manifest V3. Unlike many of
the other use cases handled in webRequest, handling onAuthRequired
doesn't lend itself to a declarative model.
Instead, introduce a new permission, `webRequestAuthProvider`, that
extensions can use if they want to handle the webRequest.onAuthRequired
event with a blocking (or asyncBlocking) listener. Like
`webRequestBlocking`, this permission does not add a new warning, as its
functionality is covered by the warning for the host permission (which
the extension requires in order to intercept the calls).
Add the new permission and wire up the functionality in the webRequest
API, adding tests for both MV3 extensions with and without the new
permission (where it is expected to fail without the new permission).
More details in the design doc here:
https://docs.google.com/document/d/1iNonK6pgl9U5BTELR3iumfOU6TbirmC--a3GkIjUbfc/edit#
Bug: 1135492
Change-Id: Ib6bbde135014e46b12e186c24197b628622d30bf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3923277
Auto-Submit: Devlin Cronin <rdevlin...@chromium.org>
Commit-Queue: Alex Gough <aj...@chromium.org>
Reviewed-by: David Bertoni <dber...@chromium.org>
Reviewed-by: Alex Gough <aj...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1054834}
---
M chrome/browser/extensions/api/web_request/web_request_apitest.cc
M chrome/common/extensions/permissions/permission_set_unittest.cc
M extensions/browser/api/web_request/web_request_api.cc
M extensions/common/api/_permission_features.json
M extensions/common/mojom/api_permission_id.mojom
M extensions/common/permissions/extensions_api_permissions.cc
M tools/metrics/histograms/enums.xml
7 files changed, 205 insertions(+), 6 deletions(-)