Remote virtual connections [3/3]: Connection messages are special-cased.

Connection namespace messages are weird: The source_id and
destination_id are NOT treated as "envelope routing information," like
for all other namespaces. Instead, they are considered part of the
payload data for CONNECT/CLOSE requests. Thus, they require special-case
handling in VirtualConnectionRouter.

Bug: b/162542369
Change-Id: I933ad63c01d9e6af9a58a67a2f05da3a76986c2c
Reviewed-on: https://chromium-review.googlesource.com/c/openscreen/+/2546885
Commit-Queue: Yuri Wiitala <miu@chromium.org>
Reviewed-by: Brandon Tolsch <btolsch@chromium.org>

6 files changed, 80 insertions, 6 deletions

diff --git a/cast/common/channel/connection_namespace_handler.cc b/cast/common/channel/connection_namespace_handler.cc
index e754450d..d3b2ea8b 100644
--- a/cast/common/channel/connection_namespace_handler.cc
+++ b/cast/common/channel/connection_namespace_handler.cc
@@ -87,9 +87,13 @@ ConnectionNamespaceHandler::ConnectionNamespaceHandler(
     : vc_router_(vc_router), vc_policy_(vc_policy) {
   OSP_DCHECK(vc_router_);
   OSP_DCHECK(vc_policy_);
+
+  vc_router_->set_connection_namespace_handler(this);
 }
 
-ConnectionNamespaceHandler::~ConnectionNamespaceHandler() = default;
+ConnectionNamespaceHandler::~ConnectionNamespaceHandler() {
+  vc_router_->set_connection_namespace_handler(nullptr);
+}
 
 void ConnectionNamespaceHandler::OpenRemoteConnection(
     VirtualConnection conn,
diff --git a/cast/common/channel/connection_namespace_handler.h b/cast/common/channel/connection_namespace_handler.h
index 4ae16509..d67440df 100644
--- a/cast/common/channel/connection_namespace_handler.h
+++ b/cast/common/channel/connection_namespace_handler.h
@@ -32,7 +32,7 @@ class VirtualConnectionRouter;
 // disallowing connections based on the VirtualConnectionPolicy, and
 // ConnectionNamespaceHandler will report new/closed connections to the local
 // VirtualConnectionRouter to enable/disable message routing.
-class ConnectionNamespaceHandler final : public CastMessageHandler {
+class ConnectionNamespaceHandler : public CastMessageHandler {
  public:
   class VirtualConnectionPolicy {
    public:
diff --git a/cast/common/channel/virtual_connection_router.cc b/cast/common/channel/virtual_connection_router.cc
index 98e6d19d..e604d4e0 100644
--- a/cast/common/channel/virtual_connection_router.cc
+++ b/cast/common/channel/virtual_connection_router.cc
@@ -7,6 +7,7 @@
 #include <utility>
 
 #include "cast/common/channel/cast_message_handler.h"
+#include "cast/common/channel/connection_namespace_handler.h"
 #include "cast/common/channel/message_util.h"
 #include "cast/common/channel/proto/cast_channel.pb.h"
 #include "util/osp_logging.h"
@@ -203,6 +204,21 @@ void VirtualConnectionRouter::OnMessage(CastSocket* socket,
       entry.second->OnMessage(this, socket, message);
     }
   } else {
+    // Connection namespace messages are weird: The message.source_id() and
+    // message.destination_id() are NOT treated as "envelope routing
+    // information," like for all other namespaces. Instead, they are considered
+    // part of the payload data for CONNECT/CLOSE requests. Thus, they require
+    // special-case handling here.
+    if (message.namespace_() == kConnectionNamespace) {
+      if (connection_handler_) {
+        connection_handler_->OnMessage(this, socket, std::move(message));
+      }
+      return;
+    }
+
+    // Drop all messages for virtual connections that do not yet exist.
+    // Exception: All transport namespace messages (e.g., device auth,
+    // heartbeats, etc.); because these are always assumed to have a route.
     if (!IsTransportNamespace(message.namespace_()) &&
         !GetConnectionData(VirtualConnection{local_id, message.source_id(),
                                              socket->socket_id()})) {
diff --git a/cast/common/channel/virtual_connection_router.h b/cast/common/channel/virtual_connection_router.h
index 5080e948..c714882f 100644
--- a/cast/common/channel/virtual_connection_router.h
+++ b/cast/common/channel/virtual_connection_router.h
@@ -19,6 +19,7 @@ namespace openscreen {
 namespace cast {
 
 class CastMessageHandler;
+class ConnectionNamespaceHandler;
 
 // Handles CastSockets by routing received messages to appropriate message
 // handlers based on the VirtualConnection's local ID and sending messages over
@@ -104,6 +105,13 @@ class VirtualConnectionRouter final : public CastSocket::Client {
   void OnMessage(CastSocket* socket,
                  ::cast::channel::CastMessage message) override;
 
+ protected:
+  friend class ConnectionNamespaceHandler;
+
+  void set_connection_namespace_handler(ConnectionNamespaceHandler* handler) {
+    connection_handler_ = handler;
+  }
+
  private:
   // This struct simply stores the remainder of the data {VirtualConnection,
   // VirtualConnection::AssociatedData} that is not broken up into map keys for
@@ -118,9 +126,12 @@ class VirtualConnectionRouter final : public CastSocket::Client {
     SocketErrorHandler* error_handler;
   };
 
+  ConnectionNamespaceHandler* connection_handler_ = nullptr;
+
   std::map<int /* socket_id */,
            std::multimap<std::string /* local_id */, VCTail>>
       connections_;
+
   std::map<int, SocketWithHandler> sockets_;
   std::map<std::string /* local_id */, CastMessageHandler*> endpoints_;
 };
diff --git a/cast/common/channel/virtual_connection_router_unittest.cc b/cast/common/channel/virtual_connection_router_unittest.cc
index 69a08c53..647a01fd 100644
--- a/cast/common/channel/virtual_connection_router_unittest.cc
+++ b/cast/common/channel/virtual_connection_router_unittest.cc
@@ -6,6 +6,7 @@
 
 #include <utility>
 
+#include "cast/common/channel/connection_namespace_handler.h"
 #include "cast/common/channel/message_util.h"
 #include "cast/common/channel/proto/cast_channel.pb.h"
 #include "cast/common/channel/testing/fake_cast_socket.h"
@@ -386,5 +387,51 @@ TEST_F(VirtualConnectionRouterTest, BroadcastsFromRemoteSource) {
   ASSERT_TRUE(remote_router_.BroadcastFromLocalPeer("wendy", message).ok());
 }
 
+// Tests that the VirtualConnectionRouter treats kConnectionNamespace messages
+// as a special case. The details of this are described in the implementation of
+// VirtualConnectionRouter::OnMessage().
+TEST_F(VirtualConnectionRouterTest, HandlesConnectionMessagesAsSpecialCase) {
+  class MockConnectionNamespaceHandler final
+      : public ConnectionNamespaceHandler,
+        public ConnectionNamespaceHandler::VirtualConnectionPolicy {
+   public:
+    explicit MockConnectionNamespaceHandler(VirtualConnectionRouter* vc_router)
+        : ConnectionNamespaceHandler(vc_router, this) {}
+    ~MockConnectionNamespaceHandler() final = default;
+    MOCK_METHOD(void,
+                OnMessage,
+                (VirtualConnectionRouter * router,
+                 CastSocket* socket,
+                 ::cast::channel::CastMessage message),
+                (final));
+    bool IsConnectionAllowed(
+        const VirtualConnection& virtual_conn) const final {
+      return true;
+    }
+  };
+  MockConnectionNamespaceHandler connection_handler(&local_router_);
+
+  MockCastMessageHandler alice;
+  local_router_.AddHandlerForLocalId("alice", &alice);
+
+  CastMessage message;
+  message.set_protocol_version(
+      ::cast::channel::CastMessage_ProtocolVersion_CASTV2_1_0);
+  message.set_source_id(kPlatformSenderId);
+  message.set_destination_id("alice");
+  message.set_namespace_(kConnectionNamespace);
+
+  CastMessage message_received;
+  EXPECT_CALL(connection_handler, OnMessage(&local_router_, local_socket_, _))
+      .WillOnce(SaveArg<2>(&message_received))
+      .RetiresOnSaturation();
+  EXPECT_CALL(alice, OnMessage(_, _, _)).Times(0);
+  local_router_.OnMessage(local_socket_, message);
+
+  EXPECT_EQ(kPlatformSenderId, message.source_id());
+  EXPECT_EQ("alice", message.destination_id());
+  EXPECT_EQ(kConnectionNamespace, message.namespace_());
+}
+
 }  // namespace cast
 }  // namespace openscreen
diff --git a/cast/receiver/application_agent.cc b/cast/receiver/application_agent.cc
index 06ca3095..df2b49e6 100644
--- a/cast/receiver/application_agent.cc
+++ b/cast/receiver/application_agent.cc
@@ -137,10 +137,6 @@ void ApplicationAgent::OnMessage(VirtualConnectionRouter* router,
   }
 
   const std::string& ns = message.namespace_();
-  if (ns == kConnectionNamespace) {
-    connection_handler_.OnMessage(router, socket, std::move(message));
-    return;
-  }
   if (ns == kAuthNamespace) {
     auth_handler_.OnMessage(router, socket, std::move(message));
     return;