From fb0e42dd2d9b93957d3ce8d9740a3e0ab603738d Mon Sep 17 00:00:00 2001 From: jamesbt365 Date: Tue, 22 Oct 2024 14:33:39 +0100 Subject: [PATCH] rework permission checks --- src/builtins/mod.rs | 4 + src/dispatch/common.rs | 215 +++++++++++++++++++++++++-------- src/structs/framework_error.rs | 13 ++ 3 files changed, 184 insertions(+), 48 deletions(-) diff --git a/src/builtins/mod.rs b/src/builtins/mod.rs index c8f4c7c1e4c..4538c0c95c8 100644 --- a/src/builtins/mod.rs +++ b/src/builtins/mod.rs @@ -173,6 +173,10 @@ pub async fn on_error( ctx.send(CreateReply::default().content(response).ephemeral(true)) .await?; } + crate::FrameworkError::PermissionFetchFailed { ctx } => { + ctx.say("An error occurred when fetching permissions.") + .await?; + } crate::FrameworkError::NotAnOwner { ctx } => { let response = "Only bot owners can call this command"; ctx.send(CreateReply::default().content(response).ephemeral(true)) diff --git a/src/dispatch/common.rs b/src/dispatch/common.rs index 6f8bcf42e04..fb713c830f5 100644 --- a/src/dispatch/common.rs +++ b/src/dispatch/common.rs @@ -2,23 +2,45 @@ use crate::serenity_prelude as serenity; +/// Used in `user_permissions`, if a user is passed to that function, their permissions will be +/// returned, in DMs the return value with be `Permissions::all()`. +struct PermissionsInfo { + /// The Permissions of the user, if requested. + user_permissions: Option, + /// The Permissions of the bot, if requested. + bot_permissions: Option, +} + /// Retrieves user permissions in the given channel. If unknown, returns None. If in DMs, returns /// `Permissions::all()`. -async fn user_permissions( - ctx: &serenity::Context, - guild_id: Option, - channel_id: serenity::ChannelId, - user_id: serenity::UserId, -) -> Option { - let guild_id = match guild_id { - Some(x) => x, - None => return Some(serenity::Permissions::all()), // no permission checks in DMs +async fn users_permissions( + ctx: crate::Context<'_, U, E>, + user_id: Option, + bot_id: Option, +) -> Option { + let guild_id = ctx.guild_id(); + let channel_id = ctx.channel_id(); + // No permission checks in DMs. + let Some(guild_id) = guild_id else { + return Some(PermissionsInfo { + user_permissions: Some(serenity::Permissions::all()), + bot_permissions: Some(serenity::Permissions::all()), + }); }; - let guild = guild_id.to_partial_guild(ctx).await.ok()?; + if let crate::Context::Application(ctx) = ctx { + // This should be present on all interactions within a guild. But discord can be a bit + // funny sometimes, so lets be safe. + if let Some(member) = &ctx.interaction.member { + return Some(PermissionsInfo { + user_permissions: member.permissions, + bot_permissions: ctx.interaction.app_permissions, + }); + } + } // Use to_channel so that it can fallback on HTTP for threads (which aren't in cache usually) - let channel = match channel_id.to_channel(ctx).await { + let channel = match channel_id.to_channel(ctx.serenity_context()).await { Ok(serenity::Channel::Guild(channel)) => channel, Ok(_other_channel) => { tracing::warn!( @@ -29,31 +51,125 @@ async fn user_permissions( Err(_) => return None, }; - let member = guild.member(ctx, user_id).await.ok()?; + // These are done by HTTP only to prevent outdated data with no GUILD_MEMBERS intent. + let user_member = if let Some(user_id) = user_id { + Some( + guild_id + .member(ctx.serenity_context(), user_id) + .await + .ok()?, + ) + } else { + None + }; - Some(guild.user_permissions_in(&channel, &member)) + let bot_member = if let Some(bot_id) = bot_id { + Some(guild_id.member(ctx.serenity_context(), bot_id).await.ok()?) + } else { + None + }; + + get_user_permissions(ctx, &channel, user_member.as_ref(), bot_member.as_ref()).await } /// Retrieves the set of permissions that are lacking, relative to the given required permission set /// /// Returns None if permissions couldn't be retrieved +async fn get_user_permissions( + ctx: crate::Context<'_, U, E>, + channel: &serenity::GuildChannel, + user: Option<&serenity::Member>, + bot: Option<&serenity::Member>, +) -> Option { + #[cfg(feature = "cache")] + if let Some(permissions) = cached_guild(ctx, channel, user, bot) { + Some(permissions) + } else { + fetch_guild(ctx, channel, user, bot).await + } + + #[cfg(not(feature = "cache"))] + fetch_guild(ctx, channel, user, bot).await +} + +#[cfg(feature = "cache")] +/// Checks the cache for the guild, returning the permissions if present. +fn cached_guild( + ctx: crate::Context<'_, U, E>, + channel: &serenity::GuildChannel, + user: Option<&serenity::Member>, + bot: Option<&serenity::Member>, +) -> Option { + ctx.guild().map(|guild| { + let user_permissions = user.map(|m| guild.user_permissions_in(channel, m)); + let bot_permissions = bot.map(|m| guild.user_permissions_in(channel, m)); + + PermissionsInfo { + user_permissions, + bot_permissions, + } + }) +} + +/// Fetches the partial guild from http, returning the permissions if available. +async fn fetch_guild( + ctx: crate::Context<'_, U, E>, + channel: &serenity::GuildChannel, + user: Option<&serenity::Member>, + bot: Option<&serenity::Member>, +) -> Option { + let partial_guild = channel.guild_id.to_partial_guild(ctx.http()).await.ok()?; + + let user_permissions = user.map(|m| partial_guild.user_permissions_in(channel, m)); + let bot_permissions = bot.map(|m| partial_guild.user_permissions_in(channel, m)); + + Some(PermissionsInfo { + user_permissions, + bot_permissions, + }) +} + +/// Retrieves the set of permissions that are lacking, relative to the given required permission set +/// +/// Returns None if permissions couldn't be retrieved. async fn missing_permissions( ctx: crate::Context<'_, U, E>, - user: serenity::UserId, - required_permissions: serenity::Permissions, -) -> Option { - if required_permissions.is_empty() { - return Some(serenity::Permissions::empty()); + user_id: serenity::UserId, + user_permissions: serenity::Permissions, + bot_id: serenity::UserId, + bot_permissions: serenity::Permissions, +) -> Option<(serenity::Permissions, serenity::Permissions)> { + // If both user and bot are None, return empty permissions + if user_permissions.is_empty() && bot_permissions.is_empty() { + return Some(( + serenity::Permissions::empty(), + serenity::Permissions::empty(), + )); } - let permissions = user_permissions( - ctx.serenity_context(), - ctx.guild_id(), - ctx.channel_id(), - user, - ) - .await; - Some(required_permissions - permissions?) + let user_id = match user_permissions.is_empty() { + true => None, + false => Some(user_id), + }; + + let bot_id = match bot_permissions.is_empty() { + true => None, + false => Some(bot_id), + }; + + // Fetch permissions, returning None if an error occurred + let permissions = users_permissions(ctx, user_id, bot_id).await?; + + let user_missing_perms = permissions + .user_permissions + .map(|permissions| user_permissions - permissions) + .unwrap_or_default(); + let bot_missing_perms = permissions + .bot_permissions + .map(|permissions| bot_permissions - permissions) + .unwrap_or_default(); + + Some((user_missing_perms, bot_missing_perms)) } /// See [`check_permissions_and_cooldown`]. Runs the check only for a single command. The caller @@ -111,35 +227,38 @@ async fn check_permissions_and_cooldown_single<'a, U, E>( } // Make sure that user has required permissions - match missing_permissions(ctx, ctx.author().id, cmd.required_permissions).await { - Some(missing_permissions) if missing_permissions.is_empty() => {} - Some(missing_permissions) => { - return Err(crate::FrameworkError::MissingUserPermissions { - ctx, - missing_permissions: Some(missing_permissions), - }) - } - // Better safe than sorry: when perms are unknown, restrict access - None => { + if let Some((user_missing_permissions, bot_missing_permissions)) = missing_permissions( + ctx, + ctx.author().id, + cmd.required_permissions, + ctx.framework().bot_id(), + cmd.required_bot_permissions, + ) + .await + { + if !user_missing_permissions.is_empty() { return Err(crate::FrameworkError::MissingUserPermissions { ctx, - missing_permissions: None, - }) + missing_permissions: Some(user_missing_permissions), + }); } - } - // Before running any pre-command checks, make sure the bot has the permissions it needs - match missing_permissions(ctx, ctx.framework().bot_id(), cmd.required_bot_permissions).await { - Some(missing_permissions) if missing_permissions.is_empty() => {} - Some(missing_permissions) => { + if !bot_missing_permissions.is_empty() { return Err(crate::FrameworkError::MissingBotPermissions { ctx, - missing_permissions, - }) + missing_permissions: bot_missing_permissions, + }); } - // When in doubt, just let it run. Not getting fancy missing permissions errors is better - // than the command not executing at all - None => {} + + // missing premission checks here. + } else { + // TODO: ask what I should do here because combining the checks loses the verbosity. + // the only previous failure point was it failing to get the guild, channel or members. + // Previously when a bots permissions could not be fetched it would just allow execution. + return Err(crate::FrameworkError::MissingUserPermissions { + missing_permissions: None, + ctx, + }); } // Only continue if command checks returns true diff --git a/src/structs/framework_error.rs b/src/structs/framework_error.rs index f20e44d1df6..94d9388e684 100644 --- a/src/structs/framework_error.rs +++ b/src/structs/framework_error.rs @@ -114,6 +114,11 @@ pub enum FrameworkError<'a, U, E> { /// General context ctx: crate::Context<'a, U, E>, }, + #[non_exhaustive] + PermissionFetchFailed { + /// General context + ctx: crate::Context<'a, U, E>, + }, /// A non-owner tried to invoke an owners-only command #[non_exhaustive] NotAnOwner { @@ -218,6 +223,7 @@ impl<'a, U, E> FrameworkError<'a, U, E> { Self::CooldownHit { ctx, .. } => ctx.serenity_context(), Self::MissingBotPermissions { ctx, .. } => ctx.serenity_context(), Self::MissingUserPermissions { ctx, .. } => ctx.serenity_context(), + Self::PermissionFetchFailed { ctx } => ctx.serenity_context(), Self::NotAnOwner { ctx, .. } => ctx.serenity_context(), Self::GuildOnly { ctx, .. } => ctx.serenity_context(), Self::DmOnly { ctx, .. } => ctx.serenity_context(), @@ -242,6 +248,7 @@ impl<'a, U, E> FrameworkError<'a, U, E> { Self::CooldownHit { ctx, .. } => ctx, Self::MissingBotPermissions { ctx, .. } => ctx, Self::MissingUserPermissions { ctx, .. } => ctx, + Self::PermissionFetchFailed { ctx } => ctx, Self::NotAnOwner { ctx, .. } => ctx, Self::GuildOnly { ctx, .. } => ctx, Self::DmOnly { ctx, .. } => ctx, @@ -367,6 +374,11 @@ impl std::fmt::Display for FrameworkError<'_, U, E> { missing_permissions, full_command_name!(ctx), ), + Self::PermissionFetchFailed { ctx } => write!( + f, + "An error occurred when trying to fetch permissions for `{}`", + full_command_name!(ctx) + ), Self::NotAnOwner { ctx } => write!( f, "owner-only command `{}` cannot be run by non-owners", @@ -436,6 +448,7 @@ impl<'a, U: std::fmt::Debug, E: std::error::Error + 'static> std::error::Error Self::CooldownHit { .. } => None, Self::MissingBotPermissions { .. } => None, Self::MissingUserPermissions { .. } => None, + Self::PermissionFetchFailed { .. } => None, Self::NotAnOwner { .. } => None, Self::GuildOnly { .. } => None, Self::DmOnly { .. } => None,