WiFi client for GCD bootstrapping

chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc

+// Copyright 2014 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 "base/cancelable_callback.h"
+#include "base/threading/sequenced_worker_pool.h"
+#include "base/threading/thread.h"
+#include "chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.h"
+#include "components/onc/onc_constants.h"
+#include "components/wifi/wifi_service.h"
+#include "content/public/browser/browser_thread.h"
+
+using wifi::WiFiService;

[stevenjb, 2014/05/15 18:16:26]

nit: ::wifi::WiFiService would make the need for this a little more clear (since
the rest of the file is in local_discovery::wifi)

[Noam Samuel, 2014/05/20 21:19:53]

Done.

+
+namespace local_discovery {
+
+namespace wifi {
+
+namespace {
+
+const int kConnectionTimeoutSeconds = 10;
+
+scoped_ptr<base::DictionaryValue> MakeProperties(const std::string& ssid,
+                                                 const std::string& password) {
+  scoped_ptr<base::DictionaryValue> properties(new base::DictionaryValue);
+
+  properties->SetString(onc::network_config::kType, onc::network_type::kWiFi);
+  base::DictionaryValue* wifi = new base::DictionaryValue;
+  properties->Set(onc::network_config::kWiFi, wifi);
+
+  wifi->SetString(onc::wifi::kSSID, ssid);
+  wifi->SetString(onc::wifi::kPassphrase, password);
+
+  return properties.Pass();
+}
+
+}  // namespace
+
+class NetworkListObserverNonChromeos : public NetworkListObserver {
+ public:
+  NetworkListObserverNonChromeos(
+      const WifiManager::SSIDListCallback& callback,
+      base::WeakPtr<WifiManagerNonChromeos> wifi_manager);
+  virtual ~NetworkListObserverNonChromeos();
+
+  virtual void Start() OVERRIDE;
+
+  void OnNetworkListChanged(const std::vector<NetworkProperties>& ssid_list);
+
+ private:
+  WifiManager::SSIDListCallback callback_;
+  bool started_;
+  base::WeakPtr<WifiManagerNonChromeos> wifi_manager_;

[stevenjb, 2014/05/15 18:16:26]

DISALLOW_COPY_AND_ASSIGN

+};
+
+class WifiServiceWrapper {

[stevenjb, 2014/05/15 18:16:26]

I think this and NetworkListObserverNonChromeos would be more clear as member
classes of WifiManagerNonChromeos (you can still forward declare them in the
header, just do it inside the class).

This would allow you to make WifiManagerNonChromeos::Add/RemoveObserver private
(it is confusing to have public methods that are not actually publicly usable),
and would make it a little more clear that these are implementation details of
WifiManagerNonChromeos.

[Noam Samuel, 2014/05/20 21:19:53]

Done.

+ public:
+  WifiServiceWrapper(
+      const base::Callback<void(const base::Closure&)>& post_callback,
+      const WifiManager::SSIDListCallback& network_list_update);
+
+  ~WifiServiceWrapper();
+
+  void Start();
+
+  void GetSSIDList(const WifiManager::SSIDListCallback& callback);
+
+  void ConnectToNetwork(const std::string& ssid,
+                        const std::string& password,

[stevenjb, 2014/05/15 18:16:26]

Take Credentials or name ConnectToPskNetwork.

[Noam Samuel, 2014/05/20 21:19:53]

Done.

+                        const WifiManager::SuccessCallback& callback);
+
+  base::WeakPtr<WifiServiceWrapper> AsWeakPtr();
+
+  void RequestScan(const base::Closure& callback);
+
+  void ConnectToNetworkByID(const std::string& network_guid,
+                            const WifiManager::SuccessCallback& callback);
+
+  void GetNetworkCredentials(const std::string& network_guid,
+                             const WifiManager::CredentialsCallback& callback);
+
+ private:
+  void GetSSIDListInternal(std::vector<NetworkProperties>* ssid_list);
+
+  void OnNetworkListChangedEvent(const std::vector<std::string>& network_guids);
+
+  void OnNetworksChangedEvent(const std::vector<std::string>& network_guids);
+
+  std::string GetConnectedGUID();
+
+  bool IsConnected(const std::string& network_guid);
+
+  void OnConnectToNetworkTimeout();
+
+  scoped_ptr<WiFiService> wifi_service_;
+
+  base::Callback<void(const base::Closure&)> post_callback_;
+  WifiManager::SSIDListCallback network_list_update_;
+
+  WifiManager::SuccessCallback connect_success_callback_;
+  base::CancelableClosure connect_failure_callback_;
+  std::string connected_network_guid_;  // SSID of previously connected network.
+  std::string
+      connecting_network_guid_;  // SSID of network we are connecting to.
+
+  base::WeakPtrFactory<WifiServiceWrapper> weak_factory_;

[stevenjb, 2014/05/15 18:16:26]

DISALLOW_COPY_AND_ASSIGN

[Noam Samuel, 2014/05/20 21:19:53]

Done.

+};
+
+NetworkListObserverNonChromeos::NetworkListObserverNonChromeos(
+    const WifiManager::SSIDListCallback& callback,
+    base::WeakPtr<WifiManagerNonChromeos> wifi_manager)
+    : callback_(callback), started_(false), wifi_manager_(wifi_manager) {
+}
+
+NetworkListObserverNonChromeos::~NetworkListObserverNonChromeos() {
+  if (started_ && wifi_manager_) {
+    wifi_manager_->RemoveObserver(this);
+  }

[stevenjb, 2014/05/15 18:16:26]

optional nit: Usually in chrome we don't use {} around single line if
statements.

[Noam Samuel, 2014/05/20 21:19:53]

Done.

+}
+
+void NetworkListObserverNonChromeos::Start() {
+  DCHECK(!started_);
+
+  if (wifi_manager_) {
+    wifi_manager_->AddObserver(this);
+  }

[stevenjb, 2014/05/15 18:16:26]

ditto

[Noam Samuel, 2014/05/20 21:19:53]

Done.

+}
+
+void NetworkListObserverNonChromeos::OnNetworkListChanged(
+    const std::vector<NetworkProperties>& ssid_list) {
+  callback_.Run(ssid_list);
+}
+
+WifiServiceWrapper::WifiServiceWrapper(
+    const base::Callback<void(const base::Closure&)>& post_callback,
+    const WifiManager::SSIDListCallback& network_list_update)
+    : post_callback_(post_callback),
+      network_list_update_(network_list_update),
+      weak_factory_(this) {
+}
+
+WifiServiceWrapper::~WifiServiceWrapper() {
+}
+
+void WifiServiceWrapper::Start() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));
+  wifi_service_.reset(WiFiService::Create());
+
+  wifi_service_->Initialize(base::MessageLoopProxy::current());
+
+  wifi_service_->SetEventObservers(
+      base::MessageLoopProxy::current(),
+      base::Bind(&WifiServiceWrapper::OnNetworksChangedEvent,
+                 base::Unretained(this)),
+      base::Bind(&WifiServiceWrapper::OnNetworkListChangedEvent,
+                 base::Unretained(this)));
+}
+
+void WifiServiceWrapper::GetSSIDList(
+    const WifiManager::SSIDListCallback& callback) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));
+
+  std::vector<NetworkProperties> network_property_list;
+
+  GetSSIDListInternal(&network_property_list);
+
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI,
+      FROM_HERE,
+      base::Bind(post_callback_, base::Bind(callback, network_property_list)));
+}
+
+void WifiServiceWrapper::ConnectToNetwork(

[tbarzic, 2014/05/15 20:50:07]

how is this going to be used as opposed to ConnectToNetworkByID?

[Noam Samuel, 2014/05/20 21:19:53]

This is for connecting to networks that are not yet necessarily configured, it
configures the network and then connects to it.

+    const std::string& ssid,
+    const std::string& password,
+    const WifiManager::SuccessCallback& callback) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));
+  scoped_ptr<base::DictionaryValue> properties = MakeProperties(ssid, password);
+  std::string network_guid;
+  bool error = false;
+  std::string error_string;

[stevenjb, 2014/05/15 18:16:26]

nit: declare these closer to where they are used

[Noam Samuel, 2014/05/20 21:19:53]

Done.

+
+  std::string internal_id;
+
+  std::vector<NetworkProperties> network_property_list;
+  GetSSIDListInternal(&network_property_list);
+
+  for (std::vector<NetworkProperties>::iterator i =
+           network_property_list.begin();
+       i != network_property_list.end();
+       i++) {
+    if (i->ssid == ssid) {
+      internal_id = i->internal_id;
+      break;
+    }
+  }
+
+  if (!internal_id.empty()) {
+    network_guid = internal_id;
+    wifi_service_->SetProperties(

[tbarzic, 2014/05/15 20:50:07]

SetProperties for setting passphrase won't work on Windows.
Use CreateNetwork instead (which could probably renamed to CreateConfiguration)
Note that it's ok for this to fail in case the network is already configured.

[Noam Samuel, 2014/05/20 21:19:53]

Done.

+        network_guid, properties.Pass(), &error_string);
+
+    if (!error_string.empty()) {
+      LOG(ERROR) << "Could not set properties on network: " << error_string;
+      error = true;
+    }
+  } else {
+    wifi_service_->CreateNetwork(
+        false, properties.Pass(), &network_guid, &error_string);
+
+    if (!error_string.empty()) {
+      LOG(ERROR) << "Could not create network: " << error_string;
+      error = true;
+    }
+  }
+
+  if (!error) {
+    ConnectToNetworkByID(network_guid, callback);
+  } else {
+    content::BrowserThread::PostTask(
+        content::BrowserThread::UI,
+        FROM_HERE,
+        base::Bind(post_callback_, base::Bind(callback, !error)));
+  }
+}
+
+void WifiServiceWrapper::OnNetworkListChangedEvent(
+    const std::vector<std::string>& network_guids) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));
+  std::vector<NetworkProperties> ssid_list;
+  GetSSIDListInternal(&ssid_list);
+  content::BrowserThread::PostTask(content::BrowserThread::UI,
+                                   FROM_HERE,
+                                   base::Bind(network_list_update_, ssid_list));
+}
+
+void WifiServiceWrapper::OnNetworksChangedEvent(
+    const std::vector<std::string>& network_guids) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));
+  if (!connecting_network_guid_.empty() &&
+      IsConnected(connecting_network_guid_)) {

[stevenjb, 2014/05/15 18:16:26]

reverse logic and early exit

[Noam Samuel, 2014/05/20 21:19:53]

Done.

+    connecting_network_guid_.clear();
+    connect_failure_callback_.Cancel();
+
+    content::BrowserThread::PostTask(
+        content::BrowserThread::UI,
+        FROM_HERE,
+        base::Bind(post_callback_,
+                   base::Bind(connect_success_callback_, true)));
+
+    connect_success_callback_.Reset();
+  }
+}
+
+base::WeakPtr<WifiServiceWrapper> WifiServiceWrapper::AsWeakPtr() {
+  return weak_factory_.GetWeakPtr();
+}
+
+void WifiServiceWrapper::RequestScan(const base::Closure& callback) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));
+  wifi_service_->RequestNetworkScan();
+
+  if (!callback.is_null()) {
+    content::BrowserThread::PostTask(content::BrowserThread::UI,
+                                     FROM_HERE,
+                                     base::Bind(post_callback_, callback));
+  }
+}
+
+void WifiServiceWrapper::ConnectToNetworkByID(
+    const std::string& network_guid,
+    const WifiManager::SuccessCallback& callback) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));
+  std::string error_string;
+  bool error = false;
+  bool connected = false;

