Fix KeyRotationDeviceCloudPolicyTest.Basic

Fix a few errors in the KeyRotationDeviceCloudPolicyTest.Basic browser
test - some of them were presumably causing the flakiness that led to
the test being disabled, some of them were accidentally added during
later refactorings:

1. Correctly pipe through LocalPolicyTestServerMixin the needed settings
   of policy_testserver: the usage of signing keys bundled into the
   Python server and the automatic rotation of the signing key with
   every policy request.
2. Fix misplaced calls in the test's SetUp() phase: everything needs to
   be done *before* calling the parent SetUp() in order to run before
   the test.
3. Make the test less fragile by doing two explicit fetches of policy
   inside the test body and checking that the key got rotated in
   between (instead of relying on the device policy stack making the
   first policy fetch during its initialization and on the right parity
   of the PolicyBuilder's policy key version).

Bug: 900631
Test: run the test 10'000 times (browser_tests --gtest_filter="KeyRotationDeviceCloudPolicyTest.Basic")
Change-Id: I5eaa4f6e03a54dcc26319d39323f5edf51904c65
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1829383
Commit-Queue: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Denis Kuznetsov <antrim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#702976}

chrome/browser/chromeos/login/test/local_policy_test_server_mixin.cc

@@ -51,9 +51,14 @@ void LocalPolicyTestServerMixin::SetUp() {
                                       policy::PolicyBuilder::kFakeDeviceId,
                                       {} /* state_keys */);
 
-  CHECK(policy_test_server_->SetSigningKeyAndSignature(
-      policy::PolicyBuilder::CreateTestSigningKey().get(),
-      policy::PolicyBuilder::GetTestSigningKeySignature()));
+  if (!canned_signing_keys_enabled_) {
+    CHECK(policy_test_server_->SetSigningKeyAndSignature(
+        policy::PolicyBuilder::CreateTestSigningKey().get(),
+        policy::PolicyBuilder::GetTestSigningKeySignature()));
+  }
+
+  if (automatic_rotation_of_signing_keys_enabled_)
+    policy_test_server_->EnableAutomaticRotationOfSigningKeys();
 
   CHECK(policy_test_server_->Start());
 }
@@ -257,6 +262,16 @@ void LocalPolicyTestServerMixin::ConfigureFakeStatisticsForZeroTouch(
                                 test::kTestHardwareClass);
 }
 
+void LocalPolicyTestServerMixin::EnableCannedSigningKeys() {
+  DCHECK(!policy_test_server_);
+  canned_signing_keys_enabled_ = true;
+}
+
+void LocalPolicyTestServerMixin::EnableAutomaticRotationOfSigningKeys() {
+  DCHECK(!policy_test_server_);
+  automatic_rotation_of_signing_keys_enabled_ = true;
+}
+
 LocalPolicyTestServerMixin::~LocalPolicyTestServerMixin() = default;
 
 }  // namespace chromeos

chrome/browser/chromeos/login/test/local_policy_test_server_mixin.h

@@ -112,9 +112,21 @@ class LocalPolicyTestServerMixin : public InProcessBrowserTestMixin {
   void ConfigureFakeStatisticsForZeroTouch(
       system::ScopedFakeStatisticsProvider* provider);
 
+  // Enables the usage of keys canned into the policy test server, instead of
+  // specifying to it the test key to be used.  This must be called before
+  // starting the server.
+  void EnableCannedSigningKeys();
+
+  // Enables the automatic rotation of the policy signing keys with each policy
+  // fetch request. This must be called before starting the server, and only
+  // works when the server serves from a temporary directory.
+  void EnableAutomaticRotationOfSigningKeys();
+
  private:
   std::unique_ptr<policy::LocalPolicyTestServer> policy_test_server_;
   base::Value server_config_;
+  bool canned_signing_keys_enabled_ = false;
+  bool automatic_rotation_of_signing_keys_enabled_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(LocalPolicyTestServerMixin);
 };

chrome/browser/chromeos/policy/device_cloud_policy_browsertest.cc

@@ -20,6 +20,7 @@
 #include "chrome/browser/chromeos/login/test/local_policy_test_server_mixin.h"
 #include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
 #include "chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.h"
+#include "chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.h"
 #include "chrome/browser/chromeos/policy/device_policy_builder.h"
 #include "chrome/browser/chromeos/policy/device_policy_cros_browser_test.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
