[CrOS Tether]: Create BleConnectionManager, which manages secure connections between the current de…

chromeos/components/tether/ble_connection_manager.cc

+// Copyright 2017 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/components/tether/ble_connection_manager.h"
+
+#include "chromeos/components/tether/ble_constants.h"
+#include "components/cryptauth/ble/bluetooth_low_energy_weave_client_connection.h"
+#include "components/proximity_auth/logging/logging.h"
+#include "device/bluetooth/bluetooth_uuid.h"
+
+namespace chromeos {
+
+namespace tether {
+
+namespace {
+const char kTetherFeature[] = "magic_tether";
+const int64_t kAdvertisingTimeoutInSeconds = 12;

[Ryan Hansberry, 2017/02/14 18:27:28]

Where'd this number come from?

[Kyle Horimoto, 2017/02/14 22:54:45]

It was derived empirically during Android work.

+const int64_t kShortErrorTimeoutInSeconds = 1;

[Ryan Hansberry, 2017/02/14 18:27:28]

Assuming this is only used by the test, this can just be 0.

[Kyle Horimoto, 2017/02/14 22:54:45]

This is used under non-test circumstances when there is an error advertising or
creating a scan filter.

+}  // namespace
+
+// static
+std::string BleConnectionManager::ConnectionReasonToString(
+    const ConnectionReason& reason) {
+  switch (reason) {
+    case ConnectionReason::TETHER_AVAILABILITY_REQUEST:
+      return "[TetherAvailabilityRequest]";
+    case ConnectionReason::CONNECT_TETHERING_REQUEST:
+      return "[ConnectTetheringRequest]";
+    case ConnectionReason::KEEP_ALIVE_REQUEST:
+      return "[KeepAliveRequest]";
+    case ConnectionReason::DISCONNECT_REQUEST:
+      return "[DisconnectRequest]";
+    default:
+      return "[invalid ConnectionReason]";
+  }
+}
+
+BleConnectionManager::ConnectionMetadata::ConnectionMetadata(
+    const cryptauth::RemoteDevice remote_device,
+    std::shared_ptr<base::Timer> timer)
+    : remote_device_(remote_device), timer_(timer), weak_ptr_factory_(this) {}
+
+BleConnectionManager::ConnectionMetadata::~ConnectionMetadata() {}
+
+void BleConnectionManager::ConnectionMetadata::RegisterConnectionReason(
+    const ConnectionReason& connection_reason) {
+  active_connection_reasons_.insert(connection_reason);
+}
+
+void BleConnectionManager::ConnectionMetadata::UnregisterConnectionReason(
+    const ConnectionReason& connection_reason) {
+  active_connection_reasons_.erase(connection_reason);
+}
+
+bool BleConnectionManager::ConnectionMetadata::HasReasonForConnection() const {
+  return !active_connection_reasons_.empty();
+}
+
+cryptauth::SecureChannel::Status
+BleConnectionManager::ConnectionMetadata::GetStatus() const {

[Ryan Hansberry, 2017/02/14 18:27:28]

Is this method really necessary if there are already  methods like
CanSendMessage()?

[Kyle Horimoto, 2017/02/14 22:54:45]

Yes. When a device is unregistered, we need to send out a "current status" =>
"disconnected" status update, so we need to know what the current status is.
You're right that the other methods could be replaced with just calling
GetStatus() and comparing that value to the expected one, but I like the more
convenient API more. If you have a strong objection, I could remove the
convenience methods.

[Ryan Hansberry, 2017/02/15 18:56:02]

IMO the convenience methods just make this class's API harder to understand.
Please remove them.

[Kyle Horimoto, 2017/02/15 22:24:14]

Done, except I kept the HasEstablishedConnection() function since it provides
different information other than just the status.

+  if (IsActivelyConnecting()) {
+    return cryptauth::SecureChannel::Status::CONNECTING;
+  } else if (!HasEstablishedConnection()) {
+    return cryptauth::SecureChannel::Status::DISCONNECTED;
+  }
+
+  return secure_channel_->status();
+}
+
+bool BleConnectionManager::ConnectionMetadata::IsActivelyConnecting() const {
+  return timer_->IsRunning();
+}
+
+bool BleConnectionManager::ConnectionMetadata::HasEstablishedConnection()
+    const {
+  return secure_channel_.get();
+}
+
+bool BleConnectionManager::ConnectionMetadata::CanSendMessage() const {
+  return secure_channel_ &&
+         secure_channel_->status() ==
+             cryptauth::SecureChannel::Status::AUTHENTICATED;
+}
+
+void BleConnectionManager::ConnectionMetadata::StartConnectionAttemptTimeout(
+    const ConnectionAttemptTimeoutHandler& handler,
+    bool use_short_error_timeout) {
+  DCHECK(!IsActivelyConnecting());
+  DCHECK(!secure_channel_);
+  DCHECK(timeout_handler_.is_null());
+
+  int64_t timeout_sec = use_short_error_timeout ? kShortErrorTimeoutInSeconds

[Ryan Hansberry, 2017/02/14 18:27:28]

It would be much better if timeout_sec were a property whose value was decided
at construction. 

[Kyle Horimoto, 2017/02/14 22:54:45]

This isn't a constant value, though. If scanning/advertising succeeds, we need
to wait the whole advertising timeout. If it could not be initialized, we do not
need to wait the full timeout and should proceed to the next device faster.

+                                                : kAdvertisingTimeoutInSeconds;
+
+  timeout_handler_ = handler;
+  timer_->Start(FROM_HERE, base::TimeDelta::FromSeconds(timeout_sec),
+                base::Bind(&ConnectionMetadata::OnConnectionAttemptTimeout,
+                           weak_ptr_factory_.GetWeakPtr()));
+}
+
+void BleConnectionManager::ConnectionMetadata::OnConnectionAttemptTimeout() {
+  // First, store |timeout_handler_| to a temporary variable and reset
+  // |timeout_handler_|.
+  ConnectionAttemptTimeoutHandler handler = timeout_handler_;
+  timeout_handler_.Reset();
+
+  // Run the handler after |timeout_handler_| has already been reset to ensure
+  // that if the callback references this instance, internal state is correct.
+  handler.Run(remote_device_);
+}
+
+void BleConnectionManager::ConnectionMetadata::SetSecureChannel(
+    std::unique_ptr<cryptauth::SecureChannel> secure_channel,
+    const SecureChannelStatusChangeHandler& status_change_handler,
+    const MessageReceivedHandler message_received_handler) {
+  DCHECK(!secure_channel_);
+  DCHECK(status_change_handler_.is_null());
+  DCHECK(message_received_handler_.is_null());
+
+  // The connection has succeeded, so cancel the timeout.
+  timer_->Stop();

[Ryan Hansberry, 2017/02/14 18:27:28]

Please name this more descriptively than just timer_, e.g.
connection_timeout_timer_, etc.

[Kyle Horimoto, 2017/02/14 22:54:45]

Done.

+
+  status_change_handler_ = status_change_handler;
+  message_received_handler_ = message_received_handler;
+
+  secure_channel_ = std::move(secure_channel);
+  secure_channel_->AddObserver(this);
+  secure_channel_->Initialize();
+}
+
+void BleConnectionManager::ConnectionMetadata::SendMessage(
+    const std::string& payload) {
+  DCHECK(CanSendMessage());
+  secure_channel_->SendMessage(std::string(kTetherFeature), payload);
+}
+
+void BleConnectionManager::ConnectionMetadata::OnSecureChannelStatusChanged(
+    cryptauth::SecureChannel* secure_channel,
+    const cryptauth::SecureChannel::Status& old_status,
+    const cryptauth::SecureChannel::Status& new_status) {
+  DCHECK(secure_channel_.get() == secure_channel);
+
+  if (new_status == cryptauth::SecureChannel::Status::CONNECTING) {
+    // BleConnectionManager already broadcasts "disconnected => connecting"
+    // status updates when a connection attempt begins, so there is no need to
+    // handle this case.
+    return;
+  }
+
+  if (new_status == cryptauth::SecureChannel::Status::DISCONNECTED) {
+    // First, store |status_change_handler_| to a temporary variable and reset
+    // |status_change_handler_|.
+    SecureChannelStatusChangeHandler handler = status_change_handler_;
+    secure_channel_->RemoveObserver(this);
+    secure_channel_.reset();
+    timeout_handler_.Reset();
+    status_change_handler_.Reset();
+    message_received_handler_.Reset();
+
+    // Run the handler after the other two handlers have already been reset to

[Ryan Hansberry, 2017/02/15 18:56:02]

s/handler/callback

+    // ensure that if the callback references this instance, internal state is

[Ryan Hansberry, 2017/02/14 18:27:28]

Why wouldn't the callback reference this instance?

[Kyle Horimoto, 2017/02/14 22:54:45]

Not sure what you mean. The callback being run here is
BleConnectionManager::OnSecureChannelStatusChanged(). When this happens,
UpdateConnectionAttempts() ends up being called. We want to make sure that if
UpdateConnectionAttempts() ends up accessing this ConnectionMetadata object, all
of the internal state is consistent. Thus, we reset all of the handlers, etc.
before this happens.

+    // correct.
+    handler.Run(remote_device_, old_status,
+                cryptauth::SecureChannel::Status::DISCONNECTED);
+    return;
+  }
+
+  status_change_handler_.Run(remote_device_, old_status, new_status);
+}
+
+void BleConnectionManager::ConnectionMetadata::OnMessageReceived(
+    cryptauth::SecureChannel* secure_channel,
+    const std::string& feature,
+    const std::string& payload) {
+  DCHECK(secure_channel_.get() == secure_channel);
+  if (feature != std::string(kTetherFeature)) {
+    // If the message received was not a tether feature, ignore it.
+    return;
+  }
+
+  message_received_handler_.Run(remote_device_, payload);
+}
+
+std::unique_ptr<base::Timer> BleConnectionManager::TimerFactory::CreateTimer() {
+  return base::MakeUnique<base::OneShotTimer>();
+}
+
+BleConnectionManager::BleConnectionManager(
+    std::unique_ptr<Delegate> delegate,
+    scoped_refptr<device::BluetoothAdapter> adapter,
+    const LocalDeviceDataProvider* local_device_data_provider,
+    const cryptauth::RemoteBeaconSeedFetcher* remote_beacon_seed_fetcher,
+    cryptauth::BluetoothThrottler* bluetooth_throttler)
+    : BleConnectionManager(
+          std::move(delegate),
+          adapter,
+          base::MakeUnique<BleScanner>(local_device_data_provider),

[Ryan Hansberry, 2017/02/14 18:27:28]

BleScanner should be changed to be injected with an adapter.

[Kyle Horimoto, 2017/02/14 22:54:45]

You're absolutely correct. That was a mistake on my part when I first wrote
BleScanner. I'll do it in a follow-up CL, though.

[Ryan Hansberry, 2017/02/15 18:56:02]

sgtm

+          base::MakeUnique<BleAdvertiser>(adapter,
+                                          local_device_data_provider,
+                                          remote_beacon_seed_fetcher),
+          base::MakeUnique<BleAdvertisementDeviceQueue>(),
+          base::WrapUnique<TimerFactory>(new TimerFactory()),
+          bluetooth_throttler) {}
+
+BleConnectionManager::BleConnectionManager(
+    std::unique_ptr<Delegate> delegate,
+    scoped_refptr<device::BluetoothAdapter> adapter,
+    std::unique_ptr<BleScanner> ble_scanner,
+    std::unique_ptr<BleAdvertiser> ble_advertiser,
+    std::unique_ptr<BleAdvertisementDeviceQueue> device_queue,
+    std::unique_ptr<TimerFactory> timer_factory,
+    cryptauth::BluetoothThrottler* bluetooth_throttler)
+    : delegate_(std::move(delegate)),
+      adapter_(adapter),
+      ble_scanner_(std::move(ble_scanner)),
+      ble_advertiser_(std::move(ble_advertiser)),
+      device_queue_(std::move(device_queue)),
+      timer_factory_(std::move(timer_factory)),
+      bluetooth_throttler_(bluetooth_throttler),
+      weak_ptr_factory_(this) {
+  ble_scanner_->AddObserver(this);
+}
+
+BleConnectionManager::~BleConnectionManager() {
+  ble_scanner_->RemoveObserver(this);
+}
+
+void BleConnectionManager::RegisterRemoteDevice(
+    const cryptauth::RemoteDevice& remote_device,
+    const ConnectionReason& connection_reason) {
+  PA_LOG(INFO) << "Registering device with ID "
+               << remote_device.GetTruncatedDeviceIdForLogs() << " for reason "
+               << ConnectionReasonToString(connection_reason);
+
+  std::shared_ptr<ConnectionMetadata> data =
+      GetConnectionMetadata(remote_device);
+  if (!data) {
+    data = AddMetadataForDevice(remote_device);
+  }
+
+  data->RegisterConnectionReason(connection_reason);
+  UpdateConnectionAttempts();
+}
+
+void BleConnectionManager::UnregisterRemoteDevice(
+    const cryptauth::RemoteDevice& remote_device,
+    const ConnectionReason& connection_reason) {
+  PA_LOG(INFO) << "Unregistering device with ID "
+               << remote_device.GetTruncatedDeviceIdForLogs() << " for reason "
+               << ConnectionReasonToString(connection_reason);
+
+  std::shared_ptr<ConnectionMetadata> data =
+      GetConnectionMetadata(remote_device);
+  if (!data) {
+    // If the device was not registered, there is nothing to do.
+    return;
+  }
+
+  data->UnregisterConnectionReason(connection_reason);
+  if (!data->HasReasonForConnection()) {
+    cryptauth::SecureChannel::Status status_before_disconnect =
+        data->GetStatus();
+    device_to_metadata_map_.erase(remote_device);
+    if (status_before_disconnect ==
+        cryptauth::SecureChannel::Status::CONNECTING) {
+      StopConnectionAttemptAndMoveToEndOfQueue(remote_device);
+    }
+    if (status_before_disconnect !=
+        cryptauth::SecureChannel::Status::DISCONNECTED) {
+      // Send a status update for the disconnection.
+      SendSecureChannelStatusChangeEvent(
+          remote_device, status_before_disconnect,
+          cryptauth::SecureChannel::Status::DISCONNECTED);
+    }
+  }
+
+  UpdateConnectionAttempts();
+}
+
+void BleConnectionManager::SendMessage(

[Ryan Hansberry, 2017/02/14 18:27:28]

This API doesn't make sense to me: if we registered a RemoteDevice for a
"ConnectionReason" already, then why do we need to send a message which will be
the same thing (e.g. a ConnectTetheringRequest)? Am I reading this wrong?

[Kyle Horimoto, 2017/02/14 22:54:45]

Sorry - I probably didn't make things clear enough.

Registration/unregistration for a specific reason does not actually send or
receive any messages. It just creates a connection. The reason that there is a
ConnectionReason (as opposed to just a register/unregister with only a
RemoteDevice) is that we want to make sure we do not close down a connection
that is active once one operation is finished. For example, imagine that a host
scan is in progress when a ConnectTetheringRequest is in the process of being
sent. If the host scan finishes first and UnregisterRemoteDevice() is called
accordingly, it would break the connection sending the ConnectTetheringRequest.
Instead, we keep a |set| of all current ConnectionReasons and only destroy the
connection when *all* of the registered ConnectionReasons are unregistered.

+    const cryptauth::RemoteDevice& remote_device,
+    const std::string& message) {
+  std::shared_ptr<ConnectionMetadata> data =
+      GetConnectionMetadata(remote_device);
+  if (!data || !data->CanSendMessage()) {
+    PA_LOG(ERROR) << "Attempted to send a message to device with ID "
+                  << remote_device.GetTruncatedDeviceIdForLogs() << ". "
+                  << "Message: \"" << message << "\"";
+    return;
+  }
+
+  PA_LOG(INFO) << "Sending message to device with ID "
+               << remote_device.GetTruncatedDeviceIdForLogs() << ". "
+               << "Message: \"" << message << "\"";
+  data->SendMessage(message);
+}
+
+void BleConnectionManager::AddObserver(Observer* observer) {
+  observer_list_.AddObserver(observer);
+}
+
+void BleConnectionManager::RemoveObserver(Observer* observer) {
+  observer_list_.RemoveObserver(observer);
+}
+
+void BleConnectionManager::OnReceivedAdvertisementFromDevice(
+    const std::string& device_address,
+    cryptauth::RemoteDevice remote_device) {
+  std::shared_ptr<ConnectionMetadata> data =
+      GetConnectionMetadata(remote_device);
+  if (!data) {
+    // If an advertisement  is received from a device that is not registered,
+    // ignore it.
+    PA_LOG(WARNING) << "Received an advertisement from a device which is not "
+                    << "registered. Bluetooth address: " << device_address
+                    << ", Remote Device ID: " << remote_device.GetDeviceId();
+    return;
+  }
+
+  PA_LOG(INFO) << "Received advertisement from device with ID "
+               << remote_device.GetTruncatedDeviceIdForLogs() << ". "
+               << "Starting authentication handshake to that device.";
+
+  // Create a connection to that device.
+  std::unique_ptr<cryptauth::Connection> connection =
+      cryptauth::weave::BluetoothLowEnergyWeaveClientConnection::Factory::
+          NewInstance(remote_device, device_address, adapter_,
+                      device::BluetoothUUID(std::string(kGattServerUuid)),
+                      bluetooth_throttler_);
+  std::unique_ptr<cryptauth::SecureChannel> secure_channel =
+      cryptauth::SecureChannel::Factory::NewInstance(
+          std::move(connection), delegate_->CreateSecureChannelDelegate());
+  data->SetSecureChannel(
+      std::move(secure_channel),
+      base::Bind(&BleConnectionManager::OnSecureChannelStatusChanged,
+                 weak_ptr_factory_.GetWeakPtr()),
+      base::Bind(&BleConnectionManager::SendMessageReceivedEvent,
+                 weak_ptr_factory_.GetWeakPtr()));
+
+  // Stop trying to connect to that device, since a connection already exists.
+  StopConnectionAttemptAndMoveToEndOfQueue(remote_device);
+  UpdateConnectionAttempts();
+}
+
+std::shared_ptr<BleConnectionManager::ConnectionMetadata>
+BleConnectionManager::GetConnectionMetadata(
+    const cryptauth::RemoteDevice& remote_device) {
+  const auto map_iter = device_to_metadata_map_.find(remote_device);
+  if (map_iter == device_to_metadata_map_.end()) {
+    return nullptr;
+  }
+
+  return map_iter->second;
+}
+
+std::shared_ptr<BleConnectionManager::ConnectionMetadata>
+BleConnectionManager::AddMetadataForDevice(
+    const cryptauth::RemoteDevice& remote_device) {
+  std::shared_ptr<ConnectionMetadata> existing_data =
+      GetConnectionMetadata(remote_device);
+  if (existing_data) {
+    return existing_data;
+  }
+
+  std::unique_ptr<base::Timer> timer = timer_factory_->CreateTimer();
+  device_to_metadata_map_.insert(
+      std::pair<cryptauth::RemoteDevice, std::shared_ptr<ConnectionMetadata>>(
+          remote_device,
+          std::shared_ptr<ConnectionMetadata>(
+              new ConnectionMetadata(remote_device, std::move(timer)))));
+  return device_to_metadata_map_.at(remote_device);
+}
+
+void BleConnectionManager::UpdateConnectionAttempts() {
+  UpdateAdvertisementQueue();
+
+  std::vector<cryptauth::RemoteDevice> should_advertise_to =
+      device_queue_->GetDevicesToWhichToAdvertise();
+  DCHECK(should_advertise_to.size() <=
+         static_cast<size_t>(kMaxConcurrentAdvertisements));
+
+  for (const auto& remote_device : should_advertise_to) {
+    std::shared_ptr<ConnectionMetadata> associated_data =
+        GetConnectionMetadata(remote_device);
+    if (!associated_data->IsActivelyConnecting()) {
+      // If there is no active attempt to connect to a device at the front of
+      // the queue, start a connection attempt.
+      StartConnectionAttempt(remote_device);
+    }
+  }
+}
+
+void BleConnectionManager::UpdateAdvertisementQueue() {
+  std::vector<cryptauth::RemoteDevice> devices_for_queue;
+  for (const auto& map_entry : device_to_metadata_map_) {
+    if (map_entry.second->HasEstablishedConnection()) {
+      // If there is already an active connection to the device, there is no
+      // need to advertise to the device to bootstrap a connection.
+      continue;
+    }
+
+    devices_for_queue.push_back(map_entry.first);
+  }
+
+  device_queue_->SetDevices(devices_for_queue);
+}
+
+void BleConnectionManager::StartConnectionAttempt(
+    const cryptauth::RemoteDevice& remote_device) {
+  std::shared_ptr<ConnectionMetadata> data =
+      GetConnectionMetadata(remote_device);
+  DCHECK(data);
+
+  PA_LOG(INFO) << "Starting connection attempt to device with ID "
+               << remote_device.GetTruncatedDeviceIdForLogs();
+
+  // Send a "disconnected => connecting" update to alert clients that a
+  // connection attempt for |remote_device| is underway.
+  SendSecureChannelStatusChangeEvent(
+      remote_device, cryptauth::SecureChannel::Status::DISCONNECTED,
+      cryptauth::SecureChannel::Status::CONNECTING);
+
+  bool success = ble_scanner_->RegisterScanFilterForDevice(remote_device) &&
+                 ble_advertiser_->StartAdvertisingToDevice(remote_device);
+
+  data->StartConnectionAttemptTimeout(
+      base::Bind(&BleConnectionManager::OnConnectionAttemptTimeout,
+                 weak_ptr_factory_.GetWeakPtr()),
+      !success);
+}
+
+void BleConnectionManager::StopConnectionAttemptAndMoveToEndOfQueue(
+    const cryptauth::RemoteDevice& remote_device) {
+  PA_LOG(INFO) << "Stopping connection attempt to device with ID "
+               << remote_device.GetTruncatedDeviceIdForLogs();
+
+  ble_scanner_->UnregisterScanFilterForDevice(remote_device);
+  ble_advertiser_->StopAdvertisingToDevice(remote_device);
+
+  device_queue_->MoveDeviceToEnd(remote_device.GetDeviceId());
+}
+
+void BleConnectionManager::OnConnectionAttemptTimeout(
+    const cryptauth::RemoteDevice& remote_device) {
+  PA_LOG(INFO) << "Connection attempt timed out for device with ID "
+               << remote_device.GetTruncatedDeviceIdForLogs();
+
+  StopConnectionAttemptAndMoveToEndOfQueue(remote_device);
+
+  // Send a "connecting => disconnected" update to alert clients that a
+  // connection attempt for |remote_device| has failed.
+  SendSecureChannelStatusChangeEvent(
+      remote_device, cryptauth::SecureChannel::Status::CONNECTING,
+      cryptauth::SecureChannel::Status::DISCONNECTED);
+
+  UpdateConnectionAttempts();
+}
+
+void BleConnectionManager::OnSecureChannelStatusChanged(
+    const cryptauth::RemoteDevice& remote_device,
+    const cryptauth::SecureChannel::Status& old_status,
+    const cryptauth::SecureChannel::Status& new_status) {
+  SendSecureChannelStatusChangeEvent(remote_device, old_status, new_status);
+  UpdateConnectionAttempts();
+}
+
+void BleConnectionManager::SendMessageReceivedEvent(
+    const cryptauth::RemoteDevice& remote_device,
+    const std::string& payload) {
+  PA_LOG(INFO) << "Broadcasting message received event: "
+               << "Device ID \"" << remote_device.GetTruncatedDeviceIdForLogs()
+               << "\" received message \"" << payload << "\".";
+  for (auto& observer : observer_list_) {
+    observer.OnMessageReceived(remote_device, payload);
+  }
+}
+
+void BleConnectionManager::SendSecureChannelStatusChangeEvent(
+    const cryptauth::RemoteDevice& remote_device,
+    const cryptauth::SecureChannel::Status& old_status,
+    const cryptauth::SecureChannel::Status& new_status) {
+  PA_LOG(INFO) << "Broadcasting status change event: "
+               << "Device ID \"" << remote_device.GetTruncatedDeviceIdForLogs()
+               << "\": " << cryptauth::SecureChannel::StatusToString(old_status)
+               << " => "
+               << cryptauth::SecureChannel::StatusToString(new_status);
+  for (auto& observer : observer_list_) {
+    observer.OnSecureChannelStatusChanged(remote_device, old_status,
+                                          new_status);
+  }
+}
+
+}  // namespace tether
+
+}  // namespace chromeos