[Merge to M62] Default (unset by admin) migration policy for EDU

If the admin left the policy for migration unset and the device is enrolled in an EDU domain, the server will send to the client kAskForEcryptfsArcUsers = 5 value for that policy. When the client receives the kAskForEcryptfsArcUsers policy value, it must check two things: 1. If the device had ARC M in M60 (specified by the flag kArcTransitionMigrationRequired) 2. Arc is enabled by poicy. If both conditions apply, then user should be asked if the migration has to be performed. Otherwise the migration is disabled for this policy value. This CL was originally owned by igorcov@ and tracked in CL:645352. BUG=747907 TEST=Manual, unit_tests --gtest_filter=ArcMigration* [email protected] (cherry picked from commit 3c98b1d) Change-Id: I668e5d4e33afa94607aa6db790c06e6f9ee61879 Reviewed-on: https://chromium-review.googlesource.com/645953 Commit-Queue: Pavol Marko <[email protected]> Reviewed-by: Xiyuan Xia <[email protected]> Reviewed-by: Hidehiko Abe <[email protected]> Reviewed-by: Maksim Ivanov <[email protected]> Cr-Original-Commit-Position: refs/heads/master@{#499237} Reviewed-on: https://chromium-review.googlesource.com/653638 Reviewed-by: Pavol Marko <[email protected]> Cr-Commit-Position: refs/branch-heads/3202@{#51} Cr-Branched-From: fa6a5d8-refs/heads/master@{#499098}
nwjs · Sep 6, 2017 · 52f8877 · 52f8877
1 parent ff28287
commit 52f8877
Show file tree

Hide file tree

Showing 6 changed files with 133 additions and 14 deletions.
diff --git a/chrome/browser/chromeos/arc/arc_util.cc b/chrome/browser/chromeos/arc/arc_util.cc
@@ -9,6 +9,7 @@
 #include <set>
 
 #include "base/callback.h"
+#include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
@@ -22,6 +23,7 @@
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/pref_names.h"
+#include "chromeos/chromeos_switches.h"
 #include "components/arc/arc_util.h"
 #include "components/prefs/pref_service.h"
 #include "components/user_manager/known_user.h"
