Network-chat: Fix remote peer making multiple connections

The system was just treating IP (and optionally port) as a unique
identifier, so if a peer had multiple possible paths to a client they
would connect multiple times.

This fixes that by generating using QUuid in each client.
We then use this during broadcast, replacing the username we
sent before (which was not used), and as part of the greeting.
The greeting now is more complex, since we need to send both
username and the ID.

Change-Id: I6c6c2ffd5198406aad48445a68dd6aab36de69c0
Reviewed-by: Konrad Kujawa <konrad.kujawa@qt.io>
Reviewed-by: Timur Pocheptsov <timur.pocheptsov@qt.io>

6 files changed, 77 insertions, 39 deletions

diff --git a/examples/network/network-chat/client.cpp b/examples/network/network-chat/client.cpp
index 54bde6cd03..495ae90d02 100644
--- a/examples/network/network-chat/client.cpp
+++ b/examples/network/network-chat/client.cpp
@@ -37,27 +37,14 @@ QString Client::nickName() const
            + ':' + QString::number(server.serverPort());
 }
 
-bool Client::hasConnection(const QHostAddress &senderIp, int senderPort) const
+bool Client::hasConnection(const QByteArray &peerUniqueId) const
 {
-    auto [begin, end] = peers.equal_range(senderIp);
-    if (begin == peers.constEnd())
-        return false;
-
-    if (senderPort == -1)
-        return true;
-
-    for (; begin != end; ++begin) {
-        Connection *connection = *begin;
-        if (connection->peerPort() == senderPort)
-            return true;
-    }
-
-    return false;
+    return peers.contains(peerUniqueId);
 }
 
 void Client::newConnection(Connection *connection)
 {
-    connection->setGreetingMessage(peerManager->userName());
+    connection->setGreetingMessage(peerManager->userName(), peerManager->uniqueId());
 
     connect(connection, &Connection::errorOccurred, this, &Client::connectionError);
     connect(connection, &Connection::disconnected, this, &Client::disconnected);
@@ -67,13 +54,18 @@ void Client::newConnection(Connection *connection)
 void Client::readyForUse()
 {
     Connection *connection = qobject_cast<Connection *>(sender());
-    if (!connection || hasConnection(connection->peerAddress(), connection->peerPort()))
+    if (!connection || hasConnection(connection->uniqueId())) {
+        if (connection) {
+            connection->disconnectFromHost();
+            connection->deleteLater();
+        }
         return;
+    }
 
     connect(connection,  &Connection::newMessage,
             this, &Client::newMessage);
 
-    peers.insert(connection->peerAddress(), connection);
+    peers.insert(connection->uniqueId(), connection);
     QString nick = connection->name();
     if (!nick.isEmpty())
         emit newParticipant(nick);
@@ -93,7 +85,7 @@ void Client::connectionError(QAbstractSocket::SocketError /* socketError */)
 
 void Client::removeConnection(Connection *connection)
 {
-    if (peers.remove(connection->peerAddress(), connection) > 0) {
+    if (peers.remove(connection->uniqueId(), connection) > 0) {
         QString nick = connection->name();
         if (!nick.isEmpty())
             emit participantLeft(nick);
diff --git a/examples/network/network-chat/client.h b/examples/network/network-chat/client.h
index 55abd84c9e..2ce6afe826 100644
--- a/examples/network/network-chat/client.h
+++ b/examples/network/network-chat/client.h
@@ -21,7 +21,7 @@ public:
 
     void sendMessage(const QString &message);
     QString nickName() const;
-    bool hasConnection(const QHostAddress &senderIp, int senderPort = -1) const;
+    bool hasConnection(const QByteArray &peerUniqueId) const;
 
 signals:
     void newMessage(const QString &from, const QString &message);
@@ -39,7 +39,7 @@ private:
 
     PeerManager *peerManager;
     Server server;
-    QMultiHash<QHostAddress, Connection *> peers;
+    QMultiHash<QByteArray, Connection *> peers;
 };
 
 #endif
diff --git a/examples/network/network-chat/connection.cpp b/examples/network/network-chat/connection.cpp
index f24e0e2a0b..0f59cc91ed 100644
--- a/examples/network/network-chat/connection.cpp
+++ b/examples/network/network-chat/connection.cpp
@@ -21,7 +21,7 @@ static const int PingInterval = 5 * 1000;
  *  plaintext   = { 0 => text }
  *  ping        = { 1 => null }
  *  pong        = { 2 => null }
- *  greeting    = { 3 => text }
+ *  greeting    = { 3 => { text, bytes } }
  */
 
 Connection::Connection(QObject *parent)
@@ -60,9 +60,15 @@ QString Connection::name() const
     return username;
 }
 
-void Connection::setGreetingMessage(const QString &message)
+void Connection::setGreetingMessage(const QString &message, const QByteArray &uniqueId)
 {
     greetingMessage = message;
+    localUniqueId = uniqueId;
+}
+
+QByteArray Connection::uniqueId() const
+{
+    return peerUniqueId;
 }
 
 bool Connection::sendMessage(const QString &message)
@@ -118,7 +124,29 @@ void Connection::processReadyRead()
             reader.next();
         } else {
             // Current state: read command payload
-            if (reader.isString()) {
+            if (currentDataType == Greeting) {
+                if (state == ReadingGreeting) {
+                    if (!reader.isContainer() || !reader.isLengthKnown() || reader.length() != 2)
+                        break; // protocol error
+                    state = ProcessingGreeting;
+                    reader.enterContainer();
+                }
+                if (state != ProcessingGreeting)
+                    break; // protocol error
+                if (reader.isString()) {
+                    auto r = reader.readString();
+                    buffer += r.data;
+                } else if (reader.isByteArray()) {
+                    auto r = reader.readByteArray();
+                    peerUniqueId += r.data;
+                    if (r.status == QCborStreamReader::EndOfString) {
+                        reader.leaveContainer();
+                        processGreeting();
+                    }
+                }
+                if (state == ProcessingGreeting)
+                    continue;
+            } else if (reader.isString()) {
                 auto r = reader.readString();
                 buffer += r.data;
                 if (r.status != QCborStreamReader::EndOfString)
@@ -126,7 +154,7 @@ void Connection::processReadyRead()
             } else if (reader.isNull()) {
                 reader.next();
             } else {
-                break;                   // protocol error
+                break; // protocol error
             }
 
             // Next state: no command read
@@ -136,13 +164,7 @@ void Connection::processReadyRead()
                 transferTimerId = -1;
             }
 
-            if (state == ReadingGreeting) {
-                if (currentDataType != Greeting)
-                    break;              // protocol error
-                processGreeting();
-            } else {
-                processData();
-            }
+            processData();
         }
     }
 
@@ -172,7 +194,10 @@ void Connection::sendGreetingMessage()
 
     writer.startMap(1);
     writer.append(Greeting);
+    writer.startArray(2);
     writer.append(greetingMessage);
+    writer.append(localUniqueId);
+    writer.endArray();
     writer.endMap();
     isGreetingMessageSent = true;
 
diff --git a/examples/network/network-chat/connection.h b/examples/network/network-chat/connection.h
index d877b17627..4b063e7def 100644
--- a/examples/network/network-chat/connection.h
+++ b/examples/network/network-chat/connection.h
@@ -20,6 +20,7 @@ public:
     enum ConnectionState {
         WaitingForGreeting,
         ReadingGreeting,
+        ProcessingGreeting,
         ReadyForUse
     };
     enum DataType {
@@ -35,9 +36,11 @@ public:
     ~Connection();
 
     QString name() const;
-    void setGreetingMessage(const QString &message);
+    void setGreetingMessage(const QString &message, const QByteArray &uniqueId);
     bool sendMessage(const QString &message);
 
+    QByteArray uniqueId() const;
+
 signals:
     void readyForUse();
     void newMessage(const QString &from, const QString &message);
@@ -62,6 +65,8 @@ private:
     QTimer pingTimer;
     QElapsedTimer pongTime;
     QString buffer;
+    QByteArray localUniqueId;
+    QByteArray peerUniqueId;
     ConnectionState state = WaitingForGreeting;
     DataType currentDataType = Undefined;
     int transferTimerId = -1;
diff --git a/examples/network/network-chat/peermanager.cpp b/examples/network/network-chat/peermanager.cpp
index da4210d85b..a70ed7da73 100644
--- a/examples/network/network-chat/peermanager.cpp
+++ b/examples/network/network-chat/peermanager.cpp
@@ -7,6 +7,7 @@
 #include "peermanager.h"
 
 #include <QNetworkInterface>
+#include <QUuid>
 
 static const qint32 BroadcastInterval = 2000;
 static const unsigned broadcastPort = 45000;
@@ -27,6 +28,11 @@ PeerManager::PeerManager(Client *client)
     if (username.isEmpty())
         username = "unknown";
 
+    // We generate a unique per-process identifier so we can avoid multiple
+    // connections to/from the same remote peer as well as ignore our own
+    // broadcasts.
+    localUniqueId = QUuid::createUuid().toByteArray();
+
     updateAddresses();
 
     broadcastSocket.bind(QHostAddress::Any, broadcastPort, QUdpSocket::ShareAddress
@@ -49,6 +55,11 @@ QString PeerManager::userName() const
     return username;
 }
 
+QByteArray PeerManager::uniqueId() const
+{
+    return localUniqueId;
+}
+
 void PeerManager::startBroadcasting()
 {
     broadcastTimer.start();
@@ -65,7 +76,7 @@ void PeerManager::sendBroadcastDatagram()
     {
         QCborStreamWriter writer(&datagram);
         writer.startArray(2);
-        writer.append(username);
+        writer.append(localUniqueId);
         writer.append(serverPort);
         writer.endArray();
     }
@@ -92,6 +103,7 @@ void PeerManager::readBroadcastDatagram()
             continue;
 
         int senderServerPort;
+        QByteArray peerUniqueId;
         {
             // decode the datagram
             QCborStreamReader reader(datagram);
@@ -101,10 +113,12 @@ void PeerManager::readBroadcastDatagram()
                 continue;
 
             reader.enterContainer();
-            if (reader.lastError() != QCborError::NoError || !reader.isString())
+            if (reader.lastError() != QCborError::NoError || !reader.isByteArray())
                 continue;
-            while (reader.readString().status == QCborStreamReader::Ok) {
-                // we don't actually need the username right now
+            auto r = reader.readByteArray();
+            while (r.status == QCborStreamReader::Ok) {
+                peerUniqueId = r.data;
+                r = reader.readByteArray();
             }
 
             if (reader.lastError() != QCborError::NoError || !reader.isUnsignedInteger())
@@ -112,10 +126,10 @@ void PeerManager::readBroadcastDatagram()
             senderServerPort = reader.toInteger();
         }
 
-        if (isLocalHostAddress(senderIp) && senderServerPort == serverPort)
+        if (peerUniqueId == localUniqueId)
             continue;
 
-        if (!client->hasConnection(senderIp)) {
+        if (!client->hasConnection(peerUniqueId)) {
             Connection *connection = new Connection(this);
             emit newConnection(connection);
             connection->connectToHost(senderIp, senderServerPort);
diff --git a/examples/network/network-chat/peermanager.h b/examples/network/network-chat/peermanager.h
index dadabd88aa..fff48540ea 100644
--- a/examples/network/network-chat/peermanager.h
+++ b/examples/network/network-chat/peermanager.h
@@ -22,6 +22,7 @@ public:
 
     void setServerPort(int port);
     QString userName() const;
+    QByteArray uniqueId() const;
     void startBroadcasting();
     bool isLocalHostAddress(const QHostAddress &address) const;
 
@@ -41,6 +42,7 @@ private:
     QUdpSocket broadcastSocket;
     QTimer broadcastTimer;
     QString username;
+    QByteArray localUniqueId;
     int serverPort = 0;
 };