Chrome cloud policies could override platform ones.

With the CloudPolicyOverridesPlatformPolicy enabled, Chrome cloud policies
use POLICY_SOURCE_PRIORITY_CLOUD instead to override the paltform policies
that have same scope. This only works for machie scope policies.

Bug: 749530
Change-Id: Ib35b4143d81ab57722c67c7018eff5203a60a93d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1572581
Commit-Queue: Owen Min <zmin@chromium.org>
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#652170}

chrome/browser/chromeos/policy/device_local_account_policy_store.cc

@@ -31,7 +31,8 @@ DeviceLocalAccountPolicyStore::DeviceLocalAccountPolicyStore(
     chromeos::DeviceSettingsService* device_settings_service,
     scoped_refptr<base::SequencedTaskRunner> background_task_runner)
     : UserCloudPolicyStoreBase(background_task_runner,
-                               PolicyScope::POLICY_SCOPE_USER),
+                               PolicyScope::POLICY_SCOPE_USER,
+                               PolicySource::POLICY_SOURCE_CLOUD),
       account_id_(account_id),
       session_manager_client_(session_manager_client),
       device_settings_service_(device_settings_service),

chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc

@@ -43,7 +43,8 @@ UserCloudPolicyStoreChromeOS::UserCloudPolicyStoreChromeOS(
     const base::FilePath& user_policy_key_dir,
     bool is_active_directory)
     : UserCloudPolicyStoreBase(background_task_runner,
-                               PolicyScope::POLICY_SCOPE_USER),
+                               PolicyScope::POLICY_SCOPE_USER,
+                               PolicySource::POLICY_SOURCE_CLOUD),
       session_manager_client_(session_manager_client),
       account_id_(account_id),
       is_active_directory_(is_active_directory),

chrome/browser/policy/cloud/machine_level_user_cloud_policy_browsertest.cc

