[Extensions] (More) Gracefully handle previously-installed policy extensions

Consider the following circumstance:
- User installs extension Alpha normally (e.g., through the webstore).
  Alpha has Manifest::Location INTERNAL.
- Corp policy pushes out an update that lists Alpha as a required
  extension.

Currently, this behaves fantastically poorly. We validate that the user
is allowed to load an extension on each run of Chrome when loading
installed extensions, and UserMayLoad() will return false if the
extension is required by corporate policy (in order to prevent users
from installing "over" a policy-required extension). This means that
the policy-required extension gets disabled with reason
DISABLE_BLOCKED_BY_POLICY.

Make this slightly less bad by introducing a new policy provider method,
UserMayInstall(). This checks whether the user is allowed to install a
given extension, rather than whether the user is allowed to load it. For
default implementations, UserMayInstall() forwards to UserMayLoad()
(since they should be treated equivalently). However, we can now move
the check for if a Manifest::INTERNAL extension is required policy to
the UserMayInstall() check rather than UserMayLoad(). The effect of this
is to allow the user to load an already-installed extension that's
required by policy, but not policy-installed itself, while still
preventing the user from installing a new copy of that extension.

This is not a perfect solution, since the installed version of the
extension will still be the Manifest::INTERNAL one. This has a number of
implications, including that the extension won't show most policy-
installed indications and won't have access to policy-only APIs. In
extreme cases, the extensions can also be different versions. In the
future, we will need to think about how to handle this scenario more
gracefully.

Bug: 894184

Change-Id: I83c9c305b56f90ea211e4a9b0120b22d601b4fb4
Reviewed-on: https://chromium-review.googlesource.com/c/1327616
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608538}

chrome/browser/extensions/api/webstore_private/webstore_private_api.cc