[stevenjb, 2014/05/15 18:16:26]

declare on line 287 when first assigned/used

[Noam Samuel, 2014/05/20 21:19:53]

Done.

+
+  std::string connected_network_id = GetConnectedGUID();
+  wifi_service_->StartConnect(network_guid, &error_string);
+
+  if (!error_string.empty()) {
+    LOG(ERROR) << "Could not connect to network by ID: " << error_string;
+    error = true;
+    wifi_service_->StartConnect(connected_network_id, &error_string);
+  }
+
+  connected = IsConnected(network_guid);
+
+  if (error || connected) {
+    content::BrowserThread::PostTask(
+        content::BrowserThread::UI,
+        FROM_HERE,
+        base::Bind(post_callback_, base::Bind(callback, !error && connected)));

[stevenjb, 2014/05/15 18:16:26]

return;

[Noam Samuel, 2014/05/20 21:19:53]

Done.

+  } else {

[stevenjb, 2014/05/15 18:16:26]

no else

[Noam Samuel, 2014/05/20 21:19:53]

Done.

+    connect_success_callback_ = callback;
+    connecting_network_guid_ = network_guid;
+    connected_network_guid_ = connected_network_id;
+
+    connect_failure_callback_.Reset(
+        base::Bind(&WifiServiceWrapper::OnConnectToNetworkTimeout,
+                   base::Unretained(this)));
+
+    base::MessageLoop::current()->PostDelayedTask(
+        FROM_HERE,
+        connect_failure_callback_.callback(),
+        base::TimeDelta::FromSeconds(kConnectionTimeoutSeconds));
+  }
+}
+
+void WifiServiceWrapper::OnConnectToNetworkTimeout() {
+  bool connected = IsConnected(connecting_network_guid_);
+  std::string error_string;
+
+  if (!connected) {
+    wifi_service_->StartConnect(connected_network_guid_, &error_string);
+  }

[stevenjb, 2014/05/15 18:16:26]

optional: no {}

[Noam Samuel, 2014/05/20 21:19:53]

Done.

+
+  connecting_network_guid_.clear();
+
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI,
+      FROM_HERE,
+      base::Bind(post_callback_,
+                 base::Bind(connect_success_callback_, connected)));
+
+  connect_success_callback_.Reset();
+}
+
+void WifiServiceWrapper::GetNetworkCredentials(
+    const std::string& network_guid,
+    const WifiManager::CredentialsCallback& callback) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));
+  std::string error_string;
+  bool error = false;
+  std::string ssid;
+  std::string key;

