[CrOS PhoneHub] Implement receiving payloads and tweak sending payloads

Implement OnPayloadReceived() by:
(1) Verifying that the received payload is represented in bytes rather
    than a file (the phone will never send a file).
(2) Converting the received bytes into a string and passing it to the
    NearbyMessageReceiver interface.

Tweak sending payloads by generating a random payload ID for each
message sent rather than starting a count from 0 for each connection.

Bug: 1149576, 1106937
Change-Id: I2310c17df2cd854648c141cda0066f83089a54d2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2543682
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: James Vecore <vecore@google.com>
Cr-Commit-Position: refs/heads/master@{#828342}

chrome/browser/chromeos/secure_channel/nearby_connection_broker_impl.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/chromeos/secure_channel/nearby_connection_broker_impl.h"
 
 #include "base/memory/ptr_util.h"
+#include "base/rand_util.h"
 #include "chrome/browser/chromeos/secure_channel/nearby_endpoint_finder.h"
 #include "chromeos/components/multidevice/logging/logging.h"
 
@@ -164,11 +165,22 @@ void NearbyConnectionBrokerImpl::SendMessage(const std::string& message,
 
   std::vector<uint8_t> message_as_bytes(message.begin(), message.end());
 
+  // Randomly generate a new payload ID for each message sent. Each payload is
+  // expected to have its own ID, so we randomly generate one each time instead
+  // of starting from 0 for each NearbyConnectionBrokerImpl instance. Note that
+  // payloads are only shared between two devices, so the chance of a collision
+  // in a 64-bit value is negligible.
+  uint64_t unsigned_payload_id = base::RandUint64();
+
+  // Interpret |unsigned_payload_id|'s bytes as a signed value for use in the
+  // SendPayload() API.
+  const int64_t* payload_id_ptr =
+      reinterpret_cast<const int64_t*>(&unsigned_payload_id);
+
   nearby_connections_->SendPayload(
       mojom::kServiceId, std::vector<std::string>{remote_endpoint_id_},
-      Payload::New(
-          next_sent_payload_id_++,
-          PayloadContent::NewBytes(BytesPayload::New(message_as_bytes))),
+      Payload::New(*payload_id_ptr, PayloadContent::NewBytes(
+                                        BytesPayload::New(message_as_bytes))),
       base::BindOnce(&NearbyConnectionBrokerImpl::OnSendPayloadResult,
                      weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
 }
@@ -253,19 +265,19 @@ void NearbyConnectionBrokerImpl::OnPayloadReceived(
     return;
   }
 
-  // TODO(khorimoto): Handle received payloads.
-}
-
-void NearbyConnectionBrokerImpl::OnPayloadTransferUpdate(
-    const std::string& endpoint_id,
-    PayloadTransferUpdatePtr update) {
-  if (remote_endpoint_id_ != endpoint_id) {
-    PA_LOG(WARNING) << "OnPayloadTransferUpdate(): unexpected endpoint ID "
-                    << endpoint_id;
+  if (!payload->content->is_bytes()) {
+    PA_LOG(WARNING) << "OnPayloadReceived(): Received unexpected payload type "
+                    << "(was expecting bytes type). Disconnecting.";
+    TransitionToDisconnected();
     return;
   }
 
-  // TODO(khorimoto): Handle received payload updates.
+  PA_LOG(VERBOSE) << "OnPayloadReceived(): Received message with payload ID "
+                  << payload->id;
+  const std::vector<uint8_t>& message_as_bytes =
+      payload->content->get_bytes()->bytes;
+  NotifyMessageReceived(
+      std::string(message_as_bytes.begin(), message_as_bytes.end()));
 }
 
 std::ostream& operator<<(std::ostream& stream,

chrome/browser/chromeos/secure_channel/nearby_connection_broker_impl.h

@@ -29,8 +29,7 @@ class NearbyEndpointFinder;
 // Deleting an instance of this class tears down any active connection and
 // performs cleanup if necessary.
 //
-// TODO(khorimoto): Add the ability to upgrade bandwidth to WebRTC and to
-// receive payloads.
+// TODO(khorimoto): Add the ability to upgrade bandwidth to WebRTC.
 class NearbyConnectionBrokerImpl
     : public NearbyConnectionBroker,
       public location::nearby::connections::mojom::ConnectionLifecycleListener,
@@ -132,10 +131,12 @@ class NearbyConnectionBrokerImpl
   void OnPayloadReceived(
       const std::string& endpoint_id,
       location::nearby::connections::mojom::PayloadPtr payload) override;
+  // Note: Intentionally left empty; SecureChannel messages are always sent as
+  // bytes and do not require transfer updates.
   void OnPayloadTransferUpdate(
       const std::string& endpoint_id,
       location::nearby::connections::mojom::PayloadTransferUpdatePtr update)
-      override;
+      override {}
 
   NearbyEndpointFinder* endpoint_finder_;
   mojo::SharedRemote<location::nearby::connections::mojom::NearbyConnections>
@@ -148,7 +149,6 @@ class NearbyConnectionBrokerImpl
       payload_listener_receiver_{this};
 
   ConnectionStatus connection_status_ = ConnectionStatus::kUninitialized;
-  int64_t next_sent_payload_id_ = 0L;
 
   // Set once an endpoint is discovered.
   std::string remote_endpoint_id_;

chrome/browser/chromeos/secure_channel/nearby_connection_broker_impl_unittest.cc

@@ -7,6 +7,8 @@
 #include <memory>
 #include <vector>
 
+#include "base/containers/span.h"
+#include "base/files/file_util.h"
 #include "base/run_loop.h"
 #include "base/test/bind.h"
 #include "base/test/task_environment.h"
@@ -22,11 +24,15 @@ namespace chromeos {
 namespace secure_channel {
 namespace {
 
+using location::nearby::connections::mojom::BytesPayload;
 using location::nearby::connections::mojom::ConnectionInfo;
 using location::nearby::connections::mojom::ConnectionLifecycleListener;
 using location::nearby::connections::mojom::ConnectionOptionsPtr;
 using location::nearby::connections::mojom::DiscoveredEndpointInfo;
 using location::nearby::connections::mojom::EndpointDiscoveryListener;
+using location::nearby::connections::mojom::FilePayload;
+using location::nearby::connections::mojom::Payload;
+using location::nearby::connections::mojom::PayloadContent;
 using location::nearby::connections::mojom::PayloadListener;
 using location::nearby::connections::mojom::PayloadPtr;
 using location::nearby::connections::mojom::Status;
@@ -35,6 +41,7 @@ using testing::_;
 using testing::Invoke;
 
 const char kEndpointId[] = "endpointId";
+const int64_t kInvalidPayloadTypeId = 1234;
 
 const std::vector<uint8_t>& GetBluetoothAddress() {
   static const std::vector<uint8_t> address{0, 1, 2, 3, 4, 5};
@@ -121,7 +128,7 @@ class NearbyConnectionBrokerImplTest : public testing::Test,
                 mojo::PendingRemote<PayloadListener> listener,
                 NearbyConnectionsMojom::AcceptConnectionCallback callback) {
               accept_connection_callback_ = std::move(callback);
-              payload_listener_listener_.Bind(std::move(listener));
+              payload_listener_.Bind(std::move(listener));
               run_loop.Quit();
             }));
 
@@ -207,14 +214,53 @@ class NearbyConnectionBrokerImplTest : public testing::Test,
     disconnect_run_loop.Run();
   }
 
-  // TODO(khormoto): Verify received messages once this functionality is
-  // implemented.
-  std::vector<std::string> received_messages_;
+  void ReceiveMessage(const std::string& message) {
+    static int64_t next_payload_id = 0;
+
+    base::RunLoop receive_run_loop;
+    on_message_received_closure_ = receive_run_loop.QuitClosure();
+
+    std::vector<uint8_t> message_as_bytes(message.begin(), message.end());
+    payload_listener_->OnPayloadReceived(
+        kEndpointId, Payload::New(next_payload_id++,
+                                  PayloadContent::NewBytes(
+                                      BytesPayload::New(message_as_bytes))));
+    receive_run_loop.Run();
+
+    EXPECT_EQ(received_messages_.back(), message);
+  }
+
+  void ReceiveInvalidPayloadType() {
+    base::RunLoop disconnect_run_loop;
+    on_disconnected_closure_ = disconnect_run_loop.QuitClosure();
+
+    // Create fake file to receive.
+    const std::vector<uint8_t> kFakeFileContent{0x01, 0x02, 0x03};
+    base::FilePath path;
+    base::CreateTemporaryFile(&path);
+    base::File output_file(path, base::File::Flags::FLAG_CREATE_ALWAYS |
+                                     base::File::Flags::FLAG_WRITE);
+    output_file.WriteAndCheck(
+        /*offset=*/0,
+        base::make_span(kFakeFileContent.begin(), kFakeFileContent.end()));
+    output_file.Flush();
+    output_file.Close();
+    base::File input_file(
+        path, base::File::Flags::FLAG_OPEN | base::File::Flags::FLAG_READ);
+
+    payload_listener_->OnPayloadReceived(
+        kEndpointId,
+        Payload::New(
+            /*id=*/kInvalidPayloadTypeId,
+            PayloadContent::NewFile(FilePayload::New(std::move(input_file)))));
+    disconnect_run_loop.Run();
+  }
 
  private:
   // mojom::NearbyMessageReceiver:
   void OnMessageReceived(const std::string& message) override {
     received_messages_.push_back(message);
+    std::move(on_message_received_closure_).Run();
   }
 
   void OnConnected() { std::move(on_connected_closure_).Run(); }
@@ -232,20 +278,29 @@ class NearbyConnectionBrokerImplTest : public testing::Test,
 
   base::OnceClosure on_connected_closure_;
   base::OnceClosure on_disconnected_closure_;
+  base::OnceClosure on_message_received_closure_;
 
   NearbyConnectionsMojom::RequestConnectionCallback
       request_connection_callback_;
   NearbyConnectionsMojom::AcceptConnectionCallback accept_connection_callback_;
 
   mojo::Remote<ConnectionLifecycleListener> connection_lifecycle_listener_;
-  mojo::Remote<PayloadListener> payload_listener_listener_;
+  mojo::Remote<PayloadListener> payload_listener_;
+
+  std::vector<std::string> received_messages_;
 };
 
-// TODO(khorimoto): Add test for receiving incoming messages.
 TEST_F(NearbyConnectionBrokerImplTest, SendAndReceive) {
   SetUpFullConnection();
   SendMessage("test1", /*success=*/true);
   SendMessage("test2", /*success=*/true);
+  ReceiveMessage("test3");
+  ReceiveMessage("test4");
+}
+
+TEST_F(NearbyConnectionBrokerImplTest, ReceiveInvalidPayloadType) {
+  SetUpFullConnection();
+  ReceiveInvalidPayloadType();
 }
 
 TEST_F(NearbyConnectionBrokerImplTest, FailToSend) {