onc_to_shill: Set defaults for {IPAddress,NameServers}ConfigType

In onc to shill translation, set IPAddressConfigType and
NameServersConfigType to "DHCP" in the resulting ONC data if no value is
provided by shill.
The default of "DHCP" is consistent with go/onc and with shill behavior.

This is important so that these values always have a value when policy
decoration happens (the shill -> onc translated ONC is an input to
policy_util::CreateManagedONC invoked in
ManagedNetworkConfigurationHandlerImpl).

The UI can then depend on these always having an ActiveValue, so
cros_network_config.mojom has been changed to set the corresponding
{ip_address,name_servers}_config_type fields as mandatory.

Bug: 1108922, 1109660, 1109670
Test: unit tests + manual on DuT (network settings)
Change-Id: I6111a3c7f6af64fac22ea33347dd17774bbda4c7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2530330Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Pavol Marko <pmarko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#827390}

chrome/test/data/extensions/api_test/networking_private/alias/test.js

@@ -54,8 +54,10 @@ chrome.test.runTests([
               },
               ConnectionState: 'NotConnected',
               GUID: 'stub_cellular1_guid',
+              IPAddressConfigType: chrome.networking.onc.IPConfigType.DHCP,
               Metered: true,
               Name: 'cellular1',
+              NameServersConfigType: chrome.networking.onc.IPConfigType.DHCP,
               Source: 'User',
               Type: 'Cellular',
             }, result);

chrome/test/data/extensions/api_test/networking_private/chromeos/test.js

@@ -541,6 +541,7 @@ var availableTests = [
           }],
           MacAddress: '00:11:22:AA:BB:CC',
           Name: 'wifi1',
+          NameServersConfigType: chrome.networkingPrivate.IPConfigType.DHCP,
           Source: 'User',
           StaticIPConfig: {
             IPAddress: '1.2.3.4',
@@ -591,8 +592,10 @@ var availableTests = [
           },
           ConnectionState: ConnectionStateType.NOT_CONNECTED,
           GUID: kCellularGuid,
+          IPAddressConfigType: chrome.networkingPrivate.IPConfigType.DHCP,
           Metered: true,
           Name: 'cellular1',
+          NameServersConfigType: chrome.networkingPrivate.IPConfigType.DHCP,
           Source: 'User',
           Type: NetworkType.CELLULAR,
         }, result);
@@ -633,7 +636,9 @@ var availableTests = [
               Connectable: false,
               ConnectionState: ConnectionStateType.NOT_CONNECTED,
               GUID: guid,
+              IPAddressConfigType: chrome.networkingPrivate.IPConfigType.DHCP,
               Name: '',
+              NameServersConfigType: chrome.networkingPrivate.IPConfigType.DHCP,
               Priority: 0,
               Source: 'None',
               Type: NetworkType.CELLULAR,
@@ -659,6 +664,14 @@ var availableTests = [
               UserPolicy: 'Direct'
             }
           },
