[CrOS PhoneHub] Implement secure_channel::NearbyConnection

This class uses NearbyConnector to request a connection, then
send/receive messages when the connection has been completed.

Also removes some "const" qualifiers in WireMessage so that operator=()
works correctly.

Bug: 1106937
Change-Id: I8925a48a65e2706b694d585d551f790691a03071
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2512515
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: James Vecore <vecore@google.com>
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#825064}

chromeos/services/secure_channel/BUILD.gn

@@ -302,6 +302,7 @@ source_set("unit_tests") {
     "foreground_eid_generator_unittest.cc",
     "multiplexed_channel_impl_unittest.cc",
     "nearby_connection_manager_impl_unittest.cc",
+    "nearby_connection_unittest.cc",
     "nearby_initiator_operation_unittest.cc",
     "pending_ble_connection_request_base_unittest.cc",
     "pending_ble_initiator_connection_request_unittest.cc",

chromeos/services/secure_channel/nearby_connection.cc

@@ -6,11 +6,19 @@
 
 #include "base/check.h"
 #include "base/memory/ptr_util.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_split.h"
+#include "chromeos/components/multidevice/logging/logging.h"
+#include "chromeos/services/secure_channel/wire_message.h"
 
 namespace chromeos {
 
 namespace secure_channel {
 
+namespace {
+const char kBluetoothAddressSeparator[] = ":";
+}  // namespace
+
 // static
 NearbyConnection::Factory* NearbyConnection::Factory::factory_instance_ =
     nullptr;
@@ -38,15 +46,21 @@ NearbyConnection::NearbyConnection(multidevice::RemoteDeviceRef remote_device,
 }
 
 NearbyConnection::~NearbyConnection() {
-  // TODO(https://crbug.com/1106937): Clean up potentially-lingering connection.
+  Disconnect();
 }
 
 void NearbyConnection::Connect() {
-  // TODO(https://crbug.com/1106937): Implement.
+  SetStatus(Status::IN_PROGRESS);
+  nearby_connector_->Connect(GetRemoteDeviceBluetoothAddressAsVector(),
+                             message_receiver_.BindNewPipeAndPassRemote(),
+                             base::BindOnce(&NearbyConnection::OnConnectResult,
+                                            weak_ptr_factory_.GetWeakPtr()));
 }
 
 void NearbyConnection::Disconnect() {
-  // TODO(https://crbug.com/1106937): Implement.
+  message_sender_.reset();
+  message_receiver_.reset();
+  SetStatus(Status::DISCONNECTED);
 }
 
 std::string NearbyConnection::GetDeviceAddress() {
@@ -54,7 +68,81 @@ std::string NearbyConnection::GetDeviceAddress() {
 }
 
 void NearbyConnection::SendMessageImpl(std::unique_ptr<WireMessage> message) {
-  // TODO(https://crbug.com/1106937): Implement.
+  queued_messages_to_send_.emplace(std::move(message));
+  ProcessQueuedMessagesToSend();
+}
+
+void NearbyConnection::OnMessageReceived(const std::string& message) {
+  OnBytesReceived(message);
+}
+
+std::vector<uint8_t>
+NearbyConnection::GetRemoteDeviceBluetoothAddressAsVector() {
+  std::vector<std::string> hex_bytes_as_strings =
+      base::SplitString(GetDeviceAddress(), kBluetoothAddressSeparator,
+                        base::WhitespaceHandling::TRIM_WHITESPACE,
+                        base::SplitResult::SPLIT_WANT_ALL);
+
+  std::vector<uint8_t> bytes;
+  for (const std::string& hex_bytes_as_string : hex_bytes_as_strings) {
+    int byte_value;
+    base::HexStringToInt(hex_bytes_as_string, &byte_value);
+    bytes.push_back(static_cast<uint8_t>(byte_value));
+  }
+
+  return bytes;
+}
+
+void NearbyConnection::OnConnectResult(
+    mojo::PendingRemote<mojom::NearbyMessageSender> message_sender) {
+  // If a connection failed to be established, disconnect.
+  if (!message_sender) {
+    PA_LOG(WARNING) << "NearbyConnector returned invalid MessageSender; "
+                    << "stopping connection attempt.";
+    Disconnect();
+    return;
+  }
+
+  message_sender_.Bind(std::move(message_sender));
+
+  message_sender_.set_disconnect_handler(
+      base::BindOnce(&NearbyConnection::Disconnect, base::Unretained(this)));
+  message_receiver_.set_disconnect_handler(
+      base::BindOnce(&NearbyConnection::Disconnect, base::Unretained(this)));
+
+  SetStatus(Status::CONNECTED);
+}
+
+void NearbyConnection::OnSendMessageResult(bool success) {
+  OnDidSendMessage(*message_being_sent_, success);
+
+  if (success) {
+    message_being_sent_.reset();
+    ProcessQueuedMessagesToSend();
+    return;
+  }
+
+  // Failing to send a message is a fatal error; disconnect.
+  PA_LOG(WARNING) << "Sending message failed; disconnecting.";
+  Disconnect();
+}
+
+void NearbyConnection::ProcessQueuedMessagesToSend() {
+  // Message is already being sent.
+  if (message_being_sent_)
+    return;
+
+  // No pending messages to send.
+  if (queued_messages_to_send_.empty())
+    return;
+
+  message_being_sent_ = std::move(queued_messages_to_send_.front());
+  queued_messages_to_send_.pop();
+
+  message_sender_->SendMessage(
+      message_being_sent_->Serialize(),
+      base::BindOnce(&NearbyConnection::OnSendMessageResult,
+                     base::Unretained(this)));
 }
 
 }  // namespace secure_channel

chromeos/services/secure_channel/nearby_connection.h

@@ -5,17 +5,28 @@
 #ifndef CHROMEOS_SERVICES_SECURE_CHANNEL_NEARBY_CONNECTION_H_
 #define CHROMEOS_SERVICES_SECURE_CHANNEL_NEARBY_CONNECTION_H_
 
+#include "base/memory/weak_ptr.h"
 #include "chromeos/services/secure_channel/connection.h"
 #include "chromeos/services/secure_channel/public/mojom/nearby_connector.mojom.h"
+#include "mojo/public/cpp/bindings/receiver.h"
+#include "mojo/public/cpp/bindings/remote.h"
 
 namespace chromeos {
 
 namespace secure_channel {
 
 // Connection implementation which creates a connection to a remote device via
-// the Nearby Connections library.
-// TODO(https://crbug.com/1106937): Add real implementation.
-class NearbyConnection : public Connection {
+// mojom::NearbyConnector. Implements mojom::NearbyMessageReceiver to receive
+// messages from the NearbyConnector and uses a
+// mojo::Remote<mojom::NearbyMessageSender> to send messages to the
+// NearbyConnector.
+//
+// When requested to send a message, this class queues messages and only sends
+// one message after the previous one has been sent successfully. If sending a
+// message fails, this is considered a fatal error, and the connection is
+// disconnected.
+class NearbyConnection : public Connection,
+                         public mojom::NearbyMessageReceiver {
  public:
   class Factory {
    public:
@@ -46,7 +57,28 @@ class NearbyConnection : public Connection {
   std::string GetDeviceAddress() override;
   void SendMessageImpl(std::unique_ptr<WireMessage> message) override;
 
+  // mojom::NearbyMessageReceiver:
+  void OnMessageReceived(const std::string& message) override;
+
+  // Returns the the remote device's address as a byte array; note that
+  // GetDeviceAddress() returns a colon-separated hex string.
+  std::vector<uint8_t> GetRemoteDeviceBluetoothAddressAsVector();
+
+  void OnConnectResult(
+      mojo::PendingRemote<mojom::NearbyMessageSender> message_sender);
+  void OnSendMessageResult(bool success);
+  void ProcessQueuedMessagesToSend();
+
   mojom::NearbyConnector* nearby_connector_;
+  mojo::Receiver<mojom::NearbyMessageReceiver> message_receiver_{this};
+  mojo::Remote<mojom::NearbyMessageSender> message_sender_;
+
+  base::queue<std::unique_ptr<WireMessage>> queued_messages_to_send_;
+
+  // Null if no message is being sent.
+  std::unique_ptr<WireMessage> message_being_sent_;
+
+  base::WeakPtrFactory<NearbyConnection> weak_ptr_factory_{this};
 };
 
 }  // namespace secure_channel

chromeos/services/secure_channel/nearby_connection_unittest.cc

+// Copyright 2020 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/services/secure_channel/nearby_connection.h"
+
+#include "base/callback.h"
+#include "base/test/task_environment.h"
+#include "chromeos/components/multidevice/remote_device_test_util.h"
+#include "chromeos/services/secure_channel/connection_observer.h"
+#include "chromeos/services/secure_channel/public/cpp/client/fake_nearby_connector.h"
+#include "chromeos/services/secure_channel/wire_message.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace chromeos {
+namespace secure_channel {
+namespace {
+
+const char kTestBluetoothAddress[] = "01:23:45:67:89:AB";
+
+// Returns the same address as above except as a byte vector.
+const std::vector<uint8_t>& GetTestBluetoothAddressAsVector() {
+  static const std::vector<uint8_t> address{0x01, 0x23, 0x45, 0x67, 0x89, 0xab};
+  return address;
+}
+
+multidevice::RemoteDeviceRef CreateTestDevice() {
+  multidevice::RemoteDeviceRef device =
+      multidevice::CreateRemoteDeviceRefForTest();
+  multidevice::GetMutableRemoteDevice(device)->bluetooth_public_address =
+      kTestBluetoothAddress;
+  return device;
+}
+
+class FakeConnectionObserver : public ConnectionObserver {
+ public:
+  FakeConnectionObserver() = default;
+  ~FakeConnectionObserver() override = default;
+
+  Connection::Status last_status_update = Connection::Status::DISCONNECTED;
+  base::Optional<WireMessage> last_received_message;
+  bool last_send_complete_success = false;
+
+  base::OnceClosure on_status_change_closure;
+  base::OnceClosure on_message_received_closure;
+  base::OnceClosure on_send_complete_closure;
+
+ private:
+  void OnConnectionStatusChanged(Connection* connection,
+                                 Connection::Status old_status,
+                                 Connection::Status new_status) override {
+    last_status_update = new_status;
+
+    if (on_status_change_closure)
+      std::move(on_status_change_closure).Run();
+  }
+
+  void OnMessageReceived(const Connection& connection,
+                         const WireMessage& message) override {
+    last_received_message = message;
+    std::move(on_message_received_closure).Run();
+  }
+
+  void OnSendCompleted(const Connection& connection,
+                       const WireMessage& message,
+                       bool success) override {
+    last_send_complete_success = success;
+    std::move(on_send_complete_closure).Run();
+  }
+};
+
+}  // namespace
+
+class SecureChannelNearbyConnectionTest : public testing::Test {
+ protected:
+  SecureChannelNearbyConnectionTest() = default;
+  ~SecureChannelNearbyConnectionTest() override = default;
+
+  void SetUp() override {
+    connection_ = NearbyConnection::Factory::Create(test_device_,
+                                                    &fake_nearby_connector_);
+    connection_->AddObserver(&fake_observer_);
+
+    base::RunLoop start_connect_run_loop;
+    fake_nearby_connector_.on_connect_closure =
+        start_connect_run_loop.QuitClosure();
+    EXPECT_EQ(Connection::Status::DISCONNECTED, connection_->status());
+    connection_->Connect();
+    start_connect_run_loop.Run();
+    EXPECT_EQ(Connection::Status::IN_PROGRESS, connection_->status());
+    EXPECT_EQ(Connection::Status::IN_PROGRESS,
+              fake_observer_.last_status_update);
+  }
+
+  void TearDown() override { connection_->RemoveObserver(&fake_observer_); }
+
+  FakeNearbyConnector::FakeConnection* CompleteConnection() {
+    base::RunLoop connect_run_loop;
+    fake_observer_.on_status_change_closure = connect_run_loop.QuitClosure();
+    FakeNearbyConnector::FakeConnection* fake_connection =
+        fake_nearby_connector_.ConnectQueuedCallback();
+    connect_run_loop.Run();
+
+    EXPECT_EQ(GetTestBluetoothAddressAsVector(),
+              fake_connection->bluetooth_public_address());
+    EXPECT_EQ(Connection::Status::CONNECTED, fake_observer_.last_status_update);
+    return fake_connection;
+  }
+
+  Connection* connection() { return connection_.get(); }
+
+  FakeNearbyConnector fake_nearby_connector_;
+  FakeConnectionObserver fake_observer_;
+
+ private:
+  base::test::TaskEnvironment task_environment_;
+  multidevice::RemoteDeviceRef test_device_ = CreateTestDevice();
+
+  std::unique_ptr<Connection> connection_;
+};
+
+TEST_F(SecureChannelNearbyConnectionTest, ConnectAndTransferMessages) {
+  FakeNearbyConnector::FakeConnection* fake_connection = CompleteConnection();
+
+  // Send message.
+  WireMessage message_to_send("send_payload", "send_feature");
+  base::RunLoop send_run_loop;
+  fake_observer_.on_send_complete_closure = send_run_loop.QuitClosure();
+  connection()->SendMessage(std::make_unique<WireMessage>(message_to_send));
+  send_run_loop.Run();
+  EXPECT_EQ(message_to_send.Serialize(), fake_connection->sent_messages()[0]);
+
+  // Receive message.
+  WireMessage message_to_receive("receive_payload", "receive_feature");
+  base::RunLoop receive_run_loop;
+  fake_observer_.on_message_received_closure = receive_run_loop.QuitClosure();
+  fake_connection->ReceiveMessage(message_to_receive.Serialize());
+  receive_run_loop.Run();
+  EXPECT_EQ("receive_payload", fake_observer_.last_received_message->payload());
+  EXPECT_EQ("receive_feature", fake_observer_.last_received_message->feature());
+
+  // Disconnect.
+  base::RunLoop disconnect_run_loop;
+  fake_observer_.on_status_change_closure = disconnect_run_loop.QuitClosure();
+  connection()->Disconnect();
+  disconnect_run_loop.Run();
+  EXPECT_EQ(Connection::Status::DISCONNECTED,
+            fake_observer_.last_status_update);
+  EXPECT_EQ(Connection::Status::DISCONNECTED, connection()->status());
+}
+
+TEST_F(SecureChannelNearbyConnectionTest, FailToSendMessage) {
+  FakeNearbyConnector::FakeConnection* fake_connection = CompleteConnection();
+
+  // Simulate an error sending a message; NearbyConnection should treat this as
+  // a fatal error and disconnect.
+  WireMessage message_to_send("payload", "feature");
+  base::RunLoop send_run_loop;
+  fake_observer_.on_send_complete_closure = send_run_loop.QuitClosure();
+  fake_connection->set_should_send_succeed(false);
+  connection()->SendMessage(std::make_unique<WireMessage>(message_to_send));
+  send_run_loop.Run();
+  EXPECT_EQ(Connection::Status::DISCONNECTED,
+            fake_observer_.last_status_update);
+  EXPECT_EQ(Connection::Status::DISCONNECTED, connection()->status());
+}
+
+TEST_F(SecureChannelNearbyConnectionTest, FailToConnect) {
+  base::RunLoop status_change_run_loop;
+  fake_observer_.on_status_change_closure =
+      status_change_run_loop.QuitClosure();
+  fake_nearby_connector_.FailQueuedCallback();
+  status_change_run_loop.Run();
+  EXPECT_EQ(Connection::Status::DISCONNECTED,
+            fake_observer_.last_status_update);
+  EXPECT_EQ(Connection::Status::DISCONNECTED, connection()->status());
+}
+
+TEST_F(SecureChannelNearbyConnectionTest, DisconnectFromRemoteDevice) {
+  FakeNearbyConnector::FakeConnection* fake_connection = CompleteConnection();
+
+  // Simulate the remote device disconnecting and verify that NearbyConnection
+  // changes its status once the Mojo connection is dropped.
+  base::RunLoop disconnect_run_loop;
+  fake_observer_.on_status_change_closure = disconnect_run_loop.QuitClosure();
+  fake_connection->Disconnect();
+  disconnect_run_loop.Run();
+  EXPECT_EQ(Connection::Status::DISCONNECTED,
+            fake_observer_.last_status_update);
+  EXPECT_EQ(Connection::Status::DISCONNECTED, connection()->status());
+}
+
+}  // namespace secure_channel
+}  // namespace chromeos

chromeos/services/secure_channel/wire_message.cc

@@ -200,6 +200,8 @@ WireMessage::WireMessage(const WireMessage& other)
       body_(other.body_),
       sequence_number_(other.sequence_number_) {}
 
+WireMessage& WireMessage::operator=(const WireMessage& other) = default;
+
 }  // namespace secure_channel
 
 }  // namespace chromeos

chromeos/services/secure_channel/wire_message.h

@@ -27,6 +27,8 @@ class WireMessage {
 
   WireMessage(const WireMessage& other);
 
+  WireMessage& operator=(const WireMessage& other);
+
   // Creates a WireMessage containing |body| (a serialized JSON) as the message
   // body.
   explicit WireMessage(const std::string& body);
@@ -53,18 +55,18 @@ class WireMessage {
 
  private:
   // The message payload.
-  const std::string payload_;
+  std::string payload_;
 
   // The feature which sends or intends to receive this message (e.g.,
   // EasyUnlock).
-  const std::string feature_;
+  std::string feature_;
 
   // The message body. When this is set |payload_| and |feature_| are empty, and
   // vice-versa.
-  const std::string body_;
+  std::string body_;
 
   // Sequence number for this message; set to -1 if no sequence number if set.
-  const int sequence_number_ = -1;
+  int sequence_number_ = -1;
 };
 
 }  // namespace secure_channel