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.

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

Add ability for admins to change certain cvars via command. #35105

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

Simyon264
Copy link
Contributor

@Simyon264 Simyon264 commented Feb 12, 2025

Requires space-wizards/RobustToolbox#5674

About the PR

Adds a command that downstreams can use to allow more fine grained control about cvar changes.

Why / Balance

Currently, the only way to change CVars is via the config files or the cvar command which is +HOST only. +HOST is a dangerous permission to give. This PR adds a framework to allow certain CVars to be changed simply via the changecvar content command. The CVars define their own min and max as well as the AdminFlag required to execute the command.

This is to allow config changes (such as playercount, for example) from admins that do not have +HOST. It will also reduce boilerplate code (see https://github.com/space-wizards/space-station-14/blob/master/Content.Server/Administration/Commands/PanicBunkerCommand.cs as an example). This PR does not actually add any CVars to the list as this is only the framework to do it.

Possible CVars to allow changing in the future:

  • The CVars that change when running the clientsidemappingsetup command.
  • Panic punker settings
  • MOTD
  • Other CVars that have simple command switches.

Technical details

This PR adds a new command. The command fetches all CVars that can be changed via reflection once autocomplete is attempted. Why not on command creation? IPostInjectInit does not trigger on commands and dependencies are not available on construction.

Media

Screenshots are slightly outdated.
image
image
image
image

Requirements

Breaking changes

(This is not breaking changes, just an FYI for forks!)
A new command changecvar has been added to allow non +HOST admins to change CVar values. To achieve this add the [CVarControl] attribute. Example:

namespace Content.Shared.MyAwesomeFork.CCVar;

[CVarDefs]
public sealed partial class MyAwesomeForkCCVars : CVars
{
    /// <summary>
    /// If true, makes people sell their souls on connection.
    /// </summary>
	[CVarControl(AdminFlags.Admin)]
    public static readonly CVarDef<bool> GameAwesomeForkSellSoulOnJoin =
        CVarDef.Create("game.awesome_fork.sell_soul_on_join", true, CVar.SERVERONLY);

    /// <summary>
    /// Whats the chance that the soul gets consumed?
    /// </summary>
	[CVarControl(AdminFlags.Server, min: 0, max: 100)]
    public static readonly CVarDef<int> GameAwesomeForkSellSoulOnJoin =
        CVarDef.Create("game.awesome_fork.chance_to_consume_soul", 20, CVar.SERVERONLY);
}

min and max currently support the following values: int, float, long and ushort.

@github-actions github-actions bot added S: Needs Review Status: Requires additional reviews before being fully accepted size/M Denotes a PR that changes 100-999 lines. S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Feb 12, 2025
@Simyon264 Simyon264 added P1: High Priority: Higher priority than other items, but isn't an emergency. S: Needs Engine PR Merged Status: Requires an existing Robust Toolbox PR to be merged first. D1: High Difficulty: Extensive codebase knowledge required. S: Needs Engine Update Status: Requires a newer version of the Robust Toolbox engine. A: Admin Tooling Area: Admin tooling and moderation. T: Of Admin Interest Type: Affects administration work a lot, and might require admins to weigh in on S: Approved Status: Reviewed and approved by at least one maintainer; a PR may require another approval. and removed S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Feb 12, 2025
@FairlySadPanda
Copy link
Contributor

You've listed the reason as:

"This is useful if you do not want to give everyone +HOST for simple playercount changes."

Could you explain why this is tied to admin privilages and not its own privilage? Do all admins need the ability to do this?

@Simyon264
Copy link
Contributor Author

Every CVar can choose its own permission flag. The command is there so you don't have 30 commands that all just flip a cvar. I just listed this as an example.
A much better example would be something like the motd, which is currently a cvar. There is the command "set-motd". All it does is check for a permission flag and then sets the cvar to the args. "set-motd" requires it's own class, dependencies and code. This PR enables the option to circumvent creating a whole nother' command for simply changing the motd. All you would need to do is add one single attribute.

@Simyon264
Copy link
Contributor Author

This PR was made specifically after Slam wanted a way to playtest damage values via CVars. He then brought up making a set of commands (Much boilerplate) to change the damage values. I suggested making something like this instead, to which he requested me to do as he was going on vacation. And now here we are.

Not needed as the command checks for the perms.
Copy link
Member

@SlamBamActionman SlamBamActionman left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for the help! ❤️

Copy link
Member

@SlamBamActionman SlamBamActionman left a comment

Choose a reason for hiding this comment

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

Search tested nicely! Good QoL!

LoC does not work at the earlier stages of server initalization
We clear out list so its no longer needed
@0x6273
Copy link
Contributor

0x6273 commented Feb 14, 2025

Why is a new command needed for this? Can't you just make the existing command not require HOST ?

@Simyon264
Copy link
Contributor Author

The existing command is in engine and admin flags are in content.

@Simyon264
Copy link
Contributor Author

To further explain:
The base CVar command contains no checks for admin flags. The only way it's gated is behind https://github.com/space-wizards/space-station-14/blob/master/Resources/engineCommandPerms.yml#L130. You can either give a person the permissions to change all CVars or none.
Botching more access control into the engine for this one specific command is unnecessary work, a new command made specifically in content is much more pratical.

@0x6273
Copy link
Contributor

0x6273 commented Feb 14, 2025

Having two commands that have the exact same function but with different permissions seems a bit hacky. Wouldn't it be better to have a permissions system in the engine instead?

@Simyon264
Copy link
Contributor Author

Yes, I agree that the commands share functionality, but implemting a permission system in RT is a much higher workload and might not even be watned. Also RT supports singleplayer. How would it work with that?

@PJB3005
Copy link
Member

PJB3005 commented Feb 14, 2025

Yeah this isn't a feature that should be in RT.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: Admin Tooling Area: Admin tooling and moderation. D1: High Difficulty: Extensive codebase knowledge required. P1: High Priority: Higher priority than other items, but isn't an emergency. S: Approved Status: Reviewed and approved by at least one maintainer; a PR may require another approval. S: Needs Engine PR Merged Status: Requires an existing Robust Toolbox PR to be merged first. S: Needs Engine Update Status: Requires a newer version of the Robust Toolbox engine. S: Needs Review Status: Requires additional reviews before being fully accepted size/M Denotes a PR that changes 100-999 lines. T: Of Admin Interest Type: Affects administration work a lot, and might require admins to weigh in on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants