fix: Adding exception for silent failure for clients getting other player's object #844Merge pull request #844 from Unity-Technologies/feature/adding-exception-for-client-side-player-object-get

SamuelBellomo · web-flow · commit 4b15869f5c08 · 2021-05-21T16:01:02.000-04:00
fix: Adding exception for silent failure for clients getting other player's object. Accessing other player objects should only be available from the server. clients should only be able to access their own players.
test: adding tests for spawn manager
diff --git a/com.unity.multiplayer.mlapi/Runtime/Core/NetworkManager.cs b/com.unity.multiplayer.mlapi/Runtime/Core/NetworkManager.cs
@@ -599,6 +599,12 @@ public void StopServer()
             var disconnectedIds = new HashSet<ulong>();
             //Don't know if I have to disconnect the clients. I'm assuming the NetworkTransport does all the cleaning on shtudown. But this way the clients get a disconnect message from server (so long it does't get lost)
 
+            // make sure all RPCs are flushed before transport disconnect clients
+            if (RpcQueueContainer != null)
+            {
+                RpcQueueContainer.ProcessAndFlushRpcQueue(queueType: RpcQueueContainer.RpcQueueProcessingTypes.Send, NetworkUpdateStage.PostLateUpdate); // flushing messages in case transport's disconnect
+            }
+
             foreach (KeyValuePair<ulong, NetworkClient> pair in ConnectedClients)
             {
                 if (!disconnectedIds.Contains(pair.Key))
diff --git a/com.unity.multiplayer.mlapi/Runtime/Spawning/NetworkSpawnManager.cs b/com.unity.multiplayer.mlapi/Runtime/Spawning/NetworkSpawnManager.cs
@@ -68,11 +68,15 @@ public NetworkObject GetLocalPlayerObject()
         }
 
         /// <summary>
