Update policy merge algorithm.

Original algorithm: choose the active policy provider with the highest
   priority and apply all policies from it.

Updated algorithm: for each policy, find the highest priority provider
 that has defined value, and returns that policy.

Bug: 1134303
Change-Id: I3461f476e5404f97a08a6a83f57346804e84e678
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2443541Reviewed-by: Sorin Jianu <sorin@chromium.org>
Reviewed-by: S. Ganesh <ganesh@chromium.org>
Commit-Queue: Xiaoling Bao <xiaolingbao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#813449}

chrome/updater/policy_service.cc

@@ -5,6 +5,7 @@
 #include "chrome/updater/policy_service.h"
 
 #include "base/check.h"
+#include "base/strings/string_util.h"
 #include "build/build_config.h"
 
 #if defined(OS_WIN)
@@ -17,19 +18,19 @@ namespace updater {
 
 // Only policy manager that are enterprise managed are used by the policy
 // service.
-PolicyService::PolicyService() : default_policy_manager_(GetPolicyManager()) {
+PolicyService::PolicyService() {
 #if defined(OS_WIN)
   auto group_policy_manager = std::make_unique<GroupPolicyManager>();
   if (group_policy_manager->IsManaged())
-    policy_managers_.emplace_back(std::move(group_policy_manager));
+    policy_managers_.push_back(std::move(group_policy_manager));
 #endif
     // TODO (crbug/1122118): Inject the DMPolicyManager here.
 #if defined(OS_MAC)
   auto mac_policy_manager = CreateManagedPreferencePolicyManager();
   if (mac_policy_manager->IsManaged())
-    policy_managers_.emplace_back(std::move(mac_policy_manager));
+    policy_managers_.push_back(std::move(mac_policy_manager));
 #endif
-  UpdateActivePolicyManager();
+  policy_managers_.push_back(GetPolicyManager());
 }
 
 PolicyService::~PolicyService() = default;
@@ -37,143 +38,164 @@ PolicyService::~PolicyService() = default;
 void PolicyService::SetPolicyManagersForTesting(
     std::vector<std::unique_ptr<PolicyManagerInterface>> managers) {
   policy_managers_ = std::move(managers);
-  UpdateActivePolicyManager();
 }
 
 std::string PolicyService::source() const {
-  return active_policy_manager_->source();
+  // Returns the source combination of all active policy providers, separated
+  // by ';'. For example: "group_policy;device_management". Note that the
+  // default provider is not "managed" and its source will be ignored.
+  std::vector<std::string> sources;
+  for (const std::unique_ptr<PolicyManagerInterface>& policy_manager :
+       policy_managers_) {
+    if (policy_manager->IsManaged())
+      sources.push_back(policy_manager->source());
+  }
+  return base::JoinString(sources, ";");
 }
 
 bool PolicyService::IsManaged() const {
-  return active_policy_manager_->IsManaged();
+  for (const std::unique_ptr<PolicyManagerInterface>& policy_manager :
+       policy_managers_) {
+    if (policy_manager->IsManaged())
+      return true;
+  }
+
+  return false;
 }
 
 bool PolicyService::GetLastCheckPeriodMinutes(int* minutes) const {
-  return active_policy_manager_->GetLastCheckPeriodMinutes(minutes) ||
-         (ShouldFallbackToDefaultManager() &&
-          default_policy_manager_->GetLastCheckPeriodMinutes(minutes));
+  for (const std::unique_ptr<PolicyManagerInterface>& policy_manager :
+       policy_managers_) {
+    if (policy_manager->GetLastCheckPeriodMinutes(minutes))
+      return true;
+  }
+
+  return false;
 }
 
 bool PolicyService::GetUpdatesSuppressedTimes(int* start_hour,
                                               int* start_min,
                                               int* duration_min) const {
-  return active_policy_manager_->GetUpdatesSuppressedTimes(
-             start_hour, start_min, duration_min) ||
-         (ShouldFallbackToDefaultManager() &&
-          default_policy_manager_->GetUpdatesSuppressedTimes(
-              start_hour, start_min, duration_min));
+  for (const std::unique_ptr<PolicyManagerInterface>& policy_manager :
+       policy_managers_) {
+    if (policy_manager->GetUpdatesSuppressedTimes(start_hour, start_min,
+                                                  duration_min)) {
+      return true;
+    }
+  }
+  return false;
 }
 
 bool PolicyService::GetDownloadPreferenceGroupPolicy(
     std::string* download_preference) const {
-  return active_policy_manager_->GetDownloadPreferenceGroupPolicy(
-             download_preference) ||
-         (ShouldFallbackToDefaultManager() &&
-          default_policy_manager_->GetDownloadPreferenceGroupPolicy(
-              download_preference));
+  for (const std::unique_ptr<PolicyManagerInterface>& policy_manager :
+       policy_managers_) {
+    if (policy_manager->GetDownloadPreferenceGroupPolicy(download_preference))
+      return true;
+  }
+  return false;
 }
 
 bool PolicyService::GetPackageCacheSizeLimitMBytes(
     int* cache_size_limit) const {
-  return active_policy_manager_->GetPackageCacheSizeLimitMBytes(
-             cache_size_limit) ||
-         (ShouldFallbackToDefaultManager() &&
-          default_policy_manager_->GetPackageCacheSizeLimitMBytes(
-              cache_size_limit));
+  for (const std::unique_ptr<PolicyManagerInterface>& policy_manager :
+       policy_managers_) {
+    if (policy_manager->GetPackageCacheSizeLimitMBytes(cache_size_limit))
+      return true;
+  }
+  return false;
 }
 
 bool PolicyService::GetPackageCacheExpirationTimeDays(
     int* cache_life_limit) const {
-  return active_policy_manager_->GetPackageCacheExpirationTimeDays(
-             cache_life_limit) ||
-         (ShouldFallbackToDefaultManager() &&
-          default_policy_manager_->GetPackageCacheExpirationTimeDays(
-              cache_life_limit));
+  for (const std::unique_ptr<PolicyManagerInterface>& policy_manager :
+       policy_managers_) {
+    if (policy_manager->GetPackageCacheExpirationTimeDays(cache_life_limit))
+      return true;
+  }
+  return false;
 }
 
 bool PolicyService::GetEffectivePolicyForAppInstalls(
     const std::string& app_id,
     int* install_policy) const {
-  return active_policy_manager_->GetEffectivePolicyForAppInstalls(
-             app_id, install_policy) ||
-         (ShouldFallbackToDefaultManager() &&
-          default_policy_manager_->GetEffectivePolicyForAppInstalls(
-              app_id, install_policy));
+  for (const std::unique_ptr<PolicyManagerInterface>& policy_manager :
+       policy_managers_) {
+    if (policy_manager->GetEffectivePolicyForAppInstalls(app_id,
+                                                         install_policy))
+      return true;
+  }
+  return false;
 }
 
 bool PolicyService::GetEffectivePolicyForAppUpdates(const std::string& app_id,
                                                     int* update_policy) const {
-  return active_policy_manager_->GetEffectivePolicyForAppUpdates(
-             app_id, update_policy) ||
-         (ShouldFallbackToDefaultManager() &&
-          default_policy_manager_->GetEffectivePolicyForAppUpdates(
-              app_id, update_policy));
+  for (const std::unique_ptr<PolicyManagerInterface>& policy_manager :
+       policy_managers_) {
+    if (policy_manager->GetEffectivePolicyForAppUpdates(app_id, update_policy))
+      return true;
+  }
+  return false;
 }
 
 bool PolicyService::GetTargetChannel(const std::string& app_id,
                                      std::string* channel) const {
-  return active_policy_manager_->GetTargetChannel(app_id, channel) ||
-         (ShouldFallbackToDefaultManager() &&
-          default_policy_manager_->GetTargetChannel(app_id, channel));
+  for (const std::unique_ptr<PolicyManagerInterface>& policy_manager :
+       policy_managers_) {
+    if (policy_manager->GetTargetChannel(app_id, channel))
+      return true;
+  }
+  return false;
 }
 
 bool PolicyService::GetTargetVersionPrefix(
     const std::string& app_id,
     std::string* target_version_prefix) const {
-  return active_policy_manager_->GetTargetVersionPrefix(
-             app_id, target_version_prefix) ||
-         (ShouldFallbackToDefaultManager() &&
-          default_policy_manager_->GetTargetVersionPrefix(
-              app_id, target_version_prefix));
+  for (const std::unique_ptr<PolicyManagerInterface>& policy_manager :
+       policy_managers_) {
+    if (policy_manager->GetTargetVersionPrefix(app_id, target_version_prefix))
+      return true;
+  }
+  return false;
 }
 
 bool PolicyService::IsRollbackToTargetVersionAllowed(
     const std::string& app_id,
     bool* rollback_allowed) const {
-  return active_policy_manager_->IsRollbackToTargetVersionAllowed(
-             app_id, rollback_allowed) ||
-         (ShouldFallbackToDefaultManager() &&
-          default_policy_manager_->IsRollbackToTargetVersionAllowed(
-              app_id, rollback_allowed));
+  for (const std::unique_ptr<PolicyManagerInterface>& policy_manager :
+       policy_managers_) {
+    if (policy_manager->IsRollbackToTargetVersionAllowed(app_id,
+                                                         rollback_allowed))
+      return true;
+  }
+  return false;
 }
 
 bool PolicyService::GetProxyMode(std::string* proxy_mode) const {
-  return active_policy_manager_->GetProxyMode(proxy_mode) ||
-         (ShouldFallbackToDefaultManager() &&
-          default_policy_manager_->GetProxyMode(proxy_mode));
+  for (const std::unique_ptr<PolicyManagerInterface>& policy_manager :
+       policy_managers_) {
+    if (policy_manager->GetProxyMode(proxy_mode))
+      return true;
+  }
+  return false;
 }
 
 bool PolicyService::GetProxyPacUrl(std::string* proxy_pac_url) const {
-  return active_policy_manager_->GetProxyPacUrl(proxy_pac_url) ||
-         (ShouldFallbackToDefaultManager() &&
-          default_policy_manager_->GetProxyPacUrl(proxy_pac_url));
+  for (const std::unique_ptr<PolicyManagerInterface>& policy_manager :
+       policy_managers_) {
+    if (policy_manager->GetProxyPacUrl(proxy_pac_url))
+      return true;
+  }
+  return false;
 }
 
 bool PolicyService::GetProxyServer(std::string* proxy_server) const {
-  return active_policy_manager_->GetProxyServer(proxy_server) ||
-         (ShouldFallbackToDefaultManager() &&
-          default_policy_manager_->GetProxyServer(proxy_server));
-}
-
-const PolicyManagerInterface& PolicyService::GetActivePolicyManager() {
-  DCHECK(active_policy_manager_);
-  return *active_policy_manager_;
-}
-
-bool PolicyService::ShouldFallbackToDefaultManager() const {
-  return active_policy_manager_ != default_policy_manager_.get();
-}
-
-void PolicyService::UpdateActivePolicyManager() {
-  // The active policy manager is either the default policy manager or the
-  // manager with the highest level that is managed.
-  active_policy_manager_ = default_policy_manager_.get();
-  for (const auto& manager : policy_managers_) {
-    if (manager->IsManaged()) {
-      active_policy_manager_ = manager.get();
-      return;
-    }
+  for (const std::unique_ptr<PolicyManagerInterface>& policy_manager :
+       policy_managers_) {
+    if (policy_manager->GetProxyServer(proxy_server))
+      return true;
   }
+  return false;
 }
 
 std::unique_ptr<PolicyService> GetUpdaterPolicyService() {

chrome/updater/policy_service.h

@@ -61,17 +61,9 @@ class PolicyService : public PolicyManagerInterface {
   const PolicyManagerInterface& GetActivePolicyManager();
 
  private:
-  bool ShouldFallbackToDefaultManager() const;
-
-  // Sets the policy manager that is managed and has the highest priority as the
-  // active policy manager. If no manager is managed, use the default policy
-  // manager as the active one.
-  void UpdateActivePolicyManager();
   // List of policy managers in descending order of priority. The first policy
   // manager's policies takes precedence over the following.
   std::vector<std::unique_ptr<PolicyManagerInterface>> policy_managers_;
-  std::unique_ptr<PolicyManagerInterface> default_policy_manager_;
-  const PolicyManagerInterface* active_policy_manager_;
 };
 
 std::unique_ptr<PolicyService> GetUpdaterPolicyService();

chrome/updater/policy_service_unittest.cc

@@ -16,10 +16,11 @@ namespace updater {
 // Policy and Device Management.
 class FakePolicyManager : public PolicyManagerInterface {
  public:
+  explicit FakePolicyManager(const std::string& source) : source_(source) {}
   ~FakePolicyManager() override = default;
 
   std::string source() const override { return source_; }
-  bool IsManaged() const override { return managed_; }
+  bool IsManaged() const override { return true; }
   bool GetLastCheckPeriodMinutes(int* minutes) const override { return false; }
   bool GetUpdatesSuppressedTimes(int* start_hour,
                                  int* start_min,
@@ -28,7 +29,14 @@ class FakePolicyManager : public PolicyManagerInterface {
   }
   bool GetDownloadPreferenceGroupPolicy(
       std::string* download_preference) const override {
+    if (download_preference_.empty())
       return false;
+
+    *download_preference = download_preference_;
+    return true;
+  }
+  void SetDownloadPreferenceGroupPolicy(const std::string& preference) {
+    download_preference_ = preference;
   }
   bool GetPackageCacheSizeLimitMBytes(int* cache_size_limit) const override {
     return false;
@@ -42,7 +50,14 @@ class FakePolicyManager : public PolicyManagerInterface {
   }
   bool GetEffectivePolicyForAppUpdates(const std::string& app_id,
                                        int* update_policy) const override {
+    auto value = update_policies_.find(app_id);
+    if (value == update_policies_.end())
       return false;
+    *update_policy = value->second;
+    return true;
+  }
+  void SetUpdatePolicy(const std::string& app_id, int update_policy) {
+    update_policies_[app_id] = update_policy;
   }
   bool GetTargetVersionPrefix(
       const std::string& app_id,
@@ -72,67 +87,96 @@ class FakePolicyManager : public PolicyManagerInterface {
     channels_[app_id] = std::move(channel);
   }
 
-  static std::unique_ptr<FakePolicyManager> GetTestingPolicyManager(
-      std::string source,
-      bool managed) {
-    auto manager = std::make_unique<FakePolicyManager>();
-    manager->source_ = std::move(source);
-    manager->managed_ = managed;
-    return manager;
-  }
-
  private:
   std::string source_;
+  std::string download_preference_;
   std::map<std::string, std::string> channels_;
-  bool managed_;
+  std::map<std::string, int> update_policies_;
 };
 
-TEST(PolicyService, ReturnsHighestPriorityManagedPolicyManager) {
+TEST(PolicyService, DefaultPolicyValue) {
   std::unique_ptr<PolicyService> policy_service(GetUpdaterPolicyService());
   std::vector<std::unique_ptr<PolicyManagerInterface>> managers;
-  managers.emplace_back(
-      FakePolicyManager::GetTestingPolicyManager("highest_unmanaged", false));
-  managers.emplace_back(
-      FakePolicyManager::GetTestingPolicyManager("highest_managed", true));
-  managers.emplace_back(
-      FakePolicyManager::GetTestingPolicyManager("managed", true));
-  managers.emplace_back(
-      FakePolicyManager::GetTestingPolicyManager("lowest_managed", true));
-  managers.emplace_back(
-      FakePolicyManager::GetTestingPolicyManager("lowest_unmanaged", false));
+  managers.push_back(GetPolicyManager());
   policy_service->SetPolicyManagersForTesting(std::move(managers));
-  ASSERT_EQ("highest_managed",
-            policy_service->GetActivePolicyManager().source());
-}
+  EXPECT_EQ(policy_service->source(), "");
 
-TEST(PolicyService, ReturnsDefaultPolicyManager) {
-  std::unique_ptr<PolicyService> policy_service(GetUpdaterPolicyService());
-  policy_service->SetPolicyManagersForTesting({});
-  ASSERT_EQ("default", policy_service->GetActivePolicyManager().source());
+  std::string version_prefix;
+  EXPECT_FALSE(policy_service->GetTargetVersionPrefix("", &version_prefix));
+  int last_check = 0;
+  EXPECT_FALSE(policy_service->GetLastCheckPeriodMinutes(&last_check));
 }
 
-TEST(PolicyService, TargetChannelUnmanagedSource) {
+TEST(PolicyService, SinglePolicyManager) {
   std::unique_ptr<PolicyService> policy_service(GetUpdaterPolicyService());
-  auto manager = FakePolicyManager::GetTestingPolicyManager("unmanaged", false);
-  manager->SetChannel("", "channel");
+  auto manager = std::make_unique<FakePolicyManager>("test_source");
+  manager->SetChannel("app1", "test_channel");
+  manager->SetUpdatePolicy("app2", 3);
   std::vector<std::unique_ptr<PolicyManagerInterface>> managers;
-  managers.emplace_back(std::move(manager));
+  managers.push_back(std::move(manager));
   policy_service->SetPolicyManagersForTesting(std::move(managers));
+  EXPECT_EQ(policy_service->source(), "test_source");
+
   std::string channel;
-  policy_service->GetTargetChannel("", &channel);
-  ASSERT_TRUE(channel.empty());
+  EXPECT_FALSE(policy_service->GetTargetChannel("app2", &channel));
+  EXPECT_TRUE(policy_service->GetTargetChannel("app1", &channel));
+  EXPECT_EQ(channel, "test_channel");
+
+  int update_policy = 0;
+  EXPECT_FALSE(
+      policy_service->GetEffectivePolicyForAppUpdates("app1", &update_policy));
+  EXPECT_TRUE(
+      policy_service->GetEffectivePolicyForAppUpdates("app2", &update_policy));
+  EXPECT_EQ(update_policy, 3);
 }
 
-TEST(PolicyService, TargetChannelManagedSource) {
+TEST(PolicyService, MultiplePolicyManagers) {
   std::unique_ptr<PolicyService> policy_service(GetUpdaterPolicyService());
-  auto manager = FakePolicyManager::GetTestingPolicyManager("managed", true);
-  manager->SetChannel("", "channel");
   std::vector<std::unique_ptr<PolicyManagerInterface>> managers;
-  managers.emplace_back(std::move(manager));
+
+  auto manager = std::make_unique<FakePolicyManager>("group_policy");
+  manager->SetChannel("app1", "channel_gp");
+  manager->SetUpdatePolicy("app2", 1);
+  managers.push_back(std::move(manager));
+
+  manager = std::make_unique<FakePolicyManager>("device_management");
+  manager->SetChannel("app1", "channel_dm");
+  manager->SetUpdatePolicy("app1", 3);
+  managers.push_back(std::move(manager));
+
+  manager = std::make_unique<FakePolicyManager>("imaginary");
+  manager->SetChannel("app1", "channel_imaginary");
+  manager->SetUpdatePolicy("app1", 2);
+  manager->SetDownloadPreferenceGroupPolicy("cacheable");
+  managers.push_back(std::move(manager));
+
+  // The default policy manager.
+  managers.push_back(GetPolicyManager());
+
   policy_service->SetPolicyManagersForTesting(std::move(managers));
+  EXPECT_EQ(policy_service->source(),
+            "group_policy;device_management;imaginary");
+
   std::string channel;
-  policy_service->GetTargetChannel("", &channel);
-  ASSERT_EQ(channel, "channel");
+  EXPECT_TRUE(policy_service->GetTargetChannel("app1", &channel));
+  EXPECT_EQ(channel, "channel_gp");
+
+  int update_policy = 0;
+  EXPECT_TRUE(
+      policy_service->GetEffectivePolicyForAppUpdates("app1", &update_policy));
+  EXPECT_EQ(update_policy, 3);
+  EXPECT_TRUE(
+      policy_service->GetEffectivePolicyForAppUpdates("app2", &update_policy));
+  EXPECT_EQ(update_policy, 1);
+
+  std::string download_preference;
+  EXPECT_TRUE(
+      policy_service->GetDownloadPreferenceGroupPolicy(&download_preference));
+  EXPECT_EQ(download_preference, "cacheable");
+
+  int cache_size_limit = 0;
+  EXPECT_FALSE(
+      policy_service->GetPackageCacheSizeLimitMBytes(&cache_size_limit));
 }
 
 }  // namespace updater