Supervised Users Initiated Installs v2

chrome/browser/supervised_user/supervised_user_service.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/supervised_user/supervised_user_service.h"
 
 #include <utility>
 
 #include "base/command_line.h"
+#include "base/feature_list.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/path_service.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/task_runner_util.h"
 #include "base/version.h"
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/component_updater/supervised_user_whitelist_installer.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_attributes_entry.h"
 #include "chrome/browser/profiles/profile_attributes_storage.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/browser/supervised_user/experimental/supervised_user_filtering_switches.h"
 #include "chrome/browser/supervised_user/permission_request_creator.h"
 #include "chrome/browser/supervised_user/supervised_user_constants.h"
+#include "chrome/browser/supervised_user/supervised_user_features.h"
 #include "chrome/browser/supervised_user/supervised_user_service_observer.h"
 #include "chrome/browser/supervised_user/supervised_user_settings_service.h"
 #include "chrome/browser/supervised_user/supervised_user_settings_service_factory.h"
 #include "chrome/browser/supervised_user/supervised_user_site_list.h"
 #include "chrome/browser/supervised_user/supervised_user_whitelist_service.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/browser_sync/browser/profile_sync_service.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_service.h"
 #include "components/signin/core/browser/profile_oauth2_token_service.h"
 #include "components/signin/core/browser/signin_manager.h"
 #include "components/signin/core/browser/signin_manager_base.h"
 #include "components/signin/core/common/signin_switches.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/user_metrics.h"
+#include "extensions/browser/extension_registry.h"

[Marc Treib, 2016/06/14 13:53:34]

This needs to go into the #ifdef ENABLE_EXTENSIONS below

[mamir, 2016/06/15 09:40:11]

Done.

 #include "ui/base/l10n/l10n_util.h"
 
 #if !defined(OS_ANDROID)
 #include "chrome/browser/supervised_user/legacy/custodian_profile_downloader_service.h"
 #include "chrome/browser/supervised_user/legacy/custodian_profile_downloader_service_factory.h"
 #include "chrome/browser/supervised_user/legacy/permission_request_creator_sync.h"
 #include "chrome/browser/supervised_user/legacy/supervised_user_pref_mapping_service.h"
 #include "chrome/browser/supervised_user/legacy/supervised_user_pref_mapping_service_factory.h"
 #include "chrome/browser/supervised_user/legacy/supervised_user_registration_utility.h"
 #include "chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service_factory.h"
 #endif
 
 #if defined(OS_CHROMEOS)
 #include "chrome/browser/chromeos/login/users/chrome_user_manager.h"
 #include "chrome/browser/chromeos/login/users/supervised_user_manager.h"
 #include "components/user_manager/user_manager.h"
 #endif
 
 #if defined(ENABLE_EXTENSIONS)
 #include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/extension_util.h"
+#include "chrome/browser/supervised_user/supervised_user_service_factory.h"

[Marc Treib, 2016/06/14 13:53:34]

This, OTOH, can go out into the regular includes. (Not feeling too strongly, but
IMO it's less confusing that way)

[mamir, 2016/06/15 09:40:11]

Done.

+#include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/extension_system.h"
 #endif
 
 #if defined(ENABLE_THEMES)
 #include "chrome/browser/themes/theme_service.h"
 #include "chrome/browser/themes/theme_service_factory.h"
 #endif
 
 using base::DictionaryValue;
 using base::UserMetricsAction;
 using content::BrowserThread;
+using extensions::Extension;
+using extensions::ExtensionPrefs;
+using extensions::ExtensionSystem;

[Marc Treib, 2016/06/14 13:53:34]

These also need to go into an #ifdef

[mamir, 2016/06/15 09:40:11]

Done.

 
 namespace {
 
 // The URL from which to download a host blacklist if no local one exists yet.
 const char kBlacklistURL[] =
     "https://www.gstatic.com/chrome/supervised_user/blacklist-20141001-1k.bin";
 // The filename under which we'll store the blacklist (in the user data dir).
 const char kBlacklistFilename[] = "su-blacklist.bin";
 
 const char* const kCustodianInfoPrefs[] = {
   prefs::kSupervisedUserCustodianName,
   prefs::kSupervisedUserCustodianEmail,
   prefs::kSupervisedUserCustodianProfileImageURL,
   prefs::kSupervisedUserCustodianProfileURL,
   prefs::kSupervisedUserSecondCustodianName,
   prefs::kSupervisedUserSecondCustodianEmail,
   prefs::kSupervisedUserSecondCustodianProfileImageURL,
   prefs::kSupervisedUserSecondCustodianProfileURL,
 };
 
 void CreateURLAccessRequest(
     const GURL& url,
     PermissionRequestCreator* creator,
     const SupervisedUserService::SuccessCallback& callback) {
   creator->CreateURLAccessRequest(url, callback);
 }
 
+void CreateExtensionInstallRequest(
+    const std::string& id,
+    PermissionRequestCreator* creator,
+    const SupervisedUserService::SuccessCallback& callback) {
+  creator->CreateExtensionInstallRequest(id, callback);
+}
+
 void CreateExtensionUpdateRequest(
     const std::string& id,
     PermissionRequestCreator* creator,
     const SupervisedUserService::SuccessCallback& callback) {
   creator->CreateExtensionUpdateRequest(id, callback);
 }
 
+// Default callback for AddExtensionInstallRequest.
+void ExtensionInstallRequestSent(const std::string& id, bool success) {
+  VLOG_IF(1, !success) << "Failed sending install request for " << id;
+}
+
 // Default callback for AddExtensionUpdateRequest.
 void ExtensionUpdateRequestSent(const std::string& id, bool success) {
   VLOG_IF(1, !success) << "Failed sending update request for " << id;
 }
 
 base::FilePath GetBlacklistPath() {
   base::FilePath blacklist_dir;
   PathService::Get(chrome::DIR_USER_DATA, &blacklist_dir);
   return blacklist_dir.AppendASCII(kBlacklistFilename);
 }
-
-#if defined(ENABLE_EXTENSIONS)
-enum ExtensionState {
-  EXTENSION_FORCED,
-  EXTENSION_BLOCKED,
-  EXTENSION_ALLOWED
-};
-
-ExtensionState GetExtensionState(const extensions::Extension* extension) {
-  bool was_installed_by_default = extension->was_installed_by_default();
-#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
-  // Note: Component extensions are protected from modification/uninstallation
-  // anyway, so there's no need to enforce them again for supervised users.
-  // Also, leave policy-installed extensions alone - they have their own
-  // management; in particular we don't want to override the force-install list.
-  if (extensions::Manifest::IsComponentLocation(extension->location()) ||
-      extensions::Manifest::IsPolicyLocation(extension->location()) ||
-      extension->is_theme() ||
-      extension->from_bookmark() ||
-      extension->is_shared_module() ||
-      was_installed_by_default) {
-    return EXTENSION_ALLOWED;
-  }
-
-  if (extension->was_installed_by_custodian())
-    return EXTENSION_FORCED;
-
-  return EXTENSION_BLOCKED;
-}
-#endif
-
 }  // namespace
 
 SupervisedUserService::~SupervisedUserService() {
   DCHECK(!did_init_ || did_shutdown_);
   url_filter_context_.ui_url_filter()->RemoveObserver(this);
 }
 
 // static
 void SupervisedUserService::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
+  registry->RegisterDictionaryPref(prefs::kSupervisedUserApprovedExtensions);
   registry->RegisterDictionaryPref(prefs::kSupervisedUserManualHosts);
   registry->RegisterDictionaryPref(prefs::kSupervisedUserManualURLs);
   registry->RegisterIntegerPref(prefs::kDefaultSupervisedUserFilteringBehavior,
                                 SupervisedUserURLFilter::ALLOW);
   registry->RegisterBooleanPref(prefs::kSupervisedUserCreationAllowed, true);
   registry->RegisterBooleanPref(prefs::kSupervisedUserSafeSites, true);
   for (const char* pref : kCustodianInfoPrefs) {
     registry->RegisterStringPref(pref, std::string());
   }
 }
@@ skipping 70 matching lines @@
 }
 
 void SupervisedUserService::ReportURL(const GURL& url,
                                       const SuccessCallback& callback) {
   if (url_reporter_)
     url_reporter_->ReportUrl(url, callback);
   else
     callback.Run(false);
 }
 
+void SupervisedUserService::AddExtensionInstallRequest(
+    const std::string& extension_id,
+    const base::Version& version,
+    const SuccessCallback& callback) {
+  std::string id = GetExtensionRequestId(extension_id, version);
+  AddPermissionRequestInternal(base::Bind(CreateExtensionInstallRequest, id),
+                               callback, 0);
+}
+
+void SupervisedUserService::AddExtensionInstallRequest(
+    const std::string& extension_id,
+    const base::Version& version) {
+  std::string id = GetExtensionRequestId(extension_id, version);
+  AddPermissionRequestInternal(base::Bind(CreateExtensionInstallRequest, id),
+                               base::Bind(ExtensionInstallRequestSent, id), 0);
+}
+
 void SupervisedUserService::AddExtensionUpdateRequest(
     const std::string& extension_id,
     const base::Version& version,
     const SuccessCallback& callback) {
-  std::string id = GetExtensionUpdateRequestId(extension_id, version);
+  std::string id = GetExtensionRequestId(extension_id, version);
   AddPermissionRequestInternal(
       base::Bind(CreateExtensionUpdateRequest, id), callback, 0);
 }
 
 void SupervisedUserService::AddExtensionUpdateRequest(
     const std::string& extension_id,
     const base::Version& version) {
-  std::string id = GetExtensionUpdateRequestId(extension_id, version);
+  std::string id = GetExtensionRequestId(extension_id, version);
   AddExtensionUpdateRequest(extension_id, version,
                             base::Bind(ExtensionUpdateRequestSent, id));
 }
 
+void SupervisedUserService::EnableExtensionIfPossible(
+    const std::string& extension_id) {
+  extensions::ExtensionRegistry* registry_ =
+      extensions::ExtensionRegistry::Get(profile_);
+  const Extension* extension = registry_->GetInstalledExtension(extension_id);

[Marc Treib, 2016/06/14 13:53:34]

Can extension be null? Seems like it could be, if we get an approval before it's
actually installed/loaded?
(If that's correct, then our test coverage is clearly not where it should be yet
;P)

[mamir, 2016/06/15 09:40:11]

Fixed and added a proper unit test ;-)
Done!

+
+  ExtensionState state = GetExtensionState(*extension);
+
+  if (state == ExtensionState::BLOCKED ||
+      state == ExtensionState::REQUIRE_APPROVAL) {
+    return;  // Extension must remain disabled.
+  }
+
+  ExtensionPrefs* extension_prefs = ExtensionPrefs::Get(profile_);
+  if (!extension_prefs->HasDisableReason(
+          extension_id, Extension::DISABLE_CUSTODIAN_APPROVAL_REQUIRED)) {
+    return;  // Extension is disabled for other reasons.
+  }
+  extension_prefs->RemoveDisableReason(
+      extension_id, Extension::DISABLE_CUSTODIAN_APPROVAL_REQUIRED);
+  // If no other disable reasons, enable it.
+  if (extension_prefs->GetDisableReasons(extension_id)
+      == Extension::DISABLE_NONE) {

[Marc Treib, 2016/06/14 13:53:34]

Is this correctly aligned?

[mamir, 2016/06/15 09:40:11]

Done.

[Marc Treib, 2016/06/15 12:31:07]

git cl formatted now? Still looks weird, but oh well.

[mamir, 2016/06/15 17:30:02]

Yup :-)

+    // Try to enable the extension, this will call the ManagmentPolicy and
+    // properly enable the extension if possible.

[Marc Treib, 2016/06/14 13:53:34]

This comment seems out of place now - we've already checked that our
ManagementPolicy implementation doesn't want to keep it disabled.

