Policy implementation for encryptfs to ext4 migration strategy

components/arc/arc_util.cc

Index: components/arc/arc_util.cc
diff --git a/components/arc/arc_util.cc b/components/arc/arc_util.cc
index 58133783c34f9ad075aba2bcb54fe94445592a1e..b2bb1768ff6b7cd64f4b1174980b2c55ca2025da 100644
--- a/components/arc/arc_util.cc
+++ b/components/arc/arc_util.cc
@@ -9,9 +9,11 @@
 #include "ash/shared/app_types.h"
 #include "base/command_line.h"
 #include "base/feature_list.h"
+#include "chrome/common/pref_names.h"
 #include "chromeos/chromeos_switches.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/session_manager_client.h"
+#include "components/prefs/pref_service.h"
 #include "components/user_manager/user_manager.h"
 #include "ui/aura/client/aura_constants.h"
 #include "ui/aura/window.h"
@@ -20,6 +22,15 @@ namespace arc {
 
 namespace {
 
+// This flag is set only in case the command line flag is set to mark the device
+// as requiring the migration. The value is set the first time the policy fetch
+// is done, and remains unchanged after that.
+// TODO(igorcov): Remove this after migration. crbug.com/725493
+ArcAvailabilityPolicyStatus g_arc_availability_policy_status = UNKNOWN;
+
+// The flag to state if the device ownership is taken.
+bool g_has_device_owner = false;
+
 // This is for finch. See also crbug.com/633704 for details.
 // TODO(hidehiko): More comments of the intention how this works, when
 // we unify the commandline flags.
@@ -31,6 +42,10 @@ constexpr char kAvailabilityNone[] = "none";
 constexpr char kAvailabilityInstalled[] = "installed";
 constexpr char kAvailabilityOfficiallySupported[] = "officially-supported";
 
+// The value of
+// enterprise_management::DeviceEcryptfsMigrationStrategyProto::ALLOW_MIGRATION
+constexpr int kAllowMigration = 2;
+
 void SetArcCpuRestrictionCallback(
     login_manager::ContainerCpuRestrictionState state,
     bool success) {
@@ -43,11 +58,56 @@ void SetArcCpuRestrictionCallback(
   LOG(ERROR) << "Failed to " << message << " ARC";
 }
 
+// Returns if the migration from ecryptfs to ext4 is allowed. It is true if it
+// is known that the device is consumer owned, meaning the flag
+// |g_has_device_owner| is set to true, or if the device policy is loaded and
+// has the value |kAllowMigration|. If the state is unknown, returns false. The
+// moment the result becomes known, it is cached and is not updated even if the
+// policy is updated.
+bool IsMigrationAllowed() {
+  if (g_arc_availability_policy_status == UNKNOWN) {
+    if (g_has_device_owner) {
+      g_arc_availability_policy_status = AVAILABLE;
+      return true;
+    }
+
+    if (!user_manager::UserManager::IsInitialized())
+      return false;
+
+    PrefService* pref_service =
+        user_manager::UserManager::Get()->GetLocalState();
+    const PrefService::Preference* pref =
+        pref_service->FindPreference(prefs::kDeviceEcryptfsMigrationStrategy);
+
+    if (!pref || !pref->IsManaged()) {
+      // There's no device owner, so it's either the policy loading and the
+      // status will be known later, or the device owner was not set yet.
+      return false;
+    }
+
+    if (pref->GetValue()->GetInt() == kAllowMigration)
+      g_arc_availability_policy_status = AVAILABLE;
+    else
+      g_arc_availability_policy_status = DISABLED;
+  }
+
+  return g_arc_availability_policy_status == AVAILABLE;
+}
+
 }  // namespace
 
 bool IsArcAvailable() {
   const auto* command_line = base::CommandLine::ForCurrentProcess();
 
+  // In the case the initial encryption was ecryptfs, the user data require
+  // migration to ext4 in order to have ARC available. The migration is
+  // forbidden if the device is managed and the policy is set to disable
+  // migration. This makes the ARC unavailable too.
+  // TODO(igorcov): Remove this after migration. crbug.com/725493
+  if (command_line->HasSwitch(chromeos::switches::kInitialEncryptionEcryptfs)) {
+    return IsMigrationAllowed();

[hidehiko, 2017/06/01 16:07:16]

Because, IsArcAvailable() is designed to return a fixed value in a process, and
used so. OTOH, this can change the value, so this is probably not a way to go, I
think.

I'm much worried about changing the timing convention, here.

Maybe code review is not an efficient tool to discuss this kind of strategy.
Could you fix the design doc first?

+  }
+
   if (command_line->HasSwitch(chromeos::switches::kArcAvailability)) {
     std::string value = command_line->GetSwitchValueASCII(
         chromeos::switches::kArcAvailability);
@@ -67,6 +127,19 @@ bool IsArcAvailable() {
        base::FeatureList::IsEnabled(kEnableArcFeature));
 }
 
+void SetHasDeviceOwner() {
+  g_has_device_owner = true;
+}
+
+void ResetGlobalDataForTesting() {
+  g_has_device_owner = false;
+  g_arc_availability_policy_status = UNKNOWN;
+}
+
+ArcAvailabilityPolicyStatus GetArcAvailabilityPolicyStatus() {
+  return g_arc_availability_policy_status;
+}
+
 bool ShouldArcAlwaysStart() {
   return base::CommandLine::ForCurrentProcess()->HasSwitch(
       chromeos::switches::kArcAlwaysStart);