Enable supervised users to accept extension permission updates

If an extensions developer pushes an update that requires additional
permissions, and the extension was previously approved by the parent,
and the parent has set the "Permissions for sites, apps and
extensions" toggle to on, then the supervised user should be able to
approve the additional permissions and re-enable the extension by
themselves without parent approval.

Currently, supervised users are not able to accept extension updates
requiring additional permissions at all. This CL fixes that behavior.

Bug: 1068660,1070760
Change-Id: Icc6af9a8a05f3782a12c2f4e001d7d6470cacefd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2140596
Commit-Queue: Toby Huang <tobyhuang@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Dan S <danan@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#761126}

chrome/browser/extensions/extension_service_sync_unittest.cc

@@ -2711,10 +2711,8 @@ TEST_F(ExtensionServiceTestSupervised,
   EXPECT_FALSE(registry()->enabled_extensions().Contains(id));
   EXPECT_TRUE(IsPendingCustodianApproval(id));
   int disable_reasons = ExtensionPrefs::Get(profile())->GetDisableReasons(id);
-  EXPECT_EQ(
-      disable_reasons,
-      extensions::disable_reason::DISABLE_PERMISSIONS_INCREASE |
-          extensions::disable_reason::DISABLE_CUSTODIAN_APPROVAL_REQUIRED);
+  EXPECT_EQ(disable_reasons,
+            extensions::disable_reason::DISABLE_PERMISSIONS_INCREASE);
 }
 
 // Tests that extension installation is blocked for child accounts without any

chrome/browser/supervised_user/supervised_user_service.cc

@@ -11,6 +11,7 @@
 #include "base/feature_list.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
+#include "base/logging.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/metrics/user_metrics.h"
 #include "base/path_service.h"
@@ -862,20 +863,22 @@ bool SupervisedUserService::MustRemainDisabled(
   bool must_remain_disabled = state == ExtensionState::REQUIRE_APPROVAL;
 
   if (must_remain_disabled) {
-    if (error)
-      *error = GetExtensionsLockedMessage();
-    // If the extension must remain disabled due to permission increase, then we
-    // do nothing and we don't add an extra disable reason.
-    ExtensionPrefs* extension_prefs = ExtensionPrefs::Get(profile_);
-    if (extension_prefs->HasDisableReason(
-            extension->id(),
-            extensions::disable_reason::DISABLE_PERMISSIONS_INCREASE)) {
-      if (reason)
-        *reason = extensions::disable_reason::DISABLE_PERMISSIONS_INCREASE;
-      return true;
+    if (base::Contains(approved_extensions_map_, extension->id())) {
+      // The parent has approved this extension in the past, so the child can
+      // approve a new version (as long as
+      // kSupervisedUserExtensionsMayRequestPermissions is true, but that's
+      // enforced elsewhere).
+      // TODO(crbug/1072857): Get rid of all the version information from
+      // approved_extensions_map_. Just store a set of approved extension ids.
+      return false;
     }
-    if (reason)
+    if (reason) {
+      // Otherwise, the parent has not approved this extension yet, so ask for
+      // parent approval.
       *reason = extensions::disable_reason::DISABLE_CUSTODIAN_APPROVAL_REQUIRED;
+    }
+    if (error)
+      *error = GetExtensionsLockedMessage();
   }
   return must_remain_disabled;
 }

extensions/browser/api/management/management_api.cc

@@ -429,7 +429,7 @@ ExtensionFunction::ResponseAction ManagementSetEnabledFunction::Run() {
   if (!target_extension || !target_extension->ShouldExposeViaManagementAPI())
     return RespondNow(Error(keys::kNoExtensionError, extension_id_));
 
-  bool enabled = params->enabled;
+  bool should_enable = params->enabled;
 
   const SupervisedUserServiceDelegate* supervised_user_service_delegate =
       ManagementAPI::GetFactoryInstance()
@@ -450,15 +450,15 @@ ExtensionFunction::ResponseAction ManagementSetEnabledFunction::Run() {
     return RespondNow(Error(keys::kUserCantModifyError, extension_id_));
   }
 
-  disable_reason::DisableReason reason;
+  disable_reason::DisableReason reason = disable_reason::DISABLE_NONE;
   bool disallow_enable =
-      enabled && policy->MustRemainDisabled(target_extension, &reason, nullptr);
+      should_enable &&
+      policy->MustRemainDisabled(target_extension, &reason, nullptr);
 
   // Figure out if we should prompt for parental approval.
   bool prompt_parent_for_approval =
       disallow_enable && is_supervised_child_who_may_install_extensions &&
-      reason ==
-          disable_reason::DisableReason::DISABLE_CUSTODIAN_APPROVAL_REQUIRED;
+      reason == disable_reason::DISABLE_CUSTODIAN_APPROVAL_REQUIRED;
 
   // If the extension can't be enabled, only continue if we plan to prompt for
   // parental approval.
@@ -472,9 +472,10 @@ ExtensionFunction::ResponseAction ManagementSetEnabledFunction::Run() {
       registry->enabled_extensions().Contains(extension_id_) ||
       registry->terminated_extensions().Contains(extension_id_);
 
-  if (!currently_enabled && enabled) {
+  if (!currently_enabled && should_enable) {
     ExtensionPrefs* prefs = ExtensionPrefs::Get(browser_context());
-    if (prefs->DidExtensionEscalatePermissions(extension_id_)) {
+    if (!prompt_parent_for_approval &&
+        prefs->DidExtensionEscalatePermissions(extension_id_)) {
       if (!user_gesture())
         return RespondNow(Error(keys::kGestureNeededForEscalationError));
 
@@ -494,8 +495,8 @@ ExtensionFunction::ResponseAction ManagementSetEnabledFunction::Run() {
                          this));  // This bind creates a reference.
       return RespondLater();
     }
-    // Handle parental approval for child accounts that have the
-    // ability to install extensions.
+    // Handle parental approval for child accounts that have the ability to
+    // install extensions.
     if (prompt_parent_for_approval &&
         // Don't re-prompt the parent for extensions that have already been
         // approved for a child.