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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/cache/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ use crate::model::prelude::*;
mod cache_update;
mod event;
mod settings;
mod wrappers;
pub(crate) mod wrappers;

#[cfg(feature = "temp_cache")]
pub(crate) use wrappers::MaybeOwnedArc;
Expand Down
2 changes: 1 addition & 1 deletion src/cache/wrappers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use typesize::TypeSize;

#[derive(Debug)]
/// A wrapper around Option<DashMap<K, V>> to ease disabling specific cache fields.
pub(crate) struct MaybeMap<K: Eq + Hash, V>(pub(super) Option<DashMap<K, V, BuildHasher>>);
pub(crate) struct MaybeMap<K: Eq + Hash, V>(pub(crate) Option<DashMap<K, V, BuildHasher>>);
impl<K: Eq + Hash, V> MaybeMap<K, V> {
pub fn iter(&self) -> impl Iterator<Item = RefMulti<'_, K, V, BuildHasher>> {
Option::iter(&self.0).flat_map(DashMap::iter)
Expand Down
85 changes: 83 additions & 2 deletions src/model/channel/message.rs
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@ impl Message {
///
/// This may return `None` if:
/// - The [`Cache`] does not have the current [`Guild`]
/// - The [`Guild`] does not have the current channel cached (should never happen).
/// - This message is not from [`MessageCreateEvent`] and the author's [`Member`] cannot be
/// found in [`Guild#structfield.members`].
#[cfg(feature = "cache")]
Expand All @@ -229,10 +230,18 @@ impl Message {
};

let guild = cache.as_ref().guild(guild_id)?;
let channel = if let Some(channel) = guild.channels.get(&self.channel_id) {
channel
} else if let Some(thread) = guild.threads.iter().find(|th| th.id == self.channel_id) {
thread
} else {
return None;
};

if let Some(member) = &self.member {
Some(guild.partial_member_permissions(self.author.id, member))
Some(guild.partial_member_permissions_in(channel, self.author.id, member))
} else {
Some(guild.member_permissions(guild.members.get(&self.author.id)?))
Some(guild.user_permissions_in(channel, guild.members.get(&self.author.id)?))
}
}

Expand Down Expand Up @@ -1443,3 +1452,75 @@ pub struct PollAnswerCount {
pub count: u64,
pub me_voted: bool,
}

// all tests here require cache, move if non-cache test is added
#[cfg(all(test, feature = "cache"))]
mod tests {
use std::collections::HashMap;

use dashmap::DashMap;

use super::{
Guild,
GuildChannel,
Member,
Message,
PermissionOverwrite,
PermissionOverwriteType,
Permissions,
User,
UserId,
};
use crate::cache::wrappers::MaybeMap;
use crate::cache::Cache;

/// Test that author_permissions checks the permissions in a channel, not just the guild.
#[test]
fn author_permissions_respects_overwrites() {
// Author of the message, with a random ID that won't collide with defaults.
let author = User {
id: UserId::new(50778944701071),
..Default::default()
};

// Channel with the message, with SEND_MESSAGES on.
let channel = GuildChannel {
permission_overwrites: vec![PermissionOverwrite {
allow: Permissions::SEND_MESSAGES,
deny: Permissions::default(),
kind: PermissionOverwriteType::Member(author.id),
}],
..Default::default()
};
let channel_id = channel.id;

// Guild with the author and channel cached, default (empty) permissions.
let guild = Guild {
channels: HashMap::from([(channel.id, channel)]),
members: HashMap::from([(author.id, Member {
user: author.clone(),
..Default::default()
})]),
..Default::default()
};

// Message, tied to the guild and the channel.
let message = Message {
author,
channel_id,
guild_id: Some(guild.id),
..Default::default()
};

// Cache, with the guild setup.
let mut cache = Cache::new();
cache.guilds = MaybeMap(Some({
let guilds = DashMap::default();
guilds.insert(guild.id, guild);
guilds
}));

// The author should only have the one permission, SEND_MESSAGES.
assert_eq!(message.author_permissions(&cache), Some(Permissions::SEND_MESSAGES));
}
}
3 changes: 3 additions & 0 deletions src/model/guild/member.rs
Original file line number Diff line number Diff line change
Expand Up @@ -435,8 +435,11 @@ impl Member {
/// And/or returns [`ModelError::ItemMissing`] if the "default channel" of the guild is not
/// found.
#[cfg(feature = "cache")]
#[deprecated = "Use Guild::member_permissions_in, as this doesn't consider permission overwrites"]
pub fn permissions(&self, cache: impl AsRef<Cache>) -> Result<Permissions> {
let guild = cache.as_ref().guild(self.guild_id).ok_or(ModelError::GuildNotFound)?;

#[allow(deprecated)]
Ok(guild.member_permissions(self))
}

Expand Down
4 changes: 4 additions & 0 deletions src/model/guild/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,8 @@ impl Guild {
required_permissions: Permissions,
) -> Result<(), Error> {
if let Some(member) = self.members.get(&cache.current_user().id) {
// This isn't used for any channel-specific permissions, but sucks still.
#[allow(deprecated)]
let bot_permissions = self.member_permissions(member);
if !bot_permissions.contains(required_permissions) {
return Err(Error::Model(ModelError::InvalidPermissions {
Expand Down Expand Up @@ -1908,6 +1910,7 @@ impl Guild {
/// Calculate a [`Member`]'s permissions in the guild.
#[inline]
#[must_use]
#[deprecated = "Use Guild::member_permissions_in, as this doesn't consider permission overwrites"]
pub fn member_permissions(&self, member: &Member) -> Permissions {
Self::user_permissions_in_(
None,
Expand All @@ -1926,6 +1929,7 @@ impl Guild {
/// Panics if the passed [`UserId`] does not match the [`PartialMember`] id, if user is Some.
#[inline]
#[must_use]
#[deprecated = "Use Guild::partial_member_permissions_in, as this doesn't consider permission overwrites"]
pub fn partial_member_permissions(
&self,
member_id: UserId,
Expand Down
2 changes: 2 additions & 0 deletions src/model/guild/partial_guild.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1035,6 +1035,7 @@ impl PartialGuild {
/// Calculate a [`Member`]'s permissions in the guild.
#[inline]
#[must_use]
#[deprecated = "Use PartialGuild::member_permissions_in, as this doesn't consider permission overwrites"]
pub fn member_permissions(&self, member: &Member) -> Permissions {
Guild::user_permissions_in_(
None,
Expand All @@ -1053,6 +1054,7 @@ impl PartialGuild {
/// Panics if the passed [`UserId`] does not match the [`PartialMember`] id, if user is Some.
#[inline]
#[must_use]
#[deprecated = "Use PartialGuild::partial_member_permissions_in, as this doesn't consider permission overwrites"]
pub fn partial_member_permissions(
&self,
member_id: UserId,
Expand Down
Loading