diff --git a/src/Module/Core/Domain/Entity/User.php b/src/Module/Core/Domain/Entity/User.php index b571488..a62dbd5 100644 --- a/src/Module/Core/Domain/Entity/User.php +++ b/src/Module/Core/Domain/Entity/User.php @@ -11,6 +11,7 @@ use ApiPlatform\Metadata\GetCollection; use ApiPlatform\Metadata\Patch; use ApiPlatform\Metadata\Post; use App\Module\Core\Infrastructure\ApiPlatform\State\Processor\UserPasswordHasherProcessor; +use App\Module\Core\Infrastructure\ApiPlatform\State\Processor\UserRbacProcessor; use App\Module\Core\Infrastructure\ApiPlatform\State\Provider\MeProvider; use App\Module\Core\Infrastructure\Doctrine\DoctrineUserRepository; use DateTimeImmutable; @@ -20,6 +21,7 @@ use Doctrine\ORM\Mapping as ORM; use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface; use Symfony\Component\Security\Core\User\UserInterface; use Symfony\Component\Serializer\Attribute\Groups; +use Symfony\Component\Serializer\Attribute\SerializedName; #[ApiResource( operations: [ @@ -36,6 +38,15 @@ use Symfony\Component\Serializer\Attribute\Groups; ), new Post(security: "is_granted('ROLE_ADMIN')", processor: UserPasswordHasherProcessor::class), new Patch(security: "is_granted('ROLE_ADMIN')", processor: UserPasswordHasherProcessor::class), + new Patch( + name: 'user_rbac_patch', + uriTemplate: '/users/{id}/rbac', + // TODO ticket #345 : remplacer par is_granted('core.users.manage') + security: "is_granted('ROLE_ADMIN')", + normalizationContext: ['groups' => ['user:list']], + denormalizationContext: ['groups' => ['user:rbac:write']], + processor: UserRbacProcessor::class, + ), new Delete(security: "is_granted('ROLE_ADMIN')"), ], denormalizationContext: ['groups' => ['user:write']], @@ -55,7 +66,7 @@ class User implements UserInterface, PasswordAuthenticatedUserInterface private ?string $username = null; #[ORM\Column(name: 'is_admin', options: ['default' => false])] - #[Groups(['me:read', 'user:list'])] + #[Groups(['me:read', 'user:list', 'user:rbac:write'])] private bool $isAdmin = false; /** @@ -70,20 +81,25 @@ class User implements UserInterface, PasswordAuthenticatedUserInterface */ #[ORM\ManyToMany(targetEntity: Role::class, fetch: 'EAGER')] #[ORM\JoinTable(name: 'user_role')] - #[Groups(['me:read', 'user:list'])] - private Collection $roles; + #[Groups(['me:read', 'user:list', 'user:rbac:write'])] + // La propriete s'appelle `rbacRoles` cote PHP pour ne pas entrer en + // collision avec UserInterface::getRoles() (qui renvoie list) ; + // on reexpose la cle JSON sous `roles` via SerializedName pour rester + // conforme au contrat API documente dans le ticket #344. + #[SerializedName('roles')] + private Collection $rbacRoles; /** * Les permissions directes accordees hors des roles. * - * Meme justification EAGER que pour $roles : garantie que + * Meme justification EAGER que pour $rbacRoles : garantie que * getEffectivePermissions() fonctionne dans tous les contextes de chargement. * * @var Collection */ #[ORM\ManyToMany(targetEntity: Permission::class, fetch: 'EAGER')] #[ORM\JoinTable(name: 'user_permission')] - #[Groups(['me:read', 'user:list'])] + #[Groups(['me:read', 'user:list', 'user:rbac:write'])] private Collection $directPermissions; #[ORM\Column] @@ -98,7 +114,7 @@ class User implements UserInterface, PasswordAuthenticatedUserInterface public function __construct() { $this->createdAt = new DateTimeImmutable(); - $this->roles = new ArrayCollection(); + $this->rbacRoles = new ArrayCollection(); $this->directPermissions = new ArrayCollection(); } @@ -131,10 +147,10 @@ class User implements UserInterface, PasswordAuthenticatedUserInterface * ROLE_ADMIN est ajoute si l'utilisateur porte le flag is_admin — c'est le * SEUL levier technique de bypass RBAC (cf. section 11 du spec). * - * Important : ne JAMAIS iterer $this->roles (la Collection de Role) ici. - * Cette methode peut etre appelee pendant un refresh JWT, moment ou la - * Collection peut ne pas etre hydratee. On se contente d'un calcul base - * sur un scalaire. + * Important : ne JAMAIS iterer $this->rbacRoles (la Collection de Role) + * ici. Cette methode peut etre appelee pendant un refresh JWT, moment ou + * la Collection peut ne pas etre hydratee. On se contente d'un calcul + * base sur un scalaire. * * @return list */ @@ -170,13 +186,13 @@ class User implements UserInterface, PasswordAuthenticatedUserInterface */ public function getRbacRoles(): Collection { - return $this->roles; + return $this->rbacRoles; } public function addRbacRole(Role $role): static { - if (!$this->roles->contains($role)) { - $this->roles->add($role); + if (!$this->rbacRoles->contains($role)) { + $this->rbacRoles->add($role); } return $this; @@ -184,7 +200,7 @@ class User implements UserInterface, PasswordAuthenticatedUserInterface public function removeRbacRole(Role $role): static { - $this->roles->removeElement($role); + $this->rbacRoles->removeElement($role); return $this; } @@ -229,7 +245,7 @@ class User implements UserInterface, PasswordAuthenticatedUserInterface { $codes = []; - foreach ($this->roles as $role) { + foreach ($this->rbacRoles as $role) { foreach ($role->getPermissions() as $permission) { $codes[$permission->getCode()] = true; } diff --git a/src/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessor.php b/src/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessor.php new file mode 100644 index 0000000..d97f846 --- /dev/null +++ b/src/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessor.php @@ -0,0 +1,71 @@ + + */ +final class UserRbacProcessor implements ProcessorInterface +{ + public function __construct( + #[Autowire(service: 'api_platform.doctrine.orm.state.persist_processor')] + private readonly ProcessorInterface $persistProcessor, + private readonly EntityManagerInterface $entityManager, + private readonly Security $security, + ) {} + + public function process(mixed $data, Operation $operation, array $uriVariables = [], array $context = []): mixed + { + if (!$data instanceof User) { + // Securite : si le provider n'a pas fourni un User, on delegue + // quand meme pour ne pas etouffer un bug de configuration. + return $this->persistProcessor->process($data, $operation, $uriVariables, $context); + } + + $currentUser = $this->security->getUser(); + + // Garde auto-suicide : l'user courant ne peut pas retirer son propre + // flag admin. On ne compare que si la cible == l'user courant. + if ($currentUser instanceof User + && null !== $currentUser->getId() + && $currentUser->getId() === $data->getId() + ) { + $originalData = $this->entityManager->getUnitOfWork()->getOriginalEntityData($data); + $wasAdmin = $originalData['isAdmin'] ?? null; + + if (true === $wasAdmin && false === $data->isAdmin()) { + throw new BadRequestHttpException( + 'Vous ne pouvez pas retirer vos propres droits administrateur.' + ); + } + } + + return $this->persistProcessor->process($data, $operation, $uriVariables, $context); + } +} diff --git a/tests/Module/Core/Api/UserRbacApiTest.php b/tests/Module/Core/Api/UserRbacApiTest.php new file mode 100644 index 0000000..9984e54 --- /dev/null +++ b/tests/Module/Core/Api/UserRbacApiTest.php @@ -0,0 +1,271 @@ +getEm(); + + $this->cleanupTestData(); + + /** @var UserPasswordHasherInterface $hasher */ + $hasher = self::getContainer()->get(UserPasswordHasherInterface::class); + + // User cible standard (non admin). + $target = new User(); + $target->setUsername('test_target'); + $target->setIsAdmin(false); + $target->setPassword($hasher->hashPassword($target, 'secret')); + $em->persist($target); + + // User admin dedie pour le cas d'auto-suicide (pas l'admin fixture). + $selfAdmin = new User(); + $selfAdmin->setUsername('test_self_admin'); + $selfAdmin->setIsAdmin(true); + $selfAdmin->setPassword($hasher->hashPassword($selfAdmin, 'secret')); + $em->persist($selfAdmin); + + // Role custom pour tester le remplacement de la collection roles. + $role = new Role('test_editor', 'Editeur (test)', false); + $em->persist($role); + + // Permission custom pour tester directPermissions. + $permission = new Permission('test.core.users.view', 'View users (test)', 'core'); + $em->persist($permission); + + $em->flush(); + $em->clear(); + } + + protected function tearDown(): void + { + $this->cleanupTestData(); + parent::tearDown(); + } + + public function testPatchRbacPromotesUserToAdmin(): void + { + $target = $this->getEm()->getRepository(User::class)->findOneBy(['username' => 'test_target']); + self::assertNotNull($target); + + $client = $this->authenticatedClient('admin', 'admin'); + $client->request('PATCH', '/api/users/'.$target->getId().'/rbac', [ + 'headers' => ['Content-Type' => 'application/merge-patch+json'], + 'json' => ['isAdmin' => true], + ]); + + self::assertResponseIsSuccessful(); + + $em = $this->getEm(); + $em->clear(); + + /** @var User $reloaded */ + $reloaded = $em->getRepository(User::class)->findOneBy(['username' => 'test_target']); + self::assertTrue($reloaded->isAdmin()); + } + + public function testPatchRbacReplacesRolesCollection(): void + { + $em = $this->getEm(); + $target = $em->getRepository(User::class)->findOneBy(['username' => 'test_target']); + $role = $em->getRepository(Role::class)->findOneBy(['code' => 'test_editor']); + self::assertNotNull($target); + self::assertNotNull($role); + + $client = $this->authenticatedClient('admin', 'admin'); + $client->request('PATCH', '/api/users/'.$target->getId().'/rbac', [ + 'headers' => ['Content-Type' => 'application/merge-patch+json'], + 'json' => ['roles' => ['/api/roles/'.$role->getId()]], + ]); + + self::assertResponseIsSuccessful(); + + $em = $this->getEm(); + $em->clear(); + + /** @var User $reloaded */ + $reloaded = $em->getRepository(User::class)->findOneBy(['username' => 'test_target']); + self::assertCount(1, $reloaded->getRbacRoles()); + self::assertSame('test_editor', $reloaded->getRbacRoles()->first()->getCode()); + } + + public function testPatchRbacReplacesDirectPermissionsCollection(): void + { + $em = $this->getEm(); + $target = $em->getRepository(User::class)->findOneBy(['username' => 'test_target']); + $permission = $em->getRepository(Permission::class)->findOneBy(['code' => 'test.core.users.view']); + self::assertNotNull($target); + self::assertNotNull($permission); + + $client = $this->authenticatedClient('admin', 'admin'); + $client->request('PATCH', '/api/users/'.$target->getId().'/rbac', [ + 'headers' => ['Content-Type' => 'application/merge-patch+json'], + 'json' => ['directPermissions' => ['/api/permissions/'.$permission->getId()]], + ]); + + self::assertResponseIsSuccessful(); + + $em = $this->getEm(); + $em->clear(); + + /** @var User $reloaded */ + $reloaded = $em->getRepository(User::class)->findOneBy(['username' => 'test_target']); + self::assertCount(1, $reloaded->getDirectPermissions()); + self::assertSame('test.core.users.view', $reloaded->getDirectPermissions()->first()->getCode()); + } + + public function testPatchRbacAsStandardUserReturns403(): void + { + $target = $this->getEm()->getRepository(User::class)->findOneBy(['username' => 'test_target']); + self::assertNotNull($target); + + $client = $this->authenticatedClient('alice', 'alice'); + $client->request('PATCH', '/api/users/'.$target->getId().'/rbac', [ + 'headers' => ['Content-Type' => 'application/merge-patch+json'], + 'json' => ['isAdmin' => true], + ]); + + self::assertResponseStatusCodeSame(403); + } + + public function testPatchRbacUnauthenticatedReturns401(): void + { + $target = $this->getEm()->getRepository(User::class)->findOneBy(['username' => 'test_target']); + self::assertNotNull($target); + + $client = self::createClient(); + $client->request('PATCH', '/api/users/'.$target->getId().'/rbac', [ + 'headers' => ['Content-Type' => 'application/merge-patch+json'], + 'json' => ['isAdmin' => true], + ]); + + self::assertResponseStatusCodeSame(401); + } + + public function testPatchRbacIgnoresUsernameField(): void + { + $target = $this->getEm()->getRepository(User::class)->findOneBy(['username' => 'test_target']); + self::assertNotNull($target); + $targetId = $target->getId(); + + $client = $this->authenticatedClient('admin', 'admin'); + $client->request('PATCH', '/api/users/'.$targetId.'/rbac', [ + 'headers' => ['Content-Type' => 'application/merge-patch+json'], + 'json' => [ + 'username' => 'test_target_renamed', + 'isAdmin' => true, + ], + ]); + + self::assertResponseIsSuccessful(); + + $em = $this->getEm(); + $em->clear(); + + /** @var User $reloaded */ + $reloaded = $em->getRepository(User::class)->find($targetId); + // `username` n'est pas dans `user:rbac:write` : ignore en denormalization. + self::assertSame('test_target', $reloaded->getUsername()); + // `isAdmin` est bien applique. + self::assertTrue($reloaded->isAdmin()); + } + + public function testPatchProfileEndpointDoesNotModifyIsAdmin(): void + { + // Confirme la decision 0fc4e16 : `isAdmin` n'est plus dans `user:write`, + // donc `PATCH /api/users/{id}` sans `/rbac` ne peut plus promouvoir. + $target = $this->getEm()->getRepository(User::class)->findOneBy(['username' => 'test_target']); + self::assertNotNull($target); + $targetId = $target->getId(); + self::assertFalse($target->isAdmin()); + + $client = $this->authenticatedClient('admin', 'admin'); + $client->request('PATCH', '/api/users/'.$targetId, [ + 'headers' => ['Content-Type' => 'application/merge-patch+json'], + 'json' => ['isAdmin' => true], + ]); + + // Peu importe le code : le champ ne doit tout simplement pas bouger. + $em = $this->getEm(); + $em->clear(); + + /** @var User $reloaded */ + $reloaded = $em->getRepository(User::class)->find($targetId); + self::assertFalse($reloaded->isAdmin()); + } + + public function testPatchRbacSelfRemovingAdminReturns400(): void + { + // On utilise le user admin dedie (test_self_admin) pour ne pas + // corrompre l'admin fixture en cas de bug. + $em = $this->getEm(); + $selfAdmin = $em->getRepository(User::class)->findOneBy(['username' => 'test_self_admin']); + self::assertNotNull($selfAdmin); + $selfAdminId = $selfAdmin->getId(); + + $client = $this->authenticatedClient('test_self_admin', 'secret'); + $client->request('PATCH', '/api/users/'.$selfAdminId.'/rbac', [ + 'headers' => ['Content-Type' => 'application/merge-patch+json'], + 'json' => ['isAdmin' => false], + ]); + + self::assertResponseStatusCodeSame(400); + + $em = $this->getEm(); + $em->clear(); + + /** @var User $reloaded */ + $reloaded = $em->getRepository(User::class)->find($selfAdminId); + self::assertTrue($reloaded->isAdmin()); + } + + private function cleanupTestData(): void + { + $em = $this->getEm(); + + // Ordre important : delier les collections avant de supprimer les + // entites referencees pour que les FK cascade s'appliquent via le + // schema PostgreSQL. + $em->createQuery( + 'DELETE FROM '.User::class.' u WHERE u.username LIKE :prefix' + )->setParameter('prefix', self::TEST_USER_PREFIX.'%')->execute(); + + $em->createQuery( + 'DELETE FROM '.Role::class.' r WHERE r.code LIKE :prefix' + )->setParameter('prefix', self::TEST_ROLE_PREFIX.'%')->execute(); + + $em->createQuery( + 'DELETE FROM '.Permission::class.' p WHERE p.code LIKE :prefix' + )->setParameter('prefix', self::TEST_PERMISSION_PREFIX.'%')->execute(); + } +} diff --git a/tests/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessorTest.php b/tests/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessorTest.php new file mode 100644 index 0000000..6dce00b --- /dev/null +++ b/tests/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessorTest.php @@ -0,0 +1,216 @@ +persistProcessor = $this->createMock(ProcessorInterface::class); + $this->entityManager = $this->createMock(EntityManagerInterface::class); + $this->unitOfWork = $this->createMock(UnitOfWork::class); + $this->security = $this->createMock(Security::class); + + $this->entityManager->method('getUnitOfWork')->willReturn($this->unitOfWork); + + $this->processor = new UserRbacProcessor( + $this->persistProcessor, + $this->entityManager, + $this->security, + ); + } + + public function testPatchPromotesUserToAdminDelegatesToPersistProcessor(): void + { + $target = $this->buildUser(42, 'alice', false); + $target->setIsAdmin(true); + + $currentAdmin = $this->buildUser(1, 'admin', true); + $this->security->method('getUser')->willReturn($currentAdmin); + + // Cible != user courant : pas de lecture d'UnitOfWork necessaire. + $this->unitOfWork->expects(self::never())->method('getOriginalEntityData'); + + $this->persistProcessor + ->expects(self::once()) + ->method('process') + ->with($target) + ->willReturn($target) + ; + + $result = $this->processor->process($target, new Patch()); + + self::assertSame($target, $result); + } + + public function testPatchUpdatesRolesCollectionDelegatesToPersistProcessor(): void + { + $target = $this->buildUser(42, 'alice', false); + $target->addRbacRole(new Role('editor', 'Editor', false)); + + $currentAdmin = $this->buildUser(1, 'admin', true); + $this->security->method('getUser')->willReturn($currentAdmin); + + $this->persistProcessor + ->expects(self::once()) + ->method('process') + ->with($target) + ->willReturn($target) + ; + + $result = $this->processor->process($target, new Patch()); + + self::assertSame($target, $result); + self::assertCount(1, $result->getRbacRoles()); + } + + public function testPatchUpdatesDirectPermissionsCollectionDelegatesToPersistProcessor(): void + { + $target = $this->buildUser(42, 'alice', false); + $target->addDirectPermission(new Permission('core.users.view', 'View', 'core')); + + $currentAdmin = $this->buildUser(1, 'admin', true); + $this->security->method('getUser')->willReturn($currentAdmin); + + $this->persistProcessor + ->expects(self::once()) + ->method('process') + ->with($target) + ->willReturn($target) + ; + + $result = $this->processor->process($target, new Patch()); + + self::assertSame($target, $result); + self::assertCount(1, $result->getDirectPermissions()); + } + + public function testPatchSelfRemovingAdminThrowsBadRequestHttpException(): void + { + // Meme identifiant : l'user courant PATCH sa propre ressource. + $self = $this->buildUser(1, 'admin', false); + + $this->security->method('getUser')->willReturn($self); + + $this->unitOfWork + ->expects(self::once()) + ->method('getOriginalEntityData') + ->with($self) + ->willReturn([ + 'id' => 1, + 'username' => 'admin', + 'isAdmin' => true, + ]) + ; + + $this->persistProcessor->expects(self::never())->method('process'); + + $this->expectException(BadRequestHttpException::class); + $this->expectExceptionMessage('Vous ne pouvez pas retirer vos propres droits administrateur.'); + + $this->processor->process($self, new Patch()); + } + + public function testPatchAdminDemotingAnotherUserIsAllowed(): void + { + // Un admin qui retire isAdmin a quelqu'un d'autre : autorise. + $target = $this->buildUser(42, 'alice', false); + $current = $this->buildUser(1, 'admin', true); + + $this->security->method('getUser')->willReturn($current); + + // Cible != user courant : pas de verification d'auto-suicide. + $this->unitOfWork->expects(self::never())->method('getOriginalEntityData'); + + $this->persistProcessor + ->expects(self::once()) + ->method('process') + ->with($target) + ->willReturn($target) + ; + + $result = $this->processor->process($target, new Patch()); + + self::assertSame($target, $result); + } + + public function testPatchSelfKeepingAdminIsAllowed(): void + { + // L'user courant se PATCH lui-meme mais garde isAdmin = true : + // aucun auto-suicide, on delegue au PersistProcessor. + $self = $this->buildUser(1, 'admin', true); + + $this->security->method('getUser')->willReturn($self); + + $this->unitOfWork + ->expects(self::once()) + ->method('getOriginalEntityData') + ->with($self) + ->willReturn([ + 'id' => 1, + 'username' => 'admin', + 'isAdmin' => true, + ]) + ; + + $this->persistProcessor + ->expects(self::once()) + ->method('process') + ->with($self) + ->willReturn($self) + ; + + $result = $this->processor->process($self, new Patch()); + + self::assertSame($self, $result); + } + + /** + * Construit un User avec un id force via reflection (les mocks + * d'UnitOfWork n'alimentent pas l'id tout seul). + */ + private function buildUser(int $id, string $username, bool $isAdmin): User + { + $user = new User(); + $user->setUsername($username); + $user->setIsAdmin($isAdmin); + + $refl = new ReflectionClass($user); + $prop = $refl->getProperty('id'); + $prop->setAccessible(true); + $prop->setValue($user, $id); + + return $user; + } +}