Rev Components are no longer leaked + Rev and Zombie icon visibility to ghosts is now controlled by a cvar (space-wizards#22194)

* Initial work on having the Rev icons not be visible to ghosts depending on a Cvar and a component.

This commit just makes it so that the revcomponent and headrev component
are not shared with clients that shouldn't know about them. This is due
to the concern that clients having access to those components, even if
no image was displayed could allow modified clients to have meta
knowledge of revs.

Currently this has the issue that if a player later
for example becomes a rev, none of the existing rev components get
networked to them. I am not sure there is currently an effecient
solution to this.

This is probably in an issue for a lot more stuff. I might just make it
so all the logic just moves to the client on whether to put the icon
again.

Also this commit adds the ShowRevIconsComponent to allow anyone with it to just
view rev icons.

* Rev components now get communicated to clients that didn't have them previously and the AntagIconSystem is now properly checking whether to give the icons.

We now dirty all the rev/headrev components when someone gets converted
or gets the ViewRevIcons component. The AntagIconSystem now checks
whether it should draw the icons mostly based on an event, this is still done
client side.

This is not a full proof solution to make it so clients can't know
someone is an antag when they shouldn't because:
1. There are other components that need similar treatment, to my
   knowledge not to for revs but for other antags. Maybe even the mind
   component. This could be addressed in future PRs.
2. We cannot ensure that clients forget about these components if the
   client gets deconverted for example. We can of course have code that
   does this, but it will necessarily need to be done on the client and
   if the client is modified then there is no way to ensure this.
   Of course at that point they should already know who their fellow
   revs are so this might not be an issue.

I now need to do the same thing for zombies in a future commit.
A similar system for nukies also needs to be looked at but I will not be
doing that in the PR this commit ends up in.

* Misc name changes and cleaning up the ZombieSystem

Changed some names around and decoupled the ZombieSystem from the
AntagStatusIconsystem. Now there is a cvar for ghost visibility for them
as well. The Zombie Component was not made SessionSpecific because:
1. Zombies are pretty visible anyways
2. The Component is needed to change the appearance of zombie players.

* Misc name changes and cleaning up the ZombieSystem

Changed some names around and decoupled the ZombieSystem from the
AntagStatusIconsystem. Now there is a cvar for ghost visibility for them
as well. The Zombie Component was not made SessionSpecific because:
1. Zombies are pretty visible anyways
2. The Component is needed to change the appearance of zombie players.

* Merged 2 if statements into 1 on the Zombiesystem.

* Cut down on code duplication in AntagStatusIconSystem

Now instead of having a seperate function for each component, there is 1 generic function. Functions for special cases
like the Rev/Headrev comp can have a separate function that does the special check and then calls the generic one.
This is done through the IAntagStatusIconComponent interface which provides a common interface to get the Icon.

* Removed some duplication from the SharedRevolutionarySystem with generics.

I have no idea why I didn't think of this sooner.

* Addressed Reviews I think

I think events get unsubbed automatically but I am probably missing something that I have not understood.
Either way this is a requested change.

* Replace war crimes with actual fixes for reviews

It was not clear to me what the reviews meant

* Addressed reviews by removing need for cvars.

Whether icons are visible to ghosts is now determined by a bool in IAntagStatusIcon which all antag components
with status icons should implement.

* Update Content.Shared/Revolutionary/SharedRevolutionarySystem.cs

---------

Co-authored-by: metalgearsloth <31366439+metalgearsloth@users.noreply.github.com>

Author: nikthechampiongr

Content.Client/Antag/AntagStatusIconSystem.cs

@@ -1,6 +1,8 @@
-using Content.Shared.Ghost;
+using Content.Shared.Antag;
+using Content.Shared.Revolutionary.Components;
 using Content.Shared.StatusIcon;
 using Content.Shared.StatusIcon.Components;
+using Content.Shared.Zombies;
 using Robust.Client.Player;
 using Robust.Shared.Prototypes;
 
@@ -9,24 +11,44 @@ namespace Content.Client.Antag;
 /// <summary>
 /// Used for assigning specified icons for antags.
 /// </summary>
-public abstract class AntagStatusIconSystem<T> : SharedStatusIconSystem
-    where T : IComponent
+public sealed class AntagStatusIconSystem : SharedStatusIconSystem
 {
     [Dependency] private readonly IPrototypeManager _prototype = default!;
     [Dependency] private readonly IPlayerManager _player = default!;
+    public override void Initialize()
+    {
+        base.Initialize();
+
+        SubscribeLocalEvent<RevolutionaryComponent, GetStatusIconsEvent>(GetRevIcon);
+        SubscribeLocalEvent<ZombieComponent, GetStatusIconsEvent>(GetIcon);
+        SubscribeLocalEvent<HeadRevolutionaryComponent, GetStatusIconsEvent>(GetIcon);
+    }
 
     /// <summary>
-    /// Will check if the local player has the same component as the one who called it and give the status icon.
+    /// Adds a Status Icon on an entity if the player is supposed to see it.
     /// </summary>
-    /// <param name="antagStatusIcon">The status icon that your antag uses</param>
-    /// <param name="args">The GetStatusIcon event.</param>
-    protected virtual void GetStatusIcon(string antagStatusIcon, ref GetStatusIconsEvent args)
+    private void GetIcon<T>(EntityUid uid, T comp, ref GetStatusIconsEvent ev) where T: IAntagStatusIconComponent
     {
-        var ent = _player.LocalPlayer?.ControlledEntity;
+        var ent = _player.LocalSession?.AttachedEntity;
+
+        var canEv = new CanDisplayStatusIconsEvent(ent);
+        RaiseLocalEvent(uid, ref canEv);
+
+        if (!canEv.Cancelled)
+            ev.StatusIcons.Add(_prototype.Index(comp.StatusIcon));
+    }
 
-        if (!HasComp<T>(ent) && !HasComp<GhostComponent>(ent))
+
+    /// <summary>
+    /// Adds the Rev Icon on an entity if the player is supposed to see it. This additional function is needed to deal
+    /// with a special case where if someone is a head rev we only want to display the headrev icon.
+    /// </summary>
+    private void GetRevIcon(EntityUid uid, RevolutionaryComponent comp, ref GetStatusIconsEvent ev)
+    {
+        if (HasComp<HeadRevolutionaryComponent>(uid))
             return;
 
-        args.StatusIcons.Add(_prototype.Index<StatusIconPrototype>(antagStatusIcon));
+        GetIcon(uid, comp, ref ev);
+
     }
 }

Content.Client/Revolutionary/RevolutionarySystem.cs

@@ -1,35 +1,44 @@
+using Content.Shared.Antag;
 using Content.Shared.Revolutionary.Components;
-using Content.Client.Antag;
+using Content.Shared.Ghost;
 using Content.Shared.StatusIcon.Components;
 
 namespace Content.Client.Revolutionary;
 
 /// <summary>
 /// Used for the client to get status icons from other revs.
 /// </summary>
-public sealed class RevolutionarySystem : AntagStatusIconSystem<RevolutionaryComponent>
+public sealed class RevolutionarySystem : EntitySystem
 {
+
     public override void Initialize()
     {
         base.Initialize();
 
-        SubscribeLocalEvent<RevolutionaryComponent, GetStatusIconsEvent>(GetRevIcon);
-        SubscribeLocalEvent<HeadRevolutionaryComponent, GetStatusIconsEvent>(GetHeadRevIcon);
+        SubscribeLocalEvent<RevolutionaryComponent, CanDisplayStatusIconsEvent>(OnCanShowRevIcon);
+        SubscribeLocalEvent<HeadRevolutionaryComponent, CanDisplayStatusIconsEvent>(OnCanShowRevIcon);
     }
 
     /// <summary>
-    /// Checks if the person who triggers the GetStatusIcon event is also a Rev or a HeadRev.
+    /// Determine whether a client should display the rev icon.
     /// </summary>
-    private void GetRevIcon(EntityUid uid, RevolutionaryComponent comp, ref GetStatusIconsEvent args)
+    private void OnCanShowRevIcon<T>(EntityUid uid, T comp, ref CanDisplayStatusIconsEvent args) where T : IAntagStatusIconComponent
     {
-        if (!HasComp<HeadRevolutionaryComponent>(uid))
-        {
-            GetStatusIcon(comp.RevStatusIcon, ref args);
-        }
+        args.Cancelled = !CanDisplayIcon(args.User, comp.IconVisibleToGhost);
     }
 
-    private void GetHeadRevIcon(EntityUid uid, HeadRevolutionaryComponent comp, ref GetStatusIconsEvent args)
+    /// <summary>
+    /// The criteria that determine whether a client should see Rev/Head rev icons.
+    /// </summary>
+    private bool CanDisplayIcon(EntityUid? uid, bool visibleToGhost)
     {
-        GetStatusIcon(comp.HeadRevStatusIcon, ref args);
+        if (HasComp<HeadRevolutionaryComponent>(uid) || HasComp<RevolutionaryComponent>(uid))
+            return true;
+
+        if (visibleToGhost && HasComp<GhostComponent>(uid))
+            return true;
+
+        return HasComp<ShowRevIconsComponent>(uid);
     }
+
 }

Content.Client/Zombies/ZombieSystem.cs

@@ -1,21 +1,20 @@
 using System.Linq;
-using Content.Client.Antag;
+using Content.Shared.Ghost;
 using Content.Shared.Humanoid;
 using Content.Shared.StatusIcon.Components;
 using Content.Shared.Zombies;
 using Robust.Client.GameObjects;
 
 namespace Content.Client.Zombies;
 
-public sealed class ZombieSystem : AntagStatusIconSystem<ZombieComponent>
+public sealed class ZombieSystem : EntitySystem
 {
-
     public override void Initialize()
     {
         base.Initialize();
 
         SubscribeLocalEvent<ZombieComponent, ComponentStartup>(OnStartup);
-        SubscribeLocalEvent<ZombieComponent, GetStatusIconsEvent>(OnGetStatusIcon);
+        SubscribeLocalEvent<ZombieComponent, CanDisplayStatusIconsEvent>(OnCanDisplayStatusIcons);
     }
 
     private void OnStartup(EntityUid uid, ZombieComponent component, ComponentStartup args)
@@ -32,8 +31,17 @@ private void OnStartup(EntityUid uid, ZombieComponent component, ComponentStartu
         }
     }
 
-    private void OnGetStatusIcon(EntityUid uid, ZombieComponent component, ref GetStatusIconsEvent args)
+    /// <summary>
+    /// Determines whether a player should be able to see the StatusIcon for zombies.
+    /// </summary>
+    private void OnCanDisplayStatusIcons(EntityUid uid, ZombieComponent component, ref CanDisplayStatusIconsEvent args)
     {
-        GetStatusIcon(component.ZombieStatusIcon, ref args);
+        if (HasComp<ZombieComponent>(args.User) || HasComp<ShowZombieIconsComponent>(args.User))
+            return;
+
+        if (component.IconVisibleToGhost && HasComp<GhostComponent>(args.User))
+            return;
+
+        args.Cancelled = true;
     }
 }

