chromeos: networkingPrivate: Set active proxy values

extensions/browser/api/networking_private/networking_private_chromeos.cc

Index: extensions/browser/api/networking_private/networking_private_chromeos.cc
diff --git a/extensions/browser/api/networking_private/networking_private_chromeos.cc b/extensions/browser/api/networking_private/networking_private_chromeos.cc
index 48f13a2ad0f6583d670c19746e57b099d3d785b7..d8c3012b2c9ff98a4fb90b7236e654e560f90228 100644
--- a/extensions/browser/api/networking_private/networking_private_chromeos.cc
+++ b/extensions/browser/api/networking_private/networking_private_chromeos.cc
@@ -8,6 +8,7 @@
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/logging.h"
+#include "base/memory/ptr_util.h"
 #include "base/values.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/shill_manager_client.h"
@@ -25,7 +26,10 @@
 #include "chromeos/network/onc/onc_translator.h"
 #include "chromeos/network/onc/onc_utils.h"
 #include "chromeos/network/portal_detector/network_portal_detector.h"
+#include "chromeos/network/proxy/ui_proxy_config.h"
+#include "chromeos/network/proxy/ui_proxy_config_service.h"
 #include "components/onc/onc_constants.h"
+#include "components/proxy_config/proxy_prefs.h"
 #include "content/public/browser/browser_context.h"
 #include "extensions/browser/api/networking_private/networking_private_api.h"
 #include "extensions/browser/extension_registry.h"
@@ -40,6 +44,7 @@ using chromeos::NetworkHandler;
 using chromeos::NetworkStateHandler;
 using chromeos::NetworkTypePattern;
 using chromeos::ShillManagerClient;
+using chromeos::UIProxyConfig;
 using extensions::NetworkingPrivateDelegate;
 
 namespace private_api = extensions::api::networking_private;
@@ -214,6 +219,53 @@ const chromeos::DeviceState* GetCellularDeviceState(const std::string& guid) {
   return device_state;
 }
 
+// Ensures that |container| contains a DictionaryValue for |key| and
+// returns the dictionary.
+base::DictionaryValue* EnsureDictionaryValue(const std::string& key,
+                                             base::DictionaryValue* container) {
+  base::DictionaryValue* dict;
+  if (!container->GetDictionary(key, &dict)) {
+    container->SetWithoutPathExpansion(
+        key, base::MakeUnique<base::DictionaryValue>());
+    container->GetDictionary(key, &dict);
+  }
+  return dict;
+}
+
+// Sets the effective ONC dictionary value of |dict| to 'ActiveExtension' and
+// sets the UserEditable property to false.
+void SetEffectiveToActiveExtension(base::DictionaryValue* dict) {
+  dict->SetStringWithoutPathExpansion(::onc::kAugmentationEffectiveSetting,
+                                      ::onc::kAugmentationActiveExtension);
+  dict->SetBooleanWithoutPathExpansion(::onc::kAugmentationUserEditable, false);
+}
+
+std::string GetProxySettingsType(const UIProxyConfig::Mode& mode) {
+  switch (mode) {
+    case UIProxyConfig::MODE_DIRECT:
+      return ::onc::proxy::kDirect;
+    case UIProxyConfig::MODE_AUTO_DETECT:
+      return ::onc::proxy::kWPAD;
+    case UIProxyConfig::MODE_PAC_SCRIPT:
+      return ::onc::proxy::kPAC;
+    case UIProxyConfig::MODE_SINGLE_PROXY:
+    case UIProxyConfig::MODE_PROXY_PER_SCHEME:
+      return ::onc::proxy::kManual;
+  }
+  NOTREACHED();
+  return ::onc::proxy::kDirect;
+}
+
+void SetManualProxy(base::DictionaryValue* manual,
+                    const std::string& key,

[michaelpg, 2016/11/04 00:58:28]

unused?

[stevenjb, 2016/11/09 22:20:18]

Done.

+                    const UIProxyConfig::ManualProxy& proxy) {
+  manual->SetStringWithoutPathExpansion(::onc::proxy::kHost,
+                                        proxy.server.host_port_pair().host());
+  uint16_t port = proxy.server.host_port_pair().port();
+  manual->SetIntegerWithoutPathExpansion(::onc::proxy::kHost,

[michaelpg, 2016/11/04 00:58:28]

kHost is being set twice...

[stevenjb, 2016/11/09 22:20:18]

Done.

+                                         static_cast<int>(port));
+}
+
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -249,7 +301,8 @@ void NetworkingPrivateChromeOS::GetProperties(
   GetManagedConfigurationHandler()->GetProperties(
       user_id_hash, service_path,
       base::Bind(&NetworkingPrivateChromeOS::GetPropertiesCallback,
-                 weak_ptr_factory_.GetWeakPtr(), success_callback),
+                 weak_ptr_factory_.GetWeakPtr(), guid, false /* managed */,
+                 success_callback),
       base::Bind(&NetworkHandlerFailureCallback, failure_callback));
 }
 
@@ -272,7 +325,8 @@ void NetworkingPrivateChromeOS::GetManagedProperties(
   GetManagedConfigurationHandler()->GetManagedProperties(
       user_id_hash, service_path,
       base::Bind(&NetworkingPrivateChromeOS::GetPropertiesCallback,
-                 weak_ptr_factory_.GetWeakPtr(), success_callback),
+                 weak_ptr_factory_.GetWeakPtr(), guid, true /* managed */,
+                 success_callback),
       base::Bind(&NetworkHandlerFailureCallback, failure_callback));
 }
 
