Fix role unbans not applying in real time (#20547)

Content.Server/Administration/Commands/RoleUnbanCommand.cs

@@ -1,7 +1,5 @@
-using System.Text;
+using Content.Server.Administration.Managers;
-using Content.Server.Database;
 using Content.Shared.Administration;
-using Robust.Server.Player;
 using Robust.Shared.Console;
 
 namespace Content.Server.Administration.Commands;
@@ -15,9 +13,6 @@ public sealed class RoleUnbanCommand : IConsoleCommand
 
     public async void Execute(IConsoleShell shell, string argStr, string[] args)
     {
-        var player = shell.Player as IPlayerSession;
-        var dbMan = IoCManager.Resolve<IServerDbManager>();
-
         if (args.Length != 1)
         {
             shell.WriteLine(Help);
@@ -30,32 +25,9 @@ public sealed class RoleUnbanCommand : IConsoleCommand
             return;
         }
 
-        var ban = await dbMan.GetServerRoleBanAsync(banId);
+        var banManager = IoCManager.Resolve<IBanManager>();
-
+        var response = await banManager.PardonRoleBan(banId, shell.Player?.UserId, DateTimeOffset.Now);
-        if (ban == null)
+        shell.WriteLine(response);
-        {
-            shell.WriteLine($"No ban found with id {banId}");
-            return;
-        }
-
-        if (ban.Unban != null)
-        {
-            var response = new StringBuilder("This ban has already been pardoned");
-
-            if (ban.Unban.UnbanningAdmin != null)
-            {
-                response.Append($" by {ban.Unban.UnbanningAdmin.Value}");
-            }
-
-            response.Append($" in {ban.Unban.UnbanTime}.");
-
-            shell.WriteLine(response.ToString());
-            return;
-        }
-
-        await dbMan.AddServerRoleUnbanAsync(new ServerRoleUnbanDef(banId, player?.UserId, DateTimeOffset.Now));
-
-        shell.WriteLine($"Pardoned ban with id {banId}");
     }
 
     public CompletionResult GetCompletion(IConsoleShell shell, string[] args)

Content.Server/Administration/Managers/BanManager.cs

@@ -1,21 +1,22 @@
 using System.Collections.Immutable;
 using System.Linq;
 using System.Net;
+using System.Text;
 using System.Threading.Tasks;
 using Content.Server.Chat.Managers;
 using Content.Server.Database;
 using Content.Server.GameTicking;
+using Content.Shared.CCVar;
 using Content.Shared.Database;
 using Content.Shared.Players;
 using Content.Shared.Players.PlayTimeTracking;
 using Content.Shared.Roles;
-using Microsoft.CodeAnalysis;
-using Content.Shared.CCVar;
 using Robust.Server.Player;
 using Robust.Shared.Configuration;
 using Robust.Shared.Enums;
 using Robust.Shared.Network;
 using Robust.Shared.Prototypes;
+using Robust.Shared.Utility;
 
 namespace Content.Server.Administration.Managers;
 
@@ -47,28 +48,25 @@ public sealed class BanManager : IBanManager, IPostInjectInit
 
     private async void OnPlayerStatusChanged(object? sender, SessionStatusEventArgs e)
     {
-        if (e.NewStatus != SessionStatus.Connected
+        if (e.NewStatus != SessionStatus.Connected || _cachedRoleBans.ContainsKey(e.Session.UserId))
-            || _cachedRoleBans.ContainsKey(e.Session.UserId))
             return;
 
         var netChannel = e.Session.ConnectedClient;
-        await CacheDbRoleBans(e.Session.UserId, netChannel.RemoteEndPoint.Address, netChannel.UserData.HWId.Length == 0 ? null : netChannel.UserData.HWId);
+        ImmutableArray<byte>? 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<bool> AddRoleBan(ServerRoleBanDef banDef)
     {
+        banDef = await _db.AddServerRoleBanAsync(banDef);
+
         if (banDef.UserId != null)
         {
-            if (!_cachedRoleBans.TryGetValue(banDef.UserId.Value, out var roleBans))
+            _cachedRoleBans.GetOrNew(banDef.UserId.Value).Add(banDef);
-            {
-                roleBans = new HashSet<ServerRoleBanDef>();
-                _cachedRoleBans.Add(banDef.UserId.Value, roleBans);
-            }
-            if (!roleBans.Contains(banDef))
-                roleBans.Add(banDef);
         }
 
-        await _db.AddServerRoleBanAsync(banDef);
         return true;
     }
 
@@ -231,6 +229,39 @@ public sealed class BanManager : IBanManager, IPostInjectInit
         }
     }
 
