[Extensions] add developerPrivate.updateAccessSettingsForSite [chromium/src : main]
Kelvin Jiang (Gerrit)
Attention is currently required from: Devlin Cronin, Emilia Paz.
Kelvin Jiang would like Emilia Paz and Devlin Cronin to review this change.
[Extensions] add developerPrivate.updateAccessSettingsForSite
This CL adds a new API method which changes the site access settings
for extensions relative to a single site/url pattern.
The states that can be changed to for a site S are:
- ON_CLICK: Revokes all runtime granted permissions (RGP) that have
some intersection with S. If extension has no withheld permissions,
then withhold all of its host permissions
- ON_SPECIFIC_SITES: (always for this site): Adds S to the extension's
RGP. Clears the extension's host permissions first before adding S
if the extension has no withheld host permissions
- ON_ALL_SITES: removes all of the extension's withheld host
permissions.
This API method only returns once all the updates have been finished,
which is accomplished using a barrier closure.
Bug: 1378098
Change-Id: I6a4fd6bc4809e24d0ebe577626e566896d8e5a98
---
M chrome/browser/extensions/api/developer_private/developer_private_api.cc
M chrome/browser/extensions/api/developer_private/developer_private_api.h
M chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc
M chrome/browser/extensions/permissions_updater.cc
M chrome/common/extensions/api/developer_private.idl
M extensions/browser/extension_function_histogram_value.h
M third_party/closure_compiler/externs/developer_private.js
M tools/metrics/histograms/enums.xml
8 files changed, 347 insertions(+), 3 deletions(-)
To view, visit change 3943191. To unsubscribe, or for help writing mail filters, visit settings.
Kelvin Jiang (Gerrit)
Attention is currently required from: Devlin Cronin, Emilia Paz.
Patch set 6:Commit-Queue +1
1 comment:
Patchset:
LMK if I should add anyone else for this
To view, visit change 3943191. To unsubscribe, or for help writing mail filters, visit settings.
Emilia Paz (Gerrit)
Attention is currently required from: Devlin Cronin, Kelvin Jiang.
7 comments:
Patchset:
THanks Kelvin, host permissions CLs are always very tricky!
I took a couple passes at this, and I think we may be removing more permissions than the ones we want. However, it may be due to pattern behavior I am not taking into account. We should add some test or play with the UI to see if the examples I added on the comments behave properly.
Separately, maybe we want the grant/revoke behavior in the scripting permissions modifier? But that can be done after this CL (and once we see how it ties with other places)
File chrome/browser/extensions/api/developer_private/developer_private_api.h:
Patch Set #6, Line 961:
updateAccessSettingsForSitenit: "updateSiteAccess" or "updateSiteAccessSettings"
File chrome/browser/extensions/api/developer_private/developer_private_api.cc:
withhold all of its
// permissions before granting `site`.First I wanna say sorry, I think I caused confusion with our offline conversation.
This method is for all changes to on site, which covers:
a. on click -> on site
- sites that match pattern change to "on site"
- other sites keep their site access setting (note that sites couldn't be "on all sites" at this point)
b. on all sites -> on site
- sites that match pattern change to "on site"
- other sites change to "on click" since broad permissions were lostWhy would we need to withheld all its permissions before granting site?
Say we have extension A with "on site" access to x.com and "on click" access to y.com. Then we change y.com to "on site" (which would call this method). If we remove all granted permissions, then x.com would lose its access (*)
We need to:
1. Remove broad granted host permissions, if any
2. Set withholding permissions to true
2. Grant host permission to site, if it's not already granted.This should be similar to `ExtensionActionRunner::UpdatePageAccessSettings` for on site [1].
However, I may be missing something with the "match all patterns" behavior and there is a reason to remove all granted permissions. Is this because you could have a "broad enough" pattern that is not considered broad but would match the pattern? (something like *://*.a.com/*). We should add a test to see if this example (*) is been covered.Patch Set #6, Line 438:
exceottypo: except
Patch Set #6, Line 441:
modifier->RemoveAllGrantedHostPermissions();This method is for all changes to on click, which covers:
a. on site -> on click
- sites that match the pattern change to "on click"
- other sites keep their site access setting (note that sites couldn't be "on all sites" at this point)
b. on all sites -> on click
- every site change to "on click"
Why do we remove all granted host permissions? If we do that, we would not cover case a., since we would be removing another site permission.E.g: say we have extension A with "on site" access to x.com and "on site" access to y.com. Then we change y.com to "on click" (which would call this method)
If we remove all granted host permissions, it would revoke access from x.com but x.com hasn't changed its access.
I think we should be:
1. Removing broad granted host permissions to revoke host permission patterns that grant access to ALL urls, if any (to cover b.)
2. Set withholding permissions to true (to cover a. and b.)
3. Remove granted host permissions to sites that match url pattern (to cover b.)This would be similar to `ExtensionActionRunner::UpdatePageAccessSettings` for on click` [1]
Same as previous comment, there may be a reason why we are removing all granted host permissions due to patterns that I am missing.
// Revoke all sites which have some intersection with `site` from the
// extension's set of runtime granted host permissions.
URLPatternSet hosts_to_withhold;
std::unique_ptr<const PermissionSet> runtime_granted_permissions =
ExtensionPrefs::Get(context)->GetRuntimeGrantedPermissions(
extension.id());
for (const URLPattern& pattern :
runtime_granted_permissions->effective_hosts()) {
if (site.OverlapsWith(pattern))
hosts_to_withhold.AddPattern(pattern);
}
std::unique_ptr<const PermissionSet> permissions_to_remove =
PermissionSet::CreateIntersection(
PermissionSet(APIPermissionSet(), ManifestPermissionSet(),
hosts_to_withhold.Clone(), hosts_to_withhold.Clone()),
*modifier->GetRevokablePermissions(),
URLPatternSet::IntersectionBehavior::kDetailed);
DCHECK(!permissions_to_remove->IsEmpty());
PermissionsUpdater(context).RevokeRuntimePermissions(
extension, *permissions_to_remove, done_callback);This is effectively 3. from previous comment
It makes senses taking the intersection between hosts to withhold, and revokable permissions (we only want to withheld permissions that can be withheld to begin with).
However, ScriptingPermissionsModifier::RemoveGrantedHostPermission` doesn't take the intersection? Seems it is sufficient to check if "host permissions were granted" and "permissions can affect the extension". Why do we take the intersection here? (I think it's correct to have the intersection, mainly wondering why we don't do it on the other side.. maybe a question for Devlin)
File chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:
updates.clear();
updates.push_back(CreateSiteAccessUpdate(
extension->id(), developer::HOST_ACCESS_ON_SPECIFIC_SITES));
UpdateAccessSettingsForSite(profile(), "*://*.example.com/*", updates);Should we check that google.com doesn't have access after losing broad host permissions?
To view, visit change 3943191. To unsubscribe, or for help writing mail filters, visit settings.
Devlin Cronin (Gerrit)
Attention is currently required from: Kelvin Jiang.
1 comment:
Patchset:
Thanks for taking a look, Emilia! I'll take a pass on this after Emilia stamps; please add me back to the attention set then : )
To view, visit change 3943191. To unsubscribe, or for help writing mail filters, visit settings.
Kelvin Jiang (Gerrit)
Attention is currently required from: Emilia Paz.
Patch set 8:Commit-Queue +1
5 comments:
File chrome/browser/extensions/api/developer_private/developer_private_api.h:
Patch Set #6, Line 961:
updateAccessSettingsForSitenit: "updateSiteAccess" or "updateSiteAccessSettings"
+ @rdevlin...@chromium.org for his input on naming
File chrome/browser/extensions/api/developer_private/developer_private_api.cc:
withhold all of its
// permissions before granting `site`.
First I wanna say sorry, I think I caused confusion with our offline conversation. […]
ack (resolved offline, also added comment mentioning that no withheld permissions = extension can run on all of its requested hosts)
Patch Set #6, Line 438:
exceottypo: except
Done
Patch Set #6, Line 441:
modifier->RemoveAllGrantedHostPermissions();
This method is for all changes to on click, which covers: […]
as discussed offline, permissions are only revoked when the "withhold host permissions/use runtime host permissions" flag in prefs is false which means the extension can run on all of its requested sites.
This is done in this case because currently, runtime granted host permissions is an allowlist instead of a blocklist.
File chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:
updates.clear();
updates.push_back(CreateSiteAccessUpdate(
extension->id(), developer::HOST_ACCESS_ON_SPECIFIC_SITES));
UpdateAccessSettingsForSite(profile(), "*://*.example.com/*", updates);
Should we check that google. […]
This is already implicitly checked by directly equating the set of runtime granted permissions and the extension's active permissions to expected values. Note that both sets do not include google.com
To view, visit change 3943191. To unsubscribe, or for help writing mail filters, visit settings.
Emilia Paz (Gerrit)
Attention is currently required from: Kelvin Jiang.
6 comments:
Patchset:
Yesterday I thought we had covered all cases. Of course I found one more today while diving deeper. See comment on #440. This is complicated.. whatever we decided should be align for all callers (chrome://extensions behavior is the same as toolbar UI - handling patterns and origins respectively)
File chrome/browser/extensions/api/developer_private/developer_private_api.cc:
Patch Set #8, Line 417:
modifier->HasWithheldHostPermissions(Prefer `PermissionsManager::HasWithheldHostPermissions` (what you had before).
]
I agree that it's easier to read when all methods are from "modifier", but `ScriptingPermissionsModifier::HasWithheldHostPermissions` will be removed after all its callers are migrated.Patch Set #8, Line 417:
(!modifier->HasWithheldHostPermissions())This works. However, I am still wondering why here we use `HasWithheldHostPermissions` and ExtensionActionRunner uses `HasBroadGrantedHostPermissions` [1]. I believe both are equivalent for this scenario (you can only get here if you come from "on all sites" or "on click", and both methods cover the "on all sites" scenario). @rdevlin...@chromium.org Is this correct?
However, could this method get called for an extension that is "on site"? Extension has no withheld permissions, so we would remove all granted host permissions (which can wrongly remove for other extensions that were set "on site").
Thus, I would either
a. Move this to the caller (similar to `ExtensionActionRunner`) to avoid the risk of been used by a caller that has extension "on site":
```
case developer::HOST_ACCESS_ON_SPECIFIC_SITES:
if (has withheld host permissions)
....
```
b. Add a comment with a warning for "from on site" scenario.c. Use `HasBroadGrantedHostPermissions` (assuming I am correct in that both are equivalent)
Patch Set #8, Line 419:
modifier->RemoveAllGrantedHostPermissions();Similarly here. `RemoveAllGrantedHostPermissions` is used, whereas ExtensionActionRunner uses `RemoveBroadGrantedHostPermissions()`. The reason here is because of what is used in the if statement (whether we check for withheld host permissions or broad granted). Thus, this is correct as long as we don't change the if statement (stay with current or a.)
Patch Set #8, Line 440:
!modifier->HasWithheldHostPermissions()This wouldn't work (sorry for not catching this on our 1:1) for a very specific case:
- Extension only requests access to 2 sites (x.com, y.com) and both are "on site"
- This means "has withheld host permissions" is false, because no permissions are withheld. If we change x.com to "on click", then here we would also remove access to y.com.
This is a scenario that gets further complicated with "Automatically allow access on the following sites" toggle in chrome://extensions/id=#. Current behavior is explained in this doc with reproduction steps and screencast [1]. Toggle is treated as broad permissions, and therefore when choosing "on click" with toggle on makes every requested site go to "on site" (I think this is what caused my confusion with granularity!)
This is the part where I am confused. Are we covering both scenarios (from the doc) here? What would happen for each? I tried to create tests, but it's tricky because I am not sure what "toggle on/off" means. Is this toggle present in the new c2s chrome://extension ui?
I think it's important to align in what is the behavior wanted. Just to double check my understanding:
Changing x.com from "on site" to "on click"
* x.com (and matching sites) change to "on click"
* every other requested site stays on their current site access (either "on site" or "on click")Ideally both chrome://extensions (through developer API) and toolbar UI (through site permissions helper) handle scenarios the same way.
[1] https://docs.google.com/document/d/1entW7pvwGBFvXq_Pm8MKIDtcmt6x6__CmS7OXg_J2_4/edit?resourcekey=0-qQF19xy6_eZQk32SB06kyQ#Patch Set #8, Line 440:
modifierditto: prefer `PermissionsManager::HasWithheldHostPermissions`
To view, visit change 3943191. To unsubscribe, or for help writing mail filters, visit settings.
Emilia Paz (Gerrit)
Attention is currently required from: Kelvin Jiang.
1 comment:
File chrome/browser/extensions/api/developer_private/developer_private_api.cc:
Patch Set #8, Line 417:
(!modifier->HasWithheldHostPermissions())This works. […]
uh oh. To even add more confusion, I created a test that fails when on click -> on site. Maybe we are not granting the pattern properly?
See crrev.com/c/3999978 with the test on this CL cherrypicked.
To view, visit change 3943191. To unsubscribe, or for help writing mail filters, visit settings.
Emilia Paz (Gerrit)
File chrome/browser/extensions/api/developer_private/developer_private_api.cc:
Patch Set #8, Line 440:
!modifier->HasWithheldHostPermissions()This wouldn't work (sorry for not catching this on our 1:1) for a very specific case: […]
Discussed offline with Kelvin.
This edge case for both scenarios A and B is handled in the same way as the SitePermissionsHelper. This is better explained in doc [1].Sorry for all the back and forward. Was just trying to make sure all cases where covered.
To view, visit change 3943191. To unsubscribe, or for help writing mail filters, visit settings.
Emilia Paz (Gerrit)
Attention is currently required from: Kelvin Jiang.
2 comments:
File chrome/browser/extensions/api/developer_private/developer_private_api.cc:
Patch Set #8, Line 417:
(!modifier->HasWithheldHostPermissions())uh oh. To even add more confusion, I created a test that fails when on click -> on site. […]
For the comment 1 on this thread: the danger of "on site" -> "on site" removing all granted permissions is not valid, because of what was discussed in `RevokePermissionsForSite`
Thus, this lgtm.
Only thing left is the issue displayed in crrev.com/c/3999978
Patch Set #8, Line 419:
modifier->RemoveAllGrantedHostPermissions();
Similarly here. […]
Not applicable anymore.
To view, visit change 3943191. To unsubscribe, or for help writing mail filters, visit settings.
Kelvin Jiang (Gerrit)
Attention is currently required from: Emilia Paz.
Patch set 9:Commit-Queue +1
5 comments:
File chrome/browser/extensions/api/developer_private/developer_private_api.cc:
// Revoke all sites which have some intersection with `site` from the
// extension's set of runtime granted host permissions.
URLPatternSet hosts_to_withhold;
std::unique_ptr<const PermissionSet> runtime_granted_permissions =
ExtensionPrefs::Get(context)->GetRuntimeGrantedPermissions(
extension.id());
for (const URLPattern& pattern :
runtime_granted_permissions->effective_hosts()) {
if (site.OverlapsWith(pattern))
hosts_to_withhold.AddPattern(pattern);
}
std::unique_ptr<const PermissionSet> permissions_to_remove =
PermissionSet::CreateIntersection(
PermissionSet(APIPermissionSet(), ManifestPermissionSet(),
hosts_to_withhold.Clone(), hosts_to_withhold.Clone()),
*modifier->GetRevokablePermissions(),
URLPatternSet::IntersectionBehavior::kDetailed);
DCHECK(!permissions_to_remove->IsEmpty());
PermissionsUpdater(context).RevokeRuntimePermissions(
extension, *permissions_to_remove, done_callback);
This is effectively 3. from previous comment […]
as discussed offline: we're populating `hosts_to_withhold` with all runtime granted permissions that have a non-empty intersection with `site`.
The intersection here is more like a sanity check to make sure we are only removing permissions that can be removed
File chrome/browser/extensions/api/developer_private/developer_private_api.cc:
Patch Set #8, Line 417:
modifier->HasWithheldHostPermissions(Prefer `PermissionsManager::HasWithheldHostPermissions` (what you had before). […]
Done
Patch Set #8, Line 417:
(!modifier->HasWithheldHostPermissions())
For the comment 1 on this thread: the danger of "on site" -> "on site" removing all granted permissi […]
ack, added a test case for what you were trying to do in that CL (ON_CLICK to ON_SPECIFIC_SITES)
would it make sense to add
```
if (modifier.HasBroadGrantedHostPermissions())
modifier.RemoveBroadGrantedHostPermissions();
```here, even if there's a chance the site may not be within the set of "broad granted host permissions"?
e.g. site to be granted is `http://google.ca/*` but the broad granted host permissions is something like `*://*.com/*` ?
Patch Set #8, Line 440:
modifierditto: prefer `PermissionsManager::HasWithheldHostPermissions`
Done
To view, visit change 3943191. To unsubscribe, or for help writing mail filters, visit settings.
Emilia Paz (Gerrit)
Attention is currently required from: Kelvin Jiang.
Patch set 9:Code-Review +1
3 comments:
Patchset:
LGTM. Thanks for clarifying many edge cases, host permissions are very tricky.
Once this is hooked with the UI we will be able to see how it affects global state better (user site permissions, toolbar UI).
File chrome/browser/extensions/api/developer_private/developer_private_api.cc:
Patch Set #9, Line 409:
GrantPermissionsForSiteMeta comment: I wish "grant / revoke" host permissions methods were all in one place.
Currently we have:
```
class ScriptingPermissionsModifier {
void GrantHostPermission(const GURL& url);
void RemoveGrantedHostPermission(const GURL& url);
}
```
we could extend that with:
```
void GrantHostPermission(const URLPattern& site, base::RepeatingClosure done_callback);
void RemoveGrantedHostPermission(const URLPattern& site, base::RepeatingClosure done_callback);
```Probably out of scope for this CL, but something to keep in mind. Permissions scope is growing, and I am afraid that if we keep adding similar methods in different places a refactor will become even harder.
(Note, there is a catch with "Remove". `DeveloperPrivateRemoveHostPermissionFunction` doesn't check if the site overlaps with the pattern as `RevokePermissionsForSite` does [1])
Patch Set #9, Line 468:
DCHECK(!permissions_to_remove->IsEmpty());optional: Should we return an error instead of DCHECK, similar to `RemoveHostPermission` [1]?
Thinking this again, DCHECK makes more sense because that scenario shouldn't happen
To view, visit change 3943191. To unsubscribe, or for help writing mail filters, visit settings.
Kelvin Jiang (Gerrit)
1 comment:
Patchset:
+Devlin for a pass since Emilia has +1-ed
To view, visit change 3943191. To unsubscribe, or for help writing mail filters, visit settings.
Devlin Cronin (Gerrit)
Attention is currently required from: Emilia Paz, Kelvin Jiang.
14 comments:
Commit Message:
Patch Set #9, Line 19:
removesWhile I could see an argument that "removing a withheld permission" is granting a permission, I think just saying "grants" here would be more clear : )
,
which is accomplished using a barrier closure.
nitty nit: this part probably doesn't belong in the CL description
Patchset:
Thanks, Kelvin! This looks pretty good
File chrome/browser/extensions/api/developer_private/developer_private_api.h:
Patch Set #6, Line 961:
updateAccessSettingsForSite
+ @rdevlin.cronin@chromium. […]
They all seem equally reasonable to me, so my de facto decision making criterion is "go for the shortest one" : ) (which is updateSiteAccess.) updateSiteAccessSettings also sounds fine, if we wanted to make it more settings-y.
File chrome/browser/extensions/api/developer_private/developer_private_api.cc:
Patch Set #9, Line 409:
GrantPermissionsForSite
Meta comment: I wish "grant / revoke" host permissions methods were all in one place. […]
+1 to anything we can do to reduce the number of permissions-related functions (or entities) we have.
this can be a ref since it's never null
Patch Set #9, Line 413:
base::RepeatingClosure done_callback) {OnceClosure?
if (!PermissionsManager::Get(context)->HasWithheldHostPermissions(
extension.id())) {
modifier->SetWithholdHostPermissions(true);
modifier->RemoveAllGrantedHostPermissions();
}This is very non-obvious from the method name and function comment. Can we update those to make it more clear that this might also _remove_ permissions?
content::BrowserContext* context,
const Extension& extension,
ScriptingPermissionsModifier* modifier,
const URLPattern& site,
base::RepeatingClosure done_callback) {same comments as above
Patch Set #9, Line 2605:
return RespondNow(Error(kNoSuchExtensionError));This results in this function being non-atomic — we might remove permissions for half the extensions specified, but not the other half. What's more, we could potentially Respond multiple times if there were a pending callback for one of them (which would result in a crash).
Let's move a check to verify all the extensions exist before trying to modify any of their permissions.
Patch Set #9, Line 2609:
return RespondNow(Error(kCannotChangeHostPermissions));ditto with this check
File chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:
std::string updates_arg = "[";
for (const auto& update : updates) {
std::string update_arg = base::StringPrintf(
R"({"id": "%s", "siteAccess": "%s"},)", update.id.c_str(),
api::developer_private::ToString(update.site_access));
updates_arg = base::StrCat({updates_arg, update_arg});
}
// Remove trailing comma from the last entry.
updates_arg.pop_back();
updates_arg = base::StrCat({updates_arg, "]"});nit: I think slightly cleaner:
```
base::Value::List update_entries;
entries.reserve(updates.size());
for (const auto& update : updates) {
entries.Append(update.ToValue());
}
std::string updates_arg;
EXPECT_TRUE(base::JSONWriter::Write(update_entries, &updates_arg));
```Patch Set #9, Line 2473:
DeveloperPrivateUpdateAccessSettingsForSite_WitheldHostPermissions) {add test comments
Patch Set #9, Line 2512:
kMailGoogleComkMailGoogleCom doesn't match — it's https, while the pattern being removed is http. Instead, it's removed because the _granted_ permission (*://mail.google.com/*) matches the removed site.
To view, visit change 3943191. To unsubscribe, or for help writing mail filters, visit settings.
Kelvin Jiang (Gerrit)
Attention is currently required from: Kelvin Jiang.
Kelvin Jiang uploaded patch set #11 to this change.
[Extensions] add developerPrivate.updateAccessSettingsForSite
This CL adds a new API method which changes the site access settings
for extensions relative to a single site/url pattern.
The states that can be changed to for a site S are:
- ON_CLICK: Revokes all runtime granted permissions (RGP) that have
some intersection with S. If extension has no withheld permissions,
then withhold all of its host permissions
- ON_SPECIFIC_SITES: (always for this site): Adds S to the extension's
RGP. Clears the extension's host permissions first before adding S
if the extension has no withheld host permissions
- ON_ALL_SITES: grants all of the extension's withheld host
permissions.
This API method only returns once all the updates have been finished.
Bug: 1378098
Change-Id: I6a4fd6bc4809e24d0ebe577626e566896d8e5a98
---
M chrome/browser/extensions/api/developer_private/developer_private_api.cc
M chrome/browser/extensions/api/developer_private/developer_private_api.h
M chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc
M chrome/browser/extensions/permissions_updater.cc
M chrome/common/extensions/api/developer_private.idl
M extensions/browser/extension_function_histogram_value.h
M third_party/closure_compiler/externs/developer_private.js
M tools/metrics/histograms/enums.xml
8 files changed, 405 insertions(+), 5 deletions(-)
To view, visit change 3943191. To unsubscribe, or for help writing mail filters, visit settings.
Kelvin Jiang (Gerrit)
Attention is currently required from: Kelvin Jiang.
Kelvin Jiang uploaded patch set #10 to this change.
[Extensions] add developerPrivate.updateAccessSettingsForSite
This CL adds a new API method which changes the site access settings
for extensions relative to a single site/url pattern.
The states that can be changed to for a site S are:
- ON_CLICK: Revokes all runtime granted permissions (RGP) that have
some intersection with S. If extension has no withheld permissions,
then withhold all of its host permissions
- ON_SPECIFIC_SITES: (always for this site): Adds S to the extension's
RGP. Clears the extension's host permissions first before adding S
if the extension has no withheld host permissions
- ON_ALL_SITES: removes all of the extension's withheld host
permissions.
This API method only returns once all the updates have been finished.
Bug: 1378098
Change-Id: I6a4fd6bc4809e24d0ebe577626e566896d8e5a98
---
M chrome/browser/extensions/api/developer_private/developer_private_api.cc
M chrome/browser/extensions/api/developer_private/developer_private_api.h
M chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc
M chrome/browser/extensions/permissions_updater.cc
M chrome/common/extensions/api/developer_private.idl
M extensions/browser/extension_function_histogram_value.h
M third_party/closure_compiler/externs/developer_private.js
M tools/metrics/histograms/enums.xml
8 files changed, 405 insertions(+), 5 deletions(-)
To view, visit change 3943191. To unsubscribe, or for help writing mail filters, visit settings.
Kelvin Jiang (Gerrit)
Attention is currently required from: Devlin Cronin.
Patch set 13:Commit-Queue +1
15 comments:
Commit Message:
Patch Set #9, Line 19:
removes
While I could see an argument that "removing a withheld permission" is granting a permission, I thin […]
Done
,
which is accomplished using a barrier closure.nitty nit: this part probably doesn't belong in the CL description
Done
,
which is accomplished using a barrier closure.nitty nit: this part probably doesn't belong in the CL description
Done
File chrome/browser/extensions/api/developer_private/developer_private_api.h:
Patch Set #6, Line 961:
updateAccessSettingsForSite
They all seem equally reasonable to me, so my de facto decision making criterion is "go for the shor […]
done
File chrome/browser/extensions/api/developer_private/developer_private_api.cc:
// Revoke all sites which have some intersection with `site` from the
// extension's set of runtime granted host permissions.
URLPatternSet hosts_to_withhold;
std::unique_ptr<const PermissionSet> runtime_granted_permissions =
ExtensionPrefs::Get(context)->GetRuntimeGrantedPermissions(
extension.id());
for (const URLPattern& pattern :
runtime_granted_permissions->effective_hosts()) {
if (site.OverlapsWith(pattern))
hosts_to_withhold.AddPattern(pattern);
}
std::unique_ptr<const PermissionSet> permissions_to_remove =
PermissionSet::CreateIntersection(
PermissionSet(APIPermissionSet(), ManifestPermissionSet(),
hosts_to_withhold.Clone(), hosts_to_withhold.Clone()),
*modifier->GetRevokablePermissions(),
URLPatternSet::IntersectionBehavior::kDetailed);
DCHECK(!permissions_to_remove->IsEmpty());
PermissionsUpdater(context).RevokeRuntimePermissions(
extension, *permissions_to_remove, done_callback);
as discussed offline: we're populating `hosts_to_withhold` with all runtime granted permissions that […]
(marking as resolved)
File chrome/browser/extensions/api/developer_private/developer_private_api.cc:
Patch Set #9, Line 409:
GrantPermissionsForSite
+1 to anything we can do to reduce the number of permissions-related functions (or entities) we have […]
Ack, created crbug.com/1393266 as a follow up for this
this can be a ref since it's never null
done
Patch Set #9, Line 413:
base::RepeatingClosure done_callback) {OnceClosure?
done
if (!PermissionsManager::Get(context)->HasWithheldHostPermissions(
extension.id())) {
modifier->SetWithholdHostPermissions(true);
modifier->RemoveAllGrantedHostPermissions();
}
This is very non-obvious from the method name and function comment. […]
Thought about this for a bit, having this in the function makes it seem non-single-purpose and it's for a more one-off case, so I've decided to leave it out of the function.
(another motivation for this is that I can't think of a suitable/non-awkward function name that hints at this particular case)
content::BrowserContext* context,
const Extension& extension,
ScriptingPermissionsModifier* modifier,
const URLPattern& site,
base::RepeatingClosure done_callback) {same comments as above
ack (same response as comment for lines 417-421)
Patch Set #9, Line 2605:
return RespondNow(Error(kNoSuchExtensionError));
This results in this function being non-atomic — we might remove permissions for half the extensions […]
Done
Patch Set #9, Line 2609:
return RespondNow(Error(kCannotChangeHostPermissions));ditto with this check
Done
File chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:
std::string updates_arg = "[";
for (const auto& update : updates) {
std::string update_arg = base::StringPrintf(
R"({"id": "%s", "siteAccess": "%s"},)", update.id.c_str(),
api::developer_private::ToString(update.site_access));
updates_arg = base::StrCat({updates_arg, update_arg});
}
// Remove trailing comma from the last entry.
updates_arg.pop_back();
updates_arg = base::StrCat({updates_arg, "]"});
nit: I think slightly cleaner: […]
done
Patch Set #9, Line 2473:
DeveloperPrivateUpdateAccessSettingsForSite_WitheldHostPermissions) {add test comments
Done
Patch Set #9, Line 2512:
kMailGoogleCom
kMailGoogleCom doesn't match — it's https, while the pattern being removed is http. […]
Done
To view, visit change 3943191. To unsubscribe, or for help writing mail filters, visit settings.
Devlin Cronin (Gerrit)
Attention is currently required from: Kelvin Jiang.
Patch set 13:Code-Review +1
3 comments:
Patchset:
LGTM % nits; thanks, Kelvin!
File chrome/browser/extensions/api/developer_private/developer_private_api.cc:
Patch Set #13, Line 2592:
const Extension* extension = GetExtensionById(update.id);We already grabbed all of these once in the loop above.
Let's create a new vector to store them, above line 2580:
std::vector<const Extension*> extensions_to_modify;
extensions_to_modify.reserve(params->updates.size());Patch Set #13, Line 2596:
PermissionsManager::Get(browser_context())let's grab and cache the PermissionsManager in a pointer outside this loop to avoid refetching it each time:
PermissionsManager* const permissions_manager = PermissionsManager::Get(browser_context());
To view, visit change 3943191. To unsubscribe, or for help writing mail filters, visit settings.
Kelvin Jiang (Gerrit)
Patch set 14:Commit-Queue +2
2 comments:
File chrome/browser/extensions/api/developer_private/developer_private_api.cc:
Patch Set #13, Line 2592:
const Extension* extension = GetExtensionById(update.id);
We already grabbed all of these once in the loop above. […]
Done
Patch Set #13, Line 2596:
PermissionsManager::Get(browser_context())
let's grab and cache the PermissionsManager in a pointer outside this loop to avoid refetching it ea […]
Done
To view, visit change 3943191. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
13 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/extensions/api/developer_private/developer_private_api.cc
Insertions: 8, Deletions: 6.
The diff is too large to show. Please review the diff.
```
[Extensions] add developerPrivate.updateAccessSettingsForSite
This CL adds a new API method which changes the site access settings
for extensions relative to a single site/url pattern.
The states that can be changed to for a site S are:
- ON_CLICK: Revokes all runtime granted permissions (RGP) that have
some intersection with S. If extension has no withheld permissions,
then withhold all of its host permissions
- ON_SPECIFIC_SITES: (always for this site): Adds S to the extension's
RGP. Clears the extension's host permissions first before adding S
if the extension has no withheld host permissions
- ON_ALL_SITES: grants all of the extension's withheld host
permissions.
This API method only returns once all the updates have been finished.
Bug: 1378098
Change-Id: I6a4fd6bc4809e24d0ebe577626e566896d8e5a98
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3943191
Reviewed-by: Emilia Paz <emil...@chromium.org>
Reviewed-by: Devlin Cronin <rdevlin...@chromium.org>
Commit-Queue: Kelvin Jiang <kelvi...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1077160}
---
M chrome/browser/extensions/api/developer_private/developer_private_api.cc
M chrome/browser/extensions/api/developer_private/developer_private_api.h
M chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc
M chrome/browser/extensions/permissions_updater.cc
M chrome/common/extensions/api/developer_private.idl
M extensions/browser/extension_function_histogram_value.h
M third_party/closure_compiler/externs/developer_private.js
M tools/metrics/histograms/enums.xml
8 files changed, 435 insertions(+), 9 deletions(-)