Implement device-local account policy pushing

chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc

Index: chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc
diff --git a/chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc b/chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc
index 155e2e3732a78923a62eca261283dca34d82645f..33ef5046121999a760855f442969b3cd56946d6f 100644
--- a/chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc
+++ b/chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc
@@ -26,9 +26,15 @@
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chrome/browser/chromeos/settings/device_settings_service.h"
 #include "chrome/browser/chromeos/settings/device_settings_test_helper.h"
+#include "chrome/browser/invalidation/fake_invalidation_service.h"
+#include "chrome/browser/policy/cloud/cloud_policy_invalidator.h"
 #include "chrome/common/chrome_paths.h"
+#include "chrome/test/base/testing_browser_process.h"
+#include "chrome/test/base/testing_profile_manager.h"
 #include "chromeos/chromeos_paths.h"
 #include "chromeos/dbus/power_policy_controller.h"
+#include "components/invalidation/invalidation.h"
+#include "components/invalidation/object_id_invalidation_map.h"
 #include "components/policy/core/common/cloud/cloud_policy_client.h"
 #include "components/policy/core/common/cloud/cloud_policy_constants.h"
 #include "components/policy/core/common/cloud/cloud_policy_service.h"
@@ -39,6 +45,7 @@
 #include "components/policy/core/common/policy_bundle.h"
 #include "components/policy/core/common/policy_map.h"
 #include "components/policy/core/common/schema_registry.h"
+#include "google/cacheinvalidation/include/types.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "net/url_request/url_request_test_util.h"
 #include "policy/policy_constants.h"
@@ -67,6 +74,9 @@ const char kExtensionVersion[] = "1.0.0.0";
 const char kExtensionCRXPath[] = "extensions/hosted_app.crx";
 const char kUpdateURL[] = "https://clients2.google.com/service/update2/crx";
 
+const int kInvalidationSource = 123;
+const char kInvalidationName[] = "invalidation";
+
 }  // namespace
 
 class MockDeviceLocalAccountPolicyServiceObserver
@@ -98,6 +108,9 @@ class DeviceLocalAccountPolicyServiceTestBase
   chromeos::CrosSettings cros_settings_;
   scoped_refptr<base::TestSimpleTaskRunner> extension_cache_task_runner_;
   MockDeviceManagementService mock_device_management_service_;
+  TestingProfileManager profile_manager_;
+  scoped_ptr<AffiliatedInvalidationServiceProvider>
+      affiliated_invalidation_service_provider_;
   scoped_ptr<DeviceLocalAccountPolicyService> service_;
 
  private:
@@ -117,6 +130,8 @@ class DeviceLocalAccountPolicyServiceTest
 
   void InstallDevicePolicy() override;
 
+  void FlushDeviceLocalAccountPolicyFetch();

[pneubeck (no reviews), 2015/01/30 14:23:58]

i think this function has enough side effect that it deserves a comment.

isn't this more of a 'FlushDeviceLocalAccountAndFetchPolicy()' or '...ToFetch..'
?

[bartfab (slow), 2015/02/03 18:31:06]

Obsolete.

+
   MockDeviceLocalAccountPolicyServiceObserver service_observer_;
 
  private:
@@ -132,12 +147,23 @@ DeviceLocalAccountPolicyServiceTestBase::
           kAccount2,
           DeviceLocalAccount::TYPE_PUBLIC_SESSION)),
       cros_settings_(&device_settings_service_),
