Handle the managed ADB sideloading feature flag

Pass the feature flag's value to the UI so that the corresponding UI
toggle can display the correct state (enabled/disabled).

Additionally check the feature flag value when checking eligibility for
enablind and disabling ADB sideloading.

--enable-features=ArcManagedAdbSideloadingSupport parameter in
/etc/chrome_dev.conf and check that in this case the toggle in settings
is enabled and ADB sideloading can be activated.

Test: Powerwash device and initialize with a managed account. Pass the
Bug: 1058386
Change-Id: Ic172b8a8a90c485f86aa26d59f2710844a71253c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2084554Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Victor Hsieh <victorhsieh@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Joel Hockey <joelhockey@chromium.org>
Commit-Queue: Jana Grill <janagrill@chromium.org>
Cr-Commit-Position: refs/heads/master@{#753579}

chrome/browser/chromeos/crostini/crostini_features.cc

@@ -5,12 +5,16 @@
 #include "chrome/browser/chromeos/crostini/crostini_features.h"
 
 #include "base/feature_list.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/browser_process_platform_part.h"
 #include "chrome/browser/chromeos/crostini/crostini_manager.h"
 #include "chrome/browser/chromeos/crostini/crostini_pref_names.h"
 #include "chrome/browser/chromeos/crostini/crostini_util.h"
+#include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chrome/browser/chromeos/virtual_machines/virtual_machines_util.h"
+#include "chrome/browser/policy/profile_policy_connector.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_features.h"
 #include "chromeos/constants/chromeos_features.h"
@@ -56,6 +60,39 @@ bool IsAllowedImpl(Profile* profile) {
   return base::FeatureList::IsEnabled(features::kCrostini);
 }
 
+bool IsArcManagedAdbSideloadingSupported(bool is_device_enterprise_managed,
+                                         bool is_profile_enterprise_managed,
+                                         bool is_owner_profile) {
+  DCHECK(is_device_enterprise_managed || is_profile_enterprise_managed);
+
+  if (!base::FeatureList::IsEnabled(
+          chromeos::features::kArcManagedAdbSideloadingSupport)) {
+    DVLOG(1) << "adb sideloading is disabled by a feature flag";
+    return false;
+  }
+
+  if (is_device_enterprise_managed) {
+    // TODO(janagrill): Add check for device policy
+    if (is_profile_enterprise_managed) {
+      // TODO(janagrill): Add check for affiliated user
+      // TODO(janagrill): Add check for user policy
+      return true;
+    }
+
+    DVLOG(1) << "adb sideloading is unsupported for this managed device";
+    return false;
+  }
+
+  if (is_owner_profile) {
+    // We know here that the profile is enterprise-managed so no need to check
+    // TODO(janagrill): Add check for user policy
+    return true;
+  }
+
+  DVLOG(1) << "Only the owner can change adb sideloading status";
+  return false;
+}
+
 }  // namespace
 
 namespace crostini {
@@ -128,4 +165,35 @@ bool CrostiniFeatures::IsContainerUpgradeUIAllowed(Profile* profile) {
              chromeos::features::kCrostiniWebUIUpgrader);
 }
 
+bool CrostiniFeatures::CanChangeAdbSideloading(Profile* profile) {
+  // First rule out a child account as it is a special case - a child can be an
+  // owner, but ADB sideloading is currently not supported for this case
+  if (profile->IsChild()) {
+    DVLOG(1) << "adb sideloading is currently unsupported for child accounts";
+    return false;
+  }
+
+  // Check the managed device and/or user case
+  auto* connector =
+      g_browser_process->platform_part()->browser_policy_connector_chromeos();
+  bool is_device_enterprise_managed = connector->IsEnterpriseManaged();
+  bool is_profile_enterprise_managed =
+      profile->GetProfilePolicyConnector()->IsManaged();
+  bool is_owner_profile = chromeos::ProfileHelper::IsOwnerProfile(profile);
+  if (is_device_enterprise_managed || is_profile_enterprise_managed) {
+    return IsArcManagedAdbSideloadingSupported(is_device_enterprise_managed,
+                                               is_profile_enterprise_managed,
+                                               is_owner_profile);
+  }
+
+  // Here we are sure that the user is not enterprise-managed and we therefore
+  // only check whether the user is the owner
+  if (!is_owner_profile) {
+    DVLOG(1) << "Only the owner can change adb sideloading status";
+    return false;
+  }
+
+  return true;
+}
+
 }  // namespace crostini

