From 80b63cd7d7e19270927aee858c5e09eada610b7b Mon Sep 17 00:00:00 2001 From: Matthieu Date: Wed, 15 Apr 2026 16:00:34 +0200 Subject: [PATCH] feat(core) : RBAC #345 - UserRbacProcessor last admin guard --- .../State/Processor/UserRbacProcessor.php | 46 ++-- .../State/Processor/UserRbacProcessorTest.php | 201 ++++++++++++++++-- 2 files changed, 214 insertions(+), 33 deletions(-) diff --git a/src/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessor.php b/src/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessor.php index 4215d74..9f0029b 100644 --- a/src/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessor.php +++ b/src/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessor.php @@ -7,6 +7,8 @@ namespace App\Module\Core\Infrastructure\ApiPlatform\State\Processor; use ApiPlatform\Metadata\Operation; use ApiPlatform\State\ProcessorInterface; use App\Module\Core\Domain\Entity\User; +use App\Module\Core\Domain\Exception\LastAdminProtectionException; +use App\Module\Core\Domain\Security\AdminHeadcountGuardInterface; use Doctrine\ORM\EntityManagerInterface; use LogicException; use Symfony\Bundle\SecurityBundle\Security; @@ -21,14 +23,12 @@ use Symfony\Component\HttpKernel\Exception\BadRequestHttpException; * ne touche JAMAIS au mot de passe — c'est une separation volontaire avec le * UserPasswordHasherProcessor qui gere le endpoint profil `/api/users/{id}`. * - * Gardes metier : + * Gardes metier (dans l'ordre d'execution) : * - Auto-suicide : un admin ne peut pas retirer son propre flag `isAdmin`. - * On compare l'etat entrant a l'etat d'origine via l'UnitOfWork Doctrine, - * en restreignant la verification au couple "user courant == user cible". - * - * TODO ticket #345 : garde "dernier admin" globale via inventaire des admins - * restants (empeche de retirer `isAdmin` au dernier admin de l'instance, meme - * si ce n'est pas sa propre operation). + * Cas particulier plus strict, avec message dedie. + * - Dernier admin global : impossible de retirer `isAdmin` si c'est le + * dernier administrateur de l'instance, meme par un tiers. Enforce via + * AdminHeadcountGuardInterface. * * @implements ProcessorInterface */ @@ -39,6 +39,7 @@ final class UserRbacProcessor implements ProcessorInterface private readonly ProcessorInterface $persistProcessor, private readonly EntityManagerInterface $entityManager, private readonly Security $security, + private readonly AdminHeadcountGuardInterface $adminHeadcountGuard, ) {} public function process(mixed $data, Operation $operation, array $uriVariables = [], array $context = []): mixed @@ -56,19 +57,26 @@ final class UserRbacProcessor implements ProcessorInterface $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; + // Calcul partage entre les deux gardes : l'user perdait-il le flag admin ? + $originalData = $this->entityManager->getUnitOfWork()->getOriginalEntityData($data); + $wasAdmin = $originalData['isAdmin'] ?? null; + $willLoseAdmin = true === $wasAdmin && false === $data->isAdmin(); - if (true === $wasAdmin && false === $data->isAdmin()) { - throw new BadRequestHttpException( - 'Vous ne pouvez pas retirer vos propres droits administrateur.' - ); + // Garde auto-suicide : cas particulier plus strict — l'user courant ne + // peut pas retirer son propre flag admin, meme si d'autres admins existent. + if ($willLoseAdmin && $currentUser instanceof User && $currentUser->getId() === $data->getId()) { + throw new BadRequestHttpException( + 'Vous ne pouvez pas retirer vos propres droits administrateur.' + ); + } + + // Garde dernier admin global : invariant general — impossible de retirer + // isAdmin si cela laisserait l'instance sans administrateur. + if ($willLoseAdmin) { + try { + $this->adminHeadcountGuard->ensureAtLeastOneAdminRemainsAfterDemotion($data); + } catch (LastAdminProtectionException $exception) { + throw new BadRequestHttpException($exception->getMessage(), $exception); } } diff --git a/tests/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessorTest.php b/tests/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessorTest.php index e30dbcb..cdf138b 100644 --- a/tests/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessorTest.php +++ b/tests/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessorTest.php @@ -9,6 +9,8 @@ use ApiPlatform\State\ProcessorInterface; use App\Module\Core\Domain\Entity\Permission; use App\Module\Core\Domain\Entity\Role; use App\Module\Core\Domain\Entity\User; +use App\Module\Core\Domain\Exception\LastAdminProtectionException; +use App\Module\Core\Domain\Security\AdminHeadcountGuardInterface; use App\Module\Core\Infrastructure\ApiPlatform\State\Processor\UserRbacProcessor; use Doctrine\ORM\EntityManagerInterface; use Doctrine\ORM\UnitOfWork; @@ -22,9 +24,9 @@ use Symfony\Bundle\SecurityBundle\Security; use Symfony\Component\HttpKernel\Exception\BadRequestHttpException; /** - * Tests unitaires du UserRbacProcessor : couvre la garde "auto-suicide" et la - * delegation au PersistProcessor Doctrine decore pour les trois champs RBAC - * (isAdmin, roles, directPermissions). + * Tests unitaires du UserRbacProcessor : couvre la garde "auto-suicide", la + * garde "dernier admin global" et la delegation au PersistProcessor Doctrine + * decore pour les trois champs RBAC (isAdmin, roles, directPermissions). * * @internal */ @@ -35,14 +37,16 @@ final class UserRbacProcessorTest extends TestCase private EntityManagerInterface&MockObject $entityManager; private MockObject&UnitOfWork $unitOfWork; private MockObject&Security $security; + private AdminHeadcountGuardInterface&MockObject $adminHeadcountGuard; private UserRbacProcessor $processor; protected function setUp(): void { - $this->persistProcessor = $this->createMock(ProcessorInterface::class); - $this->entityManager = $this->createMock(EntityManagerInterface::class); - $this->unitOfWork = $this->createMock(UnitOfWork::class); - $this->security = $this->createMock(Security::class); + $this->persistProcessor = $this->createMock(ProcessorInterface::class); + $this->entityManager = $this->createMock(EntityManagerInterface::class); + $this->unitOfWork = $this->createMock(UnitOfWork::class); + $this->security = $this->createMock(Security::class); + $this->adminHeadcountGuard = $this->createMock(AdminHeadcountGuardInterface::class); $this->entityManager->method('getUnitOfWork')->willReturn($this->unitOfWork); @@ -50,19 +54,28 @@ final class UserRbacProcessorTest extends TestCase $this->persistProcessor, $this->entityManager, $this->security, + $this->adminHeadcountGuard, ); } public function testPatchPromotesUserToAdminDelegatesToPersistProcessor(): void { - $target = $this->buildUser(42, 'alice', false); - $target->setIsAdmin(true); + $target = $this->buildUser(42, 'alice', 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'); + // La cible gagne isAdmin (false -> true) : willLoseAdmin = false, donc + // getOriginalEntityData est appele mais aucune garde ne bloque. + $this->unitOfWork + ->method('getOriginalEntityData') + ->with($target) + ->willReturn([ + 'id' => 42, + 'username' => 'alice', + 'isAdmin' => false, + ]) + ; $this->persistProcessor ->expects(self::once()) @@ -146,14 +159,30 @@ final class UserRbacProcessorTest extends TestCase public function testPatchAdminDemotingAnotherUserIsAllowed(): void { - // Un admin qui retire isAdmin a quelqu'un d'autre : autorise. + // Un admin qui retire isAdmin a quelqu'un d'autre : autorise si d'autres + // admins existent (guard ne leve pas d'exception). $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'); + // La cible perd isAdmin (true -> false) : getOriginalEntityData est appele. + $this->unitOfWork + ->method('getOriginalEntityData') + ->with($target) + ->willReturn([ + 'id' => 42, + 'username' => 'alice', + 'isAdmin' => true, + ]) + ; + + // Le garde ne leve pas d'exception : d'autres admins existent. + $this->adminHeadcountGuard + ->expects(self::once()) + ->method('ensureAtLeastOneAdminRemainsAfterDemotion') + ->with($target) + ; $this->persistProcessor ->expects(self::once()) @@ -210,6 +239,150 @@ final class UserRbacProcessorTest extends TestCase $this->processor->process(new stdClass(), new Patch()); } + // ------------------------------------------------------------------------- + // Tests de la garde "dernier admin global" + // ------------------------------------------------------------------------- + + public function testBlocksDemotionWhenLastAdminGlobally(): void + { + // L'admin courant A tente de retirer isAdmin a l'admin B (le dernier). + $adminA = $this->buildUser(1, 'adminA', true); + $adminB = $this->buildUser(2, 'adminB', false); // isAdmin -> false dans le PATCH + + $this->security->method('getUser')->willReturn($adminA); + + $this->unitOfWork + ->method('getOriginalEntityData') + ->with($adminB) + ->willReturn([ + 'id' => 2, + 'username' => 'adminB', + 'isAdmin' => true, + ]) + ; + + // Le garde signale qu'il n'y aurait plus aucun admin. + $this->adminHeadcountGuard + ->expects(self::once()) + ->method('ensureAtLeastOneAdminRemainsAfterDemotion') + ->with($adminB) + ->willThrowException(new LastAdminProtectionException()) + ; + + $this->persistProcessor->expects(self::never())->method('process'); + + $this->expectException(BadRequestHttpException::class); + $this->expectExceptionMessage('Impossible : au moins un administrateur doit rester sur l\'instance.'); + + $this->processor->process($adminB, new Patch()); + } + + public function testDelegatesDemotionWhenAdminsRemain(): void + { + // L'admin courant A retire isAdmin a l'admin B, mais d'autres admins existent. + $adminA = $this->buildUser(1, 'adminA', true); + $adminB = $this->buildUser(2, 'adminB', false); // isAdmin -> false dans le PATCH + + $this->security->method('getUser')->willReturn($adminA); + + $this->unitOfWork + ->method('getOriginalEntityData') + ->with($adminB) + ->willReturn([ + 'id' => 2, + 'username' => 'adminB', + 'isAdmin' => true, + ]) + ; + + // Le garde ne leve pas d'exception : il reste au moins un admin. + $this->adminHeadcountGuard + ->expects(self::once()) + ->method('ensureAtLeastOneAdminRemainsAfterDemotion') + ->with($adminB) + ; + + $this->persistProcessor + ->expects(self::once()) + ->method('process') + ->with($adminB) + ->willReturn($adminB) + ; + + $result = $this->processor->process($adminB, new Patch()); + + self::assertSame($adminB, $result); + } + + public function testDoesNotCallGuardWhenIsAdminUntouched(): void + { + // PATCH qui ne touche pas isAdmin (reste false) : la garde ne doit pas etre appelee. + $target = $this->buildUser(42, 'alice', false); + $current = $this->buildUser(1, 'admin', true); + + $this->security->method('getUser')->willReturn($current); + + $this->unitOfWork + ->method('getOriginalEntityData') + ->with($target) + ->willReturn([ + 'id' => 42, + 'username' => 'alice', + 'isAdmin' => false, + ]) + ; + + // isAdmin reste false : willLoseAdmin = false, garde jamais appelee. + $this->adminHeadcountGuard + ->expects(self::never()) + ->method('ensureAtLeastOneAdminRemainsAfterDemotion') + ; + + $this->persistProcessor + ->expects(self::once()) + ->method('process') + ->with($target) + ->willReturn($target) + ; + + $result = $this->processor->process($target, new Patch()); + + self::assertSame($target, $result); + } + + public function testAutoSuicideTakesPrecedenceOverLastAdminGlobal(): void + { + // L'unique admin tente de se retirer lui-meme son propre flag. + // La garde auto-suicide doit court-circuiter avant la garde dernier-admin. + $self = $this->buildUser(1, 'admin', false); // isAdmin -> false dans le PATCH + + $this->security->method('getUser')->willReturn($self); + + $this->unitOfWork + ->method('getOriginalEntityData') + ->with($self) + ->willReturn([ + 'id' => 1, + 'username' => 'admin', + 'isAdmin' => true, + ]) + ; + + // La garde dernier-admin ne doit jamais etre appelee : l'auto-suicide + // court-circuite avant. + $this->adminHeadcountGuard + ->expects(self::never()) + ->method('ensureAtLeastOneAdminRemainsAfterDemotion') + ; + + $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()); + } + /** * Construit un User avec un id force via reflection (les mocks * d'UnitOfWork n'alimentent pas l'id tout seul).