feat!: network variables - client auth, permission cleanup, containers (#1074)

* refactor!: convert NetworkTransform.NetworkState to `struct`
* feat!: remove client network permissions [MTT-1019]
* NetworkList is now a derived class
* de-interface
* removed valueRef
* added host tests
* updated NetworkVarBufferCopyTest
* container cleanup
* channel cleanup
* fixed client perm, more perm tests
* INetworkVariable -> NetworkVariableBase
* remove NetworkVariableBase.SetNetworkBehaviour()
* fix tools project tests

Co-authored-by: M. Fatih MAR <mfatihmar@gmail.com>

Author: mattwalsh-unity

com.unity.netcode.gameobjects/Editor/NetworkBehaviourEditor.cs

@@ -58,20 +58,15 @@ private void RenderNetworkVariable(int index)
             if (value == null)
             {
                 var fieldType = m_NetworkVariableFields[m_NetworkVariableNames[index]].FieldType;
-                var networkVariable = (INetworkVariable)Activator.CreateInstance(fieldType, true);
+                var networkVariable = (NetworkVariableBase)Activator.CreateInstance(fieldType, true);
                 m_NetworkVariableFields[m_NetworkVariableNames[index]].SetValue(target, networkVariable);
             }
 
             var type = m_NetworkVariableFields[m_NetworkVariableNames[index]].GetValue(target).GetType();
             var genericType = type.GetGenericArguments()[0];
 
             EditorGUILayout.BeginHorizontal();
-            if (genericType == typeof(string))
-            {
-                var networkVariable = (NetworkVariable<string>)m_NetworkVariableFields[m_NetworkVariableNames[index]].GetValue(target);
-                networkVariable.Value = EditorGUILayout.TextField(m_NetworkVariableNames[index], networkVariable.Value);
-            }
-            else if (genericType.IsValueType)
+            if (genericType.IsValueType)
             {
                 var method = typeof(NetworkBehaviourEditor).GetMethod("RenderNetworkVariableValueType", BindingFlags.Public | BindingFlags.Instance | BindingFlags.FlattenHierarchy | BindingFlags.NonPublic);
                 var genericMethod = method.MakeGenericMethod(genericType);
@@ -86,7 +81,7 @@ private void RenderNetworkVariable(int index)
             EditorGUILayout.EndHorizontal();
         }
 
-        private void RenderNetworkVariableValueType<T>(int index) where T : struct
+        private void RenderNetworkVariableValueType<T>(int index) where T : unmanaged
         {
             var networkVariable = (NetworkVariable<T>)m_NetworkVariableFields[m_NetworkVariableNames[index]].GetValue(target);
             var type = typeof(T);

com.unity.netcode.gameobjects/Prototyping/NetworkTransform.cs

@@ -128,12 +128,6 @@ public void NetworkSerialize(NetworkSerializer serializer)
             }
         }
 
-        /// <summary>
-        /// The network channel to use send updates
-        /// </summary>
-        [Tooltip("The network channel to use send updates")]
-        public NetworkChannel Channel = NetworkChannel.NetworkVariable;
-
         /// <summary>
         /// Sets whether this transform should sync in local space or in world space.
         /// This is important to set since reparenting this transform could have issues,
@@ -156,6 +150,7 @@ public void NetworkSerialize(NetworkSerializer serializer)
         public float FixedSendsPerSecond = 30f;
 
         private Transform m_Transform; // cache the transform component to reduce unnecessary bounce between managed and native
+        internal NetworkState LocalNetworkState;
         internal readonly NetworkVariable<NetworkState> ReplNetworkState = new NetworkVariable<NetworkState>(new NetworkState());
         internal NetworkState PrevNetworkState;
 
@@ -364,10 +359,6 @@ private void OnNetworkStateChanged(NetworkState oldState, NetworkState newState)
         private void Awake()
         {
             m_Transform = transform;
-
-            ReplNetworkState.Settings.SendNetworkChannel = Channel;
-            ReplNetworkState.Settings.SendTickrate = FixedSendsPerSecond;
-
             ReplNetworkState.OnValueChanged += OnNetworkStateChanged;
         }
 
@@ -385,7 +376,13 @@ private void FixedUpdate()
 
             if (IsServer)
             {
-                ReplNetworkState.SetDirty(UpdateNetworkState(ref ReplNetworkState.ValueRef));
+                // try to update local NetworkState
+                if (UpdateNetworkState(ref LocalNetworkState))
+                {
+                    // if updated (dirty), change NetVar, mark it dirty
+                    ReplNetworkState.Value = LocalNetworkState;
+                    ReplNetworkState.SetDirty(true);
+                }
             }
             // try to update previously consumed NetworkState
             // if we have any changes, that means made some updates locally

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

