serenity
serenity copied to clipboard
Simplify CreateAllowedMentions API
Current API
Currently, serenity's CreateAllowedMentions builder is a thin wrapper around the Discord REST API for setting the allowed mentions in a message. The API is a bit abstruse, with things like "parse" to toggle categories of pings or "empty_parse". You also need to take special care not to build an invalid combination of options.
Furthermore, CreateAllowedMentions disallows all pings by default. This leads to the confusing situation that
send_message(http, |f| f
.content("...")
)
will enable all pings but
send_message(http, |f| f
.content("...")
.allowed_mentions(|f| f)
)
will disable all. Anyone who just wants to toggle whether an inline reply pings the original author or not needs to do this to get normal behavior:
send_message(http, |f| f
.content("...")
.allowed_mentions(|f| f
.replied_user(true_or_false)
// By providing allowed_mentions, Discord disabled _all_ pings by
// default so we need to re-enable them to get normal behavior
.parse(crate::builder::ParseValue::Everyone)
.parse(crate::builder::ParseValue::Users)
.parse(crate::builder::ParseValue::Roles)
)
)
Issue #1236 is an example of this problem.
Proposed API
This is what the methods on the CreateAllowedMentions builder could look like:
/// Allow mentions only for some roles. Overrides [`roles`] if it was previously set.
fn specific_roles(&mut self, roles: &[RoleId]);
/// Toggle mentions for roles. Overrides [`specific_roles`] if it was previously set.
fn roles(&mut self, mention: bool);
/// Allow mentions only for some users. Overrides [`users`] if it was previously set.
fn specific_users(&mut self, users: &[UserId]);
/// Toggle mentions for users. Overrides [`specific_users`] if it was previously set.
fn users(&mut self, mention: bool);
/// Toggle mention of the replied to user, if this message is an inline reply.
fn replied_user(&mut self, mention: bool);
/// Toggle @everyone mentions.
fn everyone(&mut self, mention: bool);
(The slice parameters (&[T]) would be changed into impl IntoIterator<impl Into<T>> in the serenity implementation; I wrote &[T] for simplicity)
These methods follow a common pattern, don't require an extra enum to use (ParseValue), and have intuitive names I think.
Also, this CreateAllowedMentions would start with pings enabled by default. This matches behavior in normal messages.
To make it easier to disable all mentions (not sure how common that need is), one more method could be added:
/// Turns off mentions of all kinds (user/role/@everyone mentions and reply mentions)
fn disable_all(&mut self)
Does this API seem good, or is there room for improvement? Once a new API is decided on (or decided against) I could make a PR for it.
You also need to take special care not to build an invalid combination of options.
To give an example of this which I just encountered in my code: to whitelist a specific set of mentioned users in a message, I was led to believe I need to write this:
.allowed_mentions(|f| f.users(users_to_ping).parse(serenity::ParseValue::Users))
Which seemed to work fine as long as the message contained no users to ping, but after that suddenly failed with Invalid Form Body because .parse() with Users value is mutually exclusive with .users().
With the suggested API, such an API misuse would have been impossible.
Also, I'm thinking if maybe CreateAllowedMentions should keep disabling all mentions by default. It seems like a safer default - you definitely do not want accidental @ everyone pings. Better to have some mentions failing to ping, than some malicious message managing to ping an entire server.
Another thought I had: maybe the replied_user setting should be moved out of the CreateAllowedMentions API entirely? I think allowed_mentions was an unintuitive place for Discord to put the inline reply ping toggle in the first place.
Most of the time, you consider the two separately:
- replied_user is when you send an inline reply and want to configure how it behaves
- allowed_mentions is a general safe guard against accidental ping injection vulnerabilities