+          IPAddressConfigType: {
+            Active: 'DHCP',
+            Effective: 'UserPolicy'
+          },
+          NameServersConfigType: {
+            Active: 'DHCP',
+            Effective: 'UserPolicy'
+          },
           Source: 'UserPolicy',
           Type: NetworkType.WI_FI,
           WiFi: {

chromeos/network/network_util.cc

@@ -194,6 +194,15 @@ std::unique_ptr<base::DictionaryValue> TranslateNetworkStateToONC(
   std::unique_ptr<base::DictionaryValue> onc_dictionary =
       TranslateShillServiceToONCPart(*shill_dictionary, onc_source,
                                      &onc::kNetworkWithStateSignature, network);
+
+  // Remove IPAddressConfigType/NameServersConfigType as these were
+  // historically not provided by TranslateNetworkStateToONC.
+  // The source shill properties for those ONC properties are not provided by
+  // NetworkState::GetStateProperties, however since CL:2530330 these are
+  // assumed to have defaults that are always enforced during ONC translation.
+  onc_dictionary->RemoveKey(::onc::network_config::kIPAddressConfigType);
+  onc_dictionary->RemoveKey(::onc::network_config::kNameServersConfigType);
+
   return onc_dictionary;
 }
 

chromeos/network/onc/onc_signature.cc

@@ -325,10 +325,12 @@ const OncFieldSignature network_configuration_fields[] = {
     {::onc::network_config::kCellular, &kCellularSignature},
     {::onc::network_config::kEthernet, &kEthernetSignature},
     {::onc::network_config::kGUID, &kStringSignature},
-    {::onc::network_config::kIPAddressConfigType, &kStringSignature},
+    {::onc::network_config::kIPAddressConfigType, &kStringSignature,
+     []() { return base::Value(::onc::network_config::kIPConfigTypeDHCP); }},
     {::onc::network_config::kMetered, &kBoolSignature},
     {::onc::network_config::kName, &kStringSignature},
-    {::onc::network_config::kNameServersConfigType, &kStringSignature},
+    {::onc::network_config::kNameServersConfigType, &kStringSignature,
+     []() { return base::Value(::onc::network_config::kIPConfigTypeDHCP); }},
     {::onc::network_config::kPriority, &kIntegerSignature},
     {::onc::network_config::kProxySettings, &kProxySettingsSignature},
     {::onc::kRecommended, &kRecommendedSignature},

chromeos/network/onc/onc_signature.h

@@ -13,11 +13,20 @@
 namespace chromeos {
 namespace onc {
 
+// Generates a default value for a field.
+// This is used so that static global base::Values can be avoided (which would
+// have a non-trivial destructor and are thus prohibited).
+using DefaultValueSetterFunc = base::Value (*)();
+
 struct OncValueSignature;
 
 struct OncFieldSignature {
   const char* onc_field_name;
   const OncValueSignature* value_signature;
+  // If this is non-null, it will be called if the field doesn't have a value
+  // after shill->onc translation and the returned value will be assigned to the
+  // field.
+  DefaultValueSetterFunc default_value_setter = nullptr;
 };
 
 struct COMPONENT_EXPORT(CHROMEOS_NETWORK) OncValueSignature {

chromeos/network/onc/onc_translator_shill_to_onc.cc

@@ -167,6 +167,13 @@ class ShillToONCTranslator {
   // entry from |shill_dictionary_| to |onc_object_| if it exists.
   void CopyProperty(const OncFieldSignature* field_signature);
 
+  // Applies defaults to fields according to |onc_signature_|.
+  void SetDefaultsAccordingToSignature();
+
+  // Applies defaults to fields according to |value_signature|.
+  void SetDefaultsAccordingToSignature(
+      const OncValueSignature* value_signature);
+
   // If existent, translates the entry at |shill_property_name| in
   // |shill_dictionary_| using |table|. It is an error if no matching table
   // entry is found. Writes the result as entry at |onc_field_name| in
@@ -224,6 +231,9 @@ ShillToONCTranslator::CreateTranslatedONCObject() {
   } else {
     CopyPropertiesAccordingToSignature();
   }
+
+  SetDefaultsAccordingToSignature();
+
   return base::DictionaryValue::From(std::move(onc_object_));
 }
 
@@ -841,6 +851,27 @@ void ShillToONCTranslator::CopyProperty(
   onc_object_->SetKey(field_signature->onc_field_name, shill_value->Clone());
 }
 
+void ShillToONCTranslator::SetDefaultsAccordingToSignature() {
+  SetDefaultsAccordingToSignature(onc_signature_);
+}
+
+void ShillToONCTranslator::SetDefaultsAccordingToSignature(
+    const OncValueSignature* value_signature) {
+  if (value_signature->base_signature)
+    SetDefaultsAccordingToSignature(value_signature->base_signature);
+  if (!value_signature->fields)
+    return;
+  for (const OncFieldSignature* field_signature = value_signature->fields;
+       field_signature->onc_field_name != nullptr; ++field_signature) {
+    if (!field_signature->default_value_setter)
+      continue;
+    if (onc_object_->FindKey(field_signature->onc_field_name))
+      continue;
+    onc_object_->SetKey(field_signature->onc_field_name,
+                        field_signature->default_value_setter());
+  }
+}
+
 void ShillToONCTranslator::TranslateWithTableAndSet(
     const std::string& shill_property_name,
     const StringTranslationEntry table[],

chromeos/network/onc/onc_translator_unittest.cc

@@ -153,7 +153,7 @@ INSTANTIATE_TEST_SUITE_P(
         std::make_pair("shill_cellular_with_state.json",
                        "translation_of_shill_cellular_with_state.onc"),
         std::make_pair("shill_output_third_party_vpn.json",
-                       "third_party_vpn.onc"),
+                       "translation_of_shill_output_third_party_vpn.onc"),
         std::make_pair(
             "shill_wifi_eap_ttls_with_password_variable.json",
             "translation_of_shill_wifi_eap_ttls_with_password_variable.onc"),

chromeos/services/network_config/cros_network_config.cc

@@ -1244,12 +1244,12 @@ mojom::ManagedPropertiesPtr ManagedPropertiesToMojo(
     result->saved_ip_config = GetIPConfig(saved_ip_config);
 
   // Managed properties
-  result->ip_address_config_type =
-      GetManagedString(properties, ::onc::network_config::kIPAddressConfigType);
+  result->ip_address_config_type = GetRequiredManagedString(
+      properties, ::onc::network_config::kIPAddressConfigType);
   result->metered =
       GetManagedBoolean(properties, ::onc::network_config::kMetered);
   result->name = GetManagedString(properties, ::onc::network_config::kName);
-  result->name_servers_config_type = GetManagedString(
+  result->name_servers_config_type = GetRequiredManagedString(
       properties, ::onc::network_config::kNameServersConfigType);
   result->priority =
       GetManagedInt32(properties, ::onc::network_config::kPriority);

chromeos/services/network_config/public/mojom/cros_network_config.mojom

@@ -570,11 +570,11 @@ struct ManagedProperties {
   bool connectable = false;
   string? error_state;
   string guid;
-  ManagedString? ip_address_config_type;
+  ManagedString ip_address_config_type;
   array<IPConfigProperties>? ip_configs;
   ManagedBoolean? metered;
   ManagedString? name;
-  ManagedString? name_servers_config_type;
+  ManagedString name_servers_config_type;
   ManagedInt32? priority;
   ManagedProxySettings? proxy_settings;
   PortalState portal_state = kUnknown;

chromeos/test/data/network/policy/managed_onc_disallow_autoconnect_on_unmanaged_wifi2.onc

@@ -4,6 +4,12 @@
   "Name": {
     "Active": "wifi2"
   },
+  "IPAddressConfigType": {
+    "Active": "DHCP"
+  },
+  "NameServersConfigType": {
+    "Active": "DHCP"
+  },
   "Source": "User",
   "Type": "WiFi",
   "WiFi": {

chromeos/test/data/network/tether.onc

 {
   "Type": "Tether",
   "Name": "Tether Network",
+  "IPAddressConfigType":"DHCP",
+  "NameServersConfigType":"DHCP",
   "Tether": {
     "BatteryPercentage": 85,
     "Carrier": "Project Fi",

chromeos/test/data/network/translation_of_shill_cellular_with_state.onc

@@ -52,5 +52,7 @@
     },
     "Scanning": false
   },
+  "IPAddressConfigType":"DHCP",
+  "NameServersConfigType":"DHCP",
   "Source":"Device"
 }

chromeos/test/data/network/translation_of_shill_ethernet_with_eap.onc

@@ -11,6 +11,8 @@
     }
   },
   "GUID":"guid",
+  "IPAddressConfigType":"DHCP",
   "Name":"",
+  "NameServersConfigType":"DHCP",
   "Type":"Ethernet"
 }

chromeos/test/data/network/translation_of_shill_l2tpipsec.onc

 { "GUID": "guid",
   "Type": "VPN",
   "Name": "MyL2TPVPN",
+  "IPAddressConfigType":"DHCP",
+  "NameServersConfigType":"DHCP",
   "VPN": {
     "Type": "L2TP-IPsec",
     "Host": "some.host.org",

chromeos/test/data/network/translation_of_shill_openvpn.onc

 {
     "GUID": "{a3860e83-f03d-4cb1-bafa-b22c9e746950}",
+    "IPAddressConfigType":"DHCP",
     "Name": "my vpn",
+    "NameServersConfigType":"DHCP",
     "Type": "VPN",
     "VPN": {
         "AutoConnect": true,

chromeos/test/data/network/translation_of_shill_openvpn_clientcert.onc

 {
     "GUID": "{84391467-dd8b-4c66-9b52-860583038351}",
+    "IPAddressConfigType":"DHCP",
     "Name": "TestOpenVPN",
+    "NameServersConfigType":"DHCP",
     "Type": "VPN",
     "VPN": {
         "Host": "terminus.muc",

chromeos/test/data/network/translation_of_shill_openvpn_compression_algorithm_lz4_v2.onc

@@ -2,6 +2,8 @@
     "GUID": "{a3860e83-f03d-4cb1-bafa-b22c9e746950}",
     "Name": "my vpn",
     "Type": "VPN",
+    "IPAddressConfigType":"DHCP",
+    "NameServersConfigType":"DHCP",
     "VPN": {
         "AutoConnect": true,
         "Host": "vpn.my.domain.com",

chromeos/test/data/network/translation_of_shill_openvpn_with_errors.onc

 {
     // "GUID" missing because it was missing in the Shill dict.
     "Name": "my vpn",
+    "IPAddressConfigType":"DHCP",
+    "NameServersConfigType":"DHCP",
     "Type": "VPN",
     "VPN": {
         "Host": "vpn.my.domain.com",

chromeos/test/data/network/translation_of_shill_output_third_party_vpn.onc

+{
+    "GUID": "guid",
+    "IPAddressConfigType":"DHCP",
+    "Name": "third-party VPN",
+    "NameServersConfigType":"DHCP",
+    "Type": "VPN",
+    "VPN": {
+        "Type": "ThirdPartyVPN",
+        "ThirdPartyVPN": {
+            "ExtensionID": "deadbeefdeadbeefdeadbeefdeadbeef",
+        }
+    }
+}

chromeos/test/data/network/translation_of_shill_vpn_ipsec.onc

@@ -2,6 +2,8 @@
     "GUID": "{84391467-dd8b-4c66-9b52-860583038351}",
     "Name": "TestIpsecVPN",
     "Type": "VPN",
+    "IPAddressConfigType":"DHCP",
+    "NameServersConfigType":"DHCP",
     "VPN": {
         "Host": "terminus.muc",
         "IPsec": {

chromeos/test/data/network/translation_of_shill_wifi_clientcert.onc

 {
   "GUID":"{77db0089-0bc8-4358-929c-123xcv}",
   "Name":"",
+  "IPAddressConfigType":"DHCP",
   "ProxySettings": {
       "Type": "PAC",
       "PAC": "https://proxycfg.my.domain.com/proxy.dat"
   },
+  "NameServersConfigType":"DHCP",
   "Type":"WiFi",
   "WiFi":{
     "EAP": {

chromeos/test/data/network/translation_of_shill_wifi_eap_tls.onc

@@ -2,6 +2,8 @@
   "GUID": "{77db0089-0bc8-4358-929c-123xcv}",
   "Type": "WiFi",
   "Name": "",
+  "IPAddressConfigType":"DHCP",
+  "NameServersConfigType":"DHCP",
   "WiFi": {
     "Security": "WPA-EAP",
     "EAP": {

chromeos/test/data/network/translation_of_shill_wifi_eap_ttls.onc

@@ -2,6 +2,8 @@
   "GUID": "{77db0089-0bc8-4358-929c-123xcv}",
   "Type": "WiFi",
   "Name": "",
+  "IPAddressConfigType":"DHCP",
+  "NameServersConfigType":"DHCP",
   "WiFi": {
     "Security": "WPA-EAP",
     "EAP": {

chromeos/test/data/network/translation_of_shill_wifi_eap_ttls_with_hardcoded_password.onc

@@ -2,6 +2,8 @@
   "GUID": "{77db0089-0bc8-4358-929c-123xcv}",
   "Type": "WiFi",
   "Name": "",
+  "IPAddressConfigType":"DHCP",
+  "NameServersConfigType":"DHCP",
   "WiFi": {
     "Security": "WPA-EAP",
     "EAP": {

chromeos/test/data/network/translation_of_shill_wifi_eap_ttls_with_password_variable.onc

@@ -2,6 +2,8 @@
   "GUID": "{77db0089-0bc8-4358-929c-123xcv}",
   "Type": "WiFi",
   "Name": "",
+  "IPAddressConfigType":"DHCP",
+  "NameServersConfigType":"DHCP",
   "WiFi": {
     "Security": "WPA-EAP",
     "EAP": {

chromeos/test/data/network/translation_of_shill_wifi_non_utf8_ssid.onc

 {
   "GUID":"{64c4f86b-cf6a-4e4a-8eff-456def}",
+  "IPAddressConfigType":"DHCP",
   "Name":"OpenWrt",
+  "NameServersConfigType":"DHCP",
   "Type":"WiFi",
   "WiFi":{
     // Hex of the SSID field

chromeos/test/data/network/translation_of_shill_wifi_proxy.onc

 {
+  "IPAddressConfigType":"DHCP",
   "Name": "",
+  "NameServersConfigType":"DHCP",
   "ProxySettings": {
     "Type": "Manual",
     "Manual": {

chromeos/test/data/network/translation_of_shill_wifi_proxy_pac.onc

 {
   "Name": "",
+  "IPAddressConfigType":"DHCP",
   "ProxySettings": {
     "Type": "PAC",
     "PAC": "http://pac.foo.com/script.pac"
   },
+  "NameServersConfigType":"DHCP",
   "Type": "WiFi",
   "WiFi": {
     "Security": "None"

chromeos/test/data/network/translation_of_shill_wifi_wep_8021x_clientcert.onc

 {
   "GUID":"{77db0089-0bc8-4358-929c-123xcv}",
+  "IPAddressConfigType":"DHCP",
   "Name":"",
+  "NameServersConfigType":"DHCP",
   "ProxySettings": {
       "Type": "PAC",
       "PAC": "https://proxycfg.my.domain.com/proxy.dat"

chromeos/test/data/network/translation_of_shill_wifi_with_state.onc

@@ -2,6 +2,8 @@
   "Connectable": true,
   "GUID": "{64c4f86b-cf6a-4e4a-8eff-456def}",
   "Name": "OpenWrt",
+  "IPAddressConfigType":"DHCP",
+  "NameServersConfigType":"DHCP",
   "Priority": 1,
   "Type": "WiFi",
   "WiFi": {

extensions/browser/api/networking_private/networking_private_chromeos_unittest.cc

@@ -1093,8 +1093,10 @@ TEST_F(NetworkingPrivateApiTest, GetCellularProperties) {
                    .Build())
           .Set("ConnectionState", "Connected")
           .Set("GUID", "cellular_guid")
+          .Set("IPAddressConfigType", "DHCP")
           .Set("Metered", true)
           .Set("Name", "cellular")
+          .Set("NameServersConfigType", "DHCP")
           .Set("Source", "User")
           .Set("Type", "Cellular")
           .Build();
@@ -1153,8 +1155,10 @@ TEST_F(NetworkingPrivateApiTest, GetCellularPropertiesFromWebUi) {
                    .Build())
           .Set("ConnectionState", "Connected")
           .Set("GUID", "cellular_guid")
+          .Set("IPAddressConfigType", "DHCP")
           .Set("Metered", true)
           .Set("Name", "cellular")
+          .Set("NameServersConfigType", "DHCP")
           .Set("Source", "User")
           .Set("Type", "Cellular")
           .Build();

ui/webui/resources/cr_components/chromeos/network/network_ip_config.js

@@ -163,11 +163,8 @@ Polymer({
     }
 
     // Update the 'automatic' property.
-    if (properties.ipAddressConfigType) {
-      const ipConfigType =
-          OncMojo.getActiveValue(properties.ipAddressConfigType);
-      this.automatic_ = ipConfigType !== 'Static';
-    }
+    const ipConfigType = OncMojo.getActiveValue(properties.ipAddressConfigType);
+    this.automatic_ = ipConfigType !== 'Static';
 
     if (properties.ipConfigs || properties.staticIpConfig) {
       // Update the 'ipConfig' property.

ui/webui/resources/cr_components/chromeos/network/network_nameservers.js

@@ -130,11 +130,8 @@ Polymer({
     }
 
     // Update the 'nameserversType' property.
-    let configType;
-    if (this.managedProperties.nameServersConfigType) {
-      configType =
-          OncMojo.getActiveValue(this.managedProperties.nameServersConfigType);
-    }
+    const configType =
+        OncMojo.getActiveValue(this.managedProperties.nameServersConfigType);
     let type;
     if (configType === 'Static') {
       if (this.isGoogleNameservers_(nameservers)) {
@@ -186,8 +183,7 @@ Polymer({
     if (!managedProperties) {
       return false;
     }
-    if (managedProperties.nameServersConfigType &&
-        this.isNetworkPolicyEnforced(managedProperties.nameServersConfigType)) {
+    if (this.isNetworkPolicyEnforced(managedProperties.nameServersConfigType)) {
       return false;
     }
     if (managedProperties.staticIpConfig &&
@@ -212,8 +208,7 @@ Polymer({
     if (nameserversType !== 'custom') {
       return false;
     }
-    if (managedProperties.nameServersConfigType &&
-        this.isNetworkPolicyEnforced(managedProperties.nameServersConfigType)) {
+    if (this.isNetworkPolicyEnforced(managedProperties.nameServersConfigType)) {
       return false;
     }
     if (managedProperties.staticIpConfig &&

ui/webui/resources/cr_components/chromeos/network/onc_mojo.js

@@ -717,6 +717,8 @@
       connectable: false,
       guid: guid,
       name: OncMojo.createManagedString(name),
+      ipAddressConfigType: OncMojo.createManagedString('DHCP'),
+      nameServersConfigType: OncMojo.createManagedString('DHCP'),
       portalState: mojom.PortalState.kUnknown,
     };
     switch (type) {