@@ -433,6 +433,7 @@ class MachineLevelUserCloudPolicyManagerTest : public InProcessBrowserTest {
     std::unique_ptr<MachineLevelUserCloudPolicyStore> policy_store =
         MachineLevelUserCloudPolicyStore::Create(
             dm_token, client_id, user_data_dir,
+            /*cloud_policy_overrides=*/false,
             base::CreateSequencedTaskRunnerWithTraits(
                 {base::MayBlock(), base::TaskPriority::BEST_EFFORT}));
     policy_store->AddObserver(&observer);

chrome/browser/policy/machine_level_user_cloud_policy_controller.cc

@@ -122,11 +122,9 @@ MachineLevelUserCloudPolicyController::CreatePolicyManager(
 
   DVLOG(1) << "Creating machine level cloud policy manager";
 
-  bool does_cloud_policy_has_priority =
+  bool cloud_policy_has_priority =
       DoesCloudPolicyHasPriority(platform_provider);
-  if (does_cloud_policy_has_priority) {
-    // TODO(crbug.com/749530): Pass this flag to
-    // MachineLevelUserCloudPolicyManager.
+  if (cloud_policy_has_priority) {
     DVLOG(1) << "Cloud policies are now overriding platform policies with "
                 "machine scope.";
   }
@@ -135,7 +133,7 @@ MachineLevelUserCloudPolicyController::CreatePolicyManager(
       user_data_dir.Append(MachineLevelUserCloudPolicyController::kPolicyDir);
   std::unique_ptr<MachineLevelUserCloudPolicyStore> policy_store =
       MachineLevelUserCloudPolicyStore::Create(
-          dm_token, client_id, policy_dir,
+          dm_token, client_id, policy_dir, cloud_policy_has_priority,
           base::CreateSequencedTaskRunnerWithTraits(
               {base::MayBlock(), base::TaskPriority::BEST_EFFORT}));
   return std::make_unique<MachineLevelUserCloudPolicyManager>(

components/policy/core/common/cloud/machine_level_user_cloud_policy_manager.cc

@@ -51,11 +51,8 @@ void MachineLevelUserCloudPolicyManager::Connect(
   CreateComponentCloudPolicyService(
       dm_protocol::kChromeMachineLevelExtensionCloudPolicyType,
       policy_dir_.Append(kComponentPolicyCache),
-      (local_state->GetBoolean(
-           policy_prefs::kCloudPolicyOverridesPlatformPolicy)
-           ? POLICY_SOURCE_PRIORITY_CLOUD
-           : POLICY_SOURCE_CLOUD),
-      client.get(), schema_registry());
+      // Component cloud policies use the same source of Chrome ones.
+      store()->source(), client.get(), schema_registry());
   core()->Connect(std::move(client));
   core()->StartRefreshScheduler();
   core()->TrackRefreshDelayPref(local_state,

components/policy/core/common/cloud/machine_level_user_cloud_policy_manager_unittest.cc

@@ -26,6 +26,7 @@ class MockMachineLevelUserCloudPolicyStore
             std::string(),
             base::FilePath(),
             base::FilePath(),
+            /* cloud_policy_has_priority= */false,
             scoped_refptr<base::SequencedTaskRunner>()) {}
 
   MOCK_METHOD0(LoadImmediately, void(void));

components/policy/core/common/cloud/machine_level_user_cloud_policy_store.cc

@@ -24,11 +24,15 @@ MachineLevelUserCloudPolicyStore::MachineLevelUserCloudPolicyStore(
     const std::string& machine_client_id,
     const base::FilePath& policy_path,
     const base::FilePath& key_path,
+    bool cloud_policy_has_priority,
     scoped_refptr<base::SequencedTaskRunner> background_task_runner)
     : DesktopCloudPolicyStore(policy_path,
                               key_path,
                               background_task_runner,
-                              PolicyScope::POLICY_SCOPE_MACHINE),
+                              PolicyScope::POLICY_SCOPE_MACHINE,
+                              cloud_policy_has_priority
+                                  ? PolicySource::POLICY_SOURCE_PRIORITY_CLOUD
+                                  : PolicySource::POLICY_SOURCE_CLOUD),
       machine_dm_token_(machine_dm_token),
       machine_client_id_(machine_client_id) {}
 
@@ -40,12 +44,13 @@ MachineLevelUserCloudPolicyStore::Create(
     const std::string& machine_dm_token,
     const std::string& machine_client_id,
     const base::FilePath& policy_dir,
+    bool cloud_policy_has_priority,
     scoped_refptr<base::SequencedTaskRunner> background_task_runner) {
   base::FilePath policy_cache_file = policy_dir.Append(kPolicyCache);
   base::FilePath key_cache_file = policy_dir.Append(kKeyCache);
   return std::make_unique<MachineLevelUserCloudPolicyStore>(
       machine_dm_token, machine_client_id, policy_cache_file, key_cache_file,
-      background_task_runner);
+      cloud_policy_has_priority, background_task_runner);
 }
 
 void MachineLevelUserCloudPolicyStore::LoadImmediately() {

components/policy/core/common/cloud/machine_level_user_cloud_policy_store.h

@@ -25,6 +25,7 @@ class POLICY_EXPORT MachineLevelUserCloudPolicyStore
       const std::string& machine_client_id,
       const base::FilePath& policy_path,
       const base::FilePath& key_path,
+      bool cloud_policy_has_priority,
       scoped_refptr<base::SequencedTaskRunner> background_task_runner);
   ~MachineLevelUserCloudPolicyStore() override;
 
@@ -32,6 +33,7 @@ class POLICY_EXPORT MachineLevelUserCloudPolicyStore
       const std::string& machine_dm_token,
       const std::string& machine_client_id,
       const base::FilePath& policy_dir,
+      bool cloud_policy_has_priority,
       scoped_refptr<base::SequencedTaskRunner> background_task_runner);
 
   // override DesktopCloudPolicyStore

components/policy/core/common/cloud/machine_level_user_cloud_policy_store_unittest.cc

@@ -42,11 +42,20 @@ class MachineLevelUserCloudPolicyStoreTest : public ::testing::Test {
     store_ = CreateStore();
   }
 
-  std::unique_ptr<MachineLevelUserCloudPolicyStore> CreateStore() {
+  void SetExpectedPolicyMap(PolicySource source) {
+    expected_policy_map_.Clear();
+    expected_policy_map_.Set("IncognitoEnabled", POLICY_LEVEL_MANDATORY,
+                             POLICY_SCOPE_MACHINE, source,
+                             std::make_unique<base::Value>(false), nullptr);
+  }
+
+  std::unique_ptr<MachineLevelUserCloudPolicyStore> CreateStore(
+      bool cloud_policy_overrides = false) {
     std::unique_ptr<MachineLevelUserCloudPolicyStore> store =
         MachineLevelUserCloudPolicyStore::Create(
             PolicyBuilder::kFakeToken, PolicyBuilder::kFakeDeviceId,
-            tmp_policy_dir_.GetPath(), base::ThreadTaskRunnerHandle::Get());
+            tmp_policy_dir_.GetPath(), cloud_policy_overrides,
+            base::ThreadTaskRunnerHandle::Get());
     store->AddObserver(&observer_);
     return store;
   }
@@ -62,6 +71,7 @@ class MachineLevelUserCloudPolicyStoreTest : public ::testing::Test {
   base::ScopedTempDir tmp_policy_dir_;
   UserPolicyBuilder policy_;
   MockCloudPolicyStoreObserver observer_;
+  PolicyMap expected_policy_map_;
 
  private:
   base::test::ScopedTaskEnvironment scoped_task_environment_;
@@ -153,9 +163,35 @@ TEST_F(MachineLevelUserCloudPolicyStoreTest, StoreThenLoadPolicy) {
   loader->Load();
   base::RunLoop().RunUntilIdle();
 
+  SetExpectedPolicyMap(POLICY_SOURCE_CLOUD);
+  ASSERT_TRUE(loader->policy());
+  EXPECT_EQ(policy_.policy_data().SerializeAsString(),
+            loader->policy()->SerializeAsString());
+  EXPECT_TRUE(expected_policy_map_.Equals(loader->policy_map()));
+  EXPECT_EQ(CloudPolicyStore::STATUS_OK, loader->status());
+  loader->RemoveObserver(&observer_);
+
+  ::testing::Mock::VerifyAndClearExpectations(&observer_);
+}
+
+TEST_F(MachineLevelUserCloudPolicyStoreTest,
+       StoreAndLoadPolicyWithCloudPriority) {
+  EXPECT_CALL(observer_, OnStoreLoaded(store_.get()));
+  store_->Store(policy_.policy());
+  base::RunLoop().RunUntilIdle();
+
+  ::testing::Mock::VerifyAndClearExpectations(&observer_);
+
+  std::unique_ptr<MachineLevelUserCloudPolicyStore> loader = CreateStore(true);
+  EXPECT_CALL(observer_, OnStoreLoaded(loader.get()));
+  loader->Load();
+  base::RunLoop().RunUntilIdle();
+
+  SetExpectedPolicyMap(POLICY_SOURCE_PRIORITY_CLOUD);
   ASSERT_TRUE(loader->policy());
   EXPECT_EQ(policy_.policy_data().SerializeAsString(),
             loader->policy()->SerializeAsString());
+  EXPECT_TRUE(expected_policy_map_.Equals(loader->policy_map()));
   EXPECT_EQ(CloudPolicyStore::STATUS_OK, loader->status());
   loader->RemoveObserver(&observer_);
 

components/policy/core/common/cloud/user_cloud_policy_store.cc

@@ -172,8 +172,11 @@ DesktopCloudPolicyStore::DesktopCloudPolicyStore(
     const base::FilePath& policy_path,
     const base::FilePath& key_path,
     scoped_refptr<base::SequencedTaskRunner> background_task_runner,
-    PolicyScope policy_scope)
-    : UserCloudPolicyStoreBase(background_task_runner, policy_scope),
+    PolicyScope policy_scope,
+    PolicySource policy_source)
+    : UserCloudPolicyStoreBase(background_task_runner,
+                               policy_scope,
+                               policy_source),
       policy_path_(policy_path),
       key_path_(key_path),
       weak_factory_(this) {}
@@ -406,7 +409,8 @@ UserCloudPolicyStore::UserCloudPolicyStore(
     : DesktopCloudPolicyStore(policy_path,
                               key_path,
                               background_task_runner,
-                              PolicyScope::POLICY_SCOPE_USER) {}
+                              PolicyScope::POLICY_SCOPE_USER,
+                              PolicySource::POLICY_SOURCE_CLOUD) {}
 
 UserCloudPolicyStore::~UserCloudPolicyStore() {}
 

components/policy/core/common/cloud/user_cloud_policy_store.h

@@ -30,7 +30,8 @@ class POLICY_EXPORT DesktopCloudPolicyStore : public UserCloudPolicyStoreBase {
       const base::FilePath& policy_file,
       const base::FilePath& key_file,
       scoped_refptr<base::SequencedTaskRunner> background_task_runner,
-      PolicyScope policy_scope);
+      PolicyScope policy_scope,
+      PolicySource policy_source);
   ~DesktopCloudPolicyStore() override;
 
   // Loads policy immediately on the current thread. Virtual for mocks.

components/policy/core/common/cloud/user_cloud_policy_store_base.cc

@@ -18,9 +18,14 @@ namespace policy {
 
 UserCloudPolicyStoreBase::UserCloudPolicyStoreBase(
     scoped_refptr<base::SequencedTaskRunner> background_task_runner,
-    PolicyScope policy_scope)
+    PolicyScope policy_scope,
+    PolicySource policy_source)
     : background_task_runner_(background_task_runner),
-      policy_scope_(policy_scope) {}
+      policy_scope_(policy_scope),
+      policy_source_(policy_source) {
+  DCHECK(policy_source == POLICY_SOURCE_CLOUD ||
+         policy_source == POLICY_SOURCE_PRIORITY_CLOUD);
+}
 
 UserCloudPolicyStoreBase::~UserCloudPolicyStoreBase() {}
 
@@ -46,7 +51,7 @@ void UserCloudPolicyStoreBase::InstallPolicy(
     const std::string& policy_signature_public_key) {
   // Decode the payload.
   policy_map_.Clear();
-  DecodeProtoFields(*payload, external_data_manager(), POLICY_SOURCE_CLOUD,
+  DecodeProtoFields(*payload, external_data_manager(), policy_source_,
                     policy_scope_, &policy_map_);
   policy_ = std::move(policy_data);
   policy_signature_public_key_ = policy_signature_public_key;

components/policy/core/common/cloud/user_cloud_policy_store_base.h

@@ -28,9 +28,12 @@ class POLICY_EXPORT UserCloudPolicyStoreBase : public CloudPolicyStore {
  public:
   UserCloudPolicyStoreBase(
       scoped_refptr<base::SequencedTaskRunner> background_task_runner,
-      PolicyScope policy_scope);
+      PolicyScope policy_scope,
+      PolicySource policy_source);
   ~UserCloudPolicyStoreBase() override;
 
+  PolicySource source() { return policy_source_; }
+
  protected:
   // Creates a validator configured to validate a user policy. The caller owns
   // the resulting object until StartValidation() is invoked.
@@ -53,6 +56,7 @@ class POLICY_EXPORT UserCloudPolicyStoreBase : public CloudPolicyStore {
   // Task runner for background file operations.
   scoped_refptr<base::SequencedTaskRunner> background_task_runner_;
   PolicyScope policy_scope_;
+  PolicySource policy_source_;
 
   DISALLOW_COPY_AND_ASSIGN(UserCloudPolicyStoreBase);
 };