Add ExtensionManagement based ExternalLoader

The previous approach used pref_names::kInstallForceList directly, change it to an observer of ExtensionManagement class instead.

BUG=177351
TEST=ExtensionServiceTest,ExternalPolicyLoaderTest

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

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

chrome/browser/chromeos/extensions/device_local_account_external_policy_loader.cc

@@ -94,6 +94,8 @@ DeviceLocalAccountExternalPolicyLoader::
 void DeviceLocalAccountExternalPolicyLoader::UpdateExtensionListFromStore() {
   scoped_ptr<base::DictionaryValue> prefs(new base::DictionaryValue);
   const policy::PolicyMap& policy_map = store_->policy_map();
+  // TODO(binjin): Use two policy handlers here after
+  // ExtensionManagementPolicyHandler is introduced.
   extensions::ExtensionInstallForcelistPolicyHandler policy_handler;
   if (policy_handler.CheckPolicySettings(policy_map, NULL)) {
     PrefValueMap pref_value_map;

chrome/browser/chromeos/policy/device_local_account_extension_tracker.cc

@@ -55,6 +55,8 @@ void DeviceLocalAccountExtensionTracker::OnStoreError(CloudPolicyStore* store) {
 void DeviceLocalAccountExtensionTracker::UpdateFromStore() {
   const policy::PolicyMap& policy_map = store_->policy_map();
 
+  // TODO(binjin): Use two policy handlers here after
+  // ExtensionManagementPolicyHandler is introduced.
   extensions::ExtensionInstallForcelistPolicyHandler policy_handler;
   if (!policy_handler.CheckPolicySettings(policy_map, NULL))
     return;

chrome/browser/extensions/api/management/management_browsertest.cc

@@ -5,13 +5,14 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/memory/ref_counted.h"
-#include "base/prefs/pref_service.h"
 #include "base/prefs/scoped_user_pref_update.h"
+#include "base/run_loop.h"
 #include "base/stl_util.h"
+#include "base/strings/stringprintf.h"
 #include "chrome/browser/extensions/extension_browsertest.h"
+#include "chrome/browser/extensions/extension_management.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_test_message_listener.h"
-#include "chrome/browser/extensions/external_policy_loader.h"
 #include "chrome/browser/extensions/updater/extension_downloader.h"
 #include "chrome/browser/extensions/updater/extension_updater.h"
 #include "chrome/browser/profiles/profile.h"
@@ -19,6 +20,9 @@
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/test/base/ui_test_utils.h"
+#include "components/policy/core/browser/browser_policy_connector.h"
+#include "components/policy/core/common/mock_configuration_policy_provider.h"
+#include "components/policy/core/common/policy_map.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/test/browser_test_utils.h"
@@ -28,15 +32,41 @@
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/notification_types.h"
-#include "extensions/browser/pref_names.h"
 #include "net/url_request/url_fetcher.h"
+#include "policy/policy_constants.h"
+#include "testing/gmock/include/gmock/gmock.h"
 
 using extensions::Extension;
 using extensions::ExtensionRegistry;
 using extensions::Manifest;
+using policy::PolicyMap;
+using testing::Return;
+using testing::_;
+
+namespace {
+
+std::string BuildForceInstallPolicyValue(const char* extension_id,
+                                         const char* update_url) {
+  return base::StringPrintf("%s;%s", extension_id, update_url);
+}
+
+}  // namespace
 
 class ExtensionManagementTest : public ExtensionBrowserTest {
+ public:
+  virtual void SetUpInProcessBrowserTestFixture() OVERRIDE {
+    EXPECT_CALL(policy_provider_, IsInitializationComplete(_))
+        .WillRepeatedly(Return(true));
+    policy::BrowserPolicyConnector::SetPolicyProviderForTesting(
+        &policy_provider_);
+  }
+
  protected:
+  void UpdateProviderPolicy(const PolicyMap& policy) {
+    policy_provider_.UpdateChromePolicy(policy);
+    base::RunLoop().RunUntilIdle();
+  }
+
   // Helper method that returns whether the extension is at the given version.
   // This calls version(), which must be defined in the extension's bg page,
   // as well as asking the extension itself.
@@ -71,6 +101,9 @@ class ExtensionManagementTest : public ExtensionBrowserTest {
       return false;
     return true;
   }
+
+ private:
+  policy::MockConfigurationPolicyProvider policy_provider_;
 };
 
 #if defined(OS_LINUX)
@@ -508,19 +541,22 @@ IN_PROC_BROWSER_TEST_F(ExtensionManagementTest, ExternalPolicyRefresh) {
   const size_t size_before = registry->enabled_extensions().size();
   ASSERT_TRUE(registry->disabled_extensions().is_empty());
 
-  PrefService* prefs = browser()->profile()->GetPrefs();
-  const base::DictionaryValue* forcelist =
-      prefs->GetDictionary(extensions::pref_names::kInstallForceList);
-  ASSERT_TRUE(forcelist->empty()) << kForceInstallNotEmptyHelp;
-
-  {
-    // Set the policy as a user preference and fire notification observers.
-    DictionaryPrefUpdate pref_update(prefs,
-                                     extensions::pref_names::kInstallForceList);
-    base::DictionaryValue* forcelist = pref_update.Get();
-    extensions::ExternalPolicyLoader::AddExtension(
-        forcelist, kExtensionId, "http://localhost/autoupdate/manifest");
-  }
+  ASSERT_TRUE(extensions::ExtensionManagementFactory::GetForBrowserContext(
+                  browser()->profile())
+                  ->GetForceInstallList()
+                  ->empty())
+      << kForceInstallNotEmptyHelp;
+
+  base::ListValue forcelist;
+  forcelist.AppendString(BuildForceInstallPolicyValue(
+      kExtensionId, "http://localhost/autoupdate/manifest"));
+  PolicyMap policies;
+  policies.Set(policy::key::kExtensionInstallForcelist,
+               policy::POLICY_LEVEL_MANDATORY,
+               policy::POLICY_SCOPE_USER,
+               forcelist.DeepCopy(),
+               NULL);
+  UpdateProviderPolicy(policies);
 
   // Check if the extension got installed.
   ASSERT_TRUE(WaitForExtensionInstall());
@@ -547,7 +583,8 @@ IN_PROC_BROWSER_TEST_F(ExtensionManagementTest, ExternalPolicyRefresh) {
   EXPECT_EQ(0u, registry->disabled_extensions().size());
 
   // Check that emptying the list triggers uninstall.
-  prefs->ClearPref(extensions::pref_names::kInstallForceList);
+  policies.Erase(policy::key::kExtensionInstallForcelist);
+  UpdateProviderPolicy(policies);
   EXPECT_EQ(size_before + 1, registry->enabled_extensions().size());
   EXPECT_FALSE(service->GetExtensionById(kExtensionId, true));
 }
@@ -582,10 +619,11 @@ IN_PROC_BROWSER_TEST_F(ExtensionManagementTest,
                                      basedir.AppendASCII("v2.crx"));
 
   // Check that the policy is initially empty.
-  PrefService* prefs = browser()->profile()->GetPrefs();
-  const base::DictionaryValue* forcelist =
-      prefs->GetDictionary(extensions::pref_names::kInstallForceList);
-  ASSERT_TRUE(forcelist->empty()) << kForceInstallNotEmptyHelp;
+  ASSERT_TRUE(extensions::ExtensionManagementFactory::GetForBrowserContext(
+                  browser()->profile())
+                  ->GetForceInstallList()
+                  ->empty())
+      << kForceInstallNotEmptyHelp;
 
   // User install of the extension.
   ASSERT_TRUE(InstallExtension(basedir.AppendASCII("v2.crx"), 1));
@@ -596,13 +634,17 @@ IN_PROC_BROWSER_TEST_F(ExtensionManagementTest,
   EXPECT_TRUE(service->IsExtensionEnabled(kExtensionId));
 
   // Setup the force install policy. It should override the location.
-  {
-    DictionaryPrefUpdate pref_update(prefs,
-                                     extensions::pref_names::kInstallForceList);
-    extensions::ExternalPolicyLoader::AddExtension(
-        pref_update.Get(), kExtensionId,
-        "http://localhost/autoupdate/manifest");
-  }
+  base::ListValue forcelist;
+  forcelist.AppendString(BuildForceInstallPolicyValue(
+      kExtensionId, "http://localhost/autoupdate/manifest"));
+  PolicyMap policies;
+  policies.Set(policy::key::kExtensionInstallForcelist,
+               policy::POLICY_LEVEL_MANDATORY,
+               policy::POLICY_SCOPE_USER,
+               forcelist.DeepCopy(),
+               NULL);
+  UpdateProviderPolicy(policies);
+
   ASSERT_TRUE(WaitForExtensionInstall());
   ASSERT_EQ(size_before + 1, registry->enabled_extensions().size());
   extension = service->GetExtensionById(kExtensionId, false);
@@ -614,7 +656,8 @@ IN_PROC_BROWSER_TEST_F(ExtensionManagementTest,
   // TODO(joaodasilva): it would be nicer if the extension was kept instead,
   // and reverted location to INTERNAL or whatever it was before the policy
   // was applied.
-  prefs->ClearPref(extensions::pref_names::kInstallForceList);
+  policies.Erase(policy::key::kExtensionInstallForcelist);
+  UpdateProviderPolicy(policies);
   ASSERT_EQ(size_before, registry->enabled_extensions().size());
   extension = service->GetExtensionById(kExtensionId, true);
   EXPECT_FALSE(extension);
@@ -636,13 +679,13 @@ IN_PROC_BROWSER_TEST_F(ExtensionManagementTest,
 
   // Install the policy again. It should overwrite the extension's location,
   // and force enable it too.
-  {
-    DictionaryPrefUpdate pref_update(prefs,
-                                     extensions::pref_names::kInstallForceList);
-    base::DictionaryValue* forcelist = pref_update.Get();
-    extensions::ExternalPolicyLoader::AddExtension(
-        forcelist, kExtensionId, "http://localhost/autoupdate/manifest");
-  }
+  policies.Set(policy::key::kExtensionInstallForcelist,
+               policy::POLICY_LEVEL_MANDATORY,
+               policy::POLICY_SCOPE_USER,
+               forcelist.DeepCopy(),
+               NULL);
+  UpdateProviderPolicy(policies);
+
   ASSERT_TRUE(WaitForExtensionInstall());
   ASSERT_EQ(size_before + 1, registry->enabled_extensions().size());
   extension = service->GetExtensionById(kExtensionId, false);

chrome/browser/extensions/extension_management.cc

@@ -8,6 +8,7 @@
 #include "base/bind_helpers.h"
 #include "base/logging.h"
 #include "base/prefs/pref_service.h"
+#include "chrome/browser/extensions/external_policy_loader.h"
 #include "chrome/browser/extensions/external_provider_impl.h"
 #include "chrome/browser/extensions/standard_management_policy_provider.h"
 #include "chrome/browser/profiles/incognito_helpers.h"
@@ -75,6 +76,24 @@ bool ExtensionManagement::BlacklistedByDefault() {
   return default_settings_.installation_mode == INSTALLATION_BLOCKED;
 }
 
+scoped_ptr<base::DictionaryValue> ExtensionManagement::GetForceInstallList()
+    const {
+  scoped_ptr<base::DictionaryValue> forcelist(new base::DictionaryValue());
+  for (SettingsIdMap::const_iterator it = settings_by_id_.begin();
+       it != settings_by_id_.end();
+       ++it) {
+    if (it->second.installation_mode == INSTALLATION_FORCED) {
+      ExternalPolicyLoader::AddExtension(
+          forcelist.get(), it->first, it->second.update_url);
+    }
+  }
+  return forcelist.Pass();
+}
+
+bool ExtensionManagement::IsInstallationAllowed(const ExtensionId& id) const {
+  return ReadById(id).installation_mode != INSTALLATION_BLOCKED;
+}
+
 const ExtensionManagement::IndividualSettings& ExtensionManagement::ReadById(
     const ExtensionId& id) const {
   DCHECK(crx_file::id_util::IdIsValid(id)) << "Invalid ID: " << id;

chrome/browser/extensions/extension_management.h

@@ -110,6 +110,13 @@ class ExtensionManagement : public KeyedService {
   // from the command line, or when loaded as an unpacked extension).
   bool BlacklistedByDefault();
 
+  // Returns the force install list, in format specified by
+  // ExternalPolicyLoader::AddExtension().
+  scoped_ptr<base::DictionaryValue> GetForceInstallList() const;
+
+  // Returns if an extension with id |id| is allowed to install or not.
+  bool IsInstallationAllowed(const ExtensionId& id) const;
+
   // Helper function to read |settings_by_id_| with |id| as key. Returns a
   // constant reference to default settings if |id| does not exist.
   const IndividualSettings& ReadById(const ExtensionId& id) const;

chrome/browser/extensions/external_policy_loader.cc

@@ -4,33 +4,23 @@
 
 #include "chrome/browser/extensions/external_policy_loader.h"
 
-#include "base/bind.h"
-#include "base/logging.h"
-#include "base/prefs/pref_service.h"
 #include "base/strings/stringprintf.h"
 #include "base/values.h"
-#include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/external_provider_impl.h"
-#include "chrome/browser/profiles/profile.h"
-#include "chrome/common/pref_names.h"
-#include "content/public/browser/notification_details.h"
-#include "content/public/browser/notification_source.h"
-#include "extensions/browser/pref_names.h"
 
 namespace extensions {
 
-ExternalPolicyLoader::ExternalPolicyLoader(Profile* profile)
-    : profile_(profile) {
-  pref_change_registrar_.Init(profile_->GetPrefs());
-  pref_change_registrar_.Add(pref_names::kInstallForceList,
-                             base::Bind(&ExternalPolicyLoader::StartLoading,
-                                        base::Unretained(this)));
-  pref_change_registrar_.Add(pref_names::kAllowedTypes,
-                             base::Bind(&ExternalPolicyLoader::StartLoading,
-                                        base::Unretained(this)));
-  notification_registrar_.Add(this,
-                              chrome::NOTIFICATION_PROFILE_DESTROYED,
-                              content::Source<Profile>(profile_));
+ExternalPolicyLoader::ExternalPolicyLoader(ExtensionManagement *settings)
+    : settings_(settings) {
+  settings_->AddObserver(this);
+}
+
+ExternalPolicyLoader::~ExternalPolicyLoader() {
+  settings_->RemoveObserver(this);
+}
+
+void ExternalPolicyLoader::OnExtensionManagementSettingsChanged() {
+  StartLoading();
 }
 
 // static
@@ -42,24 +32,8 @@ void ExternalPolicyLoader::AddExtension(base::DictionaryValue* dict,
                   update_url);
 }
 
-void ExternalPolicyLoader::Observe(
-    int type,
-    const content::NotificationSource& source,
-    const content::NotificationDetails& details) {
-  if (profile_ == NULL) return;
-  DCHECK(type == chrome::NOTIFICATION_PROFILE_DESTROYED) <<
-      "Unexpected notification type.";
-  if (content::Source<Profile>(source).ptr() == profile_) {
-    notification_registrar_.RemoveAll();
-    pref_change_registrar_.RemoveAll();
-    profile_ = NULL;
-  }
-}
-
 void ExternalPolicyLoader::StartLoading() {
-  const base::DictionaryValue* forcelist =
-      profile_->GetPrefs()->GetDictionary(pref_names::kInstallForceList);
-  prefs_.reset(forcelist ? forcelist->DeepCopy() : NULL);
+  prefs_ = settings_->GetForceInstallList();
   LoadFinished();
 }
 

chrome/browser/extensions/external_policy_loader.h

@@ -8,12 +8,8 @@
 #include <string>
 
 #include "base/compiler_specific.h"
-#include "base/prefs/pref_change_registrar.h"
+#include "chrome/browser/extensions/extension_management.h"
 #include "chrome/browser/extensions/external_loader.h"
-#include "content/public/browser/notification_observer.h"
-#include "content/public/browser/notification_registrar.h"
-
-class Profile;
 
 namespace base {
 class DictionaryValue;
@@ -21,19 +17,15 @@ class DictionaryValue;
 
 namespace extensions {
 
-// A specialization of the ExternalProvider that uses
-// pref_names::kInstallForceList to look up which external extensions are
-// registered.
-class ExternalPolicyLoader
-    : public ExternalLoader,
-      public content::NotificationObserver {
+// A specialization of the ExternalProvider that uses extension management
+// policies to look up which external extensions are registered.
+class ExternalPolicyLoader : public ExternalLoader,
+                             public ExtensionManagement::Observer {
  public:
-  explicit ExternalPolicyLoader(Profile* profile);
+  explicit ExternalPolicyLoader(ExtensionManagement* settings);
 
-  // content::NotificationObserver implementation
-  virtual void Observe(int type,
-                       const content::NotificationSource& source,
-                       const content::NotificationDetails& details) OVERRIDE;
+  // ExtensionManagement::Observer implementation
+  virtual void OnExtensionManagementSettingsChanged() OVERRIDE;
 
   // Adds an extension to be updated to the pref dictionary.
   static void AddExtension(base::DictionaryValue* dict,
@@ -46,12 +38,9 @@ class ExternalPolicyLoader
  private:
   friend class base::RefCountedThreadSafe<ExternalLoader>;
 
-  virtual ~ExternalPolicyLoader() {}
-
-  PrefChangeRegistrar pref_change_registrar_;
-  content::NotificationRegistrar notification_registrar_;
+  virtual ~ExternalPolicyLoader();
 
-  Profile* profile_;
+  ExtensionManagement* settings_;
 
   DISALLOW_COPY_AND_ASSIGN(ExternalPolicyLoader);
 };

chrome/browser/extensions/external_policy_loader_unittest.cc

@@ -9,6 +9,7 @@
 #include "base/message_loop/message_loop.h"
 #include "base/values.h"
 #include "base/version.h"
+#include "chrome/browser/extensions/extension_management.h"
 #include "chrome/browser/extensions/external_policy_loader.h"
 #include "chrome/browser/extensions/external_provider_impl.h"
 #include "chrome/common/pref_names.h"
@@ -54,7 +55,8 @@ class MockExternalPolicyProviderVisitor
         pref_names::kInstallForceList, policy_forcelist.DeepCopy());
     provider_.reset(new ExternalProviderImpl(
         this,
-        new ExternalPolicyLoader(profile_.get()),
+        new ExternalPolicyLoader(
+            ExtensionManagementFactory::GetForBrowserContext(profile_.get())),
         profile_.get(),
         Manifest::INVALID_LOCATION,
         Manifest::EXTERNAL_POLICY_DOWNLOAD,

chrome/browser/extensions/external_provider_impl.cc

@@ -18,6 +18,7 @@
 #include "base/version.h"
 #include "chrome/browser/app_mode/app_mode_utils.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/extensions/extension_management.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/external_component_loader.h"
 #include "chrome/browser/extensions/external_policy_loader.h"
@@ -387,10 +388,12 @@ void ExternalProviderImpl::CreateExternalProviders(
       NOTREACHED();
     }
   } else {
-    external_loader = new ExternalPolicyLoader(profile);
+    external_loader = new ExternalPolicyLoader(
+        ExtensionManagementFactory::GetForBrowserContext(profile));
   }
 #else
-  external_loader = new ExternalPolicyLoader(profile);
+  external_loader = new ExternalPolicyLoader(
+      ExtensionManagementFactory::GetForBrowserContext(profile));
 #endif
 
   // Policies are mandatory so they can't be skipped with command line flag.

chrome/browser/extensions/install_verifier.cc

@@ -13,6 +13,7 @@
 #include "base/metrics/histogram.h"
 #include "base/prefs/pref_service.h"
 #include "base/stl_util.h"
+#include "chrome/browser/extensions/extension_management.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/install_signer.h"
 #include "chrome/common/chrome_switches.h"
@@ -322,21 +323,8 @@ void InstallVerifier::RemoveMany(const ExtensionIdSet& ids) {
 }
 
 bool InstallVerifier::AllowedByEnterprisePolicy(const std::string& id) const {
-  PrefService* pref_service = prefs_->pref_service();
-  if (pref_service->IsManagedPreference(pref_names::kInstallAllowList)) {
-    const base::ListValue* whitelist =
-        pref_service->GetList(pref_names::kInstallAllowList);
-    base::StringValue id_value(id);
-    if (whitelist && whitelist->Find(id_value) != whitelist->end())
-      return true;
-  }
-  if (pref_service->IsManagedPreference(pref_names::kInstallForceList)) {
-    const base::DictionaryValue* forcelist =
-        pref_service->GetDictionary(pref_names::kInstallForceList);
-    if (forcelist && forcelist->HasKey(id))
-      return true;
-  }
-  return false;
+  return ExtensionManagementFactory::GetForBrowserContext(context_)
+      ->IsInstallationAllowed(id);
 }
 
 std::string InstallVerifier::GetDebugPolicyProviderName() const {