chrome/browser/chromeos/crostini/crostini_features.h

@@ -43,6 +43,11 @@ class CrostiniFeatures {
   // Returns true if container upgrade ui is allowed by flag.
   virtual bool IsContainerUpgradeUIAllowed(Profile*);
 
+  // Returns whether the user is allowed to enable and disable ADB sideloading
+  // based on whether the user is the owner, whether the user and the device
+  // are managed, and feature flag and policies for managed case.
+  virtual bool CanChangeAdbSideloading(Profile* profile);
+
   // TODO(crbug.com/1004708): Move other functions from crostini_util to here.
 
  protected:

chrome/browser/chromeos/crostini/crostini_features_unittest.cc

@@ -7,10 +7,15 @@
 #include "base/test/scoped_feature_list.h"
 #include "chrome/browser/chromeos/crostini/crostini_pref_names.h"
 #include "chrome/browser/chromeos/crostini/fake_crostini_features.h"
+#include "chrome/browser/chromeos/login/users/fake_chrome_user_manager.h"
+#include "chrome/browser/chromeos/profiles/profile_helper.h"
+#include "chrome/browser/policy/profile_policy_connector.h"
 #include "chrome/common/chrome_features.h"
 #include "chrome/test/base/testing_profile.h"
 #include "chromeos/constants/chromeos_features.h"
 #include "components/prefs/pref_service.h"
+#include "components/user_manager/fake_user_manager.h"
+#include "components/user_manager/scoped_user_manager.h"
 #include "content/public/test/browser_task_environment.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -80,4 +85,116 @@ TEST(CrostiniFeaturesTest, TestRootAccessAllowed) {
   }
 }
 
