Per-server reconfiguration admins

[za419]

Add commands to control the list of per-server admins. All are only usable by admins:

• Cadence add admin <mention target>
  • Add the mentioned target to the set of admins for the current server. Duplicates generate an appropriate "Already admin" response.
• Cadence remove admin <mention target>
  • Remove the mentioned target from the set of admins for the current server. Generates errors for non-admins, oneself (to avoid mistaken retirement) and the instance admin.
• Cadence admin retire
  • Remove the sending user from the set of admins for the current server.

Add commands to inspect the list of per-server admins. These are usable globally, but can at least be configured by an admin to be disabled (Should they also be configurable to admin-only?)

• Cadence list admins
  • Mention all server admins, plus the instance admin, separately (ie: "My instance admin is @za419, and these people are admins for this server")
• Cadence is <mention> admin
  • Prints whether the target user is an administrator.

Server admins are permitted to run any admin-only command in their server. This includes ban and config, enabling them to run and customize the server to their liking. However, as part of this, a few changes need to be made (to account for the fact that multiple people can run these commands, avoid crashing the bot with bad config, and that not all configurables make sense to alter for one server):

• ban must now check whether the target user is an admin, and refuse to ban them if so (add admin does not imply unban, and these can be done in either order if necessary, but being an admin makes one ban-proof)
• config needs a mechanism to determine whether an admin command can alter a given option. My design for this is a flag "editable" which propagates to the entire subtree - If it is unset or falsy, Cadence config will refuse to alter it (printing it is probably okay though). This is set in the two per-instance config files default-config.json and config.json, allowing the instance admin to override it if desired. Propagation is performed at load time (during the defaulting of config over default-config). The editable flag shall not be saved in persistent-config, and it is itself implicitly non-editable.
  • This ensures that for example the ID of the instance admin, the streaming source, or the timer for the automatic now-playing presence are not edited (They will probably just get checked against the global default config which results from the merge of default-config and config, but it's still not sane to pretend they're mutable per-server).
• config also needs some defenses to prevent server admins from doing things which endanger the bot's stability. Ideas (not mutually exclusive):
  • Add a "persistent" option to config objects, which governs the result of deleteing them: ie if you delete customCommands.equalTo, it will revert to an empty object - Which will avoid crashing the bot, but will behave as if it was removed entirely.
  • Do a review of config consumption to ensure that no editable setting will affect other servers if set to an invalid value.
  • Do a review of config consumption to ensure that no input will leak into executed code.
  • Do a pass of exception safety around config consumption.
  • Wrap command itself in a try/catch block as a last line of defense
  • Suggestions?

Question: Should the list of server admins be stored outside of config? i.e. should performing a restore of a backed-up server config restore the list of server admins as of the backup?

---

