[Core] Add `su` and `unsu` and `sudo <command>` commands to bot #3999

Drapersniper · 2020-06-21T19:17:05Z

Type

Bugfix
Enhancement
New feature

Description of the changes

This PR does a couple of things.

It adds [p]su which allows a bot owner to elevate themselves to currently bot owner level.
It adds [p]unsu which allows a bot owner to delavate their permissions back to a standard user
Makes [p]su and [p]unsu always avaliable to bot owners.
By default at bot start up the no bot owners have elevated permissions until [p]su is run

So theres a couple of things that can be done/changed here.

we could add a CLI flag to toggle this behaviour. without the CLI flag the bot would behave exactly as it does now with bot owners always in an elevated state
Add tasks to auto delevate a bot owner after N minutes

The reasoning behind this PR is that bot owners should not have elevated permissions by default and should only elevate themselves when needed (for example a user will need to run sudo on terminal to elevate their shell, and even here after a period of time the user will need to reelevate themselves).

I would appreciate any feedback on behaviour and discussion on default behaviour introduced by this PR.

resolves #3598

flaree · 2020-06-21T19:25:33Z

I feel as if the owners should have the elevated privileges by default. A lot of users may only host for their needs and this would hinder their setup, maybe a flag to have it off by default?

yamikaitou · 2020-06-21T19:28:34Z

Personally, if I think the default should be unelevated for Team Tokens. If the bot is being run with just a single owner, it should default to elevated. If Co-Owners are used, maybe elevate the Token owner and unelevate the co-owners by default?

The idea of being able to "unsudo" is nice though, save us from having to always test stuff with an alt

Jackenmen · 2020-06-21T19:37:20Z

We could put a recommendation to enable this functionality somewhere in our docs, but I really don't think we should make this a default. I also think that status-quo wins by default so IMO we would need very good reasons to change the current default behavior and they imo would need to outweight the reasons for the current behavior.

For one, we have a lot of less-knowledgeable users and this is yet another obstacle thrown at them.

I personally also don't think that [p]sudo actually necessarily has to make that much sense if your bot is in one (or few) servers. In these cases, I would be quite surprised if the person owning the bot isn't at least a very trusted mod (since mods don't have perms to add bots) and I would assume that usually they might be even admins or guild owners. Adding [p]sudo as default makes the management of the bot harder by default and considering that currently this is discussed as a cli flag, it would require the user to always put additional flags when running the bot if they want to disable this. Truth to be told, they should be running auto-restart service, but there might be some less or more valid reasons why they don't.

Last but not least, while the point of what should be the default is debatable, there definitely needs to be a way that allows you to (depending on what will be chosen by default) enable/disable this functionality.

Jackenmen · 2020-06-21T19:43:22Z

From other things:

names of the commands can still be discussed - sudo definitely makes sense for us, but it might not be the most user-friendly for people that aren't familiar with Unix
- also, there is at least one approved 3rd-party cog that already takes use of this command name
the implementation here is very important, so if this change affects someone's cogs, it's important for us to know it now

Dav-Git · 2020-06-21T20:02:54Z

I agree with what jack said. I believe this is a cool feature but the default shouldn't be changed.

As for names of the commands...
I think a "ownermode" setting would make more sense to users that are unfamiliar with unix.

This could either be made part of [p]set though this time contrary to the proposed audio changes I'd agree to having them as standalone command. Maybe [p]ownermode enable and [p]ownermode disable? Potentially a book converter instead of seperate commands.

Jackenmen · 2020-06-21T20:06:27Z

I don't have anything specific against the ownermode name in itself, but one thing to note here is that it might be a little bit long considering that owner will need to run it whenever they want to use some owner-only command (though only once within the set timeout). It is not hard to add an alias though so I guess it wouldn't necessarily be that problematic.

Drapersniper · 2020-06-21T20:06:59Z

Added [p]sudotimer which will allow bot owners to set a time so that the sudo priviledges auto expire.

Added the --enable-sudo cli.

Jackenmen · 2020-06-21T20:08:24Z

I think [p]set sudotimer (or [p]set sudotimeout) would make more sense here

redbot/core/bot.py

mikeshardmind · 2020-06-21T21:38:25Z

This can be enabled more elegantly with a kwarg to is_owner check based on my own use of similar in the past.

Jackenmen

I don't know what's the view of org members on this functionality, so I wouldn't be able to approve this either way, but here's a code review.

From things not included in review comments:

are we sure we want to call these sudo/unsudo? I do like these names myself, but as mentioned earlier, there could be better choices we (mostly you and I, I guess) didn't think of
can it possibly be important for 3rd-party cogs to know "true owners"? If it could be (and I'm quite worried it might), then we need this to be exposed in the API
- one could also say that modifying what is put in owner ids is changing behavior and therefore breaking. The importance of this needs to be determined.

redbot/core/commands/commands.py

redbot/core/core_commands.py

Drapersniper · 2020-09-28T12:42:50Z

So in theory this is ready to merge, although @TrustyJAID asked for the sudo command to accept commands, this would make it so it runs a singular command as sudo, and then it would auto remove sudo from you after, this needs to be looked into further before it is properly implemented.

redbot/core/bot.py

Drapersniper · 2020-10-01T11:23:56Z

So adding the optional arg is gonna be far more painful than antecipated, since this PR is not dependent on it i would suggest we leave that for a follow up PR and keep this PR as it is.

…tion suggestions)

…some other time

As I said, this was pseudocode so it wasn't meant to become attr name.

…eout` to `[p]sutimeout` to open up space for `[p]sudo` which would be command based akin to `[p]mock`

NOTE: this is a bit fragile but unless I missed something, it stops becoming fragile if we successfully get through bot startup and I made a note about this in the body of `owner_ids` property.

…h bot owner permissions, without enabled this permission globaly.

Drapersniper · 2021-06-02T21:36:47Z

Keeping this one open as jack spent a lot of time on it already.

Drapersniper added Type: Enhancement Something meant to enhance existing Red features. Status: Needs Discussion Needs more discussion. labels Jun 21, 2020

Drapersniper requested a review from tekulvw as a code owner June 21, 2020 19:17

Jackenmen added Category: Bot Core Type: Feature New feature or request. and removed Type: Enhancement Something meant to enhance existing Red features. labels Jun 21, 2020

Jackenmen reviewed Jun 21, 2020

View reviewed changes

redbot/core/bot.py Outdated Show resolved Hide resolved

Drapersniper added the QA: Needed label Jun 26, 2020

Jackenmen self-assigned this Sep 20, 2020

Jackenmen requested changes Sep 20, 2020

View reviewed changes

redbot/core/commands/commands.py Outdated Show resolved Hide resolved

redbot/core/core_commands.py Outdated Show resolved Hide resolved

redbot/core/core_commands.py Outdated Show resolved Hide resolved

Drapersniper requested review from aikaterna and palmtree5 as code owners September 28, 2020 12:23

Drapersniper requested a review from Jackenmen September 28, 2020 12:41

Drapersniper commented Sep 28, 2020

View reviewed changes

redbot/core/bot.py Outdated Show resolved Hide resolved

Jackenmen added this to the 3.5.0 milestone Oct 13, 2020

Jackenmen added Breaking Change Will potentially break some cogs. and removed QA: Needed labels Oct 21, 2020

Drapersniper and others added 22 commits May 13, 2021 16:37

add sudotimeout to set showsettings

d86aa0e

Update redbot/core/bot.py

e478053

welp

4dc4ce0

fix sudotimeout docstrings

8ff8552

Make use of ContextVar (Thanks Jack for the suggestion and implementa…

29bda29

…tion suggestions)

tweaks tweaks

29d7c33

fix some minor mistakes

fc8f7aa

This lookup error didn't set the values properly reverting

daa512e

properly implement _true_owner_ids ands make it a frozenset

4a8acd6

doesnt work without --enable-sudo ... too tired will look at it at …

ffa2e05

…some other time

Fix sudo disabled, simplify/refactor a bit, add comments

26b1dcb

Add TODO entries

3abac9c

Document true_owner_ids and owner_ids properties

4e1bbdc

This might be a terrible idea...

ca1afc9

Rename _sudoed_owner_ids->_elevated_owner_ids

46da60a

As I said, this was pseudocode so it wasn't meant to become attr name.

rename [p]sudo to [p]su and [p]unsudo to [p]unsu, `[p]sudotim…

0e64cf9

…eout` to `[p]sutimeout` to open up space for `[p]sudo` which would be command based akin to `[p]mock`

Try to prevent leaks for currently running contexts

f225692

NOTE: this is a bit fragile but unless I missed something, it stops becoming fragile if we successfully get through bot startup and I made a note about this in the body of `owner_ids` property.

Make the owner_ids.setter more robust

dc5c0c4

Add a proper comment for owner_ids.setter

253b350

add [p]sudo <command> which allows you to run a one off command wit…

fe2f238

…h bot owner permissions, without enabled this permission globaly.

I guess we'll have to settle for that then

77a68b6

Rename true_owner_ids->all_owner_ids

c46aa34

Jackenmen changed the title ~~[Core] Add su and unsu and sudo <comnmand> commands to bot~~ [Core] Add su and unsu and sudo <command> commands to bot May 13, 2021

Drapersniper closed this Jun 2, 2021

Drapersniper deleted the sudo branch June 2, 2021 21:49

Drapersniper restored the sudo branch June 22, 2021 19:36

Jackenmen removed this from the 3.5.0 milestone Jun 27, 2021

Jackenmen mentioned this pull request Oct 24, 2021

Allow dropping and elevating owner permissions #5419

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core] Add `su` and `unsu` and `sudo <command>` commands to bot #3999

[Core] Add `su` and `unsu` and `sudo <command>` commands to bot #3999

Drapersniper commented Jun 21, 2020 •

edited

Loading

flaree commented Jun 21, 2020

yamikaitou commented Jun 21, 2020

Jackenmen commented Jun 21, 2020

Jackenmen commented Jun 21, 2020

Dav-Git commented Jun 21, 2020

Jackenmen commented Jun 21, 2020

Drapersniper commented Jun 21, 2020

Jackenmen commented Jun 21, 2020

mikeshardmind commented Jun 21, 2020 •

edited

Loading

Jackenmen left a comment

Drapersniper commented Sep 28, 2020

Drapersniper commented Oct 1, 2020

Drapersniper commented Jun 2, 2021

[Core] Add su and unsu and sudo <command> commands to bot #3999

[Core] Add su and unsu and sudo <command> commands to bot #3999

Conversation

Drapersniper commented Jun 21, 2020 • edited Loading

Type

Description of the changes

flaree commented Jun 21, 2020

yamikaitou commented Jun 21, 2020

Jackenmen commented Jun 21, 2020

Jackenmen commented Jun 21, 2020

Dav-Git commented Jun 21, 2020

Jackenmen commented Jun 21, 2020

Drapersniper commented Jun 21, 2020

Jackenmen commented Jun 21, 2020

mikeshardmind commented Jun 21, 2020 • edited Loading

Jackenmen left a comment

Choose a reason for hiding this comment

Drapersniper commented Sep 28, 2020

Drapersniper commented Oct 1, 2020

Drapersniper commented Jun 2, 2021

[Core] Add `su` and `unsu` and `sudo <command>` commands to bot #3999

[Core] Add `su` and `unsu` and `sudo <command>` commands to bot #3999

Drapersniper commented Jun 21, 2020 •

edited

Loading

mikeshardmind commented Jun 21, 2020 •

edited

Loading