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

chrome/browser/extensions/management_policy.h

Index: chrome/browser/extensions/management_policy.h
===================================================================
--- chrome/browser/extensions/management_policy.h	(revision 0)
+++ chrome/browser/extensions/management_policy.h	(revision 0)
@@ -0,0 +1,114 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_EXTENSIONS_MANAGEMENT_POLICY_H_
+#define CHROME_BROWSER_EXTENSIONS_MANAGEMENT_POLICY_H_
+#pragma once
+
+#include <set>
+#include <string>
+
+#include "base/basictypes.h"
+#include "chrome/common/extensions/extension.h"
+
+namespace extensions {
+// This class registers providers that want to prohibit certain actions from

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

Add a blank line here.

+// being applied to extensions. It must be called, via the ExtensionService,
+// before allowing a user or a user-level mechanism to perform the respective
+// action. (That is, installing or otherwise modifying an extension in order
+// to conform to enterprise administrator policy must be exempted from these
+// checks.)
+//
+// This "policy" and its providers should not be confused with administrator
+// policy, although admin policy is one of the sources ("Providers") of
+// restrictions registered with and exposed by the ManagementPolicy.
+class ManagementPolicy {
+ public:
+  // Each mechanism that wishes to limit users' ability to control extensions,
+  // whether one individual extension or the whole system, should implement
+  // the methods of this Provider interface that it needs. In each case, if the
+  // provider does not need to control a certain action, that method does not
+  // need to be implemented.
+  //
+  // It is not guaranteed that a particular Provider's methods will be called
+  // each time a user tries to perform one of the controlled actions (the list
+  // of providers is short-circuited as soon as a decision is possible), so
+  // implementations of these methods must have no side effects.
+  //
+  // For all of the Provider methods below, if |error| is not NULL and the
+  // method imposes a restriction on the desired action, |error| may be set
+  // to an applicable error message, but this is not required.
+  class Provider {

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

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.

+   public:
+    Provider() {}
+    virtual ~Provider() {}
+
+    // A human-readable name for this provider, for use in debug messages.
+    virtual std::string PolicyProviderName() const = 0;
+
+    // Providers should return false if a user may not intall the |extension|
+    // from the given |location|. Note that the Extension may not be fully
+    // configured when this is called (e.g., extension->location() may not be

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

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?

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

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()?

[Aaron Boodman, 2012/05/24 08:11:23]

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.

+    // correctly set yet).
+    virtual bool UserMayLoad(const Extension* extension,
+                             Extension::Location location,
+                             string16* error) const;
+
+    // Providers should return false if a user may not enable, disable, or
+    // uninstall the |extension|, or change its usage options (incognito
+    // permission, file access, etc.).
+    virtual bool UserMayModifyStatus(const Extension* extension,

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

Sure, except it's not just settings anymore.

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

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

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

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.

[Aaron Boodman, 2012/05/24 08:11:23]

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.

+                                     string16* error) const;
+
+    // Providers should return true if the |extension| must always remain
+    // enabled. This is distinct from UserMayModifyStatus() in that the latter
+    // also prohibits enabling the extension if it is currently disabled.
+    // Providers implementing this method should also implement the others
+    // above, if they wish to completely lock in an extension.
+    virtual bool MustRemainEnabled(const Extension* extension,

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

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.

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

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

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

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?

+                                   string16* error) const;
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(Provider);
+  };
+
+  ManagementPolicy();
+  ~ManagementPolicy();
+
+  // Registers or unregisters a provider, causing it to be added to or removed
+  // from the list of providers queried. Ownership of the provider remains with
+  // the caller. Providers do not need to be unregistered on shutdown.
+  void RegisterProvider(Provider* provider);
+  void UnregisterProvider(Provider* provider);
+
+  // Returns true if the user is permitted to install the given extension. If
+  // not, |error| may be set to an appropriate message.
+  bool UserMayLoad(const Extension* extension, Extension::Location location,
+                   string16* error) const;
+
+  // Returns true if the user is permitted to enable, disable, or uninstall the
+  // given extension, or change the extension's usage options (incognito mode,
+  // file access, etc.). If not, |error| may be set to an appropriate message.
+  bool UserMayModifyStatus(const Extension* extension,
+                           string16* error) const;
+
+  // Returns true if the extension must remain enabled at all times (e.g. a
+  // compoment extension). In that case, |error| may be set to an appropriate
+  // message.
+  bool MustRemainEnabled(const Extension* extension,
+                         string16* error) const;
+
+  // For use in testing.
+  void UnregisterAllProviders();
+  int GetNumProviders() const;
+
+ private:
+  typedef std::set<Provider*> ProviderList;
+  ProviderList providers_;
+
+  DISALLOW_COPY_AND_ASSIGN(ManagementPolicy);
+};
+}  // namespace
+
+#endif  // CHROME_BROWSER_EXTENSIONS_MANAGEMENT_POLICY_H_
Property changes on: chrome\browser\extensions\management_policy.h
___________________________________________________________________
Added: svn:eol-style
   + LF