From 3c7dc88fe7297f7e81a5fa8a4bd47f6dc94dc27d Mon Sep 17 00:00:00 2001 From: Matthieu Date: Wed, 15 Apr 2026 14:17:18 +0200 Subject: [PATCH] feat(core) : RBAC #344 - UserRbacProcessor + endpoint /users/{id}/rbac Ajoute une operation Patch dediee `PATCH /api/users/{id}/rbac` (nom `user_rbac_patch`) qui accepte exclusivement les champs RBAC isAdmin, roles et directPermissions via le groupe user:rbac:write. L'endpoint est separe volontairement du Patch profil existant pour isoler la modification des droits de celle des donnees profil (decision 0fc4e16). UserRbacProcessor delegue au PersistProcessor Doctrine decore et applique une garde auto-suicide : un admin ne peut pas retirer ses propres droits administrateur (compare l'etat entrant a l'etat UnitOfWork). La garde 'dernier admin' globale est reportee au ticket #345. La propriete Doctrine $roles est renommee $rbacRoles pour eviter la collision avec UserInterface::getRoles() (qui renvoie list) lors de la normalization API Platform. La cle JSON reste `roles` grace a SerializedName, le contrat API est inchange. Tests : 6 unitaires (UserRbacProcessorTest) + 8 fonctionnels (UserRbacApiTest) couvrant promotion admin, remplacement des collections roles/directPermissions, 401/403, filtrage du groupe denormalization (`username` ignore), preservation de isAdmin sur le Patch profil, et garde auto-suicide. --- src/Module/Core/Domain/Entity/User.php | 46 ++- .../State/Processor/UserRbacProcessor.php | 71 +++++ tests/Module/Core/Api/UserRbacApiTest.php | 271 ++++++++++++++++++ .../State/Processor/UserRbacProcessorTest.php | 216 ++++++++++++++ 4 files changed, 589 insertions(+), 15 deletions(-) create mode 100644 src/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessor.php create mode 100644 tests/Module/Core/Api/UserRbacApiTest.php create mode 100644 tests/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessorTest.php 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; + } +}