cros_network_config fixes to required properties and policy indicators

In C++ mojo uses StructPtr for embedded structs, regardless of whether
they are required, so it is possible to pass 'nullptr' for required
properties, causing a runtime error when the properties are received.

This CL:
* Ensures that required ManagedString properties are provided in the
  C++ code, and fixes the set of properties provided for VPNs based on
  type.
* Fixes some CSS causing incorrect indicators, and cleans up some logic
  in the JS.

Bug: 1008230
Change-Id: I51484fc74e672b7b8434228abd219428461b92f0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1824320
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#700060}

chrome/browser/resources/settings/internet_page/network_proxy_section.html

@@ -21,7 +21,7 @@
   <template>
     <style include="internet-shared cr-hidden-style iron-flex
         iron-flex-alignment">
-      cr-policy-network-indicator {
+      cr-policy-network-indicator-mojo {
         margin-inline-end: 10px;
       }
 

chromeos/services/network_config/cros_network_config.cc

@@ -617,6 +617,16 @@ mojom::ManagedStringPtr GetManagedString(const base::Value* dict,
   return nullptr;
 }
 
+mojom::ManagedStringPtr GetRequiredManagedString(const base::Value* dict,
+                                                 const char* key) {
+  mojom::ManagedStringPtr result = GetManagedString(dict, key);
+  if (!result) {
+    // Return an empty string with no policy source.
+    result = mojom::ManagedString::New();
+  }
+  return result;
+}
+
 mojom::ManagedStringListPtr GetManagedStringList(const base::Value* dict,
                                                  const char* key) {
   const base::Value* v = dict->FindKey(key);
@@ -744,8 +754,13 @@ mojom::ManagedProxyLocationPtr GetManagedProxyLocation(const base::Value* dict,
   if (!location_dict)
     return nullptr;
   auto proxy_location = mojom::ManagedProxyLocation::New();
-  proxy_location->host = GetManagedString(location_dict, ::onc::proxy::kHost);
+  proxy_location->host =
+      GetRequiredManagedString(location_dict, ::onc::proxy::kHost);
   proxy_location->port = GetManagedInt32(location_dict, ::onc::proxy::kPort);
+  if (!proxy_location->port) {
+    NET_LOG(ERROR) << "ProxyLocation: No port: " << *location_dict;
+    return nullptr;
+  }
   return proxy_location;
 }
 
@@ -763,7 +778,7 @@ void SetProxyLocation(const char* key,
 mojom::ManagedProxySettingsPtr GetManagedProxySettings(
     const base::Value* dict) {
   auto proxy_settings = mojom::ManagedProxySettings::New();
-  proxy_settings->type = GetManagedString(dict, ::onc::proxy::kType);
+  proxy_settings->type = GetRequiredManagedString(dict, ::onc::proxy::kType);
   const base::Value* manual_dict = GetDictionary(dict, ::onc::proxy::kManual);
   if (manual_dict) {
     auto manual_proxy_settings = mojom::ManagedManualProxySettings::New();
@@ -807,7 +822,7 @@ mojom::ManagedApnPropertiesPtr GetManagedApnProperties(const base::Value* dict,
   }
   auto apn = mojom::ManagedApnProperties::New();
   apn->access_point_name =
-      GetManagedString(apn_dict, ::onc::cellular_apn::kAccessPointName);
+      GetRequiredManagedString(apn_dict, ::onc::cellular_apn::kAccessPointName);
   CHECK(apn->access_point_name);
   apn->authentication =
       GetManagedString(apn_dict, ::onc::cellular_apn::kAuthentication);
@@ -988,7 +1003,7 @@ mojom::ManagedIPSecPropertiesPtr GetManagedIPSecProperties(
     return ipsec;
   }
   ipsec->authentication_type =
-      GetManagedString(ipsec_dict, ::onc::ipsec::kAuthenticationType);
+      GetRequiredManagedString(ipsec_dict, ::onc::ipsec::kAuthenticationType);
   ipsec->client_cert_pattern = GetManagedCertificatePattern(
       ipsec_dict, ::onc::client_cert::kClientCertPattern);
   ipsec->client_cert_pkcs11_id =
@@ -1295,37 +1310,43 @@ mojom::ManagedPropertiesPtr ManagedPropertiesToMojo(
 
       vpn->auto_connect = GetManagedBoolean(vpn_dict, ::onc::vpn::kAutoConnect);
       vpn->host = GetManagedString(vpn_dict, ::onc::vpn::kHost);
-      vpn->ip_sec = GetManagedIPSecProperties(vpn_dict, ::onc::vpn::kIPsec);
-      vpn->l2tp = GetManagedL2TPProperties(vpn_dict, ::onc::vpn::kL2TP);
-      vpn->open_vpn =
-          GetManagedOpenVPNProperties(vpn_dict, ::onc::vpn::kOpenVPN);
-
-      if (vpn->type == mojom::VpnType::kExtension ||
-          vpn->type == mojom::VpnType::kArc) {
-        const base::Value* third_party_dict =
-            vpn_dict->FindKey(::onc::vpn::kThirdPartyVpn);
-        if (third_party_dict) {
-          vpn->provider_id = GetManagedString(
-              third_party_dict, ::onc::third_party_vpn::kExtensionID);
-          base::Optional<std::string> provider_name = GetString(
-              third_party_dict, ::onc::third_party_vpn::kProviderName);
-          if (provider_name)
-            vpn->provider_name = *provider_name;
-          if (vpn->provider_id && vpn->provider_name.empty()) {
-            vpn->provider_name = GetVpnProviderName(
-                vpn_providers, vpn->provider_id->active_value);
-          }
-        } else {
-          // Lookup VPN provider details from the NetworkState.
-          const NetworkState::VpnProviderInfo* vpn_provider =
-              network_state->vpn_provider();
-          if (vpn_provider) {
-            vpn->provider_id = mojom::ManagedString::New();
-            vpn->provider_id->active_value = vpn_provider->id;
-            vpn->provider_name =
-                GetVpnProviderName(vpn_providers, vpn_provider->id);
+
+      switch (vpn->type) {
+        case mojom::VpnType::kL2TPIPsec:
+          vpn->ip_sec = GetManagedIPSecProperties(vpn_dict, ::onc::vpn::kIPsec);
+          vpn->l2tp = GetManagedL2TPProperties(vpn_dict, ::onc::vpn::kL2TP);
+          break;
+        case mojom::VpnType::kOpenVPN:
+          vpn->open_vpn =
+              GetManagedOpenVPNProperties(vpn_dict, ::onc::vpn::kOpenVPN);
+          break;
+        case mojom::VpnType::kExtension:
+        case mojom::VpnType::kArc:
+          const base::Value* third_party_dict =
+              vpn_dict->FindKey(::onc::vpn::kThirdPartyVpn);
+          if (third_party_dict) {
+            vpn->provider_id = GetManagedString(
+                third_party_dict, ::onc::third_party_vpn::kExtensionID);
+            base::Optional<std::string> provider_name = GetString(
+                third_party_dict, ::onc::third_party_vpn::kProviderName);
+            if (provider_name)
+              vpn->provider_name = *provider_name;
+            if (vpn->provider_id && vpn->provider_name.empty()) {
+              vpn->provider_name = GetVpnProviderName(
+                  vpn_providers, vpn->provider_id->active_value);
+            }
+          } else {
+            // Lookup VPN provider details from the NetworkState.
+            const NetworkState::VpnProviderInfo* vpn_provider =
+                network_state->vpn_provider();
+            if (vpn_provider) {
+              vpn->provider_id = mojom::ManagedString::New();
+              vpn->provider_id->active_value = vpn_provider->id;
+              vpn->provider_name =
+                  GetVpnProviderName(vpn_providers, vpn_provider->id);
+            }
           }
-        }
+          break;
       }
       result->vpn = std::move(vpn);
       break;
@@ -1355,7 +1376,7 @@ mojom::ManagedPropertiesPtr ManagedPropertiesToMojo(
           GetManagedBoolean(wifi_dict, ::onc::wifi::kHiddenSSID);
       wifi->roam_threshold =
           GetManagedInt32(wifi_dict, ::onc::wifi::kRoamThreshold);
-      wifi->ssid = GetManagedString(wifi_dict, ::onc::wifi::kSSID);
+      wifi->ssid = GetRequiredManagedString(wifi_dict, ::onc::wifi::kSSID);
       CHECK(wifi->ssid);
       wifi->signal_strength = GetInt32(wifi_dict, ::onc::wifi::kSignalStrength);
       wifi->tethering_state =

chromeos/services/network_config/cros_network_config_unittest.cc

@@ -608,6 +608,7 @@ TEST_F(CrosNetworkConfigTest, GetManagedProperties) {
             properties->connection_state);
   ASSERT_TRUE(properties->wifi);
   EXPECT_EQ(50, properties->wifi->signal_strength);
+  EXPECT_EQ(mojom::OncSource::kNone, properties->source);
 
   properties = GetManagedProperties("wifi2_guid");
   ASSERT_TRUE(properties);
@@ -618,6 +619,15 @@ TEST_F(CrosNetworkConfigTest, GetManagedProperties) {
   ASSERT_TRUE(properties->wifi);
   EXPECT_EQ(mojom::SecurityType::kWpaPsk, properties->wifi->security);
   EXPECT_EQ(100, properties->wifi->signal_strength);
+  EXPECT_EQ(mojom::OncSource::kUserPolicy, properties->source);
+
+  properties = GetManagedProperties("wifi3_guid");
+  ASSERT_TRUE(properties);
+  EXPECT_EQ("wifi3_guid", properties->guid);
+  EXPECT_EQ(mojom::NetworkType::kWiFi, properties->type);
+  EXPECT_EQ(mojom::ConnectionStateType::kNotConnected,
+            properties->connection_state);
+  EXPECT_EQ(mojom::OncSource::kDevice, properties->source);
 
   properties = GetManagedProperties("cellular_guid");
   ASSERT_TRUE(properties);

ui/webui/resources/cr_components/chromeos/network/network_property_list_mojo.html

@@ -19,7 +19,7 @@
         --cr-input-background-color: transparent;
       }
 
-      cr-policy-network-indicator {
+      cr-policy-network-indicator-mojo {
         margin-inline-start: var(--settings-controlled-by-spacing);
       }
     </style>

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

@@ -167,7 +167,7 @@ Polymer({
         return true;
       }
       const value = this.getPropertyValue_(key, prefix, propertyDict);
-      return value !== undefined && value !== '';
+      return value !== '';
     };
   },
 
@@ -247,14 +247,13 @@ Polymer({
    */
   getProperty_: function(key, propertyDict) {
     const property = this.get(key, this.propertyDict);
-    if ((property === undefined || property === null) &&
-        propertyDict.source !== undefined) {
-      // If the dictionary is policy controlled, provide an empty property
-      // object with the network policy source. See https://crbug.com/819837
-      // for more info.
+    if (property === undefined || property === null) {
       const policySource =
           OncMojo.getEnforcedPolicySourceFromOncSource(propertyDict.source);
       if (policySource != chromeos.networkConfig.mojom.PolicySource.kNone) {
+        // If the dictionary is policy controlled, provide an empty property
+        // object with the network policy source. See https://crbug.com/819837
+        // for more info.
         return /** @type{!OncMojo.ManagedProperty} */ ({
           activeValue: '',
           policySource: policySource,