Skip to content

feat!: remove client network permissions [MTT-1019] #1051

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

feat!: remove client network permissions [MTT-1019] #1051

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8990cb4
refactor!: convert NetworkTransform.NetworkState to `struct`
0xFA11 Aug 17, 2021
a304dcc
feat!: remove client network permissions [MTT-1019]
mattwalsh-unity Aug 13, 2021
1742b10
Merge branch 'develop' into feature/container_cleanup
0xFA11 Aug 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions com.unity.netcode.gameobjects/Editor/NetworkBehaviourEditor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,7 @@ private void RenderNetworkVariable(int index)
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);
Expand All @@ -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);
Expand Down
93 changes: 2 additions & 91 deletions com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -543,8 +543,7 @@ private void NetworkVariableUpdate(ulong clientId, int behaviourIndex)

// if I'm dirty AND a client, write (server always has all permissions)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment still valid?

// if I'm dirty AND the server AND the client can read me, send.
bool shouldWrite = isDirty &&
(!IsServer || NetworkVariableFields[k].CanClientRead(clientId));
bool shouldWrite = isDirty && IsServer && NetworkVariableFields[k].CanClientRead(clientId);

if (NetworkManager.NetworkConfig.EnsureNetworkVariableLengthSafety)
{
Expand Down Expand Up @@ -653,7 +652,7 @@ internal static void HandleNetworkVariableDeltas(List<INetworkVariable> networkV
}
}

if (networkManager.IsServer && !networkVariableList[i].CanClientWrite(clientId))
if (networkManager.IsServer)// ?? && !networkVariableList[i].CanClientWrite(clientId))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this even needed anymore? because essentially its checking to see if the client can actually write to this value and if not log it... I am not sure this would be possible anymore correct?

