Supervised Users Initiated Installs v2

chrome/browser/supervised_user/supervised_user_service.cc

Index: chrome/browser/supervised_user/supervised_user_service.cc
diff --git a/chrome/browser/supervised_user/supervised_user_service.cc b/chrome/browser/supervised_user/supervised_user_service.cc
index 0f71827875aeaaebb3c71482ecd4956c8741e3f2..247c7dd63df1149f48559010d738fb7964c9debd 100644
--- a/chrome/browser/supervised_user/supervised_user_service.cc
+++ b/chrome/browser/supervised_user/supervised_user_service.cc
@@ -7,6 +7,7 @@
 #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"
@@ -27,6 +28,7 @@
 #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"
@@ -48,6 +50,7 @@
 #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"
 #include "ui/base/l10n/l10n_util.h"
 
 #if !defined(OS_ANDROID)
@@ -68,6 +71,9 @@
 
 #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"
+#include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/extension_system.h"
 #endif
 
@@ -79,6 +85,9 @@
 using base::DictionaryValue;
 using base::UserMetricsAction;
 using content::BrowserThread;
+using extensions::Extension;
+using extensions::ExtensionPrefs;
+using extensions::ExtensionSystem;
 
 namespace {
 
@@ -106,6 +115,13 @@ void CreateURLAccessRequest(
   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,
@@ -113,6 +129,11 @@ void CreateExtensionUpdateRequest(
   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;
@@ -123,46 +144,6 @@ base::FilePath GetBlacklistPath() {
   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() {
@@ -173,6 +154,7 @@ SupervisedUserService::~SupervisedUserService() {
 // static
 void SupervisedUserService::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
+  registry->RegisterDictionaryPref(prefs::kSupervisedUserApprovedExtensions);
   registry->RegisterDictionaryPref(prefs::kSupervisedUserManualHosts);
   registry->RegisterDictionaryPref(prefs::kSupervisedUserManualURLs);
   registry->RegisterIntegerPref(prefs::kDefaultSupervisedUserFilteringBehavior,
@@ -263,11 +245,28 @@ void SupervisedUserService::ReportURL(const GURL& url,
     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);
 }
@@ -275,13 +274,46 @@ void SupervisedUserService::AddExtensionUpdateRequest(
 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::UpdateApprovedExtensionVersion(
+    const std::string& extension_id,
+    const base::Version& version) {
+  approved_extensions_map_[extension_id] = version;

[Marc Treib, 2016/06/08 12:08:24]

Hm, should probably make sure that the approved version can never get smaller?

[mamir, 2016/06/09 12:14:10]

omm, well, probably yes.
But I don't think we should check here.
Let's keep this method for always updating the approved version.
I will check before calling it.

[Marc Treib, 2016/06/09 13:44:09]

Hm, UpdateApprovedExtensionVersion is called in only one place, might as well
inline it there. That removes the question about where to check the version.
Also, right now we have UpdateApprovedExtensions and
UpdateApprovedExtensionVersion - they sound similar, but do very different
things.

[mamir, 2016/06/09 14:04:39]

Done.

+  std::string key = SupervisedUserSettingsService::MakeSplitSettingKey(
+      supervised_users::kApprovedExtensions, extension_id);
+  std::unique_ptr<base::Value> version_value(
+      new base::StringValue(version.GetString()));
+
+  GetSettingsService()->UpdateSetting(key, std::move(version_value));
+
+  EnableExtensionIfPossible(extension_id);
+}
+
+void SupervisedUserService::EnableExtensionIfPossible(
+    const std::string& extension_id) {
+  ExtensionService* service =
+      ExtensionSystem::Get(profile_)->extension_service();
+  ExtensionPrefs* extension_prefs = ExtensionPrefs::Get(profile_);
+  // Check if the extension was pending custodian approval.
+  if (extension_prefs->HasDisableReason(
+          extension_id, Extension::DISABLE_CUSTODIAN_APPROVAL_REQUIRED)) {
+    extension_prefs->RemoveDisableReason(
+        extension_id, Extension::DISABLE_CUSTODIAN_APPROVAL_REQUIRED);
+    // If no other disable reasons, enable it.
+    if (!extension_prefs->GetDisableReasons(extension_id)) {
+      // Try to enable the extension, this will call the ManagmentPolicy and
+      // properly enable the extension if possible.
+      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(),
@@ -523,6 +555,7 @@ SupervisedUserService::SupervisedUserService(Profile* profile)
       blacklist_state_(BlacklistLoadState::NOT_LOADED),
       weak_ptr_factory_(this) {
   url_filter_context_.ui_url_filter()->AddObserver(this);
+  extensions::ExtensionRegistry::Get(profile)->AddObserver(this);
 }
 
 void SupervisedUserService::SetActive(bool active) {
@@ -587,6 +620,10 @@ void SupervisedUserService::SetActive(bool active) {
         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)));
@@ -608,6 +645,7 @@ void SupervisedUserService::SetActive(bool active) {
     whitelist_service_->Init();
     UpdateManualHosts();
     UpdateManualURLs();
+    UpdateApprovedExtensions();
 
 #if !defined(OS_ANDROID)
     // TODO(bauerb): Get rid of the platform-specific #ifdef here.
@@ -620,6 +658,7 @@ void SupervisedUserService::SetActive(bool active) {
 
     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) {
@@ -915,6 +954,26 @@ void SupervisedUserService::UpdateManualURLs() {
       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.
@@ -935,6 +994,29 @@ void SupervisedUserService::OnForceSessionSyncChanged() {
       ->ReconfigureDatatypeManager();
 }
 
+void SupervisedUserService::OnExtensionInstalled(
+    content::BrowserContext* browser_context,
+    const extensions::Extension* extension,
+    bool is_update) {
+  // This call is responsible only for updating the approved version
+  // upon extension update if it doesn't require extra permission,
+  // and sending an approval request when it requires extra permissions
+  if (!is_update)
+    return;
+
+  ExtensionPrefs* extension_prefs = ExtensionPrefs::Get(profile_);
+  const std::string& id = extension->id();
+
+  // If the extension is disabled because it requires parent approval,
+  // but it doesn't require new permissions, then it should be enabled if
+  // it has been approved before.

[Marc Treib, 2016/06/08 12:08:24]

This comment is inaccurate, no parent approval involved really.

[mamir, 2016/06/09 12:14:10]

Sorry, that was before I removed this check but I forgot to update the comment
accordingly.

+  if (!extension_prefs->HasDisableReason(
+          id, Extension::DISABLE_PERMISSIONS_INCREASE) &&
+      approved_extensions_map_.count(id) > 0) {
+    UpdateApprovedExtensionVersion(id, *extension->version());
+  }
+}
+
 void SupervisedUserService::Shutdown() {
   if (!did_init_)
     return;
@@ -954,6 +1036,55 @@ void SupervisedUserService::Shutdown() {
 }
 
 #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;
+
+  // TODO(mamir): if(on blacklist) return BLOCKED;
+  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())) {
+    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
@@ -964,22 +1095,26 @@ std::string SupervisedUserService::GetDebugPolicyProviderName() const {
 #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 SU 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;
@@ -988,17 +1123,49 @@ bool SupervisedUserService::UserMayModifySettings(
 // 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 (reason)
+      *reason = Extension::DISABLE_CUSTODIAN_APPROVAL_REQUIRED;
+    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.
+      ExtensionPrefs* extension_prefs = ExtensionPrefs::Get(profile_);
+      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_);