+    public async Task<string> PardonRoleBan(int banId, NetUserId? unbanningAdmin, DateTimeOffset unbanTime)
+    {
+        var ban = await _db.GetServerRoleBanAsync(banId);
+
+        if (ban == null)
+        {
+            return $"No ban found with id {banId}";
+        }
+
+        if (ban.Unban != null)
+        {
+            var response = new StringBuilder("This ban has already been pardoned");
+
+            if (ban.Unban.UnbanningAdmin != null)
+            {
+                response.Append($" by {ban.Unban.UnbanningAdmin.Value}");
+            }
+
+            response.Append($" in {ban.Unban.UnbanTime}.");
+            return response.ToString();
+        }
+
+        await _db.AddServerRoleUnbanAsync(new ServerRoleUnbanDef(banId, unbanningAdmin, DateTimeOffset.Now));
+
+        if (ban.UserId is { } player && _cachedRoleBans.TryGetValue(player, out var roleBans))
+        {
+            roleBans.RemoveWhere(roleBan => roleBan.Id == ban.Id);
+            SendRoleBans(player);
+        }
+
+        return $"Pardoned ban with id {banId}";
+    }
+
     public HashSet<string>? GetJobBans(NetUserId playerUserId)
     {
         if (!_cachedRoleBans.TryGetValue(playerUserId, out var roleBans))
@@ -254,12 +285,7 @@ public sealed class BanManager : IBanManager, IPostInjectInit
 
     public void SendRoleBans(IPlayerSession pSession)
     {
-        if (!_cachedRoleBans.TryGetValue(pSession.UserId, out var roleBans))
+        var roleBans = _cachedRoleBans.GetValueOrDefault(pSession.UserId) ?? new HashSet<ServerRoleBanDef>();
-        {
-            _sawmill.Error($"Tried to send rolebans for {pSession.Name} but none cached?");
-            return;
-        }
-
         var bans = new MsgRoleBans()
         {
             Bans = roleBans.Select(o => o.Role).ToList()

Content.Server/Administration/Managers/IBanManager.cs

@@ -1,8 +1,9 @@
 using System.Collections.Immutable;
+using System.Net;
+using System.Threading.Tasks;
 using Content.Shared.Database;
 using Robust.Server.Player;
 using Robust.Shared.Network;
-using System.Net;
 
 namespace Content.Server.Administration.Managers;
 
@@ -24,18 +25,26 @@ public interface IBanManager
     public void CreateServerBan(NetUserId? target, string? targetUsername, NetUserId? banningAdmin, (IPAddress, int)? addressRange, ImmutableArray<byte>? hwid, uint? minutes, NoteSeverity severity, string reason);
     public HashSet<string>? GetRoleBans(NetUserId playerUserId);
     public HashSet<string>? GetJobBans(NetUserId playerUserId);
+
     /// <summary>
     /// Creates a job ban for the specified target, username or GUID
     /// </summary>
-    /// <param name="shell">Shell reference so we can write messages</param>
     /// <param name="target">Target user, username or GUID, null for none</param>
-    /// <param name="job">Job to be banned from</param>
+    /// <param name="role">Role to be banned from</param>
     /// <param name="severity">Severity of the resulting ban note</param>
     /// <param name="reason">Reason for the ban</param>
     /// <param name="minutes">Number of minutes to ban for. 0 and null mean permanent</param>
     /// <param name="timeOfBan">Time when the ban was applied, used for grouping role bans</param>
     public void CreateRoleBan(NetUserId? target, string? targetUsername, NetUserId? banningAdmin, (IPAddress, int)? addressRange, ImmutableArray<byte>? hwid, string role, uint? minutes, NoteSeverity severity, string reason, DateTimeOffset timeOfBan);
 
+    /// <summary>
+    /// Pardons a role ban for the specified target, username or GUID
+    /// </summary>
+    /// <param name="banId">The id of the role ban to pardon.</param>
+    /// <param name="unbanningAdmin">The admin, if any, that pardoned the role ban.</param>
+    /// <param name="unbanTime">The time at which this role ban was pardoned.</param>
+    public Task<string> PardonRoleBan(int banId, NetUserId? unbanningAdmin, DateTimeOffset unbanTime);
+
     /// <summary>
     /// Sends role bans to the target
     /// </summary>

Content.Server/Database/ServerDbBase.cs

@@ -432,7 +432,7 @@ namespace Content.Server.Database
             ImmutableArray<byte>? hwId,
             bool includeUnbanned);
 
-        public abstract Task AddServerRoleBanAsync(ServerRoleBanDef serverRoleBan);
+        public abstract Task<ServerRoleBanDef> AddServerRoleBanAsync(ServerRoleBanDef serverRoleBan);
         public abstract Task AddServerRoleUnbanAsync(ServerRoleUnbanDef serverRoleUnban);
 
         public async Task EditServerRoleBan(int id, string reason, NoteSeverity severity, DateTime? expiration, Guid editedBy, DateTime editedAt)

Content.Server/Database/ServerDbManager.cs

@@ -14,7 +14,6 @@ using Microsoft.EntityFrameworkCore;
 using Microsoft.Extensions.Logging;
 using Npgsql;
 using Prometheus;
-using Robust.Shared.Asynchronous;
 using Robust.Shared.Configuration;
 using Robust.Shared.ContentPack;
 using Robust.Shared.Network;
@@ -140,7 +139,7 @@ namespace Content.Server.Database
             ImmutableArray<byte>? hwId,
             bool includeUnbanned = true);
 
-        Task AddServerRoleBanAsync(ServerRoleBanDef serverBan);
+        Task<ServerRoleBanDef> AddServerRoleBanAsync(ServerRoleBanDef serverBan);
         Task AddServerRoleUnbanAsync(ServerRoleUnbanDef serverBan);
 
         public Task EditServerRoleBan(
@@ -453,7 +452,7 @@ namespace Content.Server.Database
             return RunDbCommand(() => _db.GetServerRoleBansAsync(address, userId, hwId, includeUnbanned));
         }
 
-        public Task AddServerRoleBanAsync(ServerRoleBanDef serverRoleBan)
+        public Task<ServerRoleBanDef> AddServerRoleBanAsync(ServerRoleBanDef serverRoleBan)
         {
             DbWriteOpsMetric.Inc();
             return RunDbCommand(() => _db.AddServerRoleBanAsync(serverRoleBan));

Content.Server/Database/ServerDbPostgres.cs

@@ -1,5 +1,6 @@
 using System.Collections.Immutable;
 using System.Data;
+using System.Diagnostics.CodeAnalysis;
 using System.Linq;
 using System.Net;
 using System.Threading;
@@ -350,6 +351,7 @@ namespace Content.Server.Database
             return query;
         }
 
+        [return: NotNullIfNotNull(nameof(ban))]
         private static ServerRoleBanDef? ConvertRoleBan(ServerRoleBan? ban)
         {
             if (ban == null)
@@ -406,11 +408,11 @@ namespace Content.Server.Database
                 unban.UnbanTime);
         }
 
