Enable affiliated cloud user policy merging

Update policy merging logic to allow for merging of cloud user policy values. This merging will only happen for users affiliated with the organization managing the browser.

A new policy, "CloudUserPolicyMerge", is also added which allows admins to opt into the new merging behavior. This policy is meant to prevent unexpected behavior resulting from the updated merging rules. In addition to affiliation, cloud user policy merging will not occur unless the policy value is set to "True".

Bug: 1185161
Change-Id: I61e92f318c184a4c44c80d22c98299a709555501
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2851644
Commit-Queue: Igor Ruvinov <igorruvinov@chromium.org>
Reviewed-by: Owen Min <zmin@chromium.org>
Reviewed-by: Yann Dago <ydago@chromium.org>
Cr-Commit-Position: refs/heads/master@{#880600}

chrome/test/data/policy/policy_test_cases.json

@@ -8847,5 +8847,8 @@
         }}
       }
     ]
+  },
+  "CloudUserPolicyMerge": {
+    "note": "This policy has no pref as it is only directly read by the policy system."
   }
 }

components/policy/core/browser/policy_conversions_client.cc

@@ -171,7 +171,9 @@ Value PolicyConversionsClient::GetPolicyValue(
   if (policy.source == POLICY_SOURCE_MERGED) {
     bool policy_has_unmerged_source = false;
     for (const auto& conflict : policy.conflicts) {
-      if (PolicyMerger::ConflictCanBeMerged(conflict.entry(), policy))
+      if (PolicyMerger::ConflictCanBeMerged(
+              conflict.entry(), policy,
+              /*is_user_cloud_merging_enabled=*/false))
         continue;
       policy_has_unmerged_source = true;
       break;

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

@@ -62,6 +62,17 @@ void UserCloudPolicyStoreBase::InstallPolicy(
 #endif
   DecodeProtoFieldsPerProfile(*payload, external_data_manager(), policy_source_,
                               policy_scope_, &policy_map_, filter);
+
+  if (policy_data->user_affiliation_ids_size() > 0) {
+    policy_map_.SetUserAffiliationIds(
+        {policy_data->user_affiliation_ids().begin(),
+         policy_data->user_affiliation_ids().end()});
+  }
+  if (policy_data->device_affiliation_ids_size() > 0) {
+    policy_map_.SetDeviceAffiliationIds(
+        {policy_data->device_affiliation_ids().begin(),
+         policy_data->device_affiliation_ids().end()});
+  }
   policy_ = std::move(policy_data);
   policy_signature_public_key_ = policy_signature_public_key;
 }

components/policy/core/common/policy_map.cc

@@ -13,6 +13,7 @@
 #include "base/strings/strcat.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
+#include "components/policy/core/common/cloud/affiliation.h"
 #include "components/policy/core/common/policy_merger.h"
 #include "components/strings/grit/components_strings.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -41,6 +42,16 @@ const std::u16string GetLocalizedString(
   return result;
 }
 
+// Inserts additional user affiliation IDs to the existing set.
+base::flat_set<std::string> CombineIds(
+    const base::flat_set<std::string>& ids_first,
+    const base::flat_set<std::string>& ids_second) {
+  base::flat_set<std::string> combined_ids;
+  combined_ids.insert(ids_first.begin(), ids_first.end());
+  combined_ids.insert(ids_second.begin(), ids_second.end());
+  return combined_ids;
+}
+
 }  // namespace
 
 PolicyMap::Entry::Entry() = default;
@@ -337,6 +348,9 @@ PolicyMap PolicyMap::Clone() const {
   for (const auto& it : map_)
     clone.Set(it.first, it.second.DeepCopy());
 
+  clone.SetUserAffiliationIds(user_affiliation_ids_);
+  clone.SetDeviceAffiliationIds(device_affiliation_ids_);
+
   return clone;
 }
 
@@ -376,6 +390,11 @@ void PolicyMap::MergeFrom(const PolicyMap& other) {
     if (other_is_higher_priority)
       *current_policy = std::move(other_policy);
   }
+
+  SetUserAffiliationIds(
+      CombineIds(GetUserAffiliationIds(), other.GetUserAffiliationIds()));
+  SetDeviceAffiliationIds(
+      CombineIds(GetDeviceAffiliationIds(), other.GetDeviceAffiliationIds()));
 }
 
 void PolicyMap::MergeValues(const std::vector<PolicyMerger*>& mergers) {
@@ -446,4 +465,26 @@ void PolicyMap::FilterErase(
   }
 }
 
