fix: preventing issues with game code registering multiple ConnectionApprovalCallbacks [MTT-3496] (#1972)

Co-authored-by: Fatih Mar <[email protected]>

Author: jeffreyrainy

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -12,10 +12,13 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Changed
 
+- (API Breaking) `ConnectionApprovalCallback` is no longer an `event` and will not allow more than 1 handler registered at a time. Also, `ConnectionApprovalCallback` is now a `Func<>` taking `ConnectionApprovalRequest` in and returning `ConnectionApprovalResponse` back out (#1972)
+
 ### Removed
 
 ### Fixed
 
+- Fixed issues when multiple `ConnectionApprovalCallback`s were registered (#1972)
 - Fixed endless dialog boxes when adding a NetworkBehaviour to a NetworkManager or vice-versa (#1947)
 
 ## [1.0.0-pre.9] - 2022-05-10

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

@@ -206,12 +206,14 @@ public GameObject GetNetworkPrefabOverride(GameObject gameObject)
         /// <summary>
         /// Gets or sets if the application should be set to run in background
         /// </summary>
-        [HideInInspector] public bool RunInBackground = true;
+        [HideInInspector]
+        public bool RunInBackground = true;
 
         /// <summary>
         /// The log level to use
         /// </summary>
-        [HideInInspector] public LogLevel LogLevel = LogLevel.Normal;
+        [HideInInspector]
+        public LogLevel LogLevel = LogLevel.Normal;
 
         /// <summary>
         /// The singleton instance of the NetworkManager
@@ -358,26 +360,59 @@ 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 Response
         /// </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 ConnectionApprovalResponse
+        {
+            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 Network Id of the client we are about to handle</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 whether or not to allow incoming client connection
         /// </summary>
-        public event Action<byte[], ulong, ConnectionApprovedDelegate> ConnectionApprovalCallback = null;
+        public Func<ConnectionApprovalRequest, ConnectionApprovalResponse> ConnectionApprovalCallback
+        {
+            get => m_ConnectionApprovalCallback;
+            set
+            {
+                if (value != null && value.GetInvocationList().Length > 1)
+                {
+                    throw new InvalidOperationException($"Only one {nameof(ConnectionApprovalCallback)} can be registered at a time.");
+                }
+                else
+                {
+                    m_ConnectionApprovalCallback = value;
+                }
+            }
+        }
 
-        internal void InvokeConnectionApproval(byte[] payload, ulong clientId, ConnectionApprovedDelegate action) => ConnectionApprovalCallback?.Invoke(payload, clientId, action);
+        private Func<ConnectionApprovalRequest, ConnectionApprovalResponse> m_ConnectionApprovalCallback;
 
         /// <summary>
         /// The current NetworkConfig
         /// </summary>
-        [HideInInspector] public NetworkConfig NetworkConfig;
+        [HideInInspector]
+        public NetworkConfig NetworkConfig;
 
         /// <summary>
         /// The current host name we are connected to, used to validate certificate
@@ -970,27 +1005,28 @@ public bool StartHost()
             IsClient = true;
             IsListening = true;
 
-            if (NetworkConfig.ConnectionApproval)
+            if (NetworkConfig.ConnectionApproval && ConnectionApprovalCallback != null)
             {
-                InvokeConnectionApproval(NetworkConfig.ConnectionData, ServerClientId,
-                    (createPlayerObject, playerPrefabHash, approved, position, rotation) =>
+                var response = ConnectionApprovalCallback(new ConnectionApprovalRequest { Payload = NetworkConfig.ConnectionData, ClientNetworkId = ServerClientId });
+                if (!response.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);
-                    });
+                response.Approved = true;
+                HandleConnectionApproval(ServerClientId, response);
             }
             else
             {
-                HandleApproval(ServerClientId, NetworkConfig.PlayerPrefab != null, null, true, null, null);
+                var response = new ConnectionApprovalResponse
+                {
+                    Approved = true,
+                    CreatePlayerObject = NetworkConfig.PlayerPrefab != null
+                };
+                HandleConnectionApproval(ServerClientId, response);
             }
 
             SpawnManager.ServerSpawnSceneObjectsOnStartSweep();
@@ -1818,15 +1854,11 @@ private void SyncTime()
         /// <summary>
         /// 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="ownerClientId">The Network Id of the client being approved</param>
+        /// <param name="response">The response to allow the player in or not, with its parameters</param>
+        internal void HandleConnectionApproval(ulong ownerClientId, ConnectionApprovalResponse response)
         {
-            if (approved)
+            if (response.Approved)
             {
                 // Inform new client it got approved
                 PendingClients.Remove(ownerClientId);
@@ -1836,10 +1868,23 @@ internal void HandleApproval(ulong ownerClientId, bool createPlayerObject, uint?
                 m_ConnectedClientsList.Add(client);
                 m_ConnectedClientIds.Add(client.ClientId);
 
-                if (createPlayerObject)
+                if (response.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(
+                        isSceneObject: false,
+                        response.PlayerPrefabHash ?? NetworkConfig.PlayerPrefab.GetComponent<NetworkObject>().GlobalObjectIdHash,
+                        ownerClientId,
+                        parentNetworkId: null,
+                        networkSceneHandle: null,
+                        response.Position,
+                        response.Rotation);
+                    SpawnManager.SpawnNetworkObjectLocally(
+                        networkObject,
+                        SpawnManager.GetNetworkObjectId(),
+                        sceneObject: false,
+                        playerObject: true,
+                        ownerClientId,
+                        destroyWithScene: false);
 
                     ConnectedClients[ownerClientId].PlayerObject = networkObject;
                 }
@@ -1879,13 +1924,13 @@ internal void HandleApproval(ulong ownerClientId, bool createPlayerObject, uint?
                     InvokeOnClientConnectedCallback(ownerClientId);
                 }
 
-                if (!createPlayerObject || (playerPrefabHash == null && NetworkConfig.PlayerPrefab == null))
+                if (!response.CreatePlayerObject || (response.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, response.PlayerPrefabHash ?? NetworkConfig.PlayerPrefab.GetComponent<NetworkObject>().GlobalObjectIdHash);
             }
             else
             {

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

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

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.ConnectionApprovalCallback = 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.ConnectionApprovalResponse NetworkManagerObject_ConnectionApprovalCallback(NetworkManager.ConnectionApprovalRequest request)
         {
-            var stringGuid = Encoding.UTF8.GetString(connectionData);
+            var response = new NetworkManager.ConnectionApprovalResponse();
+            var stringGuid = Encoding.UTF8.GetString(request.Payload);
             if (m_ValidationToken.ToString() == stringGuid)
             {
                 m_IsValidated = true;
             }
-            callback(false, null, m_IsValidated, null, null);
+
+            response.Approved = m_IsValidated;
+            response.CreatePlayerObject = false;
+            response.Position = null;
+            response.Rotation = null;
+            response.PlayerPrefabHash = null;
+
+            return response;
         }
 
         [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.ConnectionApprovalCallback = 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.ConnectionApprovalResponse ConnectionApprovalCallback(NetworkManager.ConnectionApprovalRequest request)
         {
-            string approvalToken = Encoding.ASCII.GetString(dataToken);
+            var response = new NetworkManager.ConnectionApprovalResponse();
+            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);
+                response.Approved = isTokenValid;
+                response.PlayerPrefabHash = m_GlobalObjectIdHashOverride;
+                response.Position = null;
+                response.Rotation = null;
+                response.CreatePlayerObject = true;
             }
             else
             {
-                aprovalCallback.Invoke(true, null, isTokenValid, null, null);
+                response.Approved = isTokenValid;
+                response.PlayerPrefabHash = null;
+                response.Position = null;
+                response.Rotation = null;
+                response.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 response;
         }
 
         /// <summary>