Support remote commands over FCM

RemoteCommandsInvalidator and CloudPolicyInvalidator both use the same
InvalidationService instance, so migrate them together.

When fcm is enabled use command_invalidation_topic as topic to subscribe
to invalidations, and command_invalidation_name otherwise.

Bug: 939039
Change-Id: I4b77c60c0928dc3aa54a9e306182e2848354e1ec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1658790Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Ivan Šandrk <isandrk@chromium.org>
Commit-Queue: Askar Aitzhan <askaraitzhan@google.com>
Cr-Commit-Position: refs/heads/master@{#671649}

chrome/browser/BUILD.gn

@@ -1161,6 +1161,8 @@ jumbo_split_static_library("browser") {
     "policy/chrome_browser_policy_connector.h",
     "policy/cloud/cloud_policy_invalidator.cc",
     "policy/cloud/cloud_policy_invalidator.h",
+    "policy/cloud/policy_invalidation_util.cc",
+    "policy/cloud/policy_invalidation_util.h",
     "policy/cloud/remote_commands_invalidator.cc",
     "policy/cloud/remote_commands_invalidator.h",
     "policy/cloud/remote_commands_invalidator_impl.cc",

chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc

@@ -30,6 +30,7 @@
 #include "chrome/browser/chromeos/policy/remote_commands/user_commands_factory_chromeos.h"
 #include "chrome/browser/chromeos/policy/wildcard_login_checker.h"
 #include "chrome/browser/invalidation/deprecated_profile_invalidation_provider_factory.h"
+#include "chrome/browser/invalidation/profile_invalidation_provider_factory.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
 #include "chrome/browser/net/system_network_context_manager.h"
 #include "chrome/browser/policy/cloud/remote_commands_invalidator_impl.h"
@@ -121,6 +122,11 @@ class UserCloudPolicyManagerChromeOSNotifierFactory
   UserCloudPolicyManagerChromeOSNotifierFactory()
       : BrowserContextKeyedServiceShutdownNotifierFactory(
             "UserRemoteCommandsInvalidator") {
+    if (base::FeatureList::IsEnabled(features::kPolicyFcmInvalidations)) {
+      DependsOn(
+          invalidation::ProfileInvalidationProviderFactory::GetInstance());
+      return;
+    }
     DependsOn(invalidation::DeprecatedProfileInvalidationProviderFactory::
                   GetInstance());
   }
@@ -740,9 +746,17 @@ void UserCloudPolicyManagerChromeOS::Observe(
   registrar_.Remove(this, chrome::NOTIFICATION_PROFILE_ADDED,
                     content::Source<Profile>(profile_));
 
+  // If true FCMInvalidationService will be used as invalidation service and
+  // TiclInvalidationService otherwise.
+  const bool is_fcm_enabled =
+      base::FeatureList::IsEnabled(features::kPolicyFcmInvalidations);
+
   invalidation::ProfileInvalidationProvider* const invalidation_provider =
-      invalidation::DeprecatedProfileInvalidationProviderFactory::GetForProfile(
-          profile_);
+      is_fcm_enabled
+          ? invalidation::ProfileInvalidationProviderFactory::GetForProfile(
+                profile_)
+          : invalidation::DeprecatedProfileInvalidationProviderFactory::
+                GetForProfile(profile_);
 
   if (!invalidation_provider)
     return;
@@ -750,7 +764,12 @@ void UserCloudPolicyManagerChromeOS::Observe(
   core()->StartRemoteCommandsService(
       std::make_unique<UserCommandsFactoryChromeOS>(profile_));
   invalidator_ = std::make_unique<RemoteCommandsInvalidatorImpl>(core());
-  invalidator_->Initialize(invalidation_provider->GetInvalidationService());
+
+  invalidator_->Initialize(
+      is_fcm_enabled
+          ? invalidation_provider->GetInvalidationServiceForCustomSender(
+                policy::kPolicyFCMInvalidationSenderID)
+          : invalidation_provider->GetInvalidationService());
 
   shutdown_notifier_ =
       UserCloudPolicyManagerChromeOSNotifierFactory::GetInstance()

chrome/browser/policy/cloud/cloud_policy_invalidator.cc

@@ -16,6 +16,7 @@
 #include "base/time/clock.h"
 #include "base/time/time.h"
 #include "base/values.h"
+#include "chrome/browser/policy/cloud/policy_invalidation_util.h"
 #include "chrome/common/chrome_features.h"
 #include "components/invalidation/public/invalidation_service.h"
 #include "components/invalidation/public/invalidation_util.h"
@@ -27,11 +28,6 @@
 
 namespace policy {
 
-namespace {
-
-constexpr char kFcmPolicyPublicTopicPrefix[] = "cs-";
-}  // namespace
-
 const int CloudPolicyInvalidator::kMissingPayloadDelay = 5;
 const int CloudPolicyInvalidator::kMaxFetchDelayDefault = 10000;
 const int CloudPolicyInvalidator::kMaxFetchDelayMin = 1000;
@@ -140,7 +136,7 @@ void CloudPolicyInvalidator::OnIncomingInvalidation(
 std::string CloudPolicyInvalidator::GetOwnerName() const { return "Cloud"; }
 
 bool CloudPolicyInvalidator::IsPublicTopic(const syncer::Topic& topic) const {
-  return base::StringPiece(topic).starts_with(kFcmPolicyPublicTopicPrefix);
+  return IsPublicInvalidationTopic(topic);
 }
 
 void CloudPolicyInvalidator::OnCoreConnected(CloudPolicyCore* core) {}
@@ -286,30 +282,11 @@ void CloudPolicyInvalidator::HandleInvalidation(
 void CloudPolicyInvalidator::UpdateRegistration(
     const enterprise_management::PolicyData* policy) {
   // Create the ObjectId based on the policy data.
-  if (!policy) {
-    Unregister();
-    return;
-  }
-
   // If the policy does not specify an ObjectId, then unregister.
   invalidation::ObjectId object_id;
-  if (base::FeatureList::IsEnabled(features::kPolicyFcmInvalidations)) {
-    if (!policy->has_policy_invalidation_topic() ||
-        policy->policy_invalidation_topic().empty()) {
-      Unregister();
-      return;
-    }
-    object_id = invalidation::ObjectId(syncer::kDeprecatedSourceForFCM,
-                                       policy->policy_invalidation_topic());
-  } else {
-    if (!policy->has_invalidation_source() ||
-        !policy->has_invalidation_name() ||
-        policy->invalidation_name().empty()) {
-      Unregister();
-      return;
-    }
-    object_id = invalidation::ObjectId(policy->invalidation_source(),
-                                       policy->invalidation_name());
+  if (!policy || !GetCloudPolicyObjectIdFromPolicy(*policy, &object_id)) {
+    Unregister();
+    return;
   }
 
   // If the policy object id in the policy data is different from the currently

chrome/browser/policy/cloud/policy_invalidation_util.cc

+// Copyright 2019 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/policy/cloud/policy_invalidation_util.h"
+
+#include "base/feature_list.h"
+#include "base/strings/string_piece.h"
+#include "chrome/common/chrome_features.h"
+#include "components/invalidation/public/invalidation.h"
+#include "components/policy/proto/device_management_backend.pb.h"
+#include "google/cacheinvalidation/include/types.h"
+
+namespace policy {
+
+namespace {
+
+constexpr char kFcmPolicyPublicTopicPrefix[] = "cs-";
+
+}  // namespace
+
+bool IsPublicInvalidationTopic(const syncer::Topic& topic) {
+  return base::StringPiece(topic).starts_with(kFcmPolicyPublicTopicPrefix);
+}
+
+bool GetCloudPolicyObjectIdFromPolicy(
+    const enterprise_management::PolicyData& policy,
+    invalidation::ObjectId* object_id) {
+  if (base::FeatureList::IsEnabled(features::kPolicyFcmInvalidations)) {
+    if (!policy.has_policy_invalidation_topic() ||
+        policy.policy_invalidation_topic().empty()) {
+      return false;
+    }
+    *object_id = invalidation::ObjectId(syncer::kDeprecatedSourceForFCM,
+                                        policy.policy_invalidation_topic());
+  } else {
+    if (!policy.has_invalidation_source() || !policy.has_invalidation_name() ||
+        policy.invalidation_name().empty()) {
+      return false;
+    }
+    *object_id = invalidation::ObjectId(policy.invalidation_source(),
+                                        policy.invalidation_name());
+  }
+  return true;
+}
+
+bool GetRemoteCommandObjectIdFromPolicy(
+    const enterprise_management::PolicyData& policy,
+    invalidation::ObjectId* object_id) {
+  if (base::FeatureList::IsEnabled(features::kPolicyFcmInvalidations)) {
+    if (!policy.has_command_invalidation_topic() ||
+        policy.command_invalidation_topic().empty()) {
+      return false;
+    }
+    *object_id = invalidation::ObjectId(syncer::kDeprecatedSourceForFCM,
+                                        policy.command_invalidation_topic());
+  } else {
+    if (!policy.has_command_invalidation_source() ||
+        !policy.has_command_invalidation_name() ||
+        policy.command_invalidation_name().empty()) {
+      return false;
+    }
+    *object_id = invalidation::ObjectId(policy.command_invalidation_source(),
+                                        policy.command_invalidation_name());
+  }
+  return true;
+}
+
+}  // namespace policy

chrome/browser/policy/cloud/policy_invalidation_util.h

+// Copyright 2019 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_POLICY_CLOUD_POLICY_INVALIDATION_UTIL_H_
+#define CHROME_BROWSER_POLICY_CLOUD_POLICY_INVALIDATION_UTIL_H_
+
+#include "components/invalidation/public/invalidation_util.h"
+
+namespace enterprise_management {
+
+class PolicyData;
+
+}  // namespace enterprise_management
+
+namespace invalidation {
+
+class ObjectId;
+
+}  // namespace invalidation
+
+namespace policy {
+
+// Returns true if |topic| is a public topic. If |topic| is public,
+// publish/subscribe service will fan out all the outgoing messages to all the
+// devices subscribed to this topic.
+// For example:
+// If device subscribes to "DeviceGuestModeEnabled" public topic all the
+// instances subscribed to this topic will receive all the outgoing messages
+// addressed to topic "DeviceGuestModeEnabled". But if 2 devices with different
+// InstanceID subscribe to private topic "BOOKMARK", they will receive different
+// set of messages addressed to pair ("BOOKMARK", InstanceID) respectievely.
+bool IsPublicInvalidationTopic(const syncer::Topic& topic);
+
+// Returns true if |policy| has data about policy to invalidate, and saves
+// that data in |object_id|, and false otherwise.
+bool GetCloudPolicyObjectIdFromPolicy(
+    const enterprise_management::PolicyData& policy,
+    invalidation::ObjectId* object_id);
+
+// The same as GetCloudPolicyObjectIdFromPolicy but gets the |object_id| for
+// remote command.
+bool GetRemoteCommandObjectIdFromPolicy(
+    const enterprise_management::PolicyData& policy,
+    invalidation::ObjectId* object_id);
+
+}  // namespace policy
+
+#endif  // CHROME_BROWSER_POLICY_CLOUD_POLICY_INVALIDATION_UTIL_H_

chrome/browser/policy/cloud/remote_commands_invalidator.cc

@@ -6,8 +6,11 @@
 
 #include <string>
 
+#include "base/feature_list.h"
 #include "base/logging.h"
 #include "base/syslog_logging.h"
+#include "chrome/browser/policy/cloud/policy_invalidation_util.h"
+#include "chrome/common/chrome_features.h"
 #include "components/invalidation/public/invalidation.h"
 #include "components/invalidation/public/invalidation_service.h"
 #include "components/invalidation/public/invalidation_util.h"
@@ -17,8 +20,7 @@
 
 namespace policy {
 
-RemoteCommandsInvalidator::RemoteCommandsInvalidator() {
-}
+RemoteCommandsInvalidator::RemoteCommandsInvalidator() {}
 
 RemoteCommandsInvalidator::~RemoteCommandsInvalidator() {
   DCHECK_EQ(SHUT_DOWN, state_);
@@ -116,6 +118,11 @@ std::string RemoteCommandsInvalidator::GetOwnerName() const {
   return "RemoteCommands";
 }
 
+bool RemoteCommandsInvalidator::IsPublicTopic(
+    const syncer::Topic& topic) const {
+  return IsPublicInvalidationTopic(topic);
+}
+
 void RemoteCommandsInvalidator::ReloadPolicyData(
     const enterprise_management::PolicyData* policy) {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ -127,13 +134,11 @@ void RemoteCommandsInvalidator::ReloadPolicyData(
 
   // Create the ObjectId based on the policy data.
   // If the policy does not specify an the ObjectId, then unregister.
-  if (!policy || !policy->has_command_invalidation_source() ||
-      !policy->has_command_invalidation_name()) {
+  invalidation::ObjectId object_id;
+  if (!policy || !GetRemoteCommandObjectIdFromPolicy(*policy, &object_id)) {
     Unregister();
     return;
   }
-  const invalidation::ObjectId object_id(policy->command_invalidation_source(),
-                                         policy->command_invalidation_name());
 
   // If the policy object id in the policy data is different from the currently
   // registered object id, update the object registration.

chrome/browser/policy/cloud/remote_commands_invalidator.h

@@ -46,6 +46,8 @@ class RemoteCommandsInvalidator : public syncer::InvalidationHandler {
   invalidation::InvalidationService* invalidation_service() {
     return invalidation_service_;
   }
+
+  // Whether the invalidator currently has the ability to receive invalidations.
   bool invalidations_enabled() { return invalidations_enabled_; }
 
   // syncer::InvalidationHandler:
@@ -53,6 +55,7 @@ class RemoteCommandsInvalidator : public syncer::InvalidationHandler {
   void OnIncomingInvalidation(
       const syncer::ObjectIdInvalidationMap& invalidation_map) override;
   std::string GetOwnerName() const override;
+  bool IsPublicTopic(const syncer::Topic& topic) const override;
 
  protected:
   virtual void OnInitialize() = 0;

chrome/browser/policy/cloud/remote_commands_invalidator_unittest.cc

@@ -6,7 +6,9 @@
 
 #include "base/macros.h"
 #include "base/run_loop.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/test/scoped_task_environment.h"
+#include "chrome/common/chrome_features.h"
 #include "components/invalidation/impl/deprecated_invalidator_registrar.h"
 #include "components/invalidation/impl/fake_invalidation_service.h"
 #include "components/invalidation/impl/mock_ack_handler.h"
@@ -33,10 +35,15 @@ class MockRemoteCommandInvalidator : public RemoteCommandsInvalidator {
   MOCK_METHOD0(OnStop, void());
   MOCK_METHOD0(DoRemoteCommandsFetch, void());
 
-  void SetInvalidationObjectID(const invalidation::ObjectId& object_id) {
+  void SetInvalidationObjectID(const invalidation::ObjectId& object_id,
+                               bool is_fcm_enabled) {
     em::PolicyData policy_data;
-    policy_data.set_command_invalidation_source(object_id.source());
-    policy_data.set_command_invalidation_name(object_id.name());
+    if (is_fcm_enabled) {
+      policy_data.set_command_invalidation_topic(object_id.name());
+    } else {
+      policy_data.set_command_invalidation_source(object_id.source());
+      policy_data.set_command_invalidation_name(object_id.name());
+    }
     ReloadPolicyData(&policy_data);
   }
 
@@ -49,11 +56,21 @@ class MockRemoteCommandInvalidator : public RemoteCommandsInvalidator {
   DISALLOW_COPY_AND_ASSIGN(MockRemoteCommandInvalidator);
 };
 
-class RemoteCommandsInvalidatorTest : public testing::Test {
+class RemoteCommandsInvalidatorTest : public testing::TestWithParam<bool> {
  public:
-  RemoteCommandsInvalidatorTest()
-      : kTestingObjectId1(123456, "abcdef"),
-        kTestingObjectId2(654321, "defabc") {
+  RemoteCommandsInvalidatorTest() : is_fcm_enabled_(GetParam()) {
+    feature_list_.InitWithFeatureState(features::kPolicyFcmInvalidations,
+                                       is_fcm_enabled_);
+
+    if (is_fcm_enabled_) {
+      kTestingObjectId1 =
+          invalidation::ObjectId(syncer::kDeprecatedSourceForFCM, "abcdef");
+      kTestingObjectId2 =
+          invalidation::ObjectId(syncer::kDeprecatedSourceForFCM, "defabc");
+      return;
+    }
+    kTestingObjectId1 = invalidation::ObjectId(123456, "abcdef");
+    kTestingObjectId2 = invalidation::ObjectId(654321, "defabc");
   }
 
   void EnableInvalidationService() {
@@ -135,11 +152,15 @@ class RemoteCommandsInvalidatorTest : public testing::Test {
     VerifyExpectations();
   }
 
-  const invalidation::ObjectId kTestingObjectId1;
-  const invalidation::ObjectId kTestingObjectId2;
+  invalidation::ObjectId kTestingObjectId1;
+  invalidation::ObjectId kTestingObjectId2;
+
+  const bool is_fcm_enabled_;
 
   base::test::ScopedTaskEnvironment scoped_task_environment_;
 
+  base::test::ScopedFeatureList feature_list_;
+
   invalidation::FakeInvalidationService invalidation_service_;
   StrictMock<MockRemoteCommandInvalidator> invalidator_;
 
@@ -148,14 +169,14 @@ class RemoteCommandsInvalidatorTest : public testing::Test {
 };
 
 // Verifies that only the fired invalidations will be received.
-TEST_F(RemoteCommandsInvalidatorTest, FiredInvalidation) {
+TEST_P(RemoteCommandsInvalidatorTest, FiredInvalidation) {
   InitializeAndStart();
 
   // Invalidator won't work at this point.
   EXPECT_FALSE(invalidator_.invalidations_enabled());
 
   // Load the policy data, it should work now.
-  invalidator_.SetInvalidationObjectID(kTestingObjectId1);
+  invalidator_.SetInvalidationObjectID(kTestingObjectId1, is_fcm_enabled_);
   EXPECT_TRUE(invalidator_.invalidations_enabled());
 
   base::RunLoop().RunUntilIdle();
@@ -186,7 +207,7 @@ TEST_F(RemoteCommandsInvalidatorTest, FiredInvalidation) {
 }
 
 // Verifies that no invalidation will be received when invalidator is shutdown.
-TEST_F(RemoteCommandsInvalidatorTest, ShutDown) {
+TEST_P(RemoteCommandsInvalidatorTest, ShutDown) {
   EXPECT_FALSE(invalidator_.invalidations_enabled());
   FireInvalidation(kTestingObjectId1);
 
@@ -195,7 +216,7 @@ TEST_F(RemoteCommandsInvalidatorTest, ShutDown) {
 }
 
 // Verifies that no invalidation will be received when invalidator is stopped.
-TEST_F(RemoteCommandsInvalidatorTest, Stopped) {
+TEST_P(RemoteCommandsInvalidatorTest, Stopped) {
   EXPECT_CALL(invalidator_, OnInitialize()).Times(1);
   invalidator_.Initialize(&invalidation_service_);
   VerifyExpectations();
@@ -211,13 +232,13 @@ TEST_F(RemoteCommandsInvalidatorTest, Stopped) {
 }
 
 // Verifies that stated/stopped state changes work as expected.
-TEST_F(RemoteCommandsInvalidatorTest, StartedStateChange) {
+TEST_P(RemoteCommandsInvalidatorTest, StartedStateChange) {
   InitializeAndStart();
 
   // Invalidator requires object id to work.
   VerifyInvalidationDisabled(kTestingObjectId1);
   EXPECT_FALSE(invalidator_.invalidations_enabled());
-  invalidator_.SetInvalidationObjectID(kTestingObjectId1);
+  invalidator_.SetInvalidationObjectID(kTestingObjectId1, is_fcm_enabled_);
   VerifyInvalidationEnabled(kTestingObjectId1);
 
   // Stop and restart invalidator.
@@ -233,24 +254,24 @@ TEST_F(RemoteCommandsInvalidatorTest, StartedStateChange) {
   VerifyExpectations();
 
   // Invalidator requires object id to work.
-  invalidator_.SetInvalidationObjectID(kTestingObjectId1);
+  invalidator_.SetInvalidationObjectID(kTestingObjectId1, is_fcm_enabled_);
   VerifyInvalidationEnabled(kTestingObjectId1);
 
   StopAndShutdown();
 }
 
 // Verifies that registered state changes work as expected.
-TEST_F(RemoteCommandsInvalidatorTest, RegistedStateChange) {
+TEST_P(RemoteCommandsInvalidatorTest, RegistedStateChange) {
   InitializeAndStart();
 
-  invalidator_.SetInvalidationObjectID(kTestingObjectId1);
+  invalidator_.SetInvalidationObjectID(kTestingObjectId1, is_fcm_enabled_);
   VerifyInvalidationEnabled(kTestingObjectId1);
 
-  invalidator_.SetInvalidationObjectID(kTestingObjectId2);
+  invalidator_.SetInvalidationObjectID(kTestingObjectId2, is_fcm_enabled_);
   VerifyInvalidationEnabled(kTestingObjectId2);
   VerifyInvalidationDisabled(kTestingObjectId1);
 
-  invalidator_.SetInvalidationObjectID(kTestingObjectId1);
+  invalidator_.SetInvalidationObjectID(kTestingObjectId1, is_fcm_enabled_);
   VerifyInvalidationEnabled(kTestingObjectId1);
   VerifyInvalidationDisabled(kTestingObjectId2);
 
@@ -259,7 +280,7 @@ TEST_F(RemoteCommandsInvalidatorTest, RegistedStateChange) {
   VerifyInvalidationDisabled(kTestingObjectId2);
   EXPECT_FALSE(invalidator_.invalidations_enabled());
 
-  invalidator_.SetInvalidationObjectID(kTestingObjectId2);
+  invalidator_.SetInvalidationObjectID(kTestingObjectId2, is_fcm_enabled_);
   VerifyInvalidationEnabled(kTestingObjectId2);
   VerifyInvalidationDisabled(kTestingObjectId1);
 
@@ -267,10 +288,10 @@ TEST_F(RemoteCommandsInvalidatorTest, RegistedStateChange) {
 }
 
 // Verifies that invalidation service enabled state changes work as expected.
-TEST_F(RemoteCommandsInvalidatorTest, InvalidationServiceEnabledStateChanged) {
+TEST_P(RemoteCommandsInvalidatorTest, InvalidationServiceEnabledStateChanged) {
   InitializeAndStart();
 
-  invalidator_.SetInvalidationObjectID(kTestingObjectId1);
+  invalidator_.SetInvalidationObjectID(kTestingObjectId1, is_fcm_enabled_);
   VerifyInvalidationEnabled(kTestingObjectId1);
 
   DisableInvalidationService();
@@ -291,4 +312,8 @@ TEST_F(RemoteCommandsInvalidatorTest, InvalidationServiceEnabledStateChanged) {
   StopAndShutdown();
 }
 
+INSTANTIATE_TEST_SUITE_P(ToggleIsFcmEnabled,
+                         RemoteCommandsInvalidatorTest,
+                         testing::Bool() /* is_fcm_enabled */);
+
 }  // namespace policy