Extensions remove custodianApprovalRequired after removing supervision

This CL fixes the bug where extensions remain disabled with reason
custodianApprovalRequired even after supervision is removed. This CL
accomplishes this by removing supervised user related disable reasons
on startup if the logged in profile is not a child account.

Bug: 1045625
Change-Id: I35491e240610334aa9665cddc0c9f4fa0ea00037
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2026172Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Aga Wronska <agawronska@chromium.org>
Reviewed-by: Toni Baržić <tbarzic@chromium.org>
Commit-Queue: Toby Huang <tobyhuang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#738366}

chrome/browser/chromeos/login/screens/supervision_transition_screen_browsertest.cc

@@ -50,7 +50,6 @@ class SupervisionTransitionScreenTest
 
   void SetUpOnMainThread() override {
     ASSERT_TRUE(arc_temp_dir_.CreateUniqueTempDir());
-    CHECK(logged_in_user_mixin_.GetUserPolicyMixin()->RequestPolicyUpdate());
 
     arc::ArcServiceLauncher::Get()->ResetForTesting();
     arc::ArcSessionManager::Get()->SetArcSessionRunnerForTesting(
@@ -67,7 +66,8 @@ class SupervisionTransitionScreenTest
     // WaitForActiveSession() otherwise.
     logged_in_user_mixin_.LogInUser(
         false /*issue_any_scope_token*/,
-        content::IsPreTest() /*wait_for_active_session*/);
+        content::IsPreTest() /*wait_for_active_session*/,
+        true /*request_policy_update*/);
   }
 
   // The tests simulate user type changes between regular and child user.

chrome/browser/extensions/extension_service.cc

@@ -1022,6 +1022,12 @@ void ExtensionService::CheckManagementPolicy() {
       disable_reasons &= (~disable_reason::DISABLE_BLOCKED_BY_POLICY);
     }
 
+    // If this profile is not supervised, then remove any supervised user
+    // related disable reasons.
+    if (!profile()->IsChild()) {
+      disable_reasons &= (~disable_reason::DISABLE_CUSTODIAN_APPROVAL_REQUIRED);
+    }
+
     extension_prefs_->ReplaceDisableReasons(extension->id(), disable_reasons);
     if (disable_reasons == disable_reason::DISABLE_NONE)
       to_enable.push_back(extension->id());

chrome/browser/supervised_user/supervised_user_extension_browsertest.cc

