tweak(chameleon): Admins see the chameleon verb regardless of wearer #27944
Conversation
…ring the clothing
| if (!args.CanAccess || !args.CanInteract) | ||
| return; | ||
|
|
||
| if (component.User != args.User && !_adminManager.HasAdminFlag(args.User, AdminFlags.Admin)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I do not understand what you mean or why. A helper for this check seems like an unnecessary abstraction for a specific use case.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I see. Such a refactor is outside the scope of this PR as it would affect hundreds of entity systems.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
GetVerbsEventblock 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.Userguard is specifically what gets overridden by the user being an admin and is unique to chameleon. Even if there was a hypothetical consolidatedHasAdminSight(), there is no way to take advantage of it to shortencomponent.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 && CanInteractbe one event argument, we could also add an event argument for, say,isAdminwhich is just the result of_adminManager.HasAdminFlag(args.User, AdminFlags.Admin)), and just do|| args.isAdminto 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 ofGetVerbsEventthat I could find which would want to take advantage of that.If a refactor changes the admin flags, the presence of the
AdminFlags.Adminenum 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
There was a problem hiding this comment.
Kindly do not mark a review as resolved when it is very much not yet resolved.
This comment was marked as outdated.
This comment was marked as outdated.
|
closed unless i care again |
About the PR
Admins bypass the chameleon wearer-check introduced in #25746.
Why / Balance
The verb should not be hidden from aghosts. This probably should have been done within the linked PR; oversight.
Technical details
Splits a precondition guard in two for readability. Returns when the user is neither wearing the clothing nor an admin.
Media
Breaking changes
Changelog
🆑
ADMIN: