Module sites #8
Reference in New Issue
Block a user
Delete Branch "feat/module-site-backend"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Description de la PR
Modification du .env
Check list
Code review
Found 3 issues:
Core→Sitesdans une entité de domaine (CLAUDE.md : « Communication inter-modules parShared/Domain/Contract/ou domain events — jamais d'import direct entre modules »)Shared/Domain/Contract/qui dépend directement de l'entité concrèteSites/Domain/Entity/Site— rend l'interface « partagée » tributaire du module Sites (CLAUDE.md : même règle, etShared/Domain/Contract/est décrit comme contenant des « Interfaces inter-modules » indépendantes)navigateTo('/login')placé hors dutry/finally: siauth.logout()rejette, lefinallyexécute bien les troisreset*(), puis l'exception se propage hors deonMountedavant la ligne 27 → l'utilisateur reste bloqué sur/logoutavec l'état déjà effacé. Le redirect devrait être à l'intérieur dufinally(ou après uncatch).🤖 Generated with Claude Code
- If this code review was useful, please react with 👍. Otherwise, react with 👎.
@@ -16,1 +24,4 @@resetModules()resetCurrentSite()}await navigateTo('/login')Si
auth.logout()(ligne 17) rejette, lefinallyexécute bien les trois resets mais l'exception se propage ensuite hors du handleronMounted— la ligneawait navigateTo('/login')ne s'exécute jamais. L'utilisateur reste bloqué sur/logoutavec état vidé. DéplacernavigateTo('/login')dans lefinally(après les resets), ou entourer d'untry { await auth.logout() } catch {} finally { ...; await navigateTo('/login') }.@@ -15,6 +15,8 @@ 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;use App\Module\Sites\Domain\Entity\Site;Import direct
Core→Sites(entité + exception) : viole la règle CLAUDE.md « jamais d'import direct entre modules » (ligne 138). Piste : introduire une interfaceSiteInterfacedansShared/Domain/Contract/et typerUser::$currentSite/hasSite()/switchCurrentSite()contre cette interface. L'exceptionSiteNotAuthorizedExceptionpeut aussi être remontée dansShared/Domain/Exception/.@@ -0,0 +4,4 @@namespace App\Shared\Domain\Contract;use App\Module\Sites\Domain\Entity\Site;Shared/Domain/Contract/ne doit dépendre de aucun module (cf. CLAUDE.md:138 et description deContract/ligne 23). En typantgetSite(): ?Site/setSite(Site)contre l'entité concrète du moduleSites, tout module qui adopteSiteAwareInterfacehérite d'une dépendance transitive versModule\Sites— ce qui défait la raison d'être deShared/Contract/. À remplacer par unSiteInterface(dansShared/Domain/Contract/) queModule\Sites\Domain\Entity\Siteimplémente.Code review — compléments (analyse Codex)
En repassant avec un modèle indépendant, trois findings supplémentaires high-confidence sur le cloisonnement RBAC sites ↔ users :
Le champ
sitesest serialisé dans le groupeuser:list(exposé viaGET /api/usersgardé parcore.users.view). Conséquence : tout porteur decore.users.viewlit les IRIs des sites de tous les users, même sanssites.view— alors queSitelui-même est gardé parsites.view. Info leak contournant la permission dédiée.Le champ
sitesest aussi writable dans le groupeuser:rbac:writesurPATCH /api/users/{id}/rbacgardé uniquement parcore.users.manage. Un user-manager sanssites.managepeut s'ajouter/retirer à n'importe quel site (ou l'ajouter à un autre user), puis/api/me/current-sitepour basculer dedans. Escalade de privilèges contournantsites.manage.UserRbacProcessor::ensureCurrentSiteConsistency()auto-sélectionne$sites->first()dès quecurrentSiteest null avec des sites disponibles, y compris sur unPATCH /rbacqui n'a pas touchésites. Après une suppression de site (FKonDelete: SET NULLligne 141), une simple édition de rôle bascule silencieusement l'user dans un site arbitraire — change les lectures/écritures scopées ensuite sans action explicite.Bonus (fragilité) :
SiteAwareInjectionProcessorn'injecte que sisiteest null mais ne valide jamais unsiteexplicite envoyé dans le payload. Si une future entité SiteAware exposesiteen écriture à des non-admins, elle devient cross-site writable sans garde. Le docblock dit « l'admin qui envoie un site explicite garde ce site » — à durcir ou à documenter comme contrat strict à la charge de chaque entité.🤖 Generated with Claude Code + Codex
@@ -110,0 +122,4 @@*/#[ORM\ManyToMany(targetEntity: Site::class, inversedBy: 'users', fetch: 'EAGER')]#[ORM\JoinTable(name: 'user_site')]#[Groups(['me:read', 'user:list', 'user:rbac:read', 'user:rbac:write'])]Les groupes de sérialisation de
sitesexposent la collection au-delà du scopesites.*:user:list→GET /api/users(gardé parcore.users.view) renvoie les IRIs des sites de chaque user. Quiconque peut lister les users apprend les appartenances, même sanssites.view.user:rbac:write→PATCH /api/users/{id}/rbac(gardé parcore.users.manage) permet d'écriresitessanssites.manage. Un user-manager peut ainsi s'ajouter/retirer à un site arbitraire puis basculer via/api/me/current-site.Pistes : retirer
sitesdeuser:list(ou introduire un groupeuser:list:fullgardé parsites.view), et dansUserRbacProcessorrefuser la mutation desitessi l'appelant n'a passites.manage.@@ -84,0 +121,4 @@$changed = true;}if (null === $user->getCurrentSite() && !$sites->isEmpty()) {Le bloc
if (null === currentSite && !sites->isEmpty())s'applique sur tout PATCH/rbac, même si la requête n'a pas touchésites. Scénario : un admin supprime un site → la FK passe à NULL pour les users concernés (onDelete: SET NULL). Plus tard, un manager édite seulement un rôle sur un de ces users → cet appel bascule silencieusementcurrentSitesursites->first(), changeant le contexte effectif des lectures/écritures scopées.Garder la garde (a) —
currentSiteretiré → null — mais conditionner (b) au fait quesitesa réellement été modifié par la requête courante (ou ne l'appliquer que quandcurrentSiteétait non-null avant, ou seulement surPOSTutilisateur).Code review — seconde passe profonde (5 Opus + Codex high-effort)
Après re-review avec 5 agents Opus sur des angles non couverts au premier tour + 2 passes Codex indépendantes, voici les findings vérifiés :
Critiques (data integrity / authorization)
1.
UserRbacProcessor: double flush non atomique —persistProcessor->process()flushe (L91), puisensureCurrentSiteConsistency()peut re-flusher (L130). AucunwrapInTransactionautour. Si un crash survient entre les deux (OOM, worker kill, perte DB), l'invariant documenté L34-39 «currentSite∈sites» est violé en base.2.
/api/me/current-site: TOCTOU entrehasSite()etflush()— pas de#[ORM\Version], pas de verrou. Alice PATCHcurrent-site: S2pendant qu'un admin PATCH/users/alice/rbacretire S2 de ses sites : le check in-memory d'Alice passe, puis son flush écrase leSET NULLposé par l'admin → Alice termine aveccurrentSite=S2sans ligneuser_site.3.
/api/usersn'est pas site-scopé —Usern'implémente pasSiteAwareInterface, donc tout porteur decore.users.view(permission délégable) énumère les users de tous les tenants, avecisAdmin,rbacRoles,directPermissions,sites,currentSite. Dans un déploiement multi-site où un « responsable de site » acore.users.viewpour gérer son équipe, il lit l'organigramme complet de l'instance.4. Frontend : après suppression d'un site ou switch,
auth.usern'est jamais rafraîchi — deux bugs liés côté frontend :sites.vueadmin delete (L141-155) appelleloadSites()pour rafraîchir la liste mais ne rafraîchit pasauth.user. Si l'admin supprime son site courant, la backend cascadecurrentSiteà null viaON DELETE SET NULL, mais leSiteSelectorcontinue d'afficher le site supprimé et d'émettre des/api/sites/{id}morts jusqu'au reload.useCurrentSite.switchSite()(L62-86) metauth.user.currentSiteà jour localement mais ne recharge niuseSidebar()ni les données de pages déjà affichées (filtrées server-side sur l'ancien site). Le toast success s'affiche sur des données obsolètes.Importants
5.
GET /api/sitesretourne tous les sites de l'instance, pas juste ceux du user —Site::GetCollectionest gardé parsites.viewsans filtre sur$user->getSites(). Un délégataire desites.view(prévu pour alimenter le SiteSelector) peut lire nom, adresse, CP, ville et couleur de tous les sites de tous les tenants. Ajouter unQueryCollectionExtensiondédié à Site (sauf sisites.bypass_scope).6.
User: 4 relations EAGER — explosion N+1 surGET /api/users—rbacRoles,directPermissions,sites,currentSitetous enfetch: 'EAGER'. Justifié pour/api/me(un user), mais sur une collection paginée 30 users, c'est 1 + 120 requêtes par page. Soit retirersites/currentSitedu groupeuser:list(règle aussi le point 3), soit passer en LAZY avecJOIN FETCHexplicite dansMeProvider.Bonus
auth.clearSession()(401) ne réinitialise pasuseCurrentSite/useSidebar/useModules. User A expire → user B login sur le même onglet → site d'A visible quelques ms avant hydration/api/me.SiteSelectorwatcher (L18) ne mirror que la state Pinia locale, pas d'écoutestorageevent. Tab A switche, tab B reste sur l'ancien site et ignore le clic si la tile matche sa state stale.ALTER TABLE "user" ADD current_site_id+CREATE INDEXnon-CONCURRENT dans la même transaction migration bloque les écritures suruserpendant le build — acceptable à la taille actuelle, à surveiller.Version20260420130000; prévoirSELECT id, length(full_address) FROM site WHERE length(full_address) > 255;dans le runbook pré-deploy.🤖 Generated with Claude Code + Opus 4.7 + Codex high-effort
@@ -0,0 +74,4 @@// la mutation directe et garantit la notification des watchers.// N'est appele qu'apres un succes HTTP donc pas de rollback a// prevoir sur cette ligne.auth.setCurrentSite(site)Après
switchSite()succès, seulauth.user.currentSiteest mis à jour — ni la sidebar (useSidebar()), ni les données des pages déjà rendues (filtrées server-side sur l'ancien site viaSiteScopedQueryExtension). L'utilisateur voit un toast success sur des données obsolètes ; la prochaine mutation est cross-site.Fix : ajouter
await useSidebar().refresh()+ émettre un event (mitt / pinia subscribe) que les pageswatchpour re-fetch. Envisager aussi derefreshNuxtData()si le listing a été fetched viauseAsyncData.@@ -0,0 +148,4 @@deleteModalOpen.value = falsesiteToDelete.value = nulldrawerOpen.value = falseawait loadSites()Après suppression,
loadSites()rafraîchit la liste mais pasauth.user. Si l'admin supprime son site courant, la backend passeuser.current_site_idà NULL (cascadeON DELETE SET NULLde la FK) maisauth.user.currentSitecôté Pinia reste le site supprimé. LeSiteSelectordu header continue d'afficher la tile morte jusqu'au prochain/api/me.Fix :
await auth.fetchUser()(ou équivalent) après leloadSites()dans letryblock, ou invalidercurrentSiteà la main si l'id supprimé ===auth.user.currentSite?.id.GET /api/usersn'est pas site-scopé (User n'implémente pasSiteAwareInterface) et le groupeuser:listexposesites,currentSite,rbacRoles,directPermissions,isAdmin. Tout porteur decore.users.viewénumère l'intégralité des users de tous les tenants avec leur topologie RBAC complète.Dans le modèle multi-site visé par cette PR,
core.users.viewdevrait probablement être scopé : soit en rendant User SiteAware (via la jointureuser_site), soit via unQueryCollectionExtensiondédié qui filtre sur$user->getSites() ∩ target.sites != ∅, saufsites.bypass_scope.@@ -110,0 +120,4 @@** @var Collection<int, Site>*/#[ORM\ManyToMany(targetEntity: Site::class, inversedBy: 'users', fetch: 'EAGER')]4 relations EAGER (
rbacRolesL89,directPermissionsL107,sitesL123,currentSiteL140) toutes dans le groupeuser:list. SurGET /api/usersavec pagination 30, c'est 1 + 120 requêtes par page (plus N+1 nested sirole.permissionsest EAGER).Les docblocks justifient EAGER pour
/api/meuniquement. Options :sites/currentSitedu groupeuser:list(règle aussi le leak inter-site)JOIN FETCHdansMeProviderUserListOutputminimal pour le listing.@@ -84,0 +94,4 @@// 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);Les deux flushs (L91 via
persistProcessor+ L130 viaensureCurrentSiteConsistency) ne sont pas wrappés dans la même transaction. Crash entre les deux (OOM, worker killed, perte de connexion DB) laissecurrentSitepointer vers un site absent deuser_site→ viole l'invariant L34-39.Fix : wrapper tout le
process()dans$this->entityManager->wrapInTransaction(function () use (...) { ... }).@@ -0,0 +40,4 @@operations: [new GetCollection(normalizationContext: ['groups' => ['site:read']],security: "is_granted('sites.view')",GetCollection+Getsur/api/sitessont gardés parsites.viewmais sans filtre sur$user->getSites(). Un délégataire desites.viewlit tous les sites de l'instance — nom, adresse, CP, ville, couleur — y compris ceux auxquels il n'a pas accès.Fix : ajouter un
QueryCollectionExtensionInterface+QueryItemExtensionInterfacespécifique à Site qui restreint à$user->getSites(), saufis_granted('sites.bypass_scope'). Pattern identique àSiteScopedQueryExtensionmais ciblé sur Site lui-même.@@ -0,0 +58,4 @@}try {$user->switchCurrentSite($targetSite);TOCTOU :
switchCurrentSite()lit$this->sitesen mémoire puisflush(), sans#[ORM\Version]ni lock.Alice ∈ [S1,S2] PATCH
current-site: S2. En parallèle, admin PATCH/users/alice/rbacretire S2.hasSite(S2)d'Alice est true (collection chargée avant révocation), le flush d'Alice écrase lecurrent_site_id = NULLposé parensureCurrentSiteConsistency→ Alice termine aveccurrentSite=S2sans ligneuser_site.Fix :
$em->refresh($user)avant le check, ou#[ORM\Version] private int $versionsur User + catchOptimisticLockException.- 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.- logout.vue : navigateTo('/login') dans le finally, garanti meme si auth.logout() rejette. - auth.ts : systeme de callbacks onAuthSessionCleared appeles par clearSession() (intercepteur 401 de useApi). Les composables modules s'abonnent pour reset leur state sans que Shared n'importe depuis modules/ (Option C validee par CLAUDE.md, module -> shared autorise). - useCurrentSite.ts : enregistre un reset callback + apres un switch reussi, rafraichit useSidebar().loadSidebar() + refreshNuxtData() (sinon donnees de page obsoletes cote ancien site sous toast success). - SiteSelector.vue : le court-circuit "tile deja active" est retire pour permettre un PATCH de resync quand un autre onglet a bascule le site entre temps. TODO cross-tab : ecouter un storage event dedie. - sites.vue admin : auth.refreshUser() apres delete pour refleter le ON DELETE SET NULL cote user.current_site_id. - Specs vitest : stub useSidebar/refreshNuxtData, test "tile active" retourne sur le nouveau contrat PATCH-toujours.