@@ -153,7 +155,7 @@ bool IsArcAllowedForProfile(const Profile* profile) {
   }
 
   if (!IsArcCompatibleFileSystemUsedForProfile(profile) &&
-      !IsArcMigrationAllowedForProfile(profile)) {
+      !IsArcMigrationAllowedByPolicyForProfile(profile)) {
     VLOG_IF(1, IsReportingFirstTimeForProfile(profile))
         << "Incompatible encryption and migration forbidden.";
     return false;
@@ -184,13 +186,38 @@ bool IsArcAllowedForProfile(const Profile* profile) {
   return true;
 }
 
-bool IsArcMigrationAllowedForProfile(const Profile* profile) {
+bool IsArcMigrationAllowedByPolicyForProfile(const Profile* profile) {
   if (!profile || !profile->GetPrefs()->IsManagedPreference(
                       prefs::kEcryptfsMigrationStrategy)) {
     return true;
   }
 
-  return profile->GetPrefs()->GetInteger(prefs::kEcryptfsMigrationStrategy) !=
+  int migration_strategy =
+      profile->GetPrefs()->GetInteger(prefs::kEcryptfsMigrationStrategy);
+  // |kAskForEcryptfsArcUsers| value is received only if the device is in EDU
+  // and admin left the migration policy unset. Note that when enabling ARC on
+  // the admin console, it is mandatory for the administrator to also choose a
+  // migration policy.
+  // In this default case, only a group of devices that had ARC M enabled are
+  // allowed to migrate, provided that ARC is enabled by policy.
+  // TODO(pmarko): Remove the special kAskForEcryptfsArcUsers handling when we
+  // assess that it's not necessary anymore: crbug.com/761348.
+  if (migration_strategy ==
+      static_cast<int>(
+          arc::policy_util::EcryptfsMigrationAction::kAskForEcryptfsArcUsers)) {
+    // Note that ARC enablement is controlled by policy for managed users (as
+    // it's marked 'default_for_enterprise_users': False in
+    // policy_templates.json).
+    DCHECK(profile->GetPrefs()->IsManagedPreference(prefs::kArcEnabled));
+    // We can't reuse IsArcPlayStoreEnabledForProfile here because this would
+    // lead to a circular dependency: It ends up calling this function for some
+    // cases.
+    return profile->GetPrefs()->GetBoolean(prefs::kArcEnabled) &&
+           base::CommandLine::ForCurrentProcess()->HasSwitch(
+               chromeos::switches::kArcTransitionMigrationRequired);
+  }
+
+  return migration_strategy !=
          static_cast<int>(
              arc::policy_util::EcryptfsMigrationAction::kDisallowMigration);
 }

diff --git a/chrome/browser/chromeos/arc/arc_util.h b/chrome/browser/chromeos/arc/arc_util.h
@@ -50,7 +50,7 @@ bool IsArcAllowedForProfile(const Profile* profile);
 // EcryptfsMigrationStrategy for the user doesn't disable the migration.
 // Specifically if the policy states to ask the user, it is also considered that
 // migration is allowed, so return true.
-bool IsArcMigrationAllowedForProfile(const Profile* profile);
+bool IsArcMigrationAllowedByPolicyForProfile(const Profile* profile);
 
 // Returns true if the profile is temporarily blocked to run ARC in the current
 // session, because the filesystem storing the profile is incompatible with the

diff --git a/chrome/browser/chromeos/arc/arc_util_unittest.cc b/chrome/browser/chromeos/arc/arc_util_unittest.cc
@@ -24,6 +24,7 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/testing_profile.h"
+#include "chromeos/chromeos_switches.h"
 #include "components/policy/core/common/cloud/cloud_policy_constants.h"
 #include "components/prefs/pref_service.h"
 #include "components/prefs/testing_pref_service.h"
@@ -561,7 +562,7 @@ TEST_F(ArcMigrationTest, IsMigrationAllowedUnmanagedUser) {
                     AccountId::AdFromUserEmailObjGuid(
                         profile()->GetProfileUserName(), kTestGaiaId));
   profile()->GetPrefs()->SetInteger(prefs::kEcryptfsMigrationStrategy, 0);
-  EXPECT_TRUE(IsArcMigrationAllowedForProfile(profile()));
+  EXPECT_TRUE(IsArcMigrationAllowedByPolicyForProfile(profile()));
 }
 
 TEST_F(ArcMigrationTest, IsMigrationAllowedForbiddenByPolicy) {
@@ -570,7 +571,7 @@ TEST_F(ArcMigrationTest, IsMigrationAllowedForbiddenByPolicy) {
                         profile()->GetProfileUserName(), kTestGaiaId));
   profile()->GetTestingPrefService()->SetManagedPref(
       prefs::kEcryptfsMigrationStrategy, base::MakeUnique<base::Value>(0));
-  EXPECT_FALSE(IsArcMigrationAllowedForProfile(profile()));
+  EXPECT_FALSE(IsArcMigrationAllowedByPolicyForProfile(profile()));
 }
 
 TEST_F(ArcMigrationTest, IsMigrationAllowedMigrate) {
@@ -579,7 +580,7 @@ TEST_F(ArcMigrationTest, IsMigrationAllowedMigrate) {
                         profile()->GetProfileUserName(), kTestGaiaId));
   profile()->GetTestingPrefService()->SetManagedPref(
       prefs::kEcryptfsMigrationStrategy, base::MakeUnique<base::Value>(1));
-  EXPECT_TRUE(IsArcMigrationAllowedForProfile(profile()));
+  EXPECT_TRUE(IsArcMigrationAllowedByPolicyForProfile(profile()));
 }
 
 TEST_F(ArcMigrationTest, IsMigrationAllowedWipe) {
@@ -588,7 +589,7 @@ TEST_F(ArcMigrationTest, IsMigrationAllowedWipe) {
                         profile()->GetProfileUserName(), kTestGaiaId));
   profile()->GetTestingPrefService()->SetManagedPref(
       prefs::kEcryptfsMigrationStrategy, base::MakeUnique<base::Value>(2));
