docs(specs) : documente GET /users/{id}/rbac et garde anti-ecrasement merge-patch
Ajoute les sections "Evolutions post-livraison" aux specs Sites #02 et RBAC #345 pour refleter les modifs apportees apres la livraison initiale : - GET /users/{id}/rbac symetrique au PATCH, pour charger le detail d'edition sans elargir le groupe user:list (le payload de liste reste leger, la dependance Core → Sites reste scopee a cet endpoint et a /api/me). - Garde restoreAbsentCollections() dans UserRbacProcessor qui respecte la semantique merge-patch+json : cle absente = preservee, cle = [] = videe, cle = [...] = remplacee. Restauration a partir du snapshot Doctrine des PersistentCollection pour roles / directPermissions / sites. - Nouveaux criteres de validation + matrice de semantique. Verification archi modular monolith : Commercial et Sites peuvent etre desactives dans config/modules.php sans casser l'app (sidebar filtree, switcher masque, endpoints admin rediriges via disabledRoutes). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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`.
|
- 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`.
|
- 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`.
|
- 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.
|
||||||
|
|||||||
@@ -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 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.
|
- [ ] `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).
|
- [ ] 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.
|
||||||
|
|||||||
Reference in New Issue
Block a user