Allow dropping and elevating owner permissions #5419
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: jack1142 <6032823+jack1142@users.noreply.github.com>
…tion suggestions)
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.
Jackenmen
added
Breaking Change
github-actions
bot
added
Category: Cogs - Audio
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of the changes
Supersedes #3999, fixes #3598
New user features
--enable-sudoflag that enables this whole functionality[p]su/[p]unsucommands that allow the bot owner to elevate/de-elevate their bot owner permissions[p]sucommand will automatically de-elevate after a configurable timeout (15 minutes by default) passes[p]sudo <command>command that allows the bot owner to run a specific command with bot owner permissions while keeping de-elevated permissions for everything elseImplementation
The feature is quite cool, I must say that... I cannot however say the same about the way this is (and probably how it has to be) implemented.
On the high level, this implementation provides 2 separate properties:
all_owner_idsandowner_ids. The former is a simple (frozen) set which is pretty much the same as what we have in current stable releases underowner_idsproperty.owner_idsin this implementation is where the magic happens. It magically provides you IDs of owners that are elevated in the current context and it is used for all kinds of bot owner checks likebot.is_owner(),@commands.is_owner(), etc. This all sounds relatively straightforward until we dive into how all of this "magic" is done.Internally, we have a
ContextVarthat keeps track of owners with elevated privileges in the different contexts. Wheneverowner_idsis accessed, we get the current value of that context var, or if it's not set, we use the current "globally" elevated owner IDs (as in the ones that are currently elevated through[p]sucommand). The context var is set for each processed command which means that within the context of the command, it is a constant, and the changes that are made with[p]su/unsu/sudowhile it runs do not affect it. Becauseasyncio.Tasks have their own context, setting a value of a context var does not set that value in the global context and therefore accessing the global context will keep giving us the current "globally" elevated owner IDs as the default.So based on this, we can expect that whenever the user runs the command, the command will get the currently elevated owner IDs when it tries to access
owner_idsand the changes made to the global context during its runtime will not affect it. For better or worse, this however also means that any tasks spawned within that command will inherit the value ofowner_idsthat was set for it right before its execution. This is quite intentional for short-lived tasks like waiting for a message or reaction but might not be something that cog creator wants when trying to spawn a long-lived task that needs to access the currently elevated owner ids. Another somewhat similar problem is that the same long-lived tasks can also be created on cog load and they inevitably will inherit the value ofowner_idsthat was set for[p]load.Currently, this is a limitation that I have not really tried addressing (and I don't exactly have an idea how to address it) and I'm not sure how much we can do if we don't want to make APIs completely impractical.