+bool PolicyMap::IsUserAffiliated() const {
+  return IsAffiliated(user_affiliation_ids_, device_affiliation_ids_);
+}
+
+void PolicyMap::SetUserAffiliationIds(
+    const base::flat_set<std::string>& user_ids) {
+  user_affiliation_ids_ = {user_ids.begin(), user_ids.end()};
+}
+
+const base::flat_set<std::string>& PolicyMap::GetUserAffiliationIds() const {
+  return user_affiliation_ids_;
+}
+
+void PolicyMap::SetDeviceAffiliationIds(
+    const base::flat_set<std::string>& device_ids) {
+  device_affiliation_ids_ = {device_ids.begin(), device_ids.end()};
+}
+
+const base::flat_set<std::string>& PolicyMap::GetDeviceAffiliationIds() const {
+  return device_affiliation_ids_;
+}
+
 }  // namespace policy

components/policy/core/common/policy_map.h

@@ -13,6 +13,7 @@
 #include <string>
 
 #include "base/callback.h"
+#include "base/containers/flat_set.h"
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/optional.h"
@@ -263,6 +264,22 @@ class POLICY_EXPORT PolicyMap {
                 PolicyScope scope,
                 PolicySource source);
 
+  // Returns True if at least one shared ID is found in the user and device
+  // affiliation ID sets.
+  bool IsUserAffiliated() const;
+
+  // Populates the set containing user affiliation ID strings.
+  void SetUserAffiliationIds(const base::flat_set<std::string>& user_ids);
+
+  // Returns the set containing user affiliation ID strings.
+  const base::flat_set<std::string>& GetUserAffiliationIds() const;
+
+  // Populates the set containing device affiliation ID strings.
+  void SetDeviceAffiliationIds(const base::flat_set<std::string>& device_ids);
+
+  // Returns the set containing device affiliation ID strings.
+  const base::flat_set<std::string>& GetDeviceAffiliationIds() const;
+
   bool Equals(const PolicyMap& other) const;
   bool empty() const;
   size_t size() const;
@@ -293,8 +310,12 @@ class POLICY_EXPORT PolicyMap {
       bool deletion_value);
 
   PolicyMapType map_;
-};
 
+  // Affiliation
+  bool is_user_affiliated_ = false;
+  base::flat_set<std::string> user_affiliation_ids_;
+  base::flat_set<std::string> device_affiliation_ids_;
+};
 }  // namespace policy
 
 #endif  // COMPONENTS_POLICY_CORE_COMMON_POLICY_MAP_H_

components/policy/core/common/policy_map_unittest.cc

@@ -1172,4 +1172,28 @@ TEST_F(PolicyMapTest, InvalidEntry) {
   EXPECT_TRUE(policies.GetUntrusted("b")->ignored());
 }
 
+TEST_F(PolicyMapTest, Affiliation) {
+  PolicyMap policies;
+  EXPECT_FALSE(policies.IsUserAffiliated());
+
+  base::flat_set<std::string> user_ids;
+  user_ids.insert("a");
+  base::flat_set<std::string> device_ids;
+  device_ids.insert("b");
+  policies.SetUserAffiliationIds(user_ids);
+  policies.SetUserAffiliationIds(device_ids);
+
+  // Affiliation check fails because user and device IDs don't have at least one
+  // ID in common.
+  EXPECT_FALSE(policies.IsUserAffiliated());
+
+  user_ids.insert("b");
+  device_ids.insert("c");
+  policies.SetUserAffiliationIds(user_ids);
+  policies.SetDeviceAffiliationIds(device_ids);
+
+  // Affiliation check succeeds now that 'a' is present in user and device IDs.
+  EXPECT_TRUE(policies.IsUserAffiliated());
+}
+
 }  // namespace policy

components/policy/core/common/policy_merger.cc

@@ -27,18 +27,43 @@ constexpr std::array<const char*, 6> kDictionaryPoliciesToMerge{
 }  // namespace
 
 // static
