rework permission checks

src/builtins/mod.rs

@@ -173,6 +173,10 @@ pub async fn on_error<U, E: std::fmt::Display + std::fmt::Debug>(
             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))

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<serenity::Permissions>,
+    /// The Permissions of the bot, if requested.
+    bot_permissions: Option<serenity::Permissions>,
+}
+
 /// 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<serenity::GuildId>,
-    channel_id: serenity::ChannelId,
-    user_id: serenity::UserId,
-) -> Option<serenity::Permissions> {
-    let guild_id = match guild_id {
-        Some(x) => x,
-        None => return Some(serenity::Permissions::all()), // no permission checks in DMs
+async fn users_permissions<U, E>(
+    ctx: crate::Context<'_, U, E>,
+    user_id: Option<serenity::UserId>,
+    bot_id: Option<serenity::UserId>,
+) -> Option<PermissionsInfo> {
+    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<U, E>(
+    ctx: crate::Context<'_, U, E>,
+    channel: &serenity::GuildChannel,
+    user: Option<&serenity::Member>,
+    bot: Option<&serenity::Member>,
+) -> Option<PermissionsInfo> {
+    #[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<U, E>(
+    ctx: crate::Context<'_, U, E>,
+    channel: &serenity::GuildChannel,
+    user: Option<&serenity::Member>,
+    bot: Option<&serenity::Member>,
+) -> Option<PermissionsInfo> {
+    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<U, E>(
+    ctx: crate::Context<'_, U, E>,
+    channel: &serenity::GuildChannel,
+    user: Option<&serenity::Member>,
+    bot: Option<&serenity::Member>,
+) -> Option<PermissionsInfo> {
+    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<U, E>(
     ctx: crate::Context<'_, U, E>,
-    user: serenity::UserId,
-    required_permissions: serenity::Permissions,
-) -> Option<serenity::Permissions> {
-    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

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<U, E: std::fmt::Display> 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,