Support Managed NetworkState format dictionaries for controlled settings.

chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc

Index: chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
diff --git a/chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc b/chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
index ca679e2e5a12353e1bdc4fc35a928c3775801c3b..57ef5309d5b21adc7e2fc6e834761c23de993f6b 100644
--- a/chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
+++ b/chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
@@ -126,7 +126,6 @@ const char kTagActivate[] = "activate";
 const char kTagActivationState[] = "activationState";
 const char kTagAddConnection[] = "add";
 const char kTagApn[] = "apn";
-const char kTagAutoConnect[] = "autoConnect";
 const char kTagCarrierSelectFlag[] = "showCarrierSelect";
 const char kTagCarrierUrl[] = "carrierUrl";
 const char kTagCellularAvailable[] = "cellularAvailable";
@@ -137,9 +136,8 @@ const char kTagConnect[] = "connect";
 const char kTagControlledBy[] = "controlledBy";
 const char kTagDeviceConnected[] = "deviceConnected";
 const char kTagDisconnect[] = "disconnect";
-const char kTagErrorState[] = "errorState";
+const char kTagErrorMessage[] = "errorMessage";
 const char kTagForget[] = "forget";
-const char kTagIdentity[] = "identity";
 const char kTagLanguage[] = "language";
 const char kTagLastGoodApn[] = "lastGoodApn";
 const char kTagLocalizedName[] = "localizedName";
@@ -164,7 +162,6 @@ const char kTagCarriers[] = "carriers";
 const char kTagCurrentCarrierIndex[] = "currentCarrierIndex";
 const char kTagShared[] = "shared";
 const char kTagShowActivateButton[] = "showActivateButton";
-const char kTagShowStaticIPConfig[] = "showStaticIPConfig";
 const char kTagShowViewAccountButton[] = "showViewAccountButton";
 const char kTagSimCardLockEnabled[] = "simCardLockEnabled";
 const char kTagSupportUrl[] = "supportUrl";
@@ -295,20 +292,6 @@ base::DictionaryValue* BuildIPInfoDictionary(
   return ip_info_dict.release();
 }
 