{
if (networkManager.NetworkConfig.EnsureNetworkVariableLengthSafety)
{
Expand Down Expand Up @@ -722,94 +721,6 @@ 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)
{
if (networkVariableList.Count == 0)
Expand Down
5 changes: 4 additions & 1 deletion com.unity.netcode.gameobjects/Runtime/Messaging/MessageQueue/MessageQueueProcessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,10 @@ public void ProcessMessage(in MessageFrameItem item)
InternalMessageHandler.HandleSnapshot(item.NetworkId, item.NetworkBuffer);
break;
case MessageQueueContainer.MessageType.NetworkVariableDelta:
m_NetworkManager.MessageHandler.HandleNetworkVariableDelta(item.NetworkId, item.NetworkBuffer);
if (m_NetworkManager.IsClient)
{
m_NetworkManager.MessageHandler.HandleNetworkVariableDelta(item.NetworkId, item.NetworkBuffer);
}
break;
case MessageQueueContainer.MessageType.SwitchScene:
if (m_NetworkManager.IsClient)
Expand Down
64 changes: 4 additions & 60 deletions com.unity.netcode.gameobjects/Runtime/NetworkVariable/Collections/NetworkDictionary.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ namespace Unity.Netcode
/// </summary>
/// <typeparam name="TKey">The type for the dictionary keys</typeparam>
/// <typeparam name="TValue">The type for the dictionary values</typeparam>
public class NetworkDictionary<TKey, TValue> : IDictionary<TKey, TValue>, INetworkVariable
public class NetworkDictionary<TKey, TValue> : IDictionary<TKey, TValue>, INetworkVariable where TKey : unmanaged where TValue : unmanaged
{
/// <summary>
/// Gets the last time the variable was synced
Expand Down Expand Up @@ -325,51 +325,15 @@ public void WriteField(Stream stream)
}
}

/// <inheritdoc />
public bool CanClientWrite(ulong clientId)
{
switch (Settings.WritePermission)
{
case NetworkVariablePermission.Everyone:
return true;
case NetworkVariablePermission.ServerOnly:
return false;
case NetworkVariablePermission.OwnerOnly:
return m_NetworkBehaviour.OwnerClientId == clientId;
case NetworkVariablePermission.Custom:
{
if (Settings.WritePermissionCallback == null)
{
return false;
}

return Settings.WritePermissionCallback(clientId);
}
}

return true;
}

/// <inheritdoc />
public bool CanClientRead(ulong clientId)
{
switch (Settings.ReadPermission)
{
case NetworkVariablePermission.Everyone:
case NetworkVariableReadPermission.Everyone:
return true;
case NetworkVariablePermission.ServerOnly:
return false;
case NetworkVariablePermission.OwnerOnly:
case NetworkVariableReadPermission.OwnerOnly:
return m_NetworkBehaviour.OwnerClientId == clientId;
case NetworkVariablePermission.Custom:
{
if (Settings.ReadPermissionCallback == null)
{
return false;
}

return Settings.ReadPermissionCallback(clientId);
}
}

return true;
Expand All @@ -378,27 +342,7 @@ public bool CanClientRead(ulong clientId)
/// <inheritdoc />
public bool IsDirty()
{
if (m_DirtyEvents.Count == 0)
{
return false;
}

if (Settings.SendTickrate == 0)
{
return true;
}

if (Settings.SendTickrate < 0)
{
return false;
}

if (m_NetworkBehaviour.NetworkManager.LocalTime.FixedTime - LastSyncedTime.FixedTime >= (1.0 / Settings.SendTickrate))
{
return true;
}

return false;
return m_DirtyEvents.Count > 0;
}


Expand Down
64 changes: 4 additions & 60 deletions com.unity.netcode.gameobjects/Runtime/NetworkVariable/Collections/NetworkList.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ namespace Unity.Netcode
/// Event based NetworkVariable container for syncing Lists
/// </summary>
/// <typeparam name="T">The type for the list</typeparam>
public class NetworkList<T> : IList<T>, INetworkVariable
public class NetworkList<T> : IList<T>, INetworkVariable where T : unmanaged
{
private readonly IList<T> m_List = new List<T>();
private readonly List<NetworkListEvent<T>> m_DirtyEvents = new List<NetworkListEvent<T>>();
Expand Down Expand Up @@ -86,27 +86,7 @@ public void ResetDirty()
/// <inheritdoc />
public bool IsDirty()
{
if (m_DirtyEvents.Count == 0)
{
return false;
}

if (Settings.SendTickrate == 0)
{
return true;
}

if (Settings.SendTickrate < 0)
{
return false;
}

if (m_NetworkBehaviour.NetworkManager.LocalTime.FixedTime - LastSyncedTime.FixedTime >= (1.0 / Settings.SendTickrate))
{
return true;
}

return false;
return m_DirtyEvents.Count > 0;
}

/// <inheritdoc />
Expand All @@ -115,51 +95,15 @@ public NetworkChannel GetChannel()
return Settings.SendNetworkChannel;
}

/// <inheritdoc />
public bool CanClientWrite(ulong clientId)
{
switch (Settings.WritePermission)
{
case NetworkVariablePermission.Everyone:
return true;
case NetworkVariablePermission.ServerOnly:
return false;
case NetworkVariablePermission.OwnerOnly:
return m_NetworkBehaviour.OwnerClientId == clientId;
case NetworkVariablePermission.Custom:
{
if (Settings.WritePermissionCallback == null)
{
return false;
}

return Settings.WritePermissionCallback(clientId);
}
}

return true;
}

/// <inheritdoc />
public bool CanClientRead(ulong clientId)
{
switch (Settings.ReadPermission)
{
case NetworkVariablePermission.Everyone:
case NetworkVariableReadPermission.Everyone:
return true;
case NetworkVariablePermission.ServerOnly:
return false;
case NetworkVariablePermission.OwnerOnly:
case NetworkVariableReadPermission.OwnerOnly:
return m_NetworkBehaviour.OwnerClientId == clientId;
case NetworkVariablePermission.Custom:
{
if (Settings.ReadPermissionCallback == null)
{
return false;
}

return Settings.ReadPermissionCallback(clientId);
}
}

return true;
Expand Down
Loading