Content.Shared/Antag/IAntagStatusIconComponent.cs

@@ -0,0 +1,12 @@
+using Content.Shared.StatusIcon;
+using Robust.Shared.Prototypes;
+
+namespace Content.Shared.Antag;
+
+public interface IAntagStatusIconComponent
+{
+    public ProtoId<StatusIconPrototype> StatusIcon { get; set; }
+
+    public bool IconVisibleToGhost { get; set; }
+}
+

Content.Shared/Revolutionary/Components/HeadRevolutionaryComponent.cs

@@ -1,3 +1,4 @@
+using Content.Shared.Antag;
 using Robust.Shared.GameStates;
 using Content.Shared.StatusIcon;
 using Robust.Shared.Prototypes;
@@ -8,17 +9,22 @@ namespace Content.Shared.Revolutionary.Components;
 /// Component used for marking a Head Rev for conversion and winning/losing.
 /// </summary>
 [RegisterComponent, NetworkedComponent, Access(typeof(SharedRevolutionarySystem))]
-public sealed partial class HeadRevolutionaryComponent : Component
+public sealed partial class HeadRevolutionaryComponent : Component, IAntagStatusIconComponent
 {
     /// <summary>
     /// The status icon corresponding to the head revolutionary.
     /// </summary>
     [DataField, ViewVariables(VVAccess.ReadWrite)]
-    public ProtoId<StatusIconPrototype> HeadRevStatusIcon = "HeadRevolutionaryFaction";
+    public ProtoId<StatusIconPrototype> StatusIcon { get; set; } = "HeadRevolutionaryFaction";
 
     /// <summary>
     /// How long the stun will last after the user is converted.
     /// </summary>
     [DataField, ViewVariables(VVAccess.ReadWrite)]
     public TimeSpan StunTime = TimeSpan.FromSeconds(3);
+
+    public override bool SessionSpecific => true;
+
+    [DataField]
+    public bool IconVisibleToGhost { get; set; } = true;
 }

