Use Managed properties for Preferred and Provider.

chromeos/network/onc/onc_translator_shill_to_onc.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chromeos/network/onc/onc_translator.h"
 
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/json/json_reader.h"
@@ skipping 71 matching lines @@
   // associated to |onc_field_name| and adds it to |onc_object_| at
   // |onc_field_name|.
   void TranslateAndAddNestedObject(const std::string& onc_field_name,
                                    const base::DictionaryValue& dictionary);
 
   // Creates an ONC object from |shill_dictionary_| according to the signature
   // associated to |onc_field_name| and adds it to |onc_object_| at
   // |onc_field_name|.
   void TranslateAndAddNestedObject(const std::string& onc_field_name);
 
+  // Sets |onc_field_name| in dictionary |onc_dictionary_name| in |onc_object_|
+  // to |value| if the dictionary exists.
+  void SetNestedOncValue(const std::string& onc_dictionary_name,
+                         const std::string& onc_field_name,
+                         const base::Value& value);
+
   // Translates a list of nested objects and adds the list to |onc_object_| at
   // |onc_field_name|. If there are errors while parsing individual objects or
   // if the resulting list contains no entries, the result will not be added to
   // |onc_object_|.
   void TranslateAndAddListOfObjects(const std::string& onc_field_name,
                                     const base::ListValue& list);
 
   // Applies function CopyProperty to each field of |value_signature| and its
   // base signatures.
   void CopyPropertiesAccordingToSignature(
@@ skipping 73 matching lines @@
           shill::kOpenVPNRemoteCertKUProperty, &certKU)) {
     scoped_ptr<base::ListValue> certKUs(new base::ListValue);
     certKUs->AppendString(certKU);
     onc_object_->SetWithoutPathExpansion(::onc::openvpn::kRemoteCertKU,
                                          certKUs.release());
   }
 
   for (const OncFieldSignature* field_signature = onc_signature_->fields;
        field_signature->onc_field_name != NULL; ++field_signature) {
     const std::string& onc_field_name = field_signature->onc_field_name;
-    if (onc_field_name == ::onc::vpn::kSaveCredentials ||
-        onc_field_name == ::onc::openvpn::kRemoteCertKU ||
+    if (onc_field_name == ::onc::openvpn::kRemoteCertKU ||
         onc_field_name == ::onc::openvpn::kServerCAPEMs) {
       CopyProperty(field_signature);
       continue;
     }
 
     std::string shill_property_name;
     const base::Value* shill_value = NULL;
     if (!field_translation_table_ ||
         !GetShillPropertyName(field_signature->onc_field_name,
                               field_translation_table_,
@@ skipping 29 matching lines @@
   }
 }
 
 void ShillToONCTranslator::TranslateIPsec() {
   CopyPropertiesAccordingToSignature();
   if (shill_dictionary_->HasKey(shill::kL2tpIpsecXauthUserProperty))
     TranslateAndAddNestedObject(::onc::ipsec::kXAUTH);
 }
 
 void ShillToONCTranslator::TranslateVPN() {
-  TranslateWithTableAndSet(
-      shill::kProviderTypeProperty, kVPNTypeTable, ::onc::vpn::kType);
   CopyPropertiesAccordingToSignature();
 
-  std::string vpn_type;
-  if (onc_object_->GetStringWithoutPathExpansion(::onc::vpn::kType,
-                                                 &vpn_type)) {
-    if (vpn_type == ::onc::vpn::kTypeL2TP_IPsec) {
-      TranslateAndAddNestedObject(::onc::vpn::kIPsec);
-      TranslateAndAddNestedObject(::onc::vpn::kL2TP);
-    } else {
-      TranslateAndAddNestedObject(vpn_type);
+  // Parse Shill Provider dictionary.
+  const base::DictionaryValue* provider = NULL;
+  if (!shill_dictionary_->GetDictionaryWithoutPathExpansion(
+          shill::kProviderProperty, &provider)) {
+    LOG(ERROR) << "Shill VPN object with no Provider dictionary";
+    return;
+  }
+  std::string shill_provider_type, onc_provider_type;
+  if (!provider->GetStringWithoutPathExpansion(shill::kTypeProperty,
+                                               &shill_provider_type)) {
+    LOG(ERROR) << "Shill VPN object with no Provider.Type";

[pneubeck (no reviews), 2014/08/20 08:16:16]

alternatively, instead of catching a missing input (which will be handled by
TranslateStringToONC as well),
you could check the result of TranslateStringToONC, to also catch unknown Shill
types.

[stevenjb, 2014/08/20 17:18:37]

Done.

+    return;
+  }
+  TranslateStringToONC(kVPNTypeTable, shill_provider_type, &onc_provider_type);
+  onc_object_->SetStringWithoutPathExpansion(::onc::vpn::kType,
+                                             onc_provider_type);
+  std::string provider_host;
+  if (provider->GetStringWithoutPathExpansion(shill::kHostProperty,

[pneubeck (no reviews), 2014/08/20 08:16:16]

The translation tables were meant for the cases where the translation is
symmetric.

please comment out
  { ::onc::vpn::kHost, shill::kProviderHostProperty},
in
  vpn_fields in onc_translation_tables.cc
with a note
 // Is handled differently for Shill -> ONC, see onc_translator_*. )

and add the translation in the other direction explicitly to TranslateVPN() in
onc_translator_onc_to_shill.cc:

std::string host;
if (onc_object_->GetStringWithoutPathExpansion(::onc::vpn::kHost, &host) {
  shill_dictionary_->SetStringWithoutPathExpansion(shill::kHostProperty, host);
}

[stevenjb, 2014/08/20 17:18:37]

Ugh. Yeah, I understand your point, but this is tedious. It seems like we have
enough special cases at this point to provide some better support for asymmetry
and nested dictionaries. Another day. Done.

[pneubeck (no reviews), 2014/08/20 19:49:36]

Yes, it's tedious. To get rid of it, I would prefer to work on the root cause
and align ONC and the format of Shill. Neither should be seen as static.

+                                              &provider_host)) {
+    onc_object_->SetStringWithoutPathExpansion(::onc::vpn::kHost,
+                                               provider_host);
+  }
+
+  const base::Value* save_credentials = NULL;
+  shill_dictionary_->GetWithoutPathExpansion(shill::kSaveCredentialsProperty,

[pneubeck (no reviews), 2014/08/20 08:16:16]

like the Host property above,
please comment out in
  openvpn_fields and ipsec_fields

and add it explicitly to TranslateOpenVPN and to a new TranslateIPsec.

[stevenjb, 2014/08/20 17:18:37]

Done.

+                                             &save_credentials);
+  if (save_credentials && !save_credentials->IsType(base::Value::TYPE_BOOLEAN))
+    save_credentials = NULL;
+
+  // Translate the nested dictionary.
+  if (onc_provider_type == ::onc::vpn::kTypeL2TP_IPsec) {
+    TranslateAndAddNestedObject(::onc::vpn::kIPsec, *provider);
+    TranslateAndAddNestedObject(::onc::vpn::kL2TP, *provider);
+    if (save_credentials) {
+      SetNestedOncValue(
+          ::onc::vpn::kIPsec, ::onc::vpn::kSaveCredentials, *save_credentials);
+      SetNestedOncValue(

[pneubeck (no reviews), 2014/08/20 08:16:16]

nit: wasn't set before and could be removed.

[stevenjb, 2014/08/20 17:18:37]

Done.

+          ::onc::vpn::kL2TP, ::onc::vpn::kSaveCredentials, *save_credentials);
+    }
+  } else {
+    TranslateAndAddNestedObject(onc_provider_type, *provider);
+    if (save_credentials) {
+      SetNestedOncValue(
+          onc_provider_type, ::onc::vpn::kSaveCredentials, *save_credentials);
     }
   }
 }
 
 void ShillToONCTranslator::TranslateWiFiWithState() {
   TranslateWithTableAndSet(
       shill::kSecurityProperty, kWiFiSecurityTable, ::onc::wifi::kSecurity);
   std::string ssid = shill_property_util::GetSSIDFromProperties(
       *shill_dictionary_, NULL /* ignore unknown encoding */);
   if (!ssid.empty())
@@ skipping 139 matching lines @@
     const std::string& onc_field_name,
     const base::DictionaryValue& dictionary) {
   const OncFieldSignature* field_signature =
       GetFieldSignature(*onc_signature_, onc_field_name);
   DCHECK(field_signature) << "Unable to find signature for field "
                           << onc_field_name << ".";
   ShillToONCTranslator nested_translator(dictionary,
                                          *field_signature->value_signature);
   scoped_ptr<base::DictionaryValue> nested_object =
       nested_translator.CreateTranslatedONCObject();
-  if (nested_object->empty())
-    return;
+  // Note: this may create an empty nested object which is necessary in case

[pneubeck (no reviews), 2014/08/20 08:16:16]

Instead of potentially creating empty nested objects, SetNestedOncValue could
create the missing Dictionary.

[stevenjb, 2014/08/20 17:18:38]

I was trying to avoid code duplication since an empty dictionary should be
harmless, but done.

+  // properties are added later with SetNestedOncValue.
   onc_object_->SetWithoutPathExpansion(onc_field_name, nested_object.release());
 }
 
+void ShillToONCTranslator::SetNestedOncValue(
+    const std::string& onc_dictionary_name,
+    const std::string& onc_field_name,
+    const base::Value& value) {
+  base::DictionaryValue* nested;
+  if (!onc_object_->GetDictionaryWithoutPathExpansion(
+          onc_dictionary_name, &nested)) {
+    NOTREACHED();
+  }
+  nested->SetWithoutPathExpansion(onc_field_name, value.DeepCopy());
+}
+
 void ShillToONCTranslator::TranslateAndAddListOfObjects(
     const std::string& onc_field_name,
     const base::ListValue& list) {
   const OncFieldSignature* field_signature =
       GetFieldSignature(*onc_signature_, onc_field_name);
   if (field_signature->value_signature->onc_type != base::Value::TYPE_LIST) {
     LOG(ERROR) << "ONC Field name: '" << onc_field_name << "' has type '"
                << field_signature->value_signature->onc_type
                << "', expected: base::Value::TYPE_LIST.";
     return;
@@ skipping 86 matching lines @@
     const base::DictionaryValue& shill_dictionary,
     const OncValueSignature* onc_signature) {
   CHECK(onc_signature != NULL);
 
   ShillToONCTranslator translator(shill_dictionary, *onc_signature);
   return translator.CreateTranslatedONCObject();
 }
 
 }  // namespace onc
 }  // namespace chromeos