@@ -281,8 +281,9 @@ void WebstorePrivateBeginInstallWithManifest3Function::OnWebstoreParseSuccess(
   // Check the management policy before the installation process begins.
   Profile* profile = chrome_details_.GetProfile();
   base::string16 policy_error;
-  bool allow = ExtensionSystem::Get(profile)->
-      management_policy()->UserMayLoad(dummy_extension_.get(), &policy_error);
+  bool allow =
+      ExtensionSystem::Get(profile)->management_policy()->UserMayInstall(
+          dummy_extension_.get(), &policy_error);
   if (!allow) {
     bool blocked_for_child = false;
 #if BUILDFLAG(ENABLE_SUPERVISED_USERS)

chrome/browser/extensions/extension_service_unittest.cc

@@ -7322,4 +7322,62 @@ TEST_F(ExtensionServiceTest, UninstallDisabledMigratedExtension) {
   EXPECT_FALSE(service()->GetInstalledExtension(cast_stable));
 }
 
+// Tests the case of a user installing a non-policy extension (e.g. through the
+// webstore), and that extension later becoming required by policy.
+// Regression test for https://crbug.com/894184.
+TEST_F(ExtensionServiceTest, UserInstalledExtensionThenRequiredByPolicy) {
+  InitializeEmptyExtensionServiceWithTestingPrefs();
+
+  // Install an extension as if the user did it.
+  base::FilePath path = data_dir().AppendASCII("good.crx");
+  const Extension* extension = InstallCRX(path, INSTALL_NEW);
+  ASSERT_TRUE(extension);
+  EXPECT_EQ(good_crx, extension->id());
+  EXPECT_EQ(Manifest::INTERNAL, extension->location());
+
+  std::string kVersionStr = "1.0.0.0";
+  EXPECT_EQ(kVersionStr, extension->VersionString());
+
+  {
+    ManagementPrefUpdater pref(profile_->GetTestingPrefService());
+    // Mark good.crx for force-installation.
+    pref.SetIndividualExtensionAutoInstalled(
+        good_crx, "http://example.com/update_url", true);
+  }
+
+  // Require good.crx by policy.
+  MockExternalProvider* provider =
+      AddMockExternalProvider(Manifest::EXTERNAL_POLICY_DOWNLOAD);
+  // TODO(devlin): Do we also need to check installing extensions with different
+  // versions?
+  provider->UpdateOrAddExtension(good_crx, kVersionStr,
+                                 data_dir().AppendASCII("good.crx"));
+  service()->CheckForExternalUpdates();
+
+  ExtensionManagement* management =
+      ExtensionManagementFactory::GetForBrowserContext(profile());
+  ExtensionManagement::InstallationMode installation_mode =
+      management->GetInstallationMode(extension);
+  EXPECT_EQ(ExtensionManagement::INSTALLATION_FORCED, installation_mode);
+
+  // Reload all extensions.
+  service()->ReloadExtensionsForTest();
+
+  extension = registry()->GetInstalledExtension(good_crx);
+  ASSERT_TRUE(extension);
+  ManagementPolicy* policy =
+      ExtensionSystem::Get(browser_context())->management_policy();
+  // The extension should still be installed, and should be required to
+  // remain installed.
+  EXPECT_TRUE(policy->MustRemainInstalled(extension, nullptr));
+  // TODO(devlin): This currently doesn't work, because the extension is still
+  // installed with Manifest::Location INTERNAL.
+  // EXPECT_FALSE(policy->UserMayModifySettings(extension, nullptr));
+
+  EXPECT_TRUE(registry()->enabled_extensions().GetByID(good_crx));
+  ExtensionPrefs* prefs = ExtensionPrefs::Get(profile());
+  EXPECT_EQ(disable_reason::DISABLE_NONE, prefs->GetDisableReasons(good_crx));
+  EXPECT_FALSE(prefs->IsExtensionDisabled(good_crx));
+}
+
 }  // namespace extensions

chrome/browser/extensions/standard_management_policy_provider.cc

@@ -92,15 +92,6 @@ bool StandardManagementPolicyProvider::UserMayLoad(
   if (extension->from_bookmark())
     return true;
 
-  ExtensionManagement::InstallationMode installation_mode =
-      settings_->GetInstallationMode(extension);
-
-  // Force-installed extensions cannot be overwritten manually.
-  if (!Manifest::IsPolicyLocation(extension->location()) &&
-      installation_mode == ExtensionManagement::INSTALLATION_FORCED) {
-    return ReturnLoadError(extension, error);
-  }
-
   // Check whether the extension type is allowed.
   //
   // If you get a compile error here saying that the type you added is not
@@ -127,12 +118,29 @@ bool StandardManagementPolicyProvider::UserMayLoad(
       NOTREACHED();
   }
 
+  ExtensionManagement::InstallationMode installation_mode =
+      settings_->GetInstallationMode(extension);
   if (installation_mode == ExtensionManagement::INSTALLATION_BLOCKED)
     return ReturnLoadError(extension, error);
 
   return true;
 }
 
+bool StandardManagementPolicyProvider::UserMayInstall(
+    const Extension* extension,
+    base::string16* error) const {
+  ExtensionManagement::InstallationMode installation_mode =
+      settings_->GetInstallationMode(extension);
+
+  // Force-installed extensions cannot be overwritten manually.
+  if (!Manifest::IsPolicyLocation(extension->location()) &&
+      installation_mode == ExtensionManagement::INSTALLATION_FORCED) {
+    return ReturnLoadError(extension, error);
+  }
+
+  return UserMayLoad(extension, error);
+}
+
 bool StandardManagementPolicyProvider::UserMayModifySettings(
     const Extension* extension,
     base::string16* error) const {

chrome/browser/extensions/standard_management_policy_provider.h

@@ -28,6 +28,8 @@ class StandardManagementPolicyProvider : public ManagementPolicy::Provider {
   std::string GetDebugPolicyProviderName() const override;
   bool UserMayLoad(const Extension* extension,
                    base::string16* error) const override;
+  bool UserMayInstall(const Extension* extension,
+                      base::string16* error) const override;
   bool UserMayModifySettings(const Extension* extension,
                              base::string16* error) const override;
   bool ExtensionMayModifySettings(const Extension* source_extension,

extensions/browser/management_policy.cc

@@ -29,6 +29,11 @@ bool ManagementPolicy::Provider::UserMayLoad(const Extension* extension,
   return true;
 }
 
+bool ManagementPolicy::Provider::UserMayInstall(const Extension* extension,
+                                                base::string16* error) const {
+  return UserMayLoad(extension, error);
+}
+
 bool ManagementPolicy::Provider::UserMayModifySettings(
     const Extension* extension, base::string16* error) const {
   return true;
@@ -80,6 +85,12 @@ bool ManagementPolicy::UserMayLoad(const Extension* extension,
       &Provider::UserMayLoad, "Installation", true, extension, error);
 }
 
+bool ManagementPolicy::UserMayInstall(const Extension* extension,
+                                      base::string16* error) const {
+  return ApplyToProviderList(&Provider::UserMayInstall, "Installation", true,
+                             extension, error);
+}
+
 bool ManagementPolicy::UserMayModifySettings(const Extension* extension,
                                              base::string16* error) const {
   return ApplyToProviderList(

extensions/browser/management_policy.h

@@ -61,6 +61,12 @@ class ManagementPolicy {
     virtual bool UserMayLoad(const Extension* extension,
                              base::string16* error) const;
 
+    // Returns false if the user should not be allowed to install the given
+    // |extension|. By default, this forwards to UserMayLoad() (since a user
+    // should not be able to install an extension they cannot load).
+    virtual bool UserMayInstall(const Extension* extension,
+                                base::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.).
@@ -119,6 +125,11 @@ class ManagementPolicy {
   // TODO(treib,pam): Misleading name; see comment in Provider. crbug.com/461747
   bool UserMayLoad(const Extension* extension, base::string16* error) const;
 
+  // Returns false if the user should not be allowed to install the given
+  // |extension|. By default, this forwards to UserMayLoad() (since a user
+  // should not be able to install an extension they cannot load).
+  bool UserMayInstall(const Extension* extension, base::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.

extensions/browser/policy_check.cc

@@ -18,7 +18,7 @@ PolicyCheck::~PolicyCheck() {}
 
 void PolicyCheck::Start(ResultCallback callback) {
   Errors errors;
-  if (!ExtensionSystem::Get(context_)->management_policy()->UserMayLoad(
+  if (!ExtensionSystem::Get(context_)->management_policy()->UserMayInstall(
           extension(), &error_)) {
     DCHECK(!error_.empty());
     errors.insert(DISALLOWED_BY_POLICY);