Use GUID instead of ServicePath in networkingPrivate API

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

Index: chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc
diff --git a/chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc b/chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc
index 685290e6ed11e19d517e0dc15b87c65f4be4fcd9..00be4f0ceb3ca8429d7cae669f32a3d68beab9b3 100644
--- a/chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc
+++ b/chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc
@@ -16,11 +16,13 @@
 #include "chrome/common/extensions/api/networking_private.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/shill_manager_client.h"
+#include "chromeos/network/favorite_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_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"
@@ -31,6 +33,7 @@
 namespace api = extensions::api::networking_private;
 
 using chromeos::DBusThreadManager;
+using chromeos::FavoriteState;
 using chromeos::ManagedNetworkConfigurationHandler;
 using chromeos::NetworkHandler;
 using chromeos::NetworkPortalDetector;
@@ -71,6 +74,20 @@ std::string GetUserIdHash(Profile* profile) {
   }
 }
 
+bool GetServicePathFromGuid(const std::string& guid,
+                            std::string* service_path,
+                            std::string* error) {
+  const FavoriteState* network =
+      NetworkHandler::Get()->network_state_handler()->GetFavoriteStateFromGuid(
+          guid);
+  if (!network) {
+    *error = "Error.InvalidNetworkGuid";
+    return false;
+  }
+  *service_path = network->path();
+  return true;
+}
+
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -84,9 +101,12 @@ bool NetworkingPrivateGetPropertiesFunction::RunAsync() {
   scoped_ptr<api::GetProperties::Params> params =
       api::GetProperties::Params::Create(*args_);
   EXTENSION_FUNCTION_VALIDATE(params);
+  std::string service_path;
+  if (!GetServicePathFromGuid(params->network_guid, &service_path, &error_))
+    return false;
 
   NetworkHandler::Get()->managed_network_configuration_handler()->GetProperties(
-      params->network_guid,  // service path
+      service_path,
       base::Bind(&NetworkingPrivateGetPropertiesFunction::GetPropertiesSuccess,
                  this),
       base::Bind(&NetworkingPrivateGetPropertiesFunction::GetPropertiesFailed,
@@ -97,10 +117,7 @@ bool NetworkingPrivateGetPropertiesFunction::RunAsync() {
 void NetworkingPrivateGetPropertiesFunction::GetPropertiesSuccess(
     const std::string& service_path,
     const base::DictionaryValue& dictionary) {
-  base::DictionaryValue* network_properties = dictionary.DeepCopy();
-  network_properties->SetStringWithoutPathExpansion(onc::network_config::kGUID,
-                                                    service_path);
-  SetResult(network_properties);
+  SetResult(dictionary.DeepCopy());
   SendResponse(true);
 }
 
@@ -122,13 +139,16 @@ bool NetworkingPrivateGetManagedPropertiesFunction::RunAsync() {
   scoped_ptr<api::GetManagedProperties::Params> params =
       api::GetManagedProperties::Params::Create(*args_);
   EXTENSION_FUNCTION_VALIDATE(params);
+  std::string service_path;
+  if (!GetServicePathFromGuid(params->network_guid, &service_path, &error_))
+    return false;
 
   std::string user_id_hash;

[pneubeck (no reviews), 2014/05/12 13:37:07]

?? how has this ever worked?
   ';' -> '='

please verify also in the other functions

[stevenjb, 2014/05/13 01:19:00]

Yeah, the 'user_id_hash' code is completely broken and untested. I started a fix
in https://codereview.chromium.org/242983004/ if you recall, but have not gotten
back to it (in part because the desired behavior was uncertain).

The latest decision is that networkingPrivate should always affect the default
user profile in a multi-profile environment (any thing else is just too
confusing) so we may need to change the API or at least the documentation.

Regardless, I will deal with this in that CL.

   GetUserIdHash(GetProfile());
   NetworkHandler::Get()->managed_network_configuration_handler()->
       GetManagedProperties(
           user_id_hash,
-          params->network_guid,  // service path
+          service_path,
           base::Bind(&NetworkingPrivateGetManagedPropertiesFunction::Success,
                      this),
           base::Bind(&NetworkingPrivateGetManagedPropertiesFunction::Failure,
@@ -139,10 +159,7 @@ bool NetworkingPrivateGetManagedPropertiesFunction::RunAsync() {
 void NetworkingPrivateGetManagedPropertiesFunction::Success(
     const std::string& service_path,
     const base::DictionaryValue& dictionary) {
-  base::DictionaryValue* network_properties = dictionary.DeepCopy();
-  network_properties->SetStringWithoutPathExpansion(onc::network_config::kGUID,
-                                                    service_path);
-  SetResult(network_properties);
+  SetResult(dictionary.DeepCopy());
   SendResponse(true);
 }
 
@@ -164,13 +181,14 @@ bool NetworkingPrivateGetStateFunction::RunAsync() {
   scoped_ptr<api::GetState::Params> params =
       api::GetState::Params::Create(*args_);
   EXTENSION_FUNCTION_VALIDATE(params);
-  // The |network_guid| parameter is storing the service path.
-  std::string service_path = params->network_guid;
+  std::string service_path;
+  if (!GetServicePathFromGuid(params->network_guid, &service_path, &error_))
+    return false;
 
   const NetworkState* state = NetworkHandler::Get()->network_state_handler()->
       GetNetworkState(service_path);
   if (!state) {
-    error_ = "Error.InvalidParameter";
+    error_ = "Error.NetworkUnavailable";

[pneubeck (no reviews), 2014/05/12 13:37:07]

I think, GetState should also return successfully for not in-range favorite
networks; likely with a reduced set of properties. At a minimum, it must be
documented in the API description.

However better not in this CL.

[stevenjb, 2014/05/13 01:19:00]

In the "getNetworks" CL I fix this for getNetworks and getVisibleNetworks, I
will make a note to fix this for getState also.

     return false;
   }
 
@@ -196,12 +214,15 @@ bool NetworkingPrivateSetPropertiesFunction::RunAsync() {
   scoped_ptr<api::SetProperties::Params> params =
       api::SetProperties::Params::Create(*args_);
   EXTENSION_FUNCTION_VALIDATE(params);
+  std::string service_path;
+  if (!GetServicePathFromGuid(params->network_guid, &service_path, &error_))
+    return false;
 
   scoped_ptr<base::DictionaryValue> properties_dict(
       params->properties.ToValue());
 
   NetworkHandler::Get()->managed_network_configuration_handler()->SetProperties(
-      params->network_guid,  // service path
+      service_path,
       *properties_dict,
       base::Bind(&NetworkingPrivateSetPropertiesFunction::ResultCallback,
                  this),
@@ -275,30 +296,12 @@ bool NetworkingPrivateGetVisibleNetworksFunction::RunAsync() {
   scoped_ptr<api::GetVisibleNetworks::Params> params =
       api::GetVisibleNetworks::Params::Create(*args_);
   EXTENSION_FUNCTION_VALIDATE(params);
-  NetworkTypePattern type = chromeos::onc::NetworkTypePatternFromOncType(
+  NetworkTypePattern pattern = chromeos::onc::NetworkTypePatternFromOncType(
       api::GetVisibleNetworks::Params::ToString(params->type));
 
-  NetworkStateHandler::NetworkStateList network_states;
-  NetworkHandler::Get()->network_state_handler()->GetNetworkListByType(
-      type, &network_states);
-
-  base::ListValue* network_properties_list = new base::ListValue;
-  for (NetworkStateHandler::NetworkStateList::iterator it =
-           network_states.begin();
-       it != network_states.end(); ++it) {
-    base::DictionaryValue shill_dictionary;
-    (*it)->GetStateProperties(&shill_dictionary);
-
-    scoped_ptr<base::DictionaryValue> onc_network_part =
-        chromeos::onc::TranslateShillServiceToONCPart(
-            shill_dictionary, &chromeos::onc::kNetworkWithStateSignature);
-    // TODO(stevenjb): Fix this to always use GUID: crbug.com/284827
-    onc_network_part->SetStringWithoutPathExpansion(
-        onc::network_config::kGUID, (*it)->path());
-    network_properties_list->Append(onc_network_part.release());
-  }
-
-  SetResult(network_properties_list);
+  scoped_ptr<base::ListValue> network_properties_list =
+      chromeos::network_util::TranslateNetworkListToONC(pattern);
+  SetResult(network_properties_list.release());
   SendResponse(true);
   return true;
 }
@@ -439,10 +442,13 @@ bool NetworkingPrivateStartConnectFunction::RunAsync() {
   scoped_ptr<api::StartConnect::Params> params =
       api::StartConnect::Params::Create(*args_);
   EXTENSION_FUNCTION_VALIDATE(params);
+  std::string service_path;
+  if (!GetServicePathFromGuid(params->network_guid, &service_path, &error_))
+    return false;
 
   const bool check_error_state = false;
   NetworkHandler::Get()->network_connection_handler()->ConnectToNetwork(
-      params->network_guid,  // service path
+      service_path,
       base::Bind(
           &NetworkingPrivateStartConnectFunction::ConnectionStartSuccess,
           this),
@@ -475,9 +481,12 @@ bool NetworkingPrivateStartDisconnectFunction::RunAsync() {
   scoped_ptr<api::StartDisconnect::Params> params =
       api::StartDisconnect::Params::Create(*args_);
   EXTENSION_FUNCTION_VALIDATE(params);
+  std::string service_path;
+  if (!GetServicePathFromGuid(params->network_guid, &service_path, &error_))
+    return false;
 
   NetworkHandler::Get()->network_connection_handler()->DisconnectNetwork(
-      params->network_guid,  // service path
+      service_path,
       base::Bind(
           &NetworkingPrivateStartDisconnectFunction::DisconnectionStartSuccess,
           this),
@@ -536,15 +545,18 @@ bool NetworkingPrivateVerifyAndEncryptCredentialsFunction::RunAsync() {
   scoped_ptr<api::VerifyAndEncryptCredentials::Params> params =
       api::VerifyAndEncryptCredentials::Params::Create(*args_);
   EXTENSION_FUNCTION_VALIDATE(params);
-  ShillManagerClient* shill_manager_client =
-      DBusThreadManager::Get()->GetShillManagerClient();
+  std::string service_path;
+  if (!GetServicePathFromGuid(params->network_guid, &service_path, &error_))
+    return false;
 
   ShillManagerClient::VerificationProperties verification_properties =
       ConvertVerificationProperties(params->properties);
 
+  ShillManagerClient* shill_manager_client =
+      DBusThreadManager::Get()->GetShillManagerClient();
   shill_manager_client->VerifyAndEncryptCredentials(
       verification_properties,
-      params->guid,
+      service_path,
       base::Bind(
           &NetworkingPrivateVerifyAndEncryptCredentialsFunction::ResultCallback,
           this),
@@ -694,6 +706,9 @@ bool NetworkingPrivateGetCaptivePortalStatusFunction::RunAsync() {
   scoped_ptr<api::GetCaptivePortalStatus::Params> params =
       api::GetCaptivePortalStatus::Params::Create(*args_);
   EXTENSION_FUNCTION_VALIDATE(params);
+  std::string service_path;
+  if (!GetServicePathFromGuid(params->network_guid, &service_path, &error_))
+    return false;
 
   NetworkPortalDetector* detector = NetworkPortalDetector::Get();
   if (!detector) {
@@ -702,7 +717,7 @@ bool NetworkingPrivateGetCaptivePortalStatusFunction::RunAsync() {
   }
 
   NetworkPortalDetector::CaptivePortalState state =
-      detector->GetCaptivePortalState(params->network_path);
+      detector->GetCaptivePortalState(service_path);
 
   SetResult(new base::StringValue(
       NetworkPortalDetector::CaptivePortalStatusString(state.status)));