+TEST(CrostiniFeaturesTest, TestCanChangeAdbSideloadingChildUser) {
+  content::BrowserTaskEnvironment task_environment;
+  TestingProfile profile;
+  FakeCrostiniFeatures crostini_features;
+
+  auto scoped_user_manager = std::make_unique<user_manager::ScopedUserManager>(
+      std::make_unique<chromeos::FakeChromeUserManager>());
+  auto* user_manager = static_cast<chromeos::FakeChromeUserManager*>(
+      user_manager::UserManager::Get());
+  AccountId account_id = AccountId::FromUserEmail(profile.GetProfileUserName());
+  auto* const user = user_manager->AddChildUser(account_id);
+  user_manager->UserLoggedIn(account_id, user->username_hash(),
+                             /*browser_restart=*/false,
+                             /*is_child=*/true);
+
+  EXPECT_FALSE(crostini_features.CanChangeAdbSideloading(&profile));
+}
+
+TEST(CrostiniFeaturesTest,
+     TestCanChangeAdbSideloadingManagedDisabledFeatureFlag) {
+  content::BrowserTaskEnvironment task_environment;
+  TestingProfile profile;
+  FakeCrostiniFeatures crostini_features;
+
+  // Disable feature flag
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitWithFeatures(
+      {}, {chromeos::features::kArcManagedAdbSideloadingSupport});
+
+  EXPECT_FALSE(crostini_features.CanChangeAdbSideloading(&profile));
+}
+
+TEST(CrostiniFeaturesTest, TestCanChangeAdbSideloadingManagedDeviceAndUser) {
+  content::BrowserTaskEnvironment task_environment;
+  TestingProfile profile;
+  FakeCrostiniFeatures crostini_features;
+
+  // Enable feature flag
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitWithFeatures(
+      {chromeos::features::kArcManagedAdbSideloadingSupport}, {});
+
+  // Set device to enterprise-managed
+  profile.ScopedCrosSettingsTestHelper()->InstallAttributes()->SetCloudManaged(
+      "domain.com", "device_id");
+
+  // Set profile to enterprise-managed
+  profile.GetProfilePolicyConnector()->OverrideIsManagedForTesting(true);
+
+  // TODO(janagrill): Once the device and user policies are implemented,
+  // adjust the test accordingly
+  EXPECT_TRUE(crostini_features.CanChangeAdbSideloading(&profile));
+}
+
+TEST(CrostiniFeaturesTest, TestCanChangeAdbSideloadingOwnerProfile) {
+  content::BrowserTaskEnvironment task_environment;
+  TestingProfile profile;
+  FakeCrostiniFeatures crostini_features;
+
+  // Set device to not enterprise-managed
+  profile.ScopedCrosSettingsTestHelper()
+      ->InstallAttributes()
+      ->SetConsumerOwned();
+
+  // Set profile to not enterprise-managed
+  profile.GetProfilePolicyConnector()->OverrideIsManagedForTesting(false);
+
+  // Set profile to owner
+  auto scoped_user_manager = std::make_unique<user_manager::ScopedUserManager>(
+      std::make_unique<chromeos::FakeChromeUserManager>());
+  auto* user_manager = static_cast<chromeos::FakeChromeUserManager*>(
+      user_manager::UserManager::Get());
+  AccountId account_id = AccountId::FromUserEmail(profile.GetProfileUserName());
+  user_manager->AddUser(account_id);
+  user_manager->LoginUser(account_id);
+  user_manager->SetOwnerId(account_id);
+
+  EXPECT_TRUE(crostini_features.CanChangeAdbSideloading(&profile));
+}
+
+TEST(CrostiniFeaturesTest, TestCanChangeAdbSideloadingOwnerProfileManagedUser) {
+  content::BrowserTaskEnvironment task_environment;
+  TestingProfile profile;
+  FakeCrostiniFeatures crostini_features;
+
+  // Enable feature flag
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitWithFeatures(
+      {chromeos::features::kArcManagedAdbSideloadingSupport}, {});
+
+  // Set device to not enterprise-managed
+  profile.ScopedCrosSettingsTestHelper()
+      ->InstallAttributes()
+      ->SetConsumerOwned();
+
+  // Set profile to enterprise-managed
+  profile.GetProfilePolicyConnector()->OverrideIsManagedForTesting(true);
+
+  // Set profile to owner
+  auto scoped_user_manager = std::make_unique<user_manager::ScopedUserManager>(
+      std::make_unique<chromeos::FakeChromeUserManager>());
+  auto* user_manager = static_cast<chromeos::FakeChromeUserManager*>(
+      user_manager::UserManager::Get());
+  AccountId account_id = AccountId::FromUserEmail(profile.GetProfileUserName());
+  user_manager->AddUser(account_id);
+  user_manager->LoginUser(account_id);
+  user_manager->SetOwnerId(account_id);
+
+  // TODO(janagrill): Once the user policy is implemented, adjust the test
+  EXPECT_TRUE(crostini_features.CanChangeAdbSideloading(&profile));
+}
+
 }  // namespace crostini

chrome/browser/chromeos/crostini/fake_crostini_features.cc

