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

chrome/browser/extensions/extension_service.cc

Index: chrome/browser/extensions/extension_service.cc
===================================================================
--- chrome/browser/extensions/extension_service.cc	(revision 138227)
+++ chrome/browser/extensions/extension_service.cc	(working copy)
@@ -17,7 +17,6 @@
 #include "base/metrics/histogram.h"
 #include "base/path_service.h"
 #include "base/stl_util.h"
-#include "base/string16.h"
 #include "base/string_number_conversions.h"
 #include "base/string_util.h"
 #include "base/stringprintf.h"
@@ -105,9 +104,11 @@
 #include "content/public/browser/render_process_host.h"
 #include "content/public/common/pepper_plugin_info.h"
 #include "googleurl/src/gurl.h"
+#include "grit/generated_resources.h"
 #include "net/base/registry_controlled_domain.h"
 #include "sync/api/sync_change.h"
 #include "sync/api/sync_error_factory.h"
+#include "ui/base/l10n/l10n_util.h"
 #include "webkit/database/database_tracker.h"
 #include "webkit/database/database_util.h"
 
@@ -304,7 +305,7 @@
 
   // The following call to UninstallExtension will not allow an uninstall of a
   // policy-controlled extension.
-  std::string error;
+  string16 error;
   if (!extensions_service->UninstallExtension(extension_id, false, &error)) {
     LOG(WARNING) << "Cannot uninstall extension with id " << extension_id
                  << ": " << error;
@@ -687,7 +688,7 @@
 bool ExtensionService::UninstallExtension(
     std::string extension_id,
     bool external_uninstall,
-    std::string* error) {
+    string16* error) {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   scoped_refptr<const Extension> extension(GetInstalledExtension(extension_id));
@@ -698,15 +699,13 @@
   // Policy change which triggers an uninstall will always set
   // |external_uninstall| to true so this is the only way to uninstall
   // managed extensions.
-  if (!Extension::UserMayDisable(extension->location()) &&
-      !external_uninstall) {
+  if (!external_uninstall &&
+      !system_->management_policy()->UserMayModifyStatus(
+        extension.get(), error)) {
     content::NotificationService::current()->Notify(
         chrome::NOTIFICATION_EXTENSION_UNINSTALL_NOT_ALLOWED,
         content::Source<Profile>(profile_),
         content::Details<const Extension>(extension));
-    if (error != NULL) {
-      *error = errors::kCannotUninstallManagedExtension;
-    }
     return false;
   }
 
@@ -817,6 +816,37 @@
   return extension_prefs_->IsExternalExtensionUninstalled(extension_id);
 }
 
+std::string ExtensionService::PolicyProviderName() const {
+  return "admin policy forcelist, via the ExtensionService";
+}
+
+bool ExtensionService::UserMayModifyStatus(const Extension* extension,
+                                           string16* error) const {
+  return ManagementPolicyImpl(extension, error, true);
+}
+
+bool ExtensionService::MustRemainEnabled(const Extension* extension,
+                                         string16* error) const {
+  return ManagementPolicyImpl(extension, error, false);
+}
+
+bool ExtensionService::ManagementPolicyImpl(const Extension* extension,
+    string16* error, bool modifiable_value) const {
+  // An extension that is required (by admin policy, for example) cannot be
+  // modified.
+  bool modifiable = !Extension::IsRequired(extension->location());

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

It seems like if ManagementPolicy did this check itself, this implementation of
Provider would not be necessary.

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

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

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

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.

+  // Some callers equate "no restriction" to true, others to false.
+  if (modifiable)
+    return modifiable_value;
+
+  if (error) {
+    *error = l10n_util::GetStringFUTF16(
+        IDS_EXTENSION_CANT_MODIFY_POLICY_REQUIRED,
+        UTF8ToUTF16(extension->name()));
+  }
+  return !modifiable_value;
+}
+
 void ExtensionService::EnableExtension(const std::string& extension_id) {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
@@ -857,8 +887,10 @@
   const Extension* extension = GetInstalledExtension(extension_id);
   // |extension| can be NULL if sync disables an extension that is not
   // installed yet.
-  if (extension && !Extension::UserMayDisable(extension->location()))
+  if (extension &&
+      !system_->management_policy()->UserMayModifyStatus(extension, NULL)) {
     return;
+  }
 
   extension_prefs_->SetExtensionState(extension_id, Extension::DISABLED);
   extension_prefs_->SetDisableReason(extension_id, disable_reason);
@@ -1163,8 +1195,9 @@
   for (ExtensionSet::const_iterator iter = extensions_.begin();
        iter != extensions_.end(); ++iter) {
     const Extension* extension = (*iter);
-    if (!extension_prefs_->IsExtensionAllowedByPolicy(extension->id(),
-                                                      extension->location())) {
+    if (!system_->management_policy()->UserMayLoad(extension,
+                                                   extension->location(),
+                                                   NULL)) {
       to_be_removed.push_back(extension->id());
     }
   }
@@ -2106,7 +2139,7 @@
   // installation disabled the extension, make sure it is now enabled.
   bool initial_enable =
       !extension_prefs_->IsExtensionDisabled(id) ||
-      !Extension::UserMayDisable(extension->location());
+      system_->management_policy()->MustRemainEnabled(extension, NULL);
   PendingExtensionInfo pending_extension_info;
   if (pending_extension_manager()->GetById(id, &pending_extension_info)) {
     pending_extension_manager()->Remove(id);