Refactor and fix declarative webRequest API permissions

Refactor and fix declarative webRequest API permissions

This CL introduces proper host permission checking for the declarative webRequest API. It moves the permission checking code from web_request_api.cc into a separate class that is shared with the declarative WebRequest API.


BUG=112155
TEST=no
TBR=mirandac@chromium.org,akalin@chromium.org,ben@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148965

[battre, 2012-07-25 18:12:01]

Hi.

Could you please have a look at this CL? The browser tests are still missing (I
tested it manually) but that is a lot of work. I think we can do a first review
already.

Thanks,
Dominic

https://chromiumcodereview.appspot.com/10831008/diff/2001/chrome/browser/exte...
File chrome/browser/extensions/extension_service.cc (right):

https://chromiumcodereview.appspot.com/10831008/diff/2001/chrome/browser/exte...
chrome/browser/extensions/extension_service.cc:1054:
content::Details<UnloadedExtensionInfo>(&details));
I wonder whether this change may have any negative consequences.

[Matt Perry, 2012-07-25 21:27:55]

https://chromiumcodereview.appspot.com/10831008/diff/2001/chrome/browser/exte...
File chrome/browser/extensions/api/declarative/rules_registry_service.cc
(right):

https://chromiumcodereview.appspot.com/10831008/diff/2001/chrome/browser/exte...
chrome/browser/extensions/api/declarative/rules_registry_service.cc:56: void
RulesRegistryService::RegisterDefaultRulesRegistries(bool incognito) {
The incognito parameter is redundant. You can just check
profile_->IsOffTheRecord()

https://chromiumcodereview.appspot.com/10831008/diff/2001/chrome/browser/exte...
File
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc
(right):

https://chromiumcodereview.appspot.com/10831008/diff/2001/chrome/browser/exte...
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc:180:
content::Source<Profile>(profile));
You need to register for the notification for profile->GetOriginalProfile()

https://chromiumcodereview.appspot.com/10831008/diff/2001/chrome/browser/exte...
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc:181:
InitForOTRProfile();
You should also set ready_ = true.

https://chromiumcodereview.appspot.com/10831008/diff/2001/chrome/browser/exte...
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc:189:
void RulesRegistryStorageDelegate::Inner::InitForOTRProfile() {
May as well do this step for both types of profiles. Just check if the
extension_service is loaded, and if so, do this step.

https://chromiumcodereview.appspot.com/10831008/diff/2001/chrome/browser/exte...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_action.h
(right):

https://chromiumcodereview.appspot.com/10831008/diff/2001/chrome/browser/exte...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.h:92:
virtual bool GetEnforceHostPermissions() const;
nit: ShouldEnforceHostPermissions?

https://chromiumcodereview.appspot.com/10831008/diff/2001/chrome/browser/exte...
File chrome/browser/extensions/api/web_request/web_request_api.cc (left):

https://chromiumcodereview.appspot.com/10831008/diff/2001/chrome/browser/exte...
chrome/browser/extensions/api/web_request/web_request_api.cc:1050:
cross_profile_map_[otr_profile] = original_profile;
Why did you move this to the Permissions class? It seems that this file is the
only consumer of the cross_profile_map.

https://chromiumcodereview.appspot.com/10831008/diff/2001/chrome/browser/exte...
File chrome/browser/extensions/api/web_request/web_request_api.cc (right):

https://chromiumcodereview.appspot.com/10831008/diff/2001/chrome/browser/exte...
chrome/browser/extensions/api/web_request/web_request_api.cc:1389: struct
RelevantRegistry {
I think you can define this within the function itself. Alternatively, std::pair
would be appropriate.

https://chromiumcodereview.appspot.com/10831008/diff/2001/chrome/browser/exte...
chrome/browser/extensions/api/web_request/web_request_api.cc:1403: // there are
applicable rules from mode spanning extensions.
"mode spanning" -> "spanning mode" or "profile spanning" ?

https://chromiumcodereview.appspot.com/10831008/diff/2001/chrome/browser/exte...
File chrome/browser/extensions/api/web_request/web_request_permissions.cc
(right):

https://chromiumcodereview.appspot.com/10831008/diff/2001/chrome/browser/exte...
chrome/browser/extensions/api/web_request/web_request_permissions.cc:99: bool
WebRequestPermissions::CanExtensionAccessURL(
This method can be static.

https://chromiumcodereview.appspot.com/10831008/diff/2001/chrome/browser/exte...
File chrome/browser/extensions/api/web_request/web_request_permissions.h
(right):

https://chromiumcodereview.appspot.com/10831008/diff/2001/chrome/browser/exte...
chrome/browser/extensions/api/web_request/web_request_permissions.h:45:
CrossProfileMap cross_profile_map_;
If you move this back to the event router, then this class can just be static
methods, and you won't need to pass an instance around.

https://chromiumcodereview.appspot.com/10831008/diff/2001/chrome/browser/exte...
File chrome/browser/extensions/extension_service.cc (right):

https://chromiumcodereview.appspot.com/10831008/diff/2001/chrome/browser/exte...
chrome/browser/extensions/extension_service.cc:1054:
content::Details<UnloadedExtensionInfo>(&details));
On 2012/07/25 18:12:04, battre wrote:
> I wonder whether this change may have any negative consequences.

It's unconventional. Most of our notifications for extensions are sent only from
the main profile, because the incognito profile shares the extensions. I think
you should remove this and look for another way (I don't actually see where this
notification is observed in this CL).

https://chromiumcodereview.appspot.com/10831008/diff/2001/chrome/browser/exte...
File chrome/browser/extensions/extension_system.cc (right):

https://chromiumcodereview.appspot.com/10831008/diff/2001/chrome/browser/exte...
chrome/browser/extensions/extension_system.cc:289: void
ExtensionSystemImpl::InitForOTRProfile() {
I wonder if this should just be done in the constructor, like the process
manager.

[Yoyo Zhou, 2012-07-25 21:44:45]

http://codereview.chromium.org/10831008/diff/2001/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc
(right):

http://codereview.chromium.org/10831008/diff/2001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc:180:
content::Source<Profile>(profile));
You probably want profile->GetOriginalProfile() here.

http://codereview.chromium.org/10831008/diff/2001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/10831008/diff/2001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1054:
content::Details<UnloadedExtensionInfo>(&details));
On 2012/07/25 18:12:04, battre wrote:
> I wonder whether this change may have any negative consequences.

I don't think this is a good idea. It's better to set up the observer to observe
the source profile->GetOriginalProfile().

[battre, 2012-07-26 16:38:42]

http://codereview.chromium.org/10831008/diff/2001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/declarative/rules_registry_service.cc
(right):

http://codereview.chromium.org/10831008/diff/2001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/rules_registry_service.cc:56: void
RulesRegistryService::RegisterDefaultRulesRegistries(bool incognito) {
On 2012/07/25 21:27:56, Matt Perry wrote:
> The incognito parameter is redundant. You can just check
> profile_->IsOffTheRecord()

Done.

http://codereview.chromium.org/10831008/diff/2001/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc
(right):

http://codereview.chromium.org/10831008/diff/2001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc:180:
content::Source<Profile>(profile));
On 2012/07/25 21:27:56, Matt Perry wrote:
> You need to register for the notification for profile->GetOriginalProfile()

Done.

http://codereview.chromium.org/10831008/diff/2001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc:180:
content::Source<Profile>(profile));
On 2012/07/25 21:44:45, Yoyo Zhou wrote:
> You probably want profile->GetOriginalProfile() here.

