OBSOLETE REVIEW. See http://codereview.chromium.org/10382149/

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

[Pam (message me for reviews), 2012-03-13 13:58:16]

Hi Evan,

Please take a look. I'll attach some screenshots to the bug for reference.

I'm a CSS newbie, and can't figure out how to get the extension-settings-list to
move down when the managed-mode-message is showing. If I add a bottom margin to
#managed-mode-message, it obscures the top N pixels of the first extension in
the list. Can you enlighten me?

Thanks,
- Pam

[Evan Stade, 2012-03-13 21:02:07]

https://chromiumcodereview.appspot.com/9694038/diff/1009/chrome/browser/resou...
File chrome/browser/resources/extensions/extension_list.js (right):

https://chromiumcodereview.appspot.com/9694038/diff/1009/chrome/browser/resou...
chrome/browser/resources/extensions/extension_list.js:156: if
(this.data_.managedMode) {
node.querySelector('.optional-controls').hidden = this.data_.managedMode;

https://chromiumcodereview.appspot.com/9694038/diff/1009/chrome/browser/resou...
File chrome/browser/resources/extensions/extensions.html (right):

https://chromiumcodereview.appspot.com/9694038/diff/1009/chrome/browser/resou...
chrome/browser/resources/extensions/extensions.html:39: <div
id="managed-mode-message" class="butter-bar" hidden
I believe you'd most likely want to copy the appearance and CSS of the managed
prefs banner from options. The reason margin isn't working is that header is
fixed (see how it stays in place when you scroll vertically)

https://chromiumcodereview.appspot.com/9694038/diff/1009/chrome/browser/resou...
chrome/browser/resources/extensions/extensions.html:40:
i18n-content="extensionSettingsManagedMode"></div>
2 more spaces indent

https://chromiumcodereview.appspot.com/9694038/diff/1009/chrome/browser/resou...
File chrome/browser/resources/extensions/extensions.js (right):

https://chromiumcodereview.appspot.com/9694038/diff/1009/chrome/browser/resou...
chrome/browser/resources/extensions/extensions.js:197: developer_mode = false;
duplicate logic?

https://chromiumcodereview.appspot.com/9694038/diff/1009/chrome/browser/ui/we...
File chrome/browser/ui/webui/options/extension_settings_handler.cc (right):

https://chromiumcodereview.appspot.com/9694038/diff/1009/chrome/browser/ui/we...
chrome/browser/ui/webui/options/extension_settings_handler.cc:268: return;
nit: \n

[Aaron Boodman, 2012-03-13 21:29:35]

This is not going to be the right approach. There are many ways to
disable/uninstall/otherwise configure extensions. In fact most pieces of
extension UI expose some way to disable or uninstall.

We would have to bake this further down into the extension system, and then also
disable the UI.

It would be good to step back and have some kind of mini-design-doc about how
all this will work.

For example, there is already a few different mechanisms that can prevent
extensions from being installed/disabled/uninstalled. These should be exposed in
a consistent Can(Install|Uninstall|Manage)Extension() API that the various bits
of UI can call.

Sorry to be a wet blanket.

[Pam (message me for reviews), 2012-03-13 22:43:02]

On 2012/03/13 21:29:35, Aaron Boodman wrote:
> This is not going to be the right approach. There are many ways to
> disable/uninstall/otherwise configure extensions.

This isn't the only piece of the work, just the first one, split off in an
effort to keep the CL non-titanic. As the bug says, work on the back-end will
follow. I would prefer to land it and modify it further as needed; the only
piece that's likely to really change is 10 lines in extensions.js. That may also
help to keep later patches sane.

I'm not sure that a fully general API is needed for only policy and managed
mode, but you're certainly more familiar with the other ways that extensions can
be locked than I am.  Let's discuss that separately.

- Pam

[Aaron Boodman, 2012-03-13 22:53:03]

On Tue, Mar 13, 2012 at 3:43 PM,  <pam@chromium.org> wrote:
> On 2012/03/13 21:29:35, Aaron Boodman wrote:
>>
>> This is not going to be the right approach. There are many ways to
>> disable/uninstall/otherwise configure extensions.
>
>
> This isn't the only piece of the work, just the first one, split off in an
> effort to keep the CL non-titanic. As the bug says, work on the back-end
> will
> follow. I would prefer to land it and modify it further as needed; the only
> piece that's likely to really change is 10 lines in extensions.js. That may
> also
> help to keep later patches sane.
>
> I'm not sure that a fully general API is needed for only policy and managed
> mode, but you're certainly more familiar with the other ways that extensions
> can
> be locked than I am.  Let's discuss that separately.

I would prefer to have a mini doc first before we start writing code
(I realize you've already written some code, but that was more
isolated).

- a

[Pam (message me for reviews), 2012-05-14 08:47:22]

Resolved conflicts

[Aaron Boodman, 2012-05-17 22:41:06]

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

http://codereview.chromium.org/9694038/diff/18021/chrome/app/generated_resour...
chrome/app/generated_resources.grd:4391: +        <ph
name="EXTENSION_NAME">$1<ex>Google Talk</ex></ph> (extension ID "<ph
name="EXTENSION_ID">$2<ex>nckgahadagoaajjgafhacjanaoiihapd</ex></ph>") is
required by the administrator and cannot be modified.
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/9694038/diff/18021/chrome/browser/extensions/c...
File chrome/browser/extensions/crx_installer.cc (right):

http://codereview.chromium.org/9694038/diff/18021/chrome/browser/extensions/c...
chrome/browser/extensions/crx_installer.cc:352: if
(!frontend_weak_->extension_management_policy()->UserMayInstall(
ExtensionSystem should own ManagementPolicy, not ExtensionService.

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

http://codereview.chromium.org/9694038/diff/18021/chrome/browser/extensions/e...
chrome/browser/extensions/extension_management_api.cc:61:
info->SetBoolean(keys::kMayDisableKey, service->
What's the need for this?

http://codereview.chromium.org/9694038/diff/18021/chrome/browser/extensions/e...
chrome/browser/extensions/extension_management_api.cc:61:
info->SetBoolean(keys::kMayDisableKey, service->
Wrapping: param lines should all start at same column. Also, it's weird looking
to wrap after the arrow.

http://codereview.chromium.org/9694038/diff/18021/chrome/browser/extensions/e...
chrome/browser/extensions/extension_management_api.cc:375:
keys::kUserCantModifyError, extension_id_);
Can we just have a generic kUserCantDoAnything error? The string values aren't
part of our API contract, it's OK to change them.

http://codereview.chromium.org/9694038/diff/18021/chrome/browser/extensions/e...
chrome/browser/extensions/extension_management_api.cc:434:
UserMayModifyStatus(extension, NULL)) {
Wrap at open paren. You should also change extension_management_policy ->
management_policy, which will help here.

http://codereview.chromium.org/9694038/diff/18021/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_management_policy.cc (right):

http://codereview.chromium.org/9694038/diff/18021/chrome/browser/extensions/e...
chrome/browser/extensions/extension_management_policy.cc:51: if
(!(*it)->UserMayInstall(extension_id, location, extension_name, error))
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();

http://codereview.chromium.org/9694038/diff/18021/chrome/browser/extensions/e...
chrome/browser/extensions/extension_management_policy.cc:51: if
(!(*it)->UserMayInstall(extension_id, location, extension_name, error))
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?

http://codereview.chromium.org/9694038/diff/18021/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_management_policy.h (right):

http://codereview.chromium.org/9694038/diff/18021/chrome/browser/extensions/e...
chrome/browser/extensions/extension_management_policy.h:15: class
ExtensionManagementPolicy {
extensions::ManagementPolicy - then you can make the filename shorter, too.

http://codereview.chromium.org/9694038/diff/18021/chrome/browser/extensions/e...
chrome/browser/extensions/extension_management_policy.h:17: class Delegate {
A class can typically only have a single delegate. PolicyProvider?

http://codereview.chromium.org/9694038/diff/18021/chrome/browser/extensions/e...
chrome/browser/extensions/extension_management_policy.h:33: virtual bool
UserMayModifyUsage(const Extension* extension,
ModifySettings?

http://codereview.chromium.org/9694038/diff/18021/chrome/browser/extensions/e...
chrome/browser/extensions/extension_management_policy.h:37: virtual bool
MustRemainEnabled(const Extension* extension,
It seems like these can be simplified:

// Better name needed.
enum ExtensionRunDecision {
  DEFAULT,  // It's up to the user
  EXTENSION_MUST_RUN,
  EXTENSION_MUST_NOT_RUN,
};

ExtensionRunDecision GetRunDecision(const std::string& extension_id);
bool CanUserModifySettings(const std::string& extension_id);

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).

http://codereview.chromium.org/9694038/diff/18021/chrome/browser/extensions/e...
chrome/browser/extensions/extension_management_policy.h:71: size_t
CountDelegates();
Use int.

http://codereview.chromium.org/9694038/diff/18021/chrome/browser/extensions/e...
chrome/browser/extensions/extension_management_policy.h:71: size_t
CountDelegates();
GetNumDelegates().

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

http://codereview.chromium.org/9694038/diff/18021/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:662: if
(Extension::IsRequired(location))
I don't see any Extension::IsRequired. What's this do?

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

http://codereview.chromium.org/9694038/diff/18021/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:207: //
ExtensionManagementPolicy::Delegate implementation:
We usually just say:

// ExtensionManagementPolicy::Delegate

http://codereview.chromium.org/9694038/diff/18021/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:208: // Returns true if the
extension with |extension_id| is allowed by policy
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.

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

http://codereview.chromium.org/9694038/diff/18021/chrome/browser/extensions/i...
chrome/browser/extensions/installed_loader.cc:78:
UserMayInstall(info.extension_id, info.extension_location, std::string(),
Maybe the name of this method should be UserMayRun() or UserMayLoad(), since
it's not really limited to installation.

http://codereview.chromium.org/9694038/diff/18021/chrome/browser/extensions/i...
chrome/browser/extensions/installed_loader.cc:80: // The error message from
UserMayInstall() is not well suited to this UI.
Why?

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

Replies to comments. The new patchset is over in the real review. Sorry for the
hassle that'll cause.

- Pam

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

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

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

http://codereview.chromium.org/9694038/diff/18021/chrome/browser/extensions/e...
chrome/browser/extensions/extension_management_api.cc:375:
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/9694038/diff/18021/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_management_policy.cc (right):

http://codereview.chromium.org/9694038/diff/18021/chrome/browser/extensions/e...
chrome/browser/extensions/extension_management_policy.cc:51: if
(!(*it)->UserMayInstall(extension_id, location, extension_name, error))
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.

http://codereview.chromium.org/9694038/diff/18021/chrome/browser/extensions/e...
chrome/browser/extensions/extension_management_policy.cc:51: if
(!(*it)->UserMayInstall(extension_id, location, extension_name, error))
On 2012/05/17 22:41:06, Aaron Boodman wrote:
> 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/9694038/diff/18021/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_management_policy.h (right):

http://codereview.chromium.org/9694038/diff/18021/chrome/browser/extensions/e...
chrome/browser/extensions/extension_management_policy.h:17: class Delegate {
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/9694038/diff/18021/chrome/browser/extensions/e...
chrome/browser/extensions/extension_management_policy.h:37: 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/9694038/diff/18021/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/9694038/diff/18021/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:662: if
(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/9694038/diff/18021/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/9694038/diff/18021/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.h:208: // Returns true if the
extension with |extension_id| is allowed by policy
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/9694038/diff/18021/chrome/browser/extensions/i...
File chrome/browser/extensions/installed_loader.cc (right):

http://codereview.chromium.org/9694038/diff/18021/chrome/browser/extensions/i...
chrome/browser/extensions/installed_loader.cc:80: // The error message from
UserMayInstall() is 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/9694038/diff/18021/chrome/browser/extensions/i...
chrome/browser/extensions/installed_loader.cc:83: 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.

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

All Aaron's comments and my replies have been copied to the new review
(10382149). This one is now (again) obsolete.