Skip to content

Commit

Permalink
fix: Updated dpp::role comparison operators. They now throw std::inva…
Browse files Browse the repository at this point in the history
…lid_argument when roles of two different guilds
  • Loading branch information
Commandserver committed Nov 12, 2024
1 parent 8c93966 commit b0c53bc
Show file tree
Hide file tree
Showing 6 changed files with 92 additions and 14 deletions.
28 changes: 28 additions & 0 deletions docpages/example_programs/misc/checking-member-permissions.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,34 @@ if (c && c->get_user_permissions(member).can(dpp::p_send_messages)) {
}
```
### Role Hierarchy
The recommended and correct way to compare for roles in the hierarchy is using the comparison operators (`<`, `>`) on the \ref dpp::role::operator<(dpp::role, dpp::role) "dpp::role" objects themselves.
Keep in mind that multiple roles can have the same position number.
As a result, comparing roles by position alone can lead to subtle bugs when checking for role hierarchy.
For example let's say you have a ban command, and want to make sure that any issuer of the command can only ban members lower position than their own highest role.
```cpp
bot.on_interaction_create([](const dpp::interaction_create_t& event) {
dpp::snowflake target_id = std::get<dpp::snowflake>(event.get_parameter("user"));
dpp::guild_member target = event.command.get_resolved_member(target_id);
for (dpp::snowflake issuer_role_id : event.command.member.get_roles()) {
auto issuer_role = dpp::find_role(issuer_role_id);
if (issuer_role == nullptr) continue;
for (dpp::snowflake target_role_id : target.get_roles()) {
auto target_role = dpp::find_role(target_role_id);
if (target_role == nullptr) continue;
if (target_role > issuer_role) {
event.reply("You can't ban someone whose role is higher than yours!");
return;
}
}
}
});
```

## Permissions in Interaction Events

### Default Command Permissions
Expand Down
8 changes: 4 additions & 4 deletions include/dpp/cluster.h
Original file line number Diff line number Diff line change
Expand Up @@ -2170,8 +2170,8 @@ class DPP_EXPORT cluster {
* @note This method supports audit log reasons set by the cluster::set_audit_reason() method.
* @param c Channel to set permissions for
* @param overwrite_id Overwrite to change (a user or role ID)
* @param allow allow permissions bitmask
* @param deny deny permissions bitmask
* @param allow Bitmask of allowed permissions (refer to enum dpp::permissions)
* @param deny Bitmask of denied permissions (refer to enum dpp::permissions)
* @param member true if the overwrite_id is a user id, false if it is a channel id
* @param callback Function to call when the API call completes.
* On success the callback will contain a dpp::confirmation object in confirmation_callback_t::value. On failure, the value is undefined and confirmation_callback_t::is_error() method will return true. You can obtain full error details with confirmation_callback_t::get_error().
Expand All @@ -2185,8 +2185,8 @@ class DPP_EXPORT cluster {
* @note This method supports audit log reasons set by the cluster::set_audit_reason() method.
* @param channel_id ID of the channel to set permissions for
* @param overwrite_id Overwrite to change (a user or role ID)
* @param allow allow permissions bitmask
* @param deny deny permissions bitmask
* @param allow Bitmask of allowed permissions (refer to enum dpp::permissions)
* @param deny Bitmask of denied permissions (refer to enum dpp::permissions)
* @param member true if the overwrite_id is a user id, false if it is a channel id
* @param callback Function to call when the API call completes.
* On success the callback will contain a dpp::confirmation object in confirmation_callback_t::value. On failure, the value is undefined and confirmation_callback_t::is_error() method will return true. You can obtain full error details with confirmation_callback_t::get_error().
Expand Down
8 changes: 4 additions & 4 deletions include/dpp/cluster_sync_calls.h
Original file line number Diff line number Diff line change
Expand Up @@ -590,8 +590,8 @@ DPP_DEPRECATED("Please use coroutines instead of sync functions: https://dpp.dev
* @note This method supports audit log reasons set by the cluster::set_audit_reason() method.
* @param c Channel to set permissions for
* @param overwrite_id Overwrite to change (a user or role ID)
* @param allow allow permissions bitmask
* @param deny deny permissions bitmask
* @param allow Bitmask of allowed permissions (refer to enum dpp::permissions)
* @param deny Bitmask of denied permissions (refer to enum dpp::permissions)
* @param member true if the overwrite_id is a user id, false if it is a channel id
* @return confirmation returned object on completion
* \memberof dpp::cluster
Expand All @@ -610,8 +610,8 @@ DPP_DEPRECATED("Please use coroutines instead of sync functions: https://dpp.dev
* @note This method supports audit log reasons set by the cluster::set_audit_reason() method.
* @param channel_id ID of the channel to set permissions for
* @param overwrite_id Overwrite to change (a user or role ID)
* @param allow allow permissions bitmask
* @param deny deny permissions bitmask
* @param allow Bitmask of allowed permissions (refer to enum dpp::permissions)
* @param deny Bitmask of denied permissions (refer to enum dpp::permissions)
* @param member true if the overwrite_id is a user id, false if it is a channel id
* @return confirmation returned object on completion
* \memberof dpp::cluster
Expand Down
24 changes: 23 additions & 1 deletion include/dpp/role.h
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,10 @@ class DPP_EXPORT role : public managed, public json_interface<role> {

/**
* @brief Role position.
*
* @warning multiple roles can have the same position number.
* As a result, comparing roles by position alone can lead to subtle bugs when checking for role hierarchy.
* The recommended and correct way to compare for roles in the hierarchy is using the comparison operators on the role objects themselves.
*/
uint8_t position{0};

Expand Down Expand Up @@ -320,8 +324,24 @@ class DPP_EXPORT role : public managed, public json_interface<role> {
* @param lhs first role to compare
* @param rhs second role to compare
* @return true if lhs is less than rhs
*
* @throws std::invalid_argument when roles of two different guilds are passed
*/
friend inline bool operator< (const role& lhs, const role& rhs) {
if (lhs.guild_id != rhs.guild_id) {
throw std::invalid_argument("cannot compare roles from two different guilds");
}

// the @everyone role is always the lowest role in hierarchy
if (lhs.id == lhs.guild_id) {
return rhs.id != lhs.guild_id;
}

if (lhs.position == rhs.position) {
// the higher the IDs are, the lower the position
return lhs.id > rhs.id;
}

return lhs.position < rhs.position;
}

Expand All @@ -331,9 +351,11 @@ class DPP_EXPORT role : public managed, public json_interface<role> {
* @param lhs first role to compare
* @param rhs second role to compare
* @return true if lhs is greater than rhs
*
* @throws std::invalid_argument when roles of two different guilds are passed
*/
friend inline bool operator> (const role& lhs, const role& rhs) {
return lhs.position > rhs.position;
return !(lhs < rhs);
}

/**
Expand Down
36 changes: 31 additions & 5 deletions src/unittest/test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -342,11 +342,37 @@ Markdown lol \\|\\|spoiler\\|\\| \\~\\~strikethrough\\~\\~ \\`small \\*code\\* b
set_test(TIMESTAMPTOSTRING, false);
set_test(TIMESTAMPTOSTRING, dpp::ts_to_string(1642611864) == "2022-01-19T17:04:24Z");

