From 0fc0b57e37b28a050a532f983c5142a2780b4204 Mon Sep 17 00:00:00 2001 From: Matthieu Date: Tue, 14 Apr 2026 16:37:53 +0200 Subject: [PATCH] refactor(core) : RBAC Task 1 - polish apres revue qualite - Permission : guards constructeur (code/label/module non vides, code avec point) - Permission::revive() reutilise updateMetadata() pour eviter la duplication - Suppression de SystemRolesTest (tautologique, ne capture aucun comportement) - Role::permissions : commentaire explicite sur la raison du fetch EAGER - Alignement des types de retour sur static (style User.php) - Nouveau test Role::addPermission avec permissions distinctes Ticket #343 - Task 1 polish (revue qualite). Co-Authored-By: Claude Opus 4.6 (1M context) --- src/Module/Core/Domain/Entity/Permission.php | 36 +++++++++++++++---- src/Module/Core/Domain/Entity/Role.php | 17 ++++++--- .../Core/Domain/Entity/PermissionTest.php | 30 ++++++++++++++++ tests/Module/Core/Domain/Entity/RoleTest.php | 12 +++++++ .../Core/Domain/Security/SystemRolesTest.php | 24 ------------- 5 files changed, 84 insertions(+), 35 deletions(-) delete mode 100644 tests/Module/Core/Domain/Security/SystemRolesTest.php diff --git a/src/Module/Core/Domain/Entity/Permission.php b/src/Module/Core/Domain/Entity/Permission.php index f23759a..19e5ff5 100644 --- a/src/Module/Core/Domain/Entity/Permission.php +++ b/src/Module/Core/Domain/Entity/Permission.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace App\Module\Core\Domain\Entity; use Doctrine\ORM\Mapping as ORM; +use InvalidArgumentException; // TODO: brancher repositoryClass au ticket 343 partie 2 (Task 2). #[ORM\Entity] @@ -31,8 +32,29 @@ class Permission #[ORM\Column(options: ['default' => false])] private bool $orphan = false; + /** + * Invariants : une permission doit avoir un code non vide respectant la + * convention "module.resource[.sub].action" (donc contenir au moins un + * point), un libelle non vide et un module proprietaire non vide. Ces + * garde-fous evitent la persistence de lignes incoherentes si un appelant + * (fixture, commande de synchro, import) oublie un champ ou passe une + * chaine vide. + */ public function __construct(string $code, string $label, string $module) { + if ('' === $code) { + throw new InvalidArgumentException('Le code de permission ne peut pas etre vide.'); + } + if (!str_contains($code, '.')) { + throw new InvalidArgumentException(sprintf('Le code de permission "%s" ne respecte pas la convention "module.resource[.sub].action".', $code)); + } + if ('' === $label) { + throw new InvalidArgumentException('Le libelle de permission ne peut pas etre vide.'); + } + if ('' === $module) { + throw new InvalidArgumentException('Le module proprietaire de la permission ne peut pas etre vide.'); + } + $this->code = $code; $this->label = $label; $this->module = $module; @@ -69,7 +91,7 @@ class Permission * permettre une reactivation ulterieure, mais doit etre ignoree par les * verifications d'autorisation. */ - public function markOrphan(): self + public function markOrphan(): static { $this->orphan = true; @@ -78,14 +100,14 @@ class Permission /** * Reactive une permission precedemment orpheline : son code reapparait - * dans le code source d'un module. On en profite pour rafraichir les - * metadonnees (libelle et module d'appartenance). + * dans le code source d'un module. Equivaut a updateMetadata() suivi d'un + * clearing du flag orphan ; on delegue a updateMetadata() pour ne pas + * dupliquer la logique d'affectation des metadonnees. */ - public function revive(string $label, string $module): self + public function revive(string $label, string $module): static { + $this->updateMetadata($label, $module); $this->orphan = false; - $this->label = $label; - $this->module = $module; return $this; } @@ -95,7 +117,7 @@ class Permission * statut d'orphelin. Utilise par la commande de synchronisation lorsque * seul le libelle ou le module proprietaire a change cote code. */ - public function updateMetadata(string $label, string $module): self + public function updateMetadata(string $label, string $module): static { $this->label = $label; $this->module = $module; diff --git a/src/Module/Core/Domain/Entity/Role.php b/src/Module/Core/Domain/Entity/Role.php index 8db87df..3782a58 100644 --- a/src/Module/Core/Domain/Entity/Role.php +++ b/src/Module/Core/Domain/Entity/Role.php @@ -42,6 +42,15 @@ class Role private bool $isSystem = false; /** @var Collection */ + // Choix deliberé de fetch: 'EAGER' (durcissement, pas oubli de perf) : + // - Evite un lazy-load silencieux pendant un refresh de token JWT ou une + // serialisation hors contexte EntityManager (voir ticket #343, section + // 11 risque #1) ou la collection serait inaccessible et provoquerait + // une erreur opaque. + // - Compromis accepte : surcout SQL volontaire, acceptable a l'echelle + // d'un CRM/ERP PME ou un role porte quelques dizaines de permissions. + // - Si la volumetrie augmente significativement : revoir vers une + // projection cachee (ticket a ouvrir a ce moment-la). #[ORM\ManyToMany(targetEntity: Permission::class, fetch: 'EAGER')] #[ORM\JoinTable(name: 'role_permission')] private Collection $permissions; @@ -90,7 +99,7 @@ class Role * Met a jour le libelle affichable du role. Le code reste immuable pour * garantir la stabilite des references cote fixtures et migrations. */ - public function setLabel(string $label): self + public function setLabel(string $label): static { $this->label = $label; @@ -100,7 +109,7 @@ class Role /** * Met a jour la description libre du role (champ documentaire). */ - public function setDescription(?string $description): self + public function setDescription(?string $description): static { $this->description = $description; @@ -111,7 +120,7 @@ class Role * Ajoute une permission au role. Idempotent : ajouter deux fois la meme * permission n'entraine pas de doublon dans la collection. */ - public function addPermission(Permission $permission): self + public function addPermission(Permission $permission): static { if (!$this->permissions->contains($permission)) { $this->permissions->add($permission); @@ -124,7 +133,7 @@ class Role * Retire une permission du role. Idempotent : retirer une permission * absente est un no-op silencieux. */ - public function removePermission(Permission $permission): self + public function removePermission(Permission $permission): static { $this->permissions->removeElement($permission); diff --git a/tests/Module/Core/Domain/Entity/PermissionTest.php b/tests/Module/Core/Domain/Entity/PermissionTest.php index 0b039c8..af4695c 100644 --- a/tests/Module/Core/Domain/Entity/PermissionTest.php +++ b/tests/Module/Core/Domain/Entity/PermissionTest.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace App\Tests\Module\Core\Domain\Entity; use App\Module\Core\Domain\Entity\Permission; +use InvalidArgumentException; use PHPUnit\Framework\TestCase; /** @@ -54,4 +55,33 @@ final class PermissionTest extends TestCase self::assertTrue($permission->isOrphan()); self::assertSame('Lbl', $permission->getLabel()); } + + public function testConstructorRejectsEmptyCode(): void + { + $this->expectException(InvalidArgumentException::class); + + new Permission('', 'Libelle', 'core'); + } + + public function testConstructorRejectsCodeWithoutDot(): void + { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('invalid_format'); + + new Permission('invalid_format', 'Libelle', 'core'); + } + + public function testConstructorRejectsEmptyLabel(): void + { + $this->expectException(InvalidArgumentException::class); + + new Permission('core.users.view', '', 'core'); + } + + public function testConstructorRejectsEmptyModule(): void + { + $this->expectException(InvalidArgumentException::class); + + new Permission('core.users.view', 'Libelle', ''); + } } diff --git a/tests/Module/Core/Domain/Entity/RoleTest.php b/tests/Module/Core/Domain/Entity/RoleTest.php index 3910550..23e8dd2 100644 --- a/tests/Module/Core/Domain/Entity/RoleTest.php +++ b/tests/Module/Core/Domain/Entity/RoleTest.php @@ -37,6 +37,18 @@ final class RoleTest extends TestCase self::assertSame(1, $role->getPermissions()->count()); } + public function testAddPermissionAddsMultipleDistinct(): void + { + $role = new Role('custom', 'Custom'); + $permissionView = new Permission('core.users.view', 'Voir', 'core'); + $permissionEdit = new Permission('core.users.edit', 'Editer', 'core'); + + $role->addPermission($permissionView); + $role->addPermission($permissionEdit); + + self::assertSame(2, $role->getPermissions()->count()); + } + public function testRemovePermissionRemovesWhenPresent(): void { $role = new Role('custom', 'Custom'); diff --git a/tests/Module/Core/Domain/Security/SystemRolesTest.php b/tests/Module/Core/Domain/Security/SystemRolesTest.php deleted file mode 100644 index 0a1934e..0000000 --- a/tests/Module/Core/Domain/Security/SystemRolesTest.php +++ /dev/null @@ -1,24 +0,0 @@ -