diff --git a/docpages/example_programs/misc/checking-member-permissions.md b/docpages/example_programs/misc/checking-member-permissions.md index 364234f7ad..8c53296242 100644 --- a/docpages/example_programs/misc/checking-member-permissions.md +++ b/docpages/example_programs/misc/checking-member-permissions.md @@ -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(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 diff --git a/include/dpp/cluster.h b/include/dpp/cluster.h index 9c283e4a72..179be8f434 100644 --- a/include/dpp/cluster.h +++ b/include/dpp/cluster.h @@ -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(). @@ -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(). diff --git a/include/dpp/cluster_sync_calls.h b/include/dpp/cluster_sync_calls.h index 86ccd5fc02..461736b215 100644 --- a/include/dpp/cluster_sync_calls.h +++ b/include/dpp/cluster_sync_calls.h @@ -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 @@ -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 diff --git a/include/dpp/role.h b/include/dpp/role.h index 1a76862823..0917fa15a4 100644 --- a/include/dpp/role.h +++ b/include/dpp/role.h @@ -120,6 +120,10 @@ class DPP_EXPORT role : public managed, public json_interface { /** * @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}; @@ -320,8 +324,24 @@ class DPP_EXPORT role : public managed, public json_interface { * @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; } @@ -331,9 +351,11 @@ class DPP_EXPORT role : public managed, public json_interface { * @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); } /** diff --git a/src/unittest/test.cpp b/src/unittest/test.cpp index da96a28096..3182d91c24 100644 --- a/src/unittest/test.cpp +++ b/src/unittest/test.cpp @@ -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 { diff --git a/src/unittest/test.h b/src/unittest/test.h index 97637b7711..edb942c40a 100644 --- a/src/unittest/test.h +++ b/src/unittest/test.h @@ -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);