From 4a5178a5389a4a5beb952c884515e011046362b5 Mon Sep 17 00:00:00 2001 From: Pieter-Jan Briers Date: Mon, 14 Oct 2024 05:30:31 +0200 Subject: [PATCH] Fix role ban loading bugs (#32725) This code was a mess. Now it's less of a mess and user UserDbDataManager now. Fixes the following bugs: * If you connect to a server, restart your client, connect again in the same round, you role bans would not be visible in the client. * If you role ban somebody who is not connected to the server, then they connect within the round, they will only have the recently-applied ban. Likely fixes #24781, #27282 --- .../JobRequirementsManager.cs | 5 +- .../Administration/Managers/BanManager.cs | 95 ++++++++----------- .../Administration/Managers/IBanManager.cs | 6 -- .../GameTicking/GameTicker.RoundFlow.cs | 9 +- 4 files changed, 43 insertions(+), 72 deletions(-) diff --git a/Content.Client/Players/PlayTimeTracking/JobRequirementsManager.cs b/Content.Client/Players/PlayTimeTracking/JobRequirementsManager.cs index 6d52c50290..314b59eda9 100644 --- a/Content.Client/Players/PlayTimeTracking/JobRequirementsManager.cs +++ b/Content.Client/Players/PlayTimeTracking/JobRequirementsManager.cs @@ -51,6 +51,8 @@ public sealed class JobRequirementsManager : ISharedPlaytimeManager { // Reset on disconnect, just in case. _roles.Clear(); + _jobWhitelists.Clear(); + _roleBans.Clear(); } } @@ -58,9 +60,6 @@ public sealed class JobRequirementsManager : ISharedPlaytimeManager { _sawmill.Debug($"Received roleban info containing {message.Bans.Count} entries."); - if (_roleBans.Equals(message.Bans)) - return; - _roleBans.Clear(); _roleBans.AddRange(message.Bans); Updated?.Invoke(); diff --git a/Content.Server/Administration/Managers/BanManager.cs b/Content.Server/Administration/Managers/BanManager.cs index 946770d6aa..1cdfb82224 100644 --- a/Content.Server/Administration/Managers/BanManager.cs +++ b/Content.Server/Administration/Managers/BanManager.cs @@ -14,13 +14,13 @@ using Content.Shared.Players.PlayTimeTracking; using Content.Shared.Roles; using Robust.Server.Player; using Robust.Shared.Asynchronous; +using Robust.Shared.Collections; using Robust.Shared.Configuration; using Robust.Shared.Enums; using Robust.Shared.Network; using Robust.Shared.Player; using Robust.Shared.Prototypes; using Robust.Shared.Timing; -using Robust.Shared.Utility; namespace Content.Server.Administration.Managers; @@ -45,14 +45,12 @@ public sealed partial class BanManager : IBanManager, IPostInjectInit public const string SawmillId = "admin.bans"; public const string JobPrefix = "Job:"; - private readonly Dictionary> _cachedRoleBans = new(); + private readonly Dictionary> _cachedRoleBans = new(); // Cached ban exemption flags are used to handle private readonly Dictionary _cachedBanExemptions = new(); public void Initialize() { - _playerManager.PlayerStatusChanged += OnPlayerStatusChanged; - _netManager.RegisterNetMessage(); _db.SubscribeToNotifications(OnDatabaseNotification); @@ -63,12 +61,23 @@ public sealed partial class BanManager : IBanManager, IPostInjectInit private async Task CachePlayerData(ICommonSession player, CancellationToken cancel) { - // Yeah so role ban loading code isn't integrated with exempt flag loading code. - // Have you seen how garbage role ban code code is? I don't feel like refactoring it right now. - var flags = await _db.GetBanExemption(player.UserId, cancel); + + var netChannel = player.Channel; + ImmutableArray? hwId = netChannel.UserData.HWId.Length == 0 ? null : netChannel.UserData.HWId; + var roleBans = await _db.GetServerRoleBansAsync(netChannel.RemoteEndPoint.Address, player.UserId, hwId, false); + + var userRoleBans = new List(); + foreach (var ban in roleBans) + { + userRoleBans.Add(ban); + } + cancel.ThrowIfCancellationRequested(); _cachedBanExemptions[player] = flags; + _cachedRoleBans[player] = userRoleBans; + + SendRoleBans(player); } private void ClearPlayerData(ICommonSession player) @@ -76,25 +85,15 @@ public sealed partial class BanManager : IBanManager, IPostInjectInit _cachedBanExemptions.Remove(player); } - private async void OnPlayerStatusChanged(object? sender, SessionStatusEventArgs e) - { - if (e.NewStatus != SessionStatus.Connected || _cachedRoleBans.ContainsKey(e.Session.UserId)) - return; - - var netChannel = e.Session.Channel; - ImmutableArray? hwId = netChannel.UserData.HWId.Length == 0 ? null : netChannel.UserData.HWId; - await CacheDbRoleBans(e.Session.UserId, netChannel.RemoteEndPoint.Address, hwId); - - SendRoleBans(e.Session); - } - private async Task AddRoleBan(ServerRoleBanDef banDef) { banDef = await _db.AddServerRoleBanAsync(banDef); - if (banDef.UserId != null) + if (banDef.UserId != null + && _playerManager.TryGetSessionById(banDef.UserId, out var player) + && _cachedRoleBans.TryGetValue(player, out var cachedBans)) { - _cachedRoleBans.GetOrNew(banDef.UserId.Value).Add(banDef); + cachedBans.Add(banDef); } return true; @@ -102,31 +101,21 @@ public sealed partial class BanManager : IBanManager, IPostInjectInit public HashSet? GetRoleBans(NetUserId playerUserId) { - return _cachedRoleBans.TryGetValue(playerUserId, out var roleBans) + if (!_playerManager.TryGetSessionById(playerUserId, out var session)) + return null; + + return _cachedRoleBans.TryGetValue(session, out var roleBans) ? roleBans.Select(banDef => banDef.Role).ToHashSet() : null; } - private async Task CacheDbRoleBans(NetUserId userId, IPAddress? address = null, ImmutableArray? hwId = null) - { - var roleBans = await _db.GetServerRoleBansAsync(address, userId, hwId, false); - - var userRoleBans = new HashSet(); - foreach (var ban in roleBans) - { - userRoleBans.Add(ban); - } - - _cachedRoleBans[userId] = userRoleBans; - } - public void Restart() { // Clear out players that have disconnected. - var toRemove = new List(); + var toRemove = new ValueList(); foreach (var player in _cachedRoleBans.Keys) { - if (!_playerManager.TryGetSessionById(player, out _)) + if (player.Status == SessionStatus.Disconnected) toRemove.Add(player); } @@ -138,7 +127,7 @@ public sealed partial class BanManager : IBanManager, IPostInjectInit // Check for expired bans foreach (var roleBans in _cachedRoleBans.Values) { - roleBans.RemoveWhere(ban => DateTimeOffset.Now > ban.ExpirationTime); + roleBans.RemoveAll(ban => DateTimeOffset.Now > ban.ExpirationTime); } } @@ -281,9 +270,9 @@ public sealed partial class BanManager : IBanManager, IPostInjectInit var length = expires == null ? Loc.GetString("cmd-roleban-inf") : Loc.GetString("cmd-roleban-until", ("expires", expires)); _chat.SendAdminAlert(Loc.GetString("cmd-roleban-success", ("target", targetUsername ?? "null"), ("role", role), ("reason", reason), ("length", length))); - if (target != null) + if (target != null && _playerManager.TryGetSessionById(target.Value, out var session)) { - SendRoleBans(target.Value); + SendRoleBans(session); } } @@ -311,10 +300,12 @@ public sealed partial class BanManager : IBanManager, IPostInjectInit await _db.AddServerRoleUnbanAsync(new ServerRoleUnbanDef(banId, unbanningAdmin, DateTimeOffset.Now)); - if (ban.UserId is { } player && _cachedRoleBans.TryGetValue(player, out var roleBans)) + if (ban.UserId is { } player + && _playerManager.TryGetSessionById(player, out var session) + && _cachedRoleBans.TryGetValue(session, out var roleBans)) { - roleBans.RemoveWhere(roleBan => roleBan.Id == ban.Id); - SendRoleBans(player); + roleBans.RemoveAll(roleBan => roleBan.Id == ban.Id); + SendRoleBans(session); } return $"Pardoned ban with id {banId}"; @@ -322,8 +313,12 @@ public sealed partial class BanManager : IBanManager, IPostInjectInit public HashSet>? GetJobBans(NetUserId playerUserId) { - if (!_cachedRoleBans.TryGetValue(playerUserId, out var roleBans)) + if (!_playerManager.TryGetSessionById(playerUserId, out var session)) + return null; + + if (!_cachedRoleBans.TryGetValue(session, out var roleBans)) return null; + return roleBans .Where(ban => ban.Role.StartsWith(JobPrefix, StringComparison.Ordinal)) .Select(ban => new ProtoId(ban.Role[JobPrefix.Length..])) @@ -331,19 +326,9 @@ public sealed partial class BanManager : IBanManager, IPostInjectInit } #endregion - public void SendRoleBans(NetUserId userId) - { - if (!_playerManager.TryGetSessionById(userId, out var player)) - { - return; - } - - SendRoleBans(player); - } - public void SendRoleBans(ICommonSession pSession) { - var roleBans = _cachedRoleBans.GetValueOrDefault(pSession.UserId) ?? new HashSet(); + var roleBans = _cachedRoleBans.GetValueOrDefault(pSession) ?? new List(); var bans = new MsgRoleBans() { Bans = roleBans.Select(o => o.Role).ToList() diff --git a/Content.Server/Administration/Managers/IBanManager.cs b/Content.Server/Administration/Managers/IBanManager.cs index b60e0a2535..c11e310a82 100644 --- a/Content.Server/Administration/Managers/IBanManager.cs +++ b/Content.Server/Administration/Managers/IBanManager.cs @@ -47,12 +47,6 @@ public interface IBanManager /// The time at which this role ban was pardoned. public Task PardonRoleBan(int banId, NetUserId? unbanningAdmin, DateTimeOffset unbanTime); - /// - /// Sends role bans to the target - /// - /// Player's user ID - public void SendRoleBans(NetUserId userId); - /// /// Sends role bans to the target /// diff --git a/Content.Server/GameTicking/GameTicker.RoundFlow.cs b/Content.Server/GameTicking/GameTicker.RoundFlow.cs index e544870bd2..a7dd5d6ab6 100644 --- a/Content.Server/GameTicking/GameTicker.RoundFlow.cs +++ b/Content.Server/GameTicking/GameTicker.RoundFlow.cs @@ -192,9 +192,6 @@ namespace Content.Server.GameTicking if (!_playerManager.TryGetSessionById(userId, out _)) continue; - if (_banManager.GetRoleBans(userId) == null) - continue; - total++; } @@ -238,11 +235,7 @@ namespace Content.Server.GameTicking #if DEBUG DebugTools.Assert(_userDb.IsLoadComplete(session), $"Player was readied up but didn't have user DB data loaded yet??"); #endif - if (_banManager.GetRoleBans(userId) == null) - { - Logger.ErrorS("RoleBans", $"Role bans for player {session} {userId} have not been loaded yet."); - continue; - } + readyPlayers.Add(session); HumanoidCharacterProfile profile; if (_prefsManager.TryGetCachedPreferences(userId, out var preferences)) -- 2.52.0