-        public override async Task AddServerRoleBanAsync(ServerRoleBanDef serverRoleBan)
+        public override async Task<ServerRoleBanDef> AddServerRoleBanAsync(ServerRoleBanDef serverRoleBan)
         {
             await using var db = await GetDbImpl();
 
-            db.PgDbContext.RoleBan.Add(new ServerRoleBan
+            var ban = new ServerRoleBan
             {
                 Address = serverRoleBan.Address,
                 HWId = serverRoleBan.HWId?.ToArray(),
@@ -423,9 +425,11 @@ namespace Content.Server.Database
                 PlaytimeAtNote = serverRoleBan.PlaytimeAtNote,
                 PlayerUserId = serverRoleBan.UserId?.UserId,
                 RoleId = serverRoleBan.Role,
-            });
+            };
+            db.PgDbContext.RoleBan.Add(ban);
 
             await db.PgDbContext.SaveChangesAsync();
+            return ConvertRoleBan(ban);
         }
 
         public override async Task AddServerRoleUnbanAsync(ServerRoleUnbanDef serverRoleUnban)

Content.Server/Database/ServerDbSqlite.cs

@@ -1,4 +1,5 @@
 using System.Collections.Immutable;
+using System.Diagnostics.CodeAnalysis;
 using System.Linq;
 using System.Net;
 using System.Threading;
@@ -246,11 +247,11 @@ namespace Content.Server.Database
             return hwId is { Length: > 0 } hwIdVar && hwIdVar.AsSpan().SequenceEqual(ban.HWId);
         }
 
-        public override async Task AddServerRoleBanAsync(ServerRoleBanDef serverBan)
+        public override async Task<ServerRoleBanDef> AddServerRoleBanAsync(ServerRoleBanDef serverBan)
         {
             await using var db = await GetDbImpl();
 
-            db.SqliteDbContext.RoleBan.Add(new ServerRoleBan
+            var ban = new ServerRoleBan
             {
                 Address = serverBan.Address,
                 Reason = serverBan.Reason,
@@ -263,9 +264,11 @@ namespace Content.Server.Database
                 PlaytimeAtNote = serverBan.PlaytimeAtNote,
                 PlayerUserId = serverBan.UserId?.UserId,
                 RoleId = serverBan.Role,
-            });
+            };
+            db.SqliteDbContext.RoleBan.Add(ban);
 
             await db.SqliteDbContext.SaveChangesAsync();
+            return ConvertRoleBan(ban);
         }
 
         public override async Task AddServerRoleUnbanAsync(ServerRoleUnbanDef serverUnban)
@@ -282,6 +285,7 @@ namespace Content.Server.Database
             await db.SqliteDbContext.SaveChangesAsync();
         }
 
+        [return: NotNullIfNotNull(nameof(ban))]
         private static ServerRoleBanDef? ConvertRoleBan(ServerRoleBan? ban)
         {
             if (ban == null)