Skip to content
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

tweak(chameleon): Admins see the chameleon verb regardless of wearer #27944

Closed
wants to merge 1 commit into from
Closed

tweak(chameleon): Admins see the chameleon verb regardless of wearer #27944

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 7 additions & 1 deletion Content.Server/Clothing/Systems/ChameleonClothingSystem.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
using Content.Server.IdentityManagement;
using Content.Shared.Administration;
using Content.Shared.Administration.Managers;
using Content.Shared.Clothing.Components;
using Content.Shared.Clothing.EntitySystems;
using Content.Shared.IdentityManagement.Components;
Expand All @@ -13,6 +15,7 @@ namespace Content.Server.Clothing.Systems;

public sealed class ChameleonClothingSystem : SharedChameleonClothingSystem
{
[Dependency] private readonly ISharedAdminManager _adminManager = default!;
[Dependency] private readonly IPrototypeManager _proto = default!;
[Dependency] private readonly UserInterfaceSystem _uiSystem = default!;
[Dependency] private readonly IComponentFactory _factory = default!;
Expand All @@ -33,7 +36,10 @@ private void OnMapInit(EntityUid uid, ChameleonClothingComponent component, MapI

private void OnVerb(EntityUid uid, ChameleonClothingComponent component, GetVerbsEvent<InteractionVerb> args)
{
if (!args.CanAccess || !args.CanInteract || component.User != args.User)
if (!args.CanAccess || !args.CanInteract)
return;

if (component.User != args.User && !_adminManager.HasAdminFlag(args.User, AdminFlags.Admin))
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps create a helper somewhere named HasAdminSight or similar that wraps around this flag check, to make it easier to change later or increase the complexity of.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not understand what you mean or why. A helper for this check seems like an unnecessary abstraction for a specific use case.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not understand what you mean or why. A helper for this check seems like an unnecessary abstraction for a specific use case.

Because this isn't the only place that's going to go "if an admin is looking, they get to see it anyway", and consolidating all of those flag checks into one place means easily changing the required flag(s) later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. Such a refactor is outside the scope of this PR as it would affect hundreds of entity systems.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Such a refactor is outside the scope of this PR as it would affect hundreds of entity systems.

What isn't outside the scope of this PR is setting groundwork.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I had a whole response and discarded it in favor of a short and slight reply.

"if an admin is looking, they get to see it anyway"

In every relevant case I found, that is already true. The subscribers to GetVerbsEvent block the verb when the user cannot access or interact --- admins can always access and interact --- or when it would not make sense for the interaction to take place, such as when wielding guns without holding them, using stethoscopes on inanimate objects, or entering destroyed mechs.

The conditions that actually make that true, CanAccess && CanInteract, make a very common guard, so I can see that being consolidated into one event argument to make underlying changes easier later; However, that is a breaking change across over a hundred files, and I have not looked into what side effects that could have. I will not do that in this PR.

consolidating all of those flag checks into one place means easily changing the required flag(s) later

The component.User == args.User guard is specifically what gets overridden by the user being an admin and is unique to chameleon. Even if there was a hypothetical consolidated HasAdminSight(), there is no way to take advantage of it to shorten component.User == args.User || _adminManager.HasAdminFlag(args.User, AdminFlags.Admin)); the helper would do the admin check, and then the subscriber would do the admin check again anyway.

Now, in addition to having CanAccess && CanInteract be one event argument, we could also add an event argument for, say, isAdmin which is just the result of _adminManager.HasAdminFlag(args.User, AdminFlags.Admin)) , and just do || args.isAdmin to bypass stuff; that way, the subscriber needs not import some administrative namespaces nor deal with specific admin flags. However, this chameleon entity system is the only non-admin-verb subscriber of GetVerbsEvent that I could find which would want to take advantage of that.

If a refactor changes the admin flags, the presence of the AdminFlags.Admin enum here should be easily picked up by such refactor anyway.

I have spent a while now trying to formulate a helper that reduces the number of checks on the subscriber's side while being useful in more than one location. If this is not beyond the scope of the PR, then it is very beyond the scope of my intentions with the PR, and is already beyond the effort I was happy to put in.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I had a whole response and discarded it in favor of a short and slight reply.

"if an admin is looking, they get to see it anyway"

In every relevant case I found, that is already true. The subscribers to GetVerbsEvent block the verb when the user cannot access or interact --- admins can always access and interact --- or when it would not make sense for the interaction to take place, such as when wielding guns without holding them, using stethoscopes on inanimate objects, or entering destroyed mechs.

The conditions that actually make that true, CanAccess && CanInteract, make a very common guard, so I can see that being consolidated into one event argument to make underlying changes easier later; However, that is a breaking change across over a hundred files, and I have not looked into what side effects that could have. I will not do that in this PR.

consolidating all of those flag checks into one place means easily changing the required flag(s) later

The component.User == args.User guard is specifically what gets overridden by the user being an admin and is unique to chameleon. Even if there was a hypothetical consolidated HasAdminSight(), there is no way to take advantage of it to shorten component.User == args.User || _adminManager.HasAdminFlag(args.User, AdminFlags.Admin)); the helper would do the admin check, and then the subscriber would do the admin check again anyway.

Now, in addition to having CanAccess && CanInteract be one event argument, we could also add an event argument for, say, isAdmin which is just the result of _adminManager.HasAdminFlag(args.User, AdminFlags.Admin)) , and just do || args.isAdmin to bypass stuff; that way, the subscriber needs not import some administrative namespaces nor deal with specific admin flags. However, this chameleon entity system is the only non-admin-verb subscriber of GetVerbsEvent that I could find which would want to take advantage of that.

If a refactor changes the admin flags, the presence of the AdminFlags.Admin enum here should be easily picked up by such refactor anyway.

I have spent a while now trying to formulate a helper that reduces the number of checks on the subscriber's side while being useful in more than one location. If this is not beyond the scope of the PR, then it is very beyond the scope of my intentions with the PR, and is already beyond the effort I was happy to put in.

...the point of it isn't to shorten the check it's to put the required admin flags elsewhere.

You wholly overthought what I asked, the whole helper would just be _adminManager.HasAdminFlag(user, AdminFlags.Admin)) on the inside, and its sole job is to centralize the location of this specific adminflags specifier and allow downstreams to potentially modify this check to require more/less/whatever.

I did not:

  • ask you to refactor anything
  • ask you to change any other code
  • ask for the helper to consolidate the entire check into one statement

Copy link
Contributor

Choose a reason for hiding this comment

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

Kindly do not mark a review as resolved when it is very much not yet resolved.

return;

args.Verbs.Add(new InteractionVerb()
Expand Down
Loading