Disable extension installs for child users when pref is toggled off

When the "Permissions for sites and apps" toggle in Chrome family
dashboard is disabled, we should prevent child users from installing
new extensions and approving additional permissions for existing
extensions. This CL focuses on preventing child users from installing
new extensions. The second functionality will be implemented in a
future CL. When the toggle is disabled, pref
kSupervisedUserExtensionsMayRequestPermissions becomes false, and this
is used to implement the desired effects in this CL.

Bug: 1018956,1007011
Change-Id: Ida3c53d87da55fd266edc9d021211f5cdc641d0f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1904502
Commit-Queue: Toby Huang <tobyhuang@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Dan S <danan@chromium.org>
Reviewed-by: Aga Wronska <agawronska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#718389}

chrome/browser/supervised_user/supervised_user_features.cc

@@ -11,6 +11,5 @@ const base::Feature kSupervisedUserIframeFilter{
 
 const base::Feature kSupervisedUserInitiatedExtensionInstall{
     "SupervisedUserInitiatedExtensionInstall",
-    base::FEATURE_DISABLED_BY_DEFAULT};
-
+    base::FEATURE_ENABLED_BY_DEFAULT};
 }

chrome/browser/supervised_user/supervised_user_pref_store.cc

@@ -162,10 +162,10 @@ void SupervisedUserPrefStore::OnNewSettingsAvailable(
 
 #if BUILDFLAG(ENABLE_EXTENSIONS)
     {
-      // TODO(michaelpg): Update Kids Management server to set a new bit for
+      // TODO(crbug/1024646): Update Kids Management server to set a new bit for
       // extension permissions. Until then, rely on other side effects of the
-      // "allow sites and apps to ask for permissions" setting, like
-      // geolocation being disallowed.
+      // "Permissions for sites and apps" setting, like geolocation being
+      // disallowed.
       bool permissions_disallowed = true;
       settings->GetBoolean(supervised_users::kGeolocationDisabled,
                            &permissions_disallowed);

chrome/browser/supervised_user/supervised_user_pref_store_unittest.cc

@@ -168,7 +168,7 @@ TEST_F(SupervisedUserPrefStoreTest, ConfigureSettings) {
 #if BUILDFLAG(ENABLE_EXTENSIONS)
   // The custodian can allow sites and apps to request permissions.
   // Currently tested indirectly by enabling geolocation requests.
-  // TODO(michaelpg): Update Kids Management server to set a new bit for
+  // TODO(crbug/1024646): Update Kids Management server to set a new bit for
   // extension permissions and update this test.
   fixture.changed_prefs()->Clear();
   service_.SetLocalSetting(supervised_users::kGeolocationDisabled,
@@ -202,4 +202,3 @@ TEST_F(SupervisedUserPrefStoreTest, CreatePrefStoreAfterInitialization) {
   EXPECT_TRUE(fixture.initialization_completed());
   EXPECT_EQ(0u, fixture.changed_prefs()->size());
 }
-

chrome/browser/supervised_user/supervised_user_service.cc

@@ -355,6 +355,27 @@ void SupervisedUserService::UpdateApprovedExtensions(
     ChangeExtensionStateIfNecessary(extension_id);
   }
 }
+
+bool SupervisedUserService::
+    GetSupervisedUserExtensionsMayRequestPermissionsPref() const {
+  return profile_->GetPrefs()->GetBoolean(
+      prefs::kSupervisedUserExtensionsMayRequestPermissions);
+}
+
+void SupervisedUserService::
+    SetSupervisedUserExtensionsMayRequestPermissionsPrefForTesting(
+        bool enabled) {
+  // TODO(crbug/1024646): kSupervisedUserExtensionsMayRequestPermissions is
+  // currently set indirectly by setting geolocation requests. Update Kids
+  // Management server to set a new bit for extension permissions and update
+  // this setter function.
+  GetSettingsService()->SetLocalSetting(
+      supervised_users::kGeolocationDisabled,
+      std::make_unique<base::Value>(!enabled));
+  profile_->GetPrefs()->SetBoolean(
+      prefs::kSupervisedUserExtensionsMayRequestPermissions, enabled);
+}
+
 #endif  // BUILDFLAG(ENABLE_EXTENSIONS)
 
 void SupervisedUserService::SetActive(bool active) {
@@ -713,11 +734,21 @@ SupervisedUserService::ExtensionState SupervisedUserService::GetExtensionState(
     return ExtensionState::ALLOWED;
   }
 
+  // Feature flag for gating new behavior.
   if (!base::FeatureList::IsEnabled(
           supervised_users::kSupervisedUserInitiatedExtensionInstall)) {
     return ExtensionState::BLOCKED;
   }
 
+  if (!GetSupervisedUserExtensionsMayRequestPermissionsPref()) {
+    if (!ExtensionRegistry::Get(profile_)->GetInstalledExtension(
+            extension.id())) {
+      // Block child users from installing new extensions. Already installed
+      // extensions should not be affected.
+      return ExtensionState::BLOCKED;
+    }
+  }
+
   auto extension_it = approved_extensions_map_.find(extension.id());
   // If the installed version is approved, then the extension is allowed,
   // otherwise, it requires approval.
@@ -812,14 +843,22 @@ void SupervisedUserService::OnExtensionInstalled(
 
     UpdateApprovedExtensions(id, version.GetString(),
                              syncer::SyncChange::ACTION_ADD);
+  } else {
+    // Upon extension update, the approved version may (or may not) match the
+    // installed one. Therefore, a change in extension state might be required.
+    ChangeExtensionStateIfNecessary(id);
   }
-  // Upon extension update, the approved version may (or may not) match the
-  // installed one. Therefore, a change in extension state might be required.
-  ChangeExtensionStateIfNecessary(id);
 }
 
 void SupervisedUserService::ChangeExtensionStateIfNecessary(
     const std::string& extension_id) {
+  // If the profile is not supervised, do nothing.
+  // TODO(crbug/1026900): SupervisedUserService should not be active if the
+  // profile is not even supervised during browser tests, i.e. this check
+  // shouldn't be needed.
+  if (!active_)
+    return;
+  DCHECK(ProfileIsSupervised());
   ExtensionRegistry* registry = ExtensionRegistry::Get(profile_);
   const Extension* extension = registry->GetInstalledExtension(extension_id);
   // If the extension is not installed (yet), do nothing.

chrome/browser/supervised_user/supervised_user_service.h

@@ -187,6 +187,11 @@ class SupervisedUserService : public KeyedService,
   void UpdateApprovedExtensions(const std::string& extension_id,
                                 const std::string& version,
                                 syncer::SyncChange::SyncChangeType type);
+
+  bool GetSupervisedUserExtensionsMayRequestPermissionsPref() const;
+
+  void SetSupervisedUserExtensionsMayRequestPermissionsPrefForTesting(
+      bool enabled);
 #endif  // BUILDFLAG(ENABLE_EXTENSIONS)
 
  private:
@@ -230,16 +235,17 @@ class SupervisedUserService : public KeyedService,
 
   // An extension can be in one of the following states:
   //
-  // REQUIRE_APPROVAL: if it is installed by the supervised user and
-  //    hasn't been approved by the custodian yet.
+  // BLOCKED: if kSupervisedUserExtensionsMayRequestPermissions is false and the
+  // child user is attempting to install a new extension or an existing
+  // extension is asking for additional permissions.
   // ALLOWED: Components, Themes, Default extensions ..etc
   //    are generally allowed.  Extensions that have been approved by the
   //    custodian are also allowed.
-  // BLOCKED: if it is not ALLOWED or FORCED
-  //    and supervised users initiated installs are disabled.
+  // REQUIRE_APPROVAL: if it is installed by the child user and
+  //    hasn't been approved by the custodian yet.
   enum class ExtensionState { BLOCKED, ALLOWED, REQUIRE_APPROVAL };
 
-  // Returns the state of an extension whether being FORCED, BLOCKED, ALLOWED or
+  // Returns the state of an extension whether being BLOCKED, ALLOWED or
   // REQUIRE_APPROVAL from the Supervised User service's point of view.
   ExtensionState GetExtensionState(
       const extensions::Extension& extension) const;
@@ -248,7 +254,7 @@ class SupervisedUserService : public KeyedService,
   void SetExtensionsActive();
 
   // Enables/Disables extensions upon change in approved version of the
-  // extension_id.
+  // extension_id. This function is idempotent.
   void ChangeExtensionStateIfNecessary(const std::string& extension_id);
 #endif  // BUILDFLAG(ENABLE_EXTENSIONS)
 

chrome/browser/supervised_user/supervised_user_service_unittest.cc

@@ -415,10 +415,18 @@ class SupervisedUserServiceExtensionTestBase
     return extension;
   }
 
+  void InitSupervisedUserInitiatedExtensionInstallFeature(bool enabled) {
+    if (enabled) {
+      scoped_feature_list_.InitAndEnableFeature(
+          supervised_users::kSupervisedUserInitiatedExtensionInstall);
+    }
+  }
+
   bool is_supervised_;
   extensions::ScopedCurrentChannel channel_;
   SiteListObserver site_list_observer_;
   SupervisedUserURLFilterObserver url_filter_observer_;
+  base::test::ScopedFeatureList scoped_feature_list_;
 };
 
 class SupervisedUserServiceExtensionTestUnsupervised
@@ -437,6 +445,8 @@ class SupervisedUserServiceExtensionTest
 
 TEST_F(SupervisedUserServiceExtensionTest,
        ExtensionManagementPolicyProviderWithoutSUInitiatedInstalls) {
+  InitSupervisedUserInitiatedExtensionInstallFeature(true);
+
   SupervisedUserService* supervised_user_service =
       SupervisedUserServiceFactory::GetForProfile(profile_.get());
   ASSERT_TRUE(profile_->IsSupervised());
@@ -467,9 +477,14 @@ TEST_F(SupervisedUserServiceExtensionTest,
     EXPECT_FALSE(error_1.empty());
 
     base::string16 error_2;
+    EXPECT_FALSE(
+        supervised_user_service->UserMayInstall(extension.get(), &error_2));
+    EXPECT_FALSE(error_2.empty());
+
+    base::string16 error_3;
     EXPECT_FALSE(supervised_user_service->MustRemainInstalled(extension.get(),
-                                                              &error_2));
-    EXPECT_TRUE(error_2.empty());
+                                                              &error_3));
+    EXPECT_TRUE(error_3.empty());
   }
 
 #if DCHECK_IS_ON()
@@ -477,19 +492,18 @@ TEST_F(SupervisedUserServiceExtensionTest,
 #endif
 }
 
-// TODO(crbug.com/910597): Flaky due to use of ScopedFeatureList.
 TEST_F(SupervisedUserServiceExtensionTest,
-       DISABLED_ExtensionManagementPolicyProviderWithSUInitiatedInstalls) {
-  // Enable supervised user initiated installs.
-  // TODO(crbug.com/846380): ScopedFeatureList must be initialized before the
-  // BrowserTaskEnvironment in ExtensionServiceTestBase to avoid races between
-  // threads.
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndEnableFeature(
-      supervised_users::kSupervisedUserInitiatedExtensionInstall);
+       ExtensionManagementPolicyProviderWithSUInitiatedInstalls) {
+  // Enable child users to initiate extension installs by simulating the
+  // toggling of "Permissions for sites and apps" to enabled.
+  InitSupervisedUserInitiatedExtensionInstallFeature(true);
 
   SupervisedUserService* supervised_user_service =
       SupervisedUserServiceFactory::GetForProfile(profile_.get());
+  supervised_user_service
+      ->SetSupervisedUserExtensionsMayRequestPermissionsPrefForTesting(true);
+  ASSERT_TRUE(supervised_user_service
+                  ->GetSupervisedUserExtensionsMayRequestPermissionsPref());
   ASSERT_TRUE(profile_->IsSupervised());
 
   // The supervised user should be able to load and uninstall the extensions
@@ -520,6 +534,11 @@ TEST_F(SupervisedUserServiceExtensionTest,
     EXPECT_FALSE(supervised_user_service->UserMayModifySettings(extension.get(),
                                                                 &error_4));
     EXPECT_FALSE(error_4.empty());
+
+    base::string16 error_5;
+    EXPECT_TRUE(
+        supervised_user_service->UserMayInstall(extension.get(), &error_5));
+    EXPECT_TRUE(error_5.empty());
   }
 
 #if DCHECK_IS_ON()
@@ -528,6 +547,8 @@ TEST_F(SupervisedUserServiceExtensionTest,
 }
 
 TEST_F(SupervisedUserServiceExtensionTest, NoContentPacks) {
+  InitSupervisedUserInitiatedExtensionInstallFeature(true);
+
   SupervisedUserService* supervised_user_service =
       SupervisedUserServiceFactory::GetForProfile(profile_.get());
   SupervisedUserURLFilter* url_filter = supervised_user_service->GetURLFilter();
@@ -542,6 +563,8 @@ TEST_F(SupervisedUserServiceExtensionTest, NoContentPacks) {
 }
 
 TEST_F(SupervisedUserServiceExtensionTest, InstallContentPacks) {
+  InitSupervisedUserInitiatedExtensionInstallFeature(true);
+
   SupervisedUserService* supervised_user_service =
       SupervisedUserServiceFactory::GetForProfile(profile_.get());
   SupervisedUserURLFilter* url_filter = supervised_user_service->GetURLFilter();