chromeos: Make supervised user sync types run in sync transport mode

Supervised users (including child accounts) use the sync subsystem to
load settings and URL allowlists from Google backends. Change these
data types to use sync transport mode because we want them to run
even if browser sync-the-feature is disabled.

Bug: 1043755
Test: Added to unit_tests and components_unittests
Test: Sign in child account and manually verify the types are running in chrome://sync-internals

Change-Id: Ic681adab2fd3101ec326b99ed3aee085f0597a10
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2142132
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Aga Wronska <agawronska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#758814}

chrome/browser/supervised_user/supervised_user_model_type_controller_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/supervised_user/supervised_user_sync_model_type_controller.h"
+
+#include "base/bind_helpers.h"
+#include "chrome/browser/supervised_user/supervised_user_constants.h"
+#include "chrome/test/base/testing_profile.h"
+#include "components/sync/base/model_type.h"
+#include "components/sync/driver/data_type_controller.h"
+#include "content/public/test/browser_task_environment.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using syncer::DataTypeController;
+
+class SupervisedUserSyncModelTypeControllerTest : public testing::Test {
+ private:
+  content::BrowserTaskEnvironment task_environment_;
+};
+
+TEST_F(SupervisedUserSyncModelTypeControllerTest,
+       SupervisedUserMeetsPreconditions) {
+  TestingProfile::Builder builder;
+  builder.SetSupervisedUserId(supervised_users::kChildAccountSUID);
+  std::unique_ptr<Profile> child_profile = builder.Build();
+  ASSERT_TRUE(child_profile->IsSupervised());
+
+  SupervisedUserSyncModelTypeController controller(
+      syncer::SUPERVISED_USER_SETTINGS, child_profile.get(),
+      /*dump_stack=*/base::DoNothing(),
+      /*store_factory=*/base::DoNothing(),
+      /*syncable_service=*/nullptr);
+  EXPECT_EQ(DataTypeController::PreconditionState::kPreconditionsMet,
+            controller.GetPreconditionState());
+}
+
+TEST_F(SupervisedUserSyncModelTypeControllerTest,
+       NonSupervisedUserDoesNotMeetPreconditions) {
+  TestingProfile::Builder builder;
+  std::unique_ptr<Profile> non_child_profile = builder.Build();
+  ASSERT_FALSE(non_child_profile->IsSupervised());
+
+  SupervisedUserSyncModelTypeController controller(
+      syncer::SUPERVISED_USER_SETTINGS, non_child_profile.get(),
+      /*dump_stack=*/base::DoNothing(),
+      /*store_factory=*/base::DoNothing(),
+      /*syncable_service=*/nullptr);
+  EXPECT_EQ(DataTypeController::PreconditionState::kMustStopAndClearData,
+            controller.GetPreconditionState());
+}
+
+TEST_F(SupervisedUserSyncModelTypeControllerTest, HasTransportModeDelegate) {
+  TestingProfile::Builder builder;
+  builder.SetSupervisedUserId(supervised_users::kChildAccountSUID);
+  std::unique_ptr<Profile> child_profile = builder.Build();
+  ASSERT_TRUE(child_profile->IsSupervised());
+
+  SupervisedUserSyncModelTypeController controller(
+      syncer::SUPERVISED_USER_SETTINGS, child_profile.get(),
+      /*dump_stack=*/base::DoNothing(),
+      /*store_factory=*/base::DoNothing(),
+      /*syncable_service=*/nullptr);
+  EXPECT_TRUE(controller.GetDelegateForTransportModeForTest());
+}

chrome/browser/supervised_user/supervised_user_sync_model_type_controller.cc

@@ -6,26 +6,27 @@
 
 #include "base/bind.h"
 #include "chrome/browser/profiles/profile.h"
-#include "components/browser_sync/browser_sync_client.h"
 #include "components/sync/model/model_type_store_service.h"
 
 SupervisedUserSyncModelTypeController::SupervisedUserSyncModelTypeController(
     syncer::ModelType type,
     const Profile* profile,
     const base::RepeatingClosure& dump_stack,
-    browser_sync::BrowserSyncClient* sync_client)
+    syncer::OnceModelTypeStoreFactory store_factory,
+    base::WeakPtr<syncer::SyncableService> syncable_service)
     : SyncableServiceBasedModelTypeController(
           type,
-          sync_client->GetModelTypeStoreService()->GetStoreFactory(),
-          sync_client->GetSyncableServiceForType(type),
-          dump_stack),
+          std::move(store_factory),
+          syncable_service,
+          dump_stack,
+          DelegateMode::kTransportModeWithSingleModel),
       profile_(profile) {
   DCHECK(type == syncer::SUPERVISED_USER_SETTINGS ||
          type == syncer::SUPERVISED_USER_WHITELISTS);
 }
 
 SupervisedUserSyncModelTypeController::