@@ -101,30 +102,25 @@ namespace {
 // rotating the policy key automatically with each policy fetch.
 class KeyRotationDeviceCloudPolicyTest : public DevicePolicyCrosBrowserTest {
  protected:
-  const int kTestPolicyValue = 123;
-  const int kTestPolicyOtherValue = 456;
-  const char* const kTestPolicyKey = key::kDevicePolicyRefreshRate;
-
-  KeyRotationDeviceCloudPolicyTest() {}
-
-  void SetUp() override {
-    UpdateBuiltTestPolicyValue(kTestPolicyValue);
-    DevicePolicyCrosBrowserTest::SetUp();
-    local_policy_mixin_.server()->EnableAutomaticRotationOfSigningKeys();
-    UpdateServedTestPolicy();
+  const int kInitialPolicyValue = 123;
+  const int kSecondPolicyValue = 456;
+  const int kThirdPolicyValue = 789;
+  const char* const kPolicyKey = key::kDevicePolicyRefreshRate;
+
+  KeyRotationDeviceCloudPolicyTest() {
+    UpdateBuiltTestPolicyValue(kInitialPolicyValue);
+    local_policy_mixin_.EnableCannedSigningKeys();
+    local_policy_mixin_.EnableAutomaticRotationOfSigningKeys();
   }
 
   void SetUpInProcessBrowserTestFixture() override {
     DevicePolicyCrosBrowserTest::SetUpInProcessBrowserTestFixture();
     SetFakeDevicePolicy();
+    UpdateServedTestPolicy();
   }
 
   void SetUpOnMainThread() override {
     DevicePolicyCrosBrowserTest::SetUpOnMainThread();
-    g_browser_process->platform_part()
-        ->browser_policy_connector_chromeos()
-        ->device_management_service()
-        ->ScheduleInitialization(0);
     StartObservingTestPolicy();
   }
 
@@ -146,7 +142,27 @@ class KeyRotationDeviceCloudPolicyTest : public DevicePolicyCrosBrowserTest {
         local_policy_mixin_.UpdateDevicePolicy(device_policy()->payload()));
   }
 
-  int GetTestPolicyValue() {
+  void StartDevicePolicyRefresh() {
+    g_browser_process->platform_part()
+        ->browser_policy_connector_chromeos()
+        ->GetDeviceCloudPolicyManager()
+        ->RefreshPolicies();
+  }
+
+  std::string GetOwnerPublicKey() const {
+    return chromeos::DeviceSettingsService::Get()->GetPublicKey()->as_string();
+  }
+
+  int GetInstalledPolicyKeyVersion() const {
+    return g_browser_process->platform_part()
+        ->browser_policy_connector_chromeos()
+        ->GetDeviceCloudPolicyManager()
+        ->device_store()
+        ->policy()
+        ->public_key_version();
+  }
+
+  int GetInstalledPolicyValue() {
     PolicyService* const policy_service =
         g_browser_process->platform_part()
             ->browser_policy_connector_chromeos()
@@ -155,22 +171,23 @@ class KeyRotationDeviceCloudPolicyTest : public DevicePolicyCrosBrowserTest {
         policy_service
             ->GetPolicies(PolicyNamespace(POLICY_DOMAIN_CHROME,
                                           std::string() /* component_id */))
-            .GetValue(kTestPolicyKey);
+            .GetValue(kPolicyKey);
     EXPECT_TRUE(policy_value);
     int refresh_rate = -1;
     EXPECT_TRUE(policy_value->GetAsInteger(&refresh_rate));
     return refresh_rate;
   }
 
-  void WaitForTestPolicyValue(int expected_policy_value) {
-    if (GetTestPolicyValue() == expected_policy_value)
+  void WaitForInstalledPolicyValue(int expected_policy_value) {
+    if (GetInstalledPolicyValue() == expected_policy_value)
       return;
     awaited_policy_value_ = expected_policy_value;
-    // The run loop will be terminated by TestPolicyChangedCallback() once the
-    // policy value becomes equal to the awaited value.
+    // The run loop will be terminated by OnPolicyChanged() once the policy
+    // value becomes equal to the awaited value.
     DCHECK(!policy_change_waiting_run_loop_);
     policy_change_waiting_run_loop_ = std::make_unique<base::RunLoop>();
     policy_change_waiting_run_loop_->Run();
+    policy_change_waiting_run_loop_.reset();
   }
 
  private:
@@ -178,7 +195,7 @@ class KeyRotationDeviceCloudPolicyTest : public DevicePolicyCrosBrowserTest {
     device_policy()
         ->payload()
         .mutable_device_policy_refresh_rate()
-        ->set_device_policy_refresh_rate(kTestPolicyValue);
+        ->set_device_policy_refresh_rate(kInitialPolicyValue);
     device_policy()->Build();
     session_manager_client()->set_device_policy(device_policy()->GetBlob());
   }
@@ -191,16 +208,15 @@ class KeyRotationDeviceCloudPolicyTest : public DevicePolicyCrosBrowserTest {
         PolicyNamespace(POLICY_DOMAIN_CHROME,
                         std::string() /* component_id */));
     policy_change_registrar_->Observe(
-        kTestPolicyKey,
-        base::BindRepeating(
-            &KeyRotationDeviceCloudPolicyTest::TestPolicyChangedCallback,
-            base::Unretained(this)));
+        kPolicyKey,
+        base::BindRepeating(&KeyRotationDeviceCloudPolicyTest::OnPolicyChanged,
+                            base::Unretained(this)));
   }
 
-  void TestPolicyChangedCallback(const base::Value* old_value,
-                                 const base::Value* new_value) {
+  void OnPolicyChanged(const base::Value* old_value,
+                       const base::Value* new_value) {
     if (policy_change_waiting_run_loop_ &&
-        GetTestPolicyValue() == awaited_policy_value_) {
+        GetInstalledPolicyValue() == awaited_policy_value_) {
       policy_change_waiting_run_loop_->Quit();
     }
   }
@@ -215,30 +231,35 @@ class KeyRotationDeviceCloudPolicyTest : public DevicePolicyCrosBrowserTest {
 
 }  // namespace
 
-// Disabled due to flake. https://crbug.com/900631
-IN_PROC_BROWSER_TEST_F(KeyRotationDeviceCloudPolicyTest, DISABLED_Basic) {
-  // Initially, the policy has the first value.
-  EXPECT_EQ(kTestPolicyValue, GetTestPolicyValue());
+IN_PROC_BROWSER_TEST_F(KeyRotationDeviceCloudPolicyTest, Basic) {
+  // The policy has the initial value from the cache.
+  EXPECT_EQ(kInitialPolicyValue, GetInstalledPolicyValue());
 
-  const std::string original_owner_public_key =
-      chromeos::DeviceSettingsService::Get()->GetPublicKey()->as_string();
-
-  // The server is updated to serve the new policy value, and the client fetches
-  // it.
-  UpdateBuiltTestPolicyValue(kTestPolicyOtherValue);
+  // The server is updated to serve the second policy value, and the client
+  // fetches it.
+  UpdateBuiltTestPolicyValue(kSecondPolicyValue);
+  UpdateServedTestPolicy();
+  StartDevicePolicyRefresh();
+  WaitForInstalledPolicyValue(kSecondPolicyValue);
+  EXPECT_EQ(kSecondPolicyValue, GetInstalledPolicyValue());
+
+  // Remember the key and the version after the fetch of the second value.
+  const std::string owner_public_key = GetOwnerPublicKey();
+  CHECK(!owner_public_key.empty());
+  const int key_version = GetInstalledPolicyKeyVersion();
+
+  // The server is updated to serve the third policy value, and the client
+  // fetches it.
+  UpdateBuiltTestPolicyValue(kThirdPolicyValue);
   UpdateServedTestPolicy();
-  g_browser_process->platform_part()
-      ->browser_policy_connector_chromeos()
-      ->GetDeviceCloudPolicyManager()
-      ->RefreshPolicies();
-  WaitForTestPolicyValue(kTestPolicyOtherValue);
-  EXPECT_EQ(kTestPolicyOtherValue, GetTestPolicyValue());
-
-  // The owner key has changed due to the key rotation performed by the policy
-  // test server.
-  EXPECT_NE(
-      original_owner_public_key,
-      chromeos::DeviceSettingsService::Get()->GetPublicKey()->as_string());
+  StartDevicePolicyRefresh();
+  WaitForInstalledPolicyValue(kThirdPolicyValue);
+  EXPECT_EQ(kThirdPolicyValue, GetInstalledPolicyValue());
+
+  // The owner key got rotated on the client, as requested by the server, and
+  // the key version got incremented.
+  EXPECT_NE(owner_public_key, GetOwnerPublicKey());
+  EXPECT_EQ(key_version + 1, GetInstalledPolicyKeyVersion());
 }
 
 namespace {