Cached the migration policy value

When migration policy is changed during the user session, it might
enable ARC for user with unpredictable consequences. This change caches
the migration policy value for the session and uses the cached value.

Bug: b:67013794
Test: Manual
Change-Id: Id2f8c5173b8b63045a50531ec7e8d271d8df41da
Reviewed-on: https://chromium-review.googlesource.com/692195
Commit-Queue: Igor <igorcov@chromium.org>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505422}

chrome/browser/chromeos/arc/arc_util.cc

@@ -6,6 +6,7 @@
 
 #include <linux/magic.h>
 #include <sys/statfs.h>
+#include <map>
 #include <set>
 
 #include "base/callback.h"
@@ -41,6 +42,11 @@ constexpr char kAndroidMSdkVersion[] = "23";
 base::LazyInstance<std::set<base::FilePath>>::DestructorAtExit
     g_profile_declined_set = LAZY_INSTANCE_INITIALIZER;
 
+// The cached value of migration allowed for profile. It is necessary to use
+// the same value during a user session.
+base::LazyInstance<std::map<base::FilePath, bool>>::DestructorAtExit
+    g_is_arc_migration_allowed = LAZY_INSTANCE_INITIALIZER;
+
 // Let IsAllowedForProfile() return "false" for any profile.
 bool g_disallow_for_testing = false;
 
@@ -110,6 +116,37 @@ bool IsReportingFirstTimeForProfile(const Profile* profile) {
   return inserted;
 }
 
+bool IsArcMigrationAllowedInternal(const Profile* profile) {
+  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);
+}
+
 }  // namespace
 
 bool IsArcAllowedForProfile(const Profile* profile) {
@@ -192,34 +229,16 @@ bool IsArcMigrationAllowedByPolicyForProfile(const Profile* profile) {
     return true;
   }
 
-  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);
+  // Use the profile path as unique identifier for profile.
+  const base::FilePath path = profile->GetPath();
+  auto iter = g_is_arc_migration_allowed.Get().find(path);
+  if (iter == g_is_arc_migration_allowed.Get().end()) {
+    iter = g_is_arc_migration_allowed.Get()
+               .emplace(path, IsArcMigrationAllowedInternal(profile))
+               .first;
   }
 
