Convert all connect code to use NetworkHandler instead of NetworkLibrary

chromeos/network/network_configuration_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 "chromeos/network/network_configuration_handler.h"
 
 #include <string>
 #include <vector>
 
 #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"
 #include "base/stl_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/values.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/shill_manager_client.h"
 #include "chromeos/dbus/shill_profile_client.h"
 #include "chromeos/dbus/shill_service_client.h"
@@ skipping 37 matching lines @@
   std::string name = NetworkState::GetNameFromProperties(properties);
   if (!name.empty()) {
     properties_copy->SetStringWithoutPathExpansion(
         flimflam::kNameProperty, name);
   }
   network_handler::GetPropertiesCallback(
       callback, error_callback, service_path, call_status,
       *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
 // Service and delete the service from each profile. Triggers either
 // |callback| on success or |error_callback| on failure, and calls
 // |handler|->ProfileEntryDeleterCompleted() on completion to delete itself.
 class NetworkConfigurationHandler::ProfileEntryDeleter
     : public base::SupportsWeakPtr<ProfileEntryDeleter> {
  public:
   ProfileEntryDeleter(NetworkConfigurationHandler* handler,
@@ skipping 102 matching lines @@
       dbus::ObjectPath(service_path),
       base::Bind(&GetPropertiesCallback,
                  callback, error_callback, service_path));
 }
 
 void NetworkConfigurationHandler::SetProperties(
     const std::string& service_path,
     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,
       base::Bind(&NetworkConfigurationHandler::SetPropertiesSuccessCallback,
                  AsWeakPtr(), service_path, callback),
       base::Bind(&NetworkConfigurationHandler::SetPropertiesErrorCallback,
                  AsWeakPtr(), service_path, error_callback));
 }
 
 void NetworkConfigurationHandler::ClearProperties(
     const std::string& service_path,
     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,
       base::Bind(&NetworkConfigurationHandler::ClearPropertiesSuccessCallback,
                  AsWeakPtr(), service_path, names, callback, error_callback),
       base::Bind(&NetworkConfigurationHandler::ClearPropertiesErrorCallback,
                  AsWeakPtr(), service_path, error_callback));
 }
 
 void NetworkConfigurationHandler::CreateConfiguration(
     const base::DictionaryValue& properties,
     const network_handler::StringResultCallback& callback,
     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
   // with WiMax, for example.
   if (type == flimflam::kTypeWifi) {
     std::string profile;
     properties.GetStringWithoutPathExpansion(flimflam::kProfileProperty,
                                              &profile);
     manager->ConfigureServiceForProfile(
         dbus::ObjectPath(profile),
@@ skipping 23 matching lines @@
         "RemoveConfiguration In-Progress", service_path, error_callback);
     return;
   }
   NET_LOG_USER("Remove Configuration", service_path);
   ProfileEntryDeleter* deleter =
       new ProfileEntryDeleter(this, service_path, callback, error_callback);
   profile_entry_deleters_[service_path] = deleter;
   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()
     : network_state_handler_(NULL) {
 }
 
 NetworkConfigurationHandler::~NetworkConfigurationHandler() {
   STLDeleteContainerPairSecondPointers(
       profile_entry_deleters_.begin(), profile_entry_deleters_.end());
 }
@@ skipping 98 matching lines @@
 
 // static
 NetworkConfigurationHandler* NetworkConfigurationHandler::InitializeForTest(
     NetworkStateHandler* network_state_handler) {
   NetworkConfigurationHandler* handler = new NetworkConfigurationHandler();
   handler->Init(network_state_handler);
   return handler;
 }
 
 }  // namespace chromeos