Enable policy status query for chrome://policy page.

Changes:
1) Add policy providers even when they are not active (to show potential policy conflict)
2) Returns caller the optional policy status (source of a policy, conflict etc),

Bug: 1141124
Change-Id: I90356ac7c0ec376fed8cd57a1c7dbcc76a0c7814
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2490275Reviewed-by: Sorin Jianu <sorin@chromium.org>
Commit-Queue: Xiaoling Bao <xiaolingbao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#824150}

chrome/updater/constants.h

@@ -194,6 +194,7 @@ extern const char kProxyModeSystem[];
 
 extern const char kDownloadPreferenceCacheable[];
 
+constexpr int kPolicyNotSet = -1;
 constexpr int kPolicyDisabled = 0;
 constexpr int kPolicyEnabled = 1;
 constexpr int kPolicyEnabledMachineOnly = 4;

chrome/updater/device_management/dm_policy_manager.cc

@@ -8,6 +8,7 @@
 #include "base/strings/string_util.h"
 #include "build/build_config.h"
 #include "chrome/updater/constants.h"
+#include "chrome/updater/policy_manager.h"
 
 namespace updater {
 
@@ -69,9 +70,8 @@ bool DMPolicyManager::GetLastCheckPeriodMinutes(int* minutes) const {
   return true;
 }
 
-bool DMPolicyManager::GetUpdatesSuppressedTimes(int* start_hour,
-                                                int* start_min,
-                                                int* duration_min) const {
+bool DMPolicyManager::GetUpdatesSuppressedTimes(
+    UpdatesSuppressedTimes* suppressed_times) const {
   if (!omaha_settings_.has_updates_suppressed())
     return false;
 
@@ -81,9 +81,9 @@ bool DMPolicyManager::GetUpdatesSuppressedTimes(int* start_hour,
       !updates_suppressed.has_duration_min())
     return false;
 
-  *start_hour = updates_suppressed.start_hour();
-  *start_min = updates_suppressed.start_minute();
-  *duration_min = updates_suppressed.duration_min();
+  suppressed_times->start_hour = updates_suppressed.start_hour();
+  suppressed_times->start_minute = updates_suppressed.start_minute();
+  suppressed_times->duration_minute = updates_suppressed.duration_min();
   return true;
 }
 

chrome/updater/device_management/dm_policy_manager.h

@@ -29,9 +29,8 @@ class DMPolicyManager : public PolicyManagerInterface {
   bool IsManaged() const override;
 
   bool GetLastCheckPeriodMinutes(int* minutes) const override;
-  bool GetUpdatesSuppressedTimes(int* start_hour,
-                                 int* start_min,
-                                 int* duration_min) const override;
+  bool GetUpdatesSuppressedTimes(
+      UpdatesSuppressedTimes* suppressed_times) const override;
   bool GetDownloadPreferenceGroupPolicy(
       std::string* download_preference) const override;
   bool GetProxyMode(std::string* proxy_mode) const override;

chrome/updater/device_management/dm_policy_manager_unittest.cc

@@ -114,11 +114,8 @@ TEST(DMPolicyManager, PolicyManagerFromEmptyProto) {
   EXPECT_FALSE(
       policy_manager->GetLastCheckPeriodMinutes(&last_check_period_minutes));
 
-  int start_hour = 0;
-  int start_minute = 0;
-  int duration_minute = 0;
-  EXPECT_FALSE(policy_manager->GetUpdatesSuppressedTimes(
-      &start_hour, &start_minute, &duration_minute));
+  UpdatesSuppressedTimes suppressed_times;
+  EXPECT_FALSE(policy_manager->GetUpdatesSuppressedTimes(&suppressed_times));
 
   std::string download_preference;
   EXPECT_FALSE(
@@ -192,14 +189,11 @@ TEST(DMPolicyManager, PolicyManagerFromProto) {
       policy_manager->GetLastCheckPeriodMinutes(&last_check_period_minutes));
   EXPECT_EQ(last_check_period_minutes, 111);
 
-  int start_hour = 0;
-  int start_minute = 0;
-  int duration_minute = 0;
-  EXPECT_TRUE(policy_manager->GetUpdatesSuppressedTimes(
-      &start_hour, &start_minute, &duration_minute));
-  EXPECT_EQ(start_hour, 9);
-  EXPECT_EQ(start_minute, 30);
-  EXPECT_EQ(duration_minute, 120);
+  UpdatesSuppressedTimes suppressed_times;
+  EXPECT_TRUE(policy_manager->GetUpdatesSuppressedTimes(&suppressed_times));
+  EXPECT_EQ(suppressed_times.start_hour, 9);
+  EXPECT_EQ(suppressed_times.start_minute, 30);
+  EXPECT_EQ(suppressed_times.duration_minute, 120);
 
   std::string download_preference;
   EXPECT_TRUE(
@@ -284,11 +278,8 @@ TEST(DMPolicyManager, PolicyManagerFromDMResponse) {
   EXPECT_FALSE(
       policy_manager->GetLastCheckPeriodMinutes(&last_check_period_minutes));
 
-  int start_hour = 0;
-  int start_minute = 0;
-  int duration_minute = 0;
-  EXPECT_FALSE(policy_manager->GetUpdatesSuppressedTimes(
-      &start_hour, &start_minute, &duration_minute));
+  UpdatesSuppressedTimes suppressed_times;
+  EXPECT_FALSE(policy_manager->GetUpdatesSuppressedTimes(&suppressed_times));
 
   std::string download_preference;
   EXPECT_FALSE(

chrome/updater/device_management/dm_storage_unittest.cc

@@ -191,15 +191,11 @@ TEST(DMStorage, ReadCachedOmahaPolicy) {
   EXPECT_TRUE(policy_manager->GetLastCheckPeriodMinutes(&check_interval));
   EXPECT_EQ(check_interval, 111);
 
-  int suppressed_start_hour = 0;
-  int suppressed_start_minute = 0;
-  int suppressed_duration_minute = 0;
-  EXPECT_TRUE(policy_manager->GetUpdatesSuppressedTimes(
-      &suppressed_start_hour, &suppressed_start_minute,
-      &suppressed_duration_minute));
-  EXPECT_EQ(suppressed_start_hour, 8);
-  EXPECT_EQ(suppressed_start_minute, 8);
-  EXPECT_EQ(suppressed_duration_minute, 47);
+  UpdatesSuppressedTimes suppressed_times;
+  EXPECT_TRUE(policy_manager->GetUpdatesSuppressedTimes(&suppressed_times));
+  EXPECT_EQ(suppressed_times.start_hour, 8);
+  EXPECT_EQ(suppressed_times.start_minute, 8);
+  EXPECT_EQ(suppressed_times.duration_minute, 47);
 
   // Proxy policies.
   std::string proxy_mode;

chrome/updater/installer.cc

@@ -85,8 +85,10 @@ update_client::CrxComponent Installer::MakeCrxComponent() {
   // |component.channel| is an empty string. Possible failure cases are if the
   // machine is not managed, the policy was not set or any other unexpected
   // error.
-  if (!GetUpdaterPolicyService()->GetTargetChannel(app_id_, &component.channel))
+  if (!GetUpdaterPolicyService()->GetTargetChannel(app_id_, nullptr,
+                                                   &component.channel)) {
     component.channel.clear();
+  }
   return component;
 }
 

chrome/updater/mac/managed_preference_policy_manager.mm

@@ -11,6 +11,7 @@
 #include "base/strings/string16.h"
 #include "base/strings/sys_string_conversions.h"
 #include "chrome/updater/mac/managed_preference_policy_manager_impl.h"
+#include "chrome/updater/policy_manager.h"
 
 namespace updater {
 
@@ -32,9 +33,8 @@ class ManagedPreferencePolicyManager : public PolicyManagerInterface {
   bool IsManaged() const override;
 
   bool GetLastCheckPeriodMinutes(int* minutes) const override;
-  bool GetUpdatesSuppressedTimes(int* start_hour,
-                                 int* start_min,
-                                 int* duration_min) const override;
+  bool GetUpdatesSuppressedTimes(
+      UpdatesSuppressedTimes* suppressed_times) const override;
   bool GetDownloadPreferenceGroupPolicy(
       std::string* download_preference) const override;
   bool GetPackageCacheSizeLimitMBytes(int* cache_size_limit) const override;
@@ -81,15 +81,9 @@ bool ManagedPreferencePolicyManager::GetLastCheckPeriodMinutes(
 }
 
 bool ManagedPreferencePolicyManager::GetUpdatesSuppressedTimes(
-    int* start_hour,
-    int* start_min,
-    int* duration_min) const {
-  CRUUpdatesSuppressed updatesSuppressed = [impl_ updatesSuppressed];
-  *start_hour = updatesSuppressed.start_hour;
-  *start_min = updatesSuppressed.start_minute;
-  *duration_min = updatesSuppressed.duration_minute;
-  return *start_hour != kPolicyNotSet || *start_min != kPolicyNotSet ||
-         *duration_min != kPolicyNotSet;
+    UpdatesSuppressedTimes* suppressed_times) const {
+  *suppressed_times = [impl_ updatesSuppressed];
+  return suppressed_times->valid();
 }
 
 bool ManagedPreferencePolicyManager::GetDownloadPreferenceGroupPolicy(

chrome/updater/mac/managed_preference_policy_manager_impl.h

@@ -7,6 +7,8 @@
 
 #import <Foundation/Foundation.h>
 
+#include "chrome/updater/policy_manager.h"
+
 // TODO: crbug/1073980
 //     Add a doc link for the managed preferences dictionary format.
 //
@@ -38,19 +40,10 @@
 //    </dict>
 //  </dict>
 
-constexpr int kPolicyNotSet = -1;
-
 using CRUAppPolicyDictionary = NSDictionary<NSString*, id>;
 using CRUUpdatePolicyDictionary =
     NSDictionary<NSString*, CRUAppPolicyDictionary*>;
 
-// Structure describes time window when update check is suppressed.
-struct CRUUpdatesSuppressed {
-  int start_hour = kPolicyNotSet;
-  int start_minute = kPolicyNotSet;
-  int duration_minute = kPolicyNotSet;
-};
-
 @interface CRUManagedPreferencePolicyManager : NSObject
 
 @property(nonatomic, readonly, nullable) NSString* source;
@@ -60,7 +53,8 @@ struct CRUUpdatesSuppressed {
 @property(nonatomic, readonly) int lastCheckPeriodMinutes;
 @property(nonatomic, readonly) int defaultUpdatePolicy;
 @property(nonatomic, readonly, nullable) NSString* downloadPreference;
-@property(nonatomic, readonly) CRUUpdatesSuppressed updatesSuppressed;
+@property(nonatomic, readonly)
+    updater::UpdatesSuppressedTimes updatesSuppressed;
 @property(nonatomic, readonly, nullable) NSString* proxyMode;
 @property(nonatomic, readonly, nullable) NSString* proxyServer;
 @property(nonatomic, readonly, nullable) NSString* proxyPacURL;

chrome/updater/mac/managed_preference_policy_manager_impl.mm

@@ -6,6 +6,7 @@
 
 #include "base/mac/scoped_nsobject.h"
 #include "chrome/updater/constants.h"
+#include "chrome/updater/policy_manager.h"
 
 // Constants for managed preference policy keys.
 static NSString* kGlobalPolicyKey = @"global";
@@ -66,7 +67,8 @@ base::scoped_nsobject<NSString> ReadPolicyString(id value) {
 @property(nonatomic, readonly, nullable) NSString* proxyMode;
 @property(nonatomic, readonly, nullable) NSString* proxyServer;
 @property(nonatomic, readonly, nullable) NSString* proxyPacURL;
-@property(nonatomic, readonly) CRUUpdatesSuppressed updatesSuppressed;
+@property(nonatomic, readonly)
+    updater::UpdatesSuppressedTimes updatesSuppressed;
 
 @end
 
@@ -93,7 +95,7 @@ base::scoped_nsobject<NSString> ReadPolicyString(id value) {
 
 - (int)lastCheckPeriodMinutes {
   // LastCheckPeriodMinutes is not supported in Managed Preference policy.
-  return kPolicyNotSet;
+  return updater::kPolicyNotSet;
 }
 
 - (NSString*)downloadPreference {
@@ -235,14 +237,14 @@ base::scoped_nsobject<NSString> ReadPolicyString(id value) {
   return [_globalPolicy defaultUpdatePolicy];
 }
 
-- (CRUUpdatesSuppressed)updatesSuppressed {
+- (updater::UpdatesSuppressedTimes)updatesSuppressed {
   return [_globalPolicy updatesSuppressed];
 }
 
 - (int)appUpdatePolicy:(NSString*)appid {
   appid = appid.lowercaseString;
   if (![_appPolicies objectForKey:appid])
-    return kPolicyNotSet;
+    return updater::kPolicyNotSet;
   return [_appPolicies objectForKey:appid].updatePolicy;
 }
 
@@ -259,7 +261,7 @@ base::scoped_nsobject<NSString> ReadPolicyString(id value) {
 - (int)rollbackToTargetVersion:(NSString*)appid {
   appid = appid.lowercaseString;
   if (![_appPolicies objectForKey:appid])
-    return kPolicyNotSet;
+    return updater::kPolicyNotSet;
   return [_appPolicies objectForKey:appid].rollbackToTargetVersion;
 }
 

chrome/updater/policy_manager.cc

@@ -24,9 +24,8 @@ class DefaultPolicyManager : public PolicyManagerInterface {
   bool IsManaged() const override;
 
   bool GetLastCheckPeriodMinutes(int* minutes) const override;
-  bool GetUpdatesSuppressedTimes(int* start_hour,
-                                 int* start_min,
-                                 int* duration_min) const override;
+  bool GetUpdatesSuppressedTimes(
+      UpdatesSuppressedTimes* suppressed_times) const override;
   bool GetDownloadPreferenceGroupPolicy(
       std::string* download_preference) const override;
   bool GetPackageCacheSizeLimitMBytes(int* cache_size_limit) const override;
@@ -53,7 +52,7 @@ DefaultPolicyManager::DefaultPolicyManager() = default;
 DefaultPolicyManager::~DefaultPolicyManager() = default;
 
 bool DefaultPolicyManager::IsManaged() const {
-  return false;
+  return true;
 }
 
 std::string DefaultPolicyManager::source() const {
@@ -64,9 +63,8 @@ bool DefaultPolicyManager::GetLastCheckPeriodMinutes(int* minutes) const {
   return false;
 }
 
-bool DefaultPolicyManager::GetUpdatesSuppressedTimes(int* start_hour,
-                                                     int* start_min,
-                                                     int* duration_min) const {
+bool DefaultPolicyManager::GetUpdatesSuppressedTimes(
+    UpdatesSuppressedTimes* suppressed_times) const {
   return false;
 }
 

chrome/updater/policy_manager.h

@@ -8,8 +8,36 @@
 #include <memory>
 #include <string>
 
+#include "chrome/updater/constants.h"
+
 namespace updater {
 
+// Updates are suppressed if the current time falls between the start time and
+// the duration. The duration does not account for daylight savings time.
+// For instance, if the start time is 22:00 hours, and with a duration of 8
+// hours, the updates will be suppressed for 8 hours regardless of whether
+// daylight savings time changes happen in between.
+struct UpdatesSuppressedTimes {
+  int start_hour = kPolicyNotSet;
+  int start_minute = kPolicyNotSet;
+  int duration_minute = kPolicyNotSet;
+
+  bool operator==(const UpdatesSuppressedTimes& other) const {
+    return start_hour == other.start_hour &&
+           start_minute == other.start_minute &&
+           duration_minute == other.duration_minute;
+  }
+
+  bool operator!=(const UpdatesSuppressedTimes& other) const {
+    return !(*this == other);
+  }
+
+  bool valid() const {
+    return start_hour != kPolicyNotSet && start_minute != kPolicyNotSet &&
+           duration_minute != kPolicyNotSet;
+  }
+};
+
 // The Policy Manager Interface is implemented by policy managers such as Group
 // Policy and Device Management.
 class PolicyManagerInterface {
@@ -32,15 +60,10 @@ class PolicyManagerInterface {
   virtual bool GetLastCheckPeriodMinutes(int* minutes) const = 0;
 
   // For domain-joined machines, checks the current time against the times that
-  // updates are suppressed. Updates are suppressed if the current time falls
-  // between the start time and the duration.
-  // The duration does not account for daylight savings time. For instance, if
-  // the start time is 22:00 hours, and with a duration of 8 hours, the updates
-  // will be suppressed for 8 hours regardless of whether daylight savings time
-  // changes happen in between.
-  virtual bool GetUpdatesSuppressedTimes(int* start_hour,
-                                         int* start_min,
-                                         int* duration_min) const = 0;
+  // updates are suppressed.
+  virtual bool GetUpdatesSuppressedTimes(
+      UpdatesSuppressedTimes* suppressed_times) const = 0;
+
   // Returns the policy for the download preference.
   virtual bool GetDownloadPreferenceGroupPolicy(
       std::string* download_preference) const = 0;

chrome/updater/policy_manager_unittest.cc

@@ -9,7 +9,7 @@ namespace updater {
 
 TEST(PolicyManager, GetPolicyManager) {
   std::unique_ptr<PolicyManagerInterface> policy_manager(GetPolicyManager());
-  ASSERT_FALSE(policy_manager->IsManaged());
+  ASSERT_TRUE(policy_manager->IsManaged());
 }
 
 }  // namespace updater

chrome/updater/policy_service.cc

@@ -4,6 +4,10 @@
 
 #include "chrome/updater/policy_service.h"
 
+#include <algorithm>
+
+#include "base/bind.h"
+#include "base/callback.h"
 #include "base/check.h"
 #include "base/strings/string_util.h"
 #include "build/build_config.h"
@@ -16,27 +20,48 @@
 
 namespace updater {
 
-// Only policy manager that are enterprise managed are used by the policy
+// Only policy managers that are enterprise managed are used by the policy
 // service.
 PolicyService::PolicyService() {
 #if defined(OS_WIN)
-  auto group_policy_manager = std::make_unique<GroupPolicyManager>();
-  if (group_policy_manager->IsManaged())
-    policy_managers_.push_back(std::move(group_policy_manager));
+  InsertPolicyManager(std::make_unique<GroupPolicyManager>());
 #endif
-    // TODO (crbug/1122118): Inject the DMPolicyManager here.
+  // TODO (crbug/1122118): Inject the DMPolicyManager here.
 #if defined(OS_MAC)
-  auto mac_policy_manager = CreateManagedPreferencePolicyManager();
-  if (mac_policy_manager->IsManaged())
-    policy_managers_.push_back(std::move(mac_policy_manager));
+  InsertPolicyManager(CreateManagedPreferencePolicyManager());
 #endif
-  policy_managers_.push_back(GetPolicyManager());
+  InsertPolicyManager(GetPolicyManager());
 }
 
 PolicyService::~PolicyService() = default;
 
+void PolicyService::InsertPolicyManager(
+    std::unique_ptr<PolicyManagerInterface> manager) {
+  if (manager->IsManaged()) {
+    for (auto it = policy_managers_.begin(); it != policy_managers_.end();
+         ++it) {
+      if (!(*it)->IsManaged()) {
+        policy_managers_.insert(it, std::move(manager));
+        return;
+      }
+    }
+  }
+
+  policy_managers_.push_back(std::move(manager));
+}
+
 void PolicyService::SetPolicyManagersForTesting(
     std::vector<std::unique_ptr<PolicyManagerInterface>> managers) {
+  // Testing managers are not inserted via InsertPolicyManager(). Do a
+  // quick sanity check that all managed providers are ahead of non-managed
+  // providers (there should be no adjacent pair with the reversed order).
+  DCHECK(std::adjacent_find(
+             managers.begin(), managers.end(),
+             [](const std::unique_ptr<PolicyManagerInterface>& first,
+                const std::unique_ptr<PolicyManagerInterface>& second) {
+               return !first->IsManaged() && second->IsManaged();
+             }) == managers.end());
+
   policy_managers_ = std::move(managers);
 }
 
@@ -53,149 +78,163 @@ std::string PolicyService::source() const {
   return base::JoinString(sources, ";");
 }
 
-bool PolicyService::IsManaged() const {
-  for (const std::unique_ptr<PolicyManagerInterface>& policy_manager :
-       policy_managers_) {
-    if (policy_manager->IsManaged())
-      return true;
-  }
-
-  return false;
+bool PolicyService::GetLastCheckPeriodMinutes(PolicyStatus<int>* policy_status,
+                                              int* minutes) const {
+  return QueryPolicy(
+      base::BindRepeating(&PolicyManagerInterface::GetLastCheckPeriodMinutes),
+      policy_status, minutes);
 }
 
-bool PolicyService::GetLastCheckPeriodMinutes(int* minutes) const {
-  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 {
-  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::GetUpdatesSuppressedTimes(
+    PolicyStatus<UpdatesSuppressedTimes>* policy_status,
+    UpdatesSuppressedTimes* suppressed_times) const {
+  return QueryPolicy(
+      base::BindRepeating(&PolicyManagerInterface::GetUpdatesSuppressedTimes),
+      policy_status, suppressed_times);
 }
 
 bool PolicyService::GetDownloadPreferenceGroupPolicy(
+    PolicyStatus<std::string>* policy_status,
     std::string* download_preference) const {
-  for (const std::unique_ptr<PolicyManagerInterface>& policy_manager :
-       policy_managers_) {
-    if (policy_manager->GetDownloadPreferenceGroupPolicy(download_preference))
-      return true;
-  }
-  return false;
+  return QueryPolicy(
+      base::BindRepeating(
+          &PolicyManagerInterface::GetDownloadPreferenceGroupPolicy),
+      policy_status, download_preference);
 }
 
 bool PolicyService::GetPackageCacheSizeLimitMBytes(
+    PolicyStatus<int>* policy_status,
     int* cache_size_limit) const {
-  for (const std::unique_ptr<PolicyManagerInterface>& policy_manager :
-       policy_managers_) {
-    if (policy_manager->GetPackageCacheSizeLimitMBytes(cache_size_limit))
-      return true;
-  }
-  return false;
+  return QueryPolicy(
+      base::BindRepeating(
+          &PolicyManagerInterface::GetPackageCacheSizeLimitMBytes),
+      policy_status, cache_size_limit);
 }
 
 bool PolicyService::GetPackageCacheExpirationTimeDays(
+    PolicyStatus<int>* policy_status,
     int* cache_life_limit) const {
-  for (const std::unique_ptr<PolicyManagerInterface>& policy_manager :
-       policy_managers_) {
-    if (policy_manager->GetPackageCacheExpirationTimeDays(cache_life_limit))
-      return true;
-  }
-  return false;
+  return QueryPolicy(
+      base::BindRepeating(
+          &PolicyManagerInterface::GetPackageCacheExpirationTimeDays),
+      policy_status, cache_life_limit);
 }
 
 bool PolicyService::GetEffectivePolicyForAppInstalls(
     const std::string& app_id,
+    PolicyStatus<int>* policy_status,
     int* install_policy) const {
-  for (const std::unique_ptr<PolicyManagerInterface>& policy_manager :
-       policy_managers_) {
-    if (policy_manager->GetEffectivePolicyForAppInstalls(app_id,
-                                                         install_policy))
-      return true;
-  }
-  return false;
+  return QueryAppPolicy(
+      base::BindRepeating(
+          &PolicyManagerInterface::GetEffectivePolicyForAppInstalls),
+      app_id, policy_status, install_policy);
 }
 
-bool PolicyService::GetEffectivePolicyForAppUpdates(const std::string& app_id,
-                                                    int* update_policy) const {
-  for (const std::unique_ptr<PolicyManagerInterface>& policy_manager :
-       policy_managers_) {
-    if (policy_manager->GetEffectivePolicyForAppUpdates(app_id, update_policy))
-      return true;
-  }
-  return false;
+bool PolicyService::GetEffectivePolicyForAppUpdates(
+    const std::string& app_id,
+    PolicyStatus<int>* policy_status,
+    int* update_policy) const {
+  return QueryAppPolicy(
+      base::BindRepeating(
+          &PolicyManagerInterface::GetEffectivePolicyForAppUpdates),
+      app_id, policy_status, update_policy);
 }
 
 bool PolicyService::GetTargetChannel(const std::string& app_id,
+                                     PolicyStatus<std::string>* policy_status,
                                      std::string* channel) const {
-  for (const std::unique_ptr<PolicyManagerInterface>& policy_manager :
-       policy_managers_) {
-    if (policy_manager->GetTargetChannel(app_id, channel))
-      return true;
-  }
-  return false;
+  return QueryAppPolicy(
+      base::BindRepeating(&PolicyManagerInterface::GetTargetChannel), app_id,
+      policy_status, channel);
 }
 
 bool PolicyService::GetTargetVersionPrefix(
     const std::string& app_id,
+    PolicyStatus<std::string>* policy_status,
     std::string* target_version_prefix) const {
-  for (const std::unique_ptr<PolicyManagerInterface>& policy_manager :
-       policy_managers_) {
-    if (policy_manager->GetTargetVersionPrefix(app_id, target_version_prefix))
-      return true;
-  }
-  return false;
+  return QueryAppPolicy(
+      base::BindRepeating(&PolicyManagerInterface::GetTargetVersionPrefix),
+      app_id, policy_status, target_version_prefix);
 }
 
 bool PolicyService::IsRollbackToTargetVersionAllowed(
     const std::string& app_id,
+    PolicyStatus<bool>* policy_status,
     bool* rollback_allowed) const {
-  for (const std::unique_ptr<PolicyManagerInterface>& policy_manager :
-       policy_managers_) {
-    if (policy_manager->IsRollbackToTargetVersionAllowed(app_id,
-                                                         rollback_allowed))
-      return true;
-  }
-  return false;
+  return QueryAppPolicy(
+      base::BindRepeating(
+          &PolicyManagerInterface::IsRollbackToTargetVersionAllowed),
+      app_id, policy_status, rollback_allowed);
 }
 
-bool PolicyService::GetProxyMode(std::string* proxy_mode) const {
-  for (const std::unique_ptr<PolicyManagerInterface>& policy_manager :
-       policy_managers_) {
-    if (policy_manager->GetProxyMode(proxy_mode))
-      return true;
-  }
-  return false;
+bool PolicyService::GetProxyMode(PolicyStatus<std::string>* policy_status,
+                                 std::string* proxy_mode) const {
+  return QueryPolicy(base::BindRepeating(&PolicyManagerInterface::GetProxyMode),
+                     policy_status, proxy_mode);
 }
 
-bool PolicyService::GetProxyPacUrl(std::string* proxy_pac_url) const {
+bool PolicyService::GetProxyPacUrl(PolicyStatus<std::string>* policy_status,
+                                   std::string* proxy_pac_url) const {
+  return QueryPolicy(
+      base::BindRepeating(&PolicyManagerInterface::GetProxyPacUrl),
+      policy_status, proxy_pac_url);
+}
+
+bool PolicyService::GetProxyServer(PolicyStatus<std::string>* policy_status,
+                                   std::string* proxy_server) const {
+  return QueryPolicy(
+      base::BindRepeating(&PolicyManagerInterface::GetProxyServer),
+      policy_status, proxy_server);
+}
+
+template <typename T>
+bool PolicyService::QueryPolicy(
+    const base::RepeatingCallback<bool(const PolicyManagerInterface*, T*)>&
+        policy_query_callback,
+    PolicyStatus<T>* policy_status,
+    T* result) const {
+  T value{};
+  PolicyStatus<T> status;
   for (const std::unique_ptr<PolicyManagerInterface>& policy_manager :
        policy_managers_) {
-    if (policy_manager->GetProxyPacUrl(proxy_pac_url))
-      return true;
+    if (!policy_query_callback.Run(policy_manager.get(), &value))
+      continue;
+    status.AddPolicyIfNeeded(policy_manager->IsManaged(),
+                             policy_manager->source(), value);
   }
-  return false;
+  if (!status.effective_policy())
+    return false;
+
+  *result = status.effective_policy().value().policy;
+  if (policy_status)
+    *policy_status = status;
+  return true;
 }
 
-bool PolicyService::GetProxyServer(std::string* proxy_server) const {
+template <typename T>
+bool PolicyService::QueryAppPolicy(
+    const base::RepeatingCallback<bool(const PolicyManagerInterface*,
+                                       const std::string&,
+                                       T*)>& policy_query_callback,
+    const std::string& app_id,
+    PolicyStatus<T>* policy_status,
+    T* result) const {
+  T value{};
+  PolicyStatus<T> status;
   for (const std::unique_ptr<PolicyManagerInterface>& policy_manager :
        policy_managers_) {
-    if (policy_manager->GetProxyServer(proxy_server))
-      return true;
+    if (!policy_query_callback.Run(policy_manager.get(), app_id, &value))
+      continue;
+    status.AddPolicyIfNeeded(policy_manager->IsManaged(),
+                             policy_manager->source(), value);
   }
-  return false;
+  if (!status.effective_policy())
+    return false;
+
+  *result = status.effective_policy().value().policy;
+  if (policy_status)
+    *policy_status = status;
+  return true;
 }
 
 std::unique_ptr<PolicyService> GetUpdaterPolicyService() {

chrome/updater/policy_service.h

@@ -9,61 +9,129 @@
 #include <string>
 #include <vector>
 
+#include "base/callback_forward.h"
+#include "base/optional.h"
 #include "chrome/updater/policy_manager.h"
 
 namespace updater {
 
+// This class contains the aggregate status of a policy value. It determines
+// whether a conflict exists when multiple policy providers set the same policy.
+template <typename T>
+class PolicyStatus {
+ public:
+  struct Entry {
+    Entry(const std::string& s, T p) : source(s), policy(p) {}
+    std::string source;
+    T policy{};
+  };
+
+  PolicyStatus() = default;
+  PolicyStatus(const PolicyStatus&) = default;
+
+  void AddPolicyIfNeeded(bool is_managed,
+                         const std::string& source,
+                         const T& policy) {
+    if (conflict_policy_)
+      return;  // We already have enough policies.
+
+    if (!effective_policy_ && is_managed) {
+      effective_policy_ = base::make_optional<Entry>(source, policy);
+    } else if (effective_policy_ &&
+               policy != effective_policy_.value().policy) {
+      conflict_policy_ = base::make_optional<Entry>(source, policy);
+    }
+  }
+
+  const base::Optional<Entry>& effective_policy() const {
+    return effective_policy_;
+  }
+  const base::Optional<Entry>& conflict_policy() const {
+    return conflict_policy_;
+  }
+
+ private:
+  base::Optional<Entry> effective_policy_;
+  base::Optional<Entry> conflict_policy_;
+};
+
 // The PolicyService returns policies for enterprise managed machines from the
 // source with the highest priority where the policy available.
-class PolicyService : public PolicyManagerInterface {
+class PolicyService {
  public:
   PolicyService();
   PolicyService(const PolicyService&) = delete;
   PolicyService& operator=(const PolicyService&) = delete;
-  ~PolicyService() override;
-
-  // Overrides for PolicyManagerInterface.
-  std::string source() const override;
+  ~PolicyService();
 
-  bool IsManaged() const override;
+  std::string source() const;
 
-  bool GetLastCheckPeriodMinutes(int* minutes) const override;
-  bool GetUpdatesSuppressedTimes(int* start_hour,
-                                 int* start_min,
-                                 int* duration_min) const override;
+  bool GetLastCheckPeriodMinutes(PolicyStatus<int>* policy_status,
+                                 int* minutes) const;
+  bool GetUpdatesSuppressedTimes(
+      PolicyStatus<UpdatesSuppressedTimes>* policy_status,
+      UpdatesSuppressedTimes* suppressed_times) const;
   bool GetDownloadPreferenceGroupPolicy(
-      std::string* download_preference) const override;
-  bool GetPackageCacheSizeLimitMBytes(int* cache_size_limit) const override;
-  bool GetPackageCacheExpirationTimeDays(int* cache_life_limit) const override;
+      PolicyStatus<std::string>* policy_status,
+      std::string* download_preference) const;
+  bool GetPackageCacheSizeLimitMBytes(PolicyStatus<int>* policy_status,
+                                      int* cache_size_limit) const;
+  bool GetPackageCacheExpirationTimeDays(PolicyStatus<int>* policy_status,
+                                         int* cache_life_limit) const;
 
   bool GetEffectivePolicyForAppInstalls(const std::string& app_id,
-                                        int* install_policy) const override;
+                                        PolicyStatus<int>* policy_status,
+                                        int* install_policy) const;
   bool GetEffectivePolicyForAppUpdates(const std::string& app_id,
-                                       int* update_policy) const override;
+                                       PolicyStatus<int>* policy_status,
+                                       int* update_policy) const;
   bool GetTargetChannel(const std::string& app_id,
-                        std::string* channel) const override;
-  bool GetTargetVersionPrefix(
-      const std::string& app_id,
-      std::string* target_version_prefix) const override;
+                        PolicyStatus<std::string>* policy_status,
+                        std::string* channel) const;
+  bool GetTargetVersionPrefix(const std::string& app_id,
+                              PolicyStatus<std::string>* policy_status,
+                              std::string* target_version_prefix) const;
   bool IsRollbackToTargetVersionAllowed(const std::string& app_id,
-                                        bool* rollback_allowed) const override;
-  bool GetProxyMode(std::string* proxy_mode) const override;
-  bool GetProxyPacUrl(std::string* proxy_pac_url) const override;
-  bool GetProxyServer(std::string* proxy_server) const override;
-
-  const std::vector<std::unique_ptr<PolicyManagerInterface>>&
-  policy_managers() {
-    return policy_managers_;
-  }
+                                        PolicyStatus<bool>* policy_status,
+                                        bool* rollback_allowed) const;
+  bool GetProxyMode(PolicyStatus<std::string>* policy_status,
+                    std::string* proxy_mode) const;
+  bool GetProxyPacUrl(PolicyStatus<std::string>* policy_status,
+                      std::string* proxy_pac_url) const;
+  bool GetProxyServer(PolicyStatus<std::string>* policy_status,
+                      std::string* proxy_server) const;
 
   void SetPolicyManagersForTesting(
       std::vector<std::unique_ptr<PolicyManagerInterface>> managers);
-  const PolicyManagerInterface& GetActivePolicyManager();
 
  private:
-  // List of policy managers in descending order of priority. The first policy
-  // manager's policies takes precedence over the following.
+  // List of policy providers in descending order of priority. All managed
+  // providers should be ahead of non-managed providers.
   std::vector<std::unique_ptr<PolicyManagerInterface>> policy_managers_;
+
+  // Helper function to insert the policy manager and make sure that
+  // managed providers are ahead of non-managed providers.
+  void InsertPolicyManager(std::unique_ptr<PolicyManagerInterface> manager);
+
+  // Helper function to query the policy from the managed policy providers and
+  // determines the policy status.
+  template <typename T>
+  bool QueryPolicy(
+      const base::RepeatingCallback<bool(const PolicyManagerInterface*, T*)>&
+          policy_query_callback,
+      PolicyStatus<T>* policy_status,
+      T* value) const;
+
+  // Helper function to query app policy from the managed policy providers and
+  // determines the policy status.
+  template <typename T>
+  bool QueryAppPolicy(
+      const base::RepeatingCallback<bool(const PolicyManagerInterface*,
+                                         const std::string& app_id,
+                                         T*)>& policy_query_callback,
+      const std::string& app_id,
+      PolicyStatus<T>* policy_status,
+      T* value) const;
 };
 
 std::unique_ptr<PolicyService> GetUpdaterPolicyService();

chrome/updater/policy_service_unittest.cc

@@ -4,6 +4,7 @@
 
 #include <memory>
 #include <string>
+#include <utility>
 #include <vector>
 
 #include "chrome/updater/policy_manager.h"
@@ -16,16 +17,24 @@ namespace updater {
 // Policy and Device Management.
 class FakePolicyManager : public PolicyManagerInterface {
  public:
-  explicit FakePolicyManager(const std::string& source) : source_(source) {}
+  FakePolicyManager(bool is_managed, const std::string& source)
+      : is_managed_(is_managed), source_(source) {}
   ~FakePolicyManager() override = default;
 
   std::string source() const override { return source_; }
-  bool IsManaged() const override { return true; }
+  bool IsManaged() const override { return is_managed_; }
   bool GetLastCheckPeriodMinutes(int* minutes) const override { return false; }
-  bool GetUpdatesSuppressedTimes(int* start_hour,
-                                 int* start_min,
-                                 int* duration_min) const override {
-    return false;
+  bool GetUpdatesSuppressedTimes(
+      UpdatesSuppressedTimes* suppressed_times) const override {
+    if (!suppressed_times_.valid())
+      return false;
+
+    *suppressed_times = suppressed_times_;
+    return true;
+  }
+  void SetUpdatesSuppressedTimes(
+      const UpdatesSuppressedTimes& suppressed_times) {
+    suppressed_times_ = suppressed_times;
   }
   bool GetDownloadPreferenceGroupPolicy(
       std::string* download_preference) const override {
@@ -88,7 +97,9 @@ class FakePolicyManager : public PolicyManagerInterface {
   }
 
  private:
+  bool is_managed_;
   std::string source_;
+  UpdatesSuppressedTimes suppressed_times_;
   std::string download_preference_;
   std::map<std::string, std::string> channels_;
   std::map<std::string, int> update_policies_;
@@ -99,17 +110,18 @@ TEST(PolicyService, DefaultPolicyValue) {
   std::vector<std::unique_ptr<PolicyManagerInterface>> managers;
   managers.push_back(GetPolicyManager());
   policy_service->SetPolicyManagersForTesting(std::move(managers));
-  EXPECT_EQ(policy_service->source(), "");
+  EXPECT_EQ(policy_service->source(), "default");
 
   std::string version_prefix;
-  EXPECT_FALSE(policy_service->GetTargetVersionPrefix("", &version_prefix));
+  EXPECT_FALSE(
+      policy_service->GetTargetVersionPrefix("", nullptr, &version_prefix));
   int last_check = 0;
-  EXPECT_FALSE(policy_service->GetLastCheckPeriodMinutes(&last_check));
+  EXPECT_FALSE(policy_service->GetLastCheckPeriodMinutes(nullptr, &last_check));
 }
 
 TEST(PolicyService, SinglePolicyManager) {
   std::unique_ptr<PolicyService> policy_service(GetUpdaterPolicyService());
-  auto manager = std::make_unique<FakePolicyManager>("test_source");
+  auto manager = std::make_unique<FakePolicyManager>(true, "test_source");
   manager->SetChannel("app1", "test_channel");
   manager->SetUpdatePolicy("app2", 3);
   std::vector<std::unique_ptr<PolicyManagerInterface>> managers;
@@ -117,16 +129,35 @@ TEST(PolicyService, SinglePolicyManager) {
   policy_service->SetPolicyManagersForTesting(std::move(managers));
   EXPECT_EQ(policy_service->source(), "test_source");
 
-  std::string channel;
-  EXPECT_FALSE(policy_service->GetTargetChannel("app2", &channel));
-  EXPECT_TRUE(policy_service->GetTargetChannel("app1", &channel));
-  EXPECT_EQ(channel, "test_channel");
+  PolicyStatus<std::string> app1_channel_status;
+  std::string app1_channel;
+  EXPECT_TRUE(policy_service->GetTargetChannel("app1", &app1_channel_status,
+                                               &app1_channel));
+  EXPECT_TRUE(app1_channel_status.effective_policy());
+  EXPECT_EQ(app1_channel_status.effective_policy().value().policy,
+            "test_channel");
+  EXPECT_EQ(app1_channel, "test_channel");
+  EXPECT_FALSE(app1_channel_status.conflict_policy());
 
+  PolicyStatus<std::string> app2_channel_status;
+  std::string app2_channel;
+  EXPECT_FALSE(policy_service->GetTargetChannel("app2", &app2_channel_status,
+                                                &app2_channel));
+  EXPECT_FALSE(app2_channel_status.effective_policy());
+  EXPECT_FALSE(app2_channel_status.conflict_policy());
+
+  PolicyStatus<int> app1_update_status;
   int update_policy = 0;
-  EXPECT_FALSE(
-      policy_service->GetEffectivePolicyForAppUpdates("app1", &update_policy));
-  EXPECT_TRUE(
-      policy_service->GetEffectivePolicyForAppUpdates("app2", &update_policy));
+  EXPECT_FALSE(policy_service->GetEffectivePolicyForAppUpdates(
+      "app1", &app1_update_status, &update_policy));
+  EXPECT_FALSE(app1_update_status.conflict_policy());
+
+  PolicyStatus<int> app2_update_status;
+  EXPECT_TRUE(policy_service->GetEffectivePolicyForAppUpdates(
+      "app2", &app2_update_status, &update_policy));
+  EXPECT_TRUE(app2_update_status.effective_policy());
+  EXPECT_EQ(app2_update_status.effective_policy().value().policy, 3);
+  EXPECT_FALSE(app2_update_status.conflict_policy());
   EXPECT_EQ(update_policy, 3);
 }
 
@@ -134,17 +165,22 @@ TEST(PolicyService, MultiplePolicyManagers) {
   std::unique_ptr<PolicyService> policy_service(GetUpdaterPolicyService());
   std::vector<std::unique_ptr<PolicyManagerInterface>> managers;
 
-  auto manager = std::make_unique<FakePolicyManager>("group_policy");
+  auto manager = std::make_unique<FakePolicyManager>(true, "group_policy");
+  UpdatesSuppressedTimes updates_suppressed_times = {5, 10, 30};
+  manager->SetUpdatesSuppressedTimes(updates_suppressed_times);
   manager->SetChannel("app1", "channel_gp");
   manager->SetUpdatePolicy("app2", 1);
   managers.push_back(std::move(manager));
 
-  manager = std::make_unique<FakePolicyManager>("device_management");
+  manager = std::make_unique<FakePolicyManager>(true, "device_management");
+  manager->SetUpdatesSuppressedTimes(updates_suppressed_times);
   manager->SetChannel("app1", "channel_dm");
   manager->SetUpdatePolicy("app1", 3);
   managers.push_back(std::move(manager));
 
-  manager = std::make_unique<FakePolicyManager>("imaginary");
+  manager = std::make_unique<FakePolicyManager>(true, "imaginary");
+  updates_suppressed_times = {1, 1, 20};
+  manager->SetUpdatesSuppressedTimes(updates_suppressed_times);
   manager->SetChannel("app1", "channel_imaginary");
   manager->SetUpdatePolicy("app1", 2);
   manager->SetDownloadPreferenceGroupPolicy("cacheable");
@@ -155,28 +191,170 @@ TEST(PolicyService, MultiplePolicyManagers) {
 
   policy_service->SetPolicyManagersForTesting(std::move(managers));
   EXPECT_EQ(policy_service->source(),
-            "group_policy;device_management;imaginary");
+            "group_policy;device_management;imaginary;default");
+
+  PolicyStatus<UpdatesSuppressedTimes> suppressed_time_status;
+  EXPECT_TRUE(policy_service->GetUpdatesSuppressedTimes(
+      &suppressed_time_status, &updates_suppressed_times));
+  EXPECT_TRUE(suppressed_time_status.conflict_policy());
+  EXPECT_EQ(suppressed_time_status.effective_policy().value().source,
+            "group_policy");
+  EXPECT_EQ(updates_suppressed_times.start_hour, 5);
+  EXPECT_EQ(updates_suppressed_times.start_minute, 10);
+  EXPECT_EQ(updates_suppressed_times.duration_minute, 30);
 
+  PolicyStatus<std::string> channel_status;
   std::string channel;
-  EXPECT_TRUE(policy_service->GetTargetChannel("app1", &channel));
+  EXPECT_TRUE(
+      policy_service->GetTargetChannel("app1", &channel_status, &channel));
+  const PolicyStatus<std::string>::Entry& channel_policy =
+      channel_status.effective_policy().value();
+  EXPECT_EQ(channel_policy.source, "group_policy");
+  EXPECT_EQ(channel_policy.policy, "channel_gp");
+  EXPECT_TRUE(channel_status.conflict_policy());
+  const PolicyStatus<std::string>::Entry& channel_conflict_policy =
+      channel_status.conflict_policy().value();
+  EXPECT_EQ(channel_conflict_policy.source, "device_management");
+  EXPECT_EQ(channel_conflict_policy.policy, "channel_dm");
   EXPECT_EQ(channel, "channel_gp");
 
+  PolicyStatus<int> app1_update_status;
   int update_policy = 0;
-  EXPECT_TRUE(
-      policy_service->GetEffectivePolicyForAppUpdates("app1", &update_policy));
+  EXPECT_TRUE(policy_service->GetEffectivePolicyForAppUpdates(
+      "app1", &app1_update_status, &update_policy));
+  EXPECT_TRUE(app1_update_status.effective_policy());
+  const PolicyStatus<int>::Entry& app1_update_policy =
+      app1_update_status.effective_policy().value();
+  EXPECT_EQ(app1_update_policy.source, "device_management");
+  EXPECT_EQ(app1_update_policy.policy, 3);
+  EXPECT_TRUE(app1_update_status.conflict_policy());
+  const PolicyStatus<int>::Entry& app1_update_conflict_policy =
+      app1_update_status.conflict_policy().value();
+  EXPECT_TRUE(app1_update_status.conflict_policy());
+  EXPECT_EQ(app1_update_conflict_policy.policy, 2);
+  EXPECT_EQ(app1_update_conflict_policy.source, "imaginary");
   EXPECT_EQ(update_policy, 3);
-  EXPECT_TRUE(
-      policy_service->GetEffectivePolicyForAppUpdates("app2", &update_policy));
+
+  PolicyStatus<int> app2_update_status;
+  EXPECT_TRUE(policy_service->GetEffectivePolicyForAppUpdates(
+      "app2", &app2_update_status, &update_policy));
+  EXPECT_TRUE(app2_update_status.effective_policy());
+  const PolicyStatus<int>::Entry& app2_update_policy =
+      app2_update_status.effective_policy().value();
+  EXPECT_EQ(app2_update_policy.source, "group_policy");
+  EXPECT_EQ(app2_update_policy.policy, 1);
   EXPECT_EQ(update_policy, 1);
+  EXPECT_FALSE(app2_update_status.conflict_policy());
 
+  PolicyStatus<std::string> download_preference_status;
   std::string download_preference;
+  EXPECT_TRUE(policy_service->GetDownloadPreferenceGroupPolicy(
+      &download_preference_status, &download_preference));
+  EXPECT_TRUE(download_preference_status.effective_policy());
+  const PolicyStatus<std::string>::Entry& download_preference_policy =
+      download_preference_status.effective_policy().value();
+  EXPECT_EQ(download_preference_policy.source, "imaginary");
+  EXPECT_EQ(download_preference_policy.policy, "cacheable");
+  EXPECT_EQ(download_preference, "cacheable");
+  EXPECT_FALSE(download_preference_status.conflict_policy());
+
+  int cache_size_limit = 0;
+  EXPECT_FALSE(policy_service->GetPackageCacheSizeLimitMBytes(
+      nullptr, &cache_size_limit));
+}
+
+TEST(PolicyService, MultiplePolicyManagers_WithUnmanagedOnes) {
+  std::unique_ptr<PolicyService> policy_service(GetUpdaterPolicyService());
+  std::vector<std::unique_ptr<PolicyManagerInterface>> managers;
+
+  auto manager = std::make_unique<FakePolicyManager>(true, "device_management");
+  UpdatesSuppressedTimes updates_suppressed_times = {5, 10, 30};
+  manager->SetUpdatesSuppressedTimes(updates_suppressed_times);
+  manager->SetChannel("app1", "channel_dm");
+  manager->SetUpdatePolicy("app1", 3);
+  managers.push_back(std::move(manager));
+
+  manager = std::make_unique<FakePolicyManager>(true, "imaginary");
+  updates_suppressed_times = {1, 1, 20};
+  manager->SetUpdatesSuppressedTimes(updates_suppressed_times);
+  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());
+
+  manager = std::make_unique<FakePolicyManager>(false, "group_policy");
+  updates_suppressed_times = {5, 10, 30};
+  manager->SetUpdatesSuppressedTimes(updates_suppressed_times);
+  manager->SetChannel("app1", "channel_gp");
+  manager->SetUpdatePolicy("app2", 1);
+  managers.push_back(std::move(manager));
+
+  policy_service->SetPolicyManagersForTesting(std::move(managers));
+  EXPECT_EQ(policy_service->source(), "device_management;imaginary;default");
+
+  PolicyStatus<UpdatesSuppressedTimes> suppressed_time_status;
+  EXPECT_TRUE(policy_service->GetUpdatesSuppressedTimes(
+      &suppressed_time_status, &updates_suppressed_times));
+  EXPECT_TRUE(suppressed_time_status.conflict_policy());
+  EXPECT_EQ(suppressed_time_status.effective_policy().value().source,
+            "device_management");
+  EXPECT_EQ(updates_suppressed_times.start_hour, 5);
+  EXPECT_EQ(updates_suppressed_times.start_minute, 10);
+  EXPECT_EQ(updates_suppressed_times.duration_minute, 30);
+
+  PolicyStatus<std::string> channel_status;
+  std::string channel;
   EXPECT_TRUE(
-      policy_service->GetDownloadPreferenceGroupPolicy(&download_preference));
+      policy_service->GetTargetChannel("app1", &channel_status, &channel));
+  EXPECT_TRUE(channel_status.effective_policy());
+  const PolicyStatus<std::string>::Entry& channel_status_policy =
+      channel_status.effective_policy().value();
+  EXPECT_EQ(channel_status_policy.source, "device_management");
+  EXPECT_EQ(channel_status_policy.policy, "channel_dm");
+  EXPECT_TRUE(channel_status.conflict_policy());
+  const PolicyStatus<std::string>::Entry& channel_status_conflict_policy =
+      channel_status.conflict_policy().value();
+  EXPECT_EQ(channel_status_conflict_policy.policy, "channel_imaginary");
+  EXPECT_EQ(channel_status_conflict_policy.source, "imaginary");
+  EXPECT_EQ(channel, "channel_dm");
+
+  PolicyStatus<int> app1_update_status;
+  int update_policy = 0;
+  EXPECT_TRUE(policy_service->GetEffectivePolicyForAppUpdates(
+      "app1", &app1_update_status, &update_policy));
+  const PolicyStatus<int>::Entry& app1_update_status_policy =
+      app1_update_status.effective_policy().value();
+  EXPECT_EQ(app1_update_status_policy.source, "device_management");
+  EXPECT_EQ(app1_update_status_policy.policy, 3);
+  EXPECT_TRUE(app1_update_status.conflict_policy());
+  const PolicyStatus<int>::Entry& app1_update_status_conflict_policy =
+      app1_update_status.conflict_policy().value();
+  EXPECT_EQ(app1_update_status_conflict_policy.source, "imaginary");
+  EXPECT_EQ(app1_update_status_conflict_policy.policy, 2);
+  EXPECT_EQ(update_policy, 3);
+
+  PolicyStatus<int> app2_update_status;
+  EXPECT_FALSE(policy_service->GetEffectivePolicyForAppUpdates(
+      "app2", &app2_update_status, &update_policy));
+  EXPECT_FALSE(app2_update_status.effective_policy());
+  EXPECT_FALSE(app2_update_status.conflict_policy());
+
+  PolicyStatus<std::string> download_preference_status;
+  std::string download_preference;
+  EXPECT_TRUE(policy_service->GetDownloadPreferenceGroupPolicy(
+      &download_preference_status, &download_preference));
+  EXPECT_TRUE(download_preference_status.effective_policy());
+  EXPECT_EQ(download_preference_status.effective_policy().value().source,
+            "imaginary");
   EXPECT_EQ(download_preference, "cacheable");
+  EXPECT_FALSE(download_preference_status.conflict_policy());
 
   int cache_size_limit = 0;
-  EXPECT_FALSE(
-      policy_service->GetPackageCacheSizeLimitMBytes(&cache_size_limit));
+  EXPECT_FALSE(policy_service->GetPackageCacheSizeLimitMBytes(
+      nullptr, &cache_size_limit));
 }
 
 }  // namespace updater

chrome/updater/win/group_policy_manager.cc

@@ -7,6 +7,7 @@
 #include "base/strings/string16.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/win/win_util.h"
+#include "chrome/updater/policy_manager.h"
 #include "chrome/updater/win/constants.h"
 
 namespace updater {
@@ -70,12 +71,14 @@ bool GroupPolicyManager::GetLastCheckPeriodMinutes(int* minutes) const {
   return ReadValueDW(kRegValueAutoUpdateCheckPeriodOverrideMinutes, minutes);
 }
 
-bool GroupPolicyManager::GetUpdatesSuppressedTimes(int* start_hour,
-                                                   int* start_min,
-                                                   int* duration_min) const {
-  return ReadValueDW(kRegValueUpdatesSuppressedStartHour, start_hour) &&
-         ReadValueDW(kRegValueUpdatesSuppressedStartMin, start_min) &&
-         ReadValueDW(kRegValueUpdatesSuppressedDurationMin, duration_min);
+bool GroupPolicyManager::GetUpdatesSuppressedTimes(
+    UpdatesSuppressedTimes* suppressed_times) const {
+  return ReadValueDW(kRegValueUpdatesSuppressedStartHour,
+                     &suppressed_times->start_hour) &&
+         ReadValueDW(kRegValueUpdatesSuppressedStartMin,
+                     &suppressed_times->start_minute) &&
+         ReadValueDW(kRegValueUpdatesSuppressedDurationMin,
+                     &suppressed_times->duration_minute);
 }
 
 bool GroupPolicyManager::GetDownloadPreferenceGroupPolicy(

chrome/updater/win/group_policy_manager.h

@@ -28,9 +28,8 @@ class GroupPolicyManager : public PolicyManagerInterface {
   bool IsManaged() const override;
 
   bool GetLastCheckPeriodMinutes(int* minutes) const override;
-  bool GetUpdatesSuppressedTimes(int* start_hour,
-                                 int* start_min,
-                                 int* duration_min) const override;
+  bool GetUpdatesSuppressedTimes(
+      UpdatesSuppressedTimes* suppressed_times) const override;
   bool GetDownloadPreferenceGroupPolicy(
       std::string* download_preference) const override;
   bool GetPackageCacheSizeLimitMBytes(int* cache_size_limit) const override;