-  return migration_strategy !=
-         static_cast<int>(
-             arc::policy_util::EcryptfsMigrationAction::kDisallowMigration);
+  return iter->second;
 }
 
 bool IsArcBlockedDueToIncompatibleFileSystem(const Profile* profile) {

chrome/browser/chromeos/arc/arc_util_unittest.cc

@@ -112,7 +112,7 @@ class FakeInstallAttributesManaged : public chromeos::InstallAttributes {
 class FakeUserManagerWithLocalState : public chromeos::FakeChromeUserManager {
  public:
   FakeUserManagerWithLocalState()
-      : test_local_state_(base::MakeUnique<TestingPrefServiceSimple>()) {
+      : test_local_state_(std::make_unique<TestingPrefServiceSimple>()) {
     RegisterPrefs(test_local_state_->registry());
   }
 
@@ -134,13 +134,13 @@ class ChromeArcUtilTest : public testing::Test {
   ~ChromeArcUtilTest() override = default;
 
   void SetUp() override {
-    command_line_ = base::MakeUnique<base::test::ScopedCommandLine>();
+    command_line_ = std::make_unique<base::test::ScopedCommandLine>();
 
     user_manager_enabler_ =
-        base::MakeUnique<chromeos::ScopedUserManagerEnabler>(
+        std::make_unique<chromeos::ScopedUserManagerEnabler>(
             new FakeUserManagerWithLocalState());
     chromeos::WallpaperManager::Initialize();
-    profile_ = base::MakeUnique<TestingProfile>();
+    profile_ = std::make_unique<TestingProfile>();
     profile_->set_profile_name(kTestProfileName);
   }
 
@@ -437,7 +437,7 @@ TEST_F(ChromeArcUtilTest, ArcPlayStoreEnabledForProfile_Managed) {
   // 1) Set managed preference to true, then try to set the value to false
   // via SetArcPlayStoreEnabledForProfile().
   profile()->GetTestingPrefService()->SetManagedPref(
-      prefs::kArcEnabled, base::MakeUnique<base::Value>(true));
+      prefs::kArcEnabled, std::make_unique<base::Value>(true));
   EXPECT_TRUE(IsArcPlayStoreEnabledPreferenceManagedForProfile(profile()));
   EXPECT_TRUE(IsArcPlayStoreEnabledForProfile(profile()));
   SetArcPlayStoreEnabledForProfile(profile(), false);
@@ -451,7 +451,7 @@ TEST_F(ChromeArcUtilTest, ArcPlayStoreEnabledForProfile_Managed) {
   // 2) Set managed preference to false, then try to set the value to true
   // via SetArcPlayStoreEnabledForProfile().
   profile()->GetTestingPrefService()->SetManagedPref(
-      prefs::kArcEnabled, base::MakeUnique<base::Value>(false));
+      prefs::kArcEnabled, std::make_unique<base::Value>(false));
   EXPECT_TRUE(IsArcPlayStoreEnabledPreferenceManagedForProfile(profile()));
   EXPECT_FALSE(IsArcPlayStoreEnabledForProfile(profile()));
   SetArcPlayStoreEnabledForProfile(profile(), true);
@@ -486,7 +486,7 @@ TEST_F(ChromeArcUtilTest, AreArcAllOptInPreferencesIgnorableForProfile) {
   // Backup-restore pref is managed, while location-service is not, and the
   // function returns false.
   profile()->GetTestingPrefService()->SetManagedPref(
-      prefs::kArcBackupRestoreEnabled, base::MakeUnique<base::Value>(false));
+      prefs::kArcBackupRestoreEnabled, std::make_unique<base::Value>(false));
   EXPECT_FALSE(AreArcAllOptInPreferencesIgnorableForProfile(profile()));
 
   // Location-service pref is managed, while backup-restore is not, and the
@@ -494,12 +494,12 @@ TEST_F(ChromeArcUtilTest, AreArcAllOptInPreferencesIgnorableForProfile) {
   profile()->GetTestingPrefService()->RemoveManagedPref(
       prefs::kArcBackupRestoreEnabled);
   profile()->GetTestingPrefService()->SetManagedPref(
-      prefs::kArcLocationServiceEnabled, base::MakeUnique<base::Value>(false));
+      prefs::kArcLocationServiceEnabled, std::make_unique<base::Value>(false));
   EXPECT_FALSE(AreArcAllOptInPreferencesIgnorableForProfile(profile()));
 
   // Both OptIn prefs are set to managed values, and the function returns true.
   profile()->GetTestingPrefService()->SetManagedPref(
-      prefs::kArcBackupRestoreEnabled, base::MakeUnique<base::Value>(false));
+      prefs::kArcBackupRestoreEnabled, std::make_unique<base::Value>(false));
   EXPECT_TRUE(AreArcAllOptInPreferencesIgnorableForProfile(profile()));
 }
 
@@ -526,11 +526,11 @@ class ArcMigrationTest : public testing::Test {
 
   void SetUp() override {
     user_manager_enabler_ =
-        base::MakeUnique<chromeos::ScopedUserManagerEnabler>(
+        std::make_unique<chromeos::ScopedUserManagerEnabler>(
             new FakeUserManagerWithLocalState());
     // Used by FakeChromeUserManager.
     chromeos::WallpaperManager::Initialize();
-    profile_ = base::MakeUnique<TestingProfile>();
+    profile_ = std::make_unique<TestingProfile>();
     profile_->set_profile_name(kTestProfileName);
   }
 
@@ -570,7 +570,7 @@ TEST_F(ArcMigrationTest, IsMigrationAllowedForbiddenByPolicy) {
                     AccountId::AdFromUserEmailObjGuid(
                         profile()->GetProfileUserName(), kTestGaiaId));
   profile()->GetTestingPrefService()->SetManagedPref(
-      prefs::kEcryptfsMigrationStrategy, base::MakeUnique<base::Value>(0));
+      prefs::kEcryptfsMigrationStrategy, std::make_unique<base::Value>(0));
   EXPECT_FALSE(IsArcMigrationAllowedByPolicyForProfile(profile()));
 }
 
@@ -579,7 +579,7 @@ TEST_F(ArcMigrationTest, IsMigrationAllowedMigrate) {
                     AccountId::AdFromUserEmailObjGuid(
                         profile()->GetProfileUserName(), kTestGaiaId));
   profile()->GetTestingPrefService()->SetManagedPref(
-      prefs::kEcryptfsMigrationStrategy, base::MakeUnique<base::Value>(1));
+      prefs::kEcryptfsMigrationStrategy, std::make_unique<base::Value>(1));
   EXPECT_TRUE(IsArcMigrationAllowedByPolicyForProfile(profile()));
 }
 
@@ -588,7 +588,7 @@ TEST_F(ArcMigrationTest, IsMigrationAllowedWipe) {
                     AccountId::AdFromUserEmailObjGuid(
                         profile()->GetProfileUserName(), kTestGaiaId));
   profile()->GetTestingPrefService()->SetManagedPref(
-      prefs::kEcryptfsMigrationStrategy, base::MakeUnique<base::Value>(2));
+      prefs::kEcryptfsMigrationStrategy, std::make_unique<base::Value>(2));
   EXPECT_TRUE(IsArcMigrationAllowedByPolicyForProfile(profile()));
 }
 
