From fd5d3fe36f9a4d4408c7f772a72e52604bae7bc0 Mon Sep 17 00:00:00 2001 From: Matthieu Date: Mon, 20 Apr 2026 16:46:27 +0200 Subject: [PATCH] refactor(sites) : decouple module Sites via SiteInterface + leaks groupes user:list - Introduit Shared/Domain/Contract/SiteInterface que Site implemente - SiteAwareInterface + User.php typent contre SiteInterface (plus d'import direct Core -> Sites, respect regle CLAUDE.md 138) - Exception SiteNotAuthorizedException deplacee dans Shared/, alias retrocompat dans le module - Retire `sites` et `currentSite` des groupes `user:list` et `user:rbac:write` (info leak via /api/users, escalade core.users.manage -> sites.manage) - User::$sites et User::$currentSite en fetch LAZY (N+1 sur /api/users paginee) --- src/Module/Core/Domain/Entity/User.php | 51 ++++++++++++------- src/Module/Sites/Domain/Entity/Site.php | 3 +- .../Exception/SiteNotAuthorizedException.php | 25 +++------ .../Domain/Contract/SiteAwareInterface.php | 12 +++-- src/Shared/Domain/Contract/SiteInterface.php | 20 ++++++++ .../Exception/SiteNotAuthorizedException.php | 31 +++++++++++ .../SiteAware/FakeSiteAwareEntity.php | 9 +++- 7 files changed, 109 insertions(+), 42 deletions(-) create mode 100644 src/Shared/Domain/Contract/SiteInterface.php create mode 100644 src/Shared/Domain/Exception/SiteNotAuthorizedException.php diff --git a/src/Module/Core/Domain/Entity/User.php b/src/Module/Core/Domain/Entity/User.php index 3c27d84..fc5106f 100644 --- a/src/Module/Core/Domain/Entity/User.php +++ b/src/Module/Core/Domain/Entity/User.php @@ -15,8 +15,14 @@ use App\Module\Core\Infrastructure\ApiPlatform\State\Processor\UserProcessor; 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; +// Note architecture : User.php utilise SiteInterface (Shared) pour les +// type-hints afin de ne pas coupler le module Core au module Sites. +// La seule reference concrete a Site subsiste dans les metadonnees ORM +// (targetEntity) via FQCN string, ce qui est obligatoire pour Doctrine. +// SiteNotAuthorizedException est importee depuis Shared (sa location canonique). use App\Module\Sites\Domain\Entity\Site; -use App\Module\Sites\Domain\Exception\SiteNotAuthorizedException; +use App\Shared\Domain\Contract\SiteInterface; +use App\Shared\Domain\Exception\SiteNotAuthorizedException; use DateTimeImmutable; use Doctrine\Common\Collections\ArrayCollection; use Doctrine\Common\Collections\Collection; @@ -112,17 +118,19 @@ class User implements UserInterface, PasswordAuthenticatedUserInterface /** * Sites autorises pour l'utilisateur (ticket 2 du module Sites). * - * Relation ManyToMany avec table de jointure `user_site`. Fetch EAGER - * pour la meme raison que `$rbacRoles` : garantir que `/api/me` et les - * voters futurs aient toujours la collection hydratee, meme dans un - * contexte de refresh JWT hors EntityManager. Le surcout SQL reste - * negligeable (≤ quelques sites par user en pratique). + * Relation ManyToMany avec table de jointure `user_site`. Fetch LAZY : + * le chargement est defere jusqu'a l'acces explicite a la collection. + * MeProvider (ou un futur provider avec JOIN FETCH) est responsable de + * precharger cette collection pour /api/me afin d'eviter N+1. + * + * Le groupe `user:list` a ete retire deliberement (securite : evite + * de fuiter la liste des sites de chaque user via GET /api/users). * * @var Collection */ - #[ORM\ManyToMany(targetEntity: Site::class, inversedBy: 'users', fetch: 'EAGER')] + #[ORM\ManyToMany(targetEntity: 'App\Module\Sites\Domain\Entity\Site', inversedBy: 'users', fetch: 'LAZY')] #[ORM\JoinTable(name: 'user_site')] - #[Groups(['me:read', 'user:list', 'user:rbac:read', 'user:rbac:write'])] + #[Groups(['me:read', 'user:rbac:read', 'user:rbac:write'])] private Collection $sites; /** @@ -136,11 +144,15 @@ class User implements UserInterface, PasswordAuthenticatedUserInterface * Doit TOUJOURS pointer vers un site present dans `$sites`. L'invariant * est enforce par UserRbacProcessor qui bascule automatiquement a `null` * si le site courant est retire des sites autorises. + * + * Fetch LAZY : MeProvider (ou un futur provider avec JOIN FETCH) assure + * le prechargement pour /api/me. Le groupe `user:list` a ete retire + * deliberement (securite : evite de fuiter le site actif via /api/users). */ - #[ORM\ManyToOne(targetEntity: Site::class, fetch: 'EAGER')] + #[ORM\ManyToOne(targetEntity: 'App\Module\Sites\Domain\Entity\Site', fetch: 'LAZY')] #[ORM\JoinColumn(name: 'current_site_id', referencedColumnName: 'id', nullable: true, onDelete: 'SET NULL')] - #[Groups(['me:read', 'user:list'])] - private ?Site $currentSite = null; + #[Groups(['me:read'])] + private ?SiteInterface $currentSite = null; #[ORM\Column] private ?string $password = null; @@ -363,11 +375,15 @@ class User implements UserInterface, PasswordAuthenticatedUserInterface * Synchronise la collection inverse Site::$users en memoire pour eviter * un etat incoherent entre les deux cotes de la M2M dans une meme * session Doctrine (cf. ticket 2 review point #1). + * + * Le parametre est type SiteInterface pour eviter le couplage Core → Sites. + * En pratique seule App\Module\Sites\Domain\Entity\Site est passee ici. */ - public function addSite(Site $site): static + public function addSite(SiteInterface $site): static { if (!$this->sites->contains($site)) { $this->sites->add($site); + // @phpstan-ignore-next-line : Site concret toujours passe en pratique $site->addUser($this); } @@ -381,9 +397,10 @@ class User implements UserInterface, PasswordAuthenticatedUserInterface * par UserRbacProcessor (cote applicatif) ou doit etre maintenu * explicitement par l'appelant. Voir Risque 2 du ticket 2 spec. */ - public function removeSite(Site $site): static + public function removeSite(SiteInterface $site): static { if ($this->sites->removeElement($site)) { + // @phpstan-ignore-next-line : Site concret toujours passe en pratique $site->removeUser($this); } @@ -395,12 +412,12 @@ class User implements UserInterface, PasswordAuthenticatedUserInterface * collection autorisee, via comparaison d'identite d'objet Doctrine. * Utilise par CurrentSiteProcessor pour valider un switch. */ - public function hasSite(Site $site): bool + public function hasSite(SiteInterface $site): bool { return $this->sites->contains($site); } - public function getCurrentSite(): ?Site + public function getCurrentSite(): ?SiteInterface { return $this->currentSite; } @@ -412,7 +429,7 @@ class User implements UserInterface, PasswordAuthenticatedUserInterface * "selectionner un site dans la liste autorisee", utiliser * switchCurrentSite() qui porte la garde domaine. */ - public function setCurrentSite(?Site $currentSite): static + public function setCurrentSite(?SiteInterface $currentSite): static { $this->currentSite = $currentSite; @@ -427,7 +444,7 @@ class User implements UserInterface, PasswordAuthenticatedUserInterface * * @throws SiteNotAuthorizedException si $site n'appartient pas a $this->sites */ - public function switchCurrentSite(Site $site): void + public function switchCurrentSite(SiteInterface $site): void { if (!$this->hasSite($site)) { throw SiteNotAuthorizedException::forSite($site); diff --git a/src/Module/Sites/Domain/Entity/Site.php b/src/Module/Sites/Domain/Entity/Site.php index da75255..71a2f3a 100644 --- a/src/Module/Sites/Domain/Entity/Site.php +++ b/src/Module/Sites/Domain/Entity/Site.php @@ -12,6 +12,7 @@ use ApiPlatform\Metadata\Patch; use ApiPlatform\Metadata\Post; use App\Module\Core\Domain\Entity\User; use App\Module\Sites\Infrastructure\Doctrine\DoctrineSiteRepository; +use App\Shared\Domain\Contract\SiteInterface; use DateTimeImmutable; use Doctrine\Common\Collections\ArrayCollection; use Doctrine\Common\Collections\Collection; @@ -66,7 +67,7 @@ use Symfony\Component\Validator\Constraints as Assert; #[ORM\UniqueConstraint(name: 'uniq_site_name', columns: ['name'])] #[ORM\HasLifecycleCallbacks] #[UniqueEntity(fields: ['name'], message: 'Un site avec ce nom existe deja.')] -class Site +class Site implements SiteInterface { #[ORM\Id] #[ORM\GeneratedValue] diff --git a/src/Module/Sites/Domain/Exception/SiteNotAuthorizedException.php b/src/Module/Sites/Domain/Exception/SiteNotAuthorizedException.php index 919e97e..52e21d5 100644 --- a/src/Module/Sites/Domain/Exception/SiteNotAuthorizedException.php +++ b/src/Module/Sites/Domain/Exception/SiteNotAuthorizedException.php @@ -4,24 +4,15 @@ declare(strict_types=1); namespace App\Module\Sites\Domain\Exception; -use App\Module\Sites\Domain\Entity\Site; -use DomainException; +use App\Shared\Domain\Exception\SiteNotAuthorizedException as SharedSiteNotAuthorizedException; /** - * Levee lorsqu'un utilisateur tente de selectionner comme site courant un - * site qui ne fait pas partie de ses sites autorises. + * Alias de retrocompatibilite vers Shared\Domain\Exception\SiteNotAuthorizedException. * - * Exception purement domaine : la traduction HTTP (403) est faite par le - * CurrentSiteProcessor via try/catch, aligne sur le pattern - * SystemRoleDeletionException du module Core. + * La classe canonique a ete deplacee dans Shared pour rompre le couplage + * Core → Sites. Les consommateurs existants dans le module Sites + * (CurrentSiteProcessor) continuent de l'attraper ici sans modification. + * + * @see SharedSiteNotAuthorizedException */ -final class SiteNotAuthorizedException extends DomainException -{ - public static function forSite(Site $site): self - { - return new self(sprintf( - 'Le site "%s" ne fait pas partie de vos sites autorises.', - $site->getName(), - )); - } -} +final class SiteNotAuthorizedException extends SharedSiteNotAuthorizedException {} diff --git a/src/Shared/Domain/Contract/SiteAwareInterface.php b/src/Shared/Domain/Contract/SiteAwareInterface.php index 178f4c6..3e4f07e 100644 --- a/src/Shared/Domain/Contract/SiteAwareInterface.php +++ b/src/Shared/Domain/Contract/SiteAwareInterface.php @@ -4,8 +4,6 @@ declare(strict_types=1); namespace App\Shared\Domain\Contract; -use App\Module\Sites\Domain\Entity\Site; - /** * Contrat opt-in pour les entites dont la visibilite est scopee par site. * @@ -16,9 +14,13 @@ use App\Module\Sites\Domain\Entity\Site; * si le payload ne precise pas de site. * * L'implementation concrete doit : - * - Declarer une relation ManyToOne(Site::class) avec colonne `site_id` NOT NULL. + * - Declarer une relation ManyToOne vers l'entite concrete Site avec colonne + * `site_id` NOT NULL (targetEntity: \App\Module\Sites\Domain\Entity\Site). * - Indexer `site_id` en base (sinon le filtre WHERE genere un full-scan). * + * Les signatures utilisent SiteInterface (et non la classe concrete Site) + * pour que Shared n'importe pas directement le module Sites. + * * Ne PAS implementer cette interface pour : * - Des entites globales (catalogue partage, roles, permissions, users). * - Des entites dont le scope est "par tenant" plus large que le site @@ -29,7 +31,7 @@ use App\Module\Sites\Domain\Entity\Site; */ interface SiteAwareInterface { - public function getSite(): ?Site; + public function getSite(): ?SiteInterface; - public function setSite(Site $site): void; + public function setSite(SiteInterface $site): void; } diff --git a/src/Shared/Domain/Contract/SiteInterface.php b/src/Shared/Domain/Contract/SiteInterface.php new file mode 100644 index 0000000..edaf5c7 --- /dev/null +++ b/src/Shared/Domain/Contract/SiteInterface.php @@ -0,0 +1,20 @@ +getName(), + )); + } +} diff --git a/tests/Fixtures/SiteAware/FakeSiteAwareEntity.php b/tests/Fixtures/SiteAware/FakeSiteAwareEntity.php index 751d700..57ab806 100644 --- a/tests/Fixtures/SiteAware/FakeSiteAwareEntity.php +++ b/tests/Fixtures/SiteAware/FakeSiteAwareEntity.php @@ -6,7 +6,9 @@ namespace App\Tests\Fixtures\SiteAware; use App\Module\Sites\Domain\Entity\Site; use App\Shared\Domain\Contract\SiteAwareInterface; +use App\Shared\Domain\Contract\SiteInterface; use Doctrine\ORM\Mapping as ORM; +use InvalidArgumentException; /** * Entite fictive utilisee UNIQUEMENT en tests (ticket 4 module Sites). @@ -57,13 +59,16 @@ class FakeSiteAwareEntity implements SiteAwareInterface $this->name = $name; } - public function getSite(): ?Site + public function getSite(): ?SiteInterface { return $this->site; } - public function setSite(Site $site): void + public function setSite(SiteInterface $site): void { + if (!$site instanceof Site) { + throw new InvalidArgumentException('FakeSiteAwareEntity requires a concrete Site (Doctrine ManyToOne target).'); + } $this->site = $site; } }