-// Decorate dictionary |value_dict| with policy information from |ui_data|.
-void DecorateValueDictionary(const NetworkPropertyUIData& ui_data,
-                             const base::Value& value,
-                             base::DictionaryValue* value_dict) {
-  const base::Value* recommended_value = ui_data.default_value();
-  if (ui_data.IsManaged())
-    value_dict->SetString(kTagControlledBy, kTagPolicy);
-  else if (recommended_value && recommended_value->Equals(&value))
-    value_dict->SetString(kTagControlledBy, kTagRecommended);
-
-  if (recommended_value)
-    value_dict->Set(kTagRecommendedValue, recommended_value->DeepCopy());
-}
-
 // Decorate pref value as CoreOptionsHandler::CreateValueForPref() does and
 // store it under |key| in |settings|. Takes ownership of |value|.
 void SetValueDictionary(base::DictionaryValue* settings,
@@ -319,33 +302,38 @@ void SetValueDictionary(base::DictionaryValue* settings,
   // DictionaryValue::Set() takes ownership of |value|.
   dict->Set(kTagValue, value);
   settings->Set(key, dict);
-  DecorateValueDictionary(ui_data, *value, dict);
+
+  const base::Value* recommended_value = ui_data.default_value();
+  if (ui_data.IsManaged())
+    dict->SetString(kTagControlledBy, kTagPolicy);
+  else if (recommended_value && recommended_value->Equals(value))
+    dict->SetString(kTagControlledBy, kTagRecommended);
+
+  if (recommended_value)
+    dict->Set(kTagRecommendedValue, recommended_value->DeepCopy());
 }
 
-// Creates a decorated dictionary like SetValueDictionary does, but extended for
-// the Autoconnect property, which respects additionally global network policy.
-void SetAutoconnectValueDictionary(bool network_is_private,
-                                   ::onc::ONCSource onc_source,
-                                   bool current_autoconnect,
-                                   const NetworkPropertyUIData& ui_data,
-                                   base::DictionaryValue* settings) {
+// Creates a ManagedState style dictionary. Note(stevenjb): This is bridge code

[pneubeck (no reviews), 2014/07/30 20:12:46]

here and in the commit message:
ManagedState -> ManagedProperties ?

ManagedState sounds like chromeos/network/managed_state??

[stevenjb, 2014/07/30 21:57:48]

Done.

+// until we use GetManagedProperties to retrieve all Shill properties.
+void SetManagedValueDictionary(base::DictionaryValue* settings,

[pneubeck (no reviews), 2014/07/30 20:12:46]

nit: should be the last argument

[stevenjb, 2014/07/30 21:57:48]

Fixed here and for SetValueDictionary.

+                               const char* key,

[pneubeck (no reviews), 2014/07/30 20:12:46]

const std::string&

[stevenjb, 2014/07/30 21:57:48]

Why?

[pneubeck (no reviews), 2014/07/31 20:29:36]

This CL has more important changes, that I will spare you any elaboration.
Should have marked it as 'optional nit'

+                               base::Value* value,

[pneubeck (no reviews), 2014/07/30 20:12:46]

scoped_ptr (it's actually called with .release() )

[stevenjb, 2014/07/30 21:57:48]

I'm leaving this consistent with SetValueDictionary. I don't agree that adding
scoped_ptr overhead to this file local helper function is more readable.

[pneubeck (no reviews), 2014/07/31 20:29:36]

it's documenting ownership.
Otherwise this should probably have a comment.

[stevenjb, 2014/08/01 00:53:41]

Commented.

+                               const NetworkPropertyUIData& ui_data) {
   base::DictionaryValue* dict = new base::DictionaryValue();
-  base::Value* value = new base::FundamentalValue(current_autoconnect);
-  // DictionaryValue::Set() takes ownership of |value|.
-  dict->Set(kTagValue, value);
-  settings->Set(kTagAutoConnect, dict);
-  if (onc_source != ::onc::ONC_SOURCE_USER_POLICY &&
-      onc_source != ::onc::ONC_SOURCE_DEVICE_POLICY) {
-    // Autoconnect can be controlled by the GlobalNetworkConfiguration of the
-    // ONC policy.
-    bool only_policy_autoconnect =
-        onc::PolicyAllowsOnlyPolicyNetworksToAutoconnect(network_is_private);
-    if (only_policy_autoconnect) {
-      dict->SetString(kTagControlledBy, kTagPolicy);
-      return;
-    }
+  settings->Set(key, dict);
+
+  dict->Set(::onc::kAugmentationActiveSetting, value);  // owns |value|
+  if (ui_data.default_value()) {
+    std::string effective;
+    if (ui_data.onc_source() == ::onc::ONC_SOURCE_DEVICE_POLICY)

[pneubeck (no reviews), 2014/07/30 20:12:46]

I fear that this is not fully right.
Looking at NetworkPropertyUIData,
  - onc_source() is by default the source of the policy
  - if this particular property is 'recommended', onc_source() is NONE instead.
  - default_value() is only set if the property is 'recommended'
  - if the property is not recommended it was assumed that the policy is
enforced anyways, and the actual value == policy value

Thus, this function should probably look like:

void SetManagedValueDictionary(OncSource onc_source, bool is_private, .... ) {
  DCHECK(value);
  ...
  dict->Set(::onc::kAugmentationActiveSetting, value);  // Fine, as value comes
directly from Shill

  // We don't have a better source for the user's setting than the actual value
from Shill
  std::string effective = is_private ? UserSetting : SharedSetting;     
  dict->Set(effective, value->DeepCopy());

  if (onc_source == ...NONE) {
     // No policy to consider.
  } else {
    std::string policy_key = onc_source == USER_POLICY ?
              ::onc::kAugmentationUserPolicy : ...DevicePolicy;
    if (ui_data.onc_source() == ...NONE) {
       // The policy 'recommends' this property, thus the value is editable by
the user.
       dict->SetBoolean(onc_source == USER_POLICY ? kAugmentationUserEditable :
                     kAugmentationDeviceEditable, true);
       // ui_data.default_value contains the original policy value, even if
       // the user changed it. If the policy didn't set any value, then
default_value is NULL.
       if (ui_data.default_value())
         dict->Set(policy_key, ui_data.default_value()->DeepCopy());
       // If the recommended value equals the actual value from Shill, we don't
know 
       // whether the user has set this property explicitly or not. Assume that
the 
       // user didn't modify it.
       if (value->Equals(ui_data.default_value()))
         effective = policy_key;
    } else {
       // Don't set the default Editable value of false.
       // The actual value should equal the enforce policy value. Set it here
for completeness.
       dict->Set(policy_key, value->DeepCopy());
    }
  }
  dict->SetString(::onc::kAugmentationEffectiveSetting, effective);
}

[stevenjb, 2014/07/30 21:57:48]

So, this is an intermediate helper function, and eventually all
SetValueDictionary calls will be replaced with it (in places where we can't
immediately/easily use the results of GetManagedProperties directly). What is
the source of 'is_private' in these cases? I explicitly mangled the AutoConnect
code to modify ui_data t avoid passing that argument.

I don't see any reference to UserSetting or SharedSetting in the existing
SetValueDictionary or SetAutoconnectValueDictionary. I really don't want to add
functionality I don't fully understand here, I would much rather do that in a
followup. (Again, unless I am missing something?)

[pneubeck (no reviews), 2014/07/31 20:29:36]

In SetValueDictionary, UserSetting / SharedSetting can't be mentioned as it's
following the format of CoreOptionsHandler::CreateValueForPref .

Maybe you meant that you didn't see it in current calls to getManagedProperties
of the API.
You're right, that it doesn't appear there yet, because we still don't store
user settings separately from the Service properties in Shill.
It should then be sufficient to have combinations of these fields.
Without policy:
 Active, Effective (for now without UserSetting/SharedSetting)
With user (device) policy additionally:
 UserPolicy (DevicePolicy), UserEditable (DeviceEditable)

is_private: Whether the network is shared or not (stored in the user's profile
or the default profile)
onc_source: The source of the policy that controls the (whole) network, or NONE
if not present. This is different from ui_data.onc_source() as explained
somewhere else. The latter is NONE, if this particular property is recommended
by policy.

[stevenjb, 2014/08/01 00:53:41]

I think, now that I have written it all out in the doc I shared, that I
understand this better and am at least closer to something that is at least
partly correct.

+      effective = ::onc::kAugmentationDevicePolicy;
+    else if (ui_data.onc_source() == ::onc::ONC_SOURCE_USER_POLICY)
+      effective = ::onc::kAugmentationUserPolicy;
+    else
+      effective = ::onc::kAugmentationUnmanaged;

[pneubeck (no reviews), 2014/07/30 20:12:46]

could potentially also be kAugmentationUserSetting or ..SharedSettting .
On this side it might be ok to keep this, but on the .js side you will have to
handle either value at some time.

[stevenjb, 2014/07/30 21:57:48]

Acknowledged.

+    dict->SetString(::onc::kAugmentationEffectiveSetting, effective);
+    dict->Set(effective, ui_data.default_value()->DeepCopy());
   }
-  DecorateValueDictionary(ui_data, *value, dict);
 }
 
 std::string CopyStringFromDictionary(const base::DictionaryValue& source,
@@ -430,13 +418,6 @@ int FindCurrentCarrierIndex(const base::ListValue* carriers,
   return -1;
 }
 
-void PopulateWimaxDetails(const NetworkState* wimax,
-                          const base::DictionaryValue& shill_properties,
-                          base::DictionaryValue* dictionary) {
-  CopyStringFromDictionary(
-      shill_properties, shill::kEapIdentityProperty, kTagIdentity, dictionary);
-}
-
 void CreateDictionaryFromCellularApn(const base::DictionaryValue* apn,
                                      base::DictionaryValue* dictionary) {
   CopyStringFromDictionary(*apn, shill::kApnProperty, kTagApn, dictionary);
@@ -621,8 +602,15 @@ scoped_ptr<base::DictionaryValue> PopulateConnectionDetails(
       NetworkHandler::Get()->network_state_handler()->GetDeviceState(
           network->device_path());
   if (device) {
-    shill_properties_with_device->Set(shill::kDeviceProperty,
-                                      device->properties().DeepCopy());
+    shill_properties_with_device->SetWithoutPathExpansion(
+        shill::kDeviceProperty, device->properties().DeepCopy());
+    // Get the hardware MAC address from the DeviceState.
+    // (Note: this is done in ManagedNetworkConfigurationHandler but not
+    //  in NetworkConfigurationHandler).
+    if (!device->mac_address().empty()) {
+      shill_properties_with_device->SetStringWithoutPathExpansion(
+          shill::kAddressProperty, device->mac_address());
+    }
   }
   scoped_ptr<base::DictionaryValue> dictionary =
       onc::TranslateShillServiceToONCPart(
@@ -630,7 +618,7 @@ scoped_ptr<base::DictionaryValue> PopulateConnectionDetails(
 
   dictionary->SetString(kNetworkInfoKeyServicePath, network->path());
   dictionary->SetString(
-      kTagErrorState,
+      kTagErrorMessage,
       ash::network_connect::ErrorString(network->error(), network->path()));
 
   dictionary->SetBoolean(kTagRemembered, !network->profile_path().empty());
@@ -644,9 +632,7 @@ scoped_ptr<base::DictionaryValue> PopulateConnectionDetails(
           NetworkTypePattern::Primitive(type));
   dictionary->SetBoolean(kTagDeviceConnected, connected_network != NULL);
 
-  if (type == shill::kTypeWimax)
-    PopulateWimaxDetails(network, shill_properties, dictionary.get());
-  else if (type == shill::kTypeCellular)
+  if (type == shill::kTypeCellular)
     PopulateCellularDetails(network, shill_properties, dictionary.get());
   else if (type == shill::kTypeVPN)
     PopulateVPNDetails(network, shill_properties, dictionary.get());
@@ -1308,11 +1294,6 @@ void InternetOptionsHandler::PopulateDictionaryDetailsCallback(
   dictionary->SetString(kTagNameServerType, name_server_type);
   dictionary->SetString(kTagNameServersGoogle, kGoogleNameServers);
 
-  // Enable static ip config for Ethernet or WiFi.
-  bool staticIPConfig = network->Matches(NetworkTypePattern::Ethernet()) ||
-                        network->Matches(NetworkTypePattern::WiFi());
-  dictionary->SetBoolean(kTagShowStaticIPConfig, staticIPConfig);
-
   int priority = 0;
   shill_properties.GetIntegerWithoutPathExpansion(
       shill::kPriorityProperty, &priority);
@@ -1356,11 +1337,21 @@ void InternetOptionsHandler::PopulateDictionaryDetailsCallback(
   bool auto_connect = false;
   shill_properties.GetBooleanWithoutPathExpansion(
       shill::kAutoConnectProperty, &auto_connect);
-  SetAutoconnectValueDictionary(network->IsPrivate(),
-                                onc_source,
-                                auto_connect,
-                                auto_connect_ui_data,
-                                dictionary.get());
+
+  // Autoconnect can be controlled by the GlobalNetworkConfiguration of the
+  // ONC policy.
+  scoped_ptr<base::Value> auto_connect_value(
+      new base::FundamentalValue(auto_connect));
+  if (!auto_connect_ui_data.IsManaged() &&

[pneubeck (no reviews), 2014/07/30 20:12:46]

don't use this IsManaged, as it's referring to the properties state only (which
could be recommended -> IsManaged == false).
Use onc_source != ..NONE instead.

[stevenjb, 2014/07/30 21:57:48]

Done.

[pneubeck (no reviews), 2014/07/31 20:29:36]

I meant the onc_source variable in this functions scope, not the member of
auto_connect_ui_data.

[stevenjb, 2014/08/01 00:53:41]

GGAHHHHHHH! I get it, I think, but.... I have to tell you that having a
NetworkPropertyUIData class that has an onc_source_ member that needs to be
accompanied by another onc_source argument seems... with all due respect.... bat
shit crazy.

[pneubeck (no reviews), 2014/08/01 16:09:07]

Maybe you guessed it already but this behavior of *UIData pre-dates my time at
this company. Feel free to run a 'git blame' ;-)

I fully agree that this is in a bad state. That's one of the reasons for
migrating to the ONC/getManagedProperties stuff.
Hopefully you don't find such crazyness there :-)

+      onc::PolicyAllowsOnlyPolicyNetworksToAutoconnect(network->IsPrivate())) {
+    ::onc::ONCSource source = network->IsPrivate()

[pneubeck (no reviews), 2014/07/30 20:12:46]

Please add a comment (which I should have done in the old code as well):

// This modifies auto_connect_ui_data, so that the autoconnect value appears as
set to 'false' by policy and not editable by the user.

[stevenjb, 2014/07/30 21:57:48]

Done.

+        ? ::onc::ONC_SOURCE_USER_POLICY : ::onc::ONC_SOURCE_DEVICE_POLICY;
+    auto_connect_ui_data.SetOncSourceAndValue(source, auto_connect_value.get());

[pneubeck (no reviews), 2014/07/30 20:12:46]

set the value to 'false' (although it should be the same if there was no bug in
the policy application).
If you want to, add a DCHECK to ensure this.

[stevenjb, 2014/07/30 21:57:48]

Done.

+  }
+  SetManagedValueDictionary(dictionary.get(),
+                            shill::kAutoConnectProperty,
+                            auto_connect_value.release(),
+                            auto_connect_ui_data);
 
   // Show details dialog
   web_ui()->CallJavascriptFunction(kShowDetailedInfoFunction, *dictionary);