-  EXPECT_TRUE(IsArcMigrationAllowedForProfile(profile()));
+  EXPECT_TRUE(IsArcMigrationAllowedByPolicyForProfile(profile()));
 }
 
 TEST_F(ArcMigrationTest, IsMigrationAllowedAskUser) {
@@ -597,8 +598,70 @@ TEST_F(ArcMigrationTest, IsMigrationAllowedAskUser) {
                         profile()->GetProfileUserName(), kTestGaiaId));
   profile()->GetTestingPrefService()->SetManagedPref(
       prefs::kEcryptfsMigrationStrategy, base::MakeUnique<base::Value>(3));
-  EXPECT_TRUE(IsArcMigrationAllowedForProfile(profile()));
+  EXPECT_TRUE(IsArcMigrationAllowedByPolicyForProfile(profile()));
 }
 
+TEST_F(ArcMigrationTest, IsMigrationAllowedMinimalMigration) {
+  ScopedLogIn login(GetFakeUserManager(),
+                    AccountId::AdFromUserEmailObjGuid(
+                        profile()->GetProfileUserName(), kTestGaiaId));
+  profile()->GetTestingPrefService()->SetManagedPref(
+      prefs::kEcryptfsMigrationStrategy, base::MakeUnique<base::Value>(4));
+  EXPECT_TRUE(IsArcMigrationAllowedByPolicyForProfile(profile()));
+}
+
+// Defines parameters for parametrized test
+// ArcMigrationAskForEcryptfsArcUsersTest.
+struct AskForEcryptfsArcUserTestParam {
+  bool device_supported_arc;
+  bool arc_enabled;
+  bool expect_migration_allowed;
+};
+
+class ArcMigrationAskForEcryptfsArcUsersTest
+    : public ArcMigrationTest,
+      public testing::WithParamInterface<AskForEcryptfsArcUserTestParam> {
+ protected:
+  ArcMigrationAskForEcryptfsArcUsersTest() {}
+  ~ArcMigrationAskForEcryptfsArcUsersTest() override {}
+};
+
+// Migration policy is 5 (kAskForEcryptfsArcUsers, EDU default).
+TEST_P(ArcMigrationAskForEcryptfsArcUsersTest,
+       IsMigrationAllowedAskForEcryptfsArcUsers) {
+  const AskForEcryptfsArcUserTestParam& param = GetParam();
+
+  ScopedLogIn login(GetFakeUserManager(),
+                    AccountId::AdFromUserEmailObjGuid(
+                        profile()->GetProfileUserName(), kTestGaiaId));
+  if (param.device_supported_arc) {
+    base::CommandLine::ForCurrentProcess()->AppendSwitch(
+        chromeos::switches::kArcTransitionMigrationRequired);
+  }
+  profile()->GetTestingPrefService()->SetManagedPref(
+      prefs::kEcryptfsMigrationStrategy, base::MakeUnique<base::Value>(5));
+  profile()->GetTestingPrefService()->SetManagedPref(
+      prefs::kArcEnabled, base::MakeUnique<base::Value>(param.arc_enabled));
+  EXPECT_EQ(param.expect_migration_allowed,
+            IsArcMigrationAllowedByPolicyForProfile(profile()));
+}
+
+INSTANTIATE_TEST_CASE_P(
+    ArcMigrationTest,
+    ArcMigrationAskForEcryptfsArcUsersTest,
+    ::testing::Values(
+        AskForEcryptfsArcUserTestParam{true /* device_supported_arc */,
+                                       true /* arc_enabled */,
+                                       true /* expect_migration_allowed */},
+        AskForEcryptfsArcUserTestParam{true /* device_supported_arc */,
+                                       false /* arc_enabled */,
+                                       false /* expect_migration_allowed */},
+        AskForEcryptfsArcUserTestParam{false /* device_supported_arc */,
+                                       true /* arc_enabled */,
+                                       false /* expect_migration_allowed */},
+        AskForEcryptfsArcUserTestParam{false /* device_supported_arc */,
+                                       false /* arc_enabled */,
+                                       false /* expect_migration_allowed */}));
+
 }  // namespace util
 }  // namespace arc