-    ~SupervisedUserSyncModelTypeController() {}
+    ~SupervisedUserSyncModelTypeController() = default;
 
 syncer::DataTypeController::PreconditionState
 SupervisedUserSyncModelTypeController::GetPreconditionState() const {

chrome/browser/supervised_user/supervised_user_sync_model_type_controller.h

@@ -11,12 +11,9 @@
 
 class Profile;
 
-namespace browser_sync {
-class BrowserSyncClient;
-}  // namespace browser_sync
-
 // A DataTypeController for supervised user sync datatypes, which enables or
-// disables these types based on the profile's IsSupervised state.
+// disables these types based on the profile's IsSupervised state. Runs in
+// sync transport mode.
 class SupervisedUserSyncModelTypeController
     : public syncer::SyncableServiceBasedModelTypeController {
  public:
@@ -25,7 +22,8 @@ class SupervisedUserSyncModelTypeController
       syncer::ModelType type,
       const Profile* profile,
       const base::RepeatingClosure& dump_stack,
-      browser_sync::BrowserSyncClient* sync_client);
+      syncer::OnceModelTypeStoreFactory store_factory,
+      base::WeakPtr<syncer::SyncableService> syncable_service);
   ~SupervisedUserSyncModelTypeController() override;
 
   // DataTypeController override.

chrome/browser/sync/chrome_sync_client.cc

@@ -359,9 +359,13 @@ ChromeSyncClient::CreateDataTypeControllers(syncer::SyncService* sync_service) {
 
 #if BUILDFLAG(ENABLE_SUPERVISED_USERS)
   controllers.push_back(std::make_unique<SupervisedUserSyncModelTypeController>(
-      syncer::SUPERVISED_USER_SETTINGS, profile_, dump_stack, this));
+      syncer::SUPERVISED_USER_SETTINGS, profile_, dump_stack,
+      model_type_store_factory,
+      GetSyncableServiceForType(syncer::SUPERVISED_USER_SETTINGS)));
   controllers.push_back(std::make_unique<SupervisedUserSyncModelTypeController>(
-      syncer::SUPERVISED_USER_WHITELISTS, profile_, dump_stack, this));
+      syncer::SUPERVISED_USER_WHITELISTS, profile_, dump_stack,
+      model_type_store_factory,
+      GetSyncableServiceForType(syncer::SUPERVISED_USER_WHITELISTS)));
 #endif  // BUILDFLAG(ENABLE_SUPERVISED_USERS)
 
 #if BUILDFLAG(ENABLE_EXTENSIONS)

chrome/test/BUILD.gn

@@ -5425,6 +5425,7 @@ test("unit_tests") {
       "../browser/supervised_user/child_accounts/family_info_fetcher_unittest.cc",
       "../browser/supervised_user/child_accounts/permission_request_creator_apiary_unittest.cc",
       "../browser/supervised_user/kids_management_url_checker_client_unittest.cc",
+      "../browser/supervised_user/supervised_user_model_type_controller_unittest.cc",
       "../browser/supervised_user/supervised_user_pref_store_unittest.cc",
       "../browser/supervised_user/supervised_user_service_unittest.cc",
       "../browser/supervised_user/supervised_user_settings_service_unittest.cc",

components/sync/driver/model_type_controller.cc

@@ -260,6 +260,12 @@ void ModelTypeController::RecordMemoryUsageAndCountsHistograms() {
   delegate_->RecordMemoryUsageAndCountsHistograms();
 }
 
+ModelTypeControllerDelegate*
+ModelTypeController::GetDelegateForTransportModeForTest() {
+  auto it = delegate_map_.find(SyncMode::kTransportOnly);
+  return it != delegate_map_.end() ? it->second.get() : nullptr;
+}
+
 void ModelTypeController::ReportModelError(SyncError::ErrorType error_type,
                                            const ModelError& error) {
   DCHECK(CalledOnValidThread());

components/sync/driver/model_type_controller.h

@@ -61,6 +61,8 @@ class ModelTypeController : public DataTypeController {
   void GetStatusCounters(StatusCountersCallback callback) override;
   void RecordMemoryUsageAndCountsHistograms() override;
 
+  ModelTypeControllerDelegate* GetDelegateForTransportModeForTest();
+
  protected:
   // Subclasses that use this constructor must call InitModelTypeController().
   explicit ModelTypeController(ModelType type);

components/sync/driver/profile_sync_service.cc

@@ -1471,8 +1471,13 @@ void ProfileSyncService::ConfigureDataTypeManager(ConfigureReason reason) {
 }
 
 ModelTypeSet ProfileSyncService::GetModelTypesForTransportOnlyMode() const {
-  ModelTypeSet allowed_types = {USER_CONSENTS, SECURITY_EVENTS,
-                                SHARING_MESSAGE};
+  ModelTypeSet allowed_types = {
+      SECURITY_EVENTS,
+      SHARING_MESSAGE,
+      SUPERVISED_USER_SETTINGS,
+      SUPERVISED_USER_WHITELISTS,
+      USER_CONSENTS,
+  };
 
   if (autofill_enable_account_wallet_storage_) {
     if (!GetUserSettings()->IsUsingSecondaryPassphrase() ||

components/sync/driver/profile_sync_service_unittest.cc

@@ -21,6 +21,7 @@
 #include "components/signin/public/identity_manager/identity_test_environment.h"
 #include "components/signin/public/identity_manager/identity_test_utils.h"
 #include "components/signin/public/identity_manager/primary_account_mutator.h"
+#include "components/sync/base/model_type.h"
 #include "components/sync/base/pref_names.h"
 #include "components/sync/base/user_demographics.h"
 #include "components/sync/base/user_selectable_type.h"
@@ -71,6 +72,7 @@ class FakeDataTypeManager : public DataTypeManager {
   void Configure(ModelTypeSet desired_types,
                  const ConfigureContext& context) override {
     state_ = CONFIGURED;
+    desired_types_ = desired_types;
     DCHECK(!configure_called_.is_null());
     configure_called_.Run(context.reason);
   }
@@ -79,13 +81,14 @@ class FakeDataTypeManager : public DataTypeManager {
   void ResetDataTypeErrors() override {}
   void PurgeForMigration(ModelTypeSet undesired_types) override {}
   void Stop(ShutdownReason reason) override {}
-  ModelTypeSet GetActiveDataTypes() const override { return ModelTypeSet(); }
+  ModelTypeSet GetActiveDataTypes() const override { return desired_types_; }
   bool IsNigoriEnabled() const override { return true; }
   State state() const override { return state_; }
 
  private:
   ConfigureCalled configure_called_;
   State state_;
+  ModelTypeSet desired_types_;
 };
 
 ACTION_P(ReturnNewFakeDataTypeManager, configure_called) {
@@ -177,8 +180,11 @@ class ProfileSyncServiceTest : public ::testing::Test {
   void CreateService(ProfileSyncService::StartBehavior behavior) {
     DCHECK(!service_);
 
+    // Include a regular controller and a transport-mode controller.
     DataTypeController::TypeVector controllers;
     controllers.push_back(std::make_unique<FakeDataTypeController>(BOOKMARKS));
+    controllers.push_back(
+        std::make_unique<FakeDataTypeController>(SUPERVISED_USER_SETTINGS));
 
     std::unique_ptr<SyncClientMock> sync_client =
         profile_sync_service_bundle_.CreateSyncClientMock();
@@ -199,8 +205,11 @@ class ProfileSyncServiceTest : public ::testing::Test {
   void CreateServiceWithLocalSyncBackend() {
     DCHECK(!service_);
 
+    // Include a regular controller and a transport-mode controller.
     DataTypeController::TypeVector controllers;
     controllers.push_back(std::make_unique<FakeDataTypeController>(BOOKMARKS));
+    controllers.push_back(
+        std::make_unique<FakeDataTypeController>(SUPERVISED_USER_SETTINGS));
 
     std::unique_ptr<SyncClientMock> sync_client =
         profile_sync_service_bundle_.CreateSyncClientMock();
@@ -421,6 +430,27 @@ TEST_F(ProfileSyncServiceTest, NeedsConfirmation) {
   EXPECT_EQ(kLastSyncedTime, sync_prefs.GetLastSyncedTime());
 }
 
+TEST_F(ProfileSyncServiceTest, ModelTypesForTransportMode) {
+  CreateService(ProfileSyncService::AUTO_START);
+  SignIn();
+  InitializeForNthSync();
+
+  // Disable sync-the-feature.
+  service()->GetUserSettings()->SetSyncRequested(false);
+  ASSERT_FALSE(service()->IsSyncFeatureActive());
+  ASSERT_FALSE(service()->IsSyncFeatureEnabled());
+
+  // Sync-the-transport is still active.
+  ASSERT_EQ(SyncService::TransportState::ACTIVE,
+            service()->GetTransportState());
+
+  // ModelTypes for sync-the-feature are not configured.
+  EXPECT_FALSE(service()->GetActiveDataTypes().Has(BOOKMARKS));
+
+  // ModelTypes for sync-the-transport are configured.
+  EXPECT_TRUE(service()->GetActiveDataTypes().Has(SUPERVISED_USER_SETTINGS));
+}
+
 // Verify that the SetSetupInProgress function call updates state
 // and notifies observers.
 TEST_F(ProfileSyncServiceTest, SetupInProgress) {