feat: flag to prevent dev and release builds to connect (#482)

* Added flag preventing release and debug build from connecting and appropriate message to the user when it happens

* Simplified connection approval flow

Author: LPLafontaineB

Assets/BossRoom/Scripts/Client/UI/ConnectionStatusMessageUIManager.cs

@@ -42,8 +42,11 @@ void OnConnectStatus(ConnectStatus status)
                 case ConnectStatus.LoggedInAgain:
                     PopupPanel.ShowPopupPanel("Connection Failed", "You have logged in elsewhere using the same account.");
                     break;
+                case ConnectStatus.IncompatibleBuildType:
+                    PopupPanel.ShowPopupPanel("Connection Failed", "Server and client builds are not compatible. You cannot connect a release build to a development build or an in-editor session.");
+                    break;
                 case ConnectStatus.GenericDisconnect:
-                    PopupPanel.ShowPopupPanel("Disconnected From Host", "The connection to the host was lost");
+                    PopupPanel.ShowPopupPanel("Disconnected From Host", "The connection to the host was lost.");
                     break;
                 default:
                     Debug.LogWarning($"New ConnectStatus {status} has been added, but no connect message defined for it.");

Assets/BossRoom/Scripts/Shared/Net/ConnectionManagement/ClientGameNetPortal.cs

@@ -222,7 +222,8 @@ void ConnectClient()
             {
                 playerId = m_Portal.GetPlayerId(),
                 clientScene = SceneManager.GetActiveScene().buildIndex,
-                playerName = m_Portal.PlayerName
+                playerName = m_Portal.PlayerName,
+                isDebug = Debug.isDebugBuild
             });
 
             var payloadBytes = System.Text.Encoding.UTF8.GetBytes(payload);

Assets/BossRoom/Scripts/Shared/Net/ConnectionManagement/GameNetPortal.cs

@@ -20,6 +20,7 @@ public enum ConnectStatus
         LoggedInAgain,            //logged in on a separate client, causing this one to be kicked out.
         UserRequestedDisconnect,  //Intentional Disconnect triggered by the user.
         GenericDisconnect,        //server disconnected, but no specific reason given.
+        IncompatibleBuildType,      //client build type is incompatible with server.
     }
 
     public enum OnlineMode
@@ -36,6 +37,7 @@ public class ConnectionPayload
         public string playerId;
         public int clientScene = -1;
         public string playerName;
+        public bool isDebug;
     }
 
     /// <summary>

Assets/BossRoom/Scripts/Shared/Net/ConnectionManagement/ServerGameNetPortal.cs

@@ -31,7 +31,7 @@ public class ServerGameNetPortal : MonoBehaviour
         /// <summary>
         /// The active server scene index.
         /// </summary>
-        public int ServerScene { get { return UnityEngine.SceneManagement.SceneManager.GetActiveScene().buildIndex; } }
+        static int ServerScene => SceneManager.GetActiveScene().buildIndex;
 
         LobbyServiceFacade m_LobbyServiceFacade;
 
