From 8e25ed16caf0a510b386d371800b6f93e8855684 Mon Sep 17 00:00:00 2001 From: Izan Date: Wed, 1 Sep 2021 11:50:36 +0100 Subject: Prevent infractions raising error due to role hierarchy Now explicitly states that the bot is unable to starify/kick/ban someone who's higher in the role hierarchy --- bot/exts/moderation/infraction/infractions.py | 8 ++++++++ bot/exts/moderation/infraction/superstarify.py | 4 ++++ 2 files changed, 12 insertions(+) diff --git a/bot/exts/moderation/infraction/infractions.py b/bot/exts/moderation/infraction/infractions.py index eaba97703..4afa66460 100644 --- a/bot/exts/moderation/infraction/infractions.py +++ b/bot/exts/moderation/infraction/infractions.py @@ -314,6 +314,10 @@ class Infractions(InfractionScheduler, commands.Cog): @respect_role_hierarchy(member_arg=2) async def apply_kick(self, ctx: Context, user: Member, reason: t.Optional[str], **kwargs) -> None: """Apply a kick infraction with kwargs passed to `post_infraction`.""" + if user.top_role > ctx.me.top_role: + await ctx.send(":x: I can't kick users above me in the role hierarchy.") + return + infraction = await _utils.post_infraction(ctx, user, "kick", reason, active=False, **kwargs) if infraction is None: return @@ -340,6 +344,10 @@ class Infractions(InfractionScheduler, commands.Cog): Will also remove the banned user from the Big Brother watch list if applicable. """ + if hasattr(user, 'top_role') and user.top_role > ctx.me.top_role: + await ctx.send(":x: I can't ban users above me in the role hierarchy.") + return + # In the case of a permanent ban, we don't need get_active_infractions to tell us if one is active is_temporary = kwargs.get("expires_at") is not None active_infraction = await _utils.get_active_infraction(ctx, user, "ban", is_temporary) diff --git a/bot/exts/moderation/infraction/superstarify.py b/bot/exts/moderation/infraction/superstarify.py index 05a2bbe10..2b111fed7 100644 --- a/bot/exts/moderation/infraction/superstarify.py +++ b/bot/exts/moderation/infraction/superstarify.py @@ -132,6 +132,10 @@ class Superstarify(InfractionScheduler, Cog): An optional reason can be provided, which would be added to a message stating their old nickname and linking to the nickname policy. """ + if member.top_role > ctx.me.top_role: + await ctx.send(":x: I can't starify users above me in the role hierarchy.") + return + if await _utils.get_active_infraction(ctx, member, "superstar"): return -- cgit v1.2.3 From 8fa3fa1d80933a539cef52c1949dd14d0d96dfd1 Mon Sep 17 00:00:00 2001 From: Izan Date: Wed, 1 Sep 2021 16:33:10 +0100 Subject: Fix role hierarchy check Now uses `>=` instead of `>`, as is meant to happen. --- bot/exts/moderation/infraction/infractions.py | 8 ++++---- bot/exts/moderation/infraction/superstarify.py | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/bot/exts/moderation/infraction/infractions.py b/bot/exts/moderation/infraction/infractions.py index 4afa66460..93a6b6c8b 100644 --- a/bot/exts/moderation/infraction/infractions.py +++ b/bot/exts/moderation/infraction/infractions.py @@ -314,8 +314,8 @@ class Infractions(InfractionScheduler, commands.Cog): @respect_role_hierarchy(member_arg=2) async def apply_kick(self, ctx: Context, user: Member, reason: t.Optional[str], **kwargs) -> None: """Apply a kick infraction with kwargs passed to `post_infraction`.""" - if user.top_role > ctx.me.top_role: - await ctx.send(":x: I can't kick users above me in the role hierarchy.") + if user.top_role >= ctx.me.top_role: + await ctx.send(":x: I can't kick users above or equal to me in the role hierarchy.") return infraction = await _utils.post_infraction(ctx, user, "kick", reason, active=False, **kwargs) @@ -344,8 +344,8 @@ class Infractions(InfractionScheduler, commands.Cog): Will also remove the banned user from the Big Brother watch list if applicable. """ - if hasattr(user, 'top_role') and user.top_role > ctx.me.top_role: - await ctx.send(":x: I can't ban users above me in the role hierarchy.") + if hasattr(user, 'top_role') and user.top_role >= ctx.me.top_role: + await ctx.send(":x: I can't ban users above or equal to me in the role hierarchy.") return # In the case of a permanent ban, we don't need get_active_infractions to tell us if one is active diff --git a/bot/exts/moderation/infraction/superstarify.py b/bot/exts/moderation/infraction/superstarify.py index 2b111fed7..986decdd6 100644 --- a/bot/exts/moderation/infraction/superstarify.py +++ b/bot/exts/moderation/infraction/superstarify.py @@ -132,8 +132,8 @@ class Superstarify(InfractionScheduler, Cog): An optional reason can be provided, which would be added to a message stating their old nickname and linking to the nickname policy. """ - if member.top_role > ctx.me.top_role: - await ctx.send(":x: I can't starify users above me in the role hierarchy.") + if member.top_role >= ctx.me.top_role: + await ctx.send(":x: I can't starify users above or equal to me in the role hierarchy.") return if await _utils.get_active_infraction(ctx, member, "superstar"): -- cgit v1.2.3 From 38dc1c69095c35c72a543ed7b08c8dbdad8931c2 Mon Sep 17 00:00:00 2001 From: Izan Date: Wed, 1 Sep 2021 23:11:57 +0100 Subject: Fix tests --- tests/bot/exts/backend/sync/test_users.py | 2 +- tests/bot/exts/moderation/infraction/test_infractions.py | 11 ++++++----- tests/helpers.py | 2 ++ 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/tests/bot/exts/backend/sync/test_users.py b/tests/bot/exts/backend/sync/test_users.py index 27932be95..5469f6b67 100644 --- a/tests/bot/exts/backend/sync/test_users.py +++ b/tests/bot/exts/backend/sync/test_users.py @@ -10,7 +10,7 @@ def fake_user(**kwargs): kwargs.setdefault("id", 43) kwargs.setdefault("name", "bob the test man") kwargs.setdefault("discriminator", 1337) - kwargs.setdefault("roles", [666]) + kwargs.setdefault("roles", [helpers.MockRole(id=666)]) kwargs.setdefault("in_guild", True) return kwargs diff --git a/tests/bot/exts/moderation/infraction/test_infractions.py b/tests/bot/exts/moderation/infraction/test_infractions.py index f844a9181..56528b290 100644 --- a/tests/bot/exts/moderation/infraction/test_infractions.py +++ b/tests/bot/exts/moderation/infraction/test_infractions.py @@ -13,12 +13,13 @@ class TruncationTests(unittest.IsolatedAsyncioTestCase): """Tests for ban and kick command reason truncation.""" def setUp(self): + self.me = MockMember(id=7890, roles=[MockRole(id=7890, position=5)]) self.bot = MockBot() self.cog = Infractions(self.bot) - self.user = MockMember(id=1234, top_role=MockRole(id=3577, position=10)) - self.target = MockMember(id=1265, top_role=MockRole(id=9876, position=0)) + self.user = MockMember(id=1234, roles=[MockRole(id=3577, position=10)]) + self.target = MockMember(id=1265, roles=[MockRole(id=9876, position=1)]) self.guild = MockGuild(id=4567) - self.ctx = MockContext(bot=self.bot, author=self.user, guild=self.guild) + self.ctx = MockContext(me=self.me, bot=self.bot, author=self.user, guild=self.guild) @patch("bot.exts.moderation.infraction._utils.get_active_infraction") @patch("bot.exts.moderation.infraction._utils.post_infraction") @@ -64,8 +65,8 @@ class VoiceBanTests(unittest.IsolatedAsyncioTestCase): def setUp(self): self.bot = MockBot() - self.mod = MockMember(top_role=10) - self.user = MockMember(top_role=1, roles=[MockRole(id=123456)]) + self.mod = MockMember(roles=[MockRole(id=7890123, position=10)]) + self.user = MockMember(roles=[MockRole(id=123456, position=1)]) self.guild = MockGuild() self.ctx = MockContext(bot=self.bot, author=self.mod) self.cog = Infractions(self.bot) diff --git a/tests/helpers.py b/tests/helpers.py index 3978076ed..8443150eb 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -235,6 +235,7 @@ class MockMember(CustomMockMixin, unittest.mock.Mock, ColourMixin, HashableMixin self.roles = [MockRole(name="@everyone", position=1, id=0)] if roles: self.roles.extend(roles) + self.top_role = max(self.roles) if 'mention' not in kwargs: self.mention = f"@{self.name}" @@ -439,6 +440,7 @@ class MockContext(CustomMockMixin, unittest.mock.MagicMock): def __init__(self, **kwargs) -> None: super().__init__(**kwargs) + self.me = kwargs.get('me', MockMember()) self.bot = kwargs.get('bot', MockBot()) self.guild = kwargs.get('guild', MockGuild()) self.author = kwargs.get('author', MockMember()) -- cgit v1.2.3 From a9b02a5a85e0f558a0d9f0a2d0f0639045a421e5 Mon Sep 17 00:00:00 2001 From: Izan Date: Tue, 5 Oct 2021 22:36:47 +0100 Subject: Use `isinstance` instead of `hasattr` to determine if `Member` --- bot/exts/moderation/infraction/infractions.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bot/exts/moderation/infraction/infractions.py b/bot/exts/moderation/infraction/infractions.py index 93a6b6c8b..fd47ff1c7 100644 --- a/bot/exts/moderation/infraction/infractions.py +++ b/bot/exts/moderation/infraction/infractions.py @@ -344,7 +344,7 @@ class Infractions(InfractionScheduler, commands.Cog): Will also remove the banned user from the Big Brother watch list if applicable. """ - if hasattr(user, 'top_role') and user.top_role >= ctx.me.top_role: + if isinstance(user, Member) and user.top_role >= ctx.me.top_role: await ctx.send(":x: I can't ban users above or equal to me in the role hierarchy.") return @@ -527,7 +527,7 @@ class Infractions(InfractionScheduler, commands.Cog): async def cog_command_error(self, ctx: Context, error: Exception) -> None: """Send a notification to the invoking context on a Union failure.""" if isinstance(error, commands.BadUnionArgument): - if discord.User in error.converters or discord.Member in error.converters: + if discord.User in error.converters or Member in error.converters: await ctx.send(str(error.errors[0])) error.handled = True -- cgit v1.2.3