Content.Shared/Revolutionary/Components/RevolutionaryComponent.cs

@@ -1,3 +1,4 @@
+using Content.Shared.Antag;
 using Robust.Shared.GameStates;
 using Content.Shared.StatusIcon;
 using Robust.Shared.Prototypes;
@@ -8,11 +9,16 @@ namespace Content.Shared.Revolutionary.Components;
 /// Used for marking regular revs as well as storing icon prototypes so you can see fellow revs.
 /// </summary>
 [RegisterComponent, NetworkedComponent, Access(typeof(SharedRevolutionarySystem))]
-public sealed partial class RevolutionaryComponent : Component
+public sealed partial class RevolutionaryComponent : Component, IAntagStatusIconComponent
 {
     /// <summary>
     /// The status icon prototype displayed for revolutionaries
     /// </summary>
     [DataField, ViewVariables(VVAccess.ReadWrite)]
-    public ProtoId<StatusIconPrototype> RevStatusIcon = "RevolutionaryFaction";
+    public ProtoId<StatusIconPrototype> StatusIcon { get; set; } = "RevolutionaryFaction";
+
+    public override bool SessionSpecific => true;
+
+    [DataField]
+    public bool IconVisibleToGhost { get; set; } = true;
 }

Content.Shared/Revolutionary/Components/ShowRevIconsComponent.cs

