fix: preventing issues with game code registering multiple ConnectionApprovalCallbacks [MTT-3496]

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -12,12 +12,16 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Changed
 
+- [breaking] `ConnectionApprovalCallback` is now `ConnectionApprovalHandler`. Receives a `ConnectionApprovalRequest`, and returns a `ConnectionApprovalResult`. By virtue of being a `Func<>`, there cannot be multiple handlers registered.
+
 ### Removed
 
 ### Fixed
 
 - Fixed endless dialog boxes when adding a NetworkBehaviour to a NetworkManager or vice-versa (#1947)
 
+- Fixed issues when multiple Connection Approval callbacks were registered (#1941)
+
 ## [1.0.0-pre.9] - 2022-05-10
 
 ### Fixed

com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs

@@ -358,21 +358,54 @@ public IReadOnlyList<ulong> ConnectedClientsIds
         public event Action OnServerStarted = null;
 
         /// <summary>
-        /// Delegate type called when connection has been approved. This only has to be set on the server.
+        /// Connection Approval Result
         /// </summary>
-        /// <param name="createPlayerObject">If true, a player object will be created. Otherwise the client will have no object.</param>
-        /// <param name="playerPrefabHash">The prefabHash to use for the client. If createPlayerObject is false, this is ignored. If playerPrefabHash is null, the default player prefab is used.</param>
-        /// <param name="approved">Whether or not the client was approved</param>
-        /// <param name="position">The position to spawn the client at. If null, the prefab position is used.</param>
-        /// <param name="rotation">The rotation to spawn the client with. If null, the prefab position is used.</param>
-        public delegate void ConnectionApprovedDelegate(bool createPlayerObject, uint? playerPrefabHash, bool approved, Vector3? position, Quaternion? rotation);
+        /// <param name="Approved">Whether or not the client was approved</param>
+        /// <param name="CreatePlayerObject">If true, a player object will be created. Otherwise the client will have no object.</param>
+        /// <param name="PlayerPrefabHash">The prefabHash to use for the client. If createPlayerObject is false, this is ignored. If playerPrefabHash is null, the default player prefab is used.</param>
+        /// <param name="Position">The position to spawn the client at. If null, the prefab position is used.</param>
+        /// <param name="Rotation">The rotation to spawn the client with. If null, the prefab position is used.</param>
+        public struct ConnectionApprovalResult
+        {
+            public bool Approved;
+            public bool CreatePlayerObject;
+            public uint? PlayerPrefabHash;
+            public Vector3? Position;
+            public Quaternion? Rotation;
+        }
+
+        /// <summary>
+        /// Connection Approval Request
+        /// </summary>
+        /// <param name="Payload">The connection data payload</param>
+        /// <param name="ClientNetworkId">The Client Network Id of the client we are about to approve</param>
+        public struct ConnectionApprovalRequest
+        {
+            public byte[] Payload;
+            public ulong ClientNetworkId;
+        }
 
         /// <summary>
-        /// The callback to invoke during connection approval
+        /// The callback to invoke during connection approval. Allows client code to decide to allow a client in or not
         /// </summary>
-        public event Action<byte[], ulong, ConnectionApprovedDelegate> ConnectionApprovalCallback = null;
+        public Func<ConnectionApprovalRequest, ConnectionApprovalResult> ConnectionApprovalHandler
+        {
+            get => m_ConnectionApprovalHandler;
+            set
+            {
+                if (value != null && value.GetInvocationList().Length > 1)
+                {
+                    Debug.LogError(
+                        $"Only one connection approval handler is supported in {nameof(m_ConnectionApprovalHandler)}. Rejecting further adds.");
+                }
+                else
+                {
+                    m_ConnectionApprovalHandler = value;
+                }
+            }
+        }
 
-        internal void InvokeConnectionApproval(byte[] payload, ulong clientId, ConnectionApprovedDelegate action) => ConnectionApprovalCallback?.Invoke(payload, clientId, action);
+        private Func<ConnectionApprovalRequest, ConnectionApprovalResult> m_ConnectionApprovalHandler;
 
         /// <summary>
         /// The current NetworkConfig
@@ -970,27 +1003,28 @@ public bool StartHost()
             IsClient = true;
             IsListening = true;
 
-            if (NetworkConfig.ConnectionApproval)
+            if (NetworkConfig.ConnectionApproval && ConnectionApprovalHandler != null)
             {
-                InvokeConnectionApproval(NetworkConfig.ConnectionData, ServerClientId,
-                    (createPlayerObject, playerPrefabHash, approved, position, rotation) =>
+                var decision = ConnectionApprovalHandler(new ConnectionApprovalRequest { Payload = NetworkConfig.ConnectionData, ClientNetworkId = ServerClientId });
+
+                if (!decision.Approved)
+                {
+                    if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
                     {
-                        // You cannot decline the local server. Force approved to true
-                        if (!approved)
-                        {
-                            if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
-                            {
-                                NetworkLog.LogWarning(
-                                    "You cannot decline the host connection. The connection was automatically approved.");
-                            }
-                        }
+                        NetworkLog.LogWarning(
+                            "You cannot decline the host connection. The connection was automatically approved.");
+                    }
+                }
 
-                        HandleApproval(ServerClientId, createPlayerObject, playerPrefabHash, true, position, rotation);
-                    });
+                decision.Approved = true;
+                HandleApproval(ServerClientId, decision);
             }
             else
             {
-                HandleApproval(ServerClientId, NetworkConfig.PlayerPrefab != null, null, true, null, null);
+                var decision = new ConnectionApprovalResult();
+                decision.Approved = true;
+                decision.CreatePlayerObject = NetworkConfig.PlayerPrefab != null;
+                HandleApproval(ServerClientId, decision);
             }
 
             SpawnManager.ServerSpawnSceneObjectsOnStartSweep();
@@ -1023,7 +1057,7 @@ private bool CanStart(StartType type)
             // Clients don't invoke the ConnectionApprovalCallback
             if (NetworkConfig.ConnectionApproval && type != StartType.Client)
             {
-                if (ConnectionApprovalCallback == null)
+                if (ConnectionApprovalHandler == null)
                 {
                     if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
                     {
@@ -1033,7 +1067,7 @@ private bool CanStart(StartType type)
                 }
             }
 
-            if (ConnectionApprovalCallback != null)
+            if (ConnectionApprovalHandler != null)
             {
                 if (!NetworkConfig.ConnectionApproval)
                 {
@@ -1819,14 +1853,10 @@ private void SyncTime()
         /// Server Side: Handles the approval of a client
         /// </summary>
         /// <param name="ownerClientId">client being approved</param>
-        /// <param name="createPlayerObject">whether we want to create a player or not</param>
-        /// <param name="playerPrefabHash">the GlobalObjectIdHash value for the Network Prefab to create as the player</param>
-        /// <param name="approved">Is the player approved or not?</param>
-        /// <param name="position">Used when createPlayerObject is true, position of the player when spawned </param>
-        /// <param name="rotation">Used when createPlayerObject is true, rotation of the player when spawned</param>
-        internal void HandleApproval(ulong ownerClientId, bool createPlayerObject, uint? playerPrefabHash, bool approved, Vector3? position, Quaternion? rotation)
+        /// <param name="decision">the decision to allow the player in or not, with its parameters</param>
+        internal void HandleApproval(ulong ownerClientId, ConnectionApprovalResult decision)
         {
-            if (approved)
+            if (decision.Approved)
             {
                 // Inform new client it got approved
                 PendingClients.Remove(ownerClientId);
@@ -1836,10 +1866,14 @@ internal void HandleApproval(ulong ownerClientId, bool createPlayerObject, uint?
                 m_ConnectedClientsList.Add(client);
                 m_ConnectedClientIds.Add(client.ClientId);
 
-                if (createPlayerObject)
+                if (decision.CreatePlayerObject)
                 {
-                    var networkObject = SpawnManager.CreateLocalNetworkObject(false, playerPrefabHash ?? NetworkConfig.PlayerPrefab.GetComponent<NetworkObject>().GlobalObjectIdHash, ownerClientId, null, null, position, rotation);
-                    SpawnManager.SpawnNetworkObjectLocally(networkObject, SpawnManager.GetNetworkObjectId(), false, true, ownerClientId, false);
+                    var networkObject = SpawnManager.CreateLocalNetworkObject(false,
+                        decision.PlayerPrefabHash ??
+                        NetworkConfig.PlayerPrefab.GetComponent<NetworkObject>().GlobalObjectIdHash, ownerClientId,
+                        null, null, decision.Position, decision.Rotation);
+                    SpawnManager.SpawnNetworkObjectLocally(networkObject, SpawnManager.GetNetworkObjectId(), false,
+                        true, ownerClientId, false);
 
                     ConnectedClients[ownerClientId].PlayerObject = networkObject;
                 }
@@ -1879,13 +1913,13 @@ internal void HandleApproval(ulong ownerClientId, bool createPlayerObject, uint?
                     InvokeOnClientConnectedCallback(ownerClientId);
                 }
 
-                if (!createPlayerObject || (playerPrefabHash == null && NetworkConfig.PlayerPrefab == null))
+                if (!decision.CreatePlayerObject || (decision.PlayerPrefabHash == null && NetworkConfig.PlayerPrefab == null))
                 {
                     return;
                 }
 
                 // Separating this into a contained function call for potential further future separation of when this notification is sent.
-                ApprovedPlayerSpawn(ownerClientId, playerPrefabHash ?? NetworkConfig.PlayerPrefab.GetComponent<NetworkObject>().GlobalObjectIdHash);
+                ApprovedPlayerSpawn(ownerClientId, decision.PlayerPrefabHash ?? NetworkConfig.PlayerPrefab.GetComponent<NetworkObject>().GlobalObjectIdHash);
             }
             else
             {

com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ConnectionRequestMessage.cs

@@ -101,16 +101,17 @@ public void Handle(ref NetworkContext context)
             {
                 // Note: Delegate creation allocates.
                 // Note: ToArray() also allocates. :(
-                networkManager.InvokeConnectionApproval(ConnectionData, senderId,
-                    (createPlayerObject, playerPrefabHash, approved, position, rotation) =>
-                    {
-                        var localCreatePlayerObject = createPlayerObject;
-                        networkManager.HandleApproval(senderId, localCreatePlayerObject, playerPrefabHash, approved, position, rotation);
-                    });
+                var decision = networkManager.ConnectionApprovalHandler(new NetworkManager.ConnectionApprovalRequest
+                { Payload = ConnectionData, ClientNetworkId = senderId });
+
+                networkManager.HandleApproval(senderId, decision);
             }
             else
             {
-                networkManager.HandleApproval(senderId, networkManager.NetworkConfig.PlayerPrefab != null, null, true, null, null);
+                var decision = new NetworkManager.ConnectionApprovalResult();
+                decision.Approved = true;
+                decision.CreatePlayerObject = networkManager.NetworkConfig.PlayerPrefab != null;
+                networkManager.HandleApproval(senderId, decision);
             }
         }
     }

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

@@ -24,7 +24,7 @@ public void Setup()
         [UnityTest]
         public IEnumerator ConnectionApproval()
         {
-            NetworkManagerHelper.NetworkManagerObject.ConnectionApprovalCallback += NetworkManagerObject_ConnectionApprovalCallback;
+            NetworkManagerHelper.NetworkManagerObject.ConnectionApprovalHandler += NetworkManagerObject_ConnectionApprovalCallback;
             NetworkManagerHelper.NetworkManagerObject.NetworkConfig.ConnectionApproval = true;
             NetworkManagerHelper.NetworkManagerObject.NetworkConfig.PlayerPrefab = null;
             NetworkManagerHelper.NetworkManagerObject.NetworkConfig.ConnectionData = Encoding.UTF8.GetBytes(m_ValidationToken.ToString());
@@ -47,14 +47,22 @@ public IEnumerator ConnectionApproval()
             Assert.True(m_IsValidated);
         }
 
-        private void NetworkManagerObject_ConnectionApprovalCallback(byte[] connectionData, ulong clientId, NetworkManager.ConnectionApprovedDelegate callback)
+        private NetworkManager.ConnectionApprovalResult NetworkManagerObject_ConnectionApprovalCallback(NetworkManager.ConnectionApprovalRequest request)
         {
-            var stringGuid = Encoding.UTF8.GetString(connectionData);
+            var decision = new NetworkManager.ConnectionApprovalResult();
+            var stringGuid = Encoding.UTF8.GetString(request.Payload);
             if (m_ValidationToken.ToString() == stringGuid)
             {
                 m_IsValidated = true;
             }
-            callback(false, null, m_IsValidated, null, null);
+
+            decision.Approved = m_IsValidated;
+            decision.CreatePlayerObject = false;
+            decision.Position = null;
+            decision.Rotation = null;
+            decision.PlayerPrefabHash = null;
+
+            return decision;
         }
 
         [TearDown]

testproject/Assets/Tests/Manual/Scripts/ConnectionApprovalComponent.cs

@@ -65,7 +65,7 @@ private void Start()
 
             if (NetworkManager != null && NetworkManager.NetworkConfig.ConnectionApproval)
             {
-                NetworkManager.ConnectionApprovalCallback += ConnectionApprovalCallback;
+                NetworkManager.ConnectionApprovalHandler += ConnectionApprovalCallback;
 
                 if (m_ApprovalToken != string.Empty)
                 {
@@ -151,41 +151,50 @@ public void OnDisconnectClient()
         /// <summary>
         /// Invoked only on the server, this will handle the various connection approval combinations
         /// </summary>
-        /// <param name="dataToken">key or password to get approval</param>
-        /// <param name="clientId">client identifier being approved</param>
-        /// <param name="aprovalCallback">callback that should be invoked once it is determined if client is approved or not</param>
-        private void ConnectionApprovalCallback(byte[] dataToken, ulong clientId, NetworkManager.ConnectionApprovedDelegate aprovalCallback)
+        /// <param name="request">The connection approval request</param>
+        /// <returns>ConnectionApprovalResult with the approval decision, with parameters</returns>
+        private NetworkManager.ConnectionApprovalResult ConnectionApprovalCallback(NetworkManager.ConnectionApprovalRequest request)
         {
-            string approvalToken = Encoding.ASCII.GetString(dataToken);
+            var decision = new NetworkManager.ConnectionApprovalResult();
+            string approvalToken = Encoding.ASCII.GetString(request.Payload);
             var isTokenValid = approvalToken == m_ApprovalToken;
-            if (m_SimulateFailure && m_SimulateFailure.isOn && IsServer && clientId != NetworkManager.LocalClientId)
+            if (m_SimulateFailure && m_SimulateFailure.isOn && IsServer && request.ClientNetworkId != NetworkManager.LocalClientId)
             {
                 isTokenValid = false;
             }
 
             if (m_GlobalObjectIdHashOverride != 0 && m_PlayerPrefabOverride && m_PlayerPrefabOverride.isOn)
             {
-                aprovalCallback.Invoke(true, m_GlobalObjectIdHashOverride, isTokenValid, null, null);
+                decision.Approved = isTokenValid;
+                decision.PlayerPrefabHash = m_GlobalObjectIdHashOverride;
+                decision.Position = null;
+                decision.Rotation = null;
+                decision.CreatePlayerObject = true;
             }
             else
             {
-                aprovalCallback.Invoke(true, null, isTokenValid, null, null);
+                decision.Approved = isTokenValid;
+                decision.PlayerPrefabHash = null;
+                decision.Position = null;
+                decision.Rotation = null;
+                decision.CreatePlayerObject = true;
             }
 
-
             if (m_ConnectionMessageToDisplay)
             {
                 if (isTokenValid)
                 {
-                    AddNewMessage($"Client id {clientId} is authorized!");
+                    AddNewMessage($"Client id {request.ClientNetworkId} is authorized!");
                 }
                 else
                 {
-                    AddNewMessage($"Client id {clientId} failed authorization!");
+                    AddNewMessage($"Client id {request.ClientNetworkId} failed authorization!");
                 }
 
                 m_ConnectionMessageToDisplay.gameObject.SetActive(true);
             }
+
+            return decision;
         }
 
         /// <summary>