-      extension_cache_task_runner_(new base::TestSimpleTaskRunner) {
+      extension_cache_task_runner_(new base::TestSimpleTaskRunner),
+      profile_manager_(TestingBrowserProcess::GetGlobal()) {
 }
 
 void DeviceLocalAccountPolicyServiceTestBase::SetUp() {
   chromeos::DeviceSettingsTestBase::SetUp();
 
+  ASSERT_TRUE(profile_manager_.SetUp());
+
+  affiliated_invalidation_service_provider_.reset(
+      new AffiliatedInvalidationServiceProvider);

[pneubeck (no reviews), 2015/01/30 14:23:58]

you could make the relevant methods of the provider virtual and replace it by a
mock. If i see it correctly only the un-/register functions are used.

[bartfab (slow), 2015/02/03 18:31:06]

Done.

+  // Mark the |affiliated_invalidation_service_provider_| as shut down
+  // immediately. This will prevent it from trying to find an available
+  // invalidation service, which pulls in a lot of additional run-time
+  // dependencies.
+  affiliated_invalidation_service_provider_->Shutdown();
+
   expected_policy_map_.Set(key::kDisableSpdy,
                            POLICY_LEVEL_MANDATORY,
                            POLICY_SCOPE_USER,
@@ -162,6 +188,7 @@ void DeviceLocalAccountPolicyServiceTestBase::CreatePolicyService() {
       &device_settings_test_helper_,
       &device_settings_service_,
       &cros_settings_,
+      affiliated_invalidation_service_provider_.get(),
       base::MessageLoopProxy::current(),
       extension_cache_task_runner_,
       base::MessageLoopProxy::current(),
@@ -194,11 +221,12 @@ void DeviceLocalAccountPolicyServiceTestBase::InstallDevicePolicy() {
 }
 
 DeviceLocalAccountPolicyServiceTest::DeviceLocalAccountPolicyServiceTest() {
-  CreatePolicyService();
 }
 
 void DeviceLocalAccountPolicyServiceTest::SetUp() {
   DeviceLocalAccountPolicyServiceTestBase::SetUp();
+
+  CreatePolicyService();
   service_->AddObserver(&service_observer_);
 }
 
@@ -213,6 +241,54 @@ void DeviceLocalAccountPolicyServiceTest::InstallDevicePolicy() {
   Mock::VerifyAndClearExpectations(&service_observer_);
 }
 
+void DeviceLocalAccountPolicyServiceTest::FlushDeviceLocalAccountPolicyFetch() {
+  em::DeviceManagementRequest request;
+  em::DeviceManagementResponse response;
+  response.mutable_policy_response()->add_response()->CopyFrom(
+      device_local_account_policy_.policy());
+  EXPECT_CALL(mock_device_management_service_,
+              CreateJob(DeviceManagementRequestJob::TYPE_POLICY_FETCH, _))
+      .WillOnce(mock_device_management_service_.SucceedJob(response));
+  EXPECT_CALL(mock_device_management_service_,
+              StartJob(dm_protocol::kValueRequestPolicy,
+                       std::string(), std::string(),
+                       device_policy_.policy_data().request_token(),
+                       dm_protocol::kValueUserAffiliationManaged,
+                       device_policy_.policy_data().device_id(),
+                       _))
+      .WillOnce(SaveArg<6>(&request));
+  // This will be called twice, because the ComponentCloudPolicyService will
+  // also become ready after flushing all the pending tasks.
+  EXPECT_CALL(service_observer_, OnPolicyUpdated(account_1_user_id_)).Times(2);
+  FlushDeviceSettings();
+
+  Mock::VerifyAndClearExpectations(&service_observer_);
+  Mock::VerifyAndClearExpectations(&mock_device_management_service_);
+
+  EXPECT_TRUE(request.has_policy_request());
+  ASSERT_EQ(2, request.policy_request().request_size());
+
+  const em::PolicyFetchRequest* public_account =
+      &request.policy_request().request(0);
+  const em::PolicyFetchRequest* extensions =
+      &request.policy_request().request(1);
+  // The order is not guarateed.
+  if (extensions->policy_type() ==
+      dm_protocol::kChromePublicAccountPolicyType) {
+    std::swap(public_account, extensions);
+  }
+
+  EXPECT_EQ(dm_protocol::kChromePublicAccountPolicyType,
+            public_account->policy_type());
+  EXPECT_FALSE(public_account->has_machine_id());
+  EXPECT_EQ(kAccount1, public_account->settings_entity_id());
+
+  EXPECT_EQ(dm_protocol::kChromeExtensionPolicyType,
+            extensions->policy_type());
+  EXPECT_FALSE(extensions->has_machine_id());
+  EXPECT_FALSE(extensions->has_settings_entity_id());
+}
+
 TEST_F(DeviceLocalAccountPolicyServiceTest, NoAccounts) {
   EXPECT_FALSE(service_->GetBrokerForUser(account_1_user_id_));
 }
@@ -393,50 +469,7 @@ TEST_F(DeviceLocalAccountPolicyServiceTest, FetchPolicy) {
   service_->Connect(&mock_device_management_service_);
   EXPECT_TRUE(broker->core()->client());
 
-  em::DeviceManagementRequest request;
-  em::DeviceManagementResponse response;
-  response.mutable_policy_response()->add_response()->CopyFrom(
-      device_local_account_policy_.policy());
-  EXPECT_CALL(mock_device_management_service_,
-              CreateJob(DeviceManagementRequestJob::TYPE_POLICY_FETCH, _))
-      .WillOnce(mock_device_management_service_.SucceedJob(response));
-  EXPECT_CALL(mock_device_management_service_,
-              StartJob(dm_protocol::kValueRequestPolicy,
-                       std::string(), std::string(),
-                       device_policy_.policy_data().request_token(),
-                       dm_protocol::kValueUserAffiliationManaged,
-                       device_policy_.policy_data().device_id(),
-                       _))
-      .WillOnce(SaveArg<6>(&request));
-  // This will be called twice, because the ComponentCloudPolicyService will
-  // also become ready after flushing all the pending tasks.
-  EXPECT_CALL(service_observer_, OnPolicyUpdated(account_1_user_id_)).Times(2);
-  broker->core()->client()->FetchPolicy();

[pneubeck (no reviews), 2015/01/30 14:23:58]

you dropped this FetchPolicy call during the refactoring. Intentionally?

[bartfab (slow), 2015/02/03 18:31:06]

Yes, this was intentional. We should test that the broker initiates a policy
fetch at the correct places, instead of requesting fetches on its behalf.

-  FlushDeviceSettings();
-  Mock::VerifyAndClearExpectations(&service_observer_);
-  Mock::VerifyAndClearExpectations(&mock_device_management_service_);
-  EXPECT_TRUE(request.has_policy_request());
-  ASSERT_EQ(2, request.policy_request().request_size());
-
-  const em::PolicyFetchRequest* public_account =
-      &request.policy_request().request(0);
-  const em::PolicyFetchRequest* extensions =
-      &request.policy_request().request(1);
-  // The order is not guarateed.
-  if (extensions->policy_type() ==
-      dm_protocol::kChromePublicAccountPolicyType) {
-    std::swap(public_account, extensions);
-  }
-
-  EXPECT_EQ(dm_protocol::kChromePublicAccountPolicyType,
-            public_account->policy_type());
-  EXPECT_FALSE(public_account->has_machine_id());
-  EXPECT_EQ(kAccount1, public_account->settings_entity_id());
-
-  EXPECT_EQ(dm_protocol::kChromeExtensionPolicyType,
-            extensions->policy_type());
-  EXPECT_FALSE(extensions->has_machine_id());
-  EXPECT_FALSE(extensions->has_settings_entity_id());
+  FlushDeviceLocalAccountPolicyFetch();
 
   ASSERT_TRUE(broker->core()->store());
   EXPECT_EQ(CloudPolicyStore::STATUS_OK,
@@ -488,6 +521,139 @@ TEST_F(DeviceLocalAccountPolicyServiceTest, RefreshPolicy) {
   EXPECT_TRUE(service_->IsPolicyAvailableForUser(account_1_user_id_));
 }
 
+TEST_F(DeviceLocalAccountPolicyServiceTest, Invalidation) {
+  device_local_account_policy_.policy_data().set_invalidation_source(
+      kInvalidationSource);
+  device_local_account_policy_.policy_data().set_invalidation_name(
+      "invalidation");
+  InstallDeviceLocalAccountPolicy(kAccount1);
+
+  AddDeviceLocalAccountToPolicy(kAccount1);
+  EXPECT_CALL(service_observer_, OnPolicyUpdated(account_1_user_id_));
+  InstallDevicePolicy();
+  Mock::VerifyAndClearExpectations(&service_observer_);
+
+  DeviceLocalAccountPolicyBroker* broker =
+      service_->GetBrokerForUser(account_1_user_id_);
+  ASSERT_TRUE(broker);
+  EXPECT_EQ(account_1_user_id_, broker->user_id());

[pneubeck (no reviews), 2015/01/30 14:23:58]

this seems to be tested already in TEST_F(..., GetBroker), please don't
redundantly test this here or in ConnectBeforeInvalidationServiceAvailable

[bartfab (slow), 2015/02/03 18:31:06]

Obsolete.

+  ASSERT_TRUE(broker->core()->store());
+
+  // Make a first invalidation service available.
+  invalidation::FakeInvalidationService invalidation_service_1;
+  broker->OnInvalidationServiceSet(&invalidation_service_1);
+
+  // Verify that no invalidator exists initially.
+  EXPECT_FALSE(broker->GetInvalidatorForTest());
+
+  service_->Connect(&mock_device_management_service_);
+
+  // Verify that an invalidator backed by the first invalidation service has
+  // been created and its highest handled invalidation version starts out as 0.
+  CloudPolicyInvalidator* invalidator = broker->GetInvalidatorForTest();
+  ASSERT_TRUE(invalidator);
+  EXPECT_EQ(0, invalidator->highest_handled_invalidation_version());
+  EXPECT_EQ(&invalidation_service_1,
+            invalidator->invalidation_service_for_test());
+
+  // Trigger an invalidation. The invalidation version is interpreted as a
+  // timestamp in microseconds. The policy blob contains a timestamp in
+  // milliseconds. Convert from one to the other by multiplying by 1000.
+  const int64 invalidation_version =
+      broker->core()->store()->policy()->timestamp() * 1000;
+  syncer::Invalidation invalidation = syncer::Invalidation::Init(
+      invalidation::ObjectId(kInvalidationSource, kInvalidationName),
+      invalidation_version,
+      "dummy payload");
+  syncer::ObjectIdInvalidationMap invalidation_map;
+  invalidation_map.Insert(invalidation);
+  invalidator->OnIncomingInvalidation(invalidation_map);
+
+  // Verify that the invalidation causes a policy refresh.
+  FlushDeviceLocalAccountPolicyFetch();
+
+  // Verify that the highest handled invalidation version has been recorded.
+  EXPECT_EQ(invalidation_version,
+            invalidator->highest_handled_invalidation_version());
+
+  // Make the first invalidation service unavailable.
+  broker->OnInvalidationServiceSet(nullptr);
+
+  // Verify that the invalidator has been destroyed.
+  EXPECT_FALSE(broker->GetInvalidatorForTest());
+
+  // Make a second invalidation service available instead.
+  invalidation::FakeInvalidationService invalidation_service_2;
+  broker->OnInvalidationServiceSet(&invalidation_service_2);
+
+  // Verify that an invalidator backed by the second invalidation service has
+  // been created and its highest handled invalidation version starts out as the
+  // highest invalidation version handled by the previous invalidator.
+  invalidator = broker->GetInvalidatorForTest();
+  ASSERT_TRUE(invalidator);
+  EXPECT_EQ(invalidation_version,
+            invalidator->highest_handled_invalidation_version());
+  EXPECT_EQ(&invalidation_service_2,
+            invalidator->invalidation_service_for_test());
+
+  // Make the first invalidation service available again. This implies that the
+  // second invalidation service is no longer available.
+  broker->OnInvalidationServiceSet(&invalidation_service_1);
+
+  // Verify that the invalidator backed by the second invalidation service was
+  // destroyed and an invalidation backed by the first invalidation service has
+  // been created instead. Also verify that its highest handled invalidation
+  // version starts out as 1.
+  invalidator = broker->GetInvalidatorForTest();
+  ASSERT_TRUE(invalidator);
+  EXPECT_EQ(invalidation_version,
+            invalidator->highest_handled_invalidation_version());
+  EXPECT_EQ(&invalidation_service_1,
+            invalidator->invalidation_service_for_test());
+
+  // Make the first invalidation service unavailable.
+  broker->OnInvalidationServiceSet(nullptr);
+}
+
+TEST_F(DeviceLocalAccountPolicyServiceTest,

[pneubeck (no reviews), 2015/01/30 14:23:58]

If I checked it correctly, by parameterizing DeviceCloudPolicyInvalidator with a
CloudPolicyCore* and DeviceRegisterRequest_Type, you could reuse it and the
lines 579 - 615 as well as this whole test wouldn't be necessary (or could be
moved to DeviceCloudPolicyInvalidator's unittest).

also most of the implementation might not be necessary anymore.
the IsConnected check will probably not hurt the current usage of
DeviceCloudPolicyInvalidator and could just be added there.

you know better than me how much code reduction that will be, so in the end it's
your call.

[bartfab (slow), 2015/02/03 18:31:06]

Done. After making this change, I renamed DeviceCloudPolicyInvalidator to
AffiliatedCloudPolicyInvalidator. All testing code moved to a
AffiliatedCloudPolicyInvalidator unit test. The Broker now simply owns a
AffiliatedCloudPolicyInvalidator instead of reimplementing its functionality.

+       ConnectBeforeInvalidationServiceAvailable) {
+  device_local_account_policy_.policy_data().set_invalidation_source(123);
+  device_local_account_policy_.policy_data().set_invalidation_name(
+      kInvalidationName);
+  InstallDeviceLocalAccountPolicy(kAccount1);
+
+  AddDeviceLocalAccountToPolicy(kAccount1);
+  EXPECT_CALL(service_observer_, OnPolicyUpdated(account_1_user_id_));
+  InstallDevicePolicy();
+  Mock::VerifyAndClearExpectations(&service_observer_);
+
+  DeviceLocalAccountPolicyBroker* broker =
+      service_->GetBrokerForUser(account_1_user_id_);
+  ASSERT_TRUE(broker);
+  EXPECT_EQ(account_1_user_id_, broker->user_id());

[pneubeck (no reviews), 2015/01/30 14:23:58]

same as above, please remove

[bartfab (slow), 2015/02/03 18:31:06]

Obsolete.

+  ASSERT_TRUE(broker->core()->store());
+
+  service_->Connect(&mock_device_management_service_);
+
+  // Verify that no invalidator exists initially.
+  EXPECT_FALSE(broker->GetInvalidatorForTest());
+
+  // Make an invalidation service available.
+  invalidation::FakeInvalidationService invalidation_service;
+  broker->OnInvalidationServiceSet(&invalidation_service);
+
+  // Verify that an invalidator backed by the invalidation service has been
+  // created and its highest handled invalidation version starts out as 0.
+  CloudPolicyInvalidator* invalidator = broker->GetInvalidatorForTest();
+  ASSERT_TRUE(invalidator);
+  EXPECT_EQ(0, invalidator->highest_handled_invalidation_version());
+  EXPECT_EQ(&invalidation_service,
+            invalidator->invalidation_service_for_test());
+
+  // Make the invalidation service unavailable.
+  broker->OnInvalidationServiceSet(nullptr);
+}
+
 class DeviceLocalAccountPolicyExtensionCacheTest
     : public DeviceLocalAccountPolicyServiceTestBase {
  protected:
@@ -784,15 +950,16 @@ class DeviceLocalAccountPolicyProviderTest
 };
 
 DeviceLocalAccountPolicyProviderTest::DeviceLocalAccountPolicyProviderTest() {
+}
+
+void DeviceLocalAccountPolicyProviderTest::SetUp() {
+  DeviceLocalAccountPolicyServiceTestBase::SetUp();
+
   CreatePolicyService();
   provider_ = DeviceLocalAccountPolicyProvider::Create(
       GenerateDeviceLocalAccountUserId(kAccount1,
                                        DeviceLocalAccount::TYPE_PUBLIC_SESSION),
       service_.get());
-}
-
-void DeviceLocalAccountPolicyProviderTest::SetUp() {
-  DeviceLocalAccountPolicyServiceTestBase::SetUp();
   provider_->Init(&schema_registry_);
   provider_->AddObserver(&provider_observer_);