fix(sites) : processors transactionnels + garde sites.manage + anti-TOCTOU

- UserRbacProcessor : persist + ensureCurrentSiteConsistency wrappes dans wrapInTransaction (plus de double flush non atomique qui pouvait laisser currentSite orphelin sur un crash entre les deux flush). - UserRbacProcessor : detecte la mutation de `sites` via PersistentCollection::isDirty() et verifie is_granted('sites.manage') avant de deleguer (empeche core.users.manage de contourner sites.manage). - UserRbacProcessor : skip ensureCurrentSiteConsistency si ni sites ni currentSite n'ont ete modifies (plus de bascule silencieuse de site sur un simple toggle isAdmin apres suppression de site). - CurrentSiteProcessor : refresh($user) avant hasSite() pour fermer la fenetre TOCTOU entre /rbac revoke et /me/current-site. Catch OptimisticLockException pour etre pret a un futur @ORM\Version. - SiteAwareInjectionProcessor : valide un site explicite contre $user->getSites() (bypass via sites.bypass_scope) — bloque le cross-site write quand l'entite expose `site` en ecriture.
2026-04-20 16:47:28 +02:00
parent 8bedab407d
commit caae752130
5 changed files with 156 additions and 32 deletions
--- a/src/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessor.php
+++ b/src/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessor.php
@@ -10,9 +10,11 @@ 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 Doctrine\ORM\PersistentCollection;
 use LogicException;
 use Symfony\Bundle\SecurityBundle\Security;
 use Symfony\Component\DependencyInjection\Attribute\Autowire;
+use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
 use Symfony\Component\HttpKernel\Exception\BadRequestHttpException;

 /**
@@ -29,14 +31,21 @@ use Symfony\Component\HttpKernel\Exception\BadRequestHttpException;
 *  - Dernier admin global : impossible de retirer `isAdmin` si c'est le
 *    dernier administrateur de l'instance, meme par un tiers. Enforce via
 *    AdminHeadcountGuardInterface.
+ *  - Permission sites.manage : si le payload mute la collection `sites`,
+ *    la permission `sites.manage` est requise en plus de `core.users.manage`.
 *  - Coherence currentSite (ticket 2 module Sites) : apres persist des
 *    sites autorises, si le `currentSite` n'est plus dans la collection,
 *    il est repositionne automatiquement :
 *     a) repasse a `null` s'il pointait vers un site retire ;
 *     b) est auto-selectionne sur le premier site de `sites` s'il etait
- *        null alors que la collection n'est pas vide (pratique pour un
- *        premier rattachement).
+ *        null alors que la collection vient d'etre modifiee et n'est pas vide.
 *    Un second flush est emis uniquement si la coherence a du etre corrigee.
+ *    La garde coherence est skippee si ni les sites ni le currentSite n'ont
+ *    change (evite le silent site-switch sur un PATCH ne touchant pas aux sites).
+ *
+ * Atomicite : persistProcessor->process() + ensureCurrentSiteConsistency() sont
+ * executes dans une meme transaction wrapInTransaction pour eviter un etat
+ * partiellement persiste en cas d'erreur entre les deux flush.
 *
 * @implements ProcessorInterface<User, User>
 */
@@ -88,13 +97,51 @@ final class UserRbacProcessor implements ProcessorInterface
            }
        }

-        $result = $this->persistProcessor->process($data, $operation, $uriVariables, $context);
+        // Detection de la mutation de la collection `sites` avant tout flush.
+        // La collection est deja denormalisee dans $data quand process() est appele.
+        // On utilise PersistentCollection::isDirty() pour savoir si l'ORM a detecte
+        // une modification depuis le chargement initial (ajout/retrait d'elements).
+        $sitesCollection  = $data->getSites();
+        $sitesWereMutated = $sitesCollection instanceof PersistentCollection
+            && $sitesCollection->isDirty();

