ArcNetHostImpl: Fix DCHECK and modernize Value code

Previously the translation code assumed that onc::ipconfig::kIPAddress
would never be empty, however that is not enforced in ONC. If the
address is empty it should be treated as if it is not present.

With this change the code will fall back to a Saved or Static IP
Address if the value is missing or if it is empty. If none are
available the IP Address will not be set.

Bug: 902713
Change-Id: I2f9880966661a85a8aace04e56bdac8516e5b3e4
Reviewed-on: https://chromium-review.googlesource.com/c/1379207
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618388}

components/arc/net/arc_net_host_impl.cc

@@ -59,19 +59,25 @@ bool IsDeviceOwner() {
          user_manager::UserManager::Get()->GetOwnerAccountId();
 }
 
-std::string GetStringFromOncDictionary(const base::DictionaryValue* dict,
+std::string GetStringFromONCDictionary(const base::Value* dict,
                                        const char* key,
                                        bool required) {
-  std::string value;
-  dict->GetString(key, &value);
-  if (required && value.empty())
-    NOTREACHED() << "Required parameter " << key << " was not found.";
-  return value;
+  DCHECK(dict->is_dict());
+  const base::Value* string_value =
+      dict->FindKeyOfType(key, base::Value::Type::STRING);
+  if (!string_value) {
+    LOG_IF(ERROR, required) << "Required property " << key << " not found.";
+    return std::string();
+  }
+  std::string result = string_value->GetString();
+  LOG_IF(ERROR, required && result.empty())
+      << "Required property " << key << " is empty.";
+  return result;
 }
 
 arc::mojom::SecurityType TranslateONCWifiSecurityType(
     const base::DictionaryValue* dict) {
-  std::string type = GetStringFromOncDictionary(dict, onc::wifi::kSecurity,
+  std::string type = GetStringFromONCDictionary(dict, onc::wifi::kSecurity,
                                                 true /* required */);
   if (type == onc::wifi::kWEP_PSK)
     return arc::mojom::SecurityType::WEP_PSK;
@@ -103,8 +109,8 @@ arc::mojom::WiFiPtr TranslateONCWifi(const base::DictionaryValue* dict) {
   dict->GetInteger(onc::wifi::kFrequency, &wifi->frequency);
 
   wifi->bssid =
-      GetStringFromOncDictionary(dict, onc::wifi::kBSSID, false /* required */);
-  wifi->hex_ssid = GetStringFromOncDictionary(dict, onc::wifi::kHexSSID,
+      GetStringFromONCDictionary(dict, onc::wifi::kBSSID, false /* required */);
+  wifi->hex_ssid = GetStringFromONCDictionary(dict, onc::wifi::kHexSSID,
                                               true /* required */);
 
   // Optional; defaults to false.
@@ -126,68 +132,82 @@ arc::mojom::TetheringClientState GetWifiTetheringClientState(
   return TranslateTetheringState(tethering_state);
 }
 
-std::vector<std::string> TranslateStringArray(const base::ListValue* list) {
-  std::vector<std::string> strings;
-
-  for (size_t i = 0; i < list->GetSize(); i++) {
-    std::string value;
-    list->GetString(i, &value);
-    DCHECK(!value.empty());
-    strings.push_back(value);
-  }
-
-  return strings;
-}
-
 arc::mojom::IPConfigurationPtr TranslateONCIPConfig(
-    const base::DictionaryValue* ip_dict) {
+    const base::Value* ip_dict) {
+  DCHECK(ip_dict->is_dict());
+
   arc::mojom::IPConfigurationPtr configuration =
       arc::mojom::IPConfiguration::New();
 
-  if (ip_dict->FindKey(onc::ipconfig::kIPAddress)) {
-    configuration->ip_address = GetStringFromOncDictionary(
-        ip_dict, onc::ipconfig::kIPAddress, true /* required */);
-    if (!ip_dict->GetInteger(onc::ipconfig::kRoutingPrefix,
-                             &configuration->routing_prefix)) {
-      NOTREACHED();
-    }
-    configuration->gateway = GetStringFromOncDictionary(
+  const base::Value* ip_address = ip_dict->FindKeyOfType(
+      onc::ipconfig::kIPAddress, base::Value::Type::STRING);
+  if (ip_address && !ip_address->GetString().empty()) {
+    configuration->ip_address = ip_address->GetString();
+    const base::Value* routing_prefix = ip_dict->FindKeyOfType(
+        onc::ipconfig::kRoutingPrefix, base::Value::Type::INTEGER);
+    if (routing_prefix)
+      configuration->routing_prefix = routing_prefix->GetInt();
+    else
+      LOG(ERROR) << "Required property RoutingPrefix not found.";
+    configuration->gateway = GetStringFromONCDictionary(
         ip_dict, onc::ipconfig::kGateway, true /* required */);
   }
 
-  const base::ListValue* dns_list;
-  if (ip_dict->GetList(onc::ipconfig::kNameServers, &dns_list))
-    configuration->name_servers = TranslateStringArray(dns_list);
+  const base::Value* name_servers = ip_dict->FindKeyOfType(
+      onc::ipconfig::kNameServers, base::Value::Type::LIST);
+  if (name_servers) {
+    for (const auto& entry : name_servers->GetList())
+      configuration->name_servers.push_back(entry.GetString());
+  }
 
-  std::string type = GetStringFromOncDictionary(ip_dict, onc::ipconfig::kType,
-                                                true /* required */);
-  configuration->type = type == onc::ipconfig::kIPv6
+  const base::Value* type =
+      ip_dict->FindKeyOfType(onc::ipconfig::kType, base::Value::Type::STRING);
+  configuration->type = type && type->GetString() == onc::ipconfig::kIPv6
                             ? arc::mojom::IPAddressType::IPV6
                             : arc::mojom::IPAddressType::IPV4;
 
-  configuration->web_proxy_auto_discovery_url = GetStringFromOncDictionary(
+  configuration->web_proxy_auto_discovery_url = GetStringFromONCDictionary(
       ip_dict, onc::ipconfig::kWebProxyAutoDiscoveryUrl, false /* required */);
 
   return configuration;
 }
 
-std::vector<arc::mojom::IPConfigurationPtr> TranslateONCIPConfigs(
-    const base::ListValue* list) {
-  std::vector<arc::mojom::IPConfigurationPtr> configs;
-
-  for (size_t i = 0; i < list->GetSize(); i++) {
-    const base::DictionaryValue* ip_dict = nullptr;
-
-    list->GetDictionary(i, &ip_dict);
-    DCHECK(ip_dict);
-    configs.push_back(TranslateONCIPConfig(ip_dict));
+// Returns an IPConfiguration vector from the IPConfigs ONC property, which may
+// include multiple IP configurations (e.g. IPv4 and IPv6).
+std::vector<arc::mojom::IPConfigurationPtr> IPConfigurationsFromONCIPConfigs(
+    const base::Value* dict) {
+  const base::Value* ip_config_list =
+      dict->FindKey(onc::network_config::kIPConfigs);
+  if (!ip_config_list || !ip_config_list->is_list())
+    return {};
+  std::vector<arc::mojom::IPConfigurationPtr> result;
+  for (const auto& entry : ip_config_list->GetList()) {
+    arc::mojom::IPConfigurationPtr config = TranslateONCIPConfig(&entry);
+    if (config)
+      result.push_back(std::move(config));
   }
-  return configs;
+  return result;
+}
+
+// Returns an IPConfiguration vector from ONC property |property|, which will
+// include a single IP configuration.
+std::vector<arc::mojom::IPConfigurationPtr> IPConfigurationsFromONCProperty(
+    const base::Value* dict,
+    const char* property_key) {
+  const base::Value* ip_dict = dict->FindKey(property_key);
+  if (!ip_dict)
+    return {};
+  arc::mojom::IPConfigurationPtr config = TranslateONCIPConfig(ip_dict);
+  if (!config)
+    return {};
+  std::vector<arc::mojom::IPConfigurationPtr> result;
+  result.push_back(std::move(config));
+  return result;
 }
 
 arc::mojom::ConnectionStateType TranslateONCConnectionState(
     const base::DictionaryValue* dict) {
-  std::string connection_state = GetStringFromOncDictionary(
+  std::string connection_state = GetStringFromONCDictionary(
       dict, onc::network_config::kConnectionState, false /* required */);
 
   if (connection_state == onc::connection_state::kConnected)
@@ -199,7 +219,7 @@ arc::mojom::ConnectionStateType TranslateONCConnectionState(
 
 void TranslateONCNetworkTypeDetails(const base::DictionaryValue* dict,
                                     arc::mojom::NetworkConfiguration* mojo) {
-  std::string type = GetStringFromOncDictionary(
+  std::string type = GetStringFromONCDictionary(
       dict, onc::network_config::kType, true /* required */);
   // This property will be updated as required by the relevant network types
   // below.
@@ -231,28 +251,28 @@ arc::mojom::NetworkConfigurationPtr TranslateONCConfiguration(
 
   mojo->connection_state = TranslateONCConnectionState(dict);
 
-  mojo->guid = GetStringFromOncDictionary(dict, onc::network_config::kGUID,
+  mojo->guid = GetStringFromONCDictionary(dict, onc::network_config::kGUID,
                                           true /* required */);
 
   // crbug.com/761708 - VPNs do not currently have an IPConfigs array,
   // so in order to fetch the parameters (particularly the DNS server list),
   // fall back to StaticIPConfig or SavedIPConfig.
-  const base::ListValue* ip_config_list = nullptr;
-  const base::DictionaryValue* ip_dict = nullptr;
-  if (dict->GetList(onc::network_config::kIPConfigs, &ip_config_list)) {
-    mojo->ip_configs = TranslateONCIPConfigs(ip_config_list);
-  } else if (dict->GetDictionary(onc::network_config::kStaticIPConfig,
-                                 &ip_dict) ||
-             dict->GetDictionary(onc::network_config::kSavedIPConfig,
-                                 &ip_dict)) {
-    std::vector<arc::mojom::IPConfigurationPtr> configs;
-    configs.push_back(TranslateONCIPConfig(ip_dict));
-    mojo->ip_configs = std::move(configs);
-  }
-
-  mojo->guid = GetStringFromOncDictionary(dict, onc::network_config::kGUID,
+  std::vector<arc::mojom::IPConfigurationPtr> ip_configs =
+      IPConfigurationsFromONCIPConfigs(dict);
+  if (ip_configs.empty()) {
+    ip_configs = IPConfigurationsFromONCProperty(
+        dict, onc::network_config::kStaticIPConfig);
+  }
+  if (ip_configs.empty()) {
+    ip_configs = IPConfigurationsFromONCProperty(
+        dict, onc::network_config::kSavedIPConfig);
+  }
+  if (!ip_configs.empty())
+    mojo->ip_configs = std::move(ip_configs);
+
+  mojo->guid = GetStringFromONCDictionary(dict, onc::network_config::kGUID,
                                           true /* required */);
-  mojo->mac_address = GetStringFromOncDictionary(
+  mojo->mac_address = GetStringFromONCDictionary(
       dict, onc::network_config::kMacAddress, false /* required */);
   TranslateONCNetworkTypeDetails(dict, mojo.get());