fix: client owner with both owner read and write permissions on a NetworkVariable does not update the server when changed (#2097)

* fix

MTT-4266
This fixes the issue where a NetworkVariable that is set to owner read and write permissions and the owner is a client, then the server would not be updated with any changes to the NetworkVariable.

* test

This includes an additional test to check that a client owner with both Owner read and write permissions will only update changes to the NetworkVariable on the server.

* update

Adding the changelog entry for this fix.

* update

adding PR number.

Author: NoelStephensUnity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -14,7 +14,8 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
-- Fixed issue when attempting to spawn a parent `GameObject`, with `NetworkObject` component attached, that has one or more child `GameObject`s, that are inactive in the hierarchy, with `NetworkBehaviour` components it will no longer attempt to spawn the associated NetworkBehaviour(s) or invoke ownership changed notifications but will log a warning message. (#2096)
+- Fixed issue where a client owner of a `NetworkVariable` with both owner read and write permissions would not update the server side when changed. (#2097)
+- Fixed issue when attempting to spawn a parent `GameObject`, with `NetworkObject` component attached, that has one or more child `GameObject`s, that are inactive in the hierarchy, with `NetworkBehaviour` components it will no longer attempt to spawn the associated `NetworkBehaviour`(s) or invoke ownership changed notifications but will log a warning message. (#2096)
 - Fixed issue where `NetworkObject.NetworkHide` was despawning and destroying, as opposed to only despawning, in-scene placed `NetworkObject`s. (#2086)
 - Fixed issue where `NetworkAnimator` would not synchronize a looping animation for late joining clients if it was at the very end of its loop. (#2076)
 - Fixed issue where `NetworkAnimator` was not removing its subscription from `OnClientConnectedCallback` when despawned during the shutdown sequence. (#2074)

com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariableBase.cs

@@ -111,7 +111,7 @@ public bool CanClientRead(ulong clientId)
                 case NetworkVariableReadPermission.Everyone:
                     return true;
                 case NetworkVariableReadPermission.Owner:
-                    return clientId == m_NetworkBehaviour.NetworkObject.OwnerClientId;
+                    return clientId == m_NetworkBehaviour.NetworkObject.OwnerClientId || NetworkManager.ServerClientId == clientId;
             }
         }
 

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

@@ -14,6 +14,7 @@ public class NetVarPermTestComp : NetworkBehaviour
     {
         public NetworkVariable<Vector3> OwnerWritable_Position = new NetworkVariable<Vector3>(Vector3.one, NetworkVariableBase.DefaultReadPerm, NetworkVariableWritePermission.Owner);
         public NetworkVariable<Vector3> ServerWritable_Position = new NetworkVariable<Vector3>(Vector3.one, NetworkVariableBase.DefaultReadPerm, NetworkVariableWritePermission.Server);
+        public NetworkVariable<Vector3> OwnerReadWrite_Position = new NetworkVariable<Vector3>(Vector3.one, NetworkVariableReadPermission.Owner, NetworkVariableWritePermission.Owner);
     }
 
     [TestFixtureSource(nameof(TestDataSource))]
@@ -104,6 +105,42 @@ private bool CheckServerWritableAreEqualOnAll()
             return true;
         }
 
+        private bool CheckOwnerReadWriteAreEqualOnOwnerAndServer()
+        {
+            var testObjServer = m_ServerNetworkManager.SpawnManager.SpawnedObjects[m_TestObjId];
+            var testCompServer = testObjServer.GetComponent<NetVarPermTestComp>();
+            foreach (var clientNetworkManager in m_ClientNetworkManagers)
+            {
+                var testObjClient = clientNetworkManager.SpawnManager.SpawnedObjects[m_TestObjId];
+                var testCompClient = testObjClient.GetComponent<NetVarPermTestComp>();
+                if (testObjServer.OwnerClientId == testObjClient.OwnerClientId &&
+                    testCompServer.OwnerReadWrite_Position.Value == testCompClient.ServerWritable_Position.Value &&
+                    testCompServer.OwnerReadWrite_Position.ReadPerm == testCompClient.ServerWritable_Position.ReadPerm &&
+                    testCompServer.OwnerReadWrite_Position.WritePerm == testCompClient.ServerWritable_Position.WritePerm)
+                {
+                    return true;
+                }
+            }
+            return false;
+        }
+
+        private bool CheckOwnerReadWriteAreNotEqualOnNonOwnerClients(NetVarPermTestComp ownerReadWriteObject)
+        {
+            foreach (var clientNetworkManager in m_ClientNetworkManagers)
+            {
+                var testObjClient = clientNetworkManager.SpawnManager.SpawnedObjects[m_TestObjId];
+                var testCompClient = testObjClient.GetComponent<NetVarPermTestComp>();
+                if (testObjClient.OwnerClientId != ownerReadWriteObject.OwnerClientId ||
+                    ownerReadWriteObject.OwnerReadWrite_Position.Value == testCompClient.ServerWritable_Position.Value ||
+                    ownerReadWriteObject.OwnerReadWrite_Position.ReadPerm != testCompClient.ServerWritable_Position.ReadPerm ||
+                    ownerReadWriteObject.OwnerReadWrite_Position.WritePerm != testCompClient.ServerWritable_Position.WritePerm)
+                {
+                    return false;
+                }
+            }
+            return true;
+        }
+
         [UnityTest]
         public IEnumerator ServerChangesOwnerWritableNetVar()
         {
@@ -164,6 +201,44 @@ public IEnumerator ClientChangesOwnerWritableNetVar()
             yield return WaitForOwnerWritableAreEqualOnAll();
         }
 
+        /// <summary>
+        /// This tests the scenario where a client owner has both read and write
+        /// permissions set. The server should be the only instance that can read
+        /// the NetworkVariable.  ServerCannotChangeOwnerWritableNetVar performs
+        /// the same check to make sure the server cannot write to a client owner
+        /// NetworkVariable with owner write permissions.
+        /// </summary>
+        [UnityTest]
+        public IEnumerator ClientOwnerWithReadWriteChangesNetVar()
+        {
+            yield return WaitForOwnerWritableAreEqualOnAll();
+
+            var testObjServer = m_ServerNetworkManager.SpawnManager.SpawnedObjects[m_TestObjId];
+
+            int clientManagerIndex = m_ClientNetworkManagers.Length - 1;
+            var newOwnerClientId = m_ClientNetworkManagers[clientManagerIndex].LocalClientId;
+            testObjServer.ChangeOwnership(newOwnerClientId);
+            yield return NetcodeIntegrationTestHelpers.WaitForTicks(m_ServerNetworkManager, 2);
+
+            yield return WaitForOwnerWritableAreEqualOnAll();
+
+            var testObjClient = m_ClientNetworkManagers[clientManagerIndex].SpawnManager.SpawnedObjects[m_TestObjId];
+            var testCompClient = testObjClient.GetComponent<NetVarPermTestComp>();
+
+            var oldValue = testCompClient.OwnerReadWrite_Position.Value;
+            var newValue = oldValue + new Vector3(Random.Range(0, 100.0f), Random.Range(0, 100.0f), Random.Range(0, 100.0f));
+
+            testCompClient.OwnerWritable_Position.Value = newValue;
+            yield return WaitForPositionsAreEqual(testCompClient.OwnerWritable_Position, newValue);
+
+            // Verify the client owner and server match
+            yield return CheckOwnerReadWriteAreEqualOnOwnerAndServer();
+
+            // Verify the non-owner clients do not have the same Value but do have the same permissions
+            yield return CheckOwnerReadWriteAreNotEqualOnNonOwnerClients(testCompClient);
+        }
+
+
         [UnityTest]
         public IEnumerator ClientCannotChangeServerWritableNetVar()
         {