Adding unit tests for MinimumVersionPolicyHandler

Adding delegate in the policy handler to take care of the external dependencies.

This CL adds 2 unit tests for MinimumVersionPolicyHandler. The test RequirementsNotMetState verifies that when policy contains a requirement with version greater than current version, then the state in the policy handler is set to the strongest requirement among those. The strongest requirement is defined in the policy description. The test RequirementsMetState verifies that if the current version satisfies all the requirements in the policy, then there is no change in the state in the policy handler. The keys in the policy schema have been moved to the header file so that they can be reused in the unit test without duplication.

Bug: 1048607
Change-Id: I59625dd1c16e7097b814cb85f93de6ee2a3f87c8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2044116Reviewed-by: Denis Kuznetsov [CET] <antrim@chromium.org>
Commit-Queue: Saurabh Nijhara <snijhara@google.com>
Cr-Commit-Position: refs/heads/master@{#744722}

chrome/browser/chromeos/BUILD.gn

@@ -1867,6 +1867,8 @@ source_set("chromeos") {
     "policy/login_profile_policy_provider.h",
     "policy/minimum_version_policy_handler.cc",
     "policy/minimum_version_policy_handler.h",
+    "policy/minimum_version_policy_handler_delegate_impl.cc",
+    "policy/minimum_version_policy_handler_delegate_impl.h",
     "policy/network_configuration_updater.cc",
     "policy/network_configuration_updater.h",
     "policy/off_hours/device_off_hours_controller.cc",
@@ -2963,6 +2965,7 @@ source_set("unit_tests") {
     "policy/heartbeat_scheduler_unittest.cc",
     "policy/hostname_handler_unittest.cc",
     "policy/lock_to_single_user_manager_unittest.cc",
+    "policy/minimum_version_policy_handler_unittest.cc",
     "policy/network_configuration_updater_unittest.cc",
     "policy/off_hours/device_off_hours_controller_unittest.cc",
     "policy/off_hours/off_hours_policy_applier_unittest.cc",

chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc

@@ -40,6 +40,7 @@
 #include "chrome/browser/chromeos/policy/external_data_handlers/device_wilco_dtc_configuration_external_data_handler.h"
 #include "chrome/browser/chromeos/policy/hostname_handler.h"
 #include "chrome/browser/chromeos/policy/minimum_version_policy_handler.h"
+#include "chrome/browser/chromeos/policy/minimum_version_policy_handler_delegate_impl.h"
 #include "chrome/browser/chromeos/policy/remote_commands/affiliated_remote_commands_invalidator.h"
 #include "chrome/browser/chromeos/policy/scheduled_update_checker/device_scheduled_update_checker.h"
 #include "chrome/browser/chromeos/policy/server_backed_state_keys_broker.h"
@@ -230,8 +231,12 @@ void BrowserPolicyConnectorChromeOS::Init(
   hostname_handler_ =
       std::make_unique<HostnameHandler>(chromeos::CrosSettings::Get());
 
+  minimum_version_policy_handler_delegate_ =
+      std::make_unique<MinimumVersionPolicyHandlerDelegateImpl>();
+
   minimum_version_policy_handler_ =
       std::make_unique<MinimumVersionPolicyHandler>(
+          minimum_version_policy_handler_delegate_.get(),
           chromeos::CrosSettings::Get());
 
   device_dock_mac_address_source_handler_ =

chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h

@@ -50,6 +50,7 @@ class DeviceWiFiAllowedHandler;
 struct EnrollmentConfig;
 class HostnameHandler;
 class MinimumVersionPolicyHandler;
+class MinimumVersionPolicyHandlerDelegateImpl;
 class ProxyPolicyProvider;
 class ServerBackedStateKeysBroker;
 class TPMAutoUpdateModePolicyHandler;
@@ -250,6 +251,8 @@ class BrowserPolicyConnectorChromeOS
   std::unique_ptr<BluetoothPolicyHandler> bluetooth_policy_handler_;
   std::unique_ptr<HostnameHandler> hostname_handler_;
   std::unique_ptr<MinimumVersionPolicyHandler> minimum_version_policy_handler_;
+  std::unique_ptr<MinimumVersionPolicyHandlerDelegateImpl>
+      minimum_version_policy_handler_delegate_;
   std::unique_ptr<DeviceDockMacAddressHandler>
       device_dock_mac_address_source_handler_;
   std::unique_ptr<DeviceWiFiAllowedHandler> device_wifi_allowed_handler_;

chrome/browser/chromeos/policy/minimum_version_policy_handler.cc

@@ -6,26 +6,20 @@
 
 #include "base/bind.h"
 #include "base/memory/ref_counted.h"
-#include "chrome/browser/browser_process.h"
-#include "chrome/browser/browser_process_platform_part.h"
-#include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
+#include "base/values.h"
+#include "chrome/browser/chromeos/policy/minimum_version_policy_handler_delegate_impl.h"
 #include "chromeos/settings/cros_settings_names.h"
 #include "chromeos/settings/cros_settings_provider.h"
-#include "components/user_manager/user_manager.h"
-#include "components/version_info/version_info.h"
 
 using MinimumVersionRequirement =
     policy::MinimumVersionPolicyHandler::MinimumVersionRequirement;
 
 namespace policy {
 
-namespace {
-
-constexpr char kChromeVersion[] = "chrome_version";
-constexpr char kWarningPeriod[] = "warning_period";
-constexpr char KEolWarningPeriod[] = "eol_warning_period";
-
-}  // namespace
+const char MinimumVersionPolicyHandler::kChromeVersion[] = "chrome_version";
+const char MinimumVersionPolicyHandler::kWarningPeriod[] = "warning_period";
+const char MinimumVersionPolicyHandler::KEolWarningPeriod[] =
+    "eol_warning_period";
 
 MinimumVersionRequirement::MinimumVersionRequirement(
     const base::Version version,
@@ -67,8 +61,9 @@ int MinimumVersionRequirement::Compare(
 }
 
 MinimumVersionPolicyHandler::MinimumVersionPolicyHandler(
+    Delegate* delegate,
     chromeos::CrosSettings* cros_settings)
-    : cros_settings_(cros_settings) {
+    : delegate_(delegate), cros_settings_(cros_settings) {
   policy_subscription_ = cros_settings_->AddSettingsObserver(
       chromeos::kMinimumChromeVersionEnforced,
       base::Bind(&MinimumVersionPolicyHandler::OnPolicyChanged,
@@ -89,7 +84,7 @@ void MinimumVersionPolicyHandler::RemoveObserver(Observer* observer) {
 
 bool MinimumVersionPolicyHandler::CurrentVersionSatisfies(
     const MinimumVersionRequirement& requirement) const {
-  return version_info::GetVersion().CompareTo(requirement.version()) >= 0;
+  return delegate_->GetCurrentVersion().CompareTo(requirement.version()) >= 0;
 }
 
 void MinimumVersionPolicyHandler::NotifyMinimumVersionStateChanged() {
@@ -98,11 +93,8 @@ void MinimumVersionPolicyHandler::NotifyMinimumVersionStateChanged() {
 }
 
 bool MinimumVersionPolicyHandler::IsPolicyApplicable() {
-  bool device_managed = g_browser_process->platform_part()
-                            ->browser_policy_connector_chromeos()
-                            ->IsEnterpriseManaged();
-  bool is_kiosk = user_manager::UserManager::IsInitialized() &&
-                  user_manager::UserManager::Get()->IsLoggedInAsAnyKioskApp();
+  bool device_managed = delegate_->IsEnterpriseManaged();
+  bool is_kiosk = delegate_->IsKioskMode();
   return device_managed && !is_kiosk;
 }
 
@@ -158,7 +150,8 @@ void MinimumVersionPolicyHandler::OnPolicyChanged() {
       requirements_met_ = false;
       NotifyMinimumVersionStateChanged();
     }
-  } else {
+  } else if (state_) {
+    // Reset the state if the policy is already applied.
     Reset();
     NotifyMinimumVersionStateChanged();
   }

chrome/browser/chromeos/policy/minimum_version_policy_handler.h

@@ -24,12 +24,31 @@ namespace policy {
 // checks if respective requirement is met.
 class MinimumVersionPolicyHandler {
  public:
+  static const char kChromeVersion[];
+  static const char kWarningPeriod[];
+  static const char KEolWarningPeriod[];
+
   class Observer {
    public:
     virtual void OnMinimumVersionStateChanged() = 0;
     virtual ~Observer() = default;
   };
 
+  // Delegate of MinimumVersionPolicyHandler to handle the external
+  // dependencies.
+  class Delegate {
+   public:
+    virtual ~Delegate() {}
+
+    // Check if the user is logged in as any kiosk app.
+    virtual bool IsKioskMode() const = 0;
+
+    // Check if the device is enterprise managed.
+    virtual bool IsEnterpriseManaged() const = 0;
+
+    virtual const base::Version& GetCurrentVersion() const = 0;
+  };
+
   class MinimumVersionRequirement {
    public:
     MinimumVersionRequirement(const base::Version version,
@@ -60,7 +79,8 @@ class MinimumVersionPolicyHandler {
     base::TimeDelta eol_warning_time_;
   };
 
-  explicit MinimumVersionPolicyHandler(chromeos::CrosSettings* cros_settings);
+  explicit MinimumVersionPolicyHandler(Delegate* delegate,
+                                       chromeos::CrosSettings* cros_settings);
   ~MinimumVersionPolicyHandler();
 
   void AddObserver(Observer* observer);
@@ -82,6 +102,11 @@ class MinimumVersionPolicyHandler {
 
   void Reset();
 
+  // This delegate instance is owned by the owner of
+  // MinimumVersionPolicyHandler. The owner is responsible to make sure that the
+  // delegate lives throughout the life of the policy handler.
+  Delegate* delegate_;
+
   // This represents the current minimum version requirement.
   // It is chosen as one of the configurations specified in the policy. It is
   // set to nullptr if the current version is higher than the minimum required

chrome/browser/chromeos/policy/minimum_version_policy_handler_delegate_impl.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/chromeos/policy/minimum_version_policy_handler_delegate_impl.h"
+
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/browser_process_platform_part.h"
+#include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
+#include "components/user_manager/user_manager.h"
+#include "components/version_info/version_info.h"
+
+namespace policy {
+
+MinimumVersionPolicyHandlerDelegateImpl::
+    MinimumVersionPolicyHandlerDelegateImpl() {}
+
+bool MinimumVersionPolicyHandlerDelegateImpl::IsKioskMode() const {
+  return user_manager::UserManager::IsInitialized() &&
+         user_manager::UserManager::Get()->IsLoggedInAsAnyKioskApp();
+}
+
+bool MinimumVersionPolicyHandlerDelegateImpl::IsEnterpriseManaged() const {
+  return g_browser_process->platform_part()
+      ->browser_policy_connector_chromeos()
+      ->IsEnterpriseManaged();
+}
+
+const base::Version&
+MinimumVersionPolicyHandlerDelegateImpl::GetCurrentVersion() const {
+  return version_info::GetVersion();
+}
+
+}  // namespace policy

chrome/browser/chromeos/policy/minimum_version_policy_handler_delegate_impl.h

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_CHROMEOS_POLICY_MINIMUM_VERSION_POLICY_HANDLER_DELEGATE_IMPL_H_
+#define CHROME_BROWSER_CHROMEOS_POLICY_MINIMUM_VERSION_POLICY_HANDLER_DELEGATE_IMPL_H_
+
+#include "base/version.h"
+#include "chrome/browser/chromeos/policy/minimum_version_policy_handler.h"
+
+namespace policy {
+
+class MinimumVersionPolicyHandlerDelegateImpl
+    : public MinimumVersionPolicyHandler::Delegate {
+ public:
+  MinimumVersionPolicyHandlerDelegateImpl();
+  bool IsKioskMode() const override;
+  bool IsEnterpriseManaged() const override;
+  const base::Version& GetCurrentVersion() const override;
+};
+
+}  // namespace policy
+
+#endif  // CHROME_BROWSER_CHROMEOS_POLICY_MINIMUM_VERSION_POLICY_HANDLER_DELEGATE_IMPL_H_

chrome/browser/chromeos/policy/minimum_version_policy_handler_unittest.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/chromeos/policy/minimum_version_policy_handler.h"
+
+#include "base/values.h"
+#include "chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h"
+#include "chrome/browser/chromeos/settings/scoped_testing_cros_settings.h"
+#include "chromeos/settings/cros_settings_names.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using testing::_;
+using testing::Mock;
+
+using MinimumVersionRequirement =
+    policy::MinimumVersionPolicyHandler::MinimumVersionRequirement;
+
+namespace policy {
+
+namespace {
+const char kFakeCurrentVersion[] = "80.25.4";
+const char kNewVersion[] = "81.4.2";
+const char kNewerVersion[] = "81.5.4";
+const char kNewestVersion[] = "82";
+const char kOldVersion[] = "78.1.5";
+
+const int kLongWarning = 10;
+const int kShortWarning = 2;
+const int kNoWarning = 0;
+
+}  // namespace
+
+class MinimumVersionPolicyHandlerTest
+    : public testing::Test,
+      public MinimumVersionPolicyHandler::Observer,
+      public MinimumVersionPolicyHandler::Delegate {
+ public:
+  MinimumVersionPolicyHandlerTest();
+
+  void SetUp() override;
+  void TearDown() override;
+
+  // MinimumVersionPolicyHandler::Observer
+  MOCK_METHOD0(OnMinimumVersionStateChanged, void());
+
+  // MinimumVersionPolicyHandler::Delegate:
+  bool IsKioskMode() const;
+  bool IsEnterpriseManaged() const;
+  const base::Version& GetCurrentVersion() const;
+
+  void SetCurrentVersionString(std::string version);
+
+  void CreateMinimumVersionHandler();
+  const MinimumVersionPolicyHandler::MinimumVersionRequirement* GetState()
+      const;
+
+  // Set new value for policy pref.
+  void SetPolicyPref(base::Value value);
+
+  // Create a new requirement as a dictionary to be used in the policy value.
+  base::Value CreateRequirement(const std::string& version,
+                                const int warning,
+                                const int eol_warning) const;
+
+  MinimumVersionPolicyHandler* GetMinimumVersionPolicyHandler() {
+    return minimum_version_policy_handler_.get();
+  }
+
+ private:
+  chromeos::ScopedTestingCrosSettings scoped_testing_cros_settings_;
+
+  std::unique_ptr<base::Version> current_version_;
+
+  std::unique_ptr<MinimumVersionPolicyHandler> minimum_version_policy_handler_;
+};
+
+MinimumVersionPolicyHandlerTest::MinimumVersionPolicyHandlerTest() {}
+
+void MinimumVersionPolicyHandlerTest::SetUp() {
+  CreateMinimumVersionHandler();
+  SetCurrentVersionString(kFakeCurrentVersion);
+  minimum_version_policy_handler_->AddObserver(this);
+}
+
+void MinimumVersionPolicyHandlerTest::TearDown() {}
+
+void MinimumVersionPolicyHandlerTest::CreateMinimumVersionHandler() {
+  minimum_version_policy_handler_.reset(
+      new MinimumVersionPolicyHandler(this, chromeos::CrosSettings::Get()));
+}
+
+const MinimumVersionRequirement* MinimumVersionPolicyHandlerTest::GetState()
+    const {
+  return minimum_version_policy_handler_->GetState();
+}
+
+void MinimumVersionPolicyHandlerTest::SetCurrentVersionString(
+    std::string version) {
+  current_version_.reset(new base::Version(kFakeCurrentVersion));
+  ASSERT_TRUE(current_version_->IsValid());
+}
+
+bool MinimumVersionPolicyHandlerTest::IsKioskMode() const {
+  return false;
+}
+
+bool MinimumVersionPolicyHandlerTest::IsEnterpriseManaged() const {
+  return true;
+}
+
+const base::Version& MinimumVersionPolicyHandlerTest::GetCurrentVersion()
+    const {
+  return *current_version_;
+}
+
+void MinimumVersionPolicyHandlerTest::SetPolicyPref(base::Value value) {
+  scoped_testing_cros_settings_.device_settings()->Set(
+      chromeos::kMinimumChromeVersionEnforced, value);
+}
+
+/**
+ *  Create a dictionary value to represent minimum version requirement.
+ *  @param version The minimum required version in string form.
+ *  @param warning The warning period in days.
+ *  @param eol_warning The end of life warning period in days.
+ */
+base::Value MinimumVersionPolicyHandlerTest::CreateRequirement(
+    const std::string& version,
+    const int warning,
+    const int eol_warning) const {
+  base::Value dict(base::Value::Type::DICTIONARY);
+  dict.SetStringKey(MinimumVersionPolicyHandler::kChromeVersion, version);
+  dict.SetIntKey(MinimumVersionPolicyHandler::kWarningPeriod, warning);
+  dict.SetIntKey(MinimumVersionPolicyHandler::KEolWarningPeriod, eol_warning);
+  return dict;
+}
+
+TEST_F(MinimumVersionPolicyHandlerTest, RequirementsNotMetState) {
+  // No policy applied yet. Check requirements are satisfied.
+  EXPECT_TRUE(GetMinimumVersionPolicyHandler()->RequirementsAreSatisfied());
+  EXPECT_FALSE(GetState());
+  EXPECT_CALL(*this, OnMinimumVersionStateChanged()).Times(2);
+
+  // Create policy value as a list of requirements.
+  base::Value requirement_list(base::Value::Type::LIST);
+
+  base::Value new_version_short_warning =
+      CreateRequirement(kNewVersion, kShortWarning, kNoWarning);
+  auto strongest_requirement = MinimumVersionRequirement::CreateInstanceIfValid(
+      &base::Value::AsDictionaryValue(new_version_short_warning));
+  base::Value newer_version_long_warning =
+      CreateRequirement(kNewerVersion, kLongWarning, kNoWarning);
+  base::Value newest_version_no_warning =
+      CreateRequirement(kNewestVersion, kNoWarning, kNoWarning);
+
+  requirement_list.Append(std::move(new_version_short_warning));
+  requirement_list.Append(std::move(newer_version_long_warning));
+  requirement_list.Append(std::move(newest_version_no_warning));
+
+  // Set new value for pref and check that requirements are not satisfied.
+  // The state in |MinimumVersionPolicyHandler| should be equal to the strongest
+  // requirement as defined in the policy description.
+  SetPolicyPref(std::move(requirement_list));
+
+  EXPECT_FALSE(GetMinimumVersionPolicyHandler()->RequirementsAreSatisfied());
+  EXPECT_TRUE(GetState());
+  EXPECT_TRUE(strongest_requirement);
+  EXPECT_EQ(GetState()->Compare(strongest_requirement.get()), 0);
+
+  // Reset the pref to empty list and verify state is reset.
+  base::Value requirement_list2(base::Value::Type::LIST);
+  SetPolicyPref(std::move(requirement_list2));
+  EXPECT_TRUE(GetMinimumVersionPolicyHandler()->RequirementsAreSatisfied());
+  EXPECT_FALSE(GetState());
+}
+
+TEST_F(MinimumVersionPolicyHandlerTest, RequirementsMetState) {
+  // No policy applied yet. Check requirements are satisfied.
+  EXPECT_TRUE(GetMinimumVersionPolicyHandler()->RequirementsAreSatisfied());
+  EXPECT_FALSE(GetState());
+  EXPECT_CALL(*this, OnMinimumVersionStateChanged()).Times(0);
+
+  // Create policy value as a list of requirements.
+  base::Value requirement_list(base::Value::Type::LIST);
+
+  base::Value current_version_no_warning =
+      CreateRequirement(kFakeCurrentVersion, kNoWarning, kNoWarning);
+  base::Value old_version_long_warning =
+      CreateRequirement(kOldVersion, kLongWarning, kNoWarning);
+
+  requirement_list.Append(std::move(current_version_no_warning));
+  requirement_list.Append(std::move(old_version_long_warning));
+
+  // Set new value for pref and check that requirements are still satisfied
+  // as none of the requirements has version greater than current version.
+  SetPolicyPref(std::move(requirement_list));
+
+  EXPECT_TRUE(GetMinimumVersionPolicyHandler()->RequirementsAreSatisfied());
+  EXPECT_FALSE(GetState());
+}
+
+}  // namespace policy