Support SyncTypesListDisabled policy for Chrome OS sync data types

SyncTypesListDisabled allows admins to disable individual sync
"user selectable types" by name.

Extend it to work for OS types ("osApps", "osPreferences", and
"wifiConfigurations") as well. Add migration support. While we are
rolling out SplitSettingsSync the existing browser data types names
"apps" and "preferences" will control the migrated types.

Add unit tests for SyncTypesListDisabled to SyncPolicyHandler, for
both the existing behavior and the new behavior.

      disable "apps", "bookmarks" and "preferences". Verify that
      without the SplitSettingsSync flag those data type toggles
      are disabled in browser sync settings. Verify that with
      the SplitSettingsSync flag those toggles are disabled in
      OS settings (for apps and preferences) and in browser
      settings (for bookmarks).

Bug: 1058853
Test: Use test DM server to add SyncTypesListDisabled policy to
Test: added to components_unittests
Change-Id: Id3427b6611007f1d1a0701f4b2eeceff607c06b7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2090205
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#748323}

components/sync/base/sync_prefs.cc

@@ -125,21 +125,6 @@ void RegisterObsoleteUserTypePrefs(user_prefs::PrefRegistrySyncable* registry) {
   }
 }
 
-#if defined(OS_CHROMEOS)
-const char* GetPrefNameForOsType(UserSelectableOsType type) {
-  switch (type) {
-    case UserSelectableOsType::kOsApps:
-      return prefs::kSyncOsApps;
-    case UserSelectableOsType::kOsPreferences:
-      return prefs::kSyncOsPreferences;
-    case UserSelectableOsType::kWifiConfigurations:
-      return prefs::kSyncWifiConfigurations;
-  }
-  NOTREACHED();
-  return nullptr;
-}
-#endif  // defined(OS_CHROMEOS)
-
 // Gets an offset to add noise to the birth year. If not present in prefs, the
 // offset will be randomly generated within the offset range and cached in
 // syncable prefs.
@@ -536,6 +521,20 @@ void SyncPrefs::SetOsSyncFeatureEnabled(bool enabled) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   pref_service_->SetBoolean(prefs::kOsSyncFeatureEnabled, enabled);
 }
+
+// static
+const char* SyncPrefs::GetPrefNameForOsType(UserSelectableOsType type) {
+  switch (type) {
+    case UserSelectableOsType::kOsApps:
+      return prefs::kSyncOsApps;
+    case UserSelectableOsType::kOsPreferences:
+      return prefs::kSyncOsPreferences;
+    case UserSelectableOsType::kWifiConfigurations:
+      return prefs::kSyncWifiConfigurations;
+  }
+  NOTREACHED();
+  return nullptr;
+}
 #endif  // defined(OS_CHROMEOS)
 
 bool SyncPrefs::IsManaged() const {

components/sync/base/sync_prefs.h

@@ -128,6 +128,10 @@ class SyncPrefs : public CryptoSyncPrefs,
                           UserSelectableOsTypeSet selected_types);
   bool IsOsSyncFeatureEnabled() const;
   void SetOsSyncFeatureEnabled(bool enabled);
+
+  // Maps |type| to its corresponding preference name. Returns nullptr if |type|
+  // isn't an OS type.
+  static const char* GetPrefNameForOsType(UserSelectableOsType type);
 #endif
 
   // Whether Sync is forced off by enterprise policy. Note that this only covers

components/sync/base/user_selectable_type.cc

@@ -7,6 +7,7 @@
 #include <type_traits>
 
 #include "base/logging.h"
+#include "base/optional.h"
 #include "components/sync/base/model_type.h"
 #include "components/sync/base/pref_names.h"
 
@@ -95,23 +96,29 @@ UserSelectableTypeInfo GetUserSelectableTypeInfo(UserSelectableType type) {
 }
 
 #if defined(OS_CHROMEOS)
