ONC: Use HexSSID instead of SSID.

- Comparison of a network policy with network will use HexSSID now.
- Translation from ONC to Shill now copies the HexSSID instead of translating SSID.
- Fix ONC validation of HexSSID/SSID to be case insensitive.
- When setting an ONC: SSID will be ignored if HexSSID is present
- Reading network properties reports a HexSSID in ONC.

BUG=432546

Review URL: https://codereview.chromium.org/823633004

Cr-Commit-Position: refs/heads/master@{#311281}

8 files changed, 74 insertions, 45 deletions

diff --git a/chromeos/network/onc/onc_translation_tables.cc b/chromeos/network/onc/onc_translation_tables.cc
index 6d9ade3..0b3a07e 100644
--- a/chromeos/network/onc/onc_translation_tables.cc
+++ b/chromeos/network/onc/onc_translation_tables.cc
@@ -115,11 +115,10 @@ const FieldTranslationEntry wifi_fields[] = {
     // { ::onc::wifi::kEAP, shill::kEap*},
     { ::onc::wifi::kFrequency, shill::kWifiFrequency},
     { ::onc::wifi::kFrequencyList, shill::kWifiFrequencyListProperty},
+    { ::onc::wifi::kHexSSID, shill::kWifiHexSsid},
     { ::onc::wifi::kHiddenSSID, shill::kWifiHiddenSsid},
     { ::onc::wifi::kPassphrase, shill::kPassphraseProperty},
     // This field is converted during translation, see onc_translator_*.
-    // { ::onc::wifi::kSSID, shill::kWifiHexSsid},
-    // This field is converted during translation, see onc_translator_*.
     // { ::onc::wifi::kSecurity, shill::kSecurityClassProperty },
     { ::onc::wifi::kSignalStrength, shill::kSignalStrengthProperty},
     {NULL}};
diff --git a/chromeos/network/onc/onc_translator_onc_to_shill.cc b/chromeos/network/onc/onc_translator_onc_to_shill.cc
index c9d4151..9e1e53e 100644
--- a/chromeos/network/onc/onc_translator_onc_to_shill.cc
+++ b/chromeos/network/onc/onc_translator_onc_to_shill.cc
@@ -204,11 +204,6 @@ void LocalTranslator::TranslateWiFi() {
                              shill::kSecurityClassProperty);
   }
 
-  std::string ssid;
-  onc_object_->GetStringWithoutPathExpansion(::onc::wifi::kSSID, &ssid);
-  if (!ssid.empty())
-    shill_property_util::SetSSID(ssid, shill_dictionary_);
-
   // We currently only support managed and no adhoc networks.
   shill_dictionary_->SetStringWithoutPathExpansion(shill::kModeProperty,
                                                    shill::kModeManaged);
diff --git a/chromeos/network/onc/onc_translator_shill_to_onc.cc b/chromeos/network/onc/onc_translator_shill_to_onc.cc
index 1e7885f..4360d25 100644
--- a/chromeos/network/onc/onc_translator_shill_to_onc.cc
+++ b/chromeos/network/onc/onc_translator_shill_to_onc.cc
@@ -316,9 +316,10 @@ void ShillToONCTranslator::TranslateWiFiWithState() {
   TranslateWithTableAndSet(shill::kSecurityClassProperty,
                            kWiFiSecurityTable,
                            ::onc::wifi::kSecurity);
+  bool unknown_encoding = true;
   std::string ssid = shill_property_util::GetSSIDFromProperties(
-      *shill_dictionary_, NULL /* ignore unknown encoding */);
-  if (!ssid.empty())
+      *shill_dictionary_, &unknown_encoding);
+  if (!unknown_encoding && !ssid.empty())
     onc_object_->SetStringWithoutPathExpansion(::onc::wifi::kSSID, ssid);
 
   bool link_monitor_disable;
@@ -663,8 +664,8 @@ void ShillToONCTranslator::CopyProperty(
     return;
   }
 