@@ -164,6 +164,8 @@ void ApprovalCheck(byte[] connectionData, ulong clientId, NetworkManager.Connect
         {
             if (connectionData.Length > k_MaxConnectPayload)
             {
+                // If connectionData too high, deny immediately to avoid wasting time on the server. This is intended as
+                // a bit of light protection against DOS attacks that rely on sending silly big buffers of garbage.
                 connectionApprovedCallback(false, 0, false, null, null);
                 return;
             }
@@ -178,67 +180,66 @@ void ApprovalCheck(byte[] connectionData, ulong clientId, NetworkManager.Connect
                 return;
             }
 
-            string payload = System.Text.Encoding.UTF8.GetString(connectionData);
+            var payload = System.Text.Encoding.UTF8.GetString(connectionData);
             var connectionPayload = JsonUtility.FromJson<ConnectionPayload>(payload); // https://docs.unity3d.com/2020.2/Documentation/Manual/JSONSerialization.html
-
-            ConnectStatus gameReturnStatus;
-
-            // Test for over-capacity connection. This needs to be done asap, to make sure we refuse connections asap and don't spend useless time server side
-            // on invalid users trying to connect
-            // todo this is currently still spending too much time server side.
-            if (m_Portal.NetManager.ConnectedClientsIds.Count >= CharSelectData.k_MaxLobbyPlayers)
-            {
-                gameReturnStatus = ConnectStatus.ServerFull;
-            }
-            else
-            {
-                Debug.Log("Host ApprovalCheck: connecting client with player ID: " + connectionPayload.playerId);
-
-                gameReturnStatus = SessionManager<SessionPlayerData>.Instance.SetupConnectingPlayerSessionData(clientId, connectionPayload.playerId,
-                    new SessionPlayerData(clientId, connectionPayload.playerName, m_Portal.AvatarRegistry.GetRandomAvatar().Guid.ToNetworkGuid(), 0, true))
-                    ? ConnectStatus.Success
-                    : ConnectStatus.LoggedInAgain;
-            }
+            var gameReturnStatus = GetConnectStatus(connectionPayload);
 
             if (gameReturnStatus == ConnectStatus.Success)
             {
-                int clientScene = connectionPayload.clientScene;
+                SessionManager<SessionPlayerData>.Instance.SetupConnectingPlayerSessionData(clientId, connectionPayload.playerId,
+                    new SessionPlayerData(clientId, connectionPayload.playerName, m_Portal.AvatarRegistry.GetRandomAvatar().Guid.ToNetworkGuid(), 0, true));
                 SendServerToClientConnectResult(clientId, gameReturnStatus);
 
-                //Populate our dictionaries with the playerData
-                m_ClientSceneMap[clientId] = clientScene;
+                //Populate our client scene map
+                m_ClientSceneMap[clientId] = connectionPayload.clientScene;
 
                 connectionApprovedCallback(true, null, true, Vector3.zero, Quaternion.identity);
                 // connection approval will create a player object for you
             }
             else
             {
                 //TODO-FIXME:Netcode Issue #796. We should be able to send a reason and disconnect without a coroutine delay.
-                //TODO:Netcode: In the future we expect Netcode to allow us to return more information as part of
-                //the approval callback, so that we can provide more context on a reject. In the meantime we must provide the extra information ourselves,
-                //and then manually close down the connection.
+                //TODO:Netcode: In the future we expect Netcode to allow us to return more information as part of the
+                //approval callback, so that we can provide more context on a reject. In the meantime we must provide
+                //the extra information ourselves, and then wait a short time before manually close down the connection.
                 SendServerToClientConnectResult(clientId, gameReturnStatus);
                 SendServerToClientSetDisconnectReason(clientId, gameReturnStatus);
-                StartCoroutine(WaitToDisconnect(clientId));
+                StartCoroutine(WaitToDenyApproval(connectionApprovedCallback));
                 if (m_LobbyServiceFacade.CurrentUnityLobby != null)
                 {
                     m_LobbyServiceFacade.RemovePlayerFromLobbyAsync(connectionPayload.playerId, m_LobbyServiceFacade.CurrentUnityLobby.Id, null, null);
                 }
             }
         }
 
-        IEnumerator WaitToDisconnect(ulong clientId)
+        ConnectStatus GetConnectStatus(ConnectionPayload connectionPayload)
+        {
+            if (m_Portal.NetManager.ConnectedClientsIds.Count >= CharSelectData.k_MaxLobbyPlayers)
+            {
+                return ConnectStatus.ServerFull;
+            }
+
+            if (connectionPayload.isDebug != Debug.isDebugBuild)
+            {
+                return ConnectStatus.IncompatibleBuildType;
+            }
+
+            return SessionManager<SessionPlayerData>.Instance.IsDuplicateConnection(connectionPayload.playerId) ?
+                ConnectStatus.LoggedInAgain : ConnectStatus.Success;
+        }
+
+        static IEnumerator WaitToDenyApproval(NetworkManager.ConnectionApprovedDelegate connectionApprovedCallback)
         {
             yield return new WaitForSeconds(0.5f);
-            m_Portal.NetManager.DisconnectClient(clientId);
+            connectionApprovedCallback(false, 0, false, null, null);
         }
 
         /// <summary>
         /// Sends a DisconnectReason to the indicated client. This should only be done on the server, prior to disconnecting the client.
         /// </summary>
         /// <param name="clientID"> id of the client to send to </param>
         /// <param name="status"> The reason for the upcoming disconnect.</param>
-        public void SendServerToClientSetDisconnectReason(ulong clientID, ConnectStatus status)
+        static void SendServerToClientSetDisconnectReason(ulong clientID, ConnectStatus status)
         {
             var writer = new FastBufferWriter(sizeof(ConnectStatus), Allocator.Temp);
             writer.WriteValueSafe(status);
@@ -250,7 +251,7 @@ public void SendServerToClientSetDisconnectReason(ulong clientID, ConnectStatus
         /// </summary>
         /// <param name="clientID"> id of the client to send to </param>
         /// <param name="status"> the status to pass to the client</param>
-        public void SendServerToClientConnectResult(ulong clientID, ConnectStatus status)
+        static void SendServerToClientConnectResult(ulong clientID, ConnectStatus status)
         {
             var writer = new FastBufferWriter(sizeof(ConnectStatus), Allocator.Temp);
             writer.WriteValueSafe(status);

Packages/com.unity.multiplayer.samples.coop/Utilities/Net/SessionManager.cs

@@ -101,50 +101,51 @@ void Clear()
             m_ClientIDToPlayerId.Clear();
         }
 
+        public bool IsDuplicateConnection(string playerId)
+        {
+            return m_ClientData.ContainsKey(playerId) && m_ClientData[playerId].IsConnected;
+        }
+
         /// <summary>
-        /// Adds a connecting player's session data if it is a new connection, or updates their session data in case of a reconnection. If the connection is not valid, simply returns false.
+        /// Adds a connecting player's session data if it is a new connection, or updates their session data in case of a reconnection.
         /// </summary>
         /// <param name="clientId">This is the clientId that Netcode assigned us on login. It does not persist across multiple logins from the same client. </param>
         /// <param name="playerId">This is the playerId that is unique to this client and persists across multiple logins from the same client</param>
         /// <param name="sessionPlayerData">The player's initial data</param>
-        /// <returns>True if the player connection is valid (i.e. not a duplicate connection)</returns>
-        public bool SetupConnectingPlayerSessionData(ulong clientId, string playerId, T sessionPlayerData)
+        public void SetupConnectingPlayerSessionData(ulong clientId, string playerId, T sessionPlayerData)
         {
-            bool success = true;
+            var isReconnecting = false;
 
-            //Test for Duplicate Login.
-            if (m_ClientData.ContainsKey(playerId))
+            // Test for duplicate connection
+            if (IsDuplicateConnection(playerId))
             {
-                bool isReconnecting = false;
+                Debug.LogError($"Player ID {playerId} already exists. This is a duplicate connection. Rejecting this session data.");
+                return;
+            }
 
-                // If another client is connected with the same playerId
-                if (m_ClientData[playerId].IsConnected)
-                {
-                    success = false;
-                }
-                else
+            // If another client exists with the same playerId
+            if (m_ClientData.ContainsKey(playerId))
+            {
+                if (!m_ClientData[playerId].IsConnected)
                 {
+                    // If this connecting client has the same player Id as a disconnected client, this is a reconnection.
                     isReconnecting = true;
                 }
 
-                // Reconnecting. Give data from old player to new player
-                if (isReconnecting)
-                {
-                    // Update player session data
-                    sessionPlayerData = m_ClientData[playerId];
-                    sessionPlayerData.ClientID = clientId;
-                    sessionPlayerData.IsConnected = true;
-                }
             }
 
-            //Populate our dictionaries with the SessionPlayerData
-            if (success)
+            // Reconnecting. Give data from old player to new player
+            if (isReconnecting)
             {
-                m_ClientIDToPlayerId[clientId] = playerId;
-                m_ClientData[playerId] = sessionPlayerData;
+                // Update player session data
+                sessionPlayerData = m_ClientData[playerId];
+                sessionPlayerData.ClientID = clientId;
+                sessionPlayerData.IsConnected = true;
             }
 
-            return success;
+            //Populate our dictionaries with the SessionPlayerData
+            m_ClientIDToPlayerId[clientId] = playerId;
+            m_ClientData[playerId] = sessionPlayerData;
         }
 
         public string GetPlayerId(ulong clientId)