feat(core) : RBAC #345 - UserRbacProcessor last admin guard

2026-04-15 16:00:34 +02:00
parent ba5eb804f2
commit 80b63cd7d7
2 changed files with 214 additions and 33 deletions
--- 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<User, User>
 */
@@ -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);
            }
        }

--- 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).