set_test(ROLE_COMPARE, false);
dpp::role role_1, role_2;
role_1.position = 1;
role_2.position = 2;
set_test(ROLE_COMPARE, role_1 < role_2 && role_1 != role_2);
{
set_test(ROLE_COMPARE, false);
dpp::role role_1, role_2;
role_1.position = 1;
role_1.guild_id = 123;
role_2.position = 2;
role_2.guild_id = 123;
set_test(ROLE_COMPARE, role_1 < role_2 && !(role_1 > role_2) && role_1 != role_2);
}
{
set_test(ROLE_COMPARE_CONSIDERING_EVERYONE_ROLE, false);
dpp::role role_1, role_2;
role_1.id = 123;
role_1.position = 2;
role_1.guild_id = 123;
role_2.id = 124;
role_2.position = 2;
role_2.guild_id = 123;
set_test(ROLE_COMPARE_CONSIDERING_EVERYONE_ROLE, role_1 < role_2 && !(role_1 > role_2));
}
{
set_test(ROLE_COMPARE_CONSIDERING_ID, false);
dpp::role role_1, role_2;
role_1.id = 99;
role_1.position = 2;
role_1.guild_id = 123;
role_2.id = 98;
role_2.position = 2;
role_2.guild_id = 123;
set_test(ROLE_COMPARE_CONSIDERING_ID, role_1 < role_2 && !(role_1 > role_2));
}

set_test(WEBHOOK, false);
try {
Expand Down
2 changes: 2 additions & 0 deletions src/unittest/test.h
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,8 @@ DPP_TEST(UTILITY_CDN_ENDPOINT_URL_HASH, "utility::cdn_endpoint_url_hash", tf_off
DPP_TEST(STICKER_GET_URL, "sticker::get_url aka utility::cdn_endpoint_url_sticker", tf_offline);
DPP_TEST(EMOJI_GET_URL, "emoji::get_url", tf_offline);
DPP_TEST(ROLE_COMPARE, "role::operator<", tf_offline);
DPP_TEST(ROLE_COMPARE_CONSIDERING_EVERYONE_ROLE, "role::operator<", tf_offline);
DPP_TEST(ROLE_COMPARE_CONSIDERING_ID, "role::operator<", tf_offline);
DPP_TEST(ROLE_CREATE, "cluster::role_create", tf_online);
DPP_TEST(ROLE_EDIT, "cluster::role_edit", tf_online);
DPP_TEST(ROLE_DELETE, "cluster::role_delete", tf_online);
Expand Down

0 comments on commit b0c53bc

Please sign in to comment.