+// Copyright 2020 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 "base/files/file_path.h"
+#include "base/test/scoped_feature_list.h"
+#include "chrome/browser/extensions/extension_browsertest.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/supervised_user/logged_in_user_mixin.h"
+#include "chrome/browser/supervised_user/supervised_user_features.h"
+#include "chrome/browser/supervised_user/supervised_user_service.h"
+#include "chrome/browser/supervised_user/supervised_user_service_factory.h"
+#include "chrome/test/base/mixin_based_in_process_browser_test.h"
+#include "content/public/test/test_launcher.h"
+#include "extensions/browser/disable_reason.h"
+#include "extensions/browser/extension_prefs.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/common/extension.h"
+
+namespace {
+
+constexpr char kGoodCrxId[] = "ldnnhddmnhbkjipkidpdiheffobcpfmf";
+
+}
+
+namespace extensions {
+
+// Tests for the interaction between supervised users and extensions.
+class SupervisedUserExtensionTest : public ExtensionBrowserTest {
+ public:
+  SupervisedUserExtensionTest() {
+    // Suppress regular user login to enable child user login.
+    set_chromeos_user_ = false;
+  }
+
+  // We have to essentially replicate what MixinBasedInProcessBrowserTest does
+  // here because ExtensionBrowserTest doesn't inherit from that class.
+  void SetUp() override {
+    scoped_feature_list_.InitAndEnableFeature(
+        supervised_users::kSupervisedUserInitiatedExtensionInstall);
+    mixin_host_.SetUp();
+    ExtensionBrowserTest::SetUp();
+  }
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    mixin_host_.SetUpCommandLine(command_line);
+    ExtensionBrowserTest::SetUpCommandLine(command_line);
+  }
+
+  void SetUpDefaultCommandLine(base::CommandLine* command_line) override {
+    mixin_host_.SetUpDefaultCommandLine(command_line);
+    ExtensionBrowserTest::SetUpDefaultCommandLine(command_line);
+  }
+
+  bool SetUpUserDataDirectory() override {
+    return mixin_host_.SetUpUserDataDirectory() &&
+           ExtensionBrowserTest::SetUpUserDataDirectory();
+  }
+
+  void SetUpInProcessBrowserTestFixture() override {
+    mixin_host_.SetUpInProcessBrowserTestFixture();
+    ExtensionBrowserTest::SetUpInProcessBrowserTestFixture();
+  }
+
+  void CreatedBrowserMainParts(
+      content::BrowserMainParts* browser_main_parts) override {
+    mixin_host_.CreatedBrowserMainParts(browser_main_parts);
+    ExtensionBrowserTest::CreatedBrowserMainParts(browser_main_parts);
+  }
+
+  void SetUpOnMainThread() override {
+    mixin_host_.SetUpOnMainThread();
+    logged_in_user_mixin_.LogInUser();
+    ExtensionBrowserTest::SetUpOnMainThread();
+  }
+
+  void TearDownOnMainThread() override {
+    mixin_host_.TearDownOnMainThread();
+    ExtensionBrowserTest::TearDownOnMainThread();
+  }
+
+  void TearDownInProcessBrowserTestFixture() override {
+    mixin_host_.TearDownInProcessBrowserTestFixture();
+    ExtensionBrowserTest::TearDownInProcessBrowserTestFixture();
+  }
+
+  void TearDown() override {
+    mixin_host_.TearDown();
+    ExtensionBrowserTest::TearDown();
+  }
+
+ protected:
+  SupervisedUserService* GetSupervisedUserService() {
+    return SupervisedUserServiceFactory::GetForProfile(profile());
+  }
+
+  void SetSupervisedUserExtensionsMayRequestPermissionsPref(bool enabled) {
+    GetSupervisedUserService()
+        ->SetSupervisedUserExtensionsMayRequestPermissionsPrefForTesting(
+            enabled);
+  }
+
+  bool IsDisabledForCustodianApproval(const std::string& extension_id) {
+    ExtensionPrefs* extension_prefs = ExtensionPrefs::Get(profile());
+    return extension_prefs->HasDisableReason(
+        extension_id,
+        extensions::disable_reason::DISABLE_CUSTODIAN_APPROVAL_REQUIRED);
+  }
+
+ private:
+  InProcessBrowserTestMixinHost mixin_host_;
+
+  // We want to log in as child user for all of the PRE tests, and regular user
+  // otherwise.
+  chromeos::LoggedInUserMixin logged_in_user_mixin_{
+      &mixin_host_,
+      content::IsPreTest() ? chromeos::LoggedInUserMixin::LogInType::kChild
+                           : chromeos::LoggedInUserMixin::LogInType::kRegular,
+      embedded_test_server(), this};
+
+  base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+// Removing supervision should also remove associated disable reasons, such as
+// DISABLE_CUSTODIAN_APPROVAL_REQUIRED. Extensions should become enabled again
+// after removing supervision. Prevents a regression to crbug/1045625.
+IN_PROC_BROWSER_TEST_F(SupervisedUserExtensionTest,
+                       PRE_RemovingSupervisionCustodianApprovalRequired) {
+  SetSupervisedUserExtensionsMayRequestPermissionsPref(true);
+
+  EXPECT_TRUE(profile()->IsChild());
+
+  base::FilePath path = test_data_dir_.AppendASCII("good.crx");
+  EXPECT_FALSE(LoadExtensionWithFlags(path, kFlagNone));
+  const Extension* extension =
+      extension_registry()->GetInstalledExtension(kGoodCrxId);
+  EXPECT_TRUE(extension);
+
+  // This extension is a supervised user initiated install and should remain
+  // disabled.
+  EXPECT_TRUE(extension_registry()->disabled_extensions().Contains(kGoodCrxId));
+  EXPECT_TRUE(IsDisabledForCustodianApproval(kGoodCrxId));
+}
+
+IN_PROC_BROWSER_TEST_F(SupervisedUserExtensionTest,
+                       RemovingSupervisionCustodianApprovalRequired) {
+  EXPECT_FALSE(profile()->IsChild());
+  // The extension should still be installed since we are sharing the same data
+  // directory as the PRE test.
+  const Extension* extension =
+      extension_registry()->GetInstalledExtension(kGoodCrxId);
+  EXPECT_TRUE(extension);
+  // The extension should be enabled now after removing supervision.
+  EXPECT_TRUE(extension_registry()->enabled_extensions().Contains(kGoodCrxId));
+  EXPECT_FALSE(IsDisabledForCustodianApproval(kGoodCrxId));
+}
+
+}  // namespace extensions

chrome/test/BUILD.gn

@@ -2722,6 +2722,11 @@ if (!is_android) {
         "../browser/supervised_user/supervised_user_service_browsertest.cc",
         "../browser/supervised_user/supervised_user_url_filter_browsertest.cc",
       ]
+      if (enable_extensions) {
+        sources += [
+          "../browser/supervised_user/supervised_user_extension_browsertest.cc",
+        ]
+      }
     }
     if (is_linux && is_asan && is_lsan) {
       # TODO(crbug.com/793426): Fix the flakiness on Linux Asan Lsan bot.