diff --git a/chrome/browser/chromeos/login/existing_user_controller.cc b/chrome/browser/chromeos/login/existing_user_controller.cc
@@ -243,6 +243,15 @@ bool DecodeMigrationActionFromPolicy(
   return true;
 }
 
+// Returns true if ArcEnabled policy is present and set to true. Otherwise
+// returns false.
+bool IsArcEnabledFromPolicy(
+    const enterprise_management::CloudPolicySettings* policy) {
+  if (policy->has_arcenabled())
+    return policy->arcenabled().value();
+  return false;
+}
+
 }  // namespace
 
 // static
@@ -1046,6 +1055,11 @@ void ExistingUserController::OnPolicyFetchResult(
   VLOG(1) << "Migration action: " << static_cast<int>(action);
 
   switch (action) {
+    case apu::EcryptfsMigrationAction::kDisallowMigration:
+      ContinuePerformLoginWithoutMigration(login_performer_->auth_mode(),
+                                           user_context);
+      break;
+
     case apu::EcryptfsMigrationAction::kMigrate:
       user_manager::known_user::SetUserHomeMinimalMigrationAttempted(
           user_context.GetAccountId(), false);
@@ -1080,11 +1094,18 @@ void ExistingUserController::OnPolicyFetchResult(
       break;
 
     case apu::EcryptfsMigrationAction::kAskForEcryptfsArcUsers:
-    // TODO(igorcov): Fall-through intended. This behaves as Disallow Migration
-    // until it's implemented.
-    case apu::EcryptfsMigrationAction::kDisallowMigration:
-      ContinuePerformLoginWithoutMigration(login_performer_->auth_mode(),
-                                           user_context);
+      // If the device is transitioning from ARC M to ARC N and has ARC enabled
+      // by policy, then ask the user about the migration. Otherwise disallow
+      // migration.
+      if (IsArcEnabledFromPolicy(policy_payload.get()) &&
+          base::CommandLine::ForCurrentProcess()->HasSwitch(
+              switches::kArcTransitionMigrationRequired)) {
+        ShowEncryptionMigrationScreen(user_context,
+                                      EncryptionMigrationMode::ASK_USER);
+      } else {
+        ContinuePerformLoginWithoutMigration(login_performer_->auth_mode(),
+                                             user_context);
+      }
       break;
   }
 }

diff --git a/chromeos/chromeos_switches.cc b/chromeos/chromeos_switches.cc
@@ -84,6 +84,13 @@ const char kArcAvailable[] = "arc-available";
 // If it is not set, then ARC is started in default mode.
 const char kArcStartMode[] = "arc-start-mode";
 
+// If this flag is present then the device had ARC M available and gets ARC N
+// when updating.
+// TODO(pmarko): Remove this when we assess that it's not necessary anymore:
+// crbug.com/761348.
+const char kArcTransitionMigrationRequired[] =
+    "arc-transition-migration-required";
+
 // Screenshot testing: specifies the directoru where artifacts will be stored.
 const char kArtifactsDir[] = "artifacts-dir";
 

diff --git a/chromeos/chromeos_switches.h b/chromeos/chromeos_switches.h
@@ -30,6 +30,7 @@ CHROMEOS_EXPORT extern const char kAppAutoLaunched[];
 CHROMEOS_EXPORT extern const char kAppOemManifestFile[];
 CHROMEOS_EXPORT extern const char kArcAvailability[];
 CHROMEOS_EXPORT extern const char kArcAvailable[];
+CHROMEOS_EXPORT extern const char kArcTransitionMigrationRequired[];
 CHROMEOS_EXPORT extern const char kArcStartMode[];
 CHROMEOS_EXPORT extern const char kArtifactsDir[];
 CHROMEOS_EXPORT extern const char kAshWebUIInit[];