chrome/browser/extensions/active_tab_permission_granter.cc - Issue 348313003: Create withheld permissions

Unified Diff: chrome/browser/extensions/active_tab_permission_granter.cc

Issue 348313003: Create withheld permissions (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src

Patch Set: Test fix Created 6 years, 6 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

« chrome/browser/extensions/active_script_controller.cc ('K') | « chrome/browser/extensions/active_script_controller_unittest.cc ('k') | chrome/browser/extensions/extension_util.cc » ('j') | chrome/browser/extensions/extension_util.cc » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: chrome/browser/extensions/active_tab_permission_granter.cc

diff --git a/chrome/browser/extensions/active_tab_permission_granter.cc b/chrome/browser/extensions/active_tab_permission_granter.cc

index c2d0cd5edd5e9532c41a6d68d8eca6f982374f8c..258ee97c2a75a42d9caf015635e638cd0d06a00d 100644

--- a/chrome/browser/extensions/active_tab_permission_granter.cc

+++ b/chrome/browser/extensions/active_tab_permission_granter.cc

@@ -45,19 +45,11 @@ void ActiveTabPermissionGranter::GrantIfRequested(const Extension* extension) {

const PermissionsData* permissions_data = extension->permissions_data();

- // If the extension requires action for script execution, we grant it

+ // If the extension requested all-hosts but has had it withheld, we grant it

// active tab-style permissions, even if it doesn't have the activeTab

// permission in the manifest.

- // We don't take tab id into account, because we want to know if the extension

- // should require active tab in general (not for the current tab).

- bool requires_action_for_script_execution =

- permissions_data->RequiresActionForScriptExecution(extension,

- -1, // No tab id.

- GURL());

- if (extension->permissions_data()->HasAPIPermission(

- APIPermission::kActiveTab) ||

- requires_action_for_script_execution) {

+ if (permissions_data->HasAPIPermission(APIPermission::kActiveTab) ||

+ permissions_data->HasWithheldAllHosts()) {

not at google - send to devlin 2014/06/30 14:37:03 could this second check be "permissions_data->CanA

Devlin 2014/06/30 17:06:09 Not really. Unless we overload GrantIfRequested t

not at google - send to devlin 2014/07/01 00:28:35 What's confusing about that? we already grab the U

Devlin 2014/07/01 16:27:05 I can see your point with all hosts vs web request

I can see your point with all hosts vs web request - they're very similar. But if we expand this world to include things like withholding specific hosts or specific permissions that don't pertain to all hosts, "have withheld anything" won't work. Instead, I would think that we would bundle withheld permissions that pertain to all hosts in the check for HasWithheldAllHosts(). That is, if we withhold web request, we withheld all hosts (which we essentially did). That way, the logic for the checkbox (and this) is still correct. And I'm still worried that using the URL will break in very odd edge cases. For instance, - Extension a has permission to foo.com and all urls - We withhold all urls, but still allow it foo.com - Extension a injects a script on all frames in a foo.com page with an iframe to bar.com. - The renderer requests permission to inject into the bar.com frame (because the permission is withheld). - We get here, and check the url and realize it already has permission for foo.com (top level url), so we don't grant it. Now, admittedly, this is a hell of an edge case, and the worst that will happen is that people will see multiple requests (because the extension wasn't given active tab). But that could potentially be annoying.

not at google - send to devlin 2014/07/01 17:02:11 It's a legitimate edge case, but it's a feature th

On 2014/07/01 16:27:05, Devlin wrote: > On 2014/07/01 00:28:35, kalman wrote: > > On 2014/06/30 17:06:09, Devlin wrote: > > > On 2014/06/30 14:37:03, kalman wrote: > > > > could this second check be "permissions_data->CanAccessPage(url) == > > WITHHELD" > > > > rather than exposing HasWithheldAllHosts? > > > > > > Not really. Unless we overload GrantIfRequested to take an optional enum > for > > > type of permission check (though at that point it really should just be > > renaming > > > it to "GrantEvenIfNotRequested"). > > > > > > Plus, we would need to get the url. Which could be tricky for document vs. > > top. > > > > > > > What's confusing about that? we already grab the URL on line 71. > > > > > > > > Plus, we use HasWithheldAllHosts() in ExtensionSettingsHandler (to expose > the > > > "all urls" checkbox). > > > > > > I think HasWithheldAllHosts is fine - as I mention in PermissionsData, > > AllHosts > > > is the only "weird" permission that needs its own methods. For future > > withheld > > > permissions, we can just check withheld_permissions()->Contains(x). > > > > the distinction between withholding anything and withholding all-hosts is > > confusing. say we withhold webRequest and banking sites, and an extension has > > requested webRequest and http://bankofameria.com. we should still show the > checkbox, > > however, HasWithheldAllHosts will be false. so we'll need 2 checks. > > > > what I'm saying is that the logic for the checkbox should be "have withheld > > anything". > > I can see your point with all hosts vs web request - they're very similar. But > if we expand this world to include things like withholding specific hosts or > specific permissions that don't pertain to all hosts, "have withheld anything" > won't work. Instead, I would think that we would bundle withheld permissions > that pertain to all hosts in the check for HasWithheldAllHosts(). That is, if > we withhold web request, we withheld all hosts (which we essentially did). That > way, the logic for the checkbox (and this) is still correct. > > And I'm still worried that using the URL will break in very odd edge cases. > For instance, > - Extension a has permission to http://foo.com and all urls > - We withhold all urls, but still allow it http://foo.com > - Extension a injects a script on all frames in a http://foo.com page with an iframe to > http://bar.com. > - The renderer requests permission to inject into the http://bar.com frame (because the > permission is withheld). > - We get here, and check the url and realize it already has permission for > http://foo.com (top level url), so we don't grant it. > > Now, admittedly, this is a hell of an edge case, and the worst that will happen > is that people will see multiple requests (because the extension wasn't given > active tab). But that could potentially be annoying.

It's a legitimate edge case, but it's a feature that we don't grant activeTab to frames, that behaviour is correct. in the interest of submitting this CL let's go with what we have, since it's most similar to RequiresActionForScriptExecution. but I'd like to call that out for the future. one nit in that case: it should probably be HasWithheldEffectiveAllHosts or similar, since we also withhold *.com.

Devlin 2014/07/01 18:34:08 Done.

On 2014/07/01 17:02:11, kalman wrote: > On 2014/07/01 16:27:05, Devlin wrote: > > On 2014/07/01 00:28:35, kalman wrote: > > > On 2014/06/30 17:06:09, Devlin wrote: > > > > On 2014/06/30 14:37:03, kalman wrote: > > > > > could this second check be "permissions_data->CanAccessPage(url) == > > > WITHHELD" > > > > > rather than exposing HasWithheldAllHosts? > > > > > > > > Not really. Unless we overload GrantIfRequested to take an optional enum > > for > > > > type of permission check (though at that point it really should just be > > > renaming > > > > it to "GrantEvenIfNotRequested"). > > > > > > > > Plus, we would need to get the url. Which could be tricky for document > vs. > > > top. > > > > > > > > > > What's confusing about that? we already grab the URL on line 71. > > > > > > > > > > > Plus, we use HasWithheldAllHosts() in ExtensionSettingsHandler (to expose > > the > > > > "all urls" checkbox). > > > > > > > > I think HasWithheldAllHosts is fine - as I mention in PermissionsData, > > > AllHosts > > > > is the only "weird" permission that needs its own methods. For future > > > withheld > > > > permissions, we can just check withheld_permissions()->Contains(x). > > > > > > the distinction between withholding anything and withholding all-hosts is > > > confusing. say we withhold webRequest and banking sites, and an extension > has > > > requested webRequest and http://bankofameria.com. we should still show the > > checkbox, > > > however, HasWithheldAllHosts will be false. so we'll need 2 checks. > > > > > > what I'm saying is that the logic for the checkbox should be "have withheld > > > anything". > > > > I can see your point with all hosts vs web request - they're very similar. > But > > if we expand this world to include things like withholding specific hosts or > > specific permissions that don't pertain to all hosts, "have withheld anything" > > won't work. Instead, I would think that we would bundle withheld permissions > > that pertain to all hosts in the check for HasWithheldAllHosts(). That is, if > > we withhold web request, we withheld all hosts (which we essentially did). > That > > way, the logic for the checkbox (and this) is still correct. > > > > And I'm still worried that using the URL will break in very odd edge cases. > > For instance, > > - Extension a has permission to http://foo.com and all urls > > - We withhold all urls, but still allow it http://foo.com > > - Extension a injects a script on all frames in a http://foo.com page with an > iframe to > > http://bar.com. > > - The renderer requests permission to inject into the http://bar.com frame > (because the > > permission is withheld). > > - We get here, and check the url and realize it already has permission for > > http://foo.com (top level url), so we don't grant it. > > > > Now, admittedly, this is a hell of an edge case, and the worst that will > happen > > is that people will see multiple requests (because the extension wasn't given > > active tab). But that could potentially be annoying. > > It's a legitimate edge case, but it's a feature that we don't grant activeTab to > frames, that behaviour is correct. > > in the interest of submitting this CL let's go with what we have, since it's > most similar to RequiresActionForScriptExecution. but I'd like to call that out > for the future. > > one nit in that case: it should probably be HasWithheldEffectiveAllHosts or > similar, since we also withhold *.com.

Done.

URLPattern pattern(UserScript::ValidUserScriptSchemes());

// Pattern parsing could fail if this is an unsupported URL e.g. chrome://.

if (pattern.Parse(web_contents()->GetURL().spec()) ==

@@ -67,8 +59,7 @@ void ActiveTabPermissionGranter::GrantIfRequested(const Extension* extension) {

new_apis.insert(APIPermission::kTab);

}

- if (extension->permissions_data()->HasAPIPermission(

- APIPermission::kTabCapture))

+ if (permissions_data->HasAPIPermission(APIPermission::kTabCapture))

new_apis.insert(APIPermission::kTabCaptureForTab);

if (!new_apis.empty() || !new_hosts.is_empty()) {