[mamir, 2016/06/15 09:40:11]

The comment is intended to refer to other ManagementPolicies.
Still seems out of place?

[Marc Treib, 2016/06/15 12:31:07]

Yes, because it's not really relevant - we don't expect any interference from
other policies. Of course it could still happen, but that's just a property of
EnableExtension. I don't see a reason to specifically call it out here.

[mamir, 2016/06/15 17:30:02]

Done.

+    ExtensionService* service =
+        ExtensionSystem::Get(profile_)->extension_service();
+    service->EnableExtension(extension_id);
+  }
+}
+
 // static
-std::string SupervisedUserService::GetExtensionUpdateRequestId(
+std::string SupervisedUserService::GetExtensionRequestId(
     const std::string& extension_id,
     const base::Version& version) {
   return base::StringPrintf("%s:%s", extension_id.c_str(),
                             version.GetString().c_str());
 }
 
 std::string SupervisedUserService::GetCustodianEmailAddress() const {
   std::string email = profile_->GetPrefs()->GetString(
       prefs::kSupervisedUserCustodianEmail);
 #if defined(OS_CHROMEOS)
@@ skipping 219 matching lines @@
 SupervisedUserService::SupervisedUserService(Profile* profile)
     : includes_sync_sessions_type_(true),
       profile_(profile),
       active_(false),
       delegate_(NULL),
       waiting_for_sync_initialization_(false),
       is_profile_active_(false),
       did_init_(false),
       did_shutdown_(false),
       blacklist_state_(BlacklistLoadState::NOT_LOADED),
+      registry_observer_(this),
       weak_ptr_factory_(this) {
   url_filter_context_.ui_url_filter()->AddObserver(this);
+  registry_observer_.Add(extensions::ExtensionRegistry::Get(profile));
 }
 
 void SupervisedUserService::SetActive(bool active) {
   if (active_ == active)
     return;
   active_ = active;
 
   if (!delegate_ || !delegate_->SetActive(active_)) {
     if (active_) {
 #if !defined(OS_ANDROID)
@@ skipping 44 matching lines @@
 
 #if defined(ENABLE_EXTENSIONS)
   SetExtensionsActive();
 #endif
 
   if (active_) {
     pref_change_registrar_.Add(
         prefs::kDefaultSupervisedUserFilteringBehavior,
         base::Bind(&SupervisedUserService::OnDefaultFilteringBehaviorChanged,
             base::Unretained(this)));
+    pref_change_registrar_.Add(
+        prefs::kSupervisedUserApprovedExtensions,
+        base::Bind(&SupervisedUserService::UpdateApprovedExtensions,
+                   base::Unretained(this)));
     pref_change_registrar_.Add(prefs::kSupervisedUserSafeSites,
         base::Bind(&SupervisedUserService::OnSafeSitesSettingChanged,
                    base::Unretained(this)));
     pref_change_registrar_.Add(prefs::kSupervisedUserManualHosts,
         base::Bind(&SupervisedUserService::UpdateManualHosts,
                    base::Unretained(this)));
     pref_change_registrar_.Add(prefs::kSupervisedUserManualURLs,
         base::Bind(&SupervisedUserService::UpdateManualURLs,
                    base::Unretained(this)));
     for (const char* pref : kCustodianInfoPrefs) {
       pref_change_registrar_.Add(pref,
           base::Bind(&SupervisedUserService::OnCustodianInfoChanged,
                      base::Unretained(this)));
     }
 
     // Initialize the filter.
     OnDefaultFilteringBehaviorChanged();
     OnSafeSitesSettingChanged();
     whitelist_service_->Init();
     UpdateManualHosts();
     UpdateManualURLs();
+    UpdateApprovedExtensions();
 
 #if !defined(OS_ANDROID)
     // TODO(bauerb): Get rid of the platform-specific #ifdef here.
     // http://crbug.com/313377
     BrowserList::AddObserver(this);
 #endif
   } else {
     permissions_creators_.clear();
     url_reporter_.reset();
 
     pref_change_registrar_.Remove(
         prefs::kDefaultSupervisedUserFilteringBehavior);
+    pref_change_registrar_.Remove(prefs::kSupervisedUserApprovedExtensions);
     pref_change_registrar_.Remove(prefs::kSupervisedUserManualHosts);
     pref_change_registrar_.Remove(prefs::kSupervisedUserManualURLs);
     for (const char* pref : kCustodianInfoPrefs) {
       pref_change_registrar_.Remove(pref);
     }
 
     url_filter_context_.Clear();
     FOR_EACH_OBSERVER(
         SupervisedUserServiceObserver, observer_list_, OnURLFilterChanged());
 
@@ skipping 275 matching lines @@
     bool result = it.value().GetAsBoolean(&allow);
     DCHECK(result);
     (*url_map)[GURL(it.key())] = allow;
   }
   url_filter_context_.SetManualURLs(std::move(url_map));
 
   FOR_EACH_OBSERVER(
       SupervisedUserServiceObserver, observer_list_, OnURLFilterChanged());
 }
 
+void SupervisedUserService::UpdateApprovedExtensions() {
+  const base::DictionaryValue* dict = profile_->GetPrefs()->GetDictionary(
+      prefs::kSupervisedUserApprovedExtensions);
+  approved_extensions_map_.clear();
+  for (base::DictionaryValue::Iterator it(*dict); !it.IsAtEnd(); it.Advance()) {
+    std::string version_str;
+    bool result = it.value().GetAsString(&version_str);
+    DCHECK(result);
+    base::Version version(version_str);
+    if (version.IsValid())
+      approved_extensions_map_[it.key()] = version;
+    else
+      LOG(WARNING) << "Invalid version number " << version_str;
+  }
+
+  for (const auto& extensions_entry : approved_extensions_map_) {
+    EnableExtensionIfPossible(extensions_entry.first);
+  }
+}
+
 std::string SupervisedUserService::GetSupervisedUserName() const {
 #if defined(OS_CHROMEOS)
   // The active user can be NULL in unit tests.
   if (user_manager::UserManager::Get()->GetActiveUser()) {
     return UTF16ToUTF8(user_manager::UserManager::Get()->GetUserDisplayName(
         user_manager::UserManager::Get()->GetActiveUser()->GetAccountId()));
   }
   return std::string();
 #else
   return profile_->GetPrefs()->GetString(prefs::kProfileName);
 #endif
 }
 
 void SupervisedUserService::OnForceSessionSyncChanged() {
   includes_sync_sessions_type_ =
       profile_->GetPrefs()->GetBoolean(prefs::kForceSessionSync);
   ProfileSyncServiceFactory::GetForProfile(profile_)
       ->ReconfigureDatatypeManager();
 }
 
+void SupervisedUserService::OnExtensionInstalled(
+    content::BrowserContext* browser_context,
+    const extensions::Extension* extension,
+    bool is_update) {
+  // This callback is responsible only for updating the approved version
+  // upon extension update if it doesn't require extra permission.
+  if (!is_update)
+    return;
+
+  ExtensionPrefs* extension_prefs = ExtensionPrefs::Get(profile_);
+  const std::string& id = extension->id();
+  const base::Version& version = *extension->version();
+
+  // If an already approved extension is updated without requiring
+  // new permissions, we update the approved_version and re-enable it.
+  if (!extension_prefs->HasDisableReason(
+          id, Extension::DISABLE_PERMISSIONS_INCREASE) &&
+      approved_extensions_map_.count(id) > 0 &&
+      approved_extensions_map_[id] < version) {
+    approved_extensions_map_[id] = version;
+
+    std::string key = SupervisedUserSettingsService::MakeSplitSettingKey(
+        supervised_users::kApprovedExtensions, id);
+    std::unique_ptr<base::Value> version_value(
+        new base::StringValue(version.GetString()));
+    GetSettingsService()->UpdateSetting(key, std::move(version_value));
+
+    EnableExtensionIfPossible(id);
+  }
+}
+
 void SupervisedUserService::Shutdown() {
   if (!did_init_)
     return;
   DCHECK(!did_shutdown_);
   did_shutdown_ = true;
   if (ProfileIsSupervised()) {
     content::RecordAction(UserMetricsAction("ManagedUsers_QuitBrowser"));
   }
   SetActive(false);
 
   ProfileSyncService* sync_service =
       ProfileSyncServiceFactory::GetForProfile(profile_);
 
   // Can be null in tests.
   if (sync_service)
     sync_service->RemovePreferenceProvider(this);
 }
 
 #if defined(ENABLE_EXTENSIONS)
+SupervisedUserService::ExtensionState SupervisedUserService::GetExtensionState(
+    const Extension& extension) const {
+  bool was_installed_by_default = extension.was_installed_by_default();
+#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
+  // Note: Component extensions are protected from modification/uninstallation
+  // anyway, so there's no need to enforce them again for supervised users.
+  // Also, leave policy-installed extensions alone - they have their own
+  // management; in particular we don't want to override the force-install list.
+  if (extensions::Manifest::IsComponentLocation(extension.location()) ||
+      extensions::Manifest::IsPolicyLocation(extension.location()) ||
+      extension.is_theme() || extension.from_bookmark() ||
+      extension.is_shared_module() || was_installed_by_default) {
+    return ExtensionState::ALLOWED;
+  }
+
+  if (extension.was_installed_by_custodian())
+    return ExtensionState::FORCED;
+
+  if (!base::FeatureList::IsEnabled(
+          supervised_users::kSupervisedUserInitiatedExtensionInstall)) {
+    return ExtensionState::BLOCKED;
+  }
+
+  const std::string& id = extension.id();
+
+  auto extension_it = approved_extensions_map_.find(id);
+  if (extension_it == approved_extensions_map_.end() ||
+      extension_it->second != *(extension.version())) {

[Marc Treib, 2016/06/14 13:53:34]

nit: parens around extension.version() aren't necessary

[mamir, 2016/06/15 09:40:11]

Done.

+    return ExtensionState::REQUIRE_APPROVAL;
+  }
+
+  if (ExtensionPrefs::Get(profile_)->HasDisableReason(
+          id, Extension::DISABLE_PERMISSIONS_INCREASE)) {
+    return ExtensionState::REQUIRE_APPROVAL;
+  }
+
+  return ExtensionState::ALLOWED;
+}
+
 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,
+bool SupervisedUserService::UserMayLoad(const Extension* extension,
                                         base::string16* error) const {
   DCHECK(ProfileIsSupervised());
-  ExtensionState result = GetExtensionState(extension);
-  bool may_load = (result != EXTENSION_BLOCKED);
+  ExtensionState result = GetExtensionState(*extension);
+  bool may_load = (result != ExtensionState::BLOCKED);
   if (!may_load && error)
     *error = GetExtensionsLockedMessage();
   return may_load;
 }
 
-bool SupervisedUserService::UserMayModifySettings(
-    const extensions::Extension* extension,
-    base::string16* error) const {
+bool SupervisedUserService::UserMayModifySettings(const Extension* extension,
+                                                  base::string16* error) const {
   DCHECK(ProfileIsSupervised());
-  ExtensionState result = GetExtensionState(extension);
-  bool may_modify = (result == EXTENSION_ALLOWED);
+  ExtensionState result = GetExtensionState(*extension);
+  // While the following check allows the supervised user to modify the settings
+  // and enable or disable the extension, MustRemainDisabled properly takes care
+  // of keeping an extension disabled when required.
+  // For custodian-installed extensions, the state is always FORCED, even if
+  // it's waiting for an update approval.
+  bool may_modify = result != ExtensionState::FORCED;
   if (!may_modify && error)
     *error = GetExtensionsLockedMessage();
   return may_modify;
 }
 
 // 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 {
+bool SupervisedUserService::MustRemainInstalled(const Extension* extension,
+                                                base::string16* error) const {
   DCHECK(ProfileIsSupervised());
-  ExtensionState result = GetExtensionState(extension);
-  bool may_not_uninstall = (result == EXTENSION_FORCED);
+  ExtensionState result = GetExtensionState(*extension);
+  bool may_not_uninstall = (result == ExtensionState::FORCED);
   if (may_not_uninstall && error)
     *error = GetExtensionsLockedMessage();
   return may_not_uninstall;
 }
 
+bool SupervisedUserService::MustRemainDisabled(const Extension* extension,
+                                               Extension::DisableReason* reason,
+                                               base::string16* error) const {
+  DCHECK(ProfileIsSupervised());
+  ExtensionState state = GetExtensionState(*extension);
+  bool must_remain_disabled = state == ExtensionState::BLOCKED ||
+                              state == ExtensionState::REQUIRE_APPROVAL;
+
+  if (must_remain_disabled) {
+    // If the extension must remain disabled due to permission increase,
+    // then the update request has been already sent at update time.
+    // We do nothing and we don't add an extra disable reason.
+    ExtensionPrefs* extension_prefs = ExtensionPrefs::Get(profile_);
+    if (extension_prefs->HasDisableReason(
+            extension->id(), Extension::DISABLE_PERMISSIONS_INCREASE)) {
+      if (reason)
+        *reason = Extension::DISABLE_PERMISSIONS_INCREASE;

[Marc Treib, 2016/06/14 13:53:34]

You probably also need to set error here?

[mamir, 2016/06/15 09:40:11]

Done.

+      return true;
+    }
+    if (reason)
+      *reason = Extension::DISABLE_CUSTODIAN_APPROVAL_REQUIRED;

[Marc Treib, 2016/06/14 13:53:34]

Hm, this is confusing. Can we ever get here if SU-initiated installs are not
enabled?

[mamir, 2016/06/15 09:40:11]

Not really, because if SU-initiated installs are not enabled, the extensions
will be blocked and "UserMayLoad" will return false.
(I assume MustRemainDisabled is called only for loaded extensions)

[Marc Treib, 2016/06/15 12:31:07]

If it can't happen, then the code shouldn't handle it. Add a DCHECK instead?
(Not sure in what circumstances MustRemainDisabled can be called..)

[mamir, 2016/06/15 17:30:02]

Sorry, I was wrong.
It can actually reach this part for pre-installed extensions if SU-initiated
installs aren't enabled.

+    if (error)
+      *error = l10n_util::GetStringUTF16(IDS_EXTENSIONS_LOCKED_SUPERVISED_USER);
+    if (base::FeatureList::IsEnabled(
+            supervised_users::kSupervisedUserInitiatedExtensionInstall)) {
+      // If the Extension isn't pending a custodian approval already, send
+      // an approval request.
+      if (!extension_prefs->HasDisableReason(
+              extension->id(),
+              Extension::DISABLE_CUSTODIAN_APPROVAL_REQUIRED)) {
+        // MustRemainDisabled is a const method and hence cannot call
+        // AddExtensionInstallRequest directly.
+        SupervisedUserService* supervised_user_service =
+            SupervisedUserServiceFactory::GetForProfile(profile_);
+        supervised_user_service->AddExtensionInstallRequest(
+            extension->id(), *extension->version());
+      }
+    }
+  }
+  return must_remain_disabled;
+}
+
 void SupervisedUserService::SetExtensionsActive() {
   extensions::ExtensionSystem* extension_system =
       extensions::ExtensionSystem::Get(profile_);
   extensions::ManagementPolicy* management_policy =
       extension_system->management_policy();
 
   if (management_policy) {
     if (active_)
       management_policy->RegisterProvider(this);
     else
@@ skipping 45 matching lines @@
     content::RecordAction(UserMetricsAction("ManagedUsers_SwitchProfile"));
 
   is_profile_active_ = profile_became_active;
 }
 #endif  // !defined(OS_ANDROID)
 
 void SupervisedUserService::OnSiteListUpdated() {
   FOR_EACH_OBSERVER(
       SupervisedUserServiceObserver, observer_list_, OnURLFilterChanged());
 }