Add policy controlled permission block list for extensions

Add policy controlled permission block list for extensions

This CL adds permissions block list for extensions. Currently only simple API permissions are supported, and the block list applies to both required and optional permissions of extensions.

BUG=177351
Committed: https://crrev.com/e6b58b5a41f010118c5caea9ba78bc077a5f551b
Cr-Commit-Position: refs/heads/master@{#302211}

[binjin, 2014-09-24 20:24:08]

Patchset #3 (id:30001) has been deleted

[binjin, 2014-10-14 17:20:41]

binjin@chromium.org changed reviewers:
+ joaodasilva@chromium.org

[binjin, 2014-10-14 17:22:07]

Hello Joao,

It's still a WIP, lack of a couple of browser tests. But I think it's now okay
for an initial code reviewing.

Bin

[Joao da Silva, 2014-10-15 14:39:27]

Nice test coverage in extension_service_unittest.cc.

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management.cc (right):

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:148: for (auto blocked_api :
GetBlockedAPIPermissions(id)) {
const auto&

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management.h (right):

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.h:110: // Returns true if every
permssion in |perms| are allowed for extension |id|.
permission

is allowed

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_internal.cc (right):

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_internal.cc:71: const
base::ListValue* list_value;
= NULL

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_internal.cc:79: APIPermissionSet
globally_blocked_permissions = blocked_permissions;
This is extremely subtle.

If I understood it correctly: ExtensionManagement::Refresh() parses the
default_settings_ first, and then individual settings start as a copy of the
default, right? So we if make that copy and then parse the settings for an
extension, the current |blocked_permissions| is the global configuration.

It would be much better if this was made very clear in the code. As it is,
please leave a comment here explaining why |blocked_permissions| is expected to
have the global settings at this step.

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_internal.cc:100: APIPermissionSet
permissions_to_merge_from = blocked_permissions;
Same here.

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_test_util.cc (right):

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_test_util.cc:120: const
std::string& allowd_type) {
allowed_type

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_unittest.cc (right):

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_unittest.cc:375:
extension_management_->GetBlockedAPIPermissions(kTargetExtension3));
Add tests with parsing errors in the permissions policy.

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1716: for (auto extension :
*all_extensions.get()) {
const auto&

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1720:
extensions::PermissionsUpdater(profile()).RemovePermissions(
What happens in this case? If an extension is already running, is it disabled?

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service_unittest.cc (right):

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_unittest.cc:780: void
GrantAllOptionalPermissions(std::string id) {
const std::string&

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_unittest.cc:3889: // The extension
should failed to install.
Should succeed?

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_unittest.cc:3905: PackCRX(path,
pem_path, crx_path);
Assign this to a "const Extension* extension" and then:

EXPECT_EQ(permissions_blocklist, extension->id())

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_unittest.cc:4005: PackCRX(path2,
pem_path, crx_path);
Check the ID of the returned Extension*

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
File chrome/browser/extensions/permissions_based_management_policy_provider.cc
(right):

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/permissions_based_management_policy_provider.cc:55:
return false;
What happens if a force-installed extension uses a permission that is globally
blocked? Does it still get installed or not?

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
File chrome/browser/extensions/permissions_based_management_policy_provider.h
(right):

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/permissions_based_management_policy_provider.h:16:
class PermissionsBasedManagementPolicyProvider
Document this class

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/permissions_based_management_policy_provider.h:24:
virtual std::string GetDebugPolicyProviderName() const OVERRIDE;
override (also below)

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
File
chrome/browser/extensions/permissions_based_management_policy_provider_unittest.cc
(right):

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/permissions_based_management_policy_provider_unittest.cc:46:
for (auto perm : perm_list_) {
const auto&

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/permissions_based_management_policy_provider_unittest.cc:50:
NOTREACHED();
ADD_FAILURE() << "Permission not found: " << id;

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/permissions_based_management_policy_provider_unittest.cc:87:
// Verifies that extensions with conflicting permissiosn cannot be loaded.
permissions

[binjin, 2014-10-16 18:13:58]

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management.cc (right):

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.cc:148: for (auto blocked_api :
GetBlockedAPIPermissions(id)) {
On 2014/10/15 14:39:25, Joao da Silva wrote:
> const auto&

Done.

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management.h (right):

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.h:110: // Returns true if every
permssion in |perms| are allowed for extension |id|.
On 2014/10/15 14:39:25, Joao da Silva wrote:
> permission
> 
> is allowed

Done.

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_internal.cc (right):

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_internal.cc:71: const
base::ListValue* list_value;
On 2014/10/15 14:39:25, Joao da Silva wrote:
> = NULL

Done.

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_internal.cc:79: APIPermissionSet
globally_blocked_permissions = blocked_permissions;
On 2014/10/15 14:39:25, Joao da Silva wrote:
> This is extremely subtle.
> 
> If I understood it correctly: ExtensionManagement::Refresh() parses the
> default_settings_ first, and then individual settings start as a copy of the
> default, right? So we if make that copy and then parse the settings for an
> extension, the current |blocked_permissions| is the global configuration.
> 
> It would be much better if this was made very clear in the code. As it is,
> please leave a comment here explaining why |blocked_permissions| is expected
to
> have the global settings at this step.

Done.

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_internal.cc:100: APIPermissionSet
permissions_to_merge_from = blocked_permissions;
On 2014/10/15 14:39:25, Joao da Silva wrote:
> Same here.

Done. But note that it's slightly different: |block_permissions| here might be
the result of the routines above.

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_test_util.cc (right):

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_test_util.cc:120: const
std::string& allowd_type) {
On 2014/10/15 14:39:25, Joao da Silva wrote:
> allowed_type

Done.

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_unittest.cc (right):

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_unittest.cc:375:
extension_management_->GetBlockedAPIPermissions(kTargetExtension3));
On 2014/10/15 14:39:26, Joao da Silva wrote:
> Add tests with parsing errors in the permissions policy.

I think this tests along with unit tests for
PermissionBasedManagementPolicyProvider covers the code well. The actual parsing
re-used code that is well tested as well.

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1716: for (auto extension :
*all_extensions.get()) {
On 2014/10/15 14:39:26, Joao da Silva wrote:
> const auto&

Done.

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1720:
extensions::PermissionsUpdater(profile()).RemovePermissions(
On 2014/10/15 14:39:26, Joao da Silva wrote:
> What happens in this case? If an extension is already running, is it disabled?

CheckManagementPolicy() will be called immediately below, and will unload all
extensions with conflicting required permissions.

I'm not sure about optional permissions, maybe we need to disable them manually,
a browser test might be necessary for this.

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service_unittest.cc (right):

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_unittest.cc:780: void
GrantAllOptionalPermissions(std::string id) {
On 2014/10/15 14:39:26, Joao da Silva wrote:
> const std::string&

Done.

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_unittest.cc:3889: // The extension
should failed to install.
On 2014/10/15 14:39:26, Joao da Silva wrote:
> Should succeed?

Done.

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_unittest.cc:3905: PackCRX(path,
pem_path, crx_path);
On 2014/10/15 14:39:26, Joao da Silva wrote:
> Assign this to a "const Extension* extension" and then:
> 
> EXPECT_EQ(permissions_blocklist, extension->id())

PackCRX() only create an crx_file and returns nothing. We need to extract the
crx file later to obtain the extension id, but with the force installing
mechanism here, it's not easy.

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_unittest.cc:4005: PackCRX(path2,
pem_path, crx_path);
On 2014/10/15 14:39:26, Joao da Silva wrote:
> Check the ID of the returned Extension*

Same as above.

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
File chrome/browser/extensions/permissions_based_management_policy_provider.cc
(right):

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/permissions_based_management_policy_provider.cc:55:
return false;
On 2014/10/15 14:39:26, Joao da Silva wrote:
> What happens if a force-installed extension uses a permission that is globally
> blocked? Does it still get installed or not?

It shouldn't be installed. Force installed extensions are installed from
ExternalProvider with policy location, but they're the same as other external
installed (except location) afterward. They will be checked against management
policy as well.

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
File chrome/browser/extensions/permissions_based_management_policy_provider.h
(right):

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/permissions_based_management_policy_provider.h:16:
class PermissionsBasedManagementPolicyProvider
On 2014/10/15 14:39:26, Joao da Silva wrote:
> Document this class

Done.

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/permissions_based_management_policy_provider.h:24:
virtual std::string GetDebugPolicyProviderName() const OVERRIDE;
On 2014/10/15 14:39:26, Joao da Silva wrote:
> override (also below)

Done.

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
File
chrome/browser/extensions/permissions_based_management_policy_provider_unittest.cc
(right):

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/permissions_based_management_policy_provider_unittest.cc:46:
for (auto perm : perm_list_) {
On 2014/10/15 14:39:26, Joao da Silva wrote:
> const auto&

Done.

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/permissions_based_management_policy_provider_unittest.cc:50:
NOTREACHED();
On 2014/10/15 14:39:26, Joao da Silva wrote:
> ADD_FAILURE() << "Permission not found: " << id;

Done.

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/permissions_based_management_policy_provider_unittest.cc:87:
// Verifies that extensions with conflicting permissiosn cannot be loaded.
On 2014/10/15 14:39:26, Joao da Silva wrote:
> permissions

Done.

[Joao da Silva, 2014-10-17 12:52:39]

Code lgtm, feel free to ping other reviewers.

I still think it's worth having a test for a policy with an invalid permissions
configuration, see inline.

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_unittest.cc (right):

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_unittest.cc:375:
extension_management_->GetBlockedAPIPermissions(kTargetExtension3));
On 2014/10/16 18:13:58, bjin wrote:
> On 2014/10/15 14:39:26, Joao da Silva wrote:
> > Add tests with parsing errors in the permissions policy.
> 
> I think this tests along with unit tests for
> PermissionBasedManagementPolicyProvider covers the code well. The actual
parsing
> re-used code that is well tested as well.

So what happens if the policy has an invalid permission that fails parsing? Do
we just ignore that permission? Or do we block all extensions by accident? I
really think having coverage for this is a good idea. This is easier to add in
extension_service_unittest.cc.

https://codereview.chromium.org/595363002/diff/650001/chrome/browser/extensio...
File chrome/browser/extensions/permissions_based_management_policy_provider.h
(right):

https://codereview.chromium.org/595363002/diff/650001/chrome/browser/extensio...
chrome/browser/extensions/permissions_based_management_policy_provider.h:17: //
certain extensions from loading by examing if its permission data conflits
by checking (or, by examining)

conficts

[binjin, 2014-10-17 13:03:19]

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_unittest.cc (right):

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_unittest.cc:375:
extension_management_->GetBlockedAPIPermissions(kTargetExtension3));
On 2014/10/17 12:52:39, Joao da Silva wrote:
> On 2014/10/16 18:13:58, bjin wrote:
> > On 2014/10/15 14:39:26, Joao da Silva wrote:
> > > Add tests with parsing errors in the permissions policy.
> > 
> > I think this tests along with unit tests for
> > PermissionBasedManagementPolicyProvider covers the code well. The actual
> parsing
> > re-used code that is well tested as well.
> 
> So what happens if the policy has an invalid permission that fails parsing? Do
> we just ignore that permission? Or do we block all extensions by accident? I
> really think having coverage for this is a good idea. This is easier to add in
> extension_service_unittest.cc.

To clarify, the permission parser actually allows unknown permission for
backward compatibility purpose. Since these invalid permissions(might be
introduced in a newer version of chrome) are currently unknown to the chrome
version that user are using, it's safe to just ignore them. I will add few
comments regarding this and include a few invalid permissions in example
preference.

[binjin, 2014-10-17 14:04:23]

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_unittest.cc (right):

https://codereview.chromium.org/595363002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_unittest.cc:375:
extension_management_->GetBlockedAPIPermissions(kTargetExtension3));
On 2014/10/17 12:52:39, Joao da Silva wrote:
> On 2014/10/16 18:13:58, bjin wrote:
> > On 2014/10/15 14:39:26, Joao da Silva wrote:
> > > Add tests with parsing errors in the permissions policy.
> > 
> > I think this tests along with unit tests for
> > PermissionBasedManagementPolicyProvider covers the code well. The actual
> parsing
> > re-used code that is well tested as well.
> 
> So what happens if the policy has an invalid permission that fails parsing? Do
> we just ignore that permission? Or do we block all extensions by accident? I
> really think having coverage for this is a good idea. This is easier to add in
> extension_service_unittest.cc.

Done.

https://codereview.chromium.org/595363002/diff/650001/chrome/browser/extensio...
File chrome/browser/extensions/permissions_based_management_policy_provider.h
(right):

https://codereview.chromium.org/595363002/diff/650001/chrome/browser/extensio...
chrome/browser/extensions/permissions_based_management_policy_provider.h:17: //
certain extensions from loading by examing if its permission data conflits
On 2014/10/17 12:52:39, Joao da Silva wrote:
> by checking (or, by examining)
> 
> conficts

Done.

[binjin, 2014-10-28 22:45:11]

Patchset #18 (id:710001) has been deleted

[binjin, 2014-10-29 13:46:40]

Patchset #18 (id:730001) has been deleted

[binjin, 2014-10-29 14:12:20]

Hello Joao,

Could you have a look at the new extension api test for blocked optional
permissions? Note that patchset 18 is a rebase commit without my changes.

Thanks

[Joao da Silva, 2014-10-30 10:22:13]

lgtm

Thanks for writing the test. It's really great to have that coverage.

[binjin, 2014-10-30 10:24:24]

binjin@chromium.org changed reviewers:
+ finnur@chromium.org

[binjin, 2014-10-30 10:24:25]

Finnur, could you have a look at this CL?

[Finnur, 2014-10-30 14:16:59]

Didn't do an exhaustive review, more of spot checks here and there. 

So OWNERS LGTM with a few nits.

https://codereview.chromium.org/595363002/diff/770001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management.h (right):

https://codereview.chromium.org/595363002/diff/770001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.h:73: // KeyedService
implementations.
nit: The agreed upon syntax for this was:
// InterfaceName:

https://codereview.chromium.org/595363002/diff/770001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.h:74: virtual void ShutDown();
delete 'virtual' and add 'override' at the end.

https://codereview.chromium.org/595363002/diff/770001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_internal.cc (right):

https://codereview.chromium.org/595363002/diff/770001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_internal.cc:93: NULL)) {
nit: nullptr
Same on line 114.

https://codereview.chromium.org/595363002/diff/770001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_internal.h (right):

https://codereview.chromium.org/595363002/diff/770001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_internal.h:61: // Permissions
settings for extensions. These settings won't give granted
nit: s/give granted/grant/ ?

https://codereview.chromium.org/595363002/diff/770001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_internal.h:64: // an extension
requires a permission which is on the blocklist for it,
s/is on the blocklist for it/has been blacklisted/

https://codereview.chromium.org/595363002/diff/770001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_internal.h:65: // this extension
will not be allowed to be loaded. And if it contains a
s/be loaded/load/

https://codereview.chromium.org/595363002/diff/770001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_internal.h:67: // be loaded (of
course, with permission granted from other part of
s/be loaded/load/

"with permission granted from other part of extension system"
What does that mean?

https://codereview.chromium.org/595363002/diff/770001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_internal.h:68: // extension
system), but conflicting permissions will not be usable.
nit: s/not be usable/be dropped/ ?

https://codereview.chromium.org/595363002/diff/770001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_test_util.h (right):

https://codereview.chromium.org/595363002/diff/770001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_test_util.h:133: virtual
~ExtensionManagementPolicyUpdater();
virtual -> override

https://codereview.chromium.org/595363002/diff/770001/chrome/browser/extensio...
File chrome/browser/extensions/permissions_based_management_policy_provider.h
(right):

https://codereview.chromium.org/595363002/diff/770001/chrome/browser/extensio...
chrome/browser/extensions/permissions_based_management_policy_provider.h:28:
virtual bool UserMayLoad(const Extension* extension,
virtual -> override (three cases)

https://codereview.chromium.org/595363002/diff/770001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/permissions/optional_policy_blocked/background.js
(right):

https://codereview.chromium.org/595363002/diff/770001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/permissions/optional_policy_blocked/background.js:1:
// Copyright (c) 2014 The Chromium Authors. All rights reserved.
Nit: Delete the (c)

https://codereview.chromium.org/595363002/diff/770001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/595363002/diff/770001/components/policy/resou...
components/policy/resources/policy_templates.json:1826: "blocked_permissions":
maps to a list of strings indicating the blocked API permissions for the
extension. The permissions names are same as the permission strings declared in
manifest of extension as described at <ph
name="LINK_TO_EXTENSION_DOC3">https://developer.chrome.com/extensions/declare_permissions</ph>.
This setting also can be configured for "*" extension. If the extension requires
a permission which is on the blocklist, it will not be allowed to be loaded. If
it contains a blocked permission as optional requirement, it will be handled in
normal way, but requests of conflicting permissions will be declined
automatically at runtime.
s/to be/to load/
s/normal way/the normal way/
s/requests of/requesting/

[binjin, 2014-10-30 16:41:12]

https://codereview.chromium.org/595363002/diff/770001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management.h (right):

https://codereview.chromium.org/595363002/diff/770001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.h:73: // KeyedService
implementations.
On 2014/10/30 14:16:59, Finnur wrote:
> nit: The agreed upon syntax for this was:
> // InterfaceName:

Done.

https://codereview.chromium.org/595363002/diff/770001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.h:74: virtual void ShutDown();
On 2014/10/30 14:16:59, Finnur wrote:
> delete 'virtual' and add 'override' at the end.

Done. Also found that I mis-spelled the method name.

https://codereview.chromium.org/595363002/diff/770001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_internal.cc (right):

https://codereview.chromium.org/595363002/diff/770001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_internal.cc:93: NULL)) {
On 2014/10/30 14:16:59, Finnur wrote:
> nit: nullptr
> Same on line 114.

Done.

https://codereview.chromium.org/595363002/diff/770001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_internal.h (right):

https://codereview.chromium.org/595363002/diff/770001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_internal.h:61: // Permissions
settings for extensions. These settings won't give granted
On 2014/10/30 14:16:59, Finnur wrote:
> nit: s/give granted/grant/ ?

Done.

https://codereview.chromium.org/595363002/diff/770001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_internal.h:64: // an extension
requires a permission which is on the blocklist for it,
On 2014/10/30 14:16:59, Finnur wrote:
> s/is on the blocklist for it/has been blacklisted/

Done.

https://codereview.chromium.org/595363002/diff/770001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_internal.h:65: // this extension
will not be allowed to be loaded. And if it contains a
On 2014/10/30 14:16:59, Finnur wrote:
> s/be loaded/load/

Done.

https://codereview.chromium.org/595363002/diff/770001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_internal.h:67: // be loaded (of
course, with permission granted from other part of
On 2014/10/30 14:16:59, Finnur wrote:
> s/be loaded/load/
> 
> "with permission granted from other part of extension system"
> What does that mean?

Done. I mean "granted by user if necessary", updated.

https://codereview.chromium.org/595363002/diff/770001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_internal.h:68: // extension
system), but conflicting permissions will not be usable.
On 2014/10/30 14:16:59, Finnur wrote:
> nit: s/not be usable/be dropped/ ?

Done.

https://codereview.chromium.org/595363002/diff/770001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management_test_util.h (right):

https://codereview.chromium.org/595363002/diff/770001/chrome/browser/extensio...
chrome/browser/extensions/extension_management_test_util.h:133: virtual
~ExtensionManagementPolicyUpdater();
On 2014/10/30 14:16:59, Finnur wrote:
> virtual -> override

Done.

https://codereview.chromium.org/595363002/diff/770001/chrome/browser/extensio...
File chrome/browser/extensions/permissions_based_management_policy_provider.h
(right):

https://codereview.chromium.org/595363002/diff/770001/chrome/browser/extensio...
chrome/browser/extensions/permissions_based_management_policy_provider.h:28:
virtual bool UserMayLoad(const Extension* extension,
On 2014/10/30 14:16:59, Finnur wrote:
> virtual -> override (three cases)

Done.

https://codereview.chromium.org/595363002/diff/770001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/permissions/optional_policy_blocked/background.js
(right):

https://codereview.chromium.org/595363002/diff/770001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/permissions/optional_policy_blocked/background.js:1:
// Copyright (c) 2014 The Chromium Authors. All rights reserved.
On 2014/10/30 14:16:59, Finnur wrote:
> Nit: Delete the (c)

Done.

https://codereview.chromium.org/595363002/diff/770001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/595363002/diff/770001/components/policy/resou...
components/policy/resources/policy_templates.json:1826: "blocked_permissions":
maps to a list of strings indicating the blocked API permissions for the
extension. The permissions names are same as the permission strings declared in
manifest of extension as described at <ph
name="LINK_TO_EXTENSION_DOC3">https://developer.chrome.com/extensions/declare_permissions</ph>.
This setting also can be configured for "*" extension. If the extension requires
a permission which is on the blocklist, it will not be allowed to be loaded. If
it contains a blocked permission as optional requirement, it will be handled in
normal way, but requests of conflicting permissions will be declined
automatically at runtime.
On 2014/10/30 14:16:59, Finnur wrote:
> s/to be/to load/
> s/normal way/the normal way/
> s/requests of/requesting/

Done.

[binjin, 2014-10-30 17:10:56]

The CQ bit was checked by binjin@chromium.org

[commit-bot: I haz the power, 2014-10-30 17:12:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/595363002/810001

[binjin, 2014-10-30 20:59:39]

The CQ bit was unchecked by binjin@chromium.org

[binjin, 2014-10-31 00:59:00]

The CQ bit was checked by binjin@chromium.org

[commit-bot: I haz the power, 2014-10-31 01:00:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/595363002/830001

[commit-bot: I haz the power, 2014-10-31 01:56:06]

Committed patchset #22 (id:830001)

[commit-bot: I haz the power, 2014-10-31 01:56:49]

Patchset 22 (id:??) landed as
https://crrev.com/e6b58b5a41f010118c5caea9ba78bc077a5f551b
Cr-Commit-Position: refs/heads/master@{#302211}