@@ -597,7 +597,7 @@ TEST_F(ArcMigrationTest, IsMigrationAllowedAskUser) {
                     AccountId::AdFromUserEmailObjGuid(
                         profile()->GetProfileUserName(), kTestGaiaId));
   profile()->GetTestingPrefService()->SetManagedPref(
-      prefs::kEcryptfsMigrationStrategy, base::MakeUnique<base::Value>(3));
+      prefs::kEcryptfsMigrationStrategy, std::make_unique<base::Value>(3));
   EXPECT_TRUE(IsArcMigrationAllowedByPolicyForProfile(profile()));
 }
 
@@ -606,7 +606,35 @@ TEST_F(ArcMigrationTest, IsMigrationAllowedMinimalMigration) {
                     AccountId::AdFromUserEmailObjGuid(
                         profile()->GetProfileUserName(), kTestGaiaId));
   profile()->GetTestingPrefService()->SetManagedPref(
-      prefs::kEcryptfsMigrationStrategy, base::MakeUnique<base::Value>(4));
+      prefs::kEcryptfsMigrationStrategy, std::make_unique<base::Value>(4));
+  EXPECT_TRUE(IsArcMigrationAllowedByPolicyForProfile(profile()));
+}
+
+TEST_F(ArcMigrationTest, IsMigrationAllowedCachedValueForbidden) {
+  ScopedLogIn login(GetFakeUserManager(),
+                    AccountId::AdFromUserEmailObjGuid(
+                        profile()->GetProfileUserName(), kTestGaiaId));
+  profile()->GetTestingPrefService()->SetManagedPref(
+      prefs::kEcryptfsMigrationStrategy, std::make_unique<base::Value>(0));
+  EXPECT_FALSE(IsArcMigrationAllowedByPolicyForProfile(profile()));
+  profile()->GetTestingPrefService()->SetManagedPref(
+      prefs::kEcryptfsMigrationStrategy, std::make_unique<base::Value>(1));
+
+  // The value of IsArcMigrationAllowedByPolicyForProfile() should be cached.
+  // So, even if the policy is set after the first call, the returned value
+  // should not be changed.
+  EXPECT_FALSE(IsArcMigrationAllowedByPolicyForProfile(profile()));
+}
+
+TEST_F(ArcMigrationTest, IsMigrationAllowedCachedValueAllowed) {
+  ScopedLogIn login(GetFakeUserManager(),
+                    AccountId::AdFromUserEmailObjGuid(
+                        profile()->GetProfileUserName(), kTestGaiaId));
+  profile()->GetTestingPrefService()->SetManagedPref(
+      prefs::kEcryptfsMigrationStrategy, std::make_unique<base::Value>(1));
+  EXPECT_TRUE(IsArcMigrationAllowedByPolicyForProfile(profile()));
+  profile()->GetTestingPrefService()->SetManagedPref(
+      prefs::kEcryptfsMigrationStrategy, std::make_unique<base::Value>(0));
   EXPECT_TRUE(IsArcMigrationAllowedByPolicyForProfile(profile()));
 }
 
@@ -639,9 +667,9 @@ TEST_P(ArcMigrationAskForEcryptfsArcUsersTest,
         chromeos::switches::kArcTransitionMigrationRequired);
   }
   profile()->GetTestingPrefService()->SetManagedPref(
-      prefs::kEcryptfsMigrationStrategy, base::MakeUnique<base::Value>(5));
+      prefs::kEcryptfsMigrationStrategy, std::make_unique<base::Value>(5));
   profile()->GetTestingPrefService()->SetManagedPref(
-      prefs::kArcEnabled, base::MakeUnique<base::Value>(param.arc_enabled));
+      prefs::kArcEnabled, std::make_unique<base::Value>(param.arc_enabled));
   EXPECT_EQ(param.expect_migration_allowed,
             IsArcMigrationAllowedByPolicyForProfile(profile()));
 }