@kenellorando As you are the only person besides myself who admins a server with CadenceBot, you are definitely the launch customer for this project - please do feel free to comment on the design or request changes for this and the other server-configurable CadenceBot issues (Perhaps especially #42 )

[kenellorando]

@za419

Cadence remove admin <mention target> Remove the mentioned target from the set of admins for the current server. Generates errors for non-admins, oneself (to avoid mistaken retirement) and the instance admin.

Perhaps we can also consider another configuration option to disallow regular-admins attempting removals on other regular-admins, or regular-admins making more admins. Essentially, this empowers server owners to reduce the risks of accidental admin creation or admins "going rogue" or getting hacked and messing up the server config.

Cadence admin retire

I like this, but because the remove admin command returns an error after running on oneself, admin retire should also prompt a self-removal verification for safety. Seems easier to type the retirement command alone than to type the remove command + one's name.

Should [admin list commands] also be configurable to be admin-only?

I say no, but that is a personal preference. I see there being safety with the option of hiding who the admins are, but I think everyone has the right to know who has what power.

Should the list of server admins be stored outside of config? i.e. should performing a restore of a backed-up server config restore the list of server admins as of the backup?

I generally think no, just leave the admins in the file. I would consider the list of admins part of a bot's configuration. I see the value in keeping it separate though, but I should ask though what does the process look like for "restoring" a backed-up server config?

[za419]

In order:

1. Are you suggesting a mechanism to set a (list of) "super-admins"? Thus leaving us with four classes:

   1. The instance (global) admin is implicitly super admin, and cannot be deposed except by manually editing the config file (The King)
   2. Each server has one/several super admin(s), which can do anything an admin can do, plus they can add or remove super and regular admins, and perform backup-reset-restore operations (As per below in point #4) (The aristocracy)
   3. Each server has some number (Between zero and infinity) of admins, who can access administrative commands - Banning or unbanning users, altering the server configuration for additional custom commmands, altered help text, alternate command names, and potentially in the future alternate response strings (ie reconfiguring the message for 'no results' from a search) (The bourgeoisie)
   4. Each server then has some number (between zero and infinity) of users, who can only interact with Cadence via commands (play, stop, search, request, library, now playing, and help), and custom commands (the working class/the serfs, depending on which part of history you choose this metaphor to apply to)
      (I don't feel like figuring out this Markdown, this is no longer under iv) That would be possible, and I can see the value in this. Do you think this is needed in #45 or a new issue? Do you have a recommended interface (Off the top of my head - Cadence set super admin/Cadence remove super admin? Do we need to handle the corner case of transitioning super=>admin? If we set an admin to super, do they retain admin rights when de-supered? Do we need to add a variant of retire to perform super=>admin for self, given that normal retire would de-admin entirely?)

2. Are you asking for Cadence admin retire @za419, or a two-step retirement command? I figured that it was unlikely enough that one would accidentally type a retire-self that this would be unnecessary. I wouldn't mind the first one, given that we then generate an error for Cadence admin retire @someoneElse ("Go use Cadence remove admin if super"). The latter is a bit more tricky just because CadenceBot is (mostly) stateless with respect to messages - The only real state is the server-level array of last-searched-songs for requests - This would require another large piece of global state (an array of per-server arrays of admins whose last message to Cadence was a retire request, for whom we're waiting to see something like Cadence admin confirm retire). Not that that's impossible of course - I simply find it wise to pause before adding something like that.

3. I agree that the users should have the right to know who's an admin. I will leave out configuring this then.

4. "Restoring" here has the meaning of issuing the command Cadence config restore, as defined in #43. That is to say, "Please replace my config with the last time I asked you to make a copy of it"
   I now notice I missed the opportunity to spec a Cadence config backup in #43, as per the current spec one would need to reset their config to make a copy - It isn't possible to make a copy but keep what you have. I am not sure why I made this omission, as it seems intentional... But I will go add it now.
   The main drawback here to me is the potential for surprise - Since we don't really know when the last backup was taken, it would be entirely possible to run Cadence config restore, and find it's older than expected - But now you're unable to fix it, because the backup is so old that you're no longer an admin. Alternatively, if an admin is removed for misbehavior, a backup could inadvertently restore their powers.
   This is somewhat similar in reasoning to the (not explicitly called-out) design that bans do not necessarily participate in backup-and-restore (dynamic bans, ie Cadence ban @JakobFrank, are saved to a file named bans.json, which is not a participant in backup-and-restore - Technically. Since bans gets loaded into persistent-config, the current implicit behavior is for the set of bans post-restore to be all users who are banned by either the newly-restored config or the dynamic banlist that did not participate in restore - But this is a less severe issue because bans are (typically intended to be) time-limited, so a ban until "Tuesday Jan 10 2021 10:00:00 AM UTC" that gets restored in February 2022 won't bother anyone) - There are less 'gotchas' involved if the admin list is the same pre- and post-restore (Another example: What if the backup only has one admin, who has since left the server? After restore, we now are in a state where we won't trigger admin onboarding, and no one has rights to perform any administrative operation except the instance admin - We have to depend on them to come fix our server).

[kenellorando]

1. Sorry, I wasn't trying to suggest different levels of admin access. I was imagining three levels:

   1. Instance Admin - Exactly as you mention (I'll call it IA)
   2. Regular Admins - They are a single level, but their specific access can be tuned by the IA to have the set of permissions the IA thinks they should have. Whether that includes permissions to add/remove other admins, ban/unban, server config, backup/reset, etc. is up to the IA. If the instance admin thinks his or her server admins should have that power stays with them. Just a suggestion, as it sounds tedious to implement.
   3. Regular Users - Exactly as you mention

2. I was thinking two-step verification because the remove admin is designed to error if run on oneself (as you mentioned was to "avoid mistaken retirement"), whereas there's no similar consistency when typing admin retire. Let's say someone types remove admin <self> to which the bot replies, "You can't remove yourself, d-dummy!" From the user's perspective, they might expect some check if they get curious and type admin retire. Of course, I understand that Discord two-step is tedious too, though my opinion is it may lead to some accidental self removals without it.

3. [<- There is a"4" typed here but Github formats this as another list item.] Yeah, the only drawback I could perceive was the accidental restoration of a banned admin. I still don't have any strong opinion yet on the issue of config restores. Maybe we can leave it a matter to the IA to create a backup as soon as they ban someone? Or maybe automatically backup after any ban.

[za419]

1. I see - You're suggesting a more fine-grained design, where one receives a role rather than a more generic 'admin' status. That is probably more tedious, especially on the interface side - We require a lot more commands for that. I suppose it's a greater burden on resources, but that shouldn't be an issue. So tedious yes, difficult no - If you would make use of the increased granularity, I would be willing to implement it. Perhaps then admin = (all permissions), and then admins can execute something like Cadence add permission ban @kenellorando - Simply to remove the condition of "If I can grant anyone roles, do I have to grant myself ban rights"? Or in other words - The super-admin design, but instead of a standard admin class regular users can be granted permission to use particular admin-level commands.

2. I see your point. I will at least attempt to implement this - If the implementation of the retire-and-verify flow is too complicated I'll back out from it, but I can envision the value in having it.

4. Exactly - I'm nervous about a backup being able to accidentally grant rights to bad actors, or even deny rights to good admins. Backing up after a ban is a decent solution - But then not restoring the bans/permissions is equivalent, and gives us the added power that we control what the backup is (Since the design only allows one backup of config, backing up is inherently destructive - It erases our existing backup.) So my concept of the backup/restore process is that it affects how CadenceBot acts, but not who it interacts with. In other words - Roles and bans are properties of the user, and backup-and-restore affects properties of the bot.

[kenellorando]

-1. Not so fine grained that permissions could be defined for the individual-- how about just the 'Admin' level as a whole? Cadence admin permission add ban -> "OK, all Admins can now ban". That said, for my use case it might not be worth your time; I don't know any particularly trustworthy admins outside of you or I in a certain Discord channel.

-4. Looking at it this way now, I think it's sound to leave it separate from the config. Roles and bans for user, backup and restore for bot. I like the succinctness of that.

[za419]

1. I see what you mean. I would still like to assign at least one user that isn't the IA to be able to do things - Then we could have IA, Server Admin/SA (All permissions and rights of IA plus retirement (with some way to set the new SA), but only within the Discord server), and then n admins that have the rights granted by IA/SA - Essentially, the theory being that one can add CadenceBot to a discord server without adding me and still someone can admin it. I don't know how necessary it would be - For our usecase at least, I don't know who would have any permission besides possibly configure - IE the right to add custom commands. That is kind of an 'eh'....

4. Excellent. I will go ahead with that then.