-        /// Returns the player object with a given clientId or null if one does not exist
+        /// Returns the player object with a given clientId or null if one does not exist. This is only valid server side.
         /// </summary>
         /// <returns>The player object with a given clientId or null if one does not exist</returns>
         public NetworkObject GetPlayerNetworkObject(ulong clientId)
         {
+            if (!NetworkManager.IsServer && NetworkManager.LocalClientId != clientId)
+            {
+                throw new NotServerException("Only the server can find player objects from other clients.");
+            }
             if (NetworkManager.ConnectedClients.TryGetValue(clientId, out NetworkClient networkClient))
             {
                 return networkClient.PlayerObject;
diff --git a/com.unity.multiplayer.mlapi/Tests/Runtime/MultiInstance/MultiInstanceHelpers.cs b/com.unity.multiplayer.mlapi/Tests/Runtime/MultiInstance/MultiInstanceHelpers.cs
@@ -26,17 +26,20 @@ public static class MultiInstanceHelpers
         /// <param name="clients">The clients NetworkManagers</param>
         public static bool Create(int clientCount, out NetworkManager server, out NetworkManager[] clients)
         {
-            clients = new NetworkManager[clientCount];
+            NetworkManagerInstances = new List<NetworkManager>();
+
+            CreateNewClients(clientCount, out clients);
 
-            for (int i = 0; i < clientCount; i++)
             {
                 // Create gameObject
-                var go = new GameObject("NetworkManager - Client - " + i);
+                var go = new GameObject("NetworkManager - Server");
+
                 // Create networkManager component
-                clients[i] = go.AddComponent<NetworkManager>();
+                server = go.AddComponent<NetworkManager>();
+                NetworkManagerInstances.Insert(0, server);
 
                 // Set the NetworkConfig
-                clients[i].NetworkConfig = new NetworkConfig()
+                server.NetworkConfig = new NetworkConfig()
                 {
                     // Set the current scene to prevent unexpected log messages which would trigger a failure
                     RegisteredScenes = new List<string>() { SceneManager.GetActiveScene().name },
@@ -45,18 +48,28 @@ public static bool Create(int clientCount, out NetworkManager server, out Networ
                 };
             }
 
-            NetworkManagerInstances = new List<NetworkManager>(clients);
+            return true;
+        }
+
+        /// <summary>
+        /// Used to add a client to the already existing list of clients
+        /// </summary>
+        /// <param name="clientCount">The amount of clients</param>
+        /// <param name="clients"></param>
+        /// <returns></returns>
+        public static bool CreateNewClients(int clientCount, out NetworkManager[] clients)
+        {
+            clients = new NetworkManager[clientCount];
 
+            for (int i = 0; i < clientCount; i++)
             {
                 // Create gameObject
-                var go = new GameObject("NetworkManager - Server");
-
+                var go = new GameObject("NetworkManager - Client - " + i);
                 // Create networkManager component
-                server = go.AddComponent<NetworkManager>();
-                NetworkManagerInstances.Insert(0, server);
+                clients[i] = go.AddComponent<NetworkManager>();
 
                 // Set the NetworkConfig
-                server.NetworkConfig = new NetworkConfig()
+                clients[i].NetworkConfig = new NetworkConfig()
                 {
                     // Set the current scene to prevent unexpected log messages which would trigger a failure
                     RegisteredScenes = new List<string>() { SceneManager.GetActiveScene().name },
@@ -65,9 +78,21 @@ public static bool Create(int clientCount, out NetworkManager server, out Networ
                 };
             }
 
+            NetworkManagerInstances.AddRange(clients);
             return true;
         }
 
+        /// <summary>
+        /// Stops one single client and makes sure to cleanup any static variables in this helper
+        /// </summary>
+        /// <param name="clientToStop"></param>
+        public static void StopOneClient(NetworkManager clientToStop)
+        {
+            clientToStop.StopClient();
+            Object.Destroy(clientToStop.gameObject);
+            NetworkManagerInstances.Remove(clientToStop);
+        }
+
         /// <summary>
         /// Should always be invoked when finished with a single unit test
         /// (i.e. during TearDown)
@@ -112,7 +137,7 @@ public static bool Start(bool host, NetworkManager server, NetworkManager[] clie
             }
             else
             {
-                server.StartClient();
+                server.StartServer();
             }
 
             for (int i = 0; i < clients.Length; i++)
diff --git a/com.unity.multiplayer.mlapi/Tests/Runtime/NetworkSpawnManagerTests.cs b/com.unity.multiplayer.mlapi/Tests/Runtime/NetworkSpawnManagerTests.cs
@@ -0,0 +1,201 @@
+using System;
+using System.Collections;
+using MLAPI.Exceptions;
+using NUnit.Framework;
+using UnityEngine;
+using UnityEngine.TestTools;
+using Object = System.Object;
+
+namespace MLAPI.RuntimeTests
+{
+    public class NetworkSpawnManagerTests
+    {
+        private NetworkManager m_ServerNetworkManager;
+        private NetworkManager[] m_ClientNetworkManagers;
+        private GameObject m_PlayerPrefab;
+        private int m_OriginalTargetFrameRate;
+
+        private ulong serverSideClientId => m_ServerNetworkManager.ServerClientId;
+        private ulong clientSideClientId => m_ClientNetworkManagers[0].LocalClientId;
+        private ulong otherClientSideClientId => m_ClientNetworkManagers[1].LocalClientId;
+
+        [UnitySetUp]
+        public IEnumerator Setup()
+        {
+            // Just always track the current target frame rate (will be re-applied upon TearDown)
+            m_OriginalTargetFrameRate = Application.targetFrameRate;
+
+            // Since we use frame count as a metric, we need to assure it runs at a "common update rate"
+            // between platforms (i.e. Ubuntu seems to run at much higher FPS when set to -1)
+            if (Application.targetFrameRate < 0 || Application.targetFrameRate > 120)
+            {
+                Application.targetFrameRate = 120;
+            }
+
+            // Create multiple NetworkManager instances
+            if (!MultiInstanceHelpers.Create(2, out NetworkManager server, out NetworkManager[] clients))
+            {
+                Debug.LogError("Failed to create instances");
+                Assert.Fail("Failed to create instances");
+            }
+
+            m_ServerNetworkManager = server;
+            m_ClientNetworkManagers = clients;
+
+            // Create playerPrefab
+            m_PlayerPrefab = new GameObject("Player");
+            NetworkObject networkObject = m_PlayerPrefab.AddComponent<NetworkObject>();
+
+            // Make it a prefab
+            MultiInstanceHelpers.MakeNetworkedObjectTestPrefab(networkObject);
+
+            // Set the player prefab
+            server.NetworkConfig.PlayerPrefab = m_PlayerPrefab;
+
+            for (int i = 0; i < clients.Length; i++)
+            {
+                clients[i].NetworkConfig.PlayerPrefab = m_PlayerPrefab;
+            }
+
+            // Start the instances
+            if (!MultiInstanceHelpers.Start(true, server, clients))
+            {
+                Debug.LogError("Failed to start instances");
+                Assert.Fail("Failed to start instances");
+            }
+
+            // Wait for connection on client side
+            for (int i = 0; i < clients.Length; i++)
+            {
+                yield return MultiInstanceHelpers.Run(MultiInstanceHelpers.WaitForClientConnected(clients[i]));
+            }
+
+            // Wait for connection on server side
+            yield return MultiInstanceHelpers.Run(MultiInstanceHelpers.WaitForClientsConnectedToServer(server, clientCount: 3));
+        }
+
+        [Test]
+        public void TestServerCanAccessItsOwnPlayer()
+        {
+            // server can access its own player
+            var serverSideServerPlayerObject = m_ServerNetworkManager.SpawnManager.GetPlayerNetworkObject(serverSideClientId);
+            Assert.NotNull(serverSideServerPlayerObject);
+            Assert.AreEqual(serverSideClientId, serverSideServerPlayerObject.OwnerClientId);
+        }
+
+        [Test]
+        public void TestServerCanAccessOtherPlayers()
+        {
+            // server can access other players
+            var serverSideClientPlayerObject = m_ServerNetworkManager.SpawnManager.GetPlayerNetworkObject(clientSideClientId);
+            Assert.NotNull(serverSideClientPlayerObject);
+            Assert.AreEqual(clientSideClientId, serverSideClientPlayerObject.OwnerClientId);
+
+            var serverSideOtherClientPlayerObject = m_ServerNetworkManager.SpawnManager.GetPlayerNetworkObject(otherClientSideClientId);
+            Assert.NotNull(serverSideOtherClientPlayerObject);
+            Assert.AreEqual(otherClientSideClientId, serverSideOtherClientPlayerObject.OwnerClientId);
+        }
+
+        [Test]
+        public void TestClientCantAccessServerPlayer()
+        {
+            // client can't access server player
+            Assert.Throws<NotServerException>(() =>
+            {
+                m_ClientNetworkManagers[0].SpawnManager.GetPlayerNetworkObject(serverSideClientId);
+            });
+        }
+
+        [Test]
+        public void TestClientCanAccessOwnPlayer()
+        {
+            // client can access own player
+            var clientSideClientPlayerObject = m_ClientNetworkManagers[0].SpawnManager.GetPlayerNetworkObject(clientSideClientId);
+            Assert.NotNull(clientSideClientPlayerObject);
+            Assert.AreEqual(clientSideClientId, clientSideClientPlayerObject.OwnerClientId);
+        }
+
+        [Test]
+        public void TestClientCantAccessOtherPlayer()
+        {
+            // client can't access other player
+            Assert.Throws<NotServerException>(() =>
+            {
+                m_ClientNetworkManagers[0].SpawnManager.GetPlayerNetworkObject(otherClientSideClientId);
+            });
+        }
+
+        [Test]
+        public void TestServerGetsNullValueIfInvalidId()
+        {
+            // server gets null value if invalid id
+            var nullPlayer = m_ServerNetworkManager.SpawnManager.GetPlayerNetworkObject(9999);
+            Assert.Null(nullPlayer);
+        }
+
+        [Test]
+        public void TestServerCanUseGetLocalPlayerObject()
+        {
+            // test server can use GetLocalPlayerObject
+            var serverSideServerPlayerObject = m_ServerNetworkManager.SpawnManager.GetLocalPlayerObject();
+            Assert.NotNull(serverSideServerPlayerObject);
+            Assert.AreEqual(serverSideClientId, serverSideServerPlayerObject.OwnerClientId);
+        }
+
+        [Test]
+        public void TestClientCanUseGetLocalPlayerObject()
+        {
+            // test client can use GetLocalPlayerObject
+            var clientSideClientPlayerObject = m_ClientNetworkManagers[0].SpawnManager.GetLocalPlayerObject();
+            Assert.NotNull(clientSideClientPlayerObject);
+            Assert.AreEqual(clientSideClientId, clientSideClientPlayerObject.OwnerClientId);
+        }
+
+        [UnityTest]
+        public IEnumerator TestConnectAndDisconnect()
+        {
+            // test when client connects, player object is now available
+
+            // connect new client
+            if (!MultiInstanceHelpers.CreateNewClients(1, out NetworkManager[] clients))
+            {
+                Debug.LogError("Failed to create instances");
+                Assert.Fail("Failed to create instances");
+            }
+            var newClientNetworkManager = clients[0];
+            newClientNetworkManager.NetworkConfig.PlayerPrefab = m_PlayerPrefab;
+            newClientNetworkManager.StartClient();
+            yield return MultiInstanceHelpers.Run(MultiInstanceHelpers.WaitForClientConnected(newClientNetworkManager));
+            yield return MultiInstanceHelpers.Run(MultiInstanceHelpers.WaitForCondition(() => m_ServerNetworkManager.ConnectedClients.ContainsKey(newClientNetworkManager.LocalClientId)));
+            var newClientLocalClientId = newClientNetworkManager.LocalClientId;
+
+            // test new client can get that itself locally
+            var newPlayerObject = newClientNetworkManager.SpawnManager.GetLocalPlayerObject();
+            Assert.NotNull(newPlayerObject);
+            Assert.AreEqual(newClientLocalClientId, newPlayerObject.OwnerClientId);
+            // test server can get that new client locally
+            var serverSideNewClientPlayer = m_ServerNetworkManager.SpawnManager.GetPlayerNetworkObject(newClientLocalClientId);
+            Assert.NotNull(serverSideNewClientPlayer);
+            Assert.AreEqual(newClientLocalClientId, serverSideNewClientPlayer.OwnerClientId);
+
+            // test when client disconnects, player object no longer available.
+            var nbConnectedClients = m_ServerNetworkManager.ConnectedClients.Count;
+            MultiInstanceHelpers.StopOneClient(newClientNetworkManager);
+            yield return MultiInstanceHelpers.Run(MultiInstanceHelpers.WaitForCondition(() => m_ServerNetworkManager.ConnectedClients.Count == nbConnectedClients - 1));
+            serverSideNewClientPlayer = m_ServerNetworkManager.SpawnManager.GetPlayerNetworkObject(newClientLocalClientId);
+            Assert.Null(serverSideNewClientPlayer);
+        }
+
+        [UnityTearDown]
+        public IEnumerator Teardown()
+        {
+            // Shutdown and clean up both of our NetworkManager instances
+            MultiInstanceHelpers.Destroy();
+            UnityEngine.Object.Destroy(m_PlayerPrefab);
+
+            // Set the application's target frame rate back to its original value
+            Application.targetFrameRate = m_OriginalTargetFrameRate;
+            yield return new WaitForSeconds(0); // wait for next frame so everything is destroyed, so following tests can execute from clean environment
+        }
+    }
+}
diff --git a/com.unity.multiplayer.mlapi/Tests/Runtime/NetworkSpawnManagerTests.cs.meta b/com.unity.multiplayer.mlapi/Tests/Runtime/NetworkSpawnManagerTests.cs.meta
diff --git a/com.unity.multiplayer.mlapi/Tests/Runtime/Transport/SIPTransport.cs b/com.unity.multiplayer.mlapi/Tests/Runtime/Transport/SIPTransport.cs

Original file line number	Diff line number	Diff line change
`@@ -68,11 +68,15 @@ public NetworkObject GetLocalPlayerObject()`
`68`	`68`	`}`
`69`	`69`
`70`	`70`	`/// <summary>`
`71`		`- /// Returns the player object with a given clientId or null if one does not exist`
	`71`	`+ /// Returns the player object with a given clientId or null if one does not exist. This is only valid server side.`
`72`	`72`	`/// </summary>`
`73`	`73`	`/// <returns>The player object with a given clientId or null if one does not exist</returns>`
`74`	`74`	`public NetworkObject GetPlayerNetworkObject(ulong clientId)`
`75`	`75`	`{`
	`76`	`+ if (!NetworkManager.IsServer && NetworkManager.LocalClientId != clientId)`
	`77`	`+ {`
	`78`	`+ throw new NotServerException("Only the server can find player objects from other clients.");`
	`79`	`+ }`
`76`	`80`	`if (NetworkManager.ConnectedClients.TryGetValue(clientId, out NetworkClient networkClient))`
`77`	`81`	`{`
`78`	`82`	`return networkClient.PlayerObject;`