Convert all connect code to use NetworkHandler instead of NetworkLibrary

chromeos/network/network_configuration_handler.cc

Index: chromeos/network/network_configuration_handler.cc
diff --git a/chromeos/network/network_configuration_handler.cc b/chromeos/network/network_configuration_handler.cc
index f7f118ee4dae1273501cc80b0ca1d94e7ad7e1f9..30f8735701211d6086f76b5f0e4d76184579f53f 100644
--- a/chromeos/network/network_configuration_handler.cc
+++ b/chromeos/network/network_configuration_handler.cc
@@ -9,6 +9,7 @@
 
 #include "base/bind.h"
 #include "base/format_macros.h"
+#include "base/json/json_writer.h"
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
@@ -66,6 +67,31 @@ void GetPropertiesCallback(
       *properties_copy.get());
 }
 
+void SetNetworkProfileErrorCallback(
+    const std::string& service_path,
+    const std::string& profile_path,
+    const network_handler::ErrorCallback& error_callback,
+    const std::string& dbus_error_name,
+    const std::string& dbus_error_message) {
+  network_handler::ShillErrorCallbackFunction(
+      "Config.SetNetworkProfile Failed: " + profile_path,
+      service_path, error_callback,
+      dbus_error_name, dbus_error_message);
+}
+
+bool IsPassphrase(const std::string& key) {
+  return key == flimflam::kEapPrivateKeyPasswordProperty ||
+      key == flimflam::kEapPasswordProperty ||
+      key == flimflam::kL2tpIpsecPasswordProperty ||
+      key == flimflam::kOpenVPNPasswordProperty ||
+      key == flimflam::kPassphraseProperty ||
+      key == flimflam::kOpenVPNOTPProperty ||
+      key == flimflam::kEapPrivateKeyProperty ||
+      key == flimflam::kEapPrivateKeyPasswordProperty ||
+      key == flimflam::kEapPinProperty ||
+      key == flimflam::kApnPasswordProperty;
+}
+
 }  // namespace
 
 // Helper class to request from Shill the profile entries associated with a
@@ -188,7 +214,19 @@ void NetworkConfigurationHandler::SetProperties(
     const base::DictionaryValue& properties,
     const base::Closure& callback,
     const network_handler::ErrorCallback& error_callback) {
+  if (properties.empty()) {
+    if (!callback.is_null())
+      callback.Run();
+    return;
+  }
   NET_LOG_USER("SetProperties", service_path);
+  for (base::DictionaryValue::Iterator iter(properties);

[pneubeck (no reviews), 2013/08/08 11:10:00]

uff.. isn't this a bit too much logging?

Shill can already log every dbus detail.

e.g. you could at least drop the values or, which I would prefer, make this
logging dependent on the debugging flag (either defined(NDEBUG) or ENABLE_DLOG.

[stevenjb, 2013/08/08 19:00:56]

Nobody runs Debug builds in practice.

For M30 there are going to be cases we missed where somebody's network
configuration no longer works. This way we will know exactly what properties the
UI thinks it is setting, in one convenient location. It's why I created
NetworkEventLog in the first place.

There is other logging that I have started backing off on, e.g. wifi signal
strength updates, but I'd rather start off logging too much than too little,
especially around network configuration.

+       !iter.IsAtEnd(); iter.Advance()) {
+    std::string v = "******";
+    if (!IsPassphrase(iter.key()))

[pneubeck (no reviews), 2013/08/08 11:10:00]

I don't like this blacklisting at all.
Yet another place that's very likely to become outdated and introduce a security
hole.

[stevenjb, 2013/08/08 19:00:56]

I'm not sure I agree. As you mentioned, we already log these in Shill. If we are
really concerned we can remove this logging in 31 or 32, but it was essential
for me to get this all working correctly in the first place, and I don't expect
to have gotten it 100% correct on the first try.

+      base::JSONWriter::Write(&iter.value(), &v);
+    NET_LOG_DEBUG("SetProperty",  service_path + "." + iter.key() + "=" + v);

[pneubeck (no reviews), 2013/08/08 11:10:00]

you might even consider making NET_LOG_DEBUG (not here, but in
network_event_log.h) a no op if !ENABLE_DLOG

[stevenjb, 2013/08/08 19:00:56]

Again, I don't think that is useful. The point of this is to debug real world
failures.

I would consider making the logging Dev channel only. I'd prefer to wait at
least until 31 to make that change however.

+  }
   DBusThreadManager::Get()->GetShillServiceClient()->SetProperties(
       dbus::ObjectPath(service_path),
       properties,
@@ -203,7 +241,16 @@ void NetworkConfigurationHandler::ClearProperties(
     const std::vector<std::string>& names,
     const base::Closure& callback,
     const network_handler::ErrorCallback& error_callback) {
+  if (names.empty()) {
+    if (!callback.is_null())
+      callback.Run();
+    return;
+  }
   NET_LOG_USER("ClearProperties", service_path);
+  for (std::vector<std::string>::const_iterator iter = names.begin();
+       iter != names.end(); ++iter) {
+    NET_LOG_DEBUG("ClearProperty", service_path + "." + *iter);
+  }
   DBusThreadManager::Get()->GetShillServiceClient()->ClearProperties(
       dbus::ObjectPath(service_path),
       names,
@@ -219,9 +266,18 @@ void NetworkConfigurationHandler::CreateConfiguration(
     const network_handler::ErrorCallback& error_callback) {
   ShillManagerClient* manager =
       DBusThreadManager::Get()->GetShillManagerClient();
-
   std::string type;
   properties.GetStringWithoutPathExpansion(flimflam::kTypeProperty, &type);
+
+  NET_LOG_USER("CreateConfiguration", type);
+  for (base::DictionaryValue::Iterator iter(properties);

[pneubeck (no reviews), 2013/08/08 11:10:00]

if _really_ necessary (as said above, I think it's to much and risky logging),
share the code with SetProperties

[stevenjb, 2013/08/08 19:00:56]

Done.

+       !iter.IsAtEnd(); iter.Advance()) {
+    std::string v = "******";
+    if (!IsPassphrase(iter.key()))
+      base::JSONWriter::Write(&iter.value(), &v);
+    NET_LOG_DEBUG("Configure",  type + "." + iter.key() + "=" + v);
+  }
+
   // Shill supports ConfigureServiceForProfile only for network type WiFi. In
   // all other cases, we have to rely on GetService for now. This is
   // unproblematic for VPN (user profile only), but will lead to inconsistencies
@@ -265,6 +321,22 @@ void NetworkConfigurationHandler::RemoveConfiguration(
   deleter->Run();
 }
 
+void NetworkConfigurationHandler::SetNetworkProfile(
+    const std::string& service_path,
+    const std::string& profile_path,
+    const base::Closure& callback,
+    const network_handler::ErrorCallback& error_callback) {
+  NET_LOG_USER("SetNetworkProfile", service_path + ": " + profile_path);
+  base::StringValue profile_path_value(profile_path);
+  DBusThreadManager::Get()->GetShillServiceClient()->SetProperty(
+      dbus::ObjectPath(service_path),
+      flimflam::kProfileProperty,
+      profile_path_value,
+      callback,
+      base::Bind(&SetNetworkProfileErrorCallback,
+                 service_path, profile_path, error_callback));
+}
+
 // NetworkConfigurationHandler Private methods
 
 NetworkConfigurationHandler::NetworkConfigurationHandler()