Revert of Plumbing for login apps device policy to extensions. (patch…

…set #6 id:100001 of https://codereview.chromium.org/2306143002/ ) Reason for revert: After this change, Chrome keeps crashing on the login screen (reproduced on a minnie device). Original issue's description: > Plumbing for login apps device policy to extensions. > > This CL succeeeds the CL #2150483004 > > ChromeProcessManagerDelegate: > * Enable background pages for apps in the login profile when > command-line switch --enable-login-apps is set. > > ExtensionManagement: > * Factor out common code into GetInstallListByMode from > GetForceInstallList and GetRecommendedInstallList. > * Common function UpdateForcedExtensions. > > ExtensionSystemImpl: > * Enable extensions in login profile when command-line switch > --enable-login-apps is set. > > ExternalProviderImpl: > * In CreateExternalProviders, create an ExternalPolicyLoader for login > apps in the login profile. > > ExtensionInstallListPolicyHandler: > * Common base class to parse login and force extension install lists. > > * Switch --enable-login-apps to enable login apps. > * Extension pref extensions.install.loginlist. > > BUG=576464 > > Committed: https://crrev.com/3bbfabe266cd5138393d3e87229de567d19da005 > Cr-Commit-Position: refs/heads/master@{#432917} TBR=emaxx@chromium.org,asargent@chromium.org,bartfab@chromium.org,achuith@chromium.org,antrim@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=576464 Review URL: https://codereview.chromium.org/2525483004 . Cr-Commit-Position: refs/branch-heads/2924@{#64} Cr-Branched-From: 3a87aec-refs/heads/master@{#433059}
nwjs · Nov 22, 2016 · 3d841ee · 3d841ee
1 parent f89f2f4
commit 3d841ee
Show file tree

Hide file tree

Showing 13 changed files with 64 additions and 184 deletions.
diff --git a/chrome/browser/extensions/chrome_process_manager_delegate.cc b/chrome/browser/extensions/chrome_process_manager_delegate.cc
@@ -9,7 +9,6 @@
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
-#include "chrome/browser/extensions/extension_management.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/browser.h"
@@ -22,10 +21,6 @@
 #include "extensions/browser/process_manager_factory.h"
 #include "extensions/common/one_shot_event.h"
 
-#if defined(OS_CHROMEOS)
-#include "chrome/browser/chromeos/profiles/profile_helper.h"
-#endif
-
 namespace extensions {
 
 ChromeProcessManagerDelegate::ChromeProcessManagerDelegate() {
@@ -49,34 +44,16 @@ bool ChromeProcessManagerDelegate::IsBackgroundPageAllowed(
 
   bool is_normal_session = !profile->IsGuestSession() &&
                            !profile->IsSystemProfile();
-  bool allow_for_signin_profile = false;
-
 #if defined(OS_CHROMEOS)
-  const bool is_signin_profile =
-      chromeos::ProfileHelper::IsSigninProfile(profile);
-
-  if (is_signin_profile) {
-    const bool signin_apps_enabled =
-        base::CommandLine::ForCurrentProcess()->HasSwitch(
-            switches::kEnableSigninApps);
-    const bool signin_apps_installed =
-        !ExtensionManagementFactory::GetForBrowserContext(context)
-             ->GetForceInstallList()
-             ->empty();
-    allow_for_signin_profile = signin_apps_enabled && signin_apps_installed;
-  } else {
-    const bool user_logged_in =
-        user_manager::UserManager::Get()->IsUserLoggedIn();
-    is_normal_session &= user_logged_in;
-  }
+  is_normal_session = is_normal_session &&
+                      user_manager::UserManager::Get()->IsUserLoggedIn();
 #endif
 
   // Disallow if the current session is a Guest mode session or login screen but
   // the current browser context is *not* off-the-record. Such context is
   // artificial and background page shouldn't be created in it.
   // http://crbug.com/329498
-  return is_normal_session || profile->IsOffTheRecord() ||
-         allow_for_signin_profile;
+  return is_normal_session || profile->IsOffTheRecord();
 }
 
 bool ChromeProcessManagerDelegate::DeferCreatingStartupBackgroundHosts(

diff --git a/chrome/browser/extensions/extension_management.cc b/chrome/browser/extensions/extension_management.cc
@@ -51,8 +51,6 @@ ExtensionManagement::ExtensionManagement(PrefService* pref_service)
                              pref_change_callback);
   pref_change_registrar_.Add(pref_names::kInstallForceList,
                              pref_change_callback);
-  pref_change_registrar_.Add(pref_names::kInstallSigninList,
-                             pref_change_callback);
   pref_change_registrar_.Add(pref_names::kAllowedInstallSites,
                              pref_change_callback);
   pref_change_registrar_.Add(pref_names::kAllowedTypes, pref_change_callback);
@@ -112,27 +110,33 @@ ExtensionManagement::InstallationMode ExtensionManagement::GetInstallationMode(
 }
 
 std::unique_ptr<base::DictionaryValue>
-ExtensionManagement::GetInstallListByMode(
-    InstallationMode installation_mode) const {
-  std::unique_ptr<base::DictionaryValue> extension_dict(
-      new base::DictionaryValue);
-  for (const SettingsIdMap::value_type& it : settings_by_id_) {
-    if (it.second->installation_mode == installation_mode) {
-      ExternalPolicyLoader::AddExtension(extension_dict.get(), it.first,
-                                         it.second->update_url);
+ExtensionManagement::GetForceInstallList() const {
+  std::unique_ptr<base::DictionaryValue> install_list(
+      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(
+          install_list.get(), it->first, it->second->update_url);
     }
   }
-  return extension_dict;
-}
-
-std::unique_ptr<base::DictionaryValue>
-ExtensionManagement::GetForceInstallList() const {
-  return GetInstallListByMode(INSTALLATION_FORCED);
+  return install_list;
 }
 
 std::unique_ptr<base::DictionaryValue>
 ExtensionManagement::GetRecommendedInstallList() const {
-  return GetInstallListByMode(INSTALLATION_RECOMMENDED);
+  std::unique_ptr<base::DictionaryValue> install_list(
+      new base::DictionaryValue());
+  for (SettingsIdMap::const_iterator it = settings_by_id_.begin();
+       it != settings_by_id_.end();
+       ++it) {
+    if (it->second->installation_mode == INSTALLATION_RECOMMENDED) {
+      ExternalPolicyLoader::AddExtension(
+          install_list.get(), it->first, it->second->update_url);
+    }
+  }
+  return install_list;
 }
 
 bool ExtensionManagement::IsInstallationExplicitlyAllowed(
@@ -255,9 +259,6 @@ void ExtensionManagement::Refresh() {
   const base::DictionaryValue* forced_list_pref =
       static_cast<const base::DictionaryValue*>(LoadPreference(
           pref_names::kInstallForceList, true, base::Value::TYPE_DICTIONARY));
-  const base::DictionaryValue* signin_list_pref =
-      static_cast<const base::DictionaryValue*>(LoadPreference(
-          pref_names::kInstallSigninList, true, base::Value::TYPE_DICTIONARY));
   const base::ListValue* install_sources_pref =
       static_cast<const base::ListValue*>(LoadPreference(
           pref_names::kAllowedInstallSites, true, base::Value::TYPE_LIST));
@@ -318,8 +319,22 @@ void ExtensionManagement::Refresh() {
     }
   }
 
-  UpdateForcedExtensions(forced_list_pref);
-  UpdateForcedExtensions(signin_list_pref);
+  if (forced_list_pref) {
+    std::string update_url;
+    for (base::DictionaryValue::Iterator it(*forced_list_pref); !it.IsAtEnd();
+         it.Advance()) {
+      if (!crx_file::id_util::IdIsValid(it.key()))
+        continue;
+      const base::DictionaryValue* dict_value = NULL;
+      if (it.value().GetAsDictionary(&dict_value) &&
+          dict_value->GetStringWithoutPathExpansion(
+              ExternalProviderImpl::kExternalUpdateUrl, &update_url)) {
+        internal::IndividualSettings* by_id = AccessById(it.key());
+        by_id->installation_mode = INSTALLATION_FORCED;
+        by_id->update_url = update_url;
+      }
+    }
+  }
 
   if (install_sources_pref) {
     global_settings_->has_restricted_install_sources = true;
@@ -427,27 +442,6 @@ void ExtensionManagement::NotifyExtensionManagementPrefChanged() {
     observer.OnExtensionManagementSettingsChanged();
 }
 
-void ExtensionManagement::UpdateForcedExtensions(
-    const base::DictionaryValue* extension_dict) {
-  if (!extension_dict)
-    return;
-
-  std::string update_url;
-  for (base::DictionaryValue::Iterator it(*extension_dict); !it.IsAtEnd();
-       it.Advance()) {
-    if (!crx_file::id_util::IdIsValid(it.key()))
-      continue;
-    const base::DictionaryValue* dict_value = nullptr;
-    if (it.value().GetAsDictionary(&dict_value) &&
-        dict_value->GetStringWithoutPathExpansion(
-            ExternalProviderImpl::kExternalUpdateUrl, &update_url)) {
-      internal::IndividualSettings* by_id = AccessById(it.key());
-      by_id->installation_mode = INSTALLATION_FORCED;
-      by_id->update_url = update_url;
-    }
-  }
-}
-
 internal::IndividualSettings* ExtensionManagement::AccessById(
     const ExtensionId& id) {
   DCHECK(crx_file::id_util::IdIsValid(id)) << "Invalid ID: " << id;

diff --git a/chrome/browser/extensions/extension_management.h b/chrome/browser/extensions/extension_management.h
@@ -153,14 +153,6 @@ class ExtensionManagement : public KeyedService {
   void OnExtensionPrefChanged();
   void NotifyExtensionManagementPrefChanged();
 
-  // Helper to update |extension_dict| for forced installs.
-  void UpdateForcedExtensions(const base::DictionaryValue* extension_dict);
-
-  // Helper to return an extension install list, in format specified by
-  // ExternalPolicyLoader::AddExtension().
-  std::unique_ptr<base::DictionaryValue> GetInstallListByMode(
-      InstallationMode installation_mode) const;
-
   // Helper function to access |settings_by_id_| with |id| as key.
   // Adds a new IndividualSettings entry to |settings_by_id_| if none exists for
   // |id| yet.

diff --git a/chrome/browser/extensions/extension_system_impl.cc b/chrome/browser/extensions/extension_system_impl.cc
@@ -206,11 +206,6 @@ void ExtensionSystemImpl::Shared::Init(bool extensions_enabled) {
   bool autoupdate_enabled = !profile_->IsGuestSession() &&
                             !profile_->IsSystemProfile();
 #if defined(OS_CHROMEOS)
-  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kEnableSigninApps) &&
-      chromeos::ProfileHelper::IsSigninProfile(profile_)) {
-    extensions_enabled = true;
-  }
   if (!extensions_enabled)
     autoupdate_enabled = false;
 #endif  // defined(OS_CHROMEOS)

diff --git a/chrome/browser/extensions/external_provider_impl.cc b/chrome/browser/extensions/external_provider_impl.cc
@@ -497,21 +497,7 @@ void ExternalProviderImpl::CreateExternalProviders(
   scoped_refptr<ExternalLoader> external_loader;
   scoped_refptr<ExternalLoader> external_recommended_loader;
   extensions::Manifest::Location crx_location = Manifest::INVALID_LOCATION;
-
 #if defined(OS_CHROMEOS)
-  if (chromeos::ProfileHelper::IsSigninProfile(profile)) {
-    // Download apps installed by policy in the sign-in profile.
-    external_loader = new ExternalPolicyLoader(
-        ExtensionManagementFactory::GetForBrowserContext(profile),
-        ExternalPolicyLoader::FORCED);
-    provider_list->push_back(
-        std::unique_ptr<ExternalProviderInterface>(new ExternalProviderImpl(
-            service, external_loader, profile, crx_location,
-            Manifest::EXTERNAL_POLICY_DOWNLOAD,
-            Extension::FROM_WEBSTORE | Extension::WAS_INSTALLED_BY_DEFAULT)));
-    return;
-  }
-
   policy::BrowserPolicyConnectorChromeOS* connector =
       g_browser_process->platform_part()->browser_policy_connector_chromeos();
   bool is_chrome_os_public_session = false;

diff --git a/chrome/browser/extensions/policy_handlers.cc b/chrome/browser/extensions/policy_handlers.cc
@@ -99,43 +99,43 @@ bool ExtensionListPolicyHandler::CheckAndGetList(
   return true;
 }
 
-// ExtensionInstallListPolicyHandler implementation ----------------------------
+// ExtensionInstallForcelistPolicyHandler implementation -----------------------
 
-ExtensionInstallListPolicyHandler::ExtensionInstallListPolicyHandler(
-    const char* policy_name,
-    const char* pref_name)
-    : policy::TypeCheckingPolicyHandler(policy_name, base::Value::TYPE_LIST),
-      pref_name_(pref_name) {}
+ExtensionInstallForcelistPolicyHandler::ExtensionInstallForcelistPolicyHandler()
+    : policy::TypeCheckingPolicyHandler(policy::key::kExtensionInstallForcelist,
+                                        base::Value::TYPE_LIST) {}
 
-ExtensionInstallListPolicyHandler::~ExtensionInstallListPolicyHandler() {}
+ExtensionInstallForcelistPolicyHandler::
+    ~ExtensionInstallForcelistPolicyHandler() {}
 
-bool ExtensionInstallListPolicyHandler::CheckPolicySettings(
+bool ExtensionInstallForcelistPolicyHandler::CheckPolicySettings(
     const policy::PolicyMap& policies,
     policy::PolicyErrorMap* errors) {
   const base::Value* value;
   return CheckAndGetValue(policies, errors, &value) &&
-         ParseList(value, nullptr, errors);
+      ParseList(value, NULL, errors);
 }
 
-void ExtensionInstallListPolicyHandler::ApplyPolicySettings(
+void ExtensionInstallForcelistPolicyHandler::ApplyPolicySettings(
     const policy::PolicyMap& policies,
     PrefValueMap* prefs) {
-  const base::Value* value = nullptr;
+  const base::Value* value = NULL;
   std::unique_ptr<base::DictionaryValue> dict(new base::DictionaryValue());
-  if (CheckAndGetValue(policies, nullptr, &value) && value &&
-      ParseList(value, dict.get(), nullptr)) {
-    prefs->SetValue(pref_name(), std::move(dict));
+  if (CheckAndGetValue(policies, NULL, &value) &&
+      value &&
+      ParseList(value, dict.get(), NULL)) {
+    prefs->SetValue(pref_names::kInstallForceList, std::move(dict));
   }
 }
 
-bool ExtensionInstallListPolicyHandler::ParseList(
+bool ExtensionInstallForcelistPolicyHandler::ParseList(
     const base::Value* policy_value,
     base::DictionaryValue* extension_dict,
     policy::PolicyErrorMap* errors) {
   if (!policy_value)
     return true;
 
-  const base::ListValue* policy_list_value = nullptr;
+  const base::ListValue* policy_list_value = NULL;
   if (!policy_value->GetAsList(&policy_list_value)) {
     // This should have been caught in CheckPolicySettings.
     NOTREACHED();
@@ -167,8 +167,8 @@ bool ExtensionInstallListPolicyHandler::ParseList(
       continue;
     }
 
-    const std::string extension_id = entry_string.substr(0, pos);
-    const std::string update_url = entry_string.substr(pos + 1);
+    std::string extension_id = entry_string.substr(0, pos);
+    std::string update_url = entry_string.substr(pos+1);
     if (!crx_file::id_util::IdIsValid(extension_id) ||
         !GURL(update_url).is_valid()) {
       if (errors) {
@@ -180,34 +180,14 @@ bool ExtensionInstallListPolicyHandler::ParseList(
     }
 
     if (extension_dict) {
-      ExternalPolicyLoader::AddExtension(extension_dict, extension_id,
-                                         update_url);
+      extensions::ExternalPolicyLoader::AddExtension(
+          extension_dict, extension_id, update_url);
     }
   }
 
   return true;
 }
 
-// ExtensionInstallForcelistPolicyHandler implementation -----------------------
-
-ExtensionInstallForcelistPolicyHandler::ExtensionInstallForcelistPolicyHandler()
-    : ExtensionInstallListPolicyHandler(policy::key::kExtensionInstallForcelist,
-                                        pref_names::kInstallForceList) {}
-
-ExtensionInstallForcelistPolicyHandler::
-    ~ExtensionInstallForcelistPolicyHandler() {}
-
-// ExtensionInstallSigninlistPolicyHandler implementation
-// -----------------------
-
-ExtensionInstallSigninListPolicyHandler::
-    ExtensionInstallSigninListPolicyHandler()
-    : ExtensionInstallListPolicyHandler(policy::key::kLoginApps,
-                                        pref_names::kInstallSigninList) {}
-
-ExtensionInstallSigninListPolicyHandler::
-    ~ExtensionInstallSigninListPolicyHandler() {}
-
 // ExtensionURLPatternListPolicyHandler implementation -------------------------
 
 ExtensionURLPatternListPolicyHandler::ExtensionURLPatternListPolicyHandler(

diff --git a/chrome/browser/extensions/policy_handlers.h b/chrome/browser/extensions/policy_handlers.h
@@ -49,54 +49,27 @@ class ExtensionListPolicyHandler
   DISALLOW_COPY_AND_ASSIGN(ExtensionListPolicyHandler);
 };
 
-class ExtensionInstallListPolicyHandler
+class ExtensionInstallForcelistPolicyHandler
     : public policy::TypeCheckingPolicyHandler {
  public:
+  ExtensionInstallForcelistPolicyHandler();
+  ~ExtensionInstallForcelistPolicyHandler() override;
+
   // ConfigurationPolicyHandler methods:
   bool CheckPolicySettings(const policy::PolicyMap& policies,
                            policy::PolicyErrorMap* errors) override;
   void ApplyPolicySettings(const policy::PolicyMap& policies,
                            PrefValueMap* prefs) override;
 
- protected:
-  ExtensionInstallListPolicyHandler(const char* policy_name,
-                                    const char* pref_name);
-
-  ~ExtensionInstallListPolicyHandler() override;
-
-  const char* pref_name() const { return pref_name_; }
-
  private:
   // Parses the data in |policy_value| and writes them to |extension_dict|.
   bool ParseList(const base::Value* policy_value,
                  base::DictionaryValue* extension_dict,
                  policy::PolicyErrorMap* errors);
 
-  const char* const pref_name_;
-
-  DISALLOW_COPY_AND_ASSIGN(ExtensionInstallListPolicyHandler);
-};
-
-class ExtensionInstallForcelistPolicyHandler
-    : public ExtensionInstallListPolicyHandler {
- public:
-  ExtensionInstallForcelistPolicyHandler();
-  ~ExtensionInstallForcelistPolicyHandler() override;
-
- private:
   DISALLOW_COPY_AND_ASSIGN(ExtensionInstallForcelistPolicyHandler);
 };
 
-class ExtensionInstallSigninListPolicyHandler
-    : public ExtensionInstallListPolicyHandler {
- public:
-  ExtensionInstallSigninListPolicyHandler();
-  ~ExtensionInstallSigninListPolicyHandler() override;
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(ExtensionInstallSigninListPolicyHandler);
-};
-
 // Implements additional checks for policies that are lists of extension
 // URLPatterns.
 class ExtensionURLPatternListPolicyHandler