@@ -0,0 +1,11 @@
+using Robust.Shared.GameStates;
+
+namespace Content.Shared.Revolutionary.Components;
+
+/// <summary>
+/// Determines whether Someone can see rev/headrev icons on revs.
+/// </summary>
+[RegisterComponent, NetworkedComponent]
+public sealed partial class ShowRevIconsComponent: Component
+{
+}

Content.Shared/Revolutionary/SharedRevolutionarySystem.cs

@@ -1,8 +1,11 @@
+using Content.Shared.Ghost;
 using Content.Shared.IdentityManagement;
 using Content.Shared.Mindshield.Components;
 using Content.Shared.Popups;
 using Content.Shared.Revolutionary.Components;
 using Content.Shared.Stunnable;
+using Robust.Shared.GameStates;
+using Robust.Shared.Player;
 
 namespace Content.Shared.Revolutionary;
 
@@ -14,7 +17,13 @@ public sealed class SharedRevolutionarySystem : EntitySystem
     public override void Initialize()
     {
         base.Initialize();
+
         SubscribeLocalEvent<MindShieldComponent, MapInitEvent>(MindShieldImplanted);
+        SubscribeLocalEvent<RevolutionaryComponent, ComponentGetStateAttemptEvent>(OnRevCompGetStateAttempt);
+        SubscribeLocalEvent<HeadRevolutionaryComponent, ComponentGetStateAttemptEvent>(OnRevCompGetStateAttempt);
+        SubscribeLocalEvent<RevolutionaryComponent, ComponentStartup>(DirtyRevComps);
+        SubscribeLocalEvent<HeadRevolutionaryComponent, ComponentStartup>(DirtyRevComps);
+        SubscribeLocalEvent<ShowRevIconsComponent, ComponentStartup>(DirtyRevComps);
     }
 
     /// <summary>