- onc_object_->SetWithoutPathExpansion(field_signature->onc_field_name,
-                                      shill_value->DeepCopy());
+  onc_object_->SetWithoutPathExpansion(field_signature->onc_field_name,
+                                       shill_value->DeepCopy());
 }
 
 void ShillToONCTranslator::TranslateWithTableAndSet(
diff --git a/chromeos/network/onc/onc_translator_unittest.cc b/chromeos/network/onc/onc_translator_unittest.cc
index 5d4f3ce..9444614 100644
--- a/chromeos/network/onc/onc_translator_unittest.cc
+++ b/chromeos/network/onc/onc_translator_unittest.cc
@@ -4,6 +4,9 @@
 
 #include "chromeos/network/onc/onc_translator.h"
 
+#include <string>
+#include <utility>
+
 #include "base/memory/scoped_ptr.h"
 #include "base/values.h"
 #include "chromeos/network/onc/onc_signature.h"
@@ -97,6 +100,8 @@ INSTANTIATE_TEST_CASE_P(
                        "translation_of_shill_ethernet_with_ipconfig.onc"),
         std::make_pair("shill_wifi_clientcert.json",
                        "translation_of_shill_wifi_clientcert.onc"),
+        std::make_pair("shill_wifi_non_utf8_ssid.json",
+                       "translation_of_shill_wifi_non_utf8_ssid.onc"),
         std::make_pair("shill_output_l2tpipsec.json",
                        "translation_of_shill_l2tpipsec.onc"),
         std::make_pair("shill_output_openvpn.json",
diff --git a/chromeos/network/onc/onc_utils.cc b/chromeos/network/onc/onc_utils.cc
index 7b544cf..1eaee85 100644
--- a/chromeos/network/onc/onc_utils.cc
+++ b/chromeos/network/onc/onc_utils.cc
@@ -380,6 +380,16 @@ CertPEMsByGUIDMap GetServerAndCACertsByGUID(
   return certs_by_guid;
 }
 
+void FillInHexSSIDFieldsInNetworks(base::ListValue* network_configs) {
+  for (base::ListValue::iterator it = network_configs->begin();
+       it != network_configs->end(); ++it) {
+    base::DictionaryValue* network = NULL;
+    (*it)->GetAsDictionary(&network);
+    DCHECK(network);
+    FillInHexSSIDFieldsInOncObject(kNetworkConfigurationSignature, network);
+  }
+}
+
 }  // namespace
 
 bool ParseAndValidateOncForImport(const std::string& onc_blob,
@@ -432,9 +442,6 @@ bool ParseAndValidateOncForImport(const std::string& onc_blob,
       *toplevel_onc,
       &validation_result);
 
-  FillInHexSSIDFieldsInOncObject(kToplevelConfigurationSignature,
-                                 toplevel_onc.get());
-
   if (from_policy) {
     UMA_HISTOGRAM_BOOLEAN("Enterprise.ONC.PolicyValidation",
                           validation_result == Validator::VALID);
@@ -460,6 +467,8 @@ bool ParseAndValidateOncForImport(const std::string& onc_blob,
   base::ListValue* validated_networks = NULL;
   if (toplevel_onc->GetListWithoutPathExpansion(
           toplevel_config::kNetworkConfigurations, &validated_networks)) {
+    FillInHexSSIDFieldsInNetworks(validated_networks);
+
     CertPEMsByGUIDMap server_and_ca_certs =
         GetServerAndCACertsByGUID(*certificates);
 
diff --git a/chromeos/network/onc/onc_validator.cc b/chromeos/network/onc/onc_validator.cc
index a3374ee..d9c6263 100644
--- a/chromeos/network/onc/onc_validator.cc
+++ b/chromeos/network/onc/onc_validator.cc
@@ -253,7 +253,7 @@ bool Validator::ValidateRecommendedField(
   for (const base::Value* entry : *recommended_list) {
     std::string field_name;
     if (!entry->GetAsString(&field_name)) {
-      NOTREACHED(); // The types of field values are already verified.
+      NOTREACHED();  // The types of field values are already verified.
       continue;
     }
 
@@ -399,51 +399,62 @@ bool Validator::FieldExistsAndIsEmpty(const base::DictionaryValue& object,
   return true;
 }
 
-bool Validator::IsSSIDOrHexSSIDValid(const base::DictionaryValue& object) {
+bool Validator::ValidateSSIDAndHexSSID(base::DictionaryValue* object) {
   // Check SSID validity.
   std::string ssid_string;
-  if (object.GetStringWithoutPathExpansion(::onc::wifi::kSSID, &ssid_string)) {
-    if (ssid_string.size() <= 0 ||
-        ssid_string.size() > kMaximumSSIDLengthInBytes) {
-      LOG(ERROR) << MessageHeader() << ::onc::wifi::kSSID
-                 << " has an invalid length.";
-      error_or_warning_found_ = true;
+  if (object->GetStringWithoutPathExpansion(::onc::wifi::kSSID, &ssid_string) &&
+      (ssid_string.size() <= 0 ||
+       ssid_string.size() > kMaximumSSIDLengthInBytes)) {
+    error_or_warning_found_ = true;
+    const std::string msg =
+        MessageHeader() + ::onc::wifi::kSSID + " has an invalid length.";
+    // If the HexSSID field is present, ignore errors in SSID because these
+    // might be caused by the usage of a non-UTF-8 encoding when the SSID
+    // field was automatically added (see FillInHexSSIDField).
+    if (object->HasKey(::onc::wifi::kHexSSID)) {
+      LOG(WARNING) << msg;
+    } else {
+      LOG(ERROR) << msg;
       return false;
     }
   }
 
   // Check HexSSID validity.
   std::string hex_ssid_string;
-  if (object.GetStringWithoutPathExpansion(::onc::wifi::kHexSSID,
-                                           &hex_ssid_string)) {
-    std::vector<uint8> bytes;
-    if (!base::HexStringToBytes(hex_ssid_string, &bytes)) {
+  if (object->GetStringWithoutPathExpansion(::onc::wifi::kHexSSID,
+                                            &hex_ssid_string)) {
+    std::vector<uint8> decoded_ssid;
+    if (!base::HexStringToBytes(hex_ssid_string, &decoded_ssid)) {
       LOG(ERROR) << MessageHeader() << "Field " << ::onc::wifi::kHexSSID
                  << " is not a valid hex representation: \"" << hex_ssid_string
                  << "\"";
       error_or_warning_found_ = true;
       return false;
     }
-    if (bytes.size() <= 0 || bytes.size() > kMaximumSSIDLengthInBytes) {
+    if (decoded_ssid.size() <= 0 ||
+        decoded_ssid.size() > kMaximumSSIDLengthInBytes) {
       LOG(ERROR) << MessageHeader() << ::onc::wifi::kHexSSID
                  << " has an invalid length.";
       error_or_warning_found_ = true;
       return false;
     }
-  }
-  // If both SSID and HexSSID are set, ensure that they are consistent.
-  if (ssid_string.length() > 0 && hex_ssid_string.length() > 0) {
-    std::string hexified =
-        base::HexEncode(ssid_string.c_str(), ssid_string.size());
-    if (hexified != hex_ssid_string) {
-      LOG(ERROR) << MessageHeader() << "Fields " << ::onc::wifi::kSSID
-                 << " and " << ::onc::wifi::kHexSSID
-                 << " contain inconsistent values.";
-      error_or_warning_found_ = true;
-      return false;
+
+    // If both SSID and HexSSID are set, check whether they are consistent, i.e.
+    // HexSSID contains the UTF-8 encoding of SSID. If not, remove the SSID
+    // field.
+    if (ssid_string.length() > 0) {
+      std::string decoded_ssid_string(
+          reinterpret_cast<const char*>(&decoded_ssid[0]), decoded_ssid.size());
+      if (ssid_string != decoded_ssid_string) {
+        LOG(WARNING) << MessageHeader() << "Fields " << ::onc::wifi::kSSID
+                     << " and " << ::onc::wifi::kHexSSID
+                     << " contain inconsistent values. Removing "
+                     << ::onc::wifi::kSSID << ".";
+        error_or_warning_found_ = true;
+        object->RemoveWithoutPathExpansion(::onc::wifi::kSSID, nullptr);
+      }
     }
   }
-
   return true;
 }
 
@@ -645,8 +656,7 @@ bool Validator::ValidateWiFi(base::DictionaryValue* result) {
   if (FieldExistsAndHasNoValidValue(*result, kSecurity, valid_securities))
     return false;
 
-  // Validate SSID and HexSSID fields, if present.
-  if (!IsSSIDOrHexSSIDValid(*result))
+  if (!ValidateSSIDAndHexSSID(result))
     return false;
 
   bool all_required_exist = RequireField(*result, kSecurity);
diff --git a/chromeos/network/onc/onc_validator.h b/chromeos/network/onc/onc_validator.h
index 857f31e..5a5cf6a 100644
--- a/chromeos/network/onc/onc_validator.h
+++ b/chromeos/network/onc/onc_validator.h
@@ -182,7 +182,7 @@ class CHROMEOS_EXPORT Validator : public Mapper {
   bool FieldExistsAndIsEmpty(const base::DictionaryValue& object,
                              const std::string& field_name);
 
-  bool IsSSIDOrHexSSIDValid(const base::DictionaryValue& object);
+  bool ValidateSSIDAndHexSSID(base::DictionaryValue* object);
 
   // Returns true if |key| is a key of |dict|. Otherwise, returns false and,
   // depending on |error_on_missing_field_|, logs a message and sets
diff --git a/chromeos/network/onc/onc_validator_unittest.cc b/chromeos/network/onc/onc_validator_unittest.cc
index 4daa6a6..ad15269 100644
--- a/chromeos/network/onc/onc_validator_unittest.cc
+++ b/chromeos/network/onc/onc_validator_unittest.cc
@@ -428,6 +428,16 @@ INSTANTIATE_TEST_CASE_P(
                                  false,
                                  ::onc::ONC_SOURCE_DEVICE_POLICY),
                        ExpectBothNotValid("toplevel-empty", "toplevel-empty")),
+        std::make_pair(OncParams("wifi-ssid-and-hexssid-inconsistent",
+                                 &kNetworkConfigurationSignature,
+                                 false),
+                       ExpectBothNotValid("wifi-ssid-and-hexssid-repaired",
+                                          "wifi-ssid-and-hexssid-repaired")),
+        std::make_pair(OncParams("wifi-ssid-and-hexssid-partially-invalid",
+                                 &kNetworkConfigurationSignature,
+                                 false),
+                       ExpectBothNotValid("wifi-ssid-and-hexssid-repaired",
+                                          "wifi-ssid-and-hexssid-repaired")),
         std::make_pair(
             OncParams("toplevel-with-server-and-ca-cert",
                       &kToplevelConfigurationSignature,
@@ -455,15 +465,15 @@ INSTANTIATE_TEST_CASE_P(
                                  &kNetworkConfigurationSignature,
                                  false),
                        ExpectBothNotValid("", "")),
-        std::make_pair(OncParams("network-wifi-hexssid-invalid-length",
+        std::make_pair(OncParams("wifi-hexssid-invalid-length",
                                  &kNetworkConfigurationSignature,
                                  false),
                        ExpectBothNotValid("", "")),
-        std::make_pair(OncParams("network-wifi-invalid-hexssid",
+        std::make_pair(OncParams("wifi-ssid-invalid-length",
                                  &kNetworkConfigurationSignature,
                                  false),
                        ExpectBothNotValid("", "")),
-        std::make_pair(OncParams("network-wifi-ssid-and-hexssid-inconsistent",
+        std::make_pair(OncParams("wifi-invalid-hexssid",
                                  &kNetworkConfigurationSignature,
                                  false),
                        ExpectBothNotValid("", "")),