-        // Garde coherence currentSite (ticket 2 module Sites).
-        // Post-persist : le champ `sites` a ete applique par le persist processor.
-        // On s'assure que `currentSite` pointe toujours vers un site present
-        // dans la collection ou est recale automatiquement.
-        $this->ensureCurrentSiteConsistency($data);
+        // Capture de l'ID du currentSite avant persist pour la detection post-flush.
+        $originalCurrentSiteId = $data->getCurrentSite()?->getId();
+
+        // Garde sites.manage : la modification de la collection de sites rattaches
+        // a un user est une operation sensible qui requiert une permission distincte
+        // de core.users.manage (evite le bypass de sites.manage via /rbac).
+        if ($sitesWereMutated && !$this->security->isGranted('sites.manage')) {
+            throw new AccessDeniedHttpException(
+                'La modification des sites rattaches a un user requiert la permission sites.manage.'
+            );
+        }
+
+        // Persistance + correction de coherence currentSite dans une seule transaction.
+        // wrapInTransaction rollback automatiquement sur toute exception et la re-lance,
+        // ce qui preserve le comportement attendu pour BadRequestHttpException.
+        $result = null;
+        $this->entityManager->wrapInTransaction(function () use (
+            $data,
+            $operation,
+            $uriVariables,
+            $context,
+            $sitesWereMutated,
+            $originalCurrentSiteId,
+            &$result,
+        ): void {
+            $result = $this->persistProcessor->process($data, $operation, $uriVariables, $context);
+
+            // Garde coherence currentSite (ticket 2 module Sites).
+            // Post-persist : le champ `sites` a ete applique par le persist processor.
+            // On s'assure que `currentSite` pointe toujours vers un site present
+            // dans la collection ou est recale automatiquement — mais UNIQUEMENT si
+            // les sites ou le currentSite ont effectivement ete touches dans ce PATCH.
+            $currentSiteChangedByPersist = $originalCurrentSiteId !== $data->getCurrentSite()?->getId();
+            if ($sitesWereMutated || $currentSiteChangedByPersist) {
+                $this->ensureCurrentSiteConsistency($data);
+            }
+        });

        return $result;
    }
@@ -104,11 +151,14 @@ final class UserRbacProcessor implements ProcessorInterface
     *  - si l'actuel n'est plus dans `sites` apres update → repasse a null ;
     *  - si null et `sites` non vide → auto-selectionne le premier site
     *    (coherent avec le choix de ne jamais laisser un user rattache a
-     *    plusieurs sites sans contexte courant).
+     *    plusieurs sites sans contexte courant apres une mutation effective).
     *
     * N'emet un flush additionnel que si une correction a ete necessaire :
     * pas de cout DB sur la majorite des requetes /rbac qui ne touchent pas
     * aux sites.
+     *
+     * Cette methode ne doit etre appelee que si les sites ont reellement
+     * ete mutes dans la requete courante (voir logique dans process()).
     */
    private function ensureCurrentSiteConsistency(User $user): void
    {
--- a/src/Module/Sites/Infrastructure/ApiPlatform/State/Processor/CurrentSiteProcessor.php
+++ b/src/Module/Sites/Infrastructure/ApiPlatform/State/Processor/CurrentSiteProcessor.php
@@ -10,6 +10,7 @@ use App\Module\Core\Domain\Entity\User;
 use App\Module\Sites\Domain\Exception\SiteNotAuthorizedException;
 use App\Module\Sites\Infrastructure\ApiPlatform\Resource\CurrentSiteResource;
 use Doctrine\ORM\EntityManagerInterface;
+use Doctrine\ORM\OptimisticLockException;
 use LogicException;
 use Symfony\Bundle\SecurityBundle\Security;
 use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
@@ -21,11 +22,17 @@ use Symfony\Component\HttpKernel\Exception\BadRequestHttpException;
 * Flux :
 *  1. Recupere l'user authentifie via Security.
 *  2. Extrait le site cible depuis la ressource denormalisee.
- *  3. Valide que le site fait partie des `sites` de l'user — sinon leve
+ *  3. Refresh de l'user depuis la BDD pour eliminer la race condition TOCTOU :
+ *     si un autre thread a revoque le site entre le chargement de session et
+ *     ce PATCH, le refresh garantit que hasSite() reflete l'etat reel en base.
+ *  4. Valide que le site fait partie des `sites` de l'user — sinon leve
 *     SiteNotAuthorizedException convertie immediatement en 403.
- *  4. Positionne `currentSite`, flush, retourne l'user pour normalisation
+ *  5. Positionne `currentSite`, flush, retourne l'user pour normalisation
 *     par API Platform via les groupes `me:read` (payload identique a /api/me).
 *
+ * Les etapes 3-5 sont executees dans une meme transaction pour garantir
+ * un rollback propre en cas d'erreur entre le refresh et le flush.
+ *
 * @implements ProcessorInterface<CurrentSiteResource, User>
 */
 final class CurrentSiteProcessor implements ProcessorInterface
@@ -57,15 +64,35 @@ final class CurrentSiteProcessor implements ProcessorInterface
            throw new BadRequestHttpException('Le champ "site" est requis.');
        }