@@ -37,4 +46,64 @@ private void MindShieldImplanted(EntityUid uid, MindShieldComponent comp, MapIni
             _popupSystem.PopupEntity(Loc.GetString("rev-break-control", ("name", name)), uid);
         }
     }
+
+    /// <summary>
+    /// Determines if a HeadRev component should be sent to the client.
+    /// </summary>
+    private void OnRevCompGetStateAttempt(EntityUid uid, HeadRevolutionaryComponent comp, ref ComponentGetStateAttemptEvent args)
+    {
+        args.Cancelled = !CanGetState(args.Player, comp.IconVisibleToGhost);
+    }
+
+    /// <summary>
+    /// Determines if a Rev component should be sent to the client.
+    /// </summary>
+    private void OnRevCompGetStateAttempt(EntityUid uid, RevolutionaryComponent comp, ref ComponentGetStateAttemptEvent args)
+    {
+        args.Cancelled = !CanGetState(args.Player, comp.IconVisibleToGhost);
+    }
+
+    /// <summary>
+    /// The criteria that determine whether a Rev/HeadRev component should be sent to a client.
+    /// </summary>
+    /// <param name="player"> The Player the component will be sent to.</param>
+    /// <param name="visibleToGhosts"> Whether the component permits the icon to be visible to observers. </param>
+    /// <returns></returns>
+    private bool CanGetState(ICommonSession? player, bool visibleToGhosts)
+    {
+        //Apparently this can be null in replays so I am just returning true.
+        if (player is null)
+            return true;
+
+        var uid = player.AttachedEntity;
+
+        if (HasComp<RevolutionaryComponent>(uid) || HasComp<HeadRevolutionaryComponent>(uid))
+            return true;
+
+        if (visibleToGhosts && HasComp<GhostComponent>(uid))
+            return true;
+
+        return HasComp<ShowRevIconsComponent>(uid);
+    }
+    /// <summary>
+    /// Dirties all the Rev components so they are sent to clients.
+    ///
+    /// We need to do this because if a rev component was not earlier sent to a client and for example the client
+    /// becomes a rev then we need to send all the components to it. To my knowledge there is no way to do this on a
+    /// per client basis so we are just dirtying all the components.
+    /// </summary>
+    private void DirtyRevComps<T>(EntityUid someUid, T someComp, ComponentStartup ev)
+    {
+        var revComps = AllEntityQuery<RevolutionaryComponent>();
+        while (revComps.MoveNext(out var uid, out var comp))
+        {
+            Dirty(uid, comp);
+        }
+
+        var headRevComps = AllEntityQuery<HeadRevolutionaryComponent>();
+        while (headRevComps.MoveNext(out var uid, out var comp))
+        {
+            Dirty(uid, comp);
+        }
+    }
 }

Content.Shared/StatusIcon/Components/StatusIconComponent.cs

@@ -25,3 +25,15 @@ public sealed partial class StatusIconComponent : Component
 /// <param name="StatusIcons"></param>
 [ByRefEvent]
 public record struct GetStatusIconsEvent(List<StatusIconData> StatusIcons, bool InContainer);
+
+/// <summary>
+/// Event raised on the Client-side to determine whether to display a status icon on an entity.
+/// </summary>
+/// <param name="User">The player that will see the icons</param>
+[ByRefEvent]
+public record struct CanDisplayStatusIconsEvent(EntityUid? User = null)
+{
+    public EntityUid? User = User;
+
+    public bool Cancelled = false;
+}

Content.Shared/Zombies/ShowZombieIconsComponent.cs

@@ -0,0 +1,12 @@
+using Robust.Shared.GameStates;
+
+namespace Content.Shared.Zombies;
+
+/// <summary>
+/// Makes it so an entity can view ZombieAntagIcons.
+/// </summary>
+[RegisterComponent, NetworkedComponent]
+public sealed partial class ShowZombieIconsComponent: Component
+{
+
+}