Add NetworkingPrivateDelegate class

chrome/browser/extensions/api/networking_private/networking_private_chromeos.cc

+// Copyright 2013 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/extensions/api/networking_private/networking_private_chromeos.h"
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/callback.h"
+#include "chrome/browser/chromeos/profiles/profile_helper.h"
+#include "chrome/browser/extensions/api/networking_private/networking_private_api.h"
+#include "chrome/common/extensions/api/networking_private.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/shill_manager_client.h"
+#include "chromeos/login/login_state.h"
+#include "chromeos/network/managed_network_configuration_handler.h"
+#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_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_translator.h"
+#include "chromeos/network/onc/onc_utils.h"
+#include "chromeos/network/portal_detector/network_portal_detector.h"
+#include "components/onc/onc_constants.h"
+#include "content/public/browser/browser_context.h"
+
+using chromeos::NetworkHandler;
+using chromeos::NetworkTypePattern;
+using chromeos::ShillManagerClient;
+using extensions::NetworkingPrivateDelegate;
+
+namespace {
+
+chromeos::ShillManagerClient* GetShillManagerClient() {
+  return chromeos::DBusThreadManager::Get()->GetShillManagerClient();
+}
+
+chromeos::NetworkStateHandler* GetStateHandler() {
+  return NetworkHandler::Get()->network_state_handler();
+}
+
+chromeos::ManagedNetworkConfigurationHandler* GetManagedConfigurationHandler() {
+  return NetworkHandler::Get()->managed_network_configuration_handler();
+}
+
+ShillManagerClient::VerificationProperties ConvertVerificationProperties(
+    const base::DictionaryValue& input) {
+  ShillManagerClient::VerificationProperties output;
+  // These keys are described in networking_private.json but not defined
+  // anywhere as constants.
+  input.GetStringWithoutPathExpansion("certificate", &output.certificate);

[pneubeck (no reviews), 2014/07/14 21:33:57]

IMO, a layer violation

[stevenjb, 2014/07/15 00:33:40]

Acknowledged.

+  input.GetStringWithoutPathExpansion("publicKey", &output.public_key);
+  input.GetStringWithoutPathExpansion("nonce", &output.nonce);
+  input.GetStringWithoutPathExpansion("signedData", &output.signed_data);
+  input.GetStringWithoutPathExpansion("deviceSerial", &output.device_serial);
+  input.GetStringWithoutPathExpansion("deviceSsid", &output.device_ssid);
+  input.GetStringWithoutPathExpansion("deviceBssid", &output.device_bssid);
+  return output;
+}
+
+bool GetServicePathFromGuid(const std::string& guid,
+                            std::string* service_path,
+                            std::string* error) {
+  const chromeos::NetworkState* network =
+      GetStateHandler()->GetNetworkStateFromGuid(guid);
+  if (!network) {
+    *error = extensions::networking_private::kErrorInvalidNetworkGuid;

[pneubeck (no reviews), 2014/07/14 21:33:57]

again, I think, a layer violation.
Not so, if the error constants were declared/defined in _delegate.h/.cc

If they're also defined for the API, we would probably have to translate them
:-/

[stevenjb, 2014/07/15 00:33:40]

See meta-comment.

+    return false;
+  }
+  *service_path = network->path();
+  return true;
+}
+
+bool GetUserIdHash(content::BrowserContext* browser_context,
+                   std::string* user_hash,
+                   std::string* error) {
+  std::string profile_user_hash =
+      chromeos::ProfileHelper::GetUserIdHashFromProfile(

[pneubeck (no reviews), 2014/07/14 21:33:58]

this dependency should be in the factory, to keep the dependencies of the
service minimal.

[stevenjb, 2014/07/15 00:33:39]

Hmm, I'm not convinced of the value in that. This is all "glue" code and tested
at a high level, I don't think it is a good candidate for unit testing where
tracking dependent services individually would be helpful, and I don't think
shifting the dependencies from the implementation to the factory would make this
any easier to maintain.

[pneubeck (no reviews), 2014/07/15 09:56:28]

Moving dependencies to a factory is not only helpful for testing. It's also
helping the reader to keep track of dependencies.
One look at the constructor of the service, and you know all dependencies of the
service.
One look at the create function in the factory and you know where the
dependencies come from.

Construction order is made more explicit/static, that's one of the main points
of the BCKS movement: Are all dependencies available before the new service is
constructed or not? A DCHECK in the factory ensures that without depending on
the actual usage of member functions of the service (e.g. if GetUserIdHash is
not called in a test, this will never crash and hide the broken construction
order).

There are many examples where this helped readability and maintainability a lot.

+          static_cast<Profile*>(browser_context));
+
+  // Currently Chrome OS only configures networks for the primary user.
+  // Configuration attempts from other browser contexts should fail.
+  // TODO(stevenjb): use an ExtensionsBrowserClient method to access
+  // ProfileHelper when moving this to src/extensions.
+  if (profile_user_hash != chromeos::LoginState::Get()->primary_user_hash()) {
+    // Disallow class requiring a user id hash from a non-primary user context
+    // to avoid complexites with the policy code.

[pneubeck (no reviews), 2014/07/14 21:33:57]

complications or complexities?

[stevenjb, 2014/07/15 00:33:39]

Done.

+    LOG(ERROR) << "networkingPrivate API call from non primary user: "
+               << profile_user_hash;
+    *error = "Error.NonPrimaryUser";
+    return false;
+  }
+  *user_hash = profile_user_hash;
+  return true;
+}
+
+void NetworkHandlerDictionaryCallback(
+    const NetworkingPrivateDelegate::DictionaryCallback& callback,
+    const std::string& service_path,
+    const base::DictionaryValue& dictionary) {
+  scoped_ptr<base::DictionaryValue> dictionary_copy(dictionary.DeepCopy());
+  callback.Run(dictionary_copy.Pass());
+}
+
+void NetworkHandlerFailureCallback(
+    const NetworkingPrivateDelegate::FailureCallback& callback,
+    const std::string& error_name,
+    scoped_ptr<base::DictionaryValue> error_data) {
+  callback.Run(error_name);
+}
+
+void ShillFailureCallback(
+    const NetworkingPrivateDelegate::FailureCallback& callback,
+    const std::string& error_name,
+    const std::string& error_message) {
+  callback.Run(error_name);
+}
+
+}  // namespace
+
+////////////////////////////////////////////////////////////////////////////////
+
+namespace extensions {
+
+NetworkingPrivateChromeOS::NetworkingPrivateChromeOS(
+    content::BrowserContext* browser_context)
+    : browser_context_(browser_context) {

[pneubeck (no reviews), 2014/07/14 21:33:58]

as mentioned above, please get the user hash once in the factory and don't store
the BrowserContext.

[stevenjb, 2014/07/15 00:33:40]

Acknowledged.

+}
+
+NetworkingPrivateChromeOS::~NetworkingPrivateChromeOS() {}
+
+void NetworkingPrivateChromeOS::GetProperties(
+    const std::string& guid,
+    const DictionaryCallback& success_callback,
+    const FailureCallback& failure_callback) {
+  std::string service_path, error;
+  if (!GetServicePathFromGuid(guid, &service_path, &error)) {
+    failure_callback.Run(error);
+    return;
+  }
+
+  GetManagedConfigurationHandler()->GetProperties(
+      service_path,
+      base::Bind(&NetworkHandlerDictionaryCallback, success_callback),
+      base::Bind(&NetworkHandlerFailureCallback, failure_callback));
+}
+
+void NetworkingPrivateChromeOS::GetManagedProperties(
+    const std::string& guid,
+    const DictionaryCallback& success_callback,
+    const FailureCallback& failure_callback) {
+  std::string service_path, error;
+  if (!GetServicePathFromGuid(guid, &service_path, &error)) {
+    failure_callback.Run(error);
+    return;
+  }
+
+  std::string user_id_hash;
+  if (!GetUserIdHash(browser_context_, &user_id_hash, &error)) {
+    failure_callback.Run(error);
+    return;
+  }
+
+  GetManagedConfigurationHandler()->GetManagedProperties(
+      user_id_hash,
+      service_path,
+      base::Bind(&NetworkHandlerDictionaryCallback, success_callback),
+      base::Bind(&NetworkHandlerFailureCallback, failure_callback));
+}
+
+void NetworkingPrivateChromeOS::GetState(
+    const std::string& guid,
+    const DictionaryCallback& success_callback,
+    const FailureCallback& failure_callback) {
+  std::string service_path, error;
+  if (!GetServicePathFromGuid(guid, &service_path, &error)) {
+    failure_callback.Run(error);
+    return;
+  }
+
+  const chromeos::NetworkState* network_state =
+      GetStateHandler()->GetNetworkStateFromServicePath(
+          service_path, false /* configured_only */);
+  if (!network_state) {
+    failure_callback.Run(networking_private::kErrorNetworkUnavailable);
+    return;
+  }
+
+  scoped_ptr<base::DictionaryValue> network_properties =
+      chromeos::network_util::TranslateNetworkStateToONC(network_state);
+
+  success_callback.Run(network_properties.Pass());
+}
+
+void NetworkingPrivateChromeOS::SetProperties(
+    const std::string& guid,
+    scoped_ptr<base::DictionaryValue> properties,
+    const VoidCallback& success_callback,
+    const FailureCallback& failure_callback) {
+  std::string service_path, error;
+  if (!GetServicePathFromGuid(guid, &service_path, &error)) {
+    failure_callback.Run(error);
+    return;
+  }
+
+  GetManagedConfigurationHandler()->SetProperties(
+      service_path,
+      *properties,
+      success_callback,
+      base::Bind(&NetworkHandlerFailureCallback, failure_callback));
+}
+
+void NetworkingPrivateChromeOS::CreateNetwork(
+    bool shared,
+    scoped_ptr<base::DictionaryValue> properties,
+    const StringCallback& success_callback,
+    const FailureCallback& failure_callback) {
+  std::string user_id_hash, error;
+  if (!shared && !GetUserIdHash(browser_context_, &user_id_hash, &error)) {
+    failure_callback.Run(error);

[pneubeck (no reviews), 2014/07/14 21:33:58]

optional nit:
the old comment was no so bad:
-    // Do not allow configuring a non-shared network from a non-primary user
-    // context.

[stevenjb, 2014/07/15 00:33:39]

Done.

+    return;
+  }
+
+  GetManagedConfigurationHandler()->CreateConfiguration(
+      user_id_hash,
+      *properties,
+      success_callback,
+      base::Bind(&NetworkHandlerFailureCallback, failure_callback));
+}
+
+void NetworkingPrivateChromeOS::GetNetworks(
+    const std::string& network_type,
+    bool configured_only,
+    bool visible_only,
+    int limit,
+    const NetworkListCallback& success_callback,
+    const FailureCallback& failure_callback) {
+  NetworkTypePattern pattern =
+      chromeos::onc::NetworkTypePatternFromOncType(network_type);
+  scoped_ptr<base::ListValue> network_properties_list =
+      chromeos::network_util::TranslateNetworkListToONC(
+          pattern, configured_only, visible_only, limit, false /* debugging */);
+  success_callback.Run(network_properties_list.Pass());
+}
+
+void NetworkingPrivateChromeOS::StartConnect(
+    const std::string& guid,
+    const VoidCallback& success_callback,
+    const FailureCallback& failure_callback) {
+  std::string service_path, error;
+  if (!GetServicePathFromGuid(guid, &service_path, &error)) {
+    failure_callback.Run(error);
+    return;
+  }
+
+  const bool check_error_state = false;
+  NetworkHandler::Get()->network_connection_handler()->ConnectToNetwork(
+      service_path,
+      success_callback,
+      base::Bind(&NetworkHandlerFailureCallback, failure_callback),
+      check_error_state);
+}
+
+void NetworkingPrivateChromeOS::StartDisconnect(
+    const std::string& guid,
+    const VoidCallback& success_callback,
+    const FailureCallback& failure_callback) {
+  std::string service_path, error;
+  if (!GetServicePathFromGuid(guid, &service_path, &error)) {
+    failure_callback.Run(error);
+    return;
+  }
+
+  NetworkHandler::Get()->network_connection_handler()->DisconnectNetwork(
+      service_path,
+      success_callback,
+      base::Bind(&NetworkHandlerFailureCallback, failure_callback));
+}
+
+void NetworkingPrivateChromeOS::VerifyDestination(
+    scoped_ptr<base::DictionaryValue> verification_properties,
+    const BoolCallback& success_callback,
+    const FailureCallback& failure_callback) {
+  ShillManagerClient::VerificationProperties verification_property_struct =
+      ConvertVerificationProperties(*verification_properties);
+
+  GetShillManagerClient()->VerifyDestination(
+      verification_property_struct,
+      success_callback,
+      base::Bind(&ShillFailureCallback, failure_callback));
+}
+
+void NetworkingPrivateChromeOS::VerifyAndEncryptCredentials(
+    const std::string& guid,
+    scoped_ptr<base::DictionaryValue> verification_properties,
+    const StringCallback& success_callback,
+    const FailureCallback& failure_callback) {
+  std::string service_path, error;
+  if (!GetServicePathFromGuid(guid, &service_path, &error)) {
+    failure_callback.Run(error);
+    return;
+  }
+
+  ShillManagerClient::VerificationProperties verification_property_struct =
+      ConvertVerificationProperties(*verification_properties);
+
+  GetShillManagerClient()->VerifyAndEncryptCredentials(
+      verification_property_struct,
+      service_path,
+      success_callback,
+      base::Bind(&ShillFailureCallback, failure_callback));
+}
+
+void NetworkingPrivateChromeOS::VerifyAndEncryptData(
+    scoped_ptr<base::DictionaryValue> verification_properties,
+    const std::string& data,
+    const StringCallback& success_callback,
+    const FailureCallback& failure_callback) {
+  ShillManagerClient::VerificationProperties verification_property_struct =
+      ConvertVerificationProperties(*verification_properties);
+
+  GetShillManagerClient()->VerifyAndEncryptData(
+      verification_property_struct,
+      data,
+      success_callback,
+      base::Bind(&ShillFailureCallback, failure_callback));
+}
+
+void NetworkingPrivateChromeOS::SetWifiTDLSEnabledState(
+    const std::string& ip_or_mac_address,
+    bool enabled,
+    const StringCallback& success_callback,
+    const FailureCallback& failure_callback) {
+  NetworkHandler::Get()->network_device_handler()->SetWifiTDLSEnabled(
+      ip_or_mac_address,
+      enabled,
+      success_callback,
+      base::Bind(&NetworkHandlerFailureCallback, failure_callback));
+}
+
+void NetworkingPrivateChromeOS::GetWifiTDLSStatus(
+    const std::string& ip_or_mac_address,
+    const StringCallback& success_callback,
+    const FailureCallback& failure_callback) {
+  NetworkHandler::Get()->network_device_handler()->GetWifiTDLSStatus(
+      ip_or_mac_address,
+      success_callback,
+      base::Bind(&NetworkHandlerFailureCallback, failure_callback));
+}
+
+void NetworkingPrivateChromeOS::GetCaptivePortalStatus(
+    const std::string& guid,
+    const StringCallback& success_callback,
+    const FailureCallback& failure_callback) {
+  std::string service_path, error;
+  if (!GetServicePathFromGuid(guid, &service_path, &error)) {
+    failure_callback.Run(error);
+    return;
+  }
+
+  chromeos::NetworkPortalDetector* detector =
+      chromeos::NetworkPortalDetector::Get();

[pneubeck (no reviews), 2014/07/14 21:33:57]

should be passed in by the factory through the constructor

and yes, I suggest to do the same with all network handlers as well.

[stevenjb, 2014/07/15 00:33:40]

See comment above.

+  if (!detector) {
+    failure_callback.Run(networking_private::kErrorNotReady);
+    return;
+  }
+
+  chromeos::NetworkPortalDetector::CaptivePortalState state =
+      detector->GetCaptivePortalState(service_path);
+  success_callback.Run(
+      chromeos::NetworkPortalDetector::CaptivePortalStatusString(state.status));
+}
+
+scoped_ptr<base::ListValue>
+NetworkingPrivateChromeOS::GetEnabledNetworkTypes() {
+  chromeos::NetworkStateHandler* state_handler = GetStateHandler();
+
+  scoped_ptr<base::ListValue> network_list(new base::ListValue);
+
+  if (state_handler->IsTechnologyEnabled(NetworkTypePattern::Ethernet()))
+    network_list->AppendString(api::networking_private::ToString(
+        api::networking_private::NETWORK_TYPE_ETHERNET));

[pneubeck (no reviews), 2014/07/14 21:33:57]

this translation should be in the _api.cc

[stevenjb, 2014/07/15 00:33:40]

Done.

+  if (state_handler->IsTechnologyEnabled(NetworkTypePattern::WiFi()))
+    network_list->AppendString(api::networking_private::ToString(
+        api::networking_private::NETWORK_TYPE_WIFI));
+  if (state_handler->IsTechnologyEnabled(NetworkTypePattern::Wimax()))
+    network_list->AppendString(api::networking_private::ToString(
+        api::networking_private::NETWORK_TYPE_WIMAX));
+  if (state_handler->IsTechnologyEnabled(NetworkTypePattern::Cellular()))
+    network_list->AppendString(api::networking_private::ToString(
+        api::networking_private::NETWORK_TYPE_CELLULAR));
+
+  return network_list.Pass();
+}
+
+bool NetworkingPrivateChromeOS::EnableNetworkType(const std::string& type) {
+  NetworkTypePattern pattern =
+      chromeos::onc::NetworkTypePatternFromOncType(type);
+
+  GetStateHandler()->SetTechnologyEnabled(
+      pattern, true, chromeos::network_handler::ErrorCallback());
+
+  return true;
+}
+
+bool NetworkingPrivateChromeOS::DisableNetworkType(const std::string& type) {
+  NetworkTypePattern pattern =
+      chromeos::onc::NetworkTypePatternFromOncType(type);
+
+  GetStateHandler()->SetTechnologyEnabled(
+      pattern, false, chromeos::network_handler::ErrorCallback());
+
+  return true;
+}
+
+bool NetworkingPrivateChromeOS::RequestScan() {
+  GetStateHandler()->RequestScan();
+  return true;
+}
+
+}  // namespace extensions