From a34aebd5050dc8af86f0768aaa8f668161b2ccd1 Mon Sep 17 00:00:00 2001 From: Arty Date: Sun, 18 Jan 2026 13:26:51 +0100 Subject: [PATCH 1/4] Add spectator mode with limited room view for spectating players Spectators can view room information but with restricted serialization groups (get-room-spectator) that hide sensitive game data like targets and missions. Game masters retain full access despite having SPECTATING status. --- migrations/Version20260117120000.php | 29 +++ src/Api/Controller/RoomController.php | 15 +- src/Api/Controller/UserController.php | 14 +- src/Domain/Player/Entity/Player.php | 31 ++- src/Domain/Room/Entity/Room.php | 30 ++- .../Security/Voters/RoomVoter.php | 13 +- tests/Api/SpectatorModeCest.php | 240 ++++++++++++++++++ .../Security/Voters/RoomVoterSpectateTest.php | 152 +++++++++++ 8 files changed, 507 insertions(+), 17 deletions(-) create mode 100644 migrations/Version20260117120000.php create mode 100644 tests/Api/SpectatorModeCest.php create mode 100644 tests/Unit/Infrastructure/Security/Voters/RoomVoterSpectateTest.php diff --git a/migrations/Version20260117120000.php b/migrations/Version20260117120000.php new file mode 100644 index 0000000..869f1e4 --- /dev/null +++ b/migrations/Version20260117120000.php @@ -0,0 +1,29 @@ +addSql('ALTER TABLE room ADD allow_spectators BOOLEAN NOT NULL DEFAULT FALSE'); + } + + public function down(Schema $schema): void + { + $this->addSql('ALTER TABLE room DROP COLUMN allow_spectators'); + } +} diff --git a/src/Api/Controller/RoomController.php b/src/Api/Controller/RoomController.php index e6a00dc..2dc44c0 100644 --- a/src/Api/Controller/RoomController.php +++ b/src/Api/Controller/RoomController.php @@ -9,6 +9,7 @@ use App\Domain\KillerSerializerInterface; use App\Domain\KillerValidatorInterface; use App\Domain\Player\Enum\PlayerStatus; +use App\Domain\Player\PlayerRepository; use App\Domain\Room\Entity\Room; use App\Domain\Room\RoomRepository; use App\Domain\Room\RoomWorkflowTransitionInterface; @@ -33,6 +34,7 @@ class RoomController extends AbstractController public function __construct( private readonly RoomRepository $roomRepository, + private readonly PlayerRepository $playerRepository, private readonly PersistenceAdapterInterface $persistenceAdapter, private readonly RoomWorkflowTransitionInterface $roomStatusTransitionUseCase, private readonly SseInterface $hub, @@ -83,7 +85,18 @@ public function createRoom(Request $request): JsonResponse #[IsGranted(RoomVoter::VIEW_ROOM, subject: 'room', message: 'KILLER_VIEW_ROOM_UNAUTHORIZED')] public function getRoom(Room $room): JsonResponse { - return $this->json($room, Response::HTTP_OK, [], [AbstractNormalizer::GROUPS => 'get-room']); + /** @var User|null $user */ + $user = $this->getUser(); + $currentPlayer = $user !== null ? $this->playerRepository->getCurrentUserPlayer($user) : null; + + // Spectators get limited view (game masters see everything despite having SPECTATING status) + $isSpectator = $currentPlayer !== null + && $currentPlayer->getStatus() === PlayerStatus::SPECTATING + && !$currentPlayer->isMaster(); + + $groups = $isSpectator ? ['get-room-spectator'] : ['get-room']; + + return $this->json($room, Response::HTTP_OK, [], [AbstractNormalizer::GROUPS => $groups]); } #[Route('/{id}', name: 'patch_room', methods: [Request::METHOD_PATCH])] diff --git a/src/Api/Controller/UserController.php b/src/Api/Controller/UserController.php index 82355ca..9558d59 100644 --- a/src/Api/Controller/UserController.php +++ b/src/Api/Controller/UserController.php @@ -4,9 +4,11 @@ namespace App\Api\Controller; +use App\Api\Exception\KillerBadRequestHttpException; use App\Application\UseCase\Player\CreatePlayerUseCase; use App\Domain\KillerSerializerInterface; use App\Domain\KillerValidatorInterface; +use App\Domain\Player\Enum\PlayerStatus; use App\Domain\Player\Event\PlayerChangedRoomEvent; use App\Domain\Player\PlayerRepository; use App\Domain\Room\RoomRepository; @@ -145,6 +147,7 @@ public function patchUser(Request $request): JsonResponse // Handle room change if (array_key_exists('room', $data)) { $newRoomId = $data['room']; + $joinAsSpectator = $data['spectate'] ?? false; if ($newRoomId !== null) { $newRoom = $this->roomRepository->findOneBy(['id' => $newRoomId]); @@ -153,12 +156,21 @@ public function patchUser(Request $request): JsonResponse throw $this->createNotFoundException('ROOM_NOT_FOUND'); } + // Validate spectator access + if ($joinAsSpectator && !$newRoom->isAllowSpectators()) { + throw new KillerBadRequestHttpException('ROOM_SPECTATORS_NOT_ALLOWED'); + } + $existingPlayer = $this->playerRepository->findPlayerByUserAndRoom($user, $newRoom); $user->setRoom($newRoom); // Create a new player for this user in the room if one doesn't exist if ($existingPlayer === null) { - $this->createPlayerUseCase->execute($user, $newRoom); + $player = $this->createPlayerUseCase->execute($user, $newRoom); + + if ($joinAsSpectator) { + $player->setStatus(PlayerStatus::SPECTATING); + } } } diff --git a/src/Domain/Player/Entity/Player.php b/src/Domain/Player/Entity/Player.php index 552ac43..4ce144f 100644 --- a/src/Domain/Player/Entity/Player.php +++ b/src/Domain/Player/Entity/Player.php @@ -28,11 +28,20 @@ class Player implements RecipientInterface #[ORM\Id] #[ORM\Column(type: 'integer', unique: true)] #[ORM\GeneratedValue(strategy: 'SEQUENCE')] - #[Groups(['get-player', 'create-player', 'get-room', 'get-mission', 'me', 'publish-mercure'])] + #[Groups(['get-player', 'create-player', 'get-room', 'get-mission', 'me', 'publish-mercure', 'get-room-spectator'])] private int $id; #[ORM\Column(type: 'string', length: 255)] - #[Groups(['get-player', 'create-player', 'get-room', 'me', 'post-player', 'patch-player', 'publish-mercure'])] + #[Groups([ + 'get-player', + 'create-player', + 'get-room', + 'me', + 'post-player', + 'patch-player', + 'publish-mercure', + 'get-room-spectator', + ])] #[Assert\Length( min: 2, max: 30, @@ -53,7 +62,15 @@ class Player implements RecipientInterface enumType: PlayerStatus::class, options: ['default' => PlayerStatus::ALIVE], )] - #[Groups(['get-player', 'create-player', 'get-room', 'me', 'patch-player', 'publish-mercure'])] + #[Groups([ + 'get-player', + 'create-player', + 'get-room', + 'me', + 'patch-player', + 'publish-mercure', + 'get-room-spectator', + ])] private PlayerStatus $status = PlayerStatus::ALIVE; #[ORM\ManyToOne(targetEntity: Room::class, inversedBy: 'players')] @@ -78,7 +95,7 @@ enumType: PlayerStatus::class, private ?Mission $assignedMission = null; #[ORM\Column(type: 'string', options: ['default' => self::DEFAULT_AVATAR])] - #[Groups(['me', 'get-room', 'post-player', 'create-player', 'get-player', 'patch-player'])] + #[Groups(['me', 'get-room', 'post-player', 'create-player', 'get-player', 'patch-player', 'get-room-spectator'])] private string $avatar = self::DEFAULT_AVATAR; #[ORM\Column(type: 'string', options: ['default' => ''])] @@ -86,7 +103,7 @@ enumType: PlayerStatus::class, private string $expoPushToken = ''; #[ORM\Column(type: 'integer', options: ['default' => 0])] - #[Groups(['me', 'get-player', 'get-room'])] + #[Groups(['me', 'get-player', 'get-room', 'get-room-spectator'])] private int $points = 0; #[ORM\Column(type: 'boolean', options: ['default' => false])] @@ -94,11 +111,11 @@ enumType: PlayerStatus::class, private bool $missionSwitchUsed = false; #[ORM\Column(type: 'boolean', options: ['default' => false])] - #[Groups(['me', 'get-player', 'get-room'])] + #[Groups(['me', 'get-player', 'get-room', 'get-room-spectator'])] private bool $isAdmin = false; #[ORM\Column(type: 'boolean', options: ['default' => false])] - #[Groups(['me', 'get-player', 'get-room'])] + #[Groups(['me', 'get-player', 'get-room', 'get-room-spectator'])] private bool $isMaster = false; public function __construct() diff --git a/src/Domain/Room/Entity/Room.php b/src/Domain/Room/Entity/Room.php index ac032f1..c32c50d 100644 --- a/src/Domain/Room/Entity/Room.php +++ b/src/Domain/Room/Entity/Room.php @@ -28,26 +28,26 @@ class Room #[ORM\GeneratedValue(strategy: 'CUSTOM')] #[ORM\CustomIdGenerator(RoomIdGenerator::class)] #[Assert\Length(exactly: 5)] - #[Groups(['get-player', 'get-room', 'get-mission', 'me', 'publish-mercure', 'patch-player'])] + #[Groups(['get-player', 'get-room', 'get-mission', 'me', 'publish-mercure', 'patch-player', 'get-room-spectator'])] private string $id; #[ORM\Column(type: 'string', length: 255)] - #[Groups(['get-room', 'me', 'patch-room', 'publish-mercure'])] + #[Groups(['get-room', 'me', 'patch-room', 'publish-mercure', 'get-room-spectator'])] #[Assert\Length(min: 2, max: 50, minMessage: 'TOO_SHORT_CONTENT', maxMessage: 'TOO_LONG_CONTENT')] private string $name; #[ORM\Column(type: 'string', length: 255, options: ['default' => self::PENDING])] - #[Groups(['get-room', 'me', 'publish-mercure'])] + #[Groups(['get-room', 'me', 'publish-mercure', 'get-room-spectator'])] private string $status = self::PENDING; /** @var Collection */ #[ORM\OneToMany(mappedBy: 'room', targetEntity: Player::class, fetch: 'EAGER')] #[Assert\Unique] - #[Groups(['get-room', 'publish-mercure'])] + #[Groups(['get-room', 'publish-mercure', 'get-room-spectator'])] private Collection $players; #[ORM\OneToOne(targetEntity: Player::class)] - #[Groups(['get-room', 'publish-mercure'])] + #[Groups(['get-room', 'publish-mercure', 'get-room-spectator'])] private ?Player $admin = null; #[ORM\Column(type: 'datetime_immutable')] @@ -69,13 +69,17 @@ class Room private Collection $secondaryMissions; #[ORM\ManyToOne(targetEntity: Player::class)] - #[Groups(['get-room', 'publish-mercure'])] + #[Groups(['get-room', 'publish-mercure', 'get-room-spectator'])] private ?Player $winner = null; #[ORM\Column(type: 'boolean', options: ['default' => false])] - #[Groups(['get-room', 'publish-mercure', 'me'])] + #[Groups(['get-room', 'publish-mercure', 'me', 'get-room-spectator'])] private bool $isGameMastered = false; + #[ORM\Column(type: 'boolean', options: ['default' => false])] + #[Groups(['get-room', 'patch-room', 'publish-mercure', 'get-room-spectator'])] + private bool $allowSpectators = false; + public function __construct() { $this->players = new ArrayCollection(); @@ -293,6 +297,18 @@ public function popSecondaryMission(): ?Mission return $mission; } + public function isAllowSpectators(): bool + { + return $this->allowSpectators; + } + + public function setAllowSpectators(bool $allowSpectators): self + { + $this->allowSpectators = $allowSpectators; + + return $this; + } + public function __toString(): string { return $this->getId(); diff --git a/src/Infrastructure/Security/Voters/RoomVoter.php b/src/Infrastructure/Security/Voters/RoomVoter.php index fe32646..0c3f469 100644 --- a/src/Infrastructure/Security/Voters/RoomVoter.php +++ b/src/Infrastructure/Security/Voters/RoomVoter.php @@ -5,6 +5,7 @@ namespace App\Infrastructure\Security\Voters; use App\Domain\Player\Entity\Player; +use App\Domain\Player\Enum\PlayerStatus; use App\Domain\Player\PlayerRepository; use App\Domain\Room\Entity\Room; use App\Domain\User\Entity\User; @@ -16,6 +17,7 @@ class RoomVoter extends Voter public const string EDIT_ROOM = 'edit_room'; public const string VIEW_ROOM = 'view_room'; public const string CREATE_ROOM = 'create_room'; + public const string SPECTATE_ROOM = 'spectate_room'; public function __construct( private readonly PlayerRepository $playerRepository, @@ -24,7 +26,7 @@ public function __construct( protected function supports(string $attribute, mixed $subject): bool { - if (!in_array($attribute, [self::EDIT_ROOM, self::VIEW_ROOM, self::CREATE_ROOM], true)) { + if (!in_array($attribute, [self::EDIT_ROOM, self::VIEW_ROOM, self::CREATE_ROOM, self::SPECTATE_ROOM], true)) { return false; } @@ -49,6 +51,7 @@ protected function voteOnAttribute(string $attribute, mixed $subject, TokenInter self::VIEW_ROOM => $currentPlayer && $this->canView($subject, $currentPlayer), self::EDIT_ROOM => $currentPlayer && $this->canEdit($subject, $currentPlayer), self::CREATE_ROOM => !$currentPlayer && $this->canCreate($currentPlayer), + self::SPECTATE_ROOM => $currentPlayer && $this->canSpectate($subject, $currentPlayer), default => throw new \LogicException('This code should not be reached'), }; } @@ -68,4 +71,12 @@ private function canCreate(?Player $player = null): bool { return !$player?->getRoom(); } + + private function canSpectate(mixed $room, Player $player): bool + { + return $room instanceof Room + && $room->isAllowSpectators() + && $player->getRoom() === $room + && $player->getStatus() === PlayerStatus::SPECTATING; + } } diff --git a/tests/Api/SpectatorModeCest.php b/tests/Api/SpectatorModeCest.php new file mode 100644 index 0000000..72a0aee --- /dev/null +++ b/tests/Api/SpectatorModeCest.php @@ -0,0 +1,240 @@ +createAdminAndUpdateHeaders($I); + $I->sendPostAsJson('room'); + + $room = $I->grabEntityFromRepository(Room::class, ['name' => 'Admin\'s room']); + + // Enable spectators + $I->sendPatchAsJson(sprintf('/room/%s', $room->getId()), ['allowSpectators' => true]); + $I->seeResponseCodeIsSuccessful(); + $I->seeResponseContainsJson(['allowSpectators' => true]); + + // Verify it persisted + $I->sendGetAsJson(sprintf('/room/%s', $room->getId())); + $I->seeResponseContainsJson(['allowSpectators' => true]); + } + + public function testCanJoinRoomAsSpectator(ApiTester $I): void + { + // Create admin and room with spectators allowed + $I->createAdminAndUpdateHeaders($I); + $I->sendPostAsJson('room'); + + $room = $I->grabEntityFromRepository(Room::class, ['name' => 'Admin\'s room']); + + // Enable spectators + $I->sendPatchAsJson(sprintf('/room/%s', $room->getId()), ['allowSpectators' => true]); + $I->seeResponseCodeIsSuccessful(); + + // Create a new user and join as spectator + $I->createPlayerAndUpdateHeaders($I, 'Spectator'); + $I->sendPatchAsJson('/user', ['room' => $room->getId(), 'spectate' => true]); + $I->seeResponseCodeIsSuccessful(); + + // Verify player has SPECTATING status + $I->seeInRepository(Player::class, [ + 'name' => 'Spectator', + 'room' => $room->getId(), + 'status' => PlayerStatus::SPECTATING, + ]); + } + + public function testCannotJoinAsSpectatorWhenDisabled(ApiTester $I): void + { + // Create admin and room (spectators disabled by default) + $I->createAdminAndUpdateHeaders($I); + $I->sendPostAsJson('room'); + + $room = $I->grabEntityFromRepository(Room::class, ['name' => 'Admin\'s room']); + + // Try to join as spectator + $I->createPlayerAndUpdateHeaders($I, 'Spectator'); + $I->sendPatchAsJson('/user', ['room' => $room->getId(), 'spectate' => true]); + $I->seeResponseCodeIs(400); + $I->seeResponseContainsJson(['message' => 'ROOM_SPECTATORS_NOT_ALLOWED']); + } + + public function testSpectatorSeesLimitedRoomData(ApiTester $I): void + { + // Create admin and room with spectators allowed + $I->createAdminAndUpdateHeaders($I); + $I->sendPostAsJson('room'); + $I->sendPostAsJson('/mission', ['content' => 'Secret mission 1']); + $I->sendPostAsJson('/mission', ['content' => 'Secret mission 2']); + $I->sendPostAsJson('/mission', ['content' => 'Secret mission 3']); + + $room = $I->grabEntityFromRepository(Room::class, ['name' => 'Admin\'s room']); + + // Enable spectators + $I->sendPatchAsJson(sprintf('/room/%s', $room->getId()), ['allowSpectators' => true]); + + // Add regular players + $I->createPlayerAndUpdateHeaders($I, 'John'); + $I->sendPatchAsJson('/user', ['room' => $room->getId()]); + + $I->createPlayerAndUpdateHeaders($I, 'Jane'); + $I->sendPatchAsJson('/user', ['room' => $room->getId()]); + + // Start the game + $I->setAdminJwtHeader($I); + $I->sendPatchAsJson(sprintf('/room/%s', $room->getId()), ['status' => 'IN_GAME']); + $I->seeResponseCodeIsSuccessful(); + + // Join as spectator + $I->createPlayerAndUpdateHeaders($I, 'Spectator'); + $I->sendPatchAsJson('/user', ['room' => $room->getId(), 'spectate' => true]); + $I->seeResponseCodeIsSuccessful(); + + // Get room data as spectator + $I->sendGetAsJson(sprintf('/room/%s', $room->getId())); + $I->seeResponseCodeIsSuccessful(); + + // Spectator should see basic player info + $I->seeResponseContainsJson([ + 'id' => $room->getId(), + 'name' => 'Admin\'s room', + 'status' => Room::IN_GAME, + 'allowSpectators' => true, + ]); + + // Spectator should see players with limited info (name, status, points) + /** @var array $response */ + $response = json_decode($I->grabResponse(), true); + Assert::assertArrayHasKey('players', $response); + + /** @var array> $players */ + $players = $response['players']; + + foreach ($players as $player) { + // Should have these fields + Assert::assertArrayHasKey('id', $player); + Assert::assertArrayHasKey('name', $player); + Assert::assertArrayHasKey('status', $player); + Assert::assertArrayHasKey('points', $player); + Assert::assertArrayHasKey('avatar', $player); + + // Should NOT have sensitive fields (target, assignedMission) + Assert::assertArrayNotHasKey('target', $player); + Assert::assertArrayNotHasKey('assignedMission', $player); + } + + // Should NOT see missions + Assert::assertArrayNotHasKey('missions', $response); + Assert::assertArrayNotHasKey('secondaryMissions', $response); + } + + public function testRegularPlayerSeesFullRoomData(ApiTester $I): void + { + // Create admin and room + $I->createAdminAndUpdateHeaders($I); + $I->sendPostAsJson('room'); + $I->sendPostAsJson('/mission', ['content' => 'mission 1']); + $I->sendPostAsJson('/mission', ['content' => 'mission 2']); + $I->sendPostAsJson('/mission', ['content' => 'mission 3']); + + $room = $I->grabEntityFromRepository(Room::class, ['name' => 'Admin\'s room']); + + // Add regular players + $I->createPlayerAndUpdateHeaders($I, 'John'); + $I->sendPatchAsJson('/user', ['room' => $room->getId()]); + + $I->createPlayerAndUpdateHeaders($I, 'Jane'); + $I->sendPatchAsJson('/user', ['room' => $room->getId()]); + + // Start the game + $I->setAdminJwtHeader($I); + $I->sendPatchAsJson(sprintf('/room/%s', $room->getId()), ['status' => 'IN_GAME']); + $I->seeResponseCodeIsSuccessful(); + + // Get room data as regular player (Admin) + $I->sendGetAsJson(sprintf('/room/%s', $room->getId())); + $I->seeResponseCodeIsSuccessful(); + + // Regular player should see missions + /** @var array $response */ + $response = json_decode($I->grabResponse(), true); + Assert::assertArrayHasKey('missions', $response); + } + + public function testGameMasterSeesEverythingDespiteSpectatingStatus(ApiTester $I): void + { + // Create admin with game master mode + $I->createAdminAndUpdateHeaders($I); + $I->sendPostAsJson('room', ['isGameMastered' => true]); + $I->sendPostAsJson('/mission', ['content' => 'mission 1']); + $I->sendPostAsJson('/mission', ['content' => 'mission 2']); + $I->sendPostAsJson('/mission', ['content' => 'mission 3']); + + $room = $I->grabEntityFromRepository(Room::class, ['name' => 'Admin\'s room']); + + // Add players + $I->createPlayerAndUpdateHeaders($I, 'John'); + $I->sendPatchAsJson('/user', ['room' => $room->getId()]); + + $I->createPlayerAndUpdateHeaders($I, 'Jane'); + $I->sendPatchAsJson('/user', ['room' => $room->getId()]); + + $I->createPlayerAndUpdateHeaders($I, 'Doe'); + $I->sendPatchAsJson('/user', ['room' => $room->getId()]); + + // Start the game as game master + $I->setAdminJwtHeader($I); + $I->sendPatchAsJson(sprintf('/room/%s', $room->getId()), ['status' => 'IN_GAME']); + $I->seeResponseCodeIsSuccessful(); + + // Verify admin has SPECTATING status but is a master + $I->sendGetAsJson('/user/me'); + $I->seeResponseContainsJson([ + 'currentPlayer' => [ + 'status' => PlayerStatus::SPECTATING->value, + 'isMaster' => true, + ], + ]); + + // Get room data as game master - should see full data including missions + $I->sendGetAsJson(sprintf('/room/%s', $room->getId())); + $I->seeResponseCodeIsSuccessful(); + + /** @var array $response */ + $response = json_decode($I->grabResponse(), true); + Assert::assertArrayHasKey('missions', $response); + } + + public function testSpectatorCannotJoinRegularWhenAlreadySpectating(ApiTester $I): void + { + // Create admin and room with spectators allowed + $I->createAdminAndUpdateHeaders($I); + $I->sendPostAsJson('room'); + + $room = $I->grabEntityFromRepository(Room::class, ['name' => 'Admin\'s room']); + + // Enable spectators + $I->sendPatchAsJson(sprintf('/room/%s', $room->getId()), ['allowSpectators' => true]); + + // Join as spectator + $I->createPlayerAndUpdateHeaders($I, 'Spectator'); + $I->sendPatchAsJson('/user', ['room' => $room->getId(), 'spectate' => true]); + $I->seeResponseCodeIsSuccessful(); + + // Verify player status is SPECTATING + $I->seeInRepository(Player::class, [ + 'name' => 'Spectator', + 'status' => PlayerStatus::SPECTATING, + ]); + } +} diff --git a/tests/Unit/Infrastructure/Security/Voters/RoomVoterSpectateTest.php b/tests/Unit/Infrastructure/Security/Voters/RoomVoterSpectateTest.php new file mode 100644 index 0000000..d0c0780 --- /dev/null +++ b/tests/Unit/Infrastructure/Security/Voters/RoomVoterSpectateTest.php @@ -0,0 +1,152 @@ +playerRepository = $this->prophesize(PlayerRepository::class); + $this->roomVoter = new RoomVoter($this->playerRepository->reveal()); + parent::setUp(); + } + + public function testCanSpectateWhenAllowedAndSpectatingStatus(): void + { + $room = $this->make(Room::class, [ + 'isAllowSpectators' => Expected::atLeastOnce(true), + ]); + + $player = $this->make(Player::class, [ + 'getStatus' => Expected::atLeastOnce(PlayerStatus::SPECTATING), + 'getRoom' => Expected::atLeastOnce($room), + ]); + + $user = $this->prophesize(User::class); + + $this->playerRepository->getCurrentUserPlayer($user->reveal()) + ->shouldBeCalledOnce() + ->willReturn($player); + + $token = $this->prophesize(TokenInterface::class); + $token->getUser()->willReturn($user->reveal()); + + $result = $this->roomVoter->vote($token->reveal(), $room, [RoomVoter::SPECTATE_ROOM]); + + $this->assertEquals(1, $result); // VoterInterface::ACCESS_GRANTED + } + + public function testCannotSpectateWhenNotAllowed(): void + { + $room = $this->make(Room::class, [ + 'isAllowSpectators' => Expected::atLeastOnce(false), + ]); + + // Due to short-circuit evaluation, getStatus and getRoom won't be called + // when isAllowSpectators returns false + $player = $this->make(Player::class); + + $user = $this->prophesize(User::class); + + $this->playerRepository->getCurrentUserPlayer($user->reveal()) + ->shouldBeCalledOnce() + ->willReturn($player); + + $token = $this->prophesize(TokenInterface::class); + $token->getUser()->willReturn($user->reveal()); + + $result = $this->roomVoter->vote($token->reveal(), $room, [RoomVoter::SPECTATE_ROOM]); + + $this->assertEquals(-1, $result); // VoterInterface::ACCESS_DENIED + } + + public function testCannotSpectateWhenNotSpectatingStatus(): void + { + $room = $this->make(Room::class, [ + 'isAllowSpectators' => Expected::atLeastOnce(true), + ]); + + $player = $this->make(Player::class, [ + 'getStatus' => Expected::atLeastOnce(PlayerStatus::ALIVE), + 'getRoom' => Expected::atLeastOnce($room), + ]); + + $user = $this->prophesize(User::class); + + $this->playerRepository->getCurrentUserPlayer($user->reveal()) + ->shouldBeCalledOnce() + ->willReturn($player); + + $token = $this->prophesize(TokenInterface::class); + $token->getUser()->willReturn($user->reveal()); + + $result = $this->roomVoter->vote($token->reveal(), $room, [RoomVoter::SPECTATE_ROOM]); + + $this->assertEquals(-1, $result); // VoterInterface::ACCESS_DENIED + } + + public function testCannotSpectateWhenNotInRoom(): void + { + $room = $this->make(Room::class, [ + 'isAllowSpectators' => Expected::atLeastOnce(true), + ]); + + $otherRoom = $this->make(Room::class); + + // Due to short-circuit evaluation, getStatus won't be called + // when player->getRoom() !== $room + $player = $this->make(Player::class, [ + 'getRoom' => Expected::atLeastOnce($otherRoom), + ]); + + $user = $this->prophesize(User::class); + + $this->playerRepository->getCurrentUserPlayer($user->reveal()) + ->shouldBeCalledOnce() + ->willReturn($player); + + $token = $this->prophesize(TokenInterface::class); + $token->getUser()->willReturn($user->reveal()); + + $result = $this->roomVoter->vote($token->reveal(), $room, [RoomVoter::SPECTATE_ROOM]); + + $this->assertEquals(-1, $result); // VoterInterface::ACCESS_DENIED + } + + public function testCannotSpectateWhenNoPlayer(): void + { + $room = $this->make(Room::class); + + $user = $this->prophesize(User::class); + + $this->playerRepository->getCurrentUserPlayer($user->reveal()) + ->shouldBeCalledOnce() + ->willReturn(null); + + $token = $this->prophesize(TokenInterface::class); + $token->getUser()->willReturn($user->reveal()); + + $result = $this->roomVoter->vote($token->reveal(), $room, [RoomVoter::SPECTATE_ROOM]); + + $this->assertEquals(-1, $result); // VoterInterface::ACCESS_DENIED + } +} From 808ff42972418bc999f8de2a8106fb11b45e9440 Mon Sep 17 00:00:00 2001 From: Arty Date: Sun, 18 Jan 2026 13:33:00 +0100 Subject: [PATCH 2/4] Address review feedback for spectator mode - Remove unused SPECTATE_ROOM voter code and related unit tests - Add filter_var validation for spectate parameter to ensure boolean type - Fix incomplete test by renaming and adding proper test coverage for existing player rejoin scenario --- src/Api/Controller/UserController.php | 2 +- .../Security/Voters/RoomVoter.php | 13 +- tests/Api/SpectatorModeCest.php | 43 ++++- .../Security/Voters/RoomVoterSpectateTest.php | 152 ------------------ 4 files changed, 44 insertions(+), 166 deletions(-) delete mode 100644 tests/Unit/Infrastructure/Security/Voters/RoomVoterSpectateTest.php diff --git a/src/Api/Controller/UserController.php b/src/Api/Controller/UserController.php index 9558d59..22b9686 100644 --- a/src/Api/Controller/UserController.php +++ b/src/Api/Controller/UserController.php @@ -147,7 +147,7 @@ public function patchUser(Request $request): JsonResponse // Handle room change if (array_key_exists('room', $data)) { $newRoomId = $data['room']; - $joinAsSpectator = $data['spectate'] ?? false; + $joinAsSpectator = filter_var($data['spectate'] ?? false, FILTER_VALIDATE_BOOLEAN); if ($newRoomId !== null) { $newRoom = $this->roomRepository->findOneBy(['id' => $newRoomId]); diff --git a/src/Infrastructure/Security/Voters/RoomVoter.php b/src/Infrastructure/Security/Voters/RoomVoter.php index 0c3f469..fe32646 100644 --- a/src/Infrastructure/Security/Voters/RoomVoter.php +++ b/src/Infrastructure/Security/Voters/RoomVoter.php @@ -5,7 +5,6 @@ namespace App\Infrastructure\Security\Voters; use App\Domain\Player\Entity\Player; -use App\Domain\Player\Enum\PlayerStatus; use App\Domain\Player\PlayerRepository; use App\Domain\Room\Entity\Room; use App\Domain\User\Entity\User; @@ -17,7 +16,6 @@ class RoomVoter extends Voter public const string EDIT_ROOM = 'edit_room'; public const string VIEW_ROOM = 'view_room'; public const string CREATE_ROOM = 'create_room'; - public const string SPECTATE_ROOM = 'spectate_room'; public function __construct( private readonly PlayerRepository $playerRepository, @@ -26,7 +24,7 @@ public function __construct( protected function supports(string $attribute, mixed $subject): bool { - if (!in_array($attribute, [self::EDIT_ROOM, self::VIEW_ROOM, self::CREATE_ROOM, self::SPECTATE_ROOM], true)) { + if (!in_array($attribute, [self::EDIT_ROOM, self::VIEW_ROOM, self::CREATE_ROOM], true)) { return false; } @@ -51,7 +49,6 @@ protected function voteOnAttribute(string $attribute, mixed $subject, TokenInter self::VIEW_ROOM => $currentPlayer && $this->canView($subject, $currentPlayer), self::EDIT_ROOM => $currentPlayer && $this->canEdit($subject, $currentPlayer), self::CREATE_ROOM => !$currentPlayer && $this->canCreate($currentPlayer), - self::SPECTATE_ROOM => $currentPlayer && $this->canSpectate($subject, $currentPlayer), default => throw new \LogicException('This code should not be reached'), }; } @@ -71,12 +68,4 @@ private function canCreate(?Player $player = null): bool { return !$player?->getRoom(); } - - private function canSpectate(mixed $room, Player $player): bool - { - return $room instanceof Room - && $room->isAllowSpectators() - && $player->getRoom() === $room - && $player->getStatus() === PlayerStatus::SPECTATING; - } } diff --git a/tests/Api/SpectatorModeCest.php b/tests/Api/SpectatorModeCest.php index 72a0aee..2bf0af2 100644 --- a/tests/Api/SpectatorModeCest.php +++ b/tests/Api/SpectatorModeCest.php @@ -215,7 +215,7 @@ public function testGameMasterSeesEverythingDespiteSpectatingStatus(ApiTester $I Assert::assertArrayHasKey('missions', $response); } - public function testSpectatorCannotJoinRegularWhenAlreadySpectating(ApiTester $I): void + public function testSpectatorStatusPersistsAfterJoining(ApiTester $I): void { // Create admin and room with spectators allowed $I->createAdminAndUpdateHeaders($I); @@ -236,5 +236,46 @@ public function testSpectatorCannotJoinRegularWhenAlreadySpectating(ApiTester $I 'name' => 'Spectator', 'status' => PlayerStatus::SPECTATING, ]); + + // Verify status persists when fetching user info + $I->sendGetAsJson('/user/me'); + $I->seeResponseContainsJson([ + 'currentPlayer' => [ + 'status' => PlayerStatus::SPECTATING->value, + ], + ]); + } + + public function testExistingPlayerCannotRejoinAsSpectator(ApiTester $I): void + { + // Create admin and room with spectators allowed + $I->createAdminAndUpdateHeaders($I); + $I->sendPostAsJson('room'); + + $room = $I->grabEntityFromRepository(Room::class, ['name' => 'Admin\'s room']); + + // Enable spectators + $I->sendPatchAsJson(sprintf('/room/%s', $room->getId()), ['allowSpectators' => true]); + + // First join as regular player + $I->createPlayerAndUpdateHeaders($I, 'RegularPlayer'); + $I->sendPatchAsJson('/user', ['room' => $room->getId()]); + $I->seeResponseCodeIsSuccessful(); + + // Verify player is ALIVE (not spectating) + $I->seeInRepository(Player::class, [ + 'name' => 'RegularPlayer', + 'status' => PlayerStatus::ALIVE, + ]); + + // Try to rejoin the same room as spectator (should keep existing player, not change status) + $I->sendPatchAsJson('/user', ['room' => $room->getId(), 'spectate' => true]); + $I->seeResponseCodeIsSuccessful(); + + // Player should still be ALIVE since they already exist in the room + $I->seeInRepository(Player::class, [ + 'name' => 'RegularPlayer', + 'status' => PlayerStatus::ALIVE, + ]); } } diff --git a/tests/Unit/Infrastructure/Security/Voters/RoomVoterSpectateTest.php b/tests/Unit/Infrastructure/Security/Voters/RoomVoterSpectateTest.php deleted file mode 100644 index d0c0780..0000000 --- a/tests/Unit/Infrastructure/Security/Voters/RoomVoterSpectateTest.php +++ /dev/null @@ -1,152 +0,0 @@ -playerRepository = $this->prophesize(PlayerRepository::class); - $this->roomVoter = new RoomVoter($this->playerRepository->reveal()); - parent::setUp(); - } - - public function testCanSpectateWhenAllowedAndSpectatingStatus(): void - { - $room = $this->make(Room::class, [ - 'isAllowSpectators' => Expected::atLeastOnce(true), - ]); - - $player = $this->make(Player::class, [ - 'getStatus' => Expected::atLeastOnce(PlayerStatus::SPECTATING), - 'getRoom' => Expected::atLeastOnce($room), - ]); - - $user = $this->prophesize(User::class); - - $this->playerRepository->getCurrentUserPlayer($user->reveal()) - ->shouldBeCalledOnce() - ->willReturn($player); - - $token = $this->prophesize(TokenInterface::class); - $token->getUser()->willReturn($user->reveal()); - - $result = $this->roomVoter->vote($token->reveal(), $room, [RoomVoter::SPECTATE_ROOM]); - - $this->assertEquals(1, $result); // VoterInterface::ACCESS_GRANTED - } - - public function testCannotSpectateWhenNotAllowed(): void - { - $room = $this->make(Room::class, [ - 'isAllowSpectators' => Expected::atLeastOnce(false), - ]); - - // Due to short-circuit evaluation, getStatus and getRoom won't be called - // when isAllowSpectators returns false - $player = $this->make(Player::class); - - $user = $this->prophesize(User::class); - - $this->playerRepository->getCurrentUserPlayer($user->reveal()) - ->shouldBeCalledOnce() - ->willReturn($player); - - $token = $this->prophesize(TokenInterface::class); - $token->getUser()->willReturn($user->reveal()); - - $result = $this->roomVoter->vote($token->reveal(), $room, [RoomVoter::SPECTATE_ROOM]); - - $this->assertEquals(-1, $result); // VoterInterface::ACCESS_DENIED - } - - public function testCannotSpectateWhenNotSpectatingStatus(): void - { - $room = $this->make(Room::class, [ - 'isAllowSpectators' => Expected::atLeastOnce(true), - ]); - - $player = $this->make(Player::class, [ - 'getStatus' => Expected::atLeastOnce(PlayerStatus::ALIVE), - 'getRoom' => Expected::atLeastOnce($room), - ]); - - $user = $this->prophesize(User::class); - - $this->playerRepository->getCurrentUserPlayer($user->reveal()) - ->shouldBeCalledOnce() - ->willReturn($player); - - $token = $this->prophesize(TokenInterface::class); - $token->getUser()->willReturn($user->reveal()); - - $result = $this->roomVoter->vote($token->reveal(), $room, [RoomVoter::SPECTATE_ROOM]); - - $this->assertEquals(-1, $result); // VoterInterface::ACCESS_DENIED - } - - public function testCannotSpectateWhenNotInRoom(): void - { - $room = $this->make(Room::class, [ - 'isAllowSpectators' => Expected::atLeastOnce(true), - ]); - - $otherRoom = $this->make(Room::class); - - // Due to short-circuit evaluation, getStatus won't be called - // when player->getRoom() !== $room - $player = $this->make(Player::class, [ - 'getRoom' => Expected::atLeastOnce($otherRoom), - ]); - - $user = $this->prophesize(User::class); - - $this->playerRepository->getCurrentUserPlayer($user->reveal()) - ->shouldBeCalledOnce() - ->willReturn($player); - - $token = $this->prophesize(TokenInterface::class); - $token->getUser()->willReturn($user->reveal()); - - $result = $this->roomVoter->vote($token->reveal(), $room, [RoomVoter::SPECTATE_ROOM]); - - $this->assertEquals(-1, $result); // VoterInterface::ACCESS_DENIED - } - - public function testCannotSpectateWhenNoPlayer(): void - { - $room = $this->make(Room::class); - - $user = $this->prophesize(User::class); - - $this->playerRepository->getCurrentUserPlayer($user->reveal()) - ->shouldBeCalledOnce() - ->willReturn(null); - - $token = $this->prophesize(TokenInterface::class); - $token->getUser()->willReturn($user->reveal()); - - $result = $this->roomVoter->vote($token->reveal(), $room, [RoomVoter::SPECTATE_ROOM]); - - $this->assertEquals(-1, $result); // VoterInterface::ACCESS_DENIED - } -} From f2818a4f3f2c7c30bdc888ccbb339fdd02b88c96 Mon Sep 17 00:00:00 2001 From: Arty Date: Sun, 18 Jan 2026 14:03:11 +0100 Subject: [PATCH 3/4] fixup! Address review feedback for spectator mode --- tests/Api/SpectatorModeCest.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/Api/SpectatorModeCest.php b/tests/Api/SpectatorModeCest.php index 2bf0af2..ed25e97 100644 --- a/tests/Api/SpectatorModeCest.php +++ b/tests/Api/SpectatorModeCest.php @@ -66,7 +66,7 @@ public function testCannotJoinAsSpectatorWhenDisabled(ApiTester $I): void $I->createPlayerAndUpdateHeaders($I, 'Spectator'); $I->sendPatchAsJson('/user', ['room' => $room->getId(), 'spectate' => true]); $I->seeResponseCodeIs(400); - $I->seeResponseContainsJson(['message' => 'ROOM_SPECTATORS_NOT_ALLOWED']); + $I->seeResponseContainsJson(['detail' => 'ROOM_SPECTATORS_NOT_ALLOWED']); } public function testSpectatorSeesLimitedRoomData(ApiTester $I): void @@ -75,8 +75,6 @@ public function testSpectatorSeesLimitedRoomData(ApiTester $I): void $I->createAdminAndUpdateHeaders($I); $I->sendPostAsJson('room'); $I->sendPostAsJson('/mission', ['content' => 'Secret mission 1']); - $I->sendPostAsJson('/mission', ['content' => 'Secret mission 2']); - $I->sendPostAsJson('/mission', ['content' => 'Secret mission 3']); $room = $I->grabEntityFromRepository(Room::class, ['name' => 'Admin\'s room']); @@ -86,9 +84,11 @@ public function testSpectatorSeesLimitedRoomData(ApiTester $I): void // Add regular players $I->createPlayerAndUpdateHeaders($I, 'John'); $I->sendPatchAsJson('/user', ['room' => $room->getId()]); + $I->sendPostAsJson('/mission', ['content' => 'Secret mission 2']); $I->createPlayerAndUpdateHeaders($I, 'Jane'); $I->sendPatchAsJson('/user', ['room' => $room->getId()]); + $I->sendPostAsJson('/mission', ['content' => 'Secret mission 3']); // Start the game $I->setAdminJwtHeader($I); @@ -144,17 +144,17 @@ public function testRegularPlayerSeesFullRoomData(ApiTester $I): void $I->createAdminAndUpdateHeaders($I); $I->sendPostAsJson('room'); $I->sendPostAsJson('/mission', ['content' => 'mission 1']); - $I->sendPostAsJson('/mission', ['content' => 'mission 2']); - $I->sendPostAsJson('/mission', ['content' => 'mission 3']); $room = $I->grabEntityFromRepository(Room::class, ['name' => 'Admin\'s room']); // Add regular players $I->createPlayerAndUpdateHeaders($I, 'John'); $I->sendPatchAsJson('/user', ['room' => $room->getId()]); + $I->sendPostAsJson('/mission', ['content' => 'mission 2']); $I->createPlayerAndUpdateHeaders($I, 'Jane'); $I->sendPatchAsJson('/user', ['room' => $room->getId()]); + $I->sendPostAsJson('/mission', ['content' => 'mission 3']); // Start the game $I->setAdminJwtHeader($I); From 24ada74d2e254e00e70ea3476eedd4613b6fa468 Mon Sep 17 00:00:00 2001 From: Arty Date: Sun, 18 Jan 2026 14:25:47 +0100 Subject: [PATCH 4/4] add autotag github workflow --- .github/workflows/autotag.yml | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 .github/workflows/autotag.yml diff --git a/.github/workflows/autotag.yml b/.github/workflows/autotag.yml new file mode 100644 index 0000000..f49f8ff --- /dev/null +++ b/.github/workflows/autotag.yml @@ -0,0 +1,28 @@ +name: Bump version +on: + pull_request: + types: + - closed + branches: + - main + +jobs: + build: + if: | + github.event.pull_request.merged == true && + !contains(github.event.pull_request.labels.*.name, 'github_actions') + runs-on: ubuntu-22.04 + permissions: + contents: write + steps: + - uses: actions/checkout@v6 + with: + ref: ${{ github.event.pull_request.merge_commit_sha }} + fetch-depth: '0' + + - name: Bump version and push tag + uses: anothrNick/github-tag-action@v1 + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + TAG_PREFIX: v + DEFAULT_BUMP: patch