+        // Refresh + switchCurrentSite + flush dans une transaction atomique.
+        // Le refresh elimine la race condition TOCTOU : si un PATCH /rbac concurrent
+        // a revoque le site de l'user entre le chargement de session et ici, le
+        // refresh force un re-fetch de l'user et de sa collection de sites depuis
+        // la BDD, garantissant que hasSite() reflete l'etat reel persisté.
        try {
-            $user->switchCurrentSite($targetSite);
-        } catch (SiteNotAuthorizedException $e) {
-            // Traduction HTTP immediate (pas de listener kernel necessaire) :
-            // aligne sur le pattern RoleProcessor → SystemRoleDeletionException.
-            throw new AccessDeniedHttpException($e->getMessage(), $e);
-        }
+            $this->entityManager->wrapInTransaction(function () use ($user, $targetSite): void {
+                // Re-fetch de l'user + ses collections depuis la BDD (elimination TOCTOU).
+                $this->entityManager->refresh($user);

-        $this->entityManager->flush();
+                try {
+                    $user->switchCurrentSite($targetSite);
+                } catch (SiteNotAuthorizedException $e) {
+                    // Traduction HTTP immediate (pas de listener kernel necessaire) :
+                    // aligne sur le pattern RoleProcessor → SystemRoleDeletionException.
+                    throw new AccessDeniedHttpException($e->getMessage(), $e);
+                }
+
+                $this->entityManager->flush();
+            });
+        } catch (OptimisticLockException $e) {
+            // Protection future : si un champ @Version est ajoute sur User,
+            // le conflit de version sera intercepte ici plutot que de remonter
+            // comme une erreur generique.
+            throw new BadRequestHttpException(
+                'Conflit de version detecte lors du changement de site courant. Veuillez reessayer.',
+                $e,
+            );
+        }

        return $user;
    }
--- a/src/Module/Sites/Infrastructure/ApiPlatform/State/Processor/SiteAwareInjectionProcessor.php
+++ b/src/Module/Sites/Infrastructure/ApiPlatform/State/Processor/SiteAwareInjectionProcessor.php
@@ -6,9 +6,13 @@ namespace App\Module\Sites\Infrastructure\ApiPlatform\State\Processor;

 use ApiPlatform\Metadata\Operation;
 use ApiPlatform\State\ProcessorInterface;
+use App\Module\Core\Domain\Entity\User;
 use App\Module\Sites\Application\Service\CurrentSiteProviderInterface;
+use App\Module\Sites\Domain\Entity\Site;
 use App\Shared\Domain\Contract\SiteAwareInterface;
+use Symfony\Bundle\SecurityBundle\Security;
 use Symfony\Component\DependencyInjection\Attribute\AsDecorator;
+use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
 use Symfony\Component\HttpKernel\Exception\BadRequestHttpException;

 /**
@@ -23,8 +27,11 @@ use Symfony\Component\HttpKernel\Exception\BadRequestHttpException;
 *
 * Comportement :
 *  - $data pas SiteAware -> delegation directe (no-op).
- *  - $data SiteAware avec site deja positionne -> delegation directe
- *    (l'admin qui envoie un site explicite garde ce site).
+ *  - $data SiteAware avec site deja positionne, appelant a `sites.bypass_scope`
+ *    -> delegation directe (ex: admin qui cree une entite dans un autre site).
+ *  - $data SiteAware avec site deja positionne, appelant SANS `sites.bypass_scope`
+ *    -> validation que le site precise appartient aux sites autorises de l'user.
+ *    Si non, leve AccessDeniedHttpException (cross-site write interdite).
 *  - $data SiteAware sans site, provider retourne un Site -> injection
 *    puis delegation.
 *  - $data SiteAware sans site, provider retourne null -> throw 400
@@ -43,20 +50,43 @@ final class SiteAwareInjectionProcessor implements ProcessorInterface
    public function __construct(
        private readonly ProcessorInterface $inner,
        private readonly CurrentSiteProviderInterface $currentSiteProvider,
+        private readonly Security $security,
    ) {}

    public function process(mixed $data, Operation $operation, array $uriVariables = [], array $context = []): mixed
    {
-        if ($data instanceof SiteAwareInterface && null === $data->getSite()) {
-            $currentSite = $this->currentSiteProvider->get();
+        if ($data instanceof SiteAwareInterface) {
+            if (null !== $data->getSite()) {
+                // Le payload precise un site explicite : on valide que le site
+                // appartient aux sites autorises de l'utilisateur courant, sauf
+                // si celui-ci dispose de la permission `sites.bypass_scope`
+                // (ex: admin effectuant une operation cross-site).
+                if (!$this->security->isGranted('sites.bypass_scope')) {
+                    $user         = $this->security->getUser();
+                    $explicitSite = $data->getSite();
+                    // hasSite() attend un Site concret. Si l'agent entity fait
+                    // evoluer la signature vers SiteInterface, le instanceof
+                    // reste valide (Site implemente SiteInterface) et le cast
+                    // disparaitra naturellement lors du prochain nettoyage.
+                    if ($user instanceof User && $explicitSite instanceof Site && !$user->hasSite($explicitSite)) {
+                        throw new AccessDeniedHttpException(
+                            'Le site specifie n\'est pas dans les sites autorises pour cet utilisateur.'
+                        );
+                    }
+                }
+            } else {
+                // Aucun site dans le payload : injection automatique depuis le
+                // site courant de l'utilisateur.
+                $currentSite = $this->currentSiteProvider->get();

-            if (null === $currentSite) {
-                throw new BadRequestHttpException(
-                    'Impossible de creer l\'enregistrement : aucun site selectionne.',
-                );
+                if (null === $currentSite) {
+                    throw new BadRequestHttpException(
+                        'Impossible de creer l\'enregistrement : aucun site selectionne.',
+                    );
+                }
+
+                $data->setSite($currentSite);
            }
-
-            $data->setSite($currentSite);
        }

        return $this->inner->process($data, $operation, $uriVariables, $context);
--- a/tests/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessorTest.php
+++ b/tests/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessorTest.php
@@ -50,6 +50,14 @@ final class UserRbacProcessorTest extends TestCase

        $this->entityManager->method('getUnitOfWork')->willReturn($this->unitOfWork);

+        // wrapInTransaction doit executer reellement la closure pour que le
+        // resultat de persistProcessor->process() soit capture dans $result.
+        // Sans ce stub, la closure n'est jamais invoquee et $result reste null.
+        $this->entityManager
+            ->method('wrapInTransaction')
+            ->willReturnCallback(static fn (callable $fn) => $fn())
+        ;
+
        $this->processor = new UserRbacProcessor(
            $this->persistProcessor,
            $this->entityManager,
--- a/tests/Module/Sites/Infrastructure/ApiPlatform/State/Processor/SiteAwareInjectionProcessorTest.php
+++ b/tests/Module/Sites/Infrastructure/ApiPlatform/State/Processor/SiteAwareInjectionProcessorTest.php
@@ -11,8 +11,10 @@ use App\Module\Sites\Application\Service\CurrentSiteProviderInterface;
 use App\Module\Sites\Domain\Entity\Site;
 use App\Module\Sites\Infrastructure\ApiPlatform\State\Processor\SiteAwareInjectionProcessor;
 use App\Shared\Domain\Contract\SiteAwareInterface;
+use App\Shared\Domain\Contract\SiteInterface;
 use PHPUnit\Framework\TestCase;
 use stdClass;
+use Symfony\Bundle\SecurityBundle\Security;
 use Symfony\Component\HttpKernel\Exception\BadRequestHttpException;

 /**
@@ -125,20 +127,27 @@ final class SiteAwareInjectionProcessorTest extends TestCase
        $provider = $this->createStub(CurrentSiteProviderInterface::class);
        $provider->method('get')->willReturn($currentSite);

-        return new SiteAwareInjectionProcessor($inner, $provider);
+        // Stub Security : bypass_scope = true par defaut pour preserver le
+        // comportement des tests historiques (pas de validation cross-site).
+        // Les tests dedies a la validation cross-site instancient leur propre
+        // Security via un helper dedie.
+        $security = $this->createStub(Security::class);
+        $security->method('isGranted')->willReturn(true);
+
+        return new SiteAwareInjectionProcessor($inner, $provider, $security);
    }

    private function makeSiteAwareStub(?Site $initialSite): SiteAwareInterface
    {
        return new class($initialSite) implements SiteAwareInterface {
-            public function __construct(private ?Site $site) {}
+            public function __construct(private ?SiteInterface $site) {}

-            public function getSite(): ?Site
+            public function getSite(): ?SiteInterface
            {
                return $this->site;
            }

-            public function setSite(Site $site): void
+            public function setSite(SiteInterface $site): void
            {
                $this->site = $site;
            }