child user: Centralize calls to refresh policy in-session.

Call CloudPolicyService::RefreshPolicy instead of
CloudPolicyClient::FetchPolicy in CloudPolicyRefreshScheduler, so there
is a common path for in session policy fetches.
It allows to add additional logic for policy fetch to CloudPolicyService.
It is a part of larger change: passing credentials with child user policy
fetch.

Bug: 839025
Test: CloudPolicyRefreshSchedulerTest + manually
Change-Id: I3f5b497ef28179d3d181d45b30bf11b4a4f3fcac
Reviewed-on: https://chromium-review.googlesource.com/c/1339099
Commit-Queue: Aga Wronska <agawronska@chromium.org>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609883}

components/policy/core/common/BUILD.gn

@@ -246,6 +246,8 @@ jumbo_static_library("test_support") {
     "cloud/mock_cloud_external_data_manager.h",
     "cloud/mock_cloud_policy_client.cc",
     "cloud/mock_cloud_policy_client.h",
+    "cloud/mock_cloud_policy_service.cc",
+    "cloud/mock_cloud_policy_service.h",
     "cloud/mock_cloud_policy_store.cc",
     "cloud/mock_cloud_policy_store.h",
     "cloud/mock_device_management_service.cc",

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

@@ -82,9 +82,9 @@ void CloudPolicyCore::RefreshSoon() {
 
 void CloudPolicyCore::StartRefreshScheduler() {
   if (!refresh_scheduler_) {
-    refresh_scheduler_.reset(
-        new CloudPolicyRefreshScheduler(client_.get(), store_, task_runner_,
-                                        network_connection_tracker_getter_));
+    refresh_scheduler_ = std::make_unique<CloudPolicyRefreshScheduler>(
+        client_.get(), store_, service_.get(), task_runner_,
+        network_connection_tracker_getter_);
     UpdateRefreshDelayFromPref();
     for (auto& observer : observers_)
       observer.OnRefreshSchedulerStarted(this);

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

@@ -9,10 +9,12 @@
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
+#include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/sequenced_task_runner.h"
 #include "build/build_config.h"
 #include "components/policy/core/common/cloud/cloud_policy_constants.h"
+#include "components/policy/core/common/cloud/cloud_policy_service.h"
 
 namespace policy {
 
@@ -57,10 +59,12 @@ const int64_t CloudPolicyRefreshScheduler::kRefreshDelayMaxMs =
 CloudPolicyRefreshScheduler::CloudPolicyRefreshScheduler(
     CloudPolicyClient* client,
     CloudPolicyStore* store,
+    CloudPolicyService* service,
     const scoped_refptr<base::SequencedTaskRunner>& task_runner,
     network::NetworkConnectionTrackerGetter network_connection_tracker_getter)
     : client_(client),
       store_(store),
+      service_(service),
       task_runner_(task_runner),
       network_connection_tracker_(network_connection_tracker_getter.Run()),
       error_retry_delay_ms_(kInitialErrorRetryDelayMs),
@@ -333,9 +337,12 @@ void CloudPolicyRefreshScheduler::PerformRefresh() {
     // Update |last_refresh_| so another fetch isn't triggered inadvertently.
     UpdateLastRefresh();
 
-    // The result of this operation will be reported through a callback, at
-    // which point the next refresh will be scheduled.
-    client_->FetchPolicy();
+    // The result of this operation will be reported through OnPolicyFetched()
+    // and OnPolicyRefreshed() callbacks. Next refresh will be scheduled in
+    // OnPolicyFetched().
+    service_->RefreshPolicy(
+        base::BindRepeating(&CloudPolicyRefreshScheduler::OnPolicyRefreshed,
+                            base::Unretained(this)));
     return;
   }
 
@@ -374,4 +381,10 @@ void CloudPolicyRefreshScheduler::UpdateLastRefresh() {
   last_refresh_ticks_ = base::TimeTicks::Now();
 }
 
+void CloudPolicyRefreshScheduler::OnPolicyRefreshed(bool success) {
+  // Next policy fetch is scheduled in OnPolicyFetched() callback.
+  VLOG(1) << "Scheduled policy refresh "
+          << (success ? "successful" : "unsuccessful");
+}
+
 }  // namespace policy

components/policy/core/common/cloud/cloud_policy_refresh_scheduler.h

@@ -22,6 +22,8 @@ class SequencedTaskRunner;
 
 namespace policy {
 
+class CloudPolicyService;
+
 // Observes CloudPolicyClient and CloudPolicyStore to trigger periodic policy
 // fetches and issue retries on error conditions.
 class POLICY_EXPORT CloudPolicyRefreshScheduler
@@ -39,11 +41,12 @@ class POLICY_EXPORT CloudPolicyRefreshScheduler
   static const int64_t kRefreshDelayMinMs;
   static const int64_t kRefreshDelayMaxMs;
 
-  // |client| and |store| pointers must stay valid throughout the
+  // |client|, |store| and |service| pointers must stay valid throughout the
   // lifetime of CloudPolicyRefreshScheduler.
   CloudPolicyRefreshScheduler(
       CloudPolicyClient* client,
       CloudPolicyStore* store,
+      CloudPolicyService* service,
       const scoped_refptr<base::SequencedTaskRunner>& task_runner,
       network::NetworkConnectionTrackerGetter
           network_connection_tracker_getter);
@@ -115,8 +118,15 @@ class POLICY_EXPORT CloudPolicyRefreshScheduler
   // Sets the |last_refresh_| and |last_refresh_ticks_| to current time.
   void UpdateLastRefresh();
 
+  // Called when policy was refreshed after refresh request.
+  // It is different than OnPolicyFetched(), which will be called every time
+  // policy was fetched by the |client_|, does not matter where it was
+  // requested.
+  void OnPolicyRefreshed(bool success);
+
   CloudPolicyClient* client_;
   CloudPolicyStore* store_;
+  CloudPolicyService* service_;
 
   // For scheduling delayed tasks.
   const scoped_refptr<base::SequencedTaskRunner> task_runner_;

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

@@ -17,6 +17,7 @@
 #include "build/build_config.h"
 #include "components/policy/core/common/cloud/cloud_policy_constants.h"
 #include "components/policy/core/common/cloud/mock_cloud_policy_client.h"
+#include "components/policy/core/common/cloud/mock_cloud_policy_service.h"
 #include "components/policy/core/common/cloud/mock_cloud_policy_store.h"
 #include "services/network/test/test_network_connection_tracker.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -25,6 +26,7 @@
 namespace em = enterprise_management;
 
 using testing::Mock;
+using testing::_;
 
 namespace policy {
 
@@ -39,7 +41,8 @@ const int64_t kInitialCacheAgeMinutes = 1;
 class CloudPolicyRefreshSchedulerTest : public testing::Test {
  protected:
   CloudPolicyRefreshSchedulerTest()
-      : task_runner_(new base::TestSimpleTaskRunner()) {}
+      : service_(std::make_unique<MockCloudPolicyService>(&client_, &store_)),
+        task_runner_(new base::TestSimpleTaskRunner()) {}
 
   void SetUp() override {
     client_.SetDMToken("token");
@@ -60,7 +63,7 @@ class CloudPolicyRefreshSchedulerTest : public testing::Test {
   CloudPolicyRefreshScheduler* CreateRefreshScheduler() {
     EXPECT_EQ(0u, task_runner_->NumPendingTasks());
     CloudPolicyRefreshScheduler* scheduler = new CloudPolicyRefreshScheduler(
-        &client_, &store_, task_runner_,
+        &client_, &store_, service_.get(), task_runner_,
         network::TestNetworkConnectionTracker::CreateGetter());
     // Make sure the NetworkConnectionTracker has been set up.
     base::RunLoop().RunUntilIdle();
@@ -168,6 +171,7 @@ class CloudPolicyRefreshSchedulerTest : public testing::Test {
   base::MessageLoop loop_;
   MockCloudPolicyClient client_;
   MockCloudPolicyStore store_;
+  std::unique_ptr<MockCloudPolicyService> service_;
   scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
 
   // Base time for the refresh that the scheduler should be using.
@@ -181,6 +185,7 @@ TEST_F(CloudPolicyRefreshSchedulerTest, InitialRefreshNoPolicy) {
       CreateRefreshScheduler());
   EXPECT_TRUE(task_runner_->HasPendingTask());
   EXPECT_EQ(GetLastDelay(), base::TimeDelta());
+  EXPECT_CALL(*service_.get(), RefreshPolicy(_)).Times(1);
   EXPECT_CALL(client_, FetchPolicy()).Times(1);
   task_runner_->RunUntilIdle();
 }
@@ -190,6 +195,7 @@ TEST_F(CloudPolicyRefreshSchedulerTest, InitialRefreshUnmanaged) {
   std::unique_ptr<CloudPolicyRefreshScheduler> scheduler(
       CreateRefreshScheduler());
   CheckTiming(CloudPolicyRefreshScheduler::kUnmanagedRefreshDelayMs);
+  EXPECT_CALL(*service_.get(), RefreshPolicy(_)).Times(1);
   EXPECT_CALL(client_, FetchPolicy()).Times(1);
   task_runner_->RunUntilIdle();
 }
@@ -199,6 +205,7 @@ TEST_F(CloudPolicyRefreshSchedulerTest, InitialRefreshManagedNotYetFetched) {
       CreateRefreshScheduler());
   EXPECT_TRUE(task_runner_->HasPendingTask());
   CheckInitialRefresh(false);
+  EXPECT_CALL(*service_.get(), RefreshPolicy(_)).Times(1);
   EXPECT_CALL(client_, FetchPolicy()).Times(1);
   task_runner_->RunUntilIdle();
 }
@@ -210,6 +217,7 @@ TEST_F(CloudPolicyRefreshSchedulerTest, InitialRefreshManagedAlreadyFetched) {
   std::unique_ptr<CloudPolicyRefreshScheduler> scheduler(
       CreateRefreshScheduler());
   CheckTiming(kPolicyRefreshRate);
+  EXPECT_CALL(*service_.get(), RefreshPolicy(_)).Times(1);
   EXPECT_CALL(client_, FetchPolicy()).Times(1);
   task_runner_->RunUntilIdle();
 }
@@ -230,6 +238,7 @@ TEST_F(CloudPolicyRefreshSchedulerTest, Unregistered) {
 TEST_F(CloudPolicyRefreshSchedulerTest, RefreshSoon) {
   std::unique_ptr<CloudPolicyRefreshScheduler> scheduler(
       CreateRefreshScheduler());
+  EXPECT_CALL(*service_.get(), RefreshPolicy(_)).Times(1);
   EXPECT_CALL(client_, FetchPolicy()).Times(1);
   scheduler->RefreshSoon();
   task_runner_->RunUntilIdle();
@@ -257,6 +266,7 @@ TEST_F(CloudPolicyRefreshSchedulerTest, RefreshSoonOverriding) {
   store_.NotifyStoreLoaded();
   CheckTiming(0);
 
+  EXPECT_CALL(*service_.get(), RefreshPolicy(_)).Times(1);
   EXPECT_CALL(client_, FetchPolicy()).Times(1);
   task_runner_->RunUntilIdle();
   Mock::VerifyAndClearExpectations(&client_);
@@ -269,7 +279,7 @@ TEST_F(CloudPolicyRefreshSchedulerTest, RefreshSoonOverriding) {
 TEST_F(CloudPolicyRefreshSchedulerTest, InvalidationsAvailable) {
   std::unique_ptr<CloudPolicyRefreshScheduler> scheduler(
       new CloudPolicyRefreshScheduler(
-          &client_, &store_, task_runner_,
+          &client_, &store_, service_.get(), task_runner_,
           network::TestNetworkConnectionTracker::CreateGetter()));
   scheduler->SetDesiredRefreshDelay(kPolicyRefreshRate);
 
@@ -284,6 +294,7 @@ TEST_F(CloudPolicyRefreshSchedulerTest, InvalidationsAvailable) {
 
   CheckInitialRefresh(true);
 
+  EXPECT_CALL(*service_.get(), RefreshPolicy(_)).Times(1);
   EXPECT_CALL(client_, FetchPolicy()).Times(1);
   task_runner_->RunPendingTasks();
   Mock::VerifyAndClearExpectations(&client_);
@@ -300,7 +311,7 @@ TEST_F(CloudPolicyRefreshSchedulerTest, InvalidationsAvailable) {
 TEST_F(CloudPolicyRefreshSchedulerTest, InvalidationsNotAvailable) {
   std::unique_ptr<CloudPolicyRefreshScheduler> scheduler(
       new CloudPolicyRefreshScheduler(
-          &client_, &store_, task_runner_,
+          &client_, &store_, service_.get(), task_runner_,
           network::TestNetworkConnectionTracker::CreateGetter()));
   scheduler->SetDesiredRefreshDelay(kPolicyRefreshRate);
 
@@ -316,6 +327,7 @@ TEST_F(CloudPolicyRefreshSchedulerTest, InvalidationsNotAvailable) {
   EXPECT_EQ(kPolicyRefreshRate, scheduler->GetActualRefreshDelay());
 
   // Perform that fetch now.
+  EXPECT_CALL(*service_.get(), RefreshPolicy(_)).Times(1);
   EXPECT_CALL(client_, FetchPolicy()).Times(1);
   task_runner_->RunPendingTasks();
   Mock::VerifyAndClearExpectations(&client_);
@@ -332,11 +344,12 @@ TEST_F(CloudPolicyRefreshSchedulerTest, InvalidationsNotAvailable) {
 TEST_F(CloudPolicyRefreshSchedulerTest, InvalidationsOffAndOn) {
   std::unique_ptr<CloudPolicyRefreshScheduler> scheduler(
       new CloudPolicyRefreshScheduler(
-          &client_, &store_, task_runner_,
+          &client_, &store_, service_.get(), task_runner_,
           network::TestNetworkConnectionTracker::CreateGetter()));
   scheduler->SetDesiredRefreshDelay(kPolicyRefreshRate);
   scheduler->SetInvalidationServiceAvailability(true);
   // Initial fetch.
+  EXPECT_CALL(*service_.get(), RefreshPolicy(_)).Times(1);
   EXPECT_CALL(client_, FetchPolicy()).Times(1);
   task_runner_->RunUntilIdle();
   Mock::VerifyAndClearExpectations(&client_);
@@ -352,6 +365,7 @@ TEST_F(CloudPolicyRefreshSchedulerTest, InvalidationsOffAndOn) {
   scheduler->SetInvalidationServiceAvailability(false);
   scheduler->SetInvalidationServiceAvailability(true);
   // The next refresh has been scheduled using a lower refresh rate.
+  EXPECT_CALL(*service_.get(), RefreshPolicy(_)).Times(1);
   EXPECT_CALL(client_, FetchPolicy()).Times(1);
   CheckTiming(CloudPolicyRefreshScheduler::kWithInvalidationsRefreshDelayMs);
   task_runner_->RunPendingTasks();
@@ -361,11 +375,12 @@ TEST_F(CloudPolicyRefreshSchedulerTest, InvalidationsOffAndOn) {
 TEST_F(CloudPolicyRefreshSchedulerTest, InvalidationsDisconnected) {
   std::unique_ptr<CloudPolicyRefreshScheduler> scheduler(
       new CloudPolicyRefreshScheduler(
-          &client_, &store_, task_runner_,
+          &client_, &store_, service_.get(), task_runner_,
           network::TestNetworkConnectionTracker::CreateGetter()));
   scheduler->SetDesiredRefreshDelay(kPolicyRefreshRate);
   scheduler->SetInvalidationServiceAvailability(true);
   // Initial fetch.
+  EXPECT_CALL(*service_.get(), RefreshPolicy(_)).Times(1);
   EXPECT_CALL(client_, FetchPolicy()).Times(1);
   task_runner_->RunUntilIdle();
   Mock::VerifyAndClearExpectations(&client_);
@@ -375,6 +390,7 @@ TEST_F(CloudPolicyRefreshSchedulerTest, InvalidationsDisconnected) {
   // The next refresh has been scheduled using a lower refresh rate.
   // Flush that task.
   CheckTiming(CloudPolicyRefreshScheduler::kWithInvalidationsRefreshDelayMs);
+  EXPECT_CALL(*service_.get(), RefreshPolicy(_)).Times(1);
   EXPECT_CALL(client_, FetchPolicy()).Times(1);
   task_runner_->RunPendingTasks();
   Mock::VerifyAndClearExpectations(&client_);

components/policy/core/common/cloud/cloud_policy_service.h

@@ -50,7 +50,7 @@ class POLICY_EXPORT CloudPolicyService : public CloudPolicyClient::Observer,
 
   // Refreshes policy. |callback| will be invoked after the operation completes
   // or aborts because of errors.
-  void RefreshPolicy(const RefreshPolicyCallback& callback);
+  virtual void RefreshPolicy(const RefreshPolicyCallback& callback);
 
   // Unregisters the device. |callback| will be invoked after the operation
   // completes or aborts because of errors. All pending refresh policy requests

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

@@ -10,6 +10,7 @@
 #include "base/macros.h"
 #include "components/policy/core/common/cloud/cloud_policy_constants.h"
 #include "components/policy/core/common/cloud/mock_cloud_policy_client.h"
+#include "components/policy/core/common/cloud/mock_cloud_policy_service.h"
 #include "components/policy/core/common/cloud/mock_cloud_policy_store.h"
 #include "components/policy/proto/device_management_backend.pb.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -21,17 +22,6 @@ using testing::_;
 
 namespace policy {
 
-class MockCloudPolicyServiceObserver : public CloudPolicyService::Observer {
- public:
-  MockCloudPolicyServiceObserver() {}
-  ~MockCloudPolicyServiceObserver() override {}
-
-  MOCK_METHOD0(OnCloudPolicyServiceInitializationCompleted, void());
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(MockCloudPolicyServiceObserver);
-};
-
 class CloudPolicyServiceTest : public testing::Test {
  public:
   CloudPolicyServiceTest()

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

+// Copyright 2018 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 "components/policy/core/common/cloud/mock_cloud_policy_service.h"
+#include "components/policy/core/common/cloud/cloud_policy_client.h"
+#include "components/policy/core/common/cloud/cloud_policy_store.h"
+
+namespace policy {
+
+MockCloudPolicyService::MockCloudPolicyService(CloudPolicyClient* client,
+                                               CloudPolicyStore* store)
+    : CloudPolicyService(std::string(), std::string(), client, store) {
+  // Besides recording the mock call, invoke real RefreshPolicy() method.
+  // That way FetchPolicy() is called on the |client|.
+  ON_CALL(*this, RefreshPolicy(testing::_))
+      .WillByDefault(
+          testing::Invoke(this, &MockCloudPolicyService::InvokeRefreshPolicy));
+}
+
+MockCloudPolicyService::~MockCloudPolicyService() = default;
+
+void MockCloudPolicyService::InvokeRefreshPolicy(
+    const RefreshPolicyCallback& callback) {
+  CloudPolicyService::RefreshPolicy(callback);
+}
+
+MockCloudPolicyServiceObserver::MockCloudPolicyServiceObserver() = default;
+
+MockCloudPolicyServiceObserver::~MockCloudPolicyServiceObserver() = default;
+
+}  // namespace policy

components/policy/core/common/cloud/mock_cloud_policy_service.h

+// Copyright 2018 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 COMPONENTS_POLICY_CORE_COMMON_CLOUD_MOCK_CLOUD_POLICY_SERVICE_H_
+#define COMPONENTS_POLICY_CORE_COMMON_CLOUD_MOCK_CLOUD_POLICY_SERVICE_H_
+
+#include "base/macros.h"
+#include "components/policy/core/common/cloud/cloud_policy_service.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+namespace policy {
+
+class CloudPolicyClient;
+class CloudPolicyStore;
+
+class MockCloudPolicyService : public CloudPolicyService {
+ public:
+  MockCloudPolicyService(CloudPolicyClient* client, CloudPolicyStore* store);
+  ~MockCloudPolicyService() override;
+
+  MOCK_METHOD1(RefreshPolicy, void(const RefreshPolicyCallback&));
+
+ private:
+  // Invokes real RefreshPolicy() method.
+  void InvokeRefreshPolicy(const RefreshPolicyCallback& callback);
+
+  DISALLOW_COPY_AND_ASSIGN(MockCloudPolicyService);
+};
+
+class MockCloudPolicyServiceObserver : public CloudPolicyService::Observer {
+ public:
+  MockCloudPolicyServiceObserver();
+  ~MockCloudPolicyServiceObserver() override;
+
+  MOCK_METHOD0(OnCloudPolicyServiceInitializationCompleted, void());
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MockCloudPolicyServiceObserver);
+};
+
+}  // namespace policy
+
+#endif  // COMPONENTS_POLICY_CORE_COMMON_CLOUD_MOCK_CLOUD_POLICY_SERVICE_H_