@@ -625,15 +679,18 @@ bool NetworkingPrivateChromeOS::RequestScan() {
 // Private methods
 
 void NetworkingPrivateChromeOS::GetPropertiesCallback(
+    const std::string& guid,
+    bool managed,
     const DictionaryCallback& callback,
     const std::string& service_path,
     const base::DictionaryValue& dictionary) {
   std::unique_ptr<base::DictionaryValue> dictionary_copy(dictionary.DeepCopy());
   AppendThirdPartyProviderName(dictionary_copy.get());
+  if (managed)
+    SetActiveProxyValues(guid, dictionary_copy.get());
   callback.Run(std::move(dictionary_copy));
 }
 
-// Populate ThirdPartyVPN.kProviderName for third-party VPNs.
 void NetworkingPrivateChromeOS::AppendThirdPartyProviderName(
     base::DictionaryValue* dictionary) {
   base::DictionaryValue* third_party_vpn =
@@ -656,6 +713,55 @@ void NetworkingPrivateChromeOS::AppendThirdPartyProviderName(
   }
 }
 
+void NetworkingPrivateChromeOS::SetActiveProxyValues(
+    const std::string& guid,
+    base::DictionaryValue* dictionary) {
+  chromeos::UIProxyConfigService* ui_proxy_config_service =
+      NetworkHandler::Get()->ui_proxy_config_service();
+  ui_proxy_config_service->UpdateFromPrefs(guid);
+  UIProxyConfig config;
+  ui_proxy_config_service->GetProxyConfig(guid, &config);
+
+  // We only override the active values if the proxy is set by an extension.
+  if (config.state != ProxyPrefs::CONFIG_EXTENSION)
+    return;
+
+  // Ensure that the ProxySettings dictionary exists.

[michaelpg, 2016/11/04 00:58:28]

The rest of the function assumes kProxySettings is a ManagedProxySettings dict
(not a ProxySettings dict). Is that because CONFIG_EXTENSION means that
|dictionary| is a ManagedProperties? There's an odd dependency on UIProxyConfig
I wouldn't have expected; can you add a comment to the effect that we know we're
dealing with managed properties?

[stevenjb, 2016/11/09 22:20:18]

This is only called for managed dictionaries (managed is true in
GetPropertiesCallback). I renamed this and updated the comment in the header.

I also commented about the fubar nature of using UIProxyConfig.

+  base::DictionaryValue* proxy_settings =
+      EnsureDictionaryValue(::onc::network_config::kProxySettings, dictionary);
+
+  // Ensure that the ProxySettings.Type dictionary exists and set the active
+  // type value to the value from |ui_proxy_config_service|.
+  base::DictionaryValue* proxy_type_dict =
+      EnsureDictionaryValue(::onc::network_config::kType, proxy_settings);
+  SetEffectiveToActiveExtension(proxy_type_dict);
+  proxy_type_dict->SetStringWithoutPathExpansion(
+      ::onc::kAugmentationActiveSetting, GetProxySettingsType(config.mode));
+
+  // Update any appropriate sub dictionary based on the new type.
+  if (config.mode == UIProxyConfig::MODE_SINGLE_PROXY) {
+    base::DictionaryValue* manual =
+        EnsureDictionaryValue(::onc::proxy::kManual, proxy_settings);
+    SetManualProxy(manual, ::onc::proxy::kHttp, config.single_proxy);
+    manual->RemoveWithoutPathExpansion(::onc::proxy::kHttps, nullptr);

[emaxx, 2016/11/03 21:15:44]

I'm lacking the context here, but maybe some explanation would be helpful
regarding the reason why these values are removed in this case and why this is
not necessary in other cases.

[michaelpg, 2016/11/04 00:58:28]

+1; SetManualProxy doesn't seem like it's doing whatever it's supposed to be
doing. Also not sure why a single proxy has to be http or if http is the
default; don't see that documented anywhere.

[stevenjb, 2016/11/09 22:20:18]

Short version: We only care about kHttp when we indicate "use the same proxy for
all protocols". However, I have since determined that it makes more sense just
to set all properties to the same value (which is what Shill does). I also added
a comment.

+    manual->RemoveWithoutPathExpansion(::onc::proxy::kFtp, nullptr);
+    manual->RemoveWithoutPathExpansion(::onc::proxy::kSocks, nullptr);
+  } else if (config.mode == UIProxyConfig::MODE_PROXY_PER_SCHEME) {
+    base::DictionaryValue* manual =
+        EnsureDictionaryValue(::onc::proxy::kManual, proxy_settings);
+    SetManualProxy(manual, ::onc::proxy::kHttp, config.http_proxy);
+    SetManualProxy(manual, ::onc::proxy::kHttps, config.https_proxy);
+    SetManualProxy(manual, ::onc::proxy::kFtp, config.ftp_proxy);
+    SetManualProxy(manual, ::onc::proxy::kSocks, config.socks_proxy);
+  } else if (config.mode == UIProxyConfig::MODE_PAC_SCRIPT) {
+    base::DictionaryValue* pac =
+        EnsureDictionaryValue(::onc::proxy::kPAC, proxy_settings);
+    SetEffectiveToActiveExtension(pac);
+    pac->SetStringWithoutPathExpansion(::onc::kAugmentationActiveSetting,
+                                       config.automatic_proxy.pac_url.spec());
+  }
+}
+
 void NetworkingPrivateChromeOS::ConnectFailureCallback(
     const std::string& guid,
     const VoidCallback& success_callback,