@@ -16,39 +16,45 @@ FakeCrostiniFeatures::~FakeCrostiniFeatures() {
 }
 
 bool FakeCrostiniFeatures::IsAllowed(Profile* profile) {
-  if (allowed_set_)
-    return allowed_;
+  if (allowed_.has_value())
+    return *allowed_;
   return original_features_->IsAllowed(profile);
 }
 
 bool FakeCrostiniFeatures::IsUIAllowed(Profile* profile, bool check_policy) {
-  if (ui_allowed_set_)
-    return ui_allowed_;
+  if (ui_allowed_.has_value())
+    return *ui_allowed_;
   return original_features_->IsUIAllowed(profile, check_policy);
 }
 
 bool FakeCrostiniFeatures::IsEnabled(Profile* profile) {
-  if (enabled_set_)
-    return enabled_;
+  if (enabled_.has_value())
+    return *enabled_;
   return original_features_->IsEnabled(profile);
 }
 
 bool FakeCrostiniFeatures::IsExportImportUIAllowed(Profile* profile) {
-  if (export_import_ui_allowed_set_)
-    return export_import_ui_allowed_;
+  if (export_import_ui_allowed_.has_value())
+    return *export_import_ui_allowed_;
   return original_features_->IsExportImportUIAllowed(profile);
 }
 
 bool FakeCrostiniFeatures::IsRootAccessAllowed(Profile* profile) {
-  if (root_access_allowed_set_)
-    return root_access_allowed_;
+  if (root_access_allowed_.has_value())
+    return *root_access_allowed_;
   return original_features_->IsRootAccessAllowed(profile);
 }
 
 bool FakeCrostiniFeatures::IsContainerUpgradeUIAllowed(Profile* profile) {
-  if (container_upgrade_ui_allowed_set_)
-    return container_upgrade_ui_allowed_;
+  if (container_upgrade_ui_allowed_.has_value())
+    return *container_upgrade_ui_allowed_;
   return original_features_->IsContainerUpgradeUIAllowed(profile);
 }
 
+bool FakeCrostiniFeatures::CanChangeAdbSideloading(Profile* profile) {
+  if (can_change_adb_sideloading_.has_value())
+    return *can_change_adb_sideloading_;
+  return original_features_->CanChangeAdbSideloading(profile);
+}
+
 }  // namespace crostini

chrome/browser/chromeos/crostini/fake_crostini_features.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_CHROMEOS_CROSTINI_FAKE_CROSTINI_FEATURES_H_
 
 #include "base/macros.h"
+#include "base/optional.h"
 #include "chrome/browser/chromeos/crostini/crostini_features.h"
 
 class Profile;
@@ -28,49 +29,43 @@ class FakeCrostiniFeatures : public CrostiniFeatures {
   bool IsExportImportUIAllowed(Profile* profile) override;
   bool IsRootAccessAllowed(Profile* profile) override;
   bool IsContainerUpgradeUIAllowed(Profile*) override;
+  bool CanChangeAdbSideloading(Profile* profile) override;
 
   void set_allowed(bool allowed) {
-    allowed_set_ = true;
     allowed_ = allowed;
   }
   void set_ui_allowed(bool allowed) {
-    ui_allowed_set_ = true;
     ui_allowed_ = allowed;
   }
   void set_enabled(bool enabled) {
-    enabled_set_ = true;
     enabled_ = enabled;
   }
   void set_export_import_ui_allowed(bool allowed) {
-    export_import_ui_allowed_set_ = true;
     export_import_ui_allowed_ = allowed;
   }
   void set_root_access_allowed(bool allowed) {
-    root_access_allowed_set_ = true;
     root_access_allowed_ = allowed;
   }
-  void container_upgrade_ui_allowed(bool allowed) {
-    container_upgrade_ui_allowed_set_ = true;
+  void set_container_upgrade_ui_allowed(bool allowed) {
     container_upgrade_ui_allowed_ = allowed;
   }
 
+  void set_can_change_adb_sideloading(bool can_change) {
+    can_change_adb_sideloading_ = can_change;
+  }
+
  private:
   // Original global static when this instance is created. It is captured when
   // FakeCrostiniFeatures is created and replaced at destruction.
   CrostiniFeatures* original_features_;
 
-  bool allowed_set_ = false;
-  bool allowed_ = false;
-  bool ui_allowed_set_ = false;
-  bool ui_allowed_ = false;
-  bool enabled_ = false;
-  bool enabled_set_ = false;
-  bool export_import_ui_allowed_set_ = false;
-  bool export_import_ui_allowed_ = false;
-  bool root_access_allowed_set_ = false;
-  bool root_access_allowed_ = false;
-  bool container_upgrade_ui_allowed_set_ = false;
-  bool container_upgrade_ui_allowed_ = false;
+  base::Optional<bool> allowed_;
+  base::Optional<bool> ui_allowed_;
+  base::Optional<bool> enabled_;
+  base::Optional<bool> export_import_ui_allowed_;
+  base::Optional<bool> root_access_allowed_;
+  base::Optional<bool> container_upgrade_ui_allowed_;
+  base::Optional<bool> can_change_adb_sideloading_;
 };
 
 }  // namespace crostini

chrome/browser/resources/settings/chromeos/crostini_page/crostini_arc_adb.html

@@ -36,7 +36,7 @@
           isOwnerProfile_, isEnterpriseManaged_)]]"></cr-policy-indicator>
       <cr-toggle id="arcAdbEnabledButton" aria-labelledby="enableArcAdbLabel"
           checked$="[[arcAdbEnabled_]]"
-          disabled="[[shouldDisable_(isOwnerProfile_, isEnterpriseManaged_,
+          disabled="[[shouldDisable_(canChangeAdbSideloading_,
                       arcAdbNeedPowerwash_)]]"
           on-change="onArcAdbToggleChanged_">
       </cr-toggle>

chrome/browser/resources/settings/chromeos/crostini_page/crostini_arc_adb.js

@@ -46,6 +46,14 @@ Polymer({
       },
     },
 
+    /** @private {boolean} */
+    canChangeAdbSideloading_: {
+      type: Boolean,
+      value() {
+        return loadTimeData.getBoolean('canChangeAdbSideloading');
+      },
+    },
+
     /** @private {boolean} */
     showConfirmationDialog_: {
       type: Boolean,
@@ -65,34 +73,41 @@ Polymer({
   },
 
   /**
-   * Returns whether the toggle is changeable to the user. Only the device owner
-   * is able to change it. Note that the actual guard should be in browser,
-   * otherwise a user may bypass this check by inspecting Settings with
-   * developer tool.
+   * Returns whether the toggle is changeable by the user. See
+   * CrostiniFeatures::CanChangeAdbSideloading(). Note that the actual
+   * guard should be in the browser, otherwise a user may bypass this check by
+   * inspecting Settings with developer tools.
+   * @return {boolean} Whether the control should be disabled.
    * @private
    */
-  shouldDisable_(isOwnerProfile, isEnterpriseManaged, arcAdbNeedPowerwash) {
-    return !isOwnerProfile || isEnterpriseManaged || arcAdbNeedPowerwash;
+  shouldDisable_() {
+    return !this.canChangeAdbSideloading_ || this.arcAdbNeedPowerwash_;
   },
 
-  /** @private */
-  getPolicyIndicatorType_(isOwnerProfile, isEnterpriseManaged) {
-    if (isEnterpriseManaged) {
+  /**
+   * @return {CrPolicyIndicatorType} Which policy indicator to show (if any).
+   * @private
+   */
+  getPolicyIndicatorType_() {
+    if (this.isEnterpriseManaged_) {
       return CrPolicyIndicatorType.DEVICE_POLICY;
-    } else if (!isOwnerProfile) {
+    } else if (!this.isOwnerProfile_) {
       return CrPolicyIndicatorType.OWNER;
     } else {
       return CrPolicyIndicatorType.NONE;
     }
   },
 
-  /** @private */
-  getToggleAction_(arcAdbEnabled) {
-    return arcAdbEnabled ? 'disable' : 'enable';
+  /**
+   * @return {string} Which action to perform when the toggle is changed.
+   * @private
+   */
+  getToggleAction_() {
+    return this.arcAdbEnabled_ ? 'disable' : 'enable';
   },
 
   /** @private */
-  onArcAdbToggleChanged_(event) {
+  onArcAdbToggleChanged_() {
     this.showConfirmationDialog_ = true;
   },
 

chrome/browser/ui/views/crostini/crostini_browser_test_util.cc

@@ -94,7 +94,7 @@ CrostiniDialogBrowserTest::CrostiniDialogBrowserTest(bool register_termina)
   fake_crostini_features_.set_enabled(true);
   fake_crostini_features_.set_export_import_ui_allowed(true);
   fake_crostini_features_.set_root_access_allowed(true);
-  fake_crostini_features_.container_upgrade_ui_allowed(true);
+  fake_crostini_features_.set_container_upgrade_ui_allowed(true);
 }
 
 CrostiniDialogBrowserTest::~CrostiniDialogBrowserTest() = default;

chrome/browser/ui/webui/settings/chromeos/crostini_handler.cc

@@ -12,18 +12,16 @@
 #include "base/callback_forward.h"
 #include "base/metrics/histogram_functions.h"
 #include "chrome/browser/browser_process.h"
-#include "chrome/browser/browser_process_platform_part.h"
 #include "chrome/browser/chromeos/crostini/crostini_disk.h"
+#include "chrome/browser/chromeos/crostini/crostini_features.h"
 #include "chrome/browser/chromeos/crostini/crostini_installer.h"
 #include "chrome/browser/chromeos/crostini/crostini_port_forwarder.h"
 #include "chrome/browser/chromeos/crostini/crostini_types.mojom.h"
 #include "chrome/browser/chromeos/crostini/crostini_util.h"
 #include "chrome/browser/chromeos/file_manager/path_util.h"
 #include "chrome/browser/chromeos/guest_os/guest_os_share_path.h"
-#include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
-#include "chrome/browser/policy/profile_policy_connector.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/webui/chromeos/crostini_upgrader/crostini_upgrader_dialog.h"
 #include "chrome/common/pref_names.h"
@@ -412,28 +410,7 @@ void CrostiniHandler::HandleDisableArcAdbRequest(const base::ListValue* args) {
 }
 
 bool CrostiniHandler::CheckEligibilityToChangeArcAdbSideloading() const {
-  if (!chromeos::ProfileHelper::IsOwnerProfile(profile_)) {
-    DVLOG(1) << "Only the owner can change adb sideloading status";
-    return false;
-  }
-
-  if (user_manager::UserManager::Get()->IsLoggedInAsChildUser()) {
-    DVLOG(1) << "adb sideloading is currently unsupported for child account";
-    return false;
-  }
-
-  if (profile_->GetProfilePolicyConnector()->IsManaged()) {
-    DVLOG(1) << "adb sideloading is currently unsupported for managed user";
-    return false;
-  }
-
-  policy::BrowserPolicyConnectorChromeOS* connector =
-      g_browser_process->platform_part()->browser_policy_connector_chromeos();
-  if (connector->IsEnterpriseManaged()) {
-    DVLOG(1) << "adb sideloading is currently unsupported on managed device";
-    return false;
-  }
-  return true;
+  return crostini::CrostiniFeatures::Get()->CanChangeAdbSideloading(profile_);
 }
 
 void CrostiniHandler::LaunchTerminal() {

chrome/browser/ui/webui/settings/chromeos/os_settings_localized_strings_provider.cc

@@ -760,6 +760,9 @@ void AddCrostiniStrings(content::WebUIDataSource* html_source,
                           chromeos::ProfileHelper::IsOwnerProfile(profile));
   html_source->AddBoolean("isEnterpriseManaged",
                           IsDeviceManaged() || IsProfileManaged(profile));
+  html_source->AddBoolean(
+      "canChangeAdbSideloading",
+      crostini::CrostiniFeatures::Get()->CanChangeAdbSideloading(profile));
   html_source->AddBoolean("showCrostiniContainerUpgrade",
                           crostini::ShouldAllowContainerUpgrade(profile));
   html_source->AddBoolean(