Done.

http://codereview.chromium.org/10831008/diff/2001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc:181:
InitForOTRProfile();
On 2012/07/25 21:27:56, Matt Perry wrote:
> You should also set ready_ = true.

I think the better approach is to call CheckIfReady here. Do you agree?

http://codereview.chromium.org/10831008/diff/2001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc:189:
void RulesRegistryStorageDelegate::Inner::InitForOTRProfile() {
On 2012/07/25 21:27:56, Matt Perry wrote:
> May as well do this step for both types of profiles. Just check if the
> extension_service is loaded, and if so, do this step.

This is not quite clear to me. Note the check whether the extension is enabled
in incognito mode in line 196.

http://codereview.chromium.org/10831008/diff/2001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_action.h
(right):

http://codereview.chromium.org/10831008/diff/2001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.h:92:
virtual bool GetEnforceHostPermissions() const;
On 2012/07/25 21:27:56, Matt Perry wrote:
> nit: ShouldEnforceHostPermissions?

Done.

http://codereview.chromium.org/10831008/diff/2001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_request/web_request_api.cc (left):

http://codereview.chromium.org/10831008/diff/2001/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_request/web_request_api.cc:1050:
cross_profile_map_[otr_profile] = original_profile;
On 2012/07/25 21:27:56, Matt Perry wrote:
> Why did you move this to the Permissions class? It seems that this file is the
> only consumer of the cross_profile_map.

Good point. My original approach looked differently.

Done.

http://codereview.chromium.org/10831008/diff/2001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_request/web_request_api.cc (right):

http://codereview.chromium.org/10831008/diff/2001/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_request/web_request_api.cc:1389: struct
RelevantRegistry {
On 2012/07/25 21:27:56, Matt Perry wrote:
> I think you can define this within the function itself. Alternatively,
std::pair
> would be appropriate.

Ok, going for std::pair. I cannot define it in the function due to our compiler
flags.

Done.

http://codereview.chromium.org/10831008/diff/2001/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_request/web_request_api.cc:1403: // there are
applicable rules from mode spanning extensions.
On 2012/07/25 21:27:56, Matt Perry wrote:
> "mode spanning" -> "spanning mode" or "profile spanning" ?

WDYT of my proposal?

http://codereview.chromium.org/10831008/diff/2001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_request/web_request_permissions.cc
(right):

http://codereview.chromium.org/10831008/diff/2001/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_request/web_request_permissions.cc:99: bool
WebRequestPermissions::CanExtensionAccessURL(
On 2012/07/25 21:27:56, Matt Perry wrote:
> This method can be static.

Done.

http://codereview.chromium.org/10831008/diff/2001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_request/web_request_permissions.h
(right):

http://codereview.chromium.org/10831008/diff/2001/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_request/web_request_permissions.h:45:
CrossProfileMap cross_profile_map_;
On 2012/07/25 21:27:56, Matt Perry wrote:
> If you move this back to the event router, then this class can just be static
> methods, and you won't need to pass an instance around.

Done.

I have left this class rather than moving everything back into the helpers
namespace and into web_request_api_helpers.cc because I think that the
separation of permissions is quite good for readability.
web_request_api_helpers.cc is pretty large already.

http://codereview.chromium.org/10831008/diff/2001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/10831008/diff/2001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1054:
content::Details<UnloadedExtensionInfo>(&details));
On 2012/07/25 21:44:45, Yoyo Zhou wrote:
> On 2012/07/25 18:12:04, battre wrote:
> > I wonder whether this change may have any negative consequences.
> 
> I don't think this is a good idea. It's better to set up the observer to
observe
> the source profile->GetOriginalProfile().

Done.

http://codereview.chromium.org/10831008/diff/2001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_system.cc (right):

http://codereview.chromium.org/10831008/diff/2001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_system.cc:289: void
ExtensionSystemImpl::InitForOTRProfile() {
On 2012/07/25 21:27:56, Matt Perry wrote:
> I wonder if this should just be done in the constructor, like the process
> manager.

No, RegisterDefaultRulesRegistries calls extensions::StateStore* store =
ExtensionSystem::Get(profile)->state_store(); transitively and we end up in an
infinite recursion.

[battre, 2012-07-26 16:53:09]

I added some browser tests as well.

[Matt Perry, 2012-07-26 18:42:40]

LGTM

http://codereview.chromium.org/10831008/diff/2001/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc
(right):

http://codereview.chromium.org/10831008/diff/2001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc:181:
InitForOTRProfile();
On 2012/07/26 16:38:43, battre wrote:
> On 2012/07/25 21:27:56, Matt Perry wrote:
> > You should also set ready_ = true.
> 
> I think the better approach is to call CheckIfReady here. Do you agree?

No, then it will be considered "not ready" for some amount of time. The
incognito version starts ready, so it doesn't actually need to notify the rules
registry.

http://codereview.chromium.org/10831008/diff/2001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc:189:
void RulesRegistryStorageDelegate::Inner::InitForOTRProfile() {
On 2012/07/26 16:38:43, battre wrote:
> On 2012/07/25 21:27:56, Matt Perry wrote:
> > May as well do this step for both types of profiles. Just check if the
> > extension_service is loaded, and if so, do this step.
> 
> This is not quite clear to me. Note the check whether the extension is enabled
> in incognito mode in line 196.

OK, I take that back then.

http://codereview.chromium.org/10831008/diff/2001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_request/web_request_permissions.h
(right):

http://codereview.chromium.org/10831008/diff/2001/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_request/web_request_permissions.h:45:
CrossProfileMap cross_profile_map_;
On 2012/07/26 16:38:43, battre wrote:
> On 2012/07/25 21:27:56, Matt Perry wrote:
> > If you move this back to the event router, then this class can just be
static
> > methods, and you won't need to pass an instance around.
> 
> Done.
> 
> I have left this class rather than moving everything back into the helpers
> namespace and into web_request_api_helpers.cc because I think that the
> separation of permissions is quite good for readability.
> web_request_api_helpers.cc is pretty large already.

Agreed - I only meant to move cross_profile_map_. This class looks good.

[battre, 2012-07-30 13:39:39]

I am going to TBR some stuff:
- mirandac for chrome/browser/profiles (renamed a function, trigger
initialization of parts of extension system for OTR profile)
- akalin for chrome/browser/sync (renamed a function)
- ben for *.gypi changes

[commit-bot: I haz the power, 2012-07-30 13:39:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/10831008/9016

[commit-bot: I haz the power, 2012-07-30 13:40:12]

Presubmit check for 10831008-9016 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
--tbr was specified, skipping OWNERS check

** Presubmit Warnings **
License must match:
.*? Copyright \(c\) 2012 The Chromium Authors\. All rights reserved\.\n.*? Use
of this source code is governed by a BSD-style license that can be\n.*? found in
the LICENSE file\.(?: \*/)?\n
Found a bad license header in these files:
 
chrome/test/data/extensions/api_test/webrequest_permissions/spanning/background.js
\
 
chrome/test/data/extensions/api_test/webrequest_permissions/split/background.js

Presubmit checks took 1.9s to calculate.

[commit-bot: I haz the power, 2012-07-30 13:50:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/10831008/12024

[commit-bot: I haz the power, 2012-07-30 17:21:44]

Change committed as 148965