@@ -366,7 +366,7 @@ public virtual void OnNetworkObjectParentChanged(NetworkObject parentNetworkObje
 
         private readonly List<HashSet<int>> m_ChannelMappedNetworkVariableIndexes = new List<HashSet<int>>();
         private readonly List<NetworkChannel> m_ChannelsForNetworkVariableGroups = new List<NetworkChannel>();
-        internal readonly List<INetworkVariable> NetworkVariableFields = new List<INetworkVariable>();
+        internal readonly List<NetworkVariableBase> NetworkVariableFields = new List<NetworkVariableBase>();
 
         private static Dictionary<Type, FieldInfo[]> s_FieldTypes = new Dictionary<Type, FieldInfo[]>();
 
@@ -415,19 +415,19 @@ internal void InitializeVariables()
             {
                 Type fieldType = sortedFields[i].FieldType;
 
-                if (fieldType.HasInterface(typeof(INetworkVariable)))
+                if (fieldType.IsSubclassOf(typeof(NetworkVariableBase)))
                 {
-                    var instance = (INetworkVariable)sortedFields[i].GetValue(this);
+                    var instance = (NetworkVariableBase)sortedFields[i].GetValue(this);
 
                     if (instance == null)
                     {
-                        instance = (INetworkVariable)Activator.CreateInstance(fieldType, true);
+                        instance = (NetworkVariableBase)Activator.CreateInstance(fieldType, true);
                         sortedFields[i].SetValue(this, instance);
                     }
 
-                    instance.SetNetworkBehaviour(this);
+                    instance.NetworkBehaviour = this;
 
-                    var instanceNameProperty = fieldType.GetProperty(nameof(INetworkVariable.Name));
+                    var instanceNameProperty = fieldType.GetProperty(nameof(NetworkVariableBase.Name));
                     var sanitizedVariableName = sortedFields[i].Name.Replace("<", string.Empty).Replace(">k__BackingField", string.Empty);
                     instanceNameProperty?.SetValue(instance, sanitizedVariableName);
 
@@ -442,7 +442,7 @@ internal void InitializeVariables()
 
                 for (int i = 0; i < NetworkVariableFields.Count; i++)
                 {
-                    NetworkChannel networkChannel = NetworkVariableFields[i].GetChannel();
+                    var networkChannel = NetworkVariableBase.NetworkVariableChannel;
 
                     if (!firstLevelIndex.ContainsKey(networkChannel))
                     {
@@ -514,6 +514,7 @@ private void NetworkVariableUpdate(ulong clientId, int behaviourIndex)
                     {
                         using (var writer = PooledNetworkWriter.Get(buffer))
                         {
+                            // TODO: could skip this if no variables dirty, though obsolete w/ Snapshot
                             writer.WriteUInt64Packed(NetworkObjectId);
                             writer.WriteUInt16Packed(NetworkObject.GetNetworkBehaviourOrderIndex(this));
 
@@ -537,14 +538,9 @@ private void NetworkVariableUpdate(ulong clientId, int behaviourIndex)
                                     continue;
                                 }
 
-                                bool isDirty =
-                                    NetworkVariableFields[k]
-                                        .IsDirty(); // cache this here. You never know what operations users will do in the dirty methods
-
                                 //   if I'm dirty AND a client, write (server always has all permissions)
                                 //   if I'm dirty AND the server AND the client can read me, send.
-                                bool shouldWrite = isDirty &&
-                                                   (!IsServer || NetworkVariableFields[k].CanClientRead(clientId));
+                                bool shouldWrite = NetworkVariableFields[k].ShouldWrite(clientId, IsServer);
 
                                 if (NetworkManager.NetworkConfig.EnsureNetworkVariableLengthSafety)
                                 {
@@ -628,7 +624,7 @@ private bool CouldHaveDirtyNetworkVariables()
             return false;
         }
 
-        internal static void HandleNetworkVariableDeltas(List<INetworkVariable> networkVariableList, Stream stream, ulong clientId, NetworkBehaviour logInstance, NetworkManager networkManager)
+        internal static void HandleNetworkVariableDeltas(List<NetworkVariableBase> networkVariableList, Stream stream, ulong clientId, NetworkBehaviour logInstance, NetworkManager networkManager)
         {
             using (var reader = PooledNetworkReader.Get(stream))
             {
@@ -655,6 +651,7 @@ internal static void HandleNetworkVariableDeltas(List<INetworkVariable> networkV
 
                     if (networkManager.IsServer && !networkVariableList[i].CanClientWrite(clientId))
                     {
+                        // we are choosing not to fire an exception here, because otherwise a malicious client could use this to crash the server
                         if (networkManager.NetworkConfig.EnsureNetworkVariableLengthSafety)
                         {
                             if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
@@ -674,7 +671,6 @@ internal static void HandleNetworkVariableDeltas(List<INetworkVariable> networkV
                         //A dummy read COULD be added to the interface for this situation, but it's just being too nice.
                         //This is after all a developer fault. A critical error should be fine.
                         // - TwoTen
-
                         if (NetworkLog.CurrentLogLevel <= LogLevel.Error)
                         {
                             NetworkLog.LogError($"Client wrote to {typeof(NetworkVariable<>).Name} without permission. No more variables can be read. This is critical. => {(logInstance != null ? ($"{nameof(NetworkObjectId)}: {logInstance.NetworkObjectId} - {nameof(NetworkObject.GetNetworkBehaviourOrderIndex)}(): {logInstance.NetworkObject.GetNetworkBehaviourOrderIndex(logInstance)} - VariableIndex: {i}") : string.Empty)}");
@@ -683,7 +679,6 @@ internal static void HandleNetworkVariableDeltas(List<INetworkVariable> networkV
 
                         return;
                     }
-
                     long readStartPos = stream.Position;
 
                     networkVariableList[i].ReadDelta(stream, networkManager.IsServer);
@@ -722,95 +717,7 @@ internal static void HandleNetworkVariableDeltas(List<INetworkVariable> networkV
             }
         }
 
-        internal static void HandleNetworkVariableUpdate(List<INetworkVariable> networkVariableList, Stream stream, ulong clientId, NetworkBehaviour logInstance, NetworkManager networkManager)
-        {
-            using (var reader = PooledNetworkReader.Get(stream))
-            {
-                for (int i = 0; i < networkVariableList.Count; i++)
-                {
-                    ushort varSize = 0;
-
-                    if (networkManager.NetworkConfig.EnsureNetworkVariableLengthSafety)
-                    {
-                        varSize = reader.ReadUInt16Packed();
-
-                        if (varSize == 0)
-                        {
-                            continue;
-                        }
-                    }
-                    else
-                    {
-                        if (!reader.ReadBool())
-                        {
-                            continue;
-                        }
-                    }
-
-                    if (networkManager.IsServer && !networkVariableList[i].CanClientWrite(clientId))
-                    {
-                        if (networkManager.NetworkConfig.EnsureNetworkVariableLengthSafety)
-                        {
-                            if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
-                            {
-                                NetworkLog.LogWarning($"Client wrote to {typeof(NetworkVariable<>).Name} without permission. => {(logInstance != null ? ($"{nameof(NetworkObjectId)}: {logInstance.NetworkObjectId} - {nameof(NetworkObject.GetNetworkBehaviourOrderIndex)}(): {logInstance.NetworkObject.GetNetworkBehaviourOrderIndex(logInstance)} - VariableIndex: {i}") : string.Empty)}");
-                            }
-
-                            stream.Position += varSize;
-                            continue;
-                        }
-
-                        //This client wrote somewhere they are not allowed. This is critical
-                        //We can't just skip this field. Because we don't actually know how to dummy read
-                        //That is, we don't know how many bytes to skip. Because the interface doesn't have a
-                        //Read that gives us the value. Only a Read that applies the value straight away
-                        //A dummy read COULD be added to the interface for this situation, but it's just being too nice.
-                        //This is after all a developer fault. A critical error should be fine.
-                        // - TwoTen
-                        if (NetworkLog.CurrentLogLevel <= LogLevel.Error)
-                        {
-                            NetworkLog.LogError($"Client wrote to {typeof(NetworkVariable<>).Name} without permission. No more variables can be read. This is critical. => {(logInstance != null ? ($"{nameof(NetworkObjectId)}: {logInstance.NetworkObjectId} - {nameof(NetworkObject.GetNetworkBehaviourOrderIndex)}(): {logInstance.NetworkObject.GetNetworkBehaviourOrderIndex(logInstance)} - VariableIndex: {i}") : string.Empty)}");
-                        }
-
-                        return;
-                    }
-
-                    long readStartPos = stream.Position;
-
-                    networkVariableList[i].ReadField(stream);
-
-                    if (networkManager.NetworkConfig.EnsureNetworkVariableLengthSafety)
-                    {
-                        if (stream is NetworkBuffer networkBuffer)
-                        {
-                            networkBuffer.SkipPadBits();
-                        }
-
-                        if (stream.Position > (readStartPos + varSize))
-                        {
-                            if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
-                            {
-                                NetworkLog.LogWarning($"Var update read too far. {stream.Position - (readStartPos + varSize)} bytes. => {(logInstance != null ? ($"{nameof(NetworkObjectId)}: {logInstance.NetworkObjectId} - {nameof(NetworkObject.GetNetworkBehaviourOrderIndex)}(): {logInstance.NetworkObject.GetNetworkBehaviourOrderIndex(logInstance)} - VariableIndex: {i}") : string.Empty)}");
-                            }
-
-                            stream.Position = readStartPos + varSize;
-                        }
-                        else if (stream.Position < (readStartPos + varSize))
-                        {
-                            if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
-                            {
-                                NetworkLog.LogWarning($"Var update read too little. {(readStartPos + varSize) - stream.Position} bytes. => {(logInstance != null ? ($"{nameof(NetworkObjectId)}: {logInstance.NetworkObjectId} - {nameof(NetworkObject.GetNetworkBehaviourOrderIndex)}(): {logInstance.NetworkObject.GetNetworkBehaviourOrderIndex(logInstance)} - VariableIndex: {i}") : string.Empty)}");
-                            }
-
-                            stream.Position = readStartPos + varSize;
-                        }
-                    }
-                }
-            }
-        }
-
-
-        internal static void WriteNetworkVariableData(List<INetworkVariable> networkVariableList, Stream stream, ulong clientId, NetworkManager networkManager)
+        internal static void WriteNetworkVariableData(List<NetworkVariableBase> networkVariableList, Stream stream, ulong clientId, NetworkManager networkManager)
         {
             if (networkVariableList.Count == 0)
             {
@@ -858,7 +765,7 @@ internal static void WriteNetworkVariableData(List<INetworkVariable> networkVari
             }
         }
 
-        internal static void SetNetworkVariableData(List<INetworkVariable> networkVariableList, Stream stream, NetworkManager networkManager)
+        internal static void SetNetworkVariableData(List<NetworkVariableBase> networkVariableList, Stream stream, NetworkManager networkManager)
         {
             if (networkVariableList.Count == 0)
             {

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

@@ -50,9 +50,12 @@ internal void NetworkBehaviourUpdate(NetworkManager networkManager)
                     // when client updates the server, it tells it about all its objects
                     foreach (var sobj in networkManager.SpawnManager.SpawnedObjectsList)
                     {
-                        for (int k = 0; k < sobj.ChildNetworkBehaviours.Count; k++)
+                        if (sobj.IsOwner)
                         {
-                            sobj.ChildNetworkBehaviours[k].VariableUpdate(networkManager.ServerClientId);
+                            for (int k = 0; k < sobj.ChildNetworkBehaviours.Count; k++)
+                            {
+                                sobj.ChildNetworkBehaviours[k].VariableUpdate(networkManager.ServerClientId);
+                            }
                         }
                     }
 

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

@@ -558,7 +558,7 @@ internal void ProcessSingleAck(ushort ackSequence, ulong clientId, ClientData cl
         /// This will look into all spawned objects
         /// </summary>
         /// <param name="key">The key to search for</param>
-        private INetworkVariable FindNetworkVar(VariableKey key)
+        private NetworkVariableBase FindNetworkVar(VariableKey key)
         {
             var spawnedObjects = NetworkManager.SpawnManager.SpawnedObjects;
 
@@ -895,7 +895,7 @@ internal void Despawn(SnapshotDespawnCommand command)
         /// Might not happen for all variable on every frame. Might even happen more than once.
         /// </summary>
         /// <param name="networkVariable">The NetworkVariable to write, or rather, its INetworkVariable</param>
-        internal void Store(ulong networkObjectId, int behaviourIndex, int variableIndex, INetworkVariable networkVariable)
+        internal void Store(ulong networkObjectId, int behaviourIndex, int variableIndex, NetworkVariableBase networkVariable)
         {
             VariableKey k;
             k.NetworkObjectId = networkObjectId;
@@ -914,7 +914,7 @@ internal void Store(ulong networkObjectId, int behaviourIndex, int variableIndex
             WriteVariableToSnapshot(m_Snapshot, networkVariable, pos);
         }
 
-        private void WriteVariableToSnapshot(Snapshot snapshot, INetworkVariable networkVariable, int index)
+        private void WriteVariableToSnapshot(Snapshot snapshot, NetworkVariableBase networkVariable, int index)
         {
             // write var into buffer, possibly adjusting entry's position and Length
             using (var varBuffer = PooledNetworkBuffer.Get())