alliedmodders
Fix RegConsoleCmd overwriting access requirements (bug 6330)
I believe this is correct, sanity check requested - I'm unable to test atm (will before landing).
See https://bugs.alliedmods.net/show_bug.cgi?id=6426, https://bugs.alliedmods.net/show_bug.cgi?id=6330, https://forums.alliedmods.net/showthread.php?t=265938, https://forums.alliedmods.net/showthread.php?t=260308.
This looks good. However, I'm thinking we should go a bit further by just |=ing future registrations instead of the first caller.
https://github.com/alliedmodders/sourcemod/blob/38fd55a05d1c77bb0caa53e87e91540fd10e1c3c/core/logic/AdminCache.cpp#L2071
I'm thinking we should go a bit further by just |=ing future registrations instead of the first caller.
As discussed on IRC, I think that will break more than this is fixing - it is possible this should warn or fail in the case of conflicting access though. The current behavior matches how the command description, default value, help, and engine flags operate. For now, I think it is worth landing and seeing how things play out.
Note that this doesn't affect command callbacks, those store eflags per-registration and will (and currently do) act correctly - this only affects Check(Command)Access.
EDIT: In going to add said warning message, I noticed that this has some bad interaction with UpdateAdminCmdFlags - gonna need to think about that for a bit.
@asherkin are you able to devote some ❤️ to this orphan? I (/we I think/hope 😸) still align on this being improved, do you think we can bring this to a situation where we can bring it into the tree?
I've been thinking about it for the last 5 years and I'm not sure I have a solution yet that doesn't involve rewriting a lot of the command permissions system... There is an argument for work in this area, as a lot of users expect command permissions to work more like Minecraft (?) where they're accessible to "any" assigned flag, rather than "all" assigned flags - which can't be done without more internal expressiveness here, and it is that expressiveness that would help the problem with this PR as well. Unfortunately multiple plugins registering the same command is an old beast who's ship has long sailed.