+constexpr char kOsAppsTypeName[] = "osApps";
+constexpr char kOsPreferencesTypeName[] = "osPreferences";
+constexpr char kWifiConfigurationsTypeName[] = "wifiConfigurations";
+
 UserSelectableTypeInfo GetUserSelectableOsTypeInfo(UserSelectableOsType type) {
   // UserSelectableTypeInfo::type_name is used in js code and shouldn't be
   // changed without updating js part.
   switch (type) {
     case UserSelectableOsType::kOsApps:
-      return {"osApps",
+      return {kOsAppsTypeName,
               APPS,
               {APP_LIST, APPS, APP_SETTINGS, ARC_PACKAGE, WEB_APPS}};
     case UserSelectableOsType::kOsPreferences:
-      return {"osPreferences",
+      return {kOsPreferencesTypeName,
               OS_PREFERENCES,
               {OS_PREFERENCES, OS_PRIORITY_PREFERENCES, PRINTERS}};
     case UserSelectableOsType::kWifiConfigurations:
-      return {"wifiConfigurations", WIFI_CONFIGURATIONS, {WIFI_CONFIGURATIONS}};
+      return {kWifiConfigurationsTypeName,
+              WIFI_CONFIGURATIONS,
+              {WIFI_CONFIGURATIONS}};
   }
 }
-#endif
+#endif  // defined(OS_CHROMEOS)
 
 }  // namespace
 
@@ -119,7 +126,8 @@ const char* GetUserSelectableTypeName(UserSelectableType type) {
   return GetUserSelectableTypeInfo(type).type_name;
 }
 
-UserSelectableType GetUserSelectableTypeFromString(const std::string& type) {
+base::Optional<UserSelectableType> GetUserSelectableTypeFromString(
+    const std::string& type) {
   if (type == kBookmarksTypeName) {
     return UserSelectableType::kBookmarks;
   }
@@ -150,8 +158,7 @@ UserSelectableType GetUserSelectableTypeFromString(const std::string& type) {
   if (type == kTabsTypeName) {
     return UserSelectableType::kTabs;
   }
-  NOTREACHED();
-  return UserSelectableType::kLastType;
+  return base::nullopt;
 }
 
 std::string UserSelectableTypeSetToString(UserSelectableTypeSet types) {
@@ -185,6 +192,32 @@ const char* GetUserSelectableOsTypeName(UserSelectableOsType type) {
   return GetUserSelectableOsTypeInfo(type).type_name;
 }
 
+base::Optional<UserSelectableOsType> GetUserSelectableOsTypeFromString(
+    const std::string& type) {
+  if (type == kOsAppsTypeName) {
+    return UserSelectableOsType::kOsApps;
+  }
+  if (type == kOsPreferencesTypeName) {
+    return UserSelectableOsType::kOsPreferences;
+  }
+  if (type == kWifiConfigurationsTypeName) {
+    return UserSelectableOsType::kWifiConfigurations;
+  }
+
+  // Some pref types migrated from browser prefs to OS prefs. Map the browser
+  // type name to the OS type so that enterprise policy SyncTypesListDisabled
+  // still applies to the migrated names during SplitSettingsSync roll-out.
+  // TODO(https://crbug.com/1059309): Rename "osApps" to "apps" after
+  // SplitSettingsSync is the default, and remove the mapping for "preferences".
+  if (type == kAppsTypeName) {
+    return UserSelectableOsType::kOsApps;
+  }
+  if (type == kPreferencesTypeName) {
+    return UserSelectableOsType::kOsPreferences;
+  }
+  return base::nullopt;
+}
+
 ModelTypeSet UserSelectableOsTypeToAllModelTypes(UserSelectableOsType type) {
   return GetUserSelectableOsTypeInfo(type).model_type_group;
 }

components/sync/base/user_selectable_type.h

@@ -7,6 +7,7 @@
 
 #include <string>
 
+#include "base/optional.h"
 #include "components/sync/base/enum_set.h"
 #include "components/sync/base/model_type.h"
 
@@ -33,7 +34,9 @@ using UserSelectableTypeSet = EnumSet<UserSelectableType,
                                       UserSelectableType::kLastType>;
 
 const char* GetUserSelectableTypeName(UserSelectableType type);
-UserSelectableType GetUserSelectableTypeFromString(const std::string& type);
+// Returns the type if the string matches a known type.
+base::Optional<UserSelectableType> GetUserSelectableTypeFromString(
+    const std::string& type);
 std::string UserSelectableTypeSetToString(UserSelectableTypeSet types);
 ModelTypeSet UserSelectableTypeToAllModelTypes(UserSelectableType type);
 
@@ -65,6 +68,10 @@ using UserSelectableOsTypeSet = EnumSet<UserSelectableOsType,
 const char* GetUserSelectableOsTypeName(UserSelectableOsType type);
 ModelTypeSet UserSelectableOsTypeToAllModelTypes(UserSelectableOsType type);
 ModelType UserSelectableOsTypeToCanonicalModelType(UserSelectableOsType type);
+
+// Returns the type if the string matches a known OS type.
+base::Optional<UserSelectableOsType> GetUserSelectableOsTypeFromString(
+    const std::string& type);
 #endif  // defined(OS_CHROMEOS)
 
 }  // namespace syncer

components/sync/driver/sync_policy_handler.cc

@@ -4,6 +4,9 @@
 
 #include "components/sync/driver/sync_policy_handler.h"
 
+#include <string>
+
+#include "base/optional.h"
 #include "base/values.h"
 #include "components/policy/core/common/policy_map.h"
 #include "components/policy/policy_constants.h"
@@ -12,13 +15,44 @@
 #include "components/sync/base/sync_prefs.h"
 #include "components/sync/base/user_selectable_type.h"
 
+#if defined(OS_CHROMEOS)
+#include "chromeos/constants/chromeos_features.h"
+#endif  // defined(OS_CHROMEOS)
+
 namespace syncer {
+namespace {
+
+void DisableSyncType(const std::string& type_name, PrefValueMap* prefs) {
+  base::Optional<UserSelectableType> type =
+      GetUserSelectableTypeFromString(type_name);
+  if (type.has_value()) {
+    const char* pref = SyncPrefs::GetPrefNameForType(*type);
+    if (pref)
+      prefs->SetValue(pref, base::Value(false));
+  }
+
+#if defined(OS_CHROMEOS)
+  if (chromeos::features::IsSplitSettingsSyncEnabled()) {
+    // Check for OS types. This includes types that used to be browser types,
+    // like "apps" and "preferences".
+    base::Optional<UserSelectableOsType> os_type =
+        GetUserSelectableOsTypeFromString(type_name);
+    if (os_type.has_value()) {
+      const char* os_pref = SyncPrefs::GetPrefNameForOsType(*os_type);
+      if (os_pref)
+        prefs->SetValue(os_pref, base::Value(false));
+    }
+  }
+#endif  // defined(OS_CHROMEOS)
+}
+
+}  // namespace
 
 SyncPolicyHandler::SyncPolicyHandler()
     : policy::TypeCheckingPolicyHandler(policy::key::kSyncDisabled,
                                         base::Value::Type::BOOLEAN) {}
 
-SyncPolicyHandler::~SyncPolicyHandler() {}
+SyncPolicyHandler::~SyncPolicyHandler() = default;
 
 void SyncPolicyHandler::ApplyPolicySettings(const policy::PolicyMap& policies,
                                             PrefValueMap* prefs) {
@@ -34,16 +68,10 @@ void SyncPolicyHandler::ApplyPolicySettings(const policy::PolicyMap& policies,
 
   if (disabled_sync_types_value && disabled_sync_types_value->is_list()) {
     auto list = disabled_sync_types_value->GetList();
-    bool has_one_valid_entry = false;
-    for (auto it = list.begin(); it != list.end(); ++it) {
-      if (!it->is_string())
-        continue;
-      const char* pref = SyncPrefs::GetPrefNameForType(
-          GetUserSelectableTypeFromString(it->GetString()));
-      if (!pref)
+    for (const base::Value& type_name : list) {
+      if (!type_name.is_string())
         continue;
-      prefs->SetValue(pref, base::Value(false));
-      has_one_valid_entry = true;
+      DisableSyncType(type_name.GetString(), prefs);
     }
   }
 }

components/sync/driver/sync_policy_handler_unittest.cc

@@ -14,12 +14,15 @@
 #include "components/sync/base/pref_names.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-namespace syncer {
+#if defined(OS_CHROMEOS)
+#include "base/test/scoped_feature_list.h"
+#include "chromeos/constants/chromeos_features.h"
+#endif  // defined(OS_CHROMEOS)
 
-// Test cases for the Sync policy setting.
-class SyncPolicyHandlerTest : public testing::Test {};
+namespace syncer {
+namespace {
 
-TEST_F(SyncPolicyHandlerTest, Default) {
+TEST(SyncPolicyHandlerTest, Default) {
   policy::PolicyMap policy;
   SyncPolicyHandler handler;
   PrefValueMap prefs;
@@ -27,7 +30,7 @@ TEST_F(SyncPolicyHandlerTest, Default) {
   EXPECT_FALSE(prefs.GetValue(prefs::kSyncManaged, nullptr));
 }
 
-TEST_F(SyncPolicyHandlerTest, Enabled) {
+TEST(SyncPolicyHandlerTest, Enabled) {
   policy::PolicyMap policy;
   policy.Set(policy::key::kSyncDisabled, policy::POLICY_LEVEL_MANDATORY,
              policy::POLICY_SCOPE_USER, policy::POLICY_SOURCE_CLOUD,
@@ -40,7 +43,7 @@ TEST_F(SyncPolicyHandlerTest, Enabled) {
   EXPECT_FALSE(prefs.GetValue(prefs::kSyncManaged, nullptr));
 }
 
-TEST_F(SyncPolicyHandlerTest, Disabled) {
+TEST(SyncPolicyHandlerTest, Disabled) {
   policy::PolicyMap policy;
   policy.Set(policy::key::kSyncDisabled, policy::POLICY_LEVEL_MANDATORY,
              policy::POLICY_SCOPE_USER, policy::POLICY_SOURCE_CLOUD,
@@ -59,4 +62,107 @@ TEST_F(SyncPolicyHandlerTest, Disabled) {
   EXPECT_TRUE(sync_managed);
 }
 
+TEST(SyncPolicyHandlerTest, SyncTypesListDisabled) {
+  // Start with prefs enabled so we can sense that they have changed.
+  PrefValueMap prefs;
+  prefs.SetBoolean(prefs::kSyncBookmarks, true);
+  prefs.SetBoolean(prefs::kSyncPreferences, true);
+  prefs.SetBoolean(prefs::kSyncAutofill, true);
+  prefs.SetBoolean(prefs::kSyncThemes, true);
+
+  // Create a policy that disables some types.
+  policy::PolicyMap policy;
+  base::ListValue disabled_types;
+  disabled_types.AppendString("bookmarks");
+  disabled_types.AppendString("preferences");
+  policy.Set(policy::key::kSyncTypesListDisabled,
+             policy::POLICY_LEVEL_MANDATORY, policy::POLICY_SCOPE_USER,
+             policy::POLICY_SOURCE_CLOUD, disabled_types.CreateDeepCopy(),
+             nullptr);
+  SyncPolicyHandler handler;
+  handler.ApplyPolicySettings(policy, &prefs);
+
+  // Prefs in the policy should be disabled.
+  bool enabled;
+  ASSERT_TRUE(prefs.GetBoolean(prefs::kSyncBookmarks, &enabled));
+  EXPECT_FALSE(enabled);
+  ASSERT_TRUE(prefs.GetBoolean(prefs::kSyncPreferences, &enabled));
+  EXPECT_FALSE(enabled);
+
+  // Prefs that are not part of the policy are still enabled.
+  ASSERT_TRUE(prefs.GetBoolean(prefs::kSyncAutofill, &enabled));
+  EXPECT_TRUE(enabled);
+  ASSERT_TRUE(prefs.GetBoolean(prefs::kSyncThemes, &enabled));
+  EXPECT_TRUE(enabled);
+}
+
+#if defined(OS_CHROMEOS)
+class SyncPolicyHandlerOsTest : public testing::Test {
+ public:
+  SyncPolicyHandlerOsTest() {
+    feature_list_.InitAndEnableFeature(chromeos::features::kSplitSettingsSync);
+  }
+
+  base::test::ScopedFeatureList feature_list_;
+};
+
+TEST_F(SyncPolicyHandlerOsTest, SyncTypesListDisabled_OsTypes) {
+  // Start with prefs enabled so we can sense that they have changed.
+  PrefValueMap prefs;
+  prefs.SetBoolean(prefs::kSyncOsApps, true);
+  prefs.SetBoolean(prefs::kSyncOsPreferences, true);
+  prefs.SetBoolean(prefs::kSyncWifiConfigurations, true);
+
+  // Create a policy that disables the types.
+  policy::PolicyMap policy;
+  base::ListValue disabled_types;
+  disabled_types.AppendString("osApps");
+  disabled_types.AppendString("osPreferences");
+  disabled_types.AppendString("wifiConfigurations");
+  policy.Set(policy::key::kSyncTypesListDisabled,
+             policy::POLICY_LEVEL_MANDATORY, policy::POLICY_SCOPE_USER,
+             policy::POLICY_SOURCE_CLOUD, disabled_types.CreateDeepCopy(),
+             nullptr);
+  SyncPolicyHandler handler;
+  handler.ApplyPolicySettings(policy, &prefs);
+
+  // Prefs in the policy are disabled.
+  bool enabled;
+  ASSERT_TRUE(prefs.GetBoolean(prefs::kSyncOsApps, &enabled));
+  EXPECT_FALSE(enabled);
+  ASSERT_TRUE(prefs.GetBoolean(prefs::kSyncOsPreferences, &enabled));
+  EXPECT_FALSE(enabled);
+  ASSERT_TRUE(prefs.GetBoolean(prefs::kSyncWifiConfigurations, &enabled));
+  EXPECT_FALSE(enabled);
+}
+
+TEST_F(SyncPolicyHandlerOsTest, SyncTypesListDisabled_MigratedTypes) {
+  // Start with prefs enabled so we can sense that they have changed.
+  PrefValueMap prefs;
+  prefs.SetBoolean(prefs::kSyncOsApps, true);
+  prefs.SetBoolean(prefs::kSyncOsPreferences, true);
+
+  // Create a policy that disables the types, but using the original browser
+  // policy names from before the SplitSettingsSync launch.
+  policy::PolicyMap policy;
+  base::ListValue disabled_types;
+  disabled_types.AppendString("apps");
+  disabled_types.AppendString("preferences");
+  policy.Set(policy::key::kSyncTypesListDisabled,
+             policy::POLICY_LEVEL_MANDATORY, policy::POLICY_SCOPE_USER,
+             policy::POLICY_SOURCE_CLOUD, disabled_types.CreateDeepCopy(),
+             nullptr);
+  SyncPolicyHandler handler;
+  handler.ApplyPolicySettings(policy, &prefs);
+
+  // The equivalent OS types are disabled.
+  bool enabled;
+  ASSERT_TRUE(prefs.GetBoolean(prefs::kSyncOsApps, &enabled));
+  EXPECT_FALSE(enabled);
+  ASSERT_TRUE(prefs.GetBoolean(prefs::kSyncOsPreferences, &enabled));
+  EXPECT_FALSE(enabled);
+}
+#endif  // defined(OS_CHROMEOS)
+
+}  // namespace
 }  // namespace syncer