[stevenjb, 2014/05/15 18:16:26]

declare where used

[Noam Samuel, 2014/05/20 21:19:53]

Done.

+
+  base::DictionaryValue properties;
+
+  wifi_service_->GetProperties(network_guid, &properties, &error_string);
+
+  if (!error_string.empty()) {
+    LOG(ERROR) << "Could not get network properties: " << error_string;
+    error = true;
+  }
+
+  if (!properties.GetString(onc::network_config::kName, &ssid)) {
+    LOG(ERROR) << "Could not get network SSID";
+    error = true;
+  }
+
+  if (!error) {
+    wifi_service_->GetKeyFromSystem(network_guid, &key, &error_string);

[tbarzic, 2014/05/15 20:50:07]

I'm not sure this will work from browser process.
afaik this requires (utility) process with elevated privilege.

[Noam Samuel, 2014/05/16 17:56:02]

This does work on OSX but not on Windows. There's a larger discussion of what
needs to be done on Windows (in terms of IPC security and reuse), so my instinct
is to put a NOTIMPLEMENTED() for Windows in this CL and write the Windows
implementation in a different CL.

Does this make sense?

+
+    if (!error_string.empty()) {
+      LOG(ERROR) << "Could not get key from system: " << error_string;
+      error = true;
+    }
+  }
+
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI,
+      FROM_HERE,
+      base::Bind(post_callback_, base::Bind(callback, !error, ssid, key)));
+}
+
+void WifiServiceWrapper::GetSSIDListInternal(
+    std::vector<NetworkProperties>* ssid_list) {
+  base::ListValue visible_networks;
+
+  wifi_service_->GetVisibleNetworks(onc::network_type::kWiFi,
+                                    &visible_networks);
+
+  for (size_t i = 0; i < visible_networks.GetSize(); i++) {
+    const base::DictionaryValue* network_value = NULL;
+    NetworkProperties network_properties;
+    std::string connection_status;
+
+    if (!visible_networks.GetDictionary(i, &network_value) ||
+        !network_value->GetString(onc::network_config::kName,
+                                  &network_properties.ssid) ||
+        !network_value->GetString(onc::network_config::kGUID,
+                                  &network_properties.internal_id) ||
+        !network_value->GetString(onc::network_config::kConnectionState,
+                                  &connection_status)) {
+      NOTREACHED();
+      continue;
+    }
+
+    network_properties.connected =
+        (connection_status == onc::connection_state::kConnected);
+
+    ssid_list->push_back(network_properties);
+  }
+}
+
+std::string WifiServiceWrapper::GetConnectedGUID() {
+  std::vector<NetworkProperties> ssid_list;
+  GetSSIDListInternal(&ssid_list);
+
+  for (std::vector<NetworkProperties>::const_iterator i = ssid_list.begin();
+       i != ssid_list.end();
+       i++) {

[stevenjb, 2014/05/15 18:16:26]

++i (nit: for iterators, 'iter' or 'it' is more commonly used in Chrome)

[Noam Samuel, 2014/05/20 21:19:53]

Done.

+    if (i->connected) {
+      return i->internal_id;
+    }
+  }
+
+  return "";
+}
+
+bool WifiServiceWrapper::IsConnected(const std::string& network_guid) {
+  std::vector<NetworkProperties> ssid_list;

[tbarzic, 2014/05/15 20:50:07]

how about:
return !network_guid.empty() && network_guid == GetConnectedGUID();

[Noam Samuel, 2014/05/16 17:56:02]

There's a slight neuance that I should maybe comment on: At least on OSX, at
certain times during the connection phase, both the old and new networks may at
some point seem connected, so writing the code like this ensures that we
correctly detect the network as connected.

+  GetSSIDListInternal(&ssid_list);
+
+  for (std::vector<NetworkProperties>::const_iterator i = ssid_list.begin();
+       i != ssid_list.end();
+       i++) {

[stevenjb, 2014/05/15 18:16:26]

++i

[Noam Samuel, 2014/05/20 21:19:53]

Done.

+    if (i->connected && i->internal_id == network_guid) {
+      return true;
+    }
+  }
+
+  return false;
+}
+
+scoped_ptr<WifiManager> WifiManager::Create() {
+  return scoped_ptr<WifiManager>(new WifiManagerNonChromeos());
+}
+
+WifiManagerNonChromeos::WifiManagerNonChromeos()
+    : wifi_wrapper_(NULL), weak_factory_(this) {
+}
+
+WifiManagerNonChromeos::~WifiManagerNonChromeos() {
+  if (wifi_wrapper_) {
+    content::BrowserThread::DeleteSoon(
+        content::BrowserThread::FILE, FROM_HERE, wifi_wrapper_);
+  }
+}
+
+void WifiManagerNonChromeos::Start() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  task_runner_ = content::BrowserThread::GetMessageLoopProxyForThread(
+      content::BrowserThread::FILE);
+
+  wifi_wrapper_ = new WifiServiceWrapper(

[stevenjb, 2014/05/15 18:16:26]

It is confusing that we allocate wifi_wrapper_ on the UI thread and delete it on
the FILE thread. Even though this might be safe here, it's much harder to verify
that. If it is necessary (or much easier) to do it this way, please comment why
and why it is safe.

At a glance, it seems like wifi_wrapper_ should have a WeakPtr and use
DeleteSoon on wifi_service_.

[Noam Samuel, 2014/05/20 21:19:53]

Added comment.

+      base::Bind(&WifiManagerNonChromeos::PostClosure,
+                 weak_factory_.GetWeakPtr()),
+      base::Bind(&WifiManagerNonChromeos::OnNetworkListChanged,
+                 weak_factory_.GetWeakPtr()));
+
+  task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&WifiServiceWrapper::Start, wifi_wrapper_->AsWeakPtr()));
+}
+
+void WifiManagerNonChromeos::GetSSIDList(const SSIDListCallback& callback) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  task_runner_->PostTask(FROM_HERE,
+                         base::Bind(&WifiServiceWrapper::GetSSIDList,
+                                    wifi_wrapper_->AsWeakPtr(),
+                                    callback));
+}
+
+void WifiManagerNonChromeos::RequestScan(const base::Closure& callback) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  task_runner_->PostTask(FROM_HERE,
+                         base::Bind(&WifiServiceWrapper::RequestScan,
+                                    wifi_wrapper_->AsWeakPtr(),
+                                    callback));
+}
+
+void WifiManagerNonChromeos::OnNetworkListChanged(
+    const std::vector<NetworkProperties>& ssid_list) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  FOR_EACH_OBSERVER(NetworkListObserverNonChromeos,
+                    network_list_observers_,
+                    OnNetworkListChanged(ssid_list));
+}
+
+void WifiManagerNonChromeos::PostClosure(const base::Closure& callback) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  callback.Run();
+}
+
+void WifiManagerNonChromeos::ConnectToNetwork(
+    const std::string& ssid,
+    const WifiCredentials& credentials,
+    const SuccessCallback& callback) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  task_runner_->PostTask(FROM_HERE,
+                         base::Bind(&WifiServiceWrapper::ConnectToNetwork,
+                                    wifi_wrapper_->AsWeakPtr(),
+                                    ssid,
+                                    credentials.psk,
+                                    callback));
+}
+
+void WifiManagerNonChromeos::ConnectToNetworkByID(
+    const std::string& internal_id,
+    const SuccessCallback& callback) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  task_runner_->PostTask(FROM_HERE,
+                         base::Bind(&WifiServiceWrapper::ConnectToNetworkByID,
+                                    wifi_wrapper_->AsWeakPtr(),
+                                    internal_id,
+                                    callback));
+}
+
+void WifiManagerNonChromeos::GetNetworkCredentials(
+    const std::string& internal_id,
+    const CredentialsCallback& callback) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  task_runner_->PostTask(FROM_HERE,
+                         base::Bind(&WifiServiceWrapper::GetNetworkCredentials,
+                                    wifi_wrapper_->AsWeakPtr(),
+                                    internal_id,
+                                    callback));
+}
+
+scoped_ptr<NetworkListObserver>
+WifiManagerNonChromeos::CreateNetworkListObserver(
+    const SSIDListCallback& callback) {
+  return scoped_ptr<NetworkListObserver>(
+      new NetworkListObserverNonChromeos(callback, weak_factory_.GetWeakPtr()));
+}
+
+void WifiManagerNonChromeos::AddObserver(
+    NetworkListObserverNonChromeos* observer) {
+  network_list_observers_.AddObserver(observer);
+}
+
+void WifiManagerNonChromeos::RemoveObserver(
+    NetworkListObserverNonChromeos* observer) {
+  network_list_observers_.RemoveObserver(observer);
+}
+
+}  // namespace wifi
+
+}  // namespace local_discovery