Merge pull request #13216 from birarda/bug/domain-ice-peer-id

never re-use node ID during domain connection

domain-server/src/DomainGatekeeper.cpp

@@ -479,7 +479,7 @@ SharedNodePointer DomainGatekeeper::processAgentConnectRequest(const NodeConnect
         limitedNodeList->killNodeWithUUID(existingNodeID);
     }
 
-    // add the connecting node (or re-use the matched one from eachNodeBreakable above)
+    // add the connecting node
     SharedNodePointer newNode = addVerifiedNodeFromConnectRequest(nodeConnection);
 
     // set the edit rights for this user
@@ -508,25 +508,21 @@ SharedNodePointer DomainGatekeeper::processAgentConnectRequest(const NodeConnect
     return newNode;
 }
 
-SharedNodePointer DomainGatekeeper::addVerifiedNodeFromConnectRequest(const NodeConnectionData& nodeConnection,
+SharedNodePointer DomainGatekeeper::addVerifiedNodeFromConnectRequest(const NodeConnectionData& nodeConnection) {
-                                                                      QUuid nodeID) {
     HifiSockAddr discoveredSocket = nodeConnection.senderSockAddr;
     SharedNetworkPeer connectedPeer = _icePeers.value(nodeConnection.connectUUID);
 
-    if (connectedPeer) {
+    if (connectedPeer && connectedPeer->getActiveSocket()) {
-        //  this user negotiated a connection with us via ICE, so re-use their ICE client ID
-        nodeID = nodeConnection.connectUUID;
-
-        if (connectedPeer->getActiveSocket()) {
         // set their discovered socket to whatever the activated socket on the network peer object was
         discoveredSocket = *connectedPeer->getActiveSocket();
     }
-    } else {
+
-        // we got a connectUUID we didn't recognize, either use the hinted node ID or randomly generate a new one
+    // create a new node ID for the verified connecting node
-        if (nodeID.isNull()) {
+    auto nodeID = QUuid::createUuid();
-            nodeID = QUuid::createUuid();
+
-        }
+    // add a mapping from connection node ID to ICE peer ID
-    }
+    // so that we can remove the ICE peer once we see this node connect
+    _nodeToICEPeerIDs.insert(nodeID, nodeConnection.connectUUID);
 
     auto limitedNodeList = DependencyManager::get<LimitedNodeList>();
 
@@ -541,6 +537,15 @@ SharedNodePointer DomainGatekeeper::addVerifiedNodeFromConnectRequest(const Node
     return newNode;
 }
 
+void DomainGatekeeper::cleanupICEPeerForNode(const QUuid& nodeID) {
+    // remove this node ID from our node to ICE peer ID map
+    // and the associated ICE peer (if it still exists)
+    auto icePeerID = _nodeToICEPeerIDs.take(nodeID);
+    if (!icePeerID.isNull()) {
+        _icePeers.remove(icePeerID);
+    }
+}
+
 bool DomainGatekeeper::verifyUserSignature(const QString& username,
                                            const QByteArray& usernameSignature,
                                            const HifiSockAddr& senderSockAddr) {

domain-server/src/DomainGatekeeper.h

@@ -40,7 +40,7 @@ public:
                                 const QUuid& walletUUID, const QString& nodeVersion);
     QUuid assignmentUUIDForPendingAssignment(const QUuid& tempUUID);
 
-    void removeICEPeer(const QUuid& peerUUID) { _icePeers.remove(peerUUID); }
+    void cleanupICEPeerForNode(const QUuid& nodeID);
 
     Node::LocalID findOrCreateLocalID(const QUuid& uuid);
 
@@ -77,8 +77,7 @@ private:
     SharedNodePointer processAgentConnectRequest(const NodeConnectionData& nodeConnection,
                                                  const QString& username,
                                                  const QByteArray& usernameSignature);
-    SharedNodePointer addVerifiedNodeFromConnectRequest(const NodeConnectionData& nodeConnection,
+    SharedNodePointer addVerifiedNodeFromConnectRequest(const NodeConnectionData& nodeConnection);
-                                                        QUuid nodeID = QUuid());
     
     bool verifyUserSignature(const QString& username, const QByteArray& usernameSignature,
                              const HifiSockAddr& senderSockAddr);
@@ -102,6 +101,10 @@ private:
     
     QHash<QUuid, SharedNetworkPeer> _icePeers;
 
+    using ConnectingNodeID = QUuid;
+    using ICEPeerID = QUuid;
+    QHash<ConnectingNodeID, ICEPeerID> _nodeToICEPeerIDs;
+    
     QHash<QString, QUuid> _connectionTokenHash;
 
     // the word "optimistic" below is used for keys that we request during user connection before the user has

domain-server/src/DomainServer.cpp

@@ -1017,15 +1017,22 @@ void DomainServer::processListRequestPacket(QSharedPointer<ReceivedMessage> mess
     sendingNode->setPublicSocket(nodeRequestData.publicSockAddr);
     sendingNode->setLocalSocket(nodeRequestData.localSockAddr);
 
-    // update the NodeInterestSet in case there have been any changes
     DomainServerNodeData* nodeData = static_cast<DomainServerNodeData*>(sendingNode->getLinkedData());
 
+    if (!nodeData->hasCheckedIn()) {
+        nodeData->setHasCheckedIn(true);
+
+        // on first check in, make sure we've cleaned up any ICE peer for this node
+        _gatekeeper.cleanupICEPeerForNode(sendingNode->getUUID());
+    }
+
     // guard against patched agents asking to hear about other agents
     auto safeInterestSet = nodeRequestData.interestList.toSet();
     if (sendingNode->getType() == NodeType::Agent) {
         safeInterestSet.remove(NodeType::Agent);
     }
 
+    // update the NodeInterestSet in case there have been any changes
     nodeData->setNodeInterestSet(safeInterestSet);
 
     // update the connecting hostname in case it has changed
@@ -2945,7 +2952,7 @@ void DomainServer::nodeAdded(SharedNodePointer node) {
 
 void DomainServer::nodeKilled(SharedNodePointer node) {
     // if this peer connected via ICE then remove them from our ICE peers hash
-    _gatekeeper.removeICEPeer(node->getUUID());
+    _gatekeeper.cleanupICEPeerForNode(node->getUUID());
 
     DomainServerNodeData* nodeData = static_cast<DomainServerNodeData*>(node->getLinkedData());
 
@@ -2978,6 +2985,8 @@ void DomainServer::nodeKilled(SharedNodePointer node) {
             }
         }
     }
+
+    broadcastNodeDisconnect(node);
 }
 
 SharedAssignmentPointer DomainServer::dequeueMatchingAssignment(const QUuid& assignmentUUID, NodeType_t nodeType) {
@@ -3163,18 +3172,23 @@ void DomainServer::handleKillNode(SharedNodePointer nodeToKill) {
     const QUuid& nodeUUID = nodeToKill->getUUID();
 
     limitedNodeList->killNodeWithUUID(nodeUUID);
+}
 
-    static auto removedNodePacket = NLPacket::create(PacketType::DomainServerRemovedNode, NUM_BYTES_RFC4122_UUID);
+void DomainServer::broadcastNodeDisconnect(const SharedNodePointer& disconnectedNode) {
+    auto limitedNodeList = DependencyManager::get<LimitedNodeList>();
+
+    static auto removedNodePacket = NLPacket::create(PacketType::DomainServerRemovedNode, NUM_BYTES_RFC4122_UUID, true);
 
     removedNodePacket->reset();
-    removedNodePacket->write(nodeUUID.toRfc4122());
+    removedNodePacket->write(disconnectedNode->getUUID().toRfc4122());
 
     // broadcast out the DomainServerRemovedNode message
-    limitedNodeList->eachMatchingNode([this, &nodeToKill](const SharedNodePointer& otherNode) -> bool {
+    limitedNodeList->eachMatchingNode([this, &disconnectedNode](const SharedNodePointer& otherNode) -> bool {
         // only send the removed node packet to nodes that care about the type of node this was
-        return isInInterestSet(otherNode, nodeToKill);
+        return isInInterestSet(otherNode, disconnectedNode);
     }, [&limitedNodeList](const SharedNodePointer& otherNode){
-        limitedNodeList->sendUnreliablePacket(*removedNodePacket, *otherNode);
+        auto removedNodePacketCopy = NLPacket::createCopy(*removedNodePacket);
+        limitedNodeList->sendPacket(std::move(removedNodePacketCopy), *otherNode);
     });
 }
 

domain-server/src/DomainServer.h

@@ -165,6 +165,7 @@ private:
     unsigned int countConnectedUsers();
 
     void handleKillNode(SharedNodePointer nodeToKill);
+    void broadcastNodeDisconnect(const SharedNodePointer& disconnnectedNode);
 
     void sendDomainListToNode(const SharedNodePointer& node, const HifiSockAddr& senderSockAddr);
 

domain-server/src/DomainServerNodeData.h

@@ -67,9 +67,12 @@ public:
     const QString& getPlaceName() { return _placeName; }
     void setPlaceName(const QString& placeName) { _placeName = placeName; }
 
-    bool wasAssigned() const { return _wasAssigned; };
+    bool wasAssigned() const { return _wasAssigned; }
     void setWasAssigned(bool wasAssigned) { _wasAssigned = wasAssigned; }
 
+    bool hasCheckedIn() const { return _hasCheckedIn; }
+    void setHasCheckedIn(bool hasCheckedIn) { _hasCheckedIn = hasCheckedIn; }
+    
 private:
     QJsonObject overrideValuesIfNeeded(const QJsonObject& newStats);
     QJsonArray overrideValuesIfNeeded(const QJsonArray& newStats);
@@ -94,6 +97,8 @@ private:
     QString _placeName;
 
     bool _wasAssigned { false };
+
+    bool _hasCheckedIn { false };
 };
 
 #endif // hifi_DomainServerNodeData_h