-bool PolicyMerger::ConflictCanBeMerged(const PolicyMap::Entry& conflict,
-                                       const PolicyMap::Entry& policy) {
+bool PolicyMerger::ConflictCanBeMerged(
+    const PolicyMap::Entry& conflict,
+    const PolicyMap::Entry& policy,
+    const bool is_user_cloud_merging_enabled) {
+  if (conflict.ignored() ||
+      conflict.source == POLICY_SOURCE_ENTERPRISE_DEFAULT ||
+      conflict.level != policy.level)
+    return false;
+
+  // If the policies have matching scope and are non-user, they can be merged.
+  if (conflict.scope == policy.scope && conflict.scope != POLICY_SCOPE_USER)
+    return true;
+
   // On desktop, the user cloud policy potentially comes from a different
-  // domain than e.g. GPO policy or machine-level cloud policy, so prevent
-  // merging user cloud policy with other policy sources.
+  // domain than e.g. GPO policy or machine-level cloud policy. Merging a user
+  // cloud policy with policies from other sources is only permitted if both of
+  // the following conditions are met:
+  //   1. The CloudUserPolicyMerge is set to True.
+  //   2. The user is affiliated with the machine-level cloud policy provider.
   const bool is_conflict_user_cloud_policy =
       conflict.scope == POLICY_SCOPE_USER &&
       (conflict.source == POLICY_SOURCE_CLOUD ||
        conflict.source == POLICY_SOURCE_PRIORITY_CLOUD);
-  return !is_conflict_user_cloud_policy && !conflict.ignored() &&
-         conflict.source != POLICY_SOURCE_ENTERPRISE_DEFAULT &&
-         conflict.level == policy.level && conflict.scope == policy.scope;
+
+  // Merging of user-level GPO policies is not permitted to prevent unexpected
+  // behavior. If such merging is desired, it will be implemented in a similar
+  // way as user cloud merging.
+  const bool is_conflict_user_platform_policy =
+      conflict.scope == POLICY_SCOPE_USER &&
+      conflict.source == POLICY_SOURCE_PLATFORM;
+
+  const bool is_scope_overriden =
+      is_conflict_user_cloud_policy && is_user_cloud_merging_enabled;
+
+  return !is_conflict_user_platform_policy &&
+         (!is_conflict_user_cloud_policy || is_user_cloud_merging_enabled) &&
+         (conflict.scope == policy.scope || is_scope_overriden);
 }
 
 PolicyMerger::PolicyMerger() = default;
@@ -60,6 +85,10 @@ void PolicyListMerger::Merge(PolicyMap::PolicyMapType* policies) const {
   }
 }
 
