Introduce feature flags Chrome OS device setting.

This new device setting will supersede the existing "startup flags"
setting. The latter was storing chrome://flags feature flags as raw
command line switches, which turned out to be problematic since Chrome
can't easily map the switches back to feature flags to validate them.
Subsequent changes will add support for the new way of doing things as
well as migration logic in session_manager and Chrome.

BUG=chromium:1073940
TEST=None

Change-Id: Id816469ab29e0458a36085655a5922a3dcb3e5bd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2565637
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Commit-Queue: Mattias Nissler <mnissler@chromium.org>
Cr-Commit-Position: refs/heads/master@{#832033}

chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc

@@ -620,17 +620,22 @@ void OwnerSettingsServiceChromeOS::UpdateDeviceSettings(
     } else {
       NOTREACHED();
     }
-  } else if (path == kStartUpFlags) {
-    em::StartUpFlagsProto* flags_proto = settings.mutable_start_up_flags();
-    flags_proto->Clear();
-    const base::ListValue* flags;
-    if (value.GetAsList(&flags)) {
-      for (base::ListValue::const_iterator i = flags->begin();
-           i != flags->end();
-           ++i) {
-        std::string flag;
-        if (i->GetAsString(&flag))
-          flags_proto->add_flags(flag);
+  } else if (path == kStartUpFlagsDeprecated) {
+    em::FeatureFlagsProto* feature_flags = settings.mutable_feature_flags();
+    feature_flags->Clear();
+    if (value.is_list()) {
+      for (const auto& flag : value.GetList()) {
+        if (flag.is_string())
+          feature_flags->add_switches(flag.GetString());
+      }
+    }
+  } else if (path == kFeatureFlags) {
+    em::FeatureFlagsProto* feature_flags = settings.mutable_feature_flags();
+    feature_flags->Clear();
+    if (value.is_list()) {
+      for (const auto& flag : value.GetList()) {
+        if (flag.is_string())
+          feature_flags->add_feature_flags(flag.GetString());
       }
     }
   } else if (path == kSystemUse24HourClock) {

chrome/browser/chromeos/settings/device_settings_provider.cc

@@ -105,6 +105,7 @@ const char* const kKnownSettings[] = {
     kDeviceWilcoDtcAllowed,
     kDisplayRotationDefault,
     kExtensionCacheSize,
+    kFeatureFlags,
     kHeartbeatEnabled,
     kHeartbeatFrequency,
     kLoginAuthenticationBehavior,
@@ -145,7 +146,7 @@ const char* const kKnownSettings[] = {
     kSamlLoginAuthenticationType,
     kServiceAccountIdentity,
     kSignedDataRoamingEnabled,
-    kStartUpFlags,
+    kStartUpFlagsDeprecated,
     kStatsReportingPref,
     kSystemLogUploadEnabled,
     kSystemProxySettings,
@@ -374,14 +375,24 @@ void DecodeLoginPolicies(const em::ChromeDeviceSettingsProto& policy,
       kAccountsPrefDeviceLocalAccountPromptForNetworkWhenOffline,
       policy.device_local_accounts().prompt_for_network_when_offline());
 
-  if (policy.has_start_up_flags()) {
-    std::vector<base::Value> list;
-    const em::StartUpFlagsProto& flags_proto = policy.start_up_flags();
-    const RepeatedPtrField<std::string>& flags = flags_proto.flags();
-    for (const std::string& entry : flags) {
-      list.push_back(base::Value(entry));
+  if (policy.has_feature_flags()) {
+    std::vector<base::Value> switches_list;
+    for (const std::string& entry : policy.feature_flags().switches()) {
+      switches_list.push_back(base::Value(entry));
+    }
+    if (!switches_list.empty()) {
+      new_values_cache->SetValue(kStartUpFlagsDeprecated,
+                                 base::Value(std::move(switches_list)));
+    }
+
+    std::vector<base::Value> feature_flags_list;
+    for (const std::string& entry : policy.feature_flags().feature_flags()) {
+      feature_flags_list.push_back(base::Value(entry));
+    }
+    if (!feature_flags_list.empty()) {
+      new_values_cache->SetValue(kFeatureFlags,
+                                 base::Value(std::move(feature_flags_list)));
     }
-    new_values_cache->SetValue(kStartUpFlags, base::Value(std::move(list)));
   }
 
   if (policy.has_saml_settings()) {

chrome/browser/chromeos/settings/device_settings_provider_unittest.cc

@@ -1228,4 +1228,26 @@ TEST_F(DeviceSettingsProviderTest, DeviceFamilyLinkAccountsAllowedEnabled) {
             *provider_->Get(kAccountsPrefFamilyLinkAccountsAllowed));
 }
 
+TEST_F(DeviceSettingsProviderTest, StartUpFlagsDeprecated) {
+  EXPECT_EQ(nullptr, provider_->Get(kStartUpFlagsDeprecated));
+
+  device_policy_->payload().mutable_feature_flags()->add_switches("--foo");
+  BuildAndInstallDevicePolicy();
+
+  base::ListValue expected_switches;
+  expected_switches.Append(base::Value("--foo"));
+  EXPECT_EQ(expected_switches, *provider_->Get(kStartUpFlagsDeprecated));
+}
+
+TEST_F(DeviceSettingsProviderTest, FeatureFlags) {
+  EXPECT_EQ(nullptr, provider_->Get(kFeatureFlags));
+
+  device_policy_->payload().mutable_feature_flags()->add_feature_flags("foo");
+  BuildAndInstallDevicePolicy();
+
+  base::ListValue expected_feature_flags;
+  expected_feature_flags.Append(base::Value("foo"));
+  EXPECT_EQ(expected_feature_flags, *provider_->Get(kFeatureFlags));
+}
+
 }  // namespace chromeos

chrome/browser/chromeos/settings/owner_flags_storage.cc

@@ -38,7 +38,7 @@ bool OwnerFlagsStorage::SetFlags(const std::set<std::string>& flags) {
        it != switches.end(); ++it) {
     experiments_list.AppendString(*it);
   }
-  owner_settings_service_->Set(kStartUpFlags, experiments_list);
+  owner_settings_service_->Set(kStartUpFlagsDeprecated, experiments_list);
 
   return true;
 }

chromeos/settings/cros_settings_names.cc

@@ -233,9 +233,14 @@ const char kPolicyMissingMitigationMode[] =
 const char kAllowRedeemChromeOsRegistrationOffers[] =
     "cros.echo.allow_redeem_chrome_os_registration_offers";
 
+// A list pref storing the feature flags (in the chrome://flags sense) that
+// should to be applied at the login screen.
+const char kFeatureFlags[] = "cros.feature_flags";
+
 // A list pref storing the flags that need to be applied to the browser upon
-// start-up.
-const char kStartUpFlags[] = "cros.startup_flags";
+// start-up. This lists raw flags, which isn't ideal since Chrome can't easily
+// tie this back to feature flags. Deprecated in favor of kFeatureFlags.
+const char kStartUpFlagsDeprecated[] = "cros.startup_flags";
 
 // A string pref for the restrict parameter to be appended to the Variations URL
 // when pinging the Variations server.

chromeos/settings/cros_settings_names.h

@@ -133,7 +133,8 @@ extern const char kPolicyMissingMitigationMode[];
 COMPONENT_EXPORT(CHROMEOS_SETTINGS)
 extern const char kAllowRedeemChromeOsRegistrationOffers[];
 
-COMPONENT_EXPORT(CHROMEOS_SETTINGS) extern const char kStartUpFlags[];
+COMPONENT_EXPORT(CHROMEOS_SETTINGS) extern const char kFeatureFlags[];
+COMPONENT_EXPORT(CHROMEOS_SETTINGS) extern const char kStartUpFlagsDeprecated[];
 
 COMPONENT_EXPORT(CHROMEOS_SETTINGS) extern const char kKioskAppSettingsPrefix[];
 COMPONENT_EXPORT(CHROMEOS_SETTINGS)

components/policy/proto/chrome_device_policy.proto

@@ -552,11 +552,25 @@ message AllowRedeemChromeOsRegistrationOffersProto {
   optional bool allow_redeem_offers = 1 [default = true];
 }
 
-message StartUpFlagsProto {
-  // Specifies the flags that should be applied to Google Chrome when it starts.
-  // The specified flags are applied on the login screen only. Flags set via
-  // this policy do not propagate into user sessions.
-  repeated string flags = 1;
+message FeatureFlagsProto {
+  // Specifies switches that should be passed to Google Chrome when it starts.
+  // The specified switches are applied on the login screen only. Switches set
+  // via this policy do not propagate into user sessions.
+  // This is deprecated because it turned out that storing raw switches is
+  // problematic since Chrome can't easily tie switches back to feature flags to
+  // validate them. The |feature_flags| field below works in terms of feature
+  // flag names (i.e. chrome://flags items) instead and supersedes |switches|.
+  repeated string switches = 1 [deprecated = true];
+
+  // Specifies feature flags (i.e. chrome://flags items) that should be enabled
+  // when Chrome starts. The format of the individual entries matches the format
+  // chrome://flags uses for internal bookkeeping, i.e. either the flag name as
+  // listed on chrome://flags (for flags that only have a single choice besides
+  // the default) or the flag name followed by the index of the chosen option,
+  // separated by an '@' character (for flags with multiple choices). The
+  // specified feature flags are applied on the login screen only and don't
+  // propagate into the user session.
+  repeated string feature_flags = 2;
 }
 
 message UptimeLimitProto {
@@ -1763,7 +1777,7 @@ message ChromeDeviceSettingsProto {
   optional SystemTimezoneProto system_timezone = 20;
   optional DeviceLocalAccountsProto device_local_accounts = 21;
   optional AllowRedeemChromeOsRegistrationOffersProto allow_redeem_offers = 22;
-  optional StartUpFlagsProto start_up_flags = 23;
+  optional FeatureFlagsProto feature_flags = 23;
   optional UptimeLimitProto uptime_limit = 24;
   optional VariationsParameterProto variations_parameter = 25;
   optional AttestationSettingsProto attestation_settings = 26;

components/policy/resources/policy_templates.json

@@ -24110,6 +24110,7 @@ The recommended way to configure policy on Windows is via GPO, although provisio
     ['DeviceLoginScreenSaverId', ''],
     ['DeviceLoginScreenSaverTimeout', ''],
     ['DeviceAppPack', ''],
+    ['DeviceStartUpFlags', ''],
     # Proto fields with unknown policy.
     ['', 'device_reporting.report_running_kiosk_app'],
     ['', 'camera_enabled.camera_enabled'],
@@ -24275,7 +24276,6 @@ The recommended way to configure policy on Windows is via GPO, although provisio
     'DeviceWiFiAllowed': 'device_wifi_allowed.device_wifi_allowed',
     'AutoCleanUpStrategy': 'auto_clean_up_settings.clean_up_strategy',
     'SupervisedUsersEnabled': 'supervised_users_settings.supervised_users_enabled',
-    'DeviceStartUpFlags': 'start_up_flags.flags',
     'ReportDeviceBoardStatus': 'device_reporting.report_board_status',
     'ReportDeviceCpuInfo': 'device_reporting.report_cpu_info',
     'ReportDeviceTimezoneInfo': 'device_reporting.report_timezone_info',