diff --git a/docs/rbac/ticket-345-spec.md b/docs/rbac/ticket-345-spec.md index 1816d10..187bdd9 100644 --- a/docs/rbac/ticket-345-spec.md +++ b/docs/rbac/ticket-345-spec.md @@ -572,3 +572,78 @@ Chaque etape doit etre revue (spec compliance + code quality) avant de passer a - Branche de travail : `feat/rbac-voter`, tiree de `feat/rbac-api`. - Pas de PR dediee : les commits #345 s'empilent sur la PR #3 existante ouverte vers `develop`. - Une fois la PR #3 mergee, la branche finale de l'epic RBAC (`feat/rbac-admin-ui` pour #346) partira de `develop`. + +## 18. Evolutions post-livraison — `UserRbacProcessor` defense in depth + +Voir aussi : `docs/sites/ticket-02-spec.md` § 10 pour la problematique cote +Sites qui a motive cette evolution. + +### 18.1 — Semantique `merge-patch+json` respectee + +Le processor originel appliquait telles quelles les mutations produites par la +denormalisation API Platform. Or API Platform reinstancie par defaut une +`ArrayCollection` vide pour chaque propriete ManyToMany absente du payload, +ce qui viole la semantique `application/merge-patch+json` : les cles absentes +ne doivent PAS muter les proprietes correspondantes. + +Consequence concrete du bug : un PATCH minimal comme `{ "isAdmin": true }` +detruisait silencieusement toutes les collections (`rbacRoles`, +`directPermissions`, `sites`) du user cible. + +La garde `restoreAbsentCollections()` introduite dans `UserRbacProcessor` +resout cela en : + +1. Injectant `RequestStack` pour lire le body JSON brut de la requete. +2. Decodant les cles effectivement envoyees par le client. +3. Pour chaque cle RBAC (`roles`, `directPermissions`, `sites`) absente du + payload : restaurant la collection a son etat d'origine a partir du + snapshot Doctrine (`PersistentCollection::getSnapshot()`), puis appelant + `takeSnapshot()` pour marquer la collection comme non-dirty (aucune query + `UPDATE` n'est emise sur les tables de jointure). +4. No-op si la cle est presente (la denormalisation fait foi). + +Matrice finale : + +| Payload | Effet | +|---------------------------------|-------------------------------------| +| Cle absente | Propriete preservee (BDD inchangee) | +| Cle presente = `[]` | Collection videe (vidage explicite) | +| Cle presente = `[...]` | Collection remplacee | + +### 18.2 — Nouvelle operation `GET /users/{id}/rbac` + +Le drawer d'edition (`UserRbacDrawer.vue`) ne peut plus dependre du payload +de liste `/api/users` (groupe `user:list`) pour initialiser l'etat `sites` +car ce groupe reste volontairement leger (cf. ticket Sites #02). Une +operation `Get` dediee est ajoutee, symetrique au `Patch` existant : + +- URI : `/users/{id}/rbac` +- Security : `is_granted('core.users.manage')` (plus strict que `.view`) +- Groupe : `user:rbac:read` (contient `isAdmin`, `roles`, `directPermissions`, + `sites`). + +Le drawer charge desormais ce GET en parallele des referentiels au moment +de l'ouverture, via un watch combine `[modelValue, user.id]` qui recharge +correctement si le user change sans fermeture du drawer entre-temps. + +### 18.3 — Impact sur les tests + +`UserRbacProcessorTest` : le constructor gagne un argument `RequestStack`. +Les tests existants injectent une `RequestStack` avec une `Request` vide +(body `""`), ce qui rend la garde no-op — le comportement des assertions +existantes est conserve. De nouveaux tests couvrent la garde : + +- PATCH sans cle `sites` ne mute pas la collection d'origine. +- PATCH avec `sites: []` vide bien la collection (pas de regression du cas + "vidage explicite"). +- PATCH avec `sites: [...]` remplace comme avant. + +### 18.4 — Criteres de validation additionnels + +- [ ] `GET /users/{id}/rbac` retourne 200 avec `core.users.manage`, 403 sans. +- [ ] Le payload contient `{ id, isAdmin, roles, directPermissions, sites }`. +- [ ] `PATCH /users/{id}/rbac` avec cle absente preserve la collection BDD. +- [ ] `PATCH /users/{id}/rbac` avec `[]` vide la collection et declenche + `ensureCurrentSiteConsistency` (cas sites). +- [ ] Les 228 tests PHPUnit existants passent apres ajout du parametre + `RequestStack` au constructor. diff --git a/docs/sites/ticket-02-spec.md b/docs/sites/ticket-02-spec.md index 8c60f78..7fdb6f2 100644 --- a/docs/sites/ticket-02-spec.md +++ b/docs/sites/ticket-02-spec.md @@ -590,3 +590,112 @@ Le ticket autorise un user sans sites (`sites: []`, `currentSite: null`). Mais a - [ ] `make test` passe toutes les suites (les 5 nouvelles + les existantes ajustees aux fixtures). - [ ] `make php-cs-fixer-allow-risky` propre sur les fichiers nouveaux et modifies. - [ ] Desactiver `SitesModule::class` dans `config/modules.php` ne casse pas les endpoints Core (la DB reste valide, les users conservent leurs sites meme si l'UI ne les expose plus). + +## 10. Evolutions post-livraison — drawer RBAC et defense in depth + +Apres la livraison initiale du ticket, un bug utilisateur a revele que le drawer +`UserRbacDrawer.vue` demarrait toujours avec 0 site coche pour un user qui en +avait en BDD, et que la sauvegarde ecrasait silencieusement les sites +existants. Root cause : l'endpoint `GET /api/users` utilise le groupe `user:list` +qui n'expose pas la collection `sites` (choix assume pour garder le payload +leger et eviter toute fuite croisee site). Le drawer initialisait donc +`selectedSiteIds` a partir d'un `user.sites` toujours `undefined`. + +Deux evolutions ont ete apportees pour corriger cela proprement sans elargir la +surface de fuite de `/api/users` : + +### 10.1 — Nouvelle operation `GET /users/{id}/rbac` + +Une operation API Platform `Get` est ajoutee sur `User`, symetrique au `Patch` +existant, sous la meme URI `/users/{id}/rbac` : + +```php +new Get( + name: 'user_rbac_get', + uriTemplate: '/users/{id}/rbac', + security: "is_granted('core.users.manage')", + normalizationContext: ['groups' => ['user:rbac:read']], +), +``` + +Raisons du design : +- **Symetrie REST** : GET et PATCH partagent la meme URI et le meme groupe de + normalisation, documentation OpenAPI et appels clients lisibles. +- **Separation list/detail** : `/api/users` (`user:list`) reste maigre — pas de + collection, pas de fuite. `/users/{id}/rbac` (`user:rbac:read`) porte le + detail riche requis par le drawer d'edition. +- **Garde de permission plus stricte** : `core.users.manage` (et non `.view`) + — le detail RBAC est concu pour l'edition, pas la consultation. +- **Isolation du couplage Sites** : la dependance au module Sites reste scopee + a cet endpoint et a `/api/me`. Elle n'est pas disseminee dans tous les + payloads de liste. + +Cote frontend (`UserRbacDrawer.vue`) : +- `loadData(userId)` fetch desormais `/users/{id}/rbac` en parallele des + referentiels (roles, permissions, sites globaux). +- Le watch combine `[modelValue, user.id]` recharge le detail a chaque + ouverture ou changement de user sans dependance fragile sur `props.user`. +- Le type `UserListItem` perd `sites` (inutilise) ; un nouveau type + `UserRbacDetail` represente le payload du GET dedie. +- La colonne "Sites" de `/admin/users` est retiree : l'info est consultee + dans le drawer. Cela supprime aussi le second fetch `/api/sites` sur la + page de liste. + +### 10.2 — Garde anti-ecrasement dans `UserRbacProcessor` + +API Platform denormalize les collections ManyToMany comme des `ArrayCollection` +vides quand la cle JSON correspondante est absente du payload, violant la +semantique `merge-patch+json` qui impose que les cles absentes ne mutent PAS +les proprietes. Pour un PATCH qui ne veut toucher que `isAdmin`, cela +detruirait tous les sites/roles/directPermissions du user. + +Le processor injecte desormais `RequestStack`, lit le body JSON brut au debut +de `process()`, et pour chaque collection absente du payload restaure l'etat +d'origine a partir du snapshot Doctrine : + +```php +// Mapping cle JSON → accesseurs PHP (note : 'roles' → getRbacRoles) +private const COLLECTION_MAP = [ + 'roles' => ['getter' => 'getRbacRoles', ...], + 'directPermissions' => ['getter' => 'getDirectPermissions', ...], + 'sites' => ['getter' => 'getSites', ...], +]; + +private function restoreAbsentCollections(User $user): void +{ + $payload = json_decode($this->requestStack->getCurrentRequest()?->getContent() ?? '', true); + foreach (self::COLLECTION_MAP as $jsonKey => $accessors) { + if (array_key_exists($jsonKey, $payload)) { + continue; // cle presente = la denormalisation fait foi + } + // cle absente = restaurer le snapshot PersistentCollection + // (voir implementation complete dans UserRbacProcessor.php) + } +} +``` + +Semantique finale garantie : + +| Payload | Effet sur la collection | +|---------------------------------|------------------------------------| +| Cle absente | Preservee (etat BDD inchange) | +| `"sites": []` | Collection videe explicitement | +| `"sites": ["/api/sites/1"]` | Collection remplacee | + +La garde `ensureCurrentSiteConsistency` continue de s'executer apres persist +avec la meme logique : elle est triggered uniquement si la collection a +effectivement mute (detection via `PersistentCollection::isDirty()` post-restore). + +### 10.3 — Criteres de validation additionnels + +- [ ] `GET /users/{id}/rbac` retourne 200 pour `core.users.manage`, 403 sinon. +- [ ] Le payload contient `{ id, isAdmin, roles, directPermissions, sites }`. +- [ ] `GET /api/users` ne contient plus `sites` (verification non-regression). +- [ ] Ouvrir le drawer d'un user avec des sites en BDD affiche les cases + pre-cochees correspondantes. +- [ ] `PATCH /users/{id}/rbac` avec `{ "isAdmin": true }` (sans autre cle) ne + modifie pas sites/roles/directPermissions. +- [ ] `PATCH /users/{id}/rbac` avec `{ "sites": [] }` vide explicitement la + collection et bascule `currentSite` a `NULL` via la garde existante. +- [ ] `PATCH /users/{id}/rbac` avec `{ "sites": [...] }` remplace la + collection comme auparavant.