+void PolicyListMerger::SetAllowUserCloudPolicyMerging(bool allowed) {
+  allow_user_cloud_policy_merging_ = allowed;
+}
+
 bool PolicyListMerger::CanMerge(const std::string& policy_name,
                                 PolicyMap::Entry& policy) const {
   if (policy.source == POLICY_SOURCE_MERGED)
@@ -80,6 +109,10 @@ bool PolicyListMerger::CanMerge(const std::string& policy_name,
   return true;
 }
 
+bool PolicyListMerger::AllowUserCloudPolicyMerging() const {
+  return allow_user_cloud_policy_merging_;
+}
+
 void PolicyListMerger::DoMerge(PolicyMap::Entry* policy) const {
   std::vector<const base::Value*> merged_values;
   auto compare_value_ptr = [](const base::Value* a, const base::Value* b) {
@@ -99,7 +132,8 @@ void PolicyListMerger::DoMerge(PolicyMap::Entry* policy) const {
   // Concatenates the values from accepted conflicting sources to the policy
   // value while avoiding duplicates.
   for (const auto& it : policy->conflicts) {
-    if (!PolicyMerger::ConflictCanBeMerged(it.entry(), *policy)) {
+    if (!PolicyMerger::ConflictCanBeMerged(it.entry(), *policy,
+                                           AllowUserCloudPolicyMerging())) {
       continue;
     }
 
@@ -146,6 +180,10 @@ void PolicyDictionaryMerger::SetAllowedPoliciesForTesting(
   allowed_policies_ = std::move(allowed_policies);
 }
 
+void PolicyDictionaryMerger::SetAllowUserCloudPolicyMerging(bool allowed) {
+  allow_user_cloud_policy_merging_ = allowed;
+}
+
 bool PolicyDictionaryMerger::CanMerge(const std::string& policy_name,
                                       PolicyMap::Entry& policy) const {
   if (policy.source == POLICY_SOURCE_MERGED)
@@ -176,6 +214,10 @@ bool PolicyDictionaryMerger::CanMerge(const std::string& policy_name,
   return true;
 }
 
+bool PolicyDictionaryMerger::AllowUserCloudPolicyMerging() const {
+  return allow_user_cloud_policy_merging_;
+}
+
 void PolicyDictionaryMerger::DoMerge(PolicyMap::Entry* policy) const {
   // Keep priority sorted list of potential merge targets.
   std::vector<const PolicyMap::Entry*> policies;
@@ -193,7 +235,8 @@ void PolicyDictionaryMerger::DoMerge(PolicyMap::Entry* policy) const {
 
   // Merges all the keys from the policies from different sources.
   for (const auto* it : policies) {
-    if (it != policy && !PolicyMerger::ConflictCanBeMerged(*it, *policy))
+    if (it != policy && !PolicyMerger::ConflictCanBeMerged(
+                            *it, *policy, AllowUserCloudPolicyMerging()))
       continue;
 
     const base::DictionaryValue* dict = nullptr;

components/policy/core/common/policy_merger.h

@@ -22,8 +22,13 @@ namespace policy {
 class POLICY_EXPORT PolicyMerger {
  public:
   PolicyMerger();
+
+  // Determines if a policy value is eligible for merging depending on several
+  // factors including its scope, source, and level.
   static bool ConflictCanBeMerged(const PolicyMap::Entry& conflict,
-                                  const PolicyMap::Entry& policy);
+                                  const PolicyMap::Entry& policy,
+                                  const bool is_user_cloud_merging_enabled);
+
   virtual ~PolicyMerger();
   virtual void Merge(PolicyMap::PolicyMapType* policies) const = 0;
 };
@@ -39,17 +44,25 @@ class POLICY_EXPORT PolicyListMerger : public PolicyMerger {
   // Merges the list policies from |policies| that have multiple sources.
   void Merge(PolicyMap::PolicyMapType* policies) const override;
 
+  // Sets the variable used for determining if user cloud merging is enabled.
+  void SetAllowUserCloudPolicyMerging(bool allowed);
+
  private:
   // Returns True if |policy_name| is in the list of policies to merge and if
   // |policy| has values from different sources that share the same level,
   // target and scope.
   bool CanMerge(const std::string& policy_name, PolicyMap::Entry& policy) const;
 
+  // Returns True if user cloud policy merging is enabled through the
+  // CloudUserPolicyMerge policy and the current user is affiliated.
+  bool AllowUserCloudPolicyMerging() const;
+
   // Merges the values of |policy| if they come from multiple sources. Keeps
   // track of the original values by leaving them as conflicts. |policy| must
   // remain unchanged if there is nothing to merge.
   void DoMerge(PolicyMap::Entry* policy) const;
 
+  bool allow_user_cloud_policy_merging_ = false;
   const base::flat_set<std::string> policies_to_merge_;
 
   DISALLOW_COPY_AND_ASSIGN(PolicyListMerger);
@@ -70,17 +83,25 @@ class POLICY_EXPORT PolicyDictionaryMerger : public PolicyMerger {
   void SetAllowedPoliciesForTesting(
       base::flat_set<std::string> allowed_policies);
 
+  // Sets the variable used for determining if user cloud merging is enabled.
+  void SetAllowUserCloudPolicyMerging(bool allowed);
+
  private:
   // Returns True if |policy_name| is in the list of policies to merge and if
   // |policy| has values from different sources that share the same level,
   // target and scope.
   bool CanMerge(const std::string& policy_name, PolicyMap::Entry& policy) const;
 
+  // Returns True if user cloud policy merging is enabled through the
+  // CloudUserPolicyMerge policy and the current user is affiliated.
+  bool AllowUserCloudPolicyMerging() const;
+
   // Merges the values of |policy| if they come from multiple sources. Keeps
   // track of the original values by leaving them as conflicts. |policy| stays
   // intact if there is nothing to merge.
   void DoMerge(PolicyMap::Entry* policy) const;
 
+  bool allow_user_cloud_policy_merging_ = false;
   const base::flat_set<std::string> policies_to_merge_;
   base::flat_set<std::string> allowed_policies_;
 

components/policy/core/common/policy_service_impl.cc

@@ -408,6 +408,16 @@ void PolicyServiceImpl::MergeAndTriggerUpdates() {
   PolicyDictionaryMerger policy_dictionary_merger(
       std::move(policy_dictionaries_to_merge));
 
+  // Pass affiliation and CloudUserPolicyMerge values to both mergers.
+  const bool is_affiliated = chrome_policies.IsUserAffiliated();
+  const bool is_user_cloud_merging_enabled =
+      chrome_policies.GetValue(key::kCloudUserPolicyMerge) &&
+      chrome_policies.GetValue(key::kCloudUserPolicyMerge)->GetBool();
+  policy_list_merger.SetAllowUserCloudPolicyMerging(
+      is_affiliated && is_user_cloud_merging_enabled);
+  policy_dictionary_merger.SetAllowUserCloudPolicyMerging(
+      is_affiliated && is_user_cloud_merging_enabled);
+
   std::vector<PolicyMerger*> mergers{&policy_list_merger,
                                      &policy_dictionary_merger};