diff --git a/commands/profile/images.py b/commands/profile/images.py index 736dc00..5fef66e 100644 --- a/commands/profile/images.py +++ b/commands/profile/images.py @@ -22,6 +22,7 @@ from utils.decorators import logged_command from views.embeds import EmbedColors, EmbedTemplate from views.base import BaseView from models.player import Player +from utils.permissions import is_admin # URL Validation Functions @@ -117,7 +118,7 @@ async def can_edit_player_image( If has permission, error_message is empty string """ # Admins can edit anyone - if interaction.user.guild_permissions.administrator: + if is_admin(interaction): logger.debug("User is admin, granting permission", user_id=interaction.user.id) return True, "" diff --git a/tests/test_commands_profile_images.py b/tests/test_commands_profile_images.py index 9a12f08..bef3f2d 100644 --- a/tests/test_commands_profile_images.py +++ b/tests/test_commands_profile_images.py @@ -3,17 +3,19 @@ Tests for player image management commands. Covers URL validation, permission checking, and command execution. """ + import pytest import asyncio from unittest.mock import MagicMock, patch import aiohttp +import discord from aioresponses import aioresponses from commands.profile.images import ( validate_url_format, check_url_accessibility, can_edit_player_image, - ImageCommands + ImageCommands, ) from tests.factories import PlayerFactory, TeamFactory @@ -94,7 +96,7 @@ class TestURLAccessibility: url = "https://example.com/image.jpg" with aioresponses() as m: - m.head(url, status=200, headers={'content-type': 'image/jpeg'}) + m.head(url, status=200, headers={"content-type": "image/jpeg"}) is_accessible, error = await check_url_accessibility(url) @@ -118,7 +120,7 @@ class TestURLAccessibility: url = "https://example.com/page.html" with aioresponses() as m: - m.head(url, status=200, headers={'content-type': 'text/html'}) + m.head(url, status=200, headers={"content-type": "text/html"}) is_accessible, error = await check_url_accessibility(url) @@ -157,6 +159,7 @@ class TestPermissionChecking: async def test_admin_can_edit_any_player(self): """Test administrator can edit any player's images.""" mock_interaction = MagicMock() + mock_interaction.user = MagicMock(spec=discord.Member) mock_interaction.user.id = 12345 mock_interaction.user.guild_permissions.administrator = True @@ -186,7 +189,9 @@ class TestPermissionChecking: mock_logger = MagicMock() - with patch('commands.profile.images.team_service.get_teams_by_owner') as mock_get_teams: + with patch( + "commands.profile.images.team_service.get_teams_by_owner" + ) as mock_get_teams: mock_get_teams.return_value = [user_team] has_permission, error = await can_edit_player_image( @@ -211,7 +216,9 @@ class TestPermissionChecking: mock_logger = MagicMock() - with patch('commands.profile.images.team_service.get_teams_by_owner') as mock_get_teams: + with patch( + "commands.profile.images.team_service.get_teams_by_owner" + ) as mock_get_teams: mock_get_teams.return_value = [user_team] has_permission, error = await can_edit_player_image( @@ -236,7 +243,9 @@ class TestPermissionChecking: mock_logger = MagicMock() - with patch('commands.profile.images.team_service.get_teams_by_owner') as mock_get_teams: + with patch( + "commands.profile.images.team_service.get_teams_by_owner" + ) as mock_get_teams: mock_get_teams.return_value = [user_team] has_permission, error = await can_edit_player_image( @@ -258,7 +267,9 @@ class TestPermissionChecking: mock_logger = MagicMock() - with patch('commands.profile.images.team_service.get_teams_by_owner') as mock_get_teams: + with patch( + "commands.profile.images.team_service.get_teams_by_owner" + ) as mock_get_teams: mock_get_teams.return_value = [] has_permission, error = await can_edit_player_image( @@ -299,7 +310,7 @@ class TestImageCommandsIntegration: async def test_set_image_command_structure(self, commands_cog): """Test that set_image command is properly configured.""" - assert hasattr(commands_cog, 'set_image') + assert hasattr(commands_cog, "set_image") assert commands_cog.set_image.name == "set-image" async def test_fancy_card_updates_vanity_card_field(self, commands_cog): diff --git a/utils/permissions.py b/utils/permissions.py index 1acff5a..240e112 100644 --- a/utils/permissions.py +++ b/utils/permissions.py @@ -7,6 +7,7 @@ servers and user types: - @league_only: Only available in the league server - @requires_team: User must have a team (works with global commands) """ + import logging from functools import wraps from typing import Optional, Callable @@ -21,6 +22,7 @@ logger = logging.getLogger(__name__) class PermissionError(Exception): """Raised when a user doesn't have permission to use a command.""" + pass @@ -54,17 +56,16 @@ async def get_user_team(user_id: int) -> Optional[dict]: # This call is automatically cached by TeamService config = get_config() team = await team_service.get_team_by_owner( - owner_id=user_id, - season=config.sba_season + owner_id=user_id, season=config.sba_season ) if team: logger.debug(f"User {user_id} has team: {team.lname}") return { - 'id': team.id, - 'name': team.lname, - 'abbrev': team.abbrev, - 'season': team.season + "id": team.id, + "name": team.lname, + "abbrev": team.abbrev, + "season": team.season, } logger.debug(f"User {user_id} does not have a team") @@ -77,6 +78,18 @@ def is_league_server(guild_id: int) -> bool: return guild_id == config.guild_id +def is_admin(interaction: discord.Interaction) -> bool: + """Check if the interaction user is a server administrator. + + Includes an isinstance guard for discord.Member so this is safe + to call from DM contexts (where guild_permissions is unavailable). + """ + return ( + isinstance(interaction.user, discord.Member) + and interaction.user.guild_permissions.administrator + ) + + def league_only(): """ Decorator to restrict a command to the league server only. @@ -87,14 +100,14 @@ def league_only(): async def team_command(self, interaction: discord.Interaction): # Only executes in league server """ + def decorator(func: Callable) -> Callable: @wraps(func) async def wrapper(self, interaction: discord.Interaction, *args, **kwargs): # Check if in a guild if not interaction.guild: await interaction.response.send_message( - "❌ This command can only be used in a server.", - ephemeral=True + "❌ This command can only be used in a server.", ephemeral=True ) return @@ -102,13 +115,14 @@ def league_only(): if not is_league_server(interaction.guild.id): await interaction.response.send_message( "❌ This command is only available in the SBa league server.", - ephemeral=True + ephemeral=True, ) return return await func(self, interaction, *args, **kwargs) return wrapper + return decorator @@ -123,6 +137,7 @@ def requires_team(): async def mymoves_command(self, interaction: discord.Interaction): # Only executes if user has a team """ + def decorator(func: Callable) -> Callable: @wraps(func) async def wrapper(self, interaction: discord.Interaction, *args, **kwargs): @@ -133,29 +148,33 @@ def requires_team(): if team is None: await interaction.response.send_message( "❌ This command requires you to have a team in the SBa league. Contact an admin if you believe this is an error.", - ephemeral=True + ephemeral=True, ) return # Store team info in interaction for command to use # This allows commands to access the team without another lookup - interaction.extras['user_team'] = team + interaction.extras["user_team"] = team return await func(self, interaction, *args, **kwargs) except Exception as e: # Log the error for debugging - logger.error(f"Error checking team ownership for user {interaction.user.id}: {e}", exc_info=True) + logger.error( + f"Error checking team ownership for user {interaction.user.id}: {e}", + exc_info=True, + ) # Provide helpful error message to user await interaction.response.send_message( "❌ Unable to verify team ownership due to a temporary error. Please try again in a moment. " "If this persists, contact an admin.", - ephemeral=True + ephemeral=True, ) return return wrapper + return decorator @@ -170,12 +189,14 @@ def global_command(): async def roll_command(self, interaction: discord.Interaction): # Available in all servers """ + def decorator(func: Callable) -> Callable: @wraps(func) async def wrapper(self, interaction: discord.Interaction, *args, **kwargs): return await func(self, interaction, *args, **kwargs) return wrapper + return decorator @@ -190,35 +211,35 @@ def admin_only(): async def sync_command(self, interaction: discord.Interaction): # Only executes for admins """ + def decorator(func: Callable) -> Callable: @wraps(func) async def wrapper(self, interaction: discord.Interaction, *args, **kwargs): # Check if user is guild admin if not interaction.guild: await interaction.response.send_message( - "❌ This command can only be used in a server.", - ephemeral=True + "❌ This command can only be used in a server.", ephemeral=True ) return # Check if user has admin permissions if not isinstance(interaction.user, discord.Member): await interaction.response.send_message( - "❌ Unable to verify permissions.", - ephemeral=True + "❌ Unable to verify permissions.", ephemeral=True ) return if not interaction.user.guild_permissions.administrator: await interaction.response.send_message( "❌ This command requires administrator permissions.", - ephemeral=True + ephemeral=True, ) return return await func(self, interaction, *args, **kwargs) return wrapper + return decorator @@ -241,6 +262,7 @@ def league_admin_only(): async def admin_sync_prefix(self, ctx: commands.Context): # Only league server admins can use this """ + def decorator(func: Callable) -> Callable: @wraps(func) async def wrapper(self, ctx_or_interaction, *args, **kwargs): @@ -254,6 +276,7 @@ def league_admin_only(): async def send_error(msg: str): await ctx.send(msg) + else: interaction = ctx_or_interaction guild = interaction.guild @@ -269,7 +292,9 @@ def league_admin_only(): # Check if league server if not is_league_server(guild.id): - await send_error("❌ This command is only available in the SBa league server.") + await send_error( + "❌ This command is only available in the SBa league server." + ) return # Check admin permissions @@ -284,4 +309,5 @@ def league_admin_only(): return await func(self, ctx_or_interaction, *args, **kwargs) return wrapper + return decorator