fix(review) : resout la regression drawers RBAC + race snapshot + stale-data admin
Issues remontees par la seconde passe de review de la PR #9 : - Regression `GET /api/permissions` 403 silencieux sur les drawers RBAC (UserRbacDrawer, RoleDrawer) apres le fix precedent qui imposait `core.permissions.view`. Les users porteurs de `core.users.manage` / `core.roles.manage` ne voyaient plus le catalogue pour hydrater leurs checkboxes. Elargit la security expression sur Permission en OR avec ces deux codes : les gestionnaires ont par nature besoin du catalogue (codes/libelles seuls, pas de secret expose). - Race condition dans UserRbacProcessor : `restoreAbsentCollections()` lisait le snapshot Doctrine hors transaction, puis `wrapInTransaction()` flushait plus tard. Fenetre courte mais reelle ou une modification concurrente aurait pu etre annulee par une restauration depuis un snapshot stale. Deplace l'appel a l'interieur de la transaction. - Stale-data sur les pages admin users / roles / sites : meme pattern try/finally sans catch que sur audit-log (deja corrige). Aligne les trois pages avec un catch qui reset la liste locale. - Tests manquants : garde de non-regression sur PATCH /rbac sans `sites` (assure que la collection elle-meme est preservee, pas seulement le currentSite). Couverture positive sur GET /api/permissions pour les trois branches OR de la security expression (permissions.view, users.manage, roles.manage) via des users non-admin. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -18,13 +18,20 @@ use Symfony\Component\Serializer\Attribute\Groups;
|
||||
|
||||
#[ApiResource(
|
||||
operations: [
|
||||
// Guard RBAC du catalogue de permissions : accepte les gestionnaires
|
||||
// de users et de roles en plus du code dedie `core.permissions.view`.
|
||||
// Justification : les drawers `UserRbacDrawer`/`RoleDrawer` fetchent
|
||||
// systematiquement ce catalogue pour afficher les checkboxes de
|
||||
// permissions ; exiger uniquement `core.permissions.view` casserait
|
||||
// ces workflows pour tout gestionnaire non-admin. L'endpoint n'expose
|
||||
// que des codes/libelles (pas de secret), le bypass reste acceptable.
|
||||
new GetCollection(
|
||||
normalizationContext: ['groups' => ['permission:read']],
|
||||
security: "is_granted('core.permissions.view')",
|
||||
security: "is_granted('core.permissions.view') or is_granted('core.users.manage') or is_granted('core.roles.manage')",
|
||||
),
|
||||
new Get(
|
||||
normalizationContext: ['groups' => ['permission:read']],
|
||||
security: "is_granted('core.permissions.view')",
|
||||
security: "is_granted('core.permissions.view') or is_granted('core.users.manage') or is_granted('core.roles.manage')",
|
||||
),
|
||||
],
|
||||
)]
|
||||
|
||||
@@ -93,19 +93,6 @@ final class UserRbacProcessor implements ProcessorInterface
|
||||
));
|
||||
}
|
||||
|
||||
// Garde anti-ecrasement (defense in depth) : PATCH merge-patch+json impose
|
||||
// que les cles absentes du payload ne mutent PAS les proprietes
|
||||
// correspondantes. La denormalisation API Platform ne respecte pas cet
|
||||
// invariant pour les collections ManyToMany — elle reinstancie une
|
||||
// ArrayCollection vide des que la cle n'est pas presente. Sans cette
|
||||
// garde, un client qui PATCHe juste `{ "isAdmin": true }` verrait toutes
|
||||
// ses roles/directPermissions/sites detruits.
|
||||
//
|
||||
// On lit le body brut de la requete pour connaitre les cles envoyees,
|
||||
// puis on restaure les collections absentes a partir de l'etat d'origine
|
||||
// charge par Doctrine (snapshot des PersistentCollection).
|
||||
$this->restoreAbsentCollections($data);
|
||||
|
||||
$currentUser = $this->security->getUser();
|
||||
|
||||
// Calcul partage entre les deux gardes : l'user perdait-il le flag admin ?
|
||||
@@ -164,6 +151,20 @@ final class UserRbacProcessor implements ProcessorInterface
|
||||
$originalCurrentSiteId,
|
||||
&$result,
|
||||
): void {
|
||||
// Garde anti-ecrasement (defense in depth) : PATCH merge-patch+json impose
|
||||
// que les cles absentes du payload ne mutent PAS les proprietes
|
||||
// correspondantes. La denormalisation API Platform ne respecte pas cet
|
||||
// invariant pour les collections ManyToMany — elle reinstancie une
|
||||
// ArrayCollection vide des que la cle n'est pas presente. Sans cette
|
||||
// garde, un client qui PATCHe juste `{ "isAdmin": true }` verrait toutes
|
||||
// ses roles/directPermissions/sites detruits.
|
||||
//
|
||||
// Execute dans la transaction (et non avant) : garantit que le snapshot
|
||||
// Doctrine lu pour restauration reflete le meme etat BDD que celui sur
|
||||
// lequel le persist va operer. Evite toute fenetre de race entre la
|
||||
// lecture du snapshot et le flush.
|
||||
$this->restoreAbsentCollections($data);
|
||||
|
||||
$result = $this->persistProcessor->process($data, $operation, $uriVariables, $context);
|
||||
|
||||
// Garde coherence currentSite (ticket 2 module Sites).
|
||||
|
||||
Reference in New Issue
Block a user