Refactor the various ways to control what users can do to extensions.

Refactor the various ways to control what users can do to extensions.

Add an ExtensionManagementPolicy class, held by the ExtensionService, that registers delegates and mediates questions about what can be done to extensions (install, uninstall, modify, etc.). Convert existing mechanisms to control extension operations, namely administrator policy black/whitelists and component extension requirements, to use this abstraction.

In addition to cleaning up existing mechanisms, this will support the control managed mode needs to impose (in an upcoming patch).

BUG=117987
TEST=covered by unit, browser, and api tests

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

[Pam (message me for reviews), 2012-05-14 19:18:56]

Lint fixes

[Pam (message me for reviews), 2012-05-15 17:02:06]

At long last, here is the refactoring to support letting managed mode also
control whether extensions can be modified. (That'll follow in another CL.)

It looks big, but much of this is either minor adjustments to make callers use
the new class, or added/updated tests.

Thanks for your review. Or if there's someone else you'd like to pass this along
to, let me know.

- Pam

[Aaron Boodman, 2012-05-17 22:44:00]

Whoops, I guess http://codereview.chromium.org/9694038/ was old, and I should
have reviewed this one? Is there any difference?

[Pam (message me for reviews), 2012-05-18 10:00:15]

On 2012/05/17 22:44:00, Aaron Boodman wrote:
> Whoops, I guess http://codereview.chromium.org/9694038/ was old, and I should
> have reviewed this one? Is there any difference?

This one has more comments, especially for the new class; a couple more tests
for the new class; and some minor cleanup to make lint happy. I've uploaded the
one you reviewed as patchset 4 here, and then back to this one as patchset 5, so
you can compare them directly.

[Pam (message me for reviews), 2012-05-18 20:59:50]

Here's the latest patch, ready for another look.

I agreed with and implemented most of your comments over on
http://codereview.chromium.org/9694038/, except for a few that I replied to over
there. Sorry about the inconvenience.

- Pam

[Pam (message me for reviews), 2012-05-22 12:51:06]

I've copied over your comments from the older review, to make it easier to see
my changes and replies. If you compare the current patchset against Patch Set 4,
that'll show what's different from the one you reviewed (including a bunch of
changes from updating the checkout).

Please re-review.

- Pam

http://codereview.chromium.org/10382149/diff/7116/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10382149/diff/7116/chrome/app/generated_resour...
chrome/app/generated_resources.grd:4394: +        The administrator of this
machine requires <ph name="EXTENSION_NAME">$1<ex>Google Talk</ex></ph> to be
installed. It cannot be removed or modified.
On 2012/05/17 22:41:06, Aaron Boodman wrote:
> This sentence is a bit awkward. How about:
> 
> The administrator of this machine requires 'Google Talk' to be installed. It
> cannot be removed or modified.

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/c...
File chrome/browser/extensions/crx_installer.cc (right):

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/c...
chrome/browser/extensions/crx_installer.cc:352: if
(!ExtensionSystem::Get(profile_)->management_policy()->UserMayLoad(
On 2012/05/17 22:41:06, Aaron Boodman wrote:
> ExtensionSystem should own ManagementPolicy, not ExtensionService.

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_management_api.cc (right):

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
chrome/browser/extensions/extension_management_api.cc:63:
info->SetBoolean(keys::kMayDisableKey,
On 2012/05/17 22:41:06, Aaron Boodman wrote:
> What's the need for this?

> Wrapping: param lines should all start at same column. Also, it's weird
looking
> to wrap after the arrow.

It's part of the ExtensionInfo returned by the API. It was originally set here,
then it moved into Extension::GetBasicInfo(), and now it has to move back out
since an
extension doesn't know about ManagementPolicy.

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
chrome/browser/extensions/extension_management_api.cc:377:
keys::kUserCantModifyError, extension_id_);
On 2012/05/17 22:41:06, Aaron Boodman wrote:
> Can we just have a generic kUserCantDoAnything error? The string values aren't
> part of our API contract, it's OK to change them.

Excellent, I'd love to consolidate them.

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
chrome/browser/extensions/extension_management_api.cc:432:
->UserMayModifyStatus(extension, NULL)) {
On 2012/05/17 22:41:06, Aaron Boodman wrote:
> Wrap at open paren. You should also change extension_management_policy ->
> management_policy, which will help here.

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:673: if
(extensions::Extension::IsRequired(location))
On 2012/05/17 22:41:06, Aaron Boodman wrote:
> I don't see any Extension::IsRequired. What's this do?

It's a rename (and direction flip) of Extension::UserMayDisable(). It returns
true if an extension must remain installed and enabled, namely if it's a
component or external-policy extension.

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:211: // ManagementPolicy::Provider
On 2012/05/17 22:41:06, Aaron Boodman wrote:
> We usually just say:
> 
> // ExtensionManagementPolicy::Delegate

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:214: virtual bool UserMayLoad(const
extensions::Extension* extension,
On 2012/05/17 22:41:06, Aaron Boodman wrote:
> I don't think this comment is necessary since the interface will describe what
> this is supposed to do, and the rest of the comment is impl detail.

I clarified that this particular management policy provider is checking the
admin policy.

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/i...
File chrome/browser/extensions/installed_loader.cc (right):

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/i...
chrome/browser/extensions/installed_loader.cc:73: extension = Extension::Create(
Is it permissible to Create the extension before checking the admin policy? It
looks safe, and doing that allows UserMayLoad() to take a full Extension*, which
may be useful to some providers.

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/i...
chrome/browser/extensions/installed_loader.cc:104: ->UserMayLoad(extension,
info.extension_location, NULL)) {
On 2012/05/17 22:41:06, Aaron Boodman wrote:
> Maybe the name of this method should be UserMayRun() or UserMayLoad(), since
> it's not really limited to installation.

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/i...
chrome/browser/extensions/installed_loader.cc:106: // and is therefore not well
suited to this UI.
On 2012/05/17 22:41:06, Aaron Boodman wrote:
> Why?

It is typically IDS_EXTENSION_CANT_INSTALL_POLICY_BLACKLIST, which contains the
extension ID. That's pretty ugly to have appear in user UI (as opposed to log
messages), but I'm willing to use it if you think it's good.

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/m...
File chrome/browser/extensions/management_policy.cc (right):

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/m...
chrome/browser/extensions/management_policy.cc:47: << " prohibited by " <<
(*it)->PolicyProviderName();
On 2012/05/17 22:41:06, Aaron Boodman wrote:
> For debugging purposes, I suggest giving each policy provider a human-readable
> name, and then printing out something like:
> 
> DLOG(ERROR) << "Installation prevented by " << provider->name();

Sure. To avoid spam, since these get called to check whether to show UI and so
on too, I'll make it a warning.

> It seems like the providers will also be interested in the Extension* itself.
> For example, whether it's a theme or an extension, or what permissions it has.
> Is it easy where this is called from to pass the whole extension?

Yes, as long as we also pass the location, since CrxInstaller doesn't save that
to the extension until it's successful. Done.

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/m...
File chrome/browser/extensions/management_policy.h (right):

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/m...
chrome/browser/extensions/management_policy.h:26: class ManagementPolicy {
On 2012/05/17 22:41:06, Aaron Boodman wrote:
> extensions::ManagementPolicy - then you can make the filename shorter, too.

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/m...
chrome/browser/extensions/management_policy.h:42: class Provider {
On 2012/05/17 22:41:06, Aaron Boodman wrote:
> A class can typically only have a single delegate. PolicyProvider?

There's precedent in the list of delegates in resource_queue.cc. I'm open to
other names, but I want to avoid (even more) confusion with admin policy and
those policy providers. "Source"? Or "Provider", and give up on not overloading
the names? I'll try "Provider" and a comment.

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/m...
chrome/browser/extensions/management_policy.h:61: virtual bool
UserMayModifyStatus(const Extension* extension,
On 2012/05/17 22:41:06, Aaron Boodman wrote:
> ModifySettings?

Sure, except it's not just settings anymore.

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/m...
chrome/browser/extensions/management_policy.h:69: virtual bool
MustRemainEnabled(const Extension* extension,
On 2012/05/17 22:41:06, Aaron Boodman wrote:
> If an extension must run, that means it can't be disabled or removed, so
there's
> no need for those to be called out separately.
> 
> If it must not run, that means it can't be installed or loaded (if it was
> previously installed).

That would be enough for policy force- and blacklists. But there's also the case
where an extension can either run or not run, but whichever it is, the user
can't change it. That's what managed mode will be applying.

I don't like much MustRemainEnabled() as an outlier, but it's the one that
differs between policy forcelist and managed mode.

Looking at the callers, though, UserMayModifyStatus and UserMayModifyUsage are
mutually redundant. I'll combine them.

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/m...
chrome/browser/extensions/management_policy.h:104: int GetNumProviders() const;
On 2012/05/17 22:41:06, Aaron Boodman wrote:
> Use int.

> GetNumDelegates().

[Aaron Boodman, 2012-05-22 15:56:11]

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_context_menu_browsertest.cc (right):

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
chrome/browser/extensions/extension_context_menu_browsertest.cc:522: // We need
an extension to pass to the menu constructor, but we don't care
Nit: add a blank line here.

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
chrome/browser/extensions/extension_context_menu_browsertest.cc:546:
ASSERT_FALSE(menu->IsCommandIdEnabled(ExtensionContextMenuModel::UNINSTALL));
Yay, this came out clean.

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_management_api.cc (right):

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
chrome/browser/extensions/extension_management_api.cc:63:
info->SetBoolean(keys::kMayDisableKey,
On 2012/05/22 12:51:07, Pam wrote:
> On 2012/05/17 22:41:06, Aaron Boodman wrote:
> > What's the need for this?
> 
> > Wrapping: param lines should all start at same column. Also, it's weird
> looking
> > to wrap after the arrow.
> 
> It's part of the ExtensionInfo returned by the API. It was originally set
here,
> then it moved into Extension::GetBasicInfo(), and now it has to move back out
> since an
> extension doesn't know about ManagementPolicy.

No what I meant was: is there a client that needs to know whether an extension
may be disabled?

Also, as to wrapping, I meant that wrapping at the arrow (either before or
after) looks strange to me, and I'd prefer wrapping at the open paren or storing
a pointer if possible.

That said, looking at this code, you use this pattern everywhere, and the lesser
of two evils is probably to just do it all on one virtual line, wrapped *after*
the arrow (sorry).

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_management_apitest.cc (right):

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
chrome/browser/extensions/extension_management_apitest.cc:76: std::string
GetExtensionIdByName(const ExtensionService* service,
LoadExtension() returns Extension*. You can just store the extensions as you
load them as member data, then you don't need this.

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_management_browsertest.cc (right):

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
chrome/browser/extensions/extension_management_browsertest.cc:11: #include
"chrome/browser/extensions/extension_system.h"
Extra includes?

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:664: std::string
ExtensionPrefs::PolicyProviderName() const {
Methods should typically be verb phrases, so GetPolicyProviderName().

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:673: if
(extensions::Extension::IsRequired(location))
On 2012/05/22 12:51:07, Pam wrote:
> On 2012/05/17 22:41:06, Aaron Boodman wrote:
> > I don't see any Extension::IsRequired. What's this do?
> 
> It's a rename (and direction flip) of Extension::UserMayDisable(). It returns
> true if an extension must remain installed and enabled, namely if it's a
> component or external-policy extension.

Ah. Can that bit be moved into extensions::ManagementPolicy? Is there a reason
for it to be accessible on its own?

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:214: virtual bool UserMayLoad(const
extensions::Extension* extension,
On 2012/05/22 12:51:07, Pam wrote:
> On 2012/05/17 22:41:06, Aaron Boodman wrote:
> > I don't think this comment is necessary since the interface will describe
what
> > this is supposed to do, and the rest of the comment is impl detail.
> 
> I clarified that this particular management policy provider is checking the
> admin policy.

Ah. You could make that more self-documenting and flexible by factoring this out
into a small helper class which implements ManagementPolicy::Provider.
ExtensionPrefs would have GetAdminPolicyProvider().

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

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:837: bool modifiable =
!Extension::IsRequired(extension->location());
It seems like if ManagementPolicy did this check itself, this implementation of
Provider would not be necessary.

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.h (right):

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.h:343: // The ExtensionService
exposes any restrictions imposed by an individual
This comment doesn't seem correct compared to the code.

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

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
chrome/browser/extensions/extension_system.cc:92:
management_policy_->RegisterProvider(extension_service_.get());
This circular dependency between ManagementPolicy and ExtensionService is weird
and would be great to get rid of.

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
chrome/browser/extensions/extension_system.cc:133: management_policy_.reset(new
extensions::ManagementPolicy);
You could just create the management policy in the constructor. It doesn't even
need to be a scoped pointer.

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/m...
File chrome/browser/extensions/management_policy.cc (right):

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/m...
chrome/browser/extensions/management_policy.cc:40: string16* error) const {
Seems like the logic from Extension::IsRequired() could move here.

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/m...
File chrome/browser/extensions/management_policy.h (right):

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/m...
chrome/browser/extensions/management_policy.h:16: // This class registers
providers that want to prohibit certain actions from
Add a blank line here.

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/m...
chrome/browser/extensions/management_policy.h:52: // configured when this is
called (e.g., extension->location() may not be
Is location() the only edge case like this? This bit of the comment is kinda
scary ... it would be good if it could be more specific as to what is setup and
what isn't.

Or, can the calling code be moved so that the extension is fully configured?

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/m...
chrome/browser/extensions/management_policy.h:61: virtual bool
UserMayModifyStatus(const Extension* extension,
On 2012/05/22 12:51:07, Pam wrote:
> On 2012/05/17 22:41:06, Aaron Boodman wrote:
> > ModifySettings?
> 
> Sure, except it's not just settings anymore.

I meant settings to encompass all of them, including enabledness.

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/m...
chrome/browser/extensions/management_policy.h:69: virtual bool
MustRemainEnabled(const Extension* extension,
On 2012/05/22 12:51:07, Pam wrote:
> On 2012/05/17 22:41:06, Aaron Boodman wrote:
> > If an extension must run, that means it can't be disabled or removed, so
> there's
> > no need for those to be called out separately.
> > 
> > If it must not run, that means it can't be installed or loaded (if it was
> > previously installed).
> 
> That would be enough for policy force- and blacklists. But there's also the
case
> where an extension can either run or not run, but whichever it is, the user
> can't change it. That's what managed mode will be applying.
> 
> I don't like much MustRemainEnabled() as an outlier, but it's the one that
> differs between policy forcelist and managed mode.
> 
> Looking at the callers, though, UserMayModifyStatus and UserMayModifyUsage are
> mutually redundant. I'll combine them.

Couldn't that be implemented by just looking at the current enabled status of
the extension?

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/t...
File chrome/browser/extensions/test_extension_system.cc (right):

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/t...
chrome/browser/extensions/test_extension_system.cc:82:
extensions::ManagementPolicy* TestExtensionSystem::management_policy() {
Why bother overriding this? ManagementPolicy is fine to use in tests as it is...
if callers want they can ClearProviders() on it and add their own. Seems a
little simpler.

[Pam (message me for reviews), 2012-05-23 15:00:58]

Thanks for the review. Please take another look. There are a few ongoing
discussions -- I'll try to catch you in chat later, but would appreciate written
response if I can't get back online tonight.

- Pam

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_management_api.cc (right):

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
chrome/browser/extensions/extension_management_api.cc:63:
info->SetBoolean(keys::kMayDisableKey,
On 2012/05/22 15:56:11, Aaron Boodman wrote:
> No what I meant was: is there a client that needs to know whether an extension
> may be disabled?

Sorry, I could have been clearer. It's a documented field in the ExtensionInfo
object returned by the public chrome.management.get() extension API.

> Also, as to wrapping, I meant that wrapping at the arrow (either before or
> after) looks strange to me, and I'd prefer wrapping at the open paren or
storing
> a pointer if possible.

In some places there wasn't a good open paren to wrap at (wrapping between empty
() looks ludicrous to me), but I'll change them to store pointers.

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:673: if
(extensions::Extension::IsRequired(location))
On 2012/05/22 15:56:11, Aaron Boodman wrote:
 Can that bit be moved into extensions::ManagementPolicy? Is there a reason
> for it to be accessible on its own?

I don't want to pollute extensions::ManagementPolicy with direct knowledge of
any of the policies.

I assume the former UserMayDisable was in Extension because that's where the
Extension::Location and a bunch of other methods with implementations that only
depend on the location (IsAutoUpdateableLocation, ShouldAlwaysAllowFileAccess,
etc.) are. I can't think of a better place for it, but am open to suggestions.

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:214: virtual bool UserMayLoad(const
extensions::Extension* extension,
> You could make that more self-documenting and flexible by factoring this out
> into a small helper class which implements ManagementPolicy::Provider.
> ExtensionPrefs would have GetAdminPolicyProvider().

How important is this to you? It feels like overcomplication to me.

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

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:837: bool modifiable =
!Extension::IsRequired(extension->location());
On 2012/05/22 15:56:11, Aaron Boodman wrote:
> It seems like if ManagementPolicy did this check itself, this implementation
of
> Provider would not be necessary.

True, but the ManagementPolicy class is pretty abstract. I don't think it should
know that much detail about specific policies.

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

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
chrome/browser/extensions/extension_system.cc:92:
management_policy_->RegisterProvider(extension_service_.get());
On 2012/05/22 15:56:11, Aaron Boodman wrote:
> This circular dependency between ManagementPolicy and ExtensionService is
weird
> and would be great to get rid of.

I'm not sure I follow. Moving the ManagementPolicy over to the ExtensionSystem
got rid of the circular dependency. ExtensionSystem and ExtensionService still
cross-reference each other, but that's not new.

ExtensionSystem owns a ManagementPolicy, and also owns an ExtensionService and
an ExtensionPrefs. Both the ExtensionService and the ExtensionPrefs are
Providers for the ManagementPolicy. It's a little complicated, but not too bad.

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
chrome/browser/extensions/extension_system.cc:133: management_policy_.reset(new
extensions::ManagementPolicy);
On 2012/05/22 15:56:11, Aaron Boodman wrote:
> You could just create the management policy in the constructor. It doesn't
even
> need to be a scoped pointer.

Because we don't care about leaks on shutdown? Or it should be on the stack? Or
am I missing something? Why are all the other components, the
extension_event_router_ and extension_service_ and so on, scoped pointers, and
not created until here?

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/m...
File chrome/browser/extensions/management_policy.h (right):

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/m...
chrome/browser/extensions/management_policy.h:52: // configured when this is
called (e.g., extension->location() may not be
On 2012/05/22 15:56:11, Aaron Boodman wrote:
> Is location() the only edge case like this? This bit of the comment is kinda
> scary ... it would be good if it could be more specific as to what is setup
and
> what isn't.
> 
> Or, can the calling code be moved so that the extension is fully configured?

The apparent problem is the call in CrxInstaller::ConfirmInstall. I had taken
the comment for install_source_ in CrxInstaller to mean that
extension->location() wasn't always fully set there, but I don't see why. Looks
like you wrote it, way back when -- can you explain why install_source_ is
needed in crx_installer.cc? See for example line ~221 in that file: why is it
using install_source_ there instead of extension->location()?

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/m...
chrome/browser/extensions/management_policy.h:61: virtual bool
UserMayModifyStatus(const Extension* extension,
On 2012/05/22 15:56:11, Aaron Boodman wrote:
> On 2012/05/22 12:51:07, Pam wrote:
> > On 2012/05/17 22:41:06, Aaron Boodman wrote:
> > > ModifySettings?
> > 
> > Sure, except it's not just settings anymore.
> 
> I meant settings to encompass all of them, including enabledness.

Coming at it as I do from the preferences side of the codebase, I've always
found it confusing that Settings refers both to the configuration options that
apply to an extension, and to the prefs that the extension itself sets. So I
instinctively avoid using "Settings" as a term, so I don't confuse myself. But
if "Settings" correctly covers all the things this does, I'm OK with it.

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/m...
chrome/browser/extensions/management_policy.h:69: virtual bool
MustRemainEnabled(const Extension* extension,
On 2012/05/22 15:56:11, Aaron Boodman wrote:
> On 2012/05/22 12:51:07, Pam wrote:
> > On 2012/05/17 22:41:06, Aaron Boodman wrote:
> > > If an extension must run, that means it can't be disabled or removed, so
> > there's
> > > no need for those to be called out separately.
> > > 
> > > If it must not run, that means it can't be installed or loaded (if it was
> > > previously installed).
> > 
> > That would be enough for policy force- and blacklists. But there's also the
> case
> > where an extension can either run or not run, but whichever it is, the user
> > can't change it. That's what managed mode will be applying.
> > 
> > I don't like much MustRemainEnabled() as an outlier, but it's the one that
> > differs between policy forcelist and managed mode.
> > 
> > Looking at the callers, though, UserMayModifyStatus and UserMayModifyUsage
are
> > mutually redundant. I'll combine them.
> 
> Couldn't that be implemented by just looking at the current enabled status of
> the extension?

Not enabled status, but installed status: we still want to show
installed-but-disabled extensions in the UI. Is it possible to distinguish an
extension in the process of being installed from one being loaded on startup,
given only the Extension itself?

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/t...
File chrome/browser/extensions/test_extension_system.cc (right):

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/t...
chrome/browser/extensions/test_extension_system.cc:82:
extensions::ManagementPolicy* TestExtensionSystem::management_policy() {
On 2012/05/22 15:56:11, Aaron Boodman wrote:
> Why bother overriding this? ManagementPolicy is fine to use in tests as it
is...
> if callers want they can ClearProviders() on it and add their own. Seems a
> little simpler.

TestExtensionSystem is a subclass of ExtensionSystem, not of
ExtensionSystemImpl, so we need to implement the abstract method here. And
management_policy_ is private to ExtensionSystemImpl::Shared, normally exposed
only via the ExtensionSystemImpl, so we need a local one for testing.

[Aaron Boodman, 2012-05-24 08:11:22]

Not completely done. The methods available on ManagementPolicy::Provider still
seem a bit weird to me, but I'm getting too tired, so I will have to pick this
up tomorrow.

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:673: if
(extensions::Extension::IsRequired(location))
On 2012/05/23 15:00:58, Pam wrote:
> On 2012/05/22 15:56:11, Aaron Boodman wrote:
>  Can that bit be moved into extensions::ManagementPolicy? Is there a reason
> > for it to be accessible on its own?
> 
> I don't want to pollute extensions::ManagementPolicy with direct knowledge of
> any of the policies.
> 
> I assume the former UserMayDisable was in Extension because that's where the
> Extension::Location and a bunch of other methods with implementations that
only
> depend on the location (IsAutoUpdateableLocation, ShouldAlwaysAllowFileAccess,
> etc.) are. I can't think of a better place for it, but am open to suggestions.

OK, I see.

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:213: // Retruns true if the
extension is allowed by admin policy.
sp: Retruns

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:214: virtual bool UserMayLoad(const
extensions::Extension* extension,
On 2012/05/23 15:00:58, Pam wrote:
> > You could make that more self-documenting and flexible by factoring this out
> > into a small helper class which implements ManagementPolicy::Provider.
> > ExtensionPrefs would have GetAdminPolicyProvider().
> 
> How important is this to you? It feels like overcomplication to me.

Shrug, ok.

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

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:837: bool modifiable =
!Extension::IsRequired(extension->location());
On 2012/05/23 15:00:58, Pam wrote:
> On 2012/05/22 15:56:11, Aaron Boodman wrote:
> > It seems like if ManagementPolicy did this check itself, this implementation
> of
> > Provider would not be necessary.
> 
> True, but the ManagementPolicy class is pretty abstract. I don't think it
should
> know that much detail about specific policies.

I can see that. I just want to get this interface out of ExtensionService and
was thinking putting it in ManagementPolicy would solve the problem.

As we discussed offline, you could move these checks into extnsion_prefs.cc too.

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

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/e...
chrome/browser/extensions/extension_system.cc:133: management_policy_.reset(new
extensions::ManagementPolicy);
On 2012/05/23 15:00:58, Pam wrote:
> On 2012/05/22 15:56:11, Aaron Boodman wrote:
> > You could just create the management policy in the constructor. It doesn't
> even
> > need to be a scoped pointer.
> 
> Because we don't care about leaks on shutdown? Or it should be on the stack?
Or
> am I missing something? Why are all the other components, the
> extension_event_router_ and extension_service_ and so on, scoped pointers, and
> not created until here?

I meant that it could be a plain old member. I don't know why no other pieces do
this... it's fine the way you have it.

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/m...
File chrome/browser/extensions/management_policy.h (right):

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/m...
chrome/browser/extensions/management_policy.h:52: // configured when this is
called (e.g., extension->location() may not be
On 2012/05/23 15:00:58, Pam wrote:
> On 2012/05/22 15:56:11, Aaron Boodman wrote:
> > Is location() the only edge case like this? This bit of the comment is kinda
> > scary ... it would be good if it could be more specific as to what is setup
> and
> > what isn't.
> > 
> > Or, can the calling code be moved so that the extension is fully configured?
> 
> The apparent problem is the call in CrxInstaller::ConfirmInstall. I had taken
> the comment for install_source_ in CrxInstaller to mean that
> extension->location() wasn't always fully set there, but I don't see why.
Looks
> like you wrote it, way back when -- can you explain why install_source_ is
> needed in crx_installer.cc? See for example line ~221 in that file: why is it
> using install_source_ there instead of extension->location()?

It used to be that extensions were unpacked with no particular location, and we
didn't actually set the location until much later.

This caused some problems though, so it was fixed relatively recently. The
install_source_ is now passed into SandboxedExtensionUnpacker, and from there
passed on down through the layers all the way to where the Extension* is first
created. So I believe it should be correct now, and there's no reason to pass it
separately. Line 221 could likewise be changed, as well as the comment in the .h
file.

http://codereview.chromium.org/10382149/diff/7116/chrome/browser/extensions/m...
chrome/browser/extensions/management_policy.h:61: virtual bool
UserMayModifyStatus(const Extension* extension,
On 2012/05/23 15:00:58, Pam wrote:
> On 2012/05/22 15:56:11, Aaron Boodman wrote:
> > On 2012/05/22 12:51:07, Pam wrote:
> > > On 2012/05/17 22:41:06, Aaron Boodman wrote:
> > > > ModifySettings?
> > > 
> > > Sure, except it's not just settings anymore.
> > 
> > I meant settings to encompass all of them, including enabledness.
> 
> Coming at it as I do from the preferences side of the codebase, I've always
> found it confusing that Settings refers both to the configuration options that
> apply to an extension, and to the prefs that the extension itself sets. So I
> instinctively avoid using "Settings" as a term, so I don't confuse myself. But
> if "Settings" correctly covers all the things this does, I'm OK with it.

I don't think we have a convention, but I prefer settings in this case since in
typical English it could mean either, and in this case it means both. The
comment explains the detail.

[Pam (message me for reviews), 2012-05-24 14:46:35]

All comments (so far) addressed. Looking forward to the rest. :)

- Pam

[Pam (message me for reviews), 2012-05-28 21:21:08]

Any news here?

Thanks,
- Pam

[Aaron Boodman, 2012-05-29 03:03:07]

LGTM, no need to wait for another review from me. But please do factor out the
admin policy.

http://codereview.chromium.org/10382149/diff/37004/chrome/browser/extensions/...
File chrome/browser/extensions/crx_installer.cc (right):

http://codereview.chromium.org/10382149/diff/37004/chrome/browser/extensions/...
chrome/browser/extensions/crx_installer.cc:223: // SandboxedExtensionUnpacker
sets extension->location.
Hooray.

http://codereview.chromium.org/10382149/diff/37004/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/10382149/diff/37004/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:211: // ManagementPolicy::Provider
I would really prefer this be a separate helper class.

The admin forcelist policy implementation is really a totally different thing
than ExtensionPrefs. Mixing them together ends up requiring these comments to
explain what could be clear from a different factoring.

So add admin_policy.h/cc or whatever and ExtensionPrefs::GetAdminPolicy().

http://codereview.chromium.org/10382149/diff/37004/chrome/browser/extensions/...
File chrome/browser/extensions/installed_loader.cc (right):

http://codereview.chromium.org/10382149/diff/37004/chrome/browser/extensions/...
chrome/browser/extensions/installed_loader.cc:94: // An explicit check against
policy is required to behave correctly during
Comment sounds a bit scarier than necessary. Suggest something softer, like:

// Check policy at every load because policies can change
// after extension installation.

http://codereview.chromium.org/10382149/diff/37004/chrome/browser/extensions/...
chrome/browser/extensions/installed_loader.cc:95: // startup.  This is because
extensions that were previously OK might have
Remove one space after the period :)

http://codereview.chromium.org/10382149/diff/37004/chrome/browser/extensions/...
chrome/browser/extensions/installed_loader.cc:97: // This isn't a new extension
installation, so it's OK if some extension
"so it's ok if some extension..."

But that's not actually the case here, right? They are registered at this point.
So can we just remove that caveat?

http://codereview.chromium.org/10382149/diff/37004/chrome/browser/ui/webui/ex...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

http://codereview.chromium.org/10382149/diff/37004/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:104: // This
should only happen in testing.
Can we somehow make the management policy exist in testing?

http://codereview.chromium.org/10382149/diff/37004/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:299: if
(extension_service_) {
Is this again because of testing? It would be nice to remove this case if
possible.

[Pam (message me for reviews), 2012-05-30 17:42:20]

Dear OWNERS,

Please take a look at the applicable pieces as listed below.

estade: extension_list.js; ui/webui/*
xiyuan: extension_app_item.cc

I'll be making the final changes requested by Aaron tomorrow, which will be the
middle of the night your time. If you're willing to approve the patch today with
those changes pending, it'll save me a full day of blockage. Thanks for your
consideration.

- Pam

[xiyuan, 2012-05-30 18:13:38]

extension_app_item.cc LGTM

[Evan Stade, 2012-05-30 21:20:01]

http://codereview.chromium.org/10382149/diff/42056/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extension_list.js (right):

http://codereview.chromium.org/10382149/diff/42056/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_list.js:160:
!extension.mayModifySettings;
.may-not-disable .optional-controls {
  display: none;
}

also I wonder why you chose to hide them over greying them out, like for a
disabled extension?

http://codereview.chromium.org/10382149/diff/42056/chrome/browser/ui/webui/ex...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

http://codereview.chromium.org/10382149/diff/42056/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:101:
extension_data->SetBoolean("mayModifySettings",
I would go with userModifiable

[Evan Stade, 2012-05-31 17:56:08]

lgtm with the above addressed (you can ignore the hiding question)

On 2012/05/30 21:20:01, Evan Stade wrote:
>
http://codereview.chromium.org/10382149/diff/42056/chrome/browser/resources/e...
> File chrome/browser/resources/extensions/extension_list.js (right):
> 
>
http://codereview.chromium.org/10382149/diff/42056/chrome/browser/resources/e...
> chrome/browser/resources/extensions/extension_list.js:160:
> !extension.mayModifySettings;
> .may-not-disable .optional-controls {
>   display: none;
> }
> 
> also I wonder why you chose to hide them over greying them out, like for a
> disabled extension?
> 
>
http://codereview.chromium.org/10382149/diff/42056/chrome/browser/ui/webui/ex...
> File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):
> 
>
http://codereview.chromium.org/10382149/diff/42056/chrome/browser/ui/webui/ex...
> chrome/browser/ui/webui/extensions/extension_settings_handler.cc:101:
> extension_data->SetBoolean("mayModifySettings",
> I would go with userModifiable

[commit-bot: I haz the power, 2012-06-01 18:29:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pam@chromium.org/10382149/32032

[commit-bot: I haz the power, 2012-06-01 19:44:30]

Try job failure for 10382149-32032 (retry) on linux_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-06-01 19:46:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pam@chromium.org/10382149/32032

[commit-bot: I haz the power, 2012-06-01 21:56:18]

Try job failure for 10382149-32032 (retry) on win_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-06-02 06:38:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pam@chromium.org/10382149/32032

[commit-bot: I haz the power, 2012-06-02 06:39:08]

Failed to apply patch for
chrome/browser/extensions/extension_context_menu_browsertest.cc:
While running patch -p0 --forward --force;
patching file chrome/browser/extensions/extension_context_menu_browsertest.cc
Hunk #1 FAILED at 5.
Hunk #2 succeeded at 554 with fuzz 2 (offset 46 lines).
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/extensions/extension_context_menu_browsertest.cc.rej

[commit-bot: I haz the power, 2012-06-02 10:44:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pam@chromium.org/10382149/77007

[commit-bot: I haz the power, 2012-06-02 11:16:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pam@chromium.org/10382149/83007

[commit-bot: I haz the power, 2012-06-02 13:14:18]

Change committed as 140194

[rniwa-cr, 2012-06-03 05:25:00]

executable bit removed from test_management_policy.cc in
http://src.chromium.org/viewvc/chrome?view=rev&revision=140222 per
http://build.chromium.org/p/chromium/builders/Linux/builds/25542/steps/check_...