fix: OnClientDisconnected client identifier is incorrect when pending client connection is denied [MTT-6376] (#2569)

* fix

When connection approval is enabled and a client is not approved, the OnClientDisconnected callback handler was not being invoked.
(It was when a client terminates the connection but not when the server does)

* test

The test to validate the fix for the server-host not receiving the correct client identifier when denied approval for its pending connection.
Added additional functionality to NetcodeIntegrationTest that allows you to ignore waiting for a newly created client (i.e. not the default defined NumberOfClients) to connect.
Adding additional validation to assure we don't get multiple disconnect notifications on the server-host side when a client is disconnected.


* Update CHANGELOG.md

* style

renaming NetcodeIntegrationTest.WaitForNewClientToConnect to NetcodeIntegrationTest.ShouldWaitForNewClientToConnect

Author: NoelStephensUnity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -15,6 +15,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- Fixed issue where the `OnClientDisconnected` client identifier was incorrect after a pending client connection was denied. (#2569)
 - Fixed warning "Runtime Network Prefabs was not empty at initialization time." being erroneously logged when no runtime network prefabs had been added (#2565)
 - Fixed issue where some temporary debug console logging was left in a merged PR. (#2562)
 - Fixed the "Generate Default Network Prefabs List" setting not loading correctly and always reverting to being checked. (#2545)

com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs

@@ -844,6 +844,16 @@ internal void OnClientDisconnectFromServer(ulong clientId)
             {
                 var transportId = ClientIdToTransportId(clientId);
                 NetworkManager.NetworkConfig.NetworkTransport.DisconnectRemoteClient(transportId);
+
+                try
+                {
+                    OnClientDisconnectCallback?.Invoke(clientId);
+                }
+                catch (Exception exception)
+                {
+                    Debug.LogException(exception);
+                }
+
                 // Clean up the transport to client (and vice versa) mappings
                 TransportIdCleanUp(transportId);
             }

com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTest.cs

@@ -431,6 +431,18 @@ protected virtual void OnNewClientStartedAndConnected(NetworkManager networkMana
         {
         }
 
+        /// <summary>
+        /// CreateAndStartNewClient Only
+        /// Override this method to bypass the waiting for a client to connect.
+        /// </summary>
+        /// <remarks>
+        /// Use this for testing connection and disconnection scenarios
+        /// </remarks>
+        protected virtual bool ShouldWaitForNewClientToConnect(NetworkManager networkManager)
+        {
+            return true;
+        }
+
         /// <summary>
         /// This will create, start, and connect a new client while in the middle of an
         /// integration test.
@@ -455,19 +467,22 @@ protected IEnumerator CreateAndStartNewClient()
 
             OnNewClientStarted(networkManager);
 
-            // Wait for the new client to connect
-            yield return WaitForClientsConnectedOrTimeOut();
-
-            OnNewClientStartedAndConnected(networkManager);
-            if (s_GlobalTimeoutHelper.TimedOut)
+            if (ShouldWaitForNewClientToConnect(networkManager))
             {
-                AddRemoveNetworkManager(networkManager, false);
-                Object.DestroyImmediate(networkManager.gameObject);
-            }
+                // Wait for the new client to connect
+                yield return WaitForClientsConnectedOrTimeOut();
 
-            AssertOnTimeout($"{nameof(CreateAndStartNewClient)} timed out waiting for the new client to be connected!");
-            ClientNetworkManagerPostStart(networkManager);
-            VerboseDebug($"[{networkManager.name}] Created and connected!");
+                OnNewClientStartedAndConnected(networkManager);
+                if (s_GlobalTimeoutHelper.TimedOut)
+                {
+                    AddRemoveNetworkManager(networkManager, false);
+                    Object.DestroyImmediate(networkManager.gameObject);
+                }
+
+                AssertOnTimeout($"{nameof(CreateAndStartNewClient)} timed out waiting for the new client to be connected!");
+                ClientNetworkManagerPostStart(networkManager);
+                VerboseDebug($"[{networkManager.name}] Created and connected!");
+            }
         }
 
         /// <summary>

com.unity.netcode.gameobjects/Tests/Runtime/ClientApprovalDenied.cs

@@ -0,0 +1,72 @@
+using System.Collections;
+using System.Collections.Generic;
+using NUnit.Framework;
+using Unity.Netcode.TestHelpers.Runtime;
+using UnityEngine.TestTools;
+
+
+namespace Unity.Netcode.RuntimeTests
+{
+    public class ClientApprovalDenied : NetcodeIntegrationTest
+    {
+        protected override int NumberOfClients => 2;
+        private bool m_ApproveConnection = true;
+        private ulong m_PendingClientId = 0;
+        private ulong m_DisconnectedClientId = 0;
+
+        private List<ulong> m_DisconnectedClientIdentifiers = new List<ulong>();
+
+        private void ConnectionApproval(NetworkManager.ConnectionApprovalRequest connectionApprovalRequest, NetworkManager.ConnectionApprovalResponse connectionApprovalResponse)
+        {
+            connectionApprovalResponse.Approved = m_ApproveConnection;
+            connectionApprovalResponse.CreatePlayerObject = true;
+            // When denied, store the client identifier to use for validating the client disconnected notification identifier matches
+            if (!m_ApproveConnection)
+            {
+                m_PendingClientId = connectionApprovalRequest.ClientNetworkId;
+            }
+        }
+
+        protected override void OnNewClientCreated(NetworkManager networkManager)
+        {
+            networkManager.NetworkConfig.ConnectionApproval = true;
+            base.OnNewClientCreated(networkManager);
+        }
+
+        protected override bool ShouldWaitForNewClientToConnect(NetworkManager networkManager)
+        {
+            return false;
+        }
+
+        /// <summary>
+        /// Validates that when a pending client is denied approval the server-host
+        /// OnClientDisconnected method will return the valid pending client identifier.
+        /// </summary>
+        [UnityTest]
+        public IEnumerator ClientDeniedAndDisconnectionNotificationTest()
+        {
+            m_ServerNetworkManager.NetworkConfig.ConnectionApproval = true;
+            m_ServerNetworkManager.ConnectionApprovalCallback = ConnectionApproval;
+            m_ApproveConnection = false;
+            m_ServerNetworkManager.OnClientDisconnectCallback += OnClientDisconnectCallback;
+            yield return CreateAndStartNewClient();
+            yield return WaitForConditionOrTimeOut(() => m_PendingClientId == m_DisconnectedClientId);
+            AssertOnTimeout($"Timed out waiting for disconnect notification for pending Client-{m_PendingClientId}!");
+
+            // Validate that we don't get multiple disconnect notifications for clients being disconnected
+            // Have a client disconnect remotely
+            m_ClientNetworkManagers[0].Shutdown();
+
+            // Have the server disconnect a client
+            m_ServerNetworkManager.DisconnectClient(m_ClientNetworkManagers[1].LocalClientId);
+            m_ServerNetworkManager.OnClientDisconnectCallback -= OnClientDisconnectCallback;
+        }
+
+        private void OnClientDisconnectCallback(ulong clientId)
+        {
+            Assert.False(m_DisconnectedClientIdentifiers.Contains(clientId), $"Received two disconnect notifications from Client-{clientId}!");
+            m_DisconnectedClientIdentifiers.Add(clientId);
+            m_DisconnectedClientId = clientId;
+        }
+    }
+}