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

Deprecate permission methods which do not handle overwrites #3037

Merged
merged 4 commits into from
Nov 15, 2024

Conversation

GnomedDev
Copy link
Member

These methods are footguns and shouldn't be trusted, so let's deprecate and remove them... this also fixes the author_permissions method which got it's foot blown off.

@github-actions github-actions bot added the model Related to the `model` module. label Nov 14, 2024
@arqunis arqunis added enhancement An improvement to Serenity. fix A solution to an existing bug. labels Nov 14, 2024
Copy link
Member

@jamesbt365 jamesbt365 left a comment

Choose a reason for hiding this comment

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

Not sure how frequently threads are in the cache, (this could affect poise and will cause panics because it adds a failure point that is possible) but otherwise looks good

@github-actions github-actions bot added the cache Related to the `cache`-feature. label Nov 14, 2024
@GnomedDev GnomedDev force-pushed the fix-author-permissions branch from 3a762d1 to cf95d5d Compare November 14, 2024 23:28
@GnomedDev GnomedDev force-pushed the fix-author-permissions branch from cf95d5d to 44e2f5d Compare November 14, 2024 23:34
@GnomedDev GnomedDev merged commit ad58c3f into serenity-rs:current Nov 15, 2024
22 checks passed
@GnomedDev GnomedDev deleted the fix-author-permissions branch November 15, 2024 00:08
GnomedDev added a commit that referenced this pull request Nov 16, 2024
This follows up on #3037, and also removes the now unnecessary Option
around the GuildChannel param.
@zkxs
Copy link

zkxs commented Dec 1, 2024

@GnomedDev In my bot I'm trying to check if the bot has the 'Manage Roles' permission in a guild-wide context (so no channel involved). With the deprecation of these methods (and their removal in #3042) what is the intended way to do such a check?

Currently I'm doing this:

async fn can_assign_roles(context: &Context<'_>, guild_id: GuildId) -> Result<bool, Error> {
    let bot_id = context.framework().bot_id;
    let bot_member = guild_id.member(context, bot_id).await?;
    let permissions = bot_member.permissions(context)?;
    Ok(permissions.manage_roles())
}

But this is now deprecated and I'm struggling to find the intended alternative.

@cheesycod

This comment was marked as off-topic.

@GnomedDev
Copy link
Member Author

@zkxs the intended alternative is to use the channel you have access to from the Message Create or Interaction Create events most likely triggering this code. If this code is being triggered without user interaction in a channel, I'd like you to tell us in the serenity discord so we can discuss re-adding this with a less-footgunny name.

@zkxs
Copy link

zkxs commented Dec 4, 2024

@GnomedDev, in my specific use-cases this does happen to be triggered by slash commands, so I do have a channel ID in my Serenity context and I am able to call Guild::user_permissions_in as you suggest. However, as several permissions are independent of channels I still think it is unnecessarily strict to force the user to provide a channel ID. In some contrived cases the mandatory channel ID may even be a footgun in and of itself.

For the contrived example, say a bot needs to periodically create channels. Upon install the bot would like to check if it actually has that permission so it can notify the guild owner that some action is necessary before the bot breaks later. It is nonsensical to check for the Manage Channels permission with any of the guild's channels as context: the only check that makes sense is to check the global permission because the ability to create a channel cannot be overridden at the per-channel level. Checking in an arbitrary existing channel is incorrect, as that channel may have overridden Manage Channels to be disabled for the bot.

A less contrived scenario is if the bot code is triggered externally from Discord (for example, via a web-based management interface) and wants to check a permission. The following permissions are only meaningful in a guild-wide context, so there does not need to be a channel in scope in these cases:

  • manage channels (in the context of creating a channel)
  • manage roles
  • create expressions
  • manage expressions
  • view audit log
  • view server insights
  • manage server
  • change nickname
  • manage nicknames
  • kick members
  • ban members
  • timeout members
  • administrator

@jamesbt365
Copy link
Member

the only check that makes sense is to check the global permission because the ability to create a channel cannot be overridden at the per-channel level

You would actually be wrong here, for a majority of cases. Channels created within categories respect the permissions overwrites of the category, this also happens with manage roles, as that defines if you can mess with the permissions on a channel.

So in a sense here you are hitting your own footgun.

@zkxs
Copy link

zkxs commented Dec 4, 2024

Channels created within categories respect the permissions overwrites of the category, this also happens with manage roles, as that defines if you can mess with the permissions on a channel.

@jamesbt365 not sure if you mean Manage Channels here, as Manage Roles cannot be overridden at the channel or category level as far as I can see.

But even if you meant Manage Channels, that case is still weirdly complicated. I've just tested it, and with Manage Channels denied at global level and allowed at the category level, the user cannot create channels in the category as they are "missing permissions". Both the global grant and the category grant are required, so only checking at the category level would still be incorrect. If Manage Channels is allowed globally and denied at the category it works as expected (channels can be created elsewhere but that category cannot be touched).

@jamesbt365
Copy link
Member

jamesbt365 commented Dec 4, 2024

as Manage Roles cannot be overridden at the channel or category level as far as I can see.

It can, its just called manage permissions on the UI to better fit what it actually lets you do.

I've just tested it, and with Manage Channels denied at global level and allowed at the category level, the user cannot create channels in the category as they are "missing permissions".

You can create them, provided you set the permission overwrite correctly when you create a channel (which you can do when you create or update a channel).

mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this pull request Dec 8, 2024
This follows up on serenity-rs#3037, and also removes the now unnecessary Option
around the GuildChannel param.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cache Related to the `cache`-feature. enhancement An improvement to Serenity. fix A solution to an existing bug. model Related to the `model` module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants