Hi Twentysix (Twentysix#5252), thank you for your patience while I review your repo.
Commit hash at time of review - ef43e9eba36c15a1cf8b32c6d154bd4ed313532d
- Lines 148, 216: You have no checks here for
embed_links so if the bot is missing the permission it will send the standard error message instead.
- Line 1508-1516: You might consider utilizing
await asyncio.gather(*calls) here to concurrently send the notifications to all guilds at once rather than waiting for the try/except to finish, you can even have a callback function if they fail in some way and then there’s no need to do the
await asyncio.sleep(0.5). (Optional)
- Lines 1545-1557: Again no checks for
embed_links permission for the bot before posting.
- Line 691, 703, 844, 856, 915, 929, 1174, 1349, 1744, 1815, 1884, 1892: You should not use bare excepts, preferably use
except Exception or catch known exceptions. (Optional)
- Line 1180: You might consider using
from redbot.core.utils.chat_formatting import humanize_list here simplify the display of errored ID’s. (Optional)
- Line 1130: You should probably also have a check here that the
is_staff author in question actually has ban permissions or
await self.bot.is_admin(ctx.author) to be consistent with cores ban command. It’s possible that the mod role setup on the bot is given to mods who only have power to kick and this would grant them limitless access to ban without actually having permission to ban, being also a command meant to hide the actions of the mods this can lead to some unwanted scenarios.
- Line 1202-1305: Again this should have a check that in the event that a mod does not have ban permission or even kick permission this should not execute the command unless in emergency mode. This one is less strict as kick is generally less of a problem but the members discord permissions should be taken into consideration if it’s not in emergency mode.
- Line 1546: You might consider using
message.jump_url here instead of building the link yourself. (Optional)
- Line 1704: this is over-indented. (Optional)
- Lines 590-614: Added after discussion in #testing in Red server. This is good but it occured to me that this only checks role hierarchy and it may be worthwhile checking relevant permissions on the user themselves for all action types. For example you don’t want someone considered a mod with kick but not ban perms to be able to create a warden action that allows them to setup warden to be able to ban a user at their whim.
- Lines 92: Continuation line is over-indented. (Optional)
- Line 124, 151, 299, and 503: You should not use bare excepts preferably use
except Exception or catch known exceptions. (Optional)
- Lines 444-450: Lines missing indentation. (Optional)
- Line 457: You could just use
message.jump_url instead of formatting the url yourself. (optional)
- Line 606: You can compare role hierarchy directly with the role object without needing to compare postions. (Optional)
- Line 19: you import
Union but don’t use any of them. (Optional)
- Line 24: You import
json but it’s not used anywhere. (Optional)
- Line 29: you import
GuildContext but they’re not used anywhere. (Optional)
- Line 232: You should not use bare excepts, at the very least use
except Exception if you want to capture all exceptions raised from the function. (Optional)
- Lines 64 and 134: You should be checking if the bot has permission to send embeds for each of these commands and either ignore the command (simple with the
@commands.bot_has_permissions(embed_links=True)) decorator) or provide an output to the menu that is just strings.
- You may also consider adding a check for add_reactions so that the menu is actually usable in the event someone uses the command in a channel the bot doesn’t have add reaction perms in although this only breaks browse it doesn’t affect display of the search command. (Optional)
Some special discussion about defender.py:
- Lines 670-710: These commands should probably have additional checks that the issuer also has manage messages permission. This one is kindof a grey area but as it is right now the
@commands.admin() only checks that the issuer has at least one of the bots assigned adminroles. This one is optional simply because the severity of the actions are minimal. (Optional)
- Lines 744-803: This should have checks that the issuer of the command has ban permissions to enable the module otherwise a not-so-wise server owner could tell the bot that a role should be considered admin by the bot and grant access to the voteout module. This in conjunction with the other issues with voteout listed
below above spell disaster under rare circumstances.
These are under a special discussion because the
@commands.admin() decorator is used and generally that’s considered someone who has permission within the server to perform these actions and you can put blame on the server owner for setting up the wrong role as an admin role but it should be stated that that check is strictly for the bots admin role being set. Generally when I set permission decorators on cogs I like to use the
@commands.admin_or_permissions(relevant_permission=True) so that if the server owner does not set the admin role the relevant members who can be considered admin under the actions of the command can still run the commands. So I will be marking these both as optional changes to the command permissions but I would highly recommend you consider the possibilities of allowing only bot set admin role from performing the actions.