Use Managed properties for Preferred and Provider.

chrome/browser/ui/webui/options/chromeos/internet_options_handler.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 "chrome/browser/ui/webui/options/chromeos/internet_options_handler.h"
 
 #include <ctype.h>
 
 #include <map>
 #include <string>
@@ skipping 25 matching lines @@
 #include "chromeos/network/network_connection_handler.h"
 #include "chromeos/network/network_device_handler.h"
 #include "chromeos/network/network_event_log.h"
 #include "chromeos/network/network_ip_config.h"
 #include "chromeos/network/network_profile.h"
 #include "chromeos/network/network_profile_handler.h"
 #include "chromeos/network/network_state.h"
 #include "chromeos/network/network_state_handler.h"
 #include "chromeos/network/network_util.h"
 #include "chromeos/network/onc/onc_signature.h"
+#include "chromeos/network/onc/onc_translation_tables.h"
 #include "chromeos/network/onc/onc_translator.h"
 #include "chromeos/network/onc/onc_utils.h"
 #include "components/onc/onc_constants.h"
 #include "content/public/browser/user_metrics.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_ui.h"
 #include "grit/ash_resources.h"
 #include "grit/ui_chromeos_resources.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/base/webui/web_ui_util.h"
 #include "ui/chromeos/network/network_icon.h"
 #include "ui/gfx/image/image_skia.h"
 
 namespace chromeos {
 namespace options {
 
 namespace {
 
+// The key in a Managed Value dictionary for translated values.
+// TODO(stevenjb): Consider making this part of the ONC spec.
+const char kTranslatedKey[] = "Translated";
+
 // Keys for the network description dictionary passed to the web ui. Make sure
 // to keep the strings in sync with what the JavaScript side uses.
 const char kNetworkInfoKeyIconURL[] = "iconURL";
 const char kNetworkInfoKeyServicePath[] = "servicePath";
 const char kNetworkInfoKeyPolicyManaged[] = "policyManaged";
 
 // These are keys for getting IP information from the web ui.
 const char kIpConfigAddress[] = "address";
 const char kIpConfigPrefixLength[] = "prefixLength";
 const char kIpConfigNetmask[] = "netmask";
@@ skipping 67 matching lines @@
 const char kTagLanguage[] = "language";
 const char kTagLastGoodApn[] = "lastGoodApn";
 const char kTagLocalizedName[] = "localizedName";
 const char kTagName[] = "name";
 const char kTagNameServersGoogle[] = "nameServersGoogle";
 const char kTagNameServerType[] = "nameServerType";
 const char kTagNetworkId[] = "networkId";
 const char kTagOptions[] = "options";
 const char kTagPassword[] = "password";
 const char kTagPolicy[] = "policy";
-const char kTagPreferred[] = "preferred";
-const char kTagProviderType[] = "providerType";
 const char kTagProviderApnList[] = "providerApnList";
 const char kTagRecommended[] = "recommended";
 const char kTagRecommendedValue[] = "recommendedValue";
 const char kTagRemembered[] = "remembered";
 const char kTagRememberedList[] = "rememberedList";
 const char kTagRestrictedPool[] = "restrictedPool";
 const char kTagRoamingState[] = "roamingState";
-const char kTagServerHostname[] = "serverHostname";
 const char kTagCarriers[] = "carriers";
 const char kTagCurrentCarrierIndex[] = "currentCarrierIndex";
 const char kTagShared[] = "shared";
 const char kTagShowActivateButton[] = "showActivateButton";
 const char kTagShowViewAccountButton[] = "showViewAccountButton";
 const char kTagSimCardLockEnabled[] = "simCardLockEnabled";
 const char kTagSupportUrl[] = "supportUrl";
 const char kTagTrue[] = "true";
 const char kTagUsername[] = "username";
 const char kTagValue[] = "value";
@@ skipping 158 matching lines @@
   if (onc_source == ::onc::ONC_SOURCE_DEVICE_POLICY)
     return ::onc::kAugmentationSharedSetting;
   return ::onc::kAugmentationUserSetting;
 }
 
 // Creates a GetManagedProperties style dictionary and adds it to |settings|.
 // |default_value| represents either the recommended value if |recommended|
 // is true, or the enforced value if |recommended| is false.
 // Note(stevenjb): This is bridge code until we use GetManagedProperties to
 // retrieve Shill properties.
-void SetManagedValueDictionary(const char* key,
-                               const base::Value* value,
-                               ::onc::ONCSource onc_source,
-                               bool recommended,
-                               const base::Value* default_value,
-                               base::DictionaryValue* settings) {
+void SetManagedValueDictionaryEx(const char* settings_dict_key,
+                                 const base::Value* value,
+                                 ::onc::ONCSource onc_source,
+                                 bool recommended,
+                                 const base::Value* default_value,
+                                 base::DictionaryValue* settings_dict) {
   base::DictionaryValue* dict = new base::DictionaryValue();
-  settings->Set(key, dict);
+  settings_dict->Set(settings_dict_key, dict);

[pneubeck (no reviews), 2014/08/19 19:26:38]

nit:
maybe clarify at the comment of this function whether keys with '.' are
supported or not.
Accordingly this should indeed either be 'Set' as it is or
SetWithoutPathExpansion.

Maybe say something like 'base::DictionaryValue's path expansion is applied to
|settings_dict_key|.' ?

[stevenjb, 2014/08/19 23:20:17]

This was not intended to support path expansion, settings_dict_key is expected
to be an ONC key. I added a comment and changed to SetWithout...

 
   DCHECK(value);
   dict->Set(::onc::kAugmentationActiveSetting, value->DeepCopy());
 
   if (onc_source == ::onc::ONC_SOURCE_NONE)
     return;
 
   if (recommended) {
     // If an ONC property is 'Recommended' it can be edited by the user.
     std::string editable = GetOncEditableString(onc_source);
     dict->Set(editable, new base::FundamentalValue(true));
   }
   if (default_value) {
     std::string policy_source = GetOncPolicyString(onc_source);
     dict->Set(policy_source, default_value->DeepCopy());
     if (recommended && !value->Equals(default_value)) {
       std::string setting_source = GetOncSettingString(onc_source);
       dict->Set(setting_source, value->DeepCopy());
       dict->SetString(::onc::kAugmentationEffectiveSetting, setting_source);
     } else {
       dict->SetString(::onc::kAugmentationEffectiveSetting, policy_source);
     }
   }
 }
 
+void SetManagedValueDictionary(const std::string& guid,
+                               const char* settings_dict_key,
+                               const base::Value* value,
+                               const std::string& onc_key,
+                               base::DictionaryValue* settings_dict) {
+

[pneubeck (no reviews), 2014/08/19 19:26:37]

nit: drop empty line

[stevenjb, 2014/08/19 23:20:18]

Done.

+  ::onc::ONCSource onc_source = ::onc::ONC_SOURCE_NONE;
+  const base::DictionaryValue* onc =
+      onc::FindPolicyForActiveUser(guid, &onc_source);
+  DCHECK_EQ(onc == NULL, onc_source == ::onc::ONC_SOURCE_NONE);
+  const base::Value* default_value = NULL;
+  if (onc)
+    onc->Get(onc_key, &default_value);
+  SetManagedValueDictionaryEx(settings_dict_key,
+                              value,
+                              onc_source,
+                              onc::IsRecommendedValue(onc, onc_key),
+                              default_value,
+                              settings_dict);
+}
+
+// Creates a GetManagedProperties style dictionary with an Active value and
+// a Translated value, and adds it to |settings|.
+// Note(stevenjb): This is bridge code until we use GetManagedProperties to
+// retrieve Shill properties and include Translated values.
+void SetTranslatedDictionary(const char* settings_dict_key,
+                             const std::string& value,
+                             const std::string& translated_value,
+                             base::DictionaryValue* settings_dict) {
+  base::DictionaryValue* dict = new base::DictionaryValue();
+  settings_dict->Set(settings_dict_key, dict);
+  dict->SetString(::onc::kAugmentationActiveSetting, value);
+  dict->SetString(kTranslatedKey, translated_value);
+}
+
 std::string CopyStringFromDictionary(const base::DictionaryValue& source,
                                      const std::string& src_key,
                                      const std::string& dest_key,
                                      base::DictionaryValue* dest) {
   std::string string_value;
   if (source.GetStringWithoutPathExpansion(src_key, &string_value))
     dest->SetStringWithoutPathExpansion(dest_key, string_value);
   return string_value;
 }
 
 // Fills |dictionary| with the configuration details of |vpn|. |onc| is required
 // for augmenting the policy-managed information.
 void PopulateVPNDetails(const NetworkState* vpn,
                         const base::DictionaryValue& shill_properties,
                         base::DictionaryValue* dictionary) {
   // Name and Remembered are set in PopulateConnectionDetails().
   // Provider properties are stored in the "Provider" dictionary.
-  const base::DictionaryValue* provider_properties = NULL;
+  const base::DictionaryValue* shill_provider_properties = NULL;
   if (!shill_properties.GetDictionaryWithoutPathExpansion(
-          shill::kProviderProperty, &provider_properties)) {
+          shill::kProviderProperty, &shill_provider_properties)) {
     LOG(ERROR) << "No provider properties for VPN: " << vpn->path();
     return;
   }
-  std::string provider_type;
-  provider_properties->GetStringWithoutPathExpansion(
-      shill::kTypeProperty, &provider_type);
-  dictionary->SetString(kTagProviderType,
-                        internet_options_strings::ProviderTypeString(
-                            provider_type,
-                            *provider_properties));
+  base::DictionaryValue* vpn_dictionary = new base::DictionaryValue;
+  dictionary->Set(::onc::network_config::kVPN, vpn_dictionary);
+
+  std::string shill_provider_type;
+  if (!shill_provider_properties->GetStringWithoutPathExpansion(
+          shill::kTypeProperty, &shill_provider_type)) {
+    LOG(ERROR) << "Shill VPN has no Provider.Type: " << vpn->path();
+    return;
+  }
+  std::string onc_provider_type;
+  onc::TranslateStringToONC(
+      onc::kVPNTypeTable, shill_provider_type, &onc_provider_type);
+  SetTranslatedDictionary(
+      ::onc::vpn::kType,
+      onc_provider_type,
+      internet_options_strings::ProviderTypeString(shill_provider_type,
+                                                   *shill_provider_properties),
+      vpn_dictionary);
+
+  base::DictionaryValue* provider_dictionary = new base::DictionaryValue;

[pneubeck (no reviews), 2014/08/19 19:26:37]

I have the impression that 'provider' is not the right term in this case, but
maybe I'm wrong.
Maybe something like vpn_subdictionary / vpn_details .. ?

[stevenjb, 2014/08/19 23:20:17]

Renamed provider_type_dictionary since that is what the key is. (Which, FWIW,
seems a little wonky to me, and certainly adds code complexity).

+  vpn_dictionary->Set(onc_provider_type, provider_dictionary);

[pneubeck (no reviews), 2014/08/19 19:26:38]

This will not work: The possible subdictionaries are OpenVPN, L2TP, IPsec
There is a VPN type 'L2TP-IPsec' but no dictionary 'L2TP-IPsec'.

I mean, it will work for now, but not anymore when you change to
GetManagedProperties

Since you set only the username for L2TP below, this should be easy to fix

[stevenjb, 2014/08/19 23:20:18]

Gah. OK, so... what? Let me look at the doc again...

 
   std::string username;
-  if (provider_type == shill::kProviderOpenVpn) {
-    provider_properties->GetStringWithoutPathExpansion(
+  if (shill_provider_type == shill::kProviderOpenVpn) {
+    shill_provider_properties->GetStringWithoutPathExpansion(
         shill::kOpenVPNUserProperty, &username);
   } else {
-    provider_properties->GetStringWithoutPathExpansion(
+    shill_provider_properties->GetStringWithoutPathExpansion(
         shill::kL2tpIpsecUserProperty, &username);
   }
-  dictionary->SetString(kTagUsername, username);
+  provider_dictionary->SetString(::onc::vpn::kUsername, username);

[pneubeck (no reviews), 2014/08/19 19:26:37]

The username is never set in IPsec but only in L2TP

[stevenjb, 2014/08/19 23:20:18]

OK, I think I maybe have this right now...

 
-  ::onc::ONCSource onc_source = ::onc::ONC_SOURCE_NONE;
-  const base::DictionaryValue* onc =
-      onc::FindPolicyForActiveUser(vpn->guid(), &onc_source);
-
-  NetworkPropertyUIData hostname_ui_data;
-  hostname_ui_data.ParseOncProperty(
-      onc_source,
-      onc,
-      ::onc::network_config::VpnProperty(::onc::vpn::kHost));
   std::string provider_host;
-  provider_properties->GetStringWithoutPathExpansion(
+  shill_provider_properties->GetStringWithoutPathExpansion(
       shill::kHostProperty, &provider_host);
-  SetValueDictionary(kTagServerHostname,
-                     new base::StringValue(provider_host),
-                     hostname_ui_data,
-                     dictionary);
+  SetManagedValueDictionary(
+      vpn->guid(),
+      ::onc::vpn::kHost,

[pneubeck (no reviews), 2014/08/19 19:26:38]

optional nit: having the same key with different prefixes two times in a call is
rather confusing. could you add argument comments that one refers to the key in
the result and one to the path in ONC ?

OR:
just use the same path and don't pass in 'vpn_dictionary' but 'dictionary' ?
That would also ensure that the structure of the resulting dictionary matches
ONC.

[stevenjb, 2014/08/19 23:20:17]

I really don't like using Value::Set with '.' separators, especially in
networking code where Shill sometimes uses '.' in key names.

I'll try to clarify the comments, but... there's no way this isn't going to be
confusing for now.

+      new base::StringValue(provider_host),

[pneubeck (no reviews), 2014/08/19 19:26:38]

memory leak, see other comment.

[stevenjb, 2014/08/19 23:20:17]

Bah, this is what happens when... nevermind. Fixed.

+      ::onc::network_config::VpnProperty(::onc::vpn::kHost),
+      vpn_dictionary);
 }
 
 // Given a list of supported carrier's by the device, return the index of
 // the carrier the device is currently using.
 int FindCurrentCarrierIndex(const base::ListValue* carriers,
                             const DeviceState* device) {
   DCHECK(carriers);
   DCHECK(device);
   bool gsm = (device->technology_family() == shill::kTechnologyFamilyGsm);
   int index = 0;
@@ skipping 885 matching lines @@
                      static_ip_dict.release(),
                      property_ui_data,
                      dictionary.get());
 
   dictionary->SetString(kTagNameServerType, name_server_type);
   dictionary->SetString(kTagNameServersGoogle, kGoogleNameServers);
 
   int priority = 0;
   shill_properties.GetIntegerWithoutPathExpansion(
       shill::kPriorityProperty, &priority);
-  bool preferred = priority > 0;
-  SetValueDictionary(kTagPreferred,
-                     new base::FundamentalValue(preferred),
-                     property_ui_data,
-                     dictionary.get());
+  SetManagedValueDictionary(network->guid(),
+                            ::onc::network_config::kPriority,
+                            new base::FundamentalValue(priority),

[pneubeck (no reviews), 2014/08/19 19:26:38]

this will leak the FundamentalValue.
Please, please change the functions SetManagedValueDictionary(Ex) to take a
(const Value& value) to prevent this mistake.

[stevenjb, 2014/08/19 23:20:17]

Fixed.

+                            ::onc::network_config::kPriority,
+                            dictionary.get());
 
   std::string onc_path_to_auto_connect;
   if (network->Matches(NetworkTypePattern::WiFi())) {
     content::RecordAction(
         base::UserMetricsAction("Options_NetworkShowDetailsWifi"));
     if (network->IsConnectedState()) {
       content::RecordAction(
           base::UserMetricsAction("Options_NetworkShowDetailsWifiConnected"));
     }
     onc_path_to_auto_connect =
@@ skipping 43 matching lines @@
             network->IsPrivate())) {
       auto_connect_recommended = false;
       auto_connect_onc_source = network->IsPrivate()
                                     ? ::onc::ONC_SOURCE_USER_POLICY
                                     : ::onc::ONC_SOURCE_DEVICE_POLICY;
       if (auto_connect) {
         LOG(WARNING) << "Policy prevents autoconnect, but value is True.";
         auto_connect_value.reset(new base::FundamentalValue(false));
       }
     }
-    SetManagedValueDictionary(shill::kAutoConnectProperty,
-                              auto_connect_value.get(),
-                              auto_connect_onc_source,
-                              auto_connect_recommended,
-                              auto_connect_default_value,
-                              dictionary.get());
+    SetManagedValueDictionaryEx(shill::kAutoConnectProperty,
+                                auto_connect_value.get(),
+                                auto_connect_onc_source,
+                                auto_connect_recommended,
+                                auto_connect_default_value,
+                                dictionary.get());
   }
 
   // Show details dialog
   web_ui()->CallJavascriptFunction(kShowDetailedInfoFunction, *dictionary);
 }
 
 gfx::NativeWindow InternetOptionsHandler::GetNativeWindow() const {
   return web_ui()->GetWebContents()->GetTopLevelNativeWindow();
 }
 
@@ skipping 181 matching lines @@
   dictionary->SetBoolean(
       kTagWimaxAvailable,
       handler->IsTechnologyAvailable(NetworkTypePattern::Wimax()));
   dictionary->SetBoolean(
       kTagWimaxEnabled,
       handler->IsTechnologyEnabled(NetworkTypePattern::Wimax()));
 }
 
 }  // namespace options
 }  // namespace chromeos