Move crostini_util IsCrostini[UI]AlllowedForProfile to CrostiniFeatures

Refactor only change TBR=benwells@chromium.org TBR=stevenjb@chromium.org Bug: 1004708 Change-Id: Icc6e0d686cde3071eb78246089f62440f317c829 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1893475 Commit-Queue: Joel Hockey <joelhockey@chromium.org> Reviewed-by: Nicholas Verne <nverne@chromium.org> Cr-Commit-Position: refs/heads/master@{#712037}
chromium · Nov 4, 2019 · 5b24a95 · 5b24a95
1 parent 642bac8
commit 5b24a95
Show file tree

Hide file tree

Showing 22 changed files with 130 additions and 123 deletions.
diff --git a/chrome/browser/apps/app_service/crostini_apps.cc b/chrome/browser/apps/app_service/crostini_apps.cc
@@ -59,7 +59,7 @@ void CrostiniApps::ReInitializeForTesting(
 void CrostiniApps::Initialize(
     const mojo::Remote<apps::mojom::AppService>& app_service) {
   DCHECK(profile_);
-  if (!crostini::IsCrostiniUIAllowedForProfile(profile_)) {
+  if (!crostini::CrostiniFeatures::Get()->IsUIAllowed(profile_)) {
     return;
   }
   registry_ = crostini::CrostiniRegistryServiceFactory::GetForProfile(profile_);

diff --git a/chrome/browser/chromeos/crostini/crostini_features.cc b/chrome/browser/chromeos/crostini/crostini_features.cc
@@ -5,13 +5,59 @@
 #include "chrome/browser/chromeos/crostini/crostini_features.h"
 
 #include "base/feature_list.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/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/profiles/profile.h"
 #include "chrome/common/chrome_features.h"
 #include "chromeos/constants/chromeos_features.h"
+#include "chromeos/constants/chromeos_switches.h"
 #include "components/prefs/pref_service.h"
 
+namespace {
+
+bool IsUnaffiliatedCrostiniAllowedByPolicy() {
+  bool unaffiliated_crostini_allowed;
+  if (chromeos::CrosSettings::Get()->GetBoolean(
+          chromeos::kDeviceUnaffiliatedCrostiniAllowed,
+          &unaffiliated_crostini_allowed)) {
+    return unaffiliated_crostini_allowed;
+  }
+  // If device policy is not set, allow Crostini.
+  return true;
+}
+
+bool IsAllowedImpl(Profile* profile) {
+  if (!profile || profile->IsChild() || profile->IsLegacySupervised() ||
+      profile->IsOffTheRecord() ||
+      chromeos::ProfileHelper::IsEphemeralUserProfile(profile) ||
+      chromeos::ProfileHelper::IsLockScreenAppProfile(profile)) {
+    return false;
+  }
+  if (!crostini::CrostiniManager::IsDevKvmPresent()) {
+    // Hardware is physically incapable, no matter what the user wants.
+    return false;
+  }
+
+  bool kernelnext = base::CommandLine::ForCurrentProcess()->HasSwitch(
+      chromeos::switches::kKernelnextRestrictVMs);
+  bool kernelnext_override =
+      base::FeatureList::IsEnabled(features::kKernelnextVMs);
+  if (kernelnext && !kernelnext_override) {
+    // The host kernel is on an experimental version. In future updates this
+    // device may not have VM support, so we allow enabling VMs, but guard them
+    // on a chrome://flags switch (enable-experimental-kernel-vm-support).
+    return false;
+  }
+
+  return base::FeatureList::IsEnabled(features::kCrostini);
+}
+
+}  // namespace
+
 namespace crostini {
 
 static CrostiniFeatures* g_crostini_features = nullptr;
@@ -31,10 +77,30 @@ CrostiniFeatures::CrostiniFeatures() = default;
 
 CrostiniFeatures::~CrostiniFeatures() = default;
 
+bool CrostiniFeatures::IsAllowed(Profile* profile) {
+  const user_manager::User* user =
+      chromeos::ProfileHelper::Get()->GetUserByProfile(profile);
+  if (!IsUnaffiliatedCrostiniAllowedByPolicy() && !user->IsAffiliated()) {
+    return false;
+  }
+  if (!profile->GetPrefs()->GetBoolean(
+          crostini::prefs::kUserCrostiniAllowedByPolicy)) {
+    return false;
+  }
+  if (!virtual_machines::AreVirtualMachinesAllowedByPolicy()) {
+    return false;
+  }
+  return IsAllowedImpl(profile);
+}
+
 bool CrostiniFeatures::IsUIAllowed(Profile* profile, bool check_policy) {
-  // TODO(crbug.com/1004708): Move implementation from crostini_util to here and
-  // redirect all callers.
-  return crostini::IsCrostiniUIAllowedForProfile(profile);
+  if (!chromeos::ProfileHelper::IsPrimaryProfile(profile)) {
+    return false;
+  }
+  if (check_policy) {
+    return g_crostini_features->IsAllowed(profile);
+  }
+  return IsAllowedImpl(profile);
 }
 
 bool CrostiniFeatures::IsEnabled(Profile* profile) {

diff --git a/chrome/browser/chromeos/crostini/crostini_features.h b/chrome/browser/chromeos/crostini/crostini_features.h
@@ -17,6 +17,11 @@ class CrostiniFeatures {
  public:
   static CrostiniFeatures* Get();
 
+  // Returns true if crostini is allowed to run for |profile|.
+  // Otherwise, returns false, e.g. if crostini is not available on the device,
+  // or it is in the flow to set up managed account creation.
+  virtual bool IsAllowed(Profile* profile);
+
   // When |check_policy| is true, returns true if fully interactive crostini UI
   // may be shown. Implies crostini is allowed to run.
   // When check_policy is false, returns true if crostini UI is not forbidden by

diff --git a/chrome/browser/chromeos/crostini/crostini_manager.cc b/chrome/browser/chromeos/crostini/crostini_manager.cc
@@ -722,7 +722,7 @@ void CrostiniManager::MaybeUpgradeCrostiniAfterChecks() {
   if (!is_cros_termina_registered_) {
     return;
   }
-  if (!IsCrostiniAllowedForProfile(profile_)) {
+  if (!CrostiniFeatures::Get()->IsAllowed(profile_)) {
     return;
   }
   termina_update_check_needed_ = true;

diff --git a/chrome/browser/chromeos/crostini/crostini_package_service.cc b/chrome/browser/chromeos/crostini/crostini_package_service.cc
@@ -10,9 +10,9 @@
 #include "base/stl_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/chromeos/crostini/crostini_features.h"
 #include "chrome/browser/chromeos/crostini/crostini_manager_factory.h"
 #include "chrome/browser/chromeos/crostini/crostini_registry_service_factory.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/profiles/profile.h"
@@ -473,7 +473,7 @@ void CrostiniPackageService::UninstallApplication(
   const ContainerId container_id(vm_name, container_name);
 
   // Policies can change under us, and crostini may now be forbidden.
-  if (!IsCrostiniUIAllowedForProfile(profile_)) {
+  if (!CrostiniFeatures::Get()->IsUIAllowed(profile_)) {
     LOG(ERROR) << "Can't uninstall because policy no longer allows Crostini";
     UpdatePackageOperationStatus(container_id, PackageOperationStatus::FAILED,
                                  0);

diff --git a/chrome/browser/chromeos/crostini/crostini_util.cc b/chrome/browser/chromeos/crostini/crostini_util.cc
@@ -25,7 +25,6 @@
 #include "chrome/browser/chromeos/file_manager/path_util.h"
 #include "chrome/browser/chromeos/guest_os/guest_os_share_path.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/profiles/profile.h"
 #include "chrome/browser/ui/app_list/crostini/crostini_app_icon.h"
@@ -38,7 +37,6 @@
 #include "chrome/common/chrome_features.h"
 #include "chrome/grit/generated_resources.h"
 #include "chromeos/constants/chromeos_features.h"
-#include "chromeos/constants/chromeos_switches.h"
 #include "components/prefs/pref_service.h"
 #include "components/user_manager/user.h"
 #include "google_apis/gaia/gaia_auth_util.h"
@@ -268,32 +266,6 @@ class IconLoadWaiter : public CrostiniAppIcon::Observer {
   base::OnceCallback<void(const std::vector<gfx::ImageSkia>&)> callback_;
 };
 
-bool IsCrostiniAllowedForProfileImpl(Profile* profile) {
-  if (!profile || profile->IsChild() || profile->IsLegacySupervised() ||
-      profile->IsOffTheRecord() ||
-      chromeos::ProfileHelper::IsEphemeralUserProfile(profile) ||
-      chromeos::ProfileHelper::IsLockScreenAppProfile(profile)) {
-    return false;
-  }
-  if (!crostini::CrostiniManager::IsDevKvmPresent()) {
-    // Hardware is physically incapable, no matter what the user wants.
-    return false;
-  }
-
-  bool kernelnext = base::CommandLine::ForCurrentProcess()->HasSwitch(
-      chromeos::switches::kKernelnextRestrictVMs);
-  bool kernelnext_override =
-      base::FeatureList::IsEnabled(features::kKernelnextVMs);
-  if (kernelnext && !kernelnext_override) {
-    // The host kernel is on an experimental version. In future updates this
-    // device may not have VM support, so we allow enabling VMs, but guard them
-    // on a chrome://flags switch (enable-experimental-kernel-vm-support).
-    return false;
-  }
-
-  return base::FeatureList::IsEnabled(features::kCrostini);
-}
-
 }  // namespace
 
 namespace crostini {
@@ -327,32 +299,6 @@ bool IsUninstallable(Profile* profile, const std::string& app_id) {
   return false;
 }
 
-bool IsCrostiniAllowedForProfile(Profile* profile) {
-  const user_manager::User* user =
-      chromeos::ProfileHelper::Get()->GetUserByProfile(profile);
-  if (!IsUnaffiliatedCrostiniAllowedByPolicy() && !user->IsAffiliated()) {
-    return false;
-  }
-  if (!profile->GetPrefs()->GetBoolean(
-          crostini::prefs::kUserCrostiniAllowedByPolicy)) {
-    return false;
-  }
-  if (!virtual_machines::AreVirtualMachinesAllowedByPolicy()) {
-    return false;
-  }
-  return IsCrostiniAllowedForProfileImpl(profile);
-}
-
-bool IsCrostiniUIAllowedForProfile(Profile* profile, bool check_policy) {
-  if (!chromeos::ProfileHelper::IsPrimaryProfile(profile)) {
-    return false;
-  }
-  if (check_policy) {
-    return IsCrostiniAllowedForProfile(profile);
-  }
-  return IsCrostiniAllowedForProfileImpl(profile);
-}
-
 bool IsCrostiniRunning(Profile* profile) {
   return crostini::CrostiniManager::GetForProfile(profile)->IsVmRunning(
       kCrostiniDefaultVmName);
@@ -393,7 +339,7 @@ void LaunchCrostiniApp(Profile* profile,
                        const std::vector<storage::FileSystemURL>& files,
                        LaunchCrostiniAppCallback callback) {
   // Policies can change under us, and crostini may now be forbidden.
-  if (!IsCrostiniUIAllowedForProfile(profile)) {
+  if (!CrostiniFeatures::Get()->IsUIAllowed(profile)) {
     return std::move(callback).Run(false, "Crostini UI not allowed");
   }
   auto* crostini_manager = crostini::CrostiniManager::GetForProfile(profile);
@@ -503,17 +449,6 @@ base::Optional<std::string> CrostiniAppIdFromAppName(
   return app_name.substr(strlen(kCrostiniAppNamePrefix));
 }
 
-bool IsUnaffiliatedCrostiniAllowedByPolicy() {
-  bool unaffiliated_crostini_allowed;
-  if (chromeos::CrosSettings::Get()->GetBoolean(
-          chromeos::kDeviceUnaffiliatedCrostiniAllowed,
-          &unaffiliated_crostini_allowed)) {
-    return unaffiliated_crostini_allowed;
-  }
-  // If device policy is not set, allow Crostini.
-  return true;
-}
-
 void AddNewLxdContainerToPrefs(Profile* profile,
                                std::string vm_name,
                                std::string container_name) {

diff --git a/chrome/browser/chromeos/crostini/crostini_util.h b/chrome/browser/chromeos/crostini/crostini_util.h
@@ -55,18 +55,6 @@ using LaunchCrostiniAppCallback =
 // Checks if user profile is able to a crostini app with a given app_id.
 bool IsUninstallable(Profile* profile, const std::string& app_id);
 
-// Returns true if crostini is allowed to run for |profile|.
-// Otherwise, returns false, e.g. if crostini is not available on the device,
-// or it is in the flow to set up managed account creation.
-bool IsCrostiniAllowedForProfile(Profile* profile);
-
-// When |check_policy| is true, returns true if fully interactive crostini UI
-// may be shown. Implies crostini is allowed to run.
-// When check_policy is false, returns true if crostini UI is not forbidden by
-// hardware, flags, etc, even if it is forbidden by the enterprise policy. The
-// UI uses this to indicate that crostini is available but disabled by policy.
-bool IsCrostiniUIAllowedForProfile(Profile* profile, bool check_policy = true);
-
 // Returns whether the default Crostini VM is running for the user.
 bool IsCrostiniRunning(Profile* profile);
 
@@ -184,10 +172,6 @@ constexpr char kCrostiniBusterImageAlias[] = "debian/buster";
 constexpr base::FilePath::CharType kHomeDirectory[] =
     FILE_PATH_LITERAL("/home");
 
-// Whether running Crostini is allowed for unaffiliated users per enterprise
-// policy.
-bool IsUnaffiliatedCrostiniAllowedByPolicy();
-
 // Add a newly created LXD container to the kCrostiniContainers pref
 void AddNewLxdContainerToPrefs(Profile* profile,
                                std::string vm_name,

diff --git a/chrome/browser/chromeos/crostini/fake_crostini_features.cc b/chrome/browser/chromeos/crostini/fake_crostini_features.cc
@@ -15,6 +15,12 @@ FakeCrostiniFeatures::~FakeCrostiniFeatures() {
   CrostiniFeatures::SetForTesting(original_features_);
 }
 
+bool FakeCrostiniFeatures::IsAllowed(Profile* profile) {
+  if (allowed_set_)
+    return allowed_;
+  return original_features_->IsAllowed(profile);
+}
+
 bool FakeCrostiniFeatures::IsUIAllowed(Profile* profile, bool check_policy) {
   if (ui_allowed_set_)
     return ui_allowed_;

diff --git a/chrome/browser/chromeos/crostini/fake_crostini_features.h b/chrome/browser/chromeos/crostini/fake_crostini_features.h
@@ -22,11 +22,16 @@ class FakeCrostiniFeatures : public CrostiniFeatures {
   ~FakeCrostiniFeatures() override;
 
   // CrostiniFeatures:
+  bool IsAllowed(Profile* profile) override;
   bool IsUIAllowed(Profile* profile, bool check_policy) override;
   bool IsEnabled(Profile* profile) override;
   bool IsExportImportUIAllowed(Profile* profile) override;
   bool IsRootAccessAllowed(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;
@@ -49,6 +54,8 @@ class FakeCrostiniFeatures : public CrostiniFeatures {
   // 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;

diff --git a/chrome/browser/chromeos/dbus/chrome_features_service_provider.cc b/chrome/browser/chromeos/dbus/chrome_features_service_provider.cc
@@ -12,8 +12,8 @@
 #include "base/bind.h"
 #include "base/feature_list.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/chromeos/crostini/crostini_features.h"
 #include "chrome/browser/chromeos/crostini/crostini_pref_names.h"
-#include "chrome/browser/chromeos/crostini/crostini_util.h"
 #include "chrome/browser/chromeos/plugin_vm/plugin_vm_util.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/profiles/profile_manager.h"
@@ -159,7 +159,7 @@ void ChromeFeaturesServiceProvider::IsCrostiniEnabled(
   Profile* profile = GetSenderProfile(method_call, response_sender);
   SendResponse(
       method_call, response_sender,
-      profile ? crostini::IsCrostiniAllowedForProfile(profile) : false);
+      profile ? crostini::CrostiniFeatures::Get()->IsAllowed(profile) : false);
 }
 
 void ChromeFeaturesServiceProvider::IsPluginVmEnabled(

diff --git a/chrome/browser/chromeos/extensions/autotest_private/autotest_private_api.cc b/chrome/browser/chromeos/extensions/autotest_private/autotest_private_api.cc
@@ -59,12 +59,12 @@
 #include "chrome/browser/chromeos/arc/tracing/arc_app_performance_tracing_session.h"
 #include "chrome/browser/chromeos/assistant/assistant_util.h"
 #include "chrome/browser/chromeos/crostini/crostini_export_import.h"
+#include "chrome/browser/chromeos/crostini/crostini_features.h"
 #include "chrome/browser/chromeos/crostini/crostini_installer.h"
 #include "chrome/browser/chromeos/crostini/crostini_manager.h"
 #include "chrome/browser/chromeos/crostini/crostini_pref_names.h"
 #include "chrome/browser/chromeos/crostini/crostini_registry_service.h"
 #include "chrome/browser/chromeos/crostini/crostini_registry_service_factory.h"
-#include "chrome/browser/chromeos/crostini/crostini_util.h"
 #include "chrome/browser/chromeos/file_manager/path_util.h"
 #include "chrome/browser/chromeos/login/lock/screen_locker.h"
 #include "chrome/browser/chromeos/printing/cups_printers_manager.h"
@@ -1461,7 +1461,7 @@ AutotestPrivateSetCrostiniEnabledFunction::Run() {
   DVLOG(1) << "AutotestPrivateSetCrostiniEnabledFunction " << params->enabled;
 
   Profile* profile = Profile::FromBrowserContext(browser_context());
-  if (!crostini::IsCrostiniUIAllowedForProfile(profile))
+  if (!crostini::CrostiniFeatures::Get()->IsUIAllowed(profile))
     return RespondNow(Error(kCrostiniNotAvailableForCurrentUserError));
 
   // Set the preference to indicate Crostini is enabled/disabled.
@@ -1487,7 +1487,7 @@ AutotestPrivateRunCrostiniInstallerFunction::Run() {
   DVLOG(1) << "AutotestPrivateInstallCrostiniFunction";
 
   Profile* profile = Profile::FromBrowserContext(browser_context());
-  if (!crostini::IsCrostiniUIAllowedForProfile(profile))
+  if (!crostini::CrostiniFeatures::Get()->IsUIAllowed(profile))
     return RespondNow(Error(kCrostiniNotAvailableForCurrentUserError));
 
   // Run GUI installer which will install crostini vm / container and
@@ -1527,7 +1527,7 @@ AutotestPrivateRunCrostiniUninstallerFunction::Run() {
   DVLOG(1) << "AutotestPrivateRunCrostiniUninstallerFunction";
 
   Profile* profile = Profile::FromBrowserContext(browser_context());
-  if (!crostini::IsCrostiniUIAllowedForProfile(profile))
+  if (!crostini::CrostiniFeatures::Get()->IsUIAllowed(profile))
     return RespondNow(Error(kCrostiniNotAvailableForCurrentUserError));
 
   // Run GUI uninstaller which will remove crostini vm / container. We then
@@ -1563,7 +1563,7 @@ ExtensionFunction::ResponseAction AutotestPrivateExportCrostiniFunction::Run() {
   DVLOG(1) << "AutotestPrivateExportCrostiniFunction " << params->path;
 
   Profile* profile = Profile::FromBrowserContext(browser_context());
-  if (!crostini::IsCrostiniUIAllowedForProfile(profile)) {
+  if (!crostini::CrostiniFeatures::Get()->IsUIAllowed(profile)) {
     return RespondNow(Error(kCrostiniNotAvailableForCurrentUserError));
   }
 
@@ -1605,7 +1605,7 @@ ExtensionFunction::ResponseAction AutotestPrivateImportCrostiniFunction::Run() {
   DVLOG(1) << "AutotestPrivateImportCrostiniFunction " << params->path;
 
   Profile* profile = Profile::FromBrowserContext(browser_context());
-  if (!crostini::IsCrostiniUIAllowedForProfile(profile))
+  if (!crostini::CrostiniFeatures::Get()->IsUIAllowed(profile))
     return RespondNow(Error(kCrostiniNotAvailableForCurrentUserError));
 
   base::FilePath path(params->path);