From caae75213001b0bcacc5085230263a94843555e6 Mon Sep 17 00:00:00 2001 From: Matthieu Date: Mon, 20 Apr 2026 16:47:28 +0200 Subject: [PATCH] fix(sites) : processors transactionnels + garde sites.manage + anti-TOCTOU MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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. --- .../State/Processor/UserRbacProcessor.php | 68 ++++++++++++++++--- .../State/Processor/CurrentSiteProcessor.php | 45 +++++++++--- .../Processor/SiteAwareInjectionProcessor.php | 50 +++++++++++--- .../State/Processor/UserRbacProcessorTest.php | 8 +++ .../SiteAwareInjectionProcessorTest.php | 17 +++-- 5 files changed, 156 insertions(+), 32 deletions(-) diff --git a/src/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessor.php b/src/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessor.php index 7800cf3..5b0cd15 100644 --- 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 */ @@ -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 { diff --git a/src/Module/Sites/Infrastructure/ApiPlatform/State/Processor/CurrentSiteProcessor.php b/src/Module/Sites/Infrastructure/ApiPlatform/State/Processor/CurrentSiteProcessor.php index 5e2579a..74ff3e2 100644 --- 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 */ 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; } diff --git a/src/Module/Sites/Infrastructure/ApiPlatform/State/Processor/SiteAwareInjectionProcessor.php b/src/Module/Sites/Infrastructure/ApiPlatform/State/Processor/SiteAwareInjectionProcessor.php index 8d31125..b78463b 100644 --- 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); diff --git a/tests/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessorTest.php b/tests/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessorTest.php index cdf138b..ad664ac 100644 --- 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, diff --git a/tests/Module/Sites/Infrastructure/ApiPlatform/State/Processor/SiteAwareInjectionProcessorTest.php b/tests/Module/Sites/Infrastructure/ApiPlatform/State/Processor/SiteAwareInjectionProcessorTest.php index 65c1e54..c2c9c87 100644 --- 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; }