From 1cf550721b75ff0ffc0ea85b1b45cbc02dce6a56 Mon Sep 17 00:00:00 2001 From: Matthieu Date: Wed, 15 Apr 2026 10:31:10 +0200 Subject: [PATCH] docs(rbac) : spec ticket #344 - API CRUD roles & permissions --- docs/rbac/ticket-344-spec.md | 275 +++++++++++++++++++++++++++++++++++ 1 file changed, 275 insertions(+) create mode 100644 docs/rbac/ticket-344-spec.md diff --git a/docs/rbac/ticket-344-spec.md b/docs/rbac/ticket-344-spec.md new file mode 100644 index 0000000..80db2e6 --- /dev/null +++ b/docs/rbac/ticket-344-spec.md @@ -0,0 +1,275 @@ +# Ticket #344 - 2/5 - API CRUD Roles & Permissions (Backend) + +## 1. Objectif + +Exposer via API Platform le socle RBAC livre par le ticket #343 (entites `Role`, `Permission`, relations `User->roles`/`directPermissions`, flag `isAdmin`). Ce ticket livre la surface HTTP minimale permettant : + +- de lister et consulter les permissions synchronisees par `app:sync-permissions`, +- de gerer le cycle de vie des roles (CRUD) tout en protegeant les roles systeme, +- d'attribuer `isAdmin`, les roles RBAC et les permissions directes a un utilisateur sans polluer le groupe `user:write` (commit `0fc4e16`). + +Le ticket n'introduit **aucune logique d'autorisation metier** : toute la verification `is_granted('module.resource.action')` est traitee par le voter du ticket #345. A ce stade, les operations sont gardees par un simple `is_granted('ROLE_ADMIN')`, remplace au #345. + +## 2. Perimetre + +### IN + +- Exposer l'entite `Permission` en API Platform en lecture seule (`GetCollection`, `Get`), groupe `permission:read`, filtres `module` et `orphan`. +- Exposer l'entite `Role` en API Platform avec CRUD complet (`GetCollection`, `Get`, `Post`, `Patch`, `Delete`), groupes `role:read` et `role:write`, filtre `isSystem`. +- Ajouter un processor `RoleProcessor` decorant `PersistProcessor` et `RemoveProcessor` pour : + - refuser la suppression d'un role systeme en traduisant `SystemRoleDeletionException` en `403`, + - empecher la mutation de `code` et `isSystem` sur un role systeme existant. +- Ajouter une operation nommee `user_rbac_patch` (`PATCH /api/users/{id}/rbac`) sur l'entite `User` avec son propre groupe `user:rbac:write` exposant `isAdmin`, `roles` et `directPermissions`. Laisser `user:write` propre pour les champs profil (compatible avec la decision de `0fc4e16`). Le nom explicite est indispensable : API Platform 4 identifie les operations par nom, un `new Patch` sans `name:` entrerait en collision avec l'operation profil existante. +- Ajouter un processor `UserRbacProcessor` qui persiste les mutations RBAC de l'utilisateur sans toucher au password hashing (decorator de `PersistProcessor`, pas du `UserPasswordHasherProcessor`). +- Ajouter sur `Role` les contraintes Symfony Validator : `UniqueEntity(fields: ['code'])`, `Assert\NotBlank` et `Assert\Regex` sur `code`, `Assert\NotBlank` sur `label` (cf. section 6). +- Garder toutes les operations sous `is_granted('ROLE_ADMIN')` avec un commentaire `// TODO ticket #345 : remplacer par is_granted('core.roles.manage')`. +- Tests PHPUnit unitaires (processors) et fonctionnels (`ApiTestCase`) couvrant les chemins nominaux et les cas 403/422. + +### OUT + +- Ticket `#345` : voter `PermissionVoter`, remplacement du `is_granted('ROLE_ADMIN')` par les codes de permission, composable front `usePermissions`. +- Ticket `#346` : ecrans d'administration front (liste/edition des roles et permissions). +- Ticket `#347` : UX des erreurs 403 et integration front de l'ecran de gestion des permissions utilisateur. +- Endpoint d'ecriture sur `Permission` : la table reste la propriete exclusive de `app:sync-permissions` (source de verite = code). +- Lecture des permissions effectives d'un `User` via `/api/me` : traitee au #345 en meme temps que le voter. +- Exposition d'un endpoint de bulk-assign permissions sur plusieurs utilisateurs : hors scope. + +## 3. Fichiers a creer + +### Infrastructure - Processors + +- `/home/matthieu/dev_malio/Coltura/src/Module/Core/Infrastructure/ApiPlatform/State/Processor/RoleProcessor.php` + Decorator de `ApiPlatform\Doctrine\Common\State\PersistProcessor` et `RemoveProcessor`. Charge de la garde `ensureDeletable()` et de la protection des champs immuables sur un role systeme. + +- `/home/matthieu/dev_malio/Coltura/src/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessor.php` + Decorator de `PersistProcessor` specifique a l'operation `PATCH /api/users/{id}/rbac`. Persiste les mutations `isAdmin`, `roles`, `directPermissions` sans passer par `UserPasswordHasherProcessor`. + +### Tests unitaires + +- `/home/matthieu/dev_malio/Coltura/tests/Module/Core/Infrastructure/ApiPlatform/State/Processor/RoleProcessorTest.php` +- `/home/matthieu/dev_malio/Coltura/tests/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessorTest.php` + +### Tests fonctionnels + +- `/home/matthieu/dev_malio/Coltura/tests/Module/Core/Api/PermissionApiTest.php` +- `/home/matthieu/dev_malio/Coltura/tests/Module/Core/Api/RoleApiTest.php` +- `/home/matthieu/dev_malio/Coltura/tests/Module/Core/Api/UserRbacApiTest.php` + +## 4. Fichiers a modifier + +### Entite `Permission` + +`/home/matthieu/dev_malio/Coltura/src/Module/Core/Domain/Entity/Permission.php` + +- Ajouter l'attribut `#[ApiResource]` avec operations `GetCollection` + `Get` uniquement. +- Normalization context : groupe `permission:read` uniquement. +- Pas de `denormalizationContext` (lecture seule). +- Security `is_granted('ROLE_ADMIN')` sur les deux operations (TODO #345). +- Ajouter `#[Groups(['permission:read'])]` sur `$id`, `$code`, `$label`, `$module`, `$orphan`. Pas d'ajout du groupe `role:read` : on laisse API Platform serialiser la relation `Role::$permissions` en IRIs par defaut, le front resoudra les details en 2 appels si necessaire (decision explicite pour garder les payloads petits et les permissions paginable independamment). +- Ajouter les filtres API Platform `SearchFilter` sur `module` (exact) et `BooleanFilter` sur `orphan`. + +Extrait attendu : + +```php +#[ApiResource( + operations: [ + new GetCollection( + security: "is_granted('ROLE_ADMIN')", + normalizationContext: ['groups' => ['permission:read']], + ), + new Get( + security: "is_granted('ROLE_ADMIN')", + normalizationContext: ['groups' => ['permission:read']], + ), + ], +)] +#[ApiFilter(SearchFilter::class, properties: ['module' => 'exact'])] +#[ApiFilter(BooleanFilter::class, properties: ['orphan'])] +``` + +### Entite `Role` + +`/home/matthieu/dev_malio/Coltura/src/Module/Core/Domain/Entity/Role.php` + +- Ajouter l'attribut `#[ApiResource]` avec operations `GetCollection`, `Get`, `Post`, `Patch`, `Delete`. +- Normalization context : `role:read`. Denormalization context : `role:write`. +- Processor `RoleProcessor::class` sur `Post`, `Patch` et `Delete`. +- Security `is_granted('ROLE_ADMIN')` sur les 5 operations (TODO #345). +- Groupes : + - `$id` : `role:read`. + - `$code` : `role:read`, `role:write`. L'immuabilite apres creation est portee par `RoleProcessor` (variante A, cf. section 5), pas par un decoupage de groupes. + - `$label` : `role:read`, `role:write`. + - `$description` : `role:read`, `role:write`. + - `$isSystem` : `role:read` (jamais writable via API). + - `$permissions` : `role:read`, `role:write`. Serialise en IRIs (comportement API Platform par defaut sur une relation ManyToMany). +- Filtre `BooleanFilter` sur `isSystem`. +- **Important** : le constructeur actuel `public function __construct(string $code, string $label, bool $isSystem = false, ?string $description = null)` doit etre compatible avec la denormalisation API Platform sur `POST`. API Platform 4 resout les arguments du constructeur par nom de propriete denormalise. Verifier (ou adapter) que `isSystem` ne peut pas etre injecte par le POST car il n'est pas dans `role:write`. + +### Entite `User` + +`/home/matthieu/dev_malio/Coltura/src/Module/Core/Domain/Entity/User.php` + +- Ajouter dans la liste des operations `ApiResource` existantes une operation dediee : + +```php +new Patch( + name: 'user_rbac_patch', + uriTemplate: '/users/{id}/rbac', + security: "is_granted('ROLE_ADMIN')", + denormalizationContext: ['groups' => ['user:rbac:write']], + processor: UserRbacProcessor::class, +), +``` + +Le `name:` est OBLIGATOIRE : sans lui, API Platform 4 deduit un nom par defaut qui peut collisionner avec la `Patch` profil existante (meme classe, meme methode HTTP) et provoquer un ecrasement silencieux de la route `/api/users/{id}`. + +- Ajouter le groupe `user:rbac:write` sur les proprietes : + - `$isAdmin` + - `$roles` + - `$directPermissions` +- Ne PAS toucher `user:write` : la decision de `0fc4e16` est confirmee par ce ticket. + +Raison de l'endpoint dedie (option B) : +- Separation des preoccupations : un `PATCH /api/users/{id}` reste un endpoint "profil" ; la promotion admin et la gestion des permissions est un acte administratif explicite et tracable. +- Facilite future l'ajout d'un audit log dedie (`#355` audit log project) sur l'endpoint RBAC sans polluer l'audit profil. +- Contrat front simple : une seule route, un seul groupe, une seule validation. + +## 5. Regles metier et cas limites + +### Role + +- **Creation (`POST /api/roles`)** : + - `code`, `label` obligatoires. `description` optionnel. `permissions` optionnel (tableau d'IRIs). + - `isSystem` est toujours `false` pour les roles crees via API (n'est pas dans `role:write`). + - Unicite du `code` geree par la contrainte DB `uniq_role_code` → 422 via `UniqueEntity` validator a ajouter sur l'entite (voir section 6). + +- **Modification (`PATCH /api/roles/{id}`)** : + - `label`, `description`, `permissions` modifiables librement, y compris sur un role systeme (utile pour customiser l'apparence dans l'UI sans casser la relation). + - `code` **immuable apres creation** — strategie retenue (variante A) : un seul groupe `role:write` contenant `code`, et une garde centralisee dans `RoleProcessor`. Le processor compare la valeur entrante a l'etat d'origine via `UnitOfWork::getOriginalEntityData($role)['code']` ; si elle differe, leve `BadRequestHttpException` avec un message francais explicite. Regle unique et uniforme : roles systeme ET roles customs sont concernes. Justification : garder la regle metier dans le domaine applicatif plutot que dupliquer les groupes de serialisation. + +- **Suppression (`DELETE /api/roles/{id}`)** : + - `RoleProcessor` appelle `$role->ensureDeletable()` avant de deleguer au `RemoveProcessor`. + - `SystemRoleDeletionException` est catchee et re-levee en `Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException` (403). + - Les relations `user_role` et `role_permission` sur ce role sont nettoyees automatiquement par le `ON DELETE CASCADE` des contraintes `FK_2DE8C6A3D60322AC` (`user_role.role_id`) et `FK_6F7DF886D60322AC` (`role_permission.role_id`) posees dans `migrations/Version20260414150034.php`. Aucun nettoyage manuel necessaire dans `RoleProcessor`. Verifier en test fonctionnel par un DELETE d'un role custom attache a un user, puis assert que le user existe toujours et que `user_role` est vide pour ce couple. + +### Permission + +- Lecture seule via API. Aucun endpoint de mutation. +- Si un admin veut forcer une permission sur un utilisateur, il passe par `directPermissions` de `User`. + +### User (operation RBAC) + +- `PATCH /api/users/{id}/rbac` n'accepte que `isAdmin`, `roles`, `directPermissions`. Tout autre champ dans le payload est ignore (comportement par defaut d'API Platform avec un `denormalizationContext` restreint). +- **Garde minimale auto-suicide** : `UserRbacProcessor` refuse (`BadRequestHttpException` 400) toute requete ou l'user cible est egal a l'user courant du `Security::getUser()` ET `isAdmin` passe de `true` a `false`. Sans cette garde, un admin peut se degrader seul et perdre acces a l'endpoint, creant une situation de recovery penible. C'est une garde locale et pragmatique, volontairement plus stricte que "le dernier admin" : on interdit l'auto-degradation, point. La garde "plus d'un admin restant" reste reportee au #345 ou un inventaire global fera sens avec le voter. TODO a placer dans le processor avec reference a #345. +- Le password n'est jamais touche par cet endpoint (contrairement a `UserPasswordHasherProcessor` sur `PATCH /api/users/{id}`). + +## 6. Validation + +- Ajouter sur `Role` une contrainte `#[UniqueEntity(fields: ['code'])]` pour un 422 propre au lieu d'un 500 SQL en cas de conflit. +- Ajouter sur `Role::$code` un `#[Assert\NotBlank]` et un `#[Assert\Regex('/^[a-z][a-z0-9_]*$/')]` (meme convention que les permissions). +- Ajouter sur `Role::$label` un `#[Assert\NotBlank]`. + +## 7. Plan de tests + +### Unitaires + +**`RoleProcessorTest`** + +- `process()` d'un role non-systeme en DELETE delegue au `RemoveProcessor` sans lever. +- `process()` d'un role systeme en DELETE leve `AccessDeniedHttpException` (403) et n'appelle pas le decorator. +- `process()` d'un role systeme en PATCH dont le `code` a change leve `BadRequestHttpException`. +- `process()` d'un role systeme en PATCH dont seuls `label`/`permissions` changent delegue au `PersistProcessor`. +- `process()` d'un role non-systeme en POST delegue au `PersistProcessor`. + +**`UserRbacProcessorTest`** + +- `process()` persiste un user avec `isAdmin = true` via le decorator. +- `process()` persiste une collection de `roles` mise a jour. +- `process()` ne declenche jamais le hashing de password (verifier que `UserPasswordHasherProcessor` n'est pas dans la chaine). + +### Fonctionnels (`ApiTestCase`) + +**`PermissionApiTest`** + +- `GET /api/permissions` en tant qu'admin retourne la liste des permissions synchronisees. +- `GET /api/permissions?module=core` filtre par module. +- `GET /api/permissions?orphan=true` retourne uniquement les orphelines. +- `GET /api/permissions/{id}` retourne les champs attendus (groupe `permission:read`). +- `POST /api/permissions` en tant qu'admin retourne `405 Method Not Allowed`. +- `GET /api/permissions` non authentifie retourne `401`. +- `GET /api/permissions` en tant que user standard retourne `403`. + +**`RoleApiTest`** + +- `POST /api/roles` avec `{code, label, description}` retourne `201` et persiste `isSystem = false`. +- `POST /api/roles` avec un `code` deja utilise retourne `422`. +- `POST /api/roles` avec un `code` invalide (`MAJ`, `space`) retourne `422`. +- `PATCH /api/roles/{id}` sur un role custom modifie `label` et ajoute des permissions via IRIs → `200`. +- `PATCH /api/roles/{id}` sur le role `admin` (systeme) modifiant seulement `label` → `200`. +- `PATCH /api/roles/{id}` sur le role `admin` tentant de modifier `code` → `400`. +- `DELETE /api/roles/{id}` sur un role custom → `204`. +- `DELETE /api/roles/{id}` sur le role `admin` → `403` avec `SystemRoleDeletionException` traduite. +- `DELETE /api/roles/{id}` d'un role custom attache a un user : le user reste, la relation `user_role` est nettoyee par le CASCADE. +- Toute operation sans auth retourne `401`. +- Toute operation en tant que user standard retourne `403`. + +**`UserRbacApiTest`** + +- `PATCH /api/users/{id}/rbac` en tant qu'admin avec `{isAdmin: true}` promeut le user. +- `PATCH /api/users/{id}/rbac` avec `{roles: [IRI...]}` remplace la collection de roles RBAC. +- `PATCH /api/users/{id}/rbac` avec `{directPermissions: [IRI...]}` remplace les permissions directes. +- `PATCH /api/users/{id}/rbac` en tant que user standard retourne `403`. +- `PATCH /api/users/{id}/rbac` non authentifie retourne `401`. +- `PATCH /api/users/{id}/rbac` avec un champ `username` dans le payload n'est pas persiste (denormalization context restreint). +- `PATCH /api/users/{id}` sans `/rbac` avec `{isAdmin: true}` ne modifie PAS `isAdmin` (confirme la decision `0fc4e16`). + +## 8. Securite et traduction d'exceptions + +- `SystemRoleDeletionException` → `AccessDeniedHttpException` (403) dans `RoleProcessor` (pas via un listener global : on garde la traduction locale au perimetre RBAC). +- `BadRequestHttpException` pour la mutation de `code` sur un role systeme : message explicite en francais, dans le payload Hydra `hydra:description`. +- Toutes les routes ont pour l'instant `security: "is_granted('ROLE_ADMIN')"`. Un commentaire `// TODO ticket #345` doit etre present sur chaque attribut pour faciliter le remplacement. + +## 9. Conventions et architecture + +- Respect strict du modular monolith : tous les fichiers crees vivent dans `src/Module/Core/` ou `tests/Module/Core/`. Aucun import depuis un autre module. +- `declare(strict_types=1)` en tete des nouveaux fichiers. +- Commentaires PHP en francais, identifiants anglais (`CLAUDE.md`). +- Processors branches via l'autoconfiguration Symfony ; aucun wiring manuel dans `services.yaml` attendu si le constructeur est injecte proprement. +- Pattern de decorator : utiliser `#[AsDecorator]` ou `#[Autoconfigure]` pour brancher le processor en tant que decorator du `PersistProcessor` API Platform, selon le pattern deja utilise par `UserPasswordHasherProcessor`. +- Aucune nouvelle entree necessaire dans `config/modules.php` ni `config/sidebar.php`. + +## 10. Ordre d'execution recommande + +1. Ajouter l'attribut `#[ApiResource]` et les `#[Groups]` sur `Permission`. Ecrire `PermissionApiTest`. +2. Ajouter les contraintes Validator sur `Role`. Ajouter `#[ApiResource]` et les `#[Groups]` sur `Role` **sans** processor dans un premier temps pour valider le CRUD nominal. +3. Creer `RoleProcessor` et le brancher en decorator. Ajouter les gardes systeme. Ecrire `RoleProcessorTest` + cas `RoleApiTest`. +4. Creer `UserRbacProcessor`. Ajouter l'operation `/users/{id}/rbac` et le groupe `user:rbac:write` sur `User`. Ecrire `UserRbacProcessorTest` + `UserRbacApiTest`. +5. `make test` complet + `make php-cs-fixer-allow-risky`. +6. Documentation : referencer ce spec dans `docs/rbac/` et mettre a jour le fil conducteur RBAC si un index existe. + +## 11. Risques et points d'attention + +- **Constructeur de `Role` et denormalisation POST** : API Platform 4 resout les arguments du constructeur par nom ; `isSystem` est dans la signature mais pas dans `role:write`, donc un client ne peut pas l'injecter — a verifier par un test explicite ("POST avec `isSystem: true` est ignore"). +- **`code` immuable** : strategie retenue (garde dans processor) simple mais demande une lecture de l'etat initial du role avant persistance. Utiliser `UnitOfWork::getOriginalEntityData()` pour recuperer la valeur d'origine proprement. +- **Cascade de delete role → user_role** : depend de `ON DELETE CASCADE` pose par la migration #343. Verifier explicitement en test fonctionnel qu'aucune `ForeignKeyConstraintViolationException` ne remonte. +- **`UniqueEntity` sur `code`** : ne couvre pas les conflits en race condition, la DB reste la garde ultime. Acceptable. +- **Pas de filtre sur le `module` de Permission cote front** au #346 sans le filtre API : s'assurer que le filtre est bien pose ici. +- **Auto-retrait du dernier admin** : garde d'**auto-suicide** posee dans `UserRbacProcessor` (un admin ne peut pas se degrader lui-meme, cf. section 5). La garde "plus d'un admin restant" au niveau global reste reportee au voter #345. +- **Infra de test fonctionnel (fixtures et isolation)** : les tests `*ApiTest` dependent de la presence en base des roles systeme `admin` et `user`. L'infra actuelle doit fournir soit un reload des fixtures par classe de test, soit `DAMADoctrineTestBundle` pour transactionner chaque test. A verifier au debut de l'etape 1 de l'ordre d'execution ; si absent, ajouter un trait de bootstrap minimal `RbacFixturesTrait` qui insere les deux roles systeme avant chaque classe de test (pas par test, trop couteux). Ne pas bloquer le ticket sur cette question, adapter au vol. + +## 12. Criteres d'acceptation (DoD) + +- `GET /api/permissions` et `GET /api/permissions/{id}` fonctionnent, filtres `module` et `orphan` operationnels. +- CRUD complet sur `/api/roles` operationnel, avec `isSystem` en lecture seule cote API. +- `DELETE /api/roles/{admin_id}` retourne `403` avec un message metier. +- `PATCH /api/roles/{admin_id}` autorise la modification de `label`/`permissions` mais refuse la modification de `code` avec `400`. +- `PATCH /api/users/{id}/rbac` permet de modifier `isAdmin`, `roles` et `directPermissions` ; `PATCH /api/users/{id}` (profil) ne les modifie jamais. +- Les operations API sont gardees par `is_granted('ROLE_ADMIN')` et commentees avec la TODO pointant vers #345. +- `make test` passe ; `make php-cs-fixer-allow-risky` ne laisse aucun delta. +- Aucun import croise entre modules ; tous les fichiers crees vivent dans `Module/Core/` ou `tests/Module/Core/`. +- Le spec est mergee avec le code (meme PR ou PR precedente) pour rester la reference du ticket. + +## 13. Remarques de branche + +- Le ticket enonce "Branche a creer : `feat/rbac-api` depuis develop apres merge de #2". +- Branche courante locale : `feat/rbac-core`. A confirmer avec l'utilisateur si PR #2 est mergee : si oui, se rebaser sur `develop` et creer `feat/rbac-api` propre ; sinon, empiler `feat/rbac-api` sur `feat/rbac-core` en attendant le merge.