Supervised users: Slightly relax restrictions around extensions.

Previously, SupervisedUserService implemented extensions::ManagementPolicy::Provider::UserMayModifySettings, preventing any modifications to extensions (including e.g. disabling on permissions increase). This CL switches to MustRemainInstalled, to prevent uninstallation of custodian-installed extensions, and disables all UI that would allow re-enabling.

As a side-effect, this fixes crbug.com/458082 :)

BUG=397951,458082, 461261

Review URL: https://codereview.chromium.org/925563002

Cr-Commit-Position: refs/heads/master@{#318037}

3 files changed, 112 insertions, 118 deletions

diff --git a/chrome/browser/supervised_user/supervised_user_service.cc b/chrome/browser/supervised_user/supervised_user_service.cc
index 316af32..e464400 100644
--- a/chrome/browser/supervised_user/supervised_user_service.cc
+++ b/chrome/browser/supervised_user/supervised_user_service.cc
@@ -78,6 +78,42 @@ const char* const kCustodianInfoPrefs[] = {
   prefs::kSupervisedUserSecondCustodianProfileURL,
 };
 
+#if defined(ENABLE_EXTENSIONS)
+enum ExtensionState {
+  EXTENSION_FORCED,
+  EXTENSION_BLOCKED,
+  EXTENSION_ALLOWED
+};
+
+ExtensionState GetExtensionState(const extensions::Extension* extension) {
+  if (extension->is_theme())
+    return EXTENSION_ALLOWED;
+
+  bool was_installed_by_default = extension->was_installed_by_default();
+  bool was_installed_by_custodian = extension->was_installed_by_custodian();
+#if defined(OS_CHROMEOS)
+  // On Chrome OS all external sources are controlled by us so it means that
+  // they are "default". Method was_installed_by_default returns false because
+  // extensions creation flags are ignored in case of default extensions with
+  // update URL(the flags aren't passed to OnExternalExtensionUpdateUrlFound).
+  // TODO(dpolukhin): remove this Chrome OS specific code as soon as creation
+  // flags are not ignored.
+  was_installed_by_default =
+      extensions::Manifest::IsExternalLocation(extension->location());
+#endif
+  if (extensions::Manifest::IsComponentLocation(extension->location()) ||
+      was_installed_by_default ||
+      was_installed_by_custodian) {
+    // Enforce default extensions as well as custodian-installed extensions
+    // (if we'd allow the supervised user to uninstall them, there'd be no way
+    // to get them back).
+    return EXTENSION_FORCED;
+  }
+
+  return EXTENSION_BLOCKED;
+}
+#endif
+
 }  // namespace
 
 base::FilePath SupervisedUserService::Delegate::GetBlacklistPath() const {
@@ -344,54 +380,6 @@ void SupervisedUserService::AddPermissionRequestCreator(
   permissions_creators_.push_back(creator.release());
 }
 
-#if defined(ENABLE_EXTENSIONS)
-std::string SupervisedUserService::GetDebugPolicyProviderName() const {
-  // Save the string space in official builds.
-#ifdef NDEBUG
-  NOTREACHED();
-  return std::string();
-#else
-  return "Supervised User Service";
-#endif
-}
-
-bool SupervisedUserService::UserMayLoad(const extensions::Extension* extension,
-                                        base::string16* error) const {
-  base::string16 tmp_error;
-  if (ExtensionManagementPolicyImpl(extension, &tmp_error))
-    return true;
-
-  bool was_installed_by_default = extension->was_installed_by_default();
-  bool was_installed_by_custodian = extension->was_installed_by_custodian();
-#if defined(OS_CHROMEOS)
-  // On Chrome OS all external sources are controlled by us so it means that
-  // they are "default". Method was_installed_by_default returns false because
-  // extensions creation flags are ignored in case of default extensions with
-  // update URL(the flags aren't passed to OnExternalExtensionUpdateUrlFound).
-  // TODO(dpolukhin): remove this Chrome OS specific code as soon as creation
-  // flags are not ignored.
-  was_installed_by_default =
-      extensions::Manifest::IsExternalLocation(extension->location());
-#endif
-  if (extensions::Manifest::IsComponentLocation(extension->location()) ||
-      was_installed_by_default ||
-      was_installed_by_custodian) {
-    return true;
-  }
-
-  if (error)
-    *error = tmp_error;
-  return false;
-}
-
-bool SupervisedUserService::UserMayModifySettings(
-    const extensions::Extension* extension,
-    base::string16* error) const {
-  return ExtensionManagementPolicyImpl(extension, error);
-}
-
-#endif  // defined(ENABLE_EXTENSIONS)
-
 syncer::ModelTypeSet SupervisedUserService::GetPreferredDataTypes() const {
   if (!ProfileIsSupervised())
     return syncer::ModelTypeSet();
@@ -481,16 +469,38 @@ void SupervisedUserService::FinishSetupSync() {
 }
 
 #if defined(ENABLE_EXTENSIONS)
-bool SupervisedUserService::ExtensionManagementPolicyImpl(
+std::string SupervisedUserService::GetDebugPolicyProviderName() const {
+  // Save the string space in official builds.
+#ifdef NDEBUG
+  NOTREACHED();
+  return std::string();
+#else
+  return "Supervised User Service";
+#endif
+}
+
+bool SupervisedUserService::UserMayLoad(const extensions::Extension* extension,
+                                        base::string16* error) const {
+  DCHECK(ProfileIsSupervised());
+  ExtensionState result = GetExtensionState(extension);
+  bool may_load = (result != EXTENSION_BLOCKED);
+  if (!may_load && error)
+    *error = l10n_util::GetStringUTF16(IDS_EXTENSIONS_LOCKED_SUPERVISED_USER);
+  return may_load;
+}
+
+// Note: Having MustRemainInstalled always say "true" for custodian-installed
+// extensions does NOT prevent remote uninstalls (which is a bit unexpected, but
+// exactly what we want).
+bool SupervisedUserService::MustRemainInstalled(
     const extensions::Extension* extension,
     base::string16* error) const {
-  // |extension| can be NULL in unit_tests.
-  if (!ProfileIsSupervised() || (extension && extension->is_theme()))
-    return true;
-
-  if (error)
+  DCHECK(ProfileIsSupervised());
+  ExtensionState result = GetExtensionState(extension);
+  bool may_not_uninstall = (result == EXTENSION_FORCED);
+  if (may_not_uninstall && error)
     *error = l10n_util::GetStringUTF16(IDS_EXTENSIONS_LOCKED_SUPERVISED_USER);
-  return false;
+  return may_not_uninstall;
 }
 
 void SupervisedUserService::SetExtensionsActive() {
diff --git a/chrome/browser/supervised_user/supervised_user_service.h b/chrome/browser/supervised_user/supervised_user_service.h
index 8b16b44..e4b77e1 100644
--- a/chrome/browser/supervised_user/supervised_user_service.h
+++ b/chrome/browser/supervised_user/supervised_user_service.h
@@ -162,15 +162,6 @@ class SupervisedUserService : public KeyedService,
   void AddPermissionRequestCreator(
       scoped_ptr<PermissionRequestCreator> creator);
 
-#if defined(ENABLE_EXTENSIONS)
-  // extensions::ManagementPolicy::Provider implementation:
-  std::string GetDebugPolicyProviderName() const override;
-  bool UserMayLoad(const extensions::Extension* extension,
-                   base::string16* error) const override;
-  bool UserMayModifySettings(const extensions::Extension* extension,
-                             base::string16* error) const override;
-#endif
-
   // SyncTypePreferenceProvider implementation:
   syncer::ModelTypeSet GetPreferredDataTypes() const override;
 
@@ -192,6 +183,8 @@ class SupervisedUserService : public KeyedService,
                            ChangesIncludedSessionOnChangedSettings);
   FRIEND_TEST_ALL_PREFIXES(SupervisedUserServiceTest,
                            ChangesSyncSessionStateOnChangedSettings);
+  FRIEND_TEST_ALL_PREFIXES(SupervisedUserServiceExtensionTest,
+                           ExtensionManagementPolicyProvider);
 
   // A bridge from the UI thread to the SupervisedUserURLFilters, one of which
   // lives on the IO thread. This class mediates access to them and makes sure
@@ -259,11 +252,12 @@ class SupervisedUserService : public KeyedService,
   void OnCustodianInfoChanged();
 
 #if defined(ENABLE_EXTENSIONS)
-  // Internal implementation for ExtensionManagementPolicy::Delegate methods.
-  // If |error| is not NULL, it will be filled with an error message if the
-  // requested extension action (install, modify status, etc.) is not permitted.
-  bool ExtensionManagementPolicyImpl(const extensions::Extension* extension,
-                                     base::string16* error) const;
+  // extensions::ManagementPolicy::Provider implementation:
+  std::string GetDebugPolicyProviderName() const override;
+  bool UserMayLoad(const extensions::Extension* extension,
+                   base::string16* error) const override;
+  bool MustRemainInstalled(const extensions::Extension* extension,
+                           base::string16* error) const override;
 
   // Extensions helper to SetActive().
   void SetExtensionsActive();
diff --git a/chrome/browser/supervised_user/supervised_user_service_unittest.cc b/chrome/browser/supervised_user/supervised_user_service_unittest.cc
index 6730401..5e604a7 100644
--- a/chrome/browser/supervised_user/supervised_user_service_unittest.cc
+++ b/chrome/browser/supervised_user/supervised_user_service_unittest.cc
@@ -427,61 +427,51 @@ class SupervisedUserServiceExtensionTest
       : SupervisedUserServiceExtensionTestBase(true) {}
 };
 
-TEST_F(SupervisedUserServiceExtensionTestUnsupervised,
-       ExtensionManagementPolicyProvider) {
+TEST_F(SupervisedUserServiceExtensionTest, ExtensionManagementPolicyProvider) {
   SupervisedUserService* supervised_user_service =
       SupervisedUserServiceFactory::GetForProfile(profile_.get());
-  EXPECT_FALSE(profile_->IsSupervised());
+  ASSERT_TRUE(profile_->IsSupervised());
 
-  scoped_refptr<extensions::Extension> extension = MakeExtension(false);
-  base::string16 error_1;
-  EXPECT_TRUE(supervised_user_service->UserMayLoad(extension.get(), &error_1));
-  EXPECT_EQ(base::string16(), error_1);
+  // Check that a supervised user can install and uninstall a theme.
+  {
+    scoped_refptr<extensions::Extension> theme = MakeThemeExtension();
 
-  base::string16 error_2;
-  EXPECT_TRUE(
-      supervised_user_service->UserMayModifySettings(extension.get(),
-                                                     &error_2));
-  EXPECT_EQ(base::string16(), error_2);
-}
+    base::string16 error_1;
+    EXPECT_TRUE(supervised_user_service->UserMayLoad(theme.get(), &error_1));
+    EXPECT_TRUE(error_1.empty());
 
-TEST_F(SupervisedUserServiceExtensionTest, ExtensionManagementPolicyProvider) {
-  SupervisedUserService* supervised_user_service =
-      SupervisedUserServiceFactory::GetForProfile(profile_.get());
-  ASSERT_TRUE(profile_->IsSupervised());
+    base::string16 error_2;
+    EXPECT_FALSE(
+        supervised_user_service->MustRemainInstalled(theme.get(), &error_2));
+    EXPECT_TRUE(error_2.empty());
+  }
+
+  // Now check a different kind of extension; the supervised user should not be
+  // able to load it.
+  {
+    scoped_refptr<extensions::Extension> extension = MakeExtension(false);
+
+    base::string16 error;
+    EXPECT_FALSE(supervised_user_service->UserMayLoad(extension.get(), &error));
+    EXPECT_FALSE(error.empty());
+  }
 
-  // Check that a supervised user can install a theme.
-  scoped_refptr<extensions::Extension> theme = MakeThemeExtension();
-  base::string16 error_1;
-  EXPECT_TRUE(supervised_user_service->UserMayLoad(theme.get(), &error_1));
-  EXPECT_TRUE(error_1.empty());
-  EXPECT_TRUE(
-      supervised_user_service->UserMayModifySettings(theme.get(), &error_1));
-  EXPECT_TRUE(error_1.empty());
-
-  // Now check a different kind of extension.
-  scoped_refptr<extensions::Extension> extension = MakeExtension(false);
-  EXPECT_FALSE(supervised_user_service->UserMayLoad(extension.get(), &error_1));
-  EXPECT_FALSE(error_1.empty());
-
-  base::string16 error_2;
-  EXPECT_FALSE(supervised_user_service->UserMayModifySettings(extension.get(),
-                                                              &error_2));
-  EXPECT_FALSE(error_2.empty());
-
-  // Check that an extension that was installed by the custodian may be loaded.
-  base::string16 error_3;
-  scoped_refptr<extensions::Extension> extension_2 = MakeExtension(true);
-  EXPECT_TRUE(supervised_user_service->UserMayLoad(extension_2.get(),
-                                                   &error_3));
-  EXPECT_TRUE(error_3.empty());
-
-  // The supervised user should still not be able to uninstall or disable the
-  // extension.
-  base::string16 error_4;
-  EXPECT_FALSE(supervised_user_service->UserMayModifySettings(extension_2.get(),
-                                                              &error_4));
-  EXPECT_FALSE(error_4.empty());
+  {
+    // Check that a custodian-installed extension may be loaded, but not
+    // uninstalled.
+    scoped_refptr<extensions::Extension> extension = MakeExtension(true);
+
+    base::string16 error_1;
+    EXPECT_TRUE(
+        supervised_user_service->UserMayLoad(extension.get(), &error_1));
+    EXPECT_TRUE(error_1.empty());
+
+    base::string16 error_2;
+    EXPECT_TRUE(
+        supervised_user_service->MustRemainInstalled(extension.get(),
+                                                     &error_2));
+    EXPECT_FALSE(error_2.empty());
+  }
 
 #ifndef NDEBUG
   EXPECT_FALSE(supervised_user_service->GetDebugPolicyProviderName().empty());