From 534bdbccdd92c8096a191f305f268e2cc2a11ac5 Mon Sep 17 00:00:00 2001 From: Matthieu Date: Wed, 15 Apr 2026 14:28:02 +0200 Subject: [PATCH] refactor(core) : RBAC #344 - polish review - narrow rbac read group + fail-fast processors --- src/Module/Core/Domain/Entity/User.php | 12 +++++++----- .../ApiPlatform/State/Processor/RoleProcessor.php | 15 +++++++++------ .../State/Processor/UserRbacProcessor.php | 12 +++++++++--- .../State/Processor/RoleProcessorTest.php | 15 +++++++++++++++ .../State/Processor/UserRbacProcessorTest.php | 14 ++++++++++++++ 5 files changed, 54 insertions(+), 14 deletions(-) diff --git a/src/Module/Core/Domain/Entity/User.php b/src/Module/Core/Domain/Entity/User.php index a62dbd5..0426121 100644 --- a/src/Module/Core/Domain/Entity/User.php +++ b/src/Module/Core/Domain/Entity/User.php @@ -43,7 +43,7 @@ use Symfony\Component\Serializer\Attribute\SerializedName; uriTemplate: '/users/{id}/rbac', // TODO ticket #345 : remplacer par is_granted('core.users.manage') security: "is_granted('ROLE_ADMIN')", - normalizationContext: ['groups' => ['user:list']], + normalizationContext: ['groups' => ['user:rbac:read']], denormalizationContext: ['groups' => ['user:rbac:write']], processor: UserRbacProcessor::class, ), @@ -58,7 +58,7 @@ class User implements UserInterface, PasswordAuthenticatedUserInterface #[ORM\Id] #[ORM\GeneratedValue] #[ORM\Column] - #[Groups(['me:read', 'user:list'])] + #[Groups(['me:read', 'user:list', 'user:rbac:read'])] private ?int $id = null; #[ORM\Column(length: 180, unique: true)] @@ -66,7 +66,7 @@ class User implements UserInterface, PasswordAuthenticatedUserInterface private ?string $username = null; #[ORM\Column(name: 'is_admin', options: ['default' => false])] - #[Groups(['me:read', 'user:list', 'user:rbac:write'])] + #[Groups(['me:read', 'user:list', 'user:rbac:write', 'user:rbac:read'])] private bool $isAdmin = false; /** @@ -81,7 +81,7 @@ class User implements UserInterface, PasswordAuthenticatedUserInterface */ #[ORM\ManyToMany(targetEntity: Role::class, fetch: 'EAGER')] #[ORM\JoinTable(name: 'user_role')] - #[Groups(['me:read', 'user:list', 'user:rbac:write'])] + #[Groups(['me:read', 'user:list', 'user:rbac:write', 'user:rbac:read'])] // La propriete s'appelle `rbacRoles` cote PHP pour ne pas entrer en // collision avec UserInterface::getRoles() (qui renvoie list) ; // on reexpose la cle JSON sous `roles` via SerializedName pour rester @@ -99,7 +99,7 @@ class User implements UserInterface, PasswordAuthenticatedUserInterface */ #[ORM\ManyToMany(targetEntity: Permission::class, fetch: 'EAGER')] #[ORM\JoinTable(name: 'user_permission')] - #[Groups(['me:read', 'user:list', 'user:rbac:write'])] + #[Groups(['me:read', 'user:list', 'user:rbac:write', 'user:rbac:read'])] private Collection $directPermissions; #[ORM\Column] @@ -152,6 +152,8 @@ class User implements UserInterface, PasswordAuthenticatedUserInterface * la Collection peut ne pas etre hydratee. On se contente d'un calcul * base sur un scalaire. * + * @see getRbacRoles() pour la collection RBAC metier (exposee en JSON sous la cle "roles"). + * * @return list */ public function getRoles(): array diff --git a/src/Module/Core/Infrastructure/ApiPlatform/State/Processor/RoleProcessor.php b/src/Module/Core/Infrastructure/ApiPlatform/State/Processor/RoleProcessor.php index 70a8ba3..cb1be0d 100644 --- a/src/Module/Core/Infrastructure/ApiPlatform/State/Processor/RoleProcessor.php +++ b/src/Module/Core/Infrastructure/ApiPlatform/State/Processor/RoleProcessor.php @@ -10,6 +10,7 @@ use ApiPlatform\State\ProcessorInterface; use App\Module\Core\Domain\Entity\Role; use App\Module\Core\Domain\Exception\SystemRoleDeletionException; use Doctrine\ORM\EntityManagerInterface; +use LogicException; use Symfony\Component\DependencyInjection\Attribute\Autowire; use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException; use Symfony\Component\HttpKernel\Exception\BadRequestHttpException; @@ -44,12 +45,14 @@ final class RoleProcessor implements ProcessorInterface public function process(mixed $data, Operation $operation, array $uriVariables = [], array $context = []): mixed { if (!$data instanceof Role) { - // Securite : si le provider n'a pas fourni un Role, on delegue - // quand meme au processor approprie pour ne pas etouffer - // silencieusement un bug de configuration. - return $operation instanceof DeleteOperationInterface - ? $this->removeProcessor->process($data, $operation, $uriVariables, $context) - : $this->persistProcessor->process($data, $operation, $uriVariables, $context); + // Ce processor est wire exclusivement sur les operations Role. + // Si on arrive ici avec autre chose, c'est une misconfiguration + // qu'il faut faire remonter fort. + throw new LogicException(sprintf( + 'RoleProcessor attend une instance de %s, %s recu.', + Role::class, + get_debug_type($data), + )); } if ($operation instanceof DeleteOperationInterface) { diff --git a/src/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessor.php b/src/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessor.php index d97f846..4215d74 100644 --- a/src/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessor.php +++ b/src/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessor.php @@ -8,6 +8,7 @@ use ApiPlatform\Metadata\Operation; use ApiPlatform\State\ProcessorInterface; use App\Module\Core\Domain\Entity\User; use Doctrine\ORM\EntityManagerInterface; +use LogicException; use Symfony\Bundle\SecurityBundle\Security; use Symfony\Component\DependencyInjection\Attribute\Autowire; use Symfony\Component\HttpKernel\Exception\BadRequestHttpException; @@ -43,9 +44,14 @@ final class UserRbacProcessor implements ProcessorInterface public function process(mixed $data, Operation $operation, array $uriVariables = [], array $context = []): mixed { if (!$data instanceof User) { - // Securite : si le provider n'a pas fourni un User, on delegue - // quand meme pour ne pas etouffer un bug de configuration. - return $this->persistProcessor->process($data, $operation, $uriVariables, $context); + // Ce processor est wire exclusivement sur l'operation user_rbac_patch + // qui cible User. Si on arrive ici avec autre chose, c'est une + // misconfiguration qu'il faut faire remonter fort. + throw new LogicException(sprintf( + 'UserRbacProcessor attend une instance de %s, %s recu.', + User::class, + get_debug_type($data), + )); } $currentUser = $this->security->getUser(); diff --git a/tests/Module/Core/Infrastructure/ApiPlatform/State/Processor/RoleProcessorTest.php b/tests/Module/Core/Infrastructure/ApiPlatform/State/Processor/RoleProcessorTest.php index c90fe5a..d124e91 100644 --- a/tests/Module/Core/Infrastructure/ApiPlatform/State/Processor/RoleProcessorTest.php +++ b/tests/Module/Core/Infrastructure/ApiPlatform/State/Processor/RoleProcessorTest.php @@ -12,10 +12,12 @@ use App\Module\Core\Domain\Entity\Role; use App\Module\Core\Infrastructure\ApiPlatform\State\Processor\RoleProcessor; use Doctrine\ORM\EntityManagerInterface; use Doctrine\ORM\UnitOfWork; +use LogicException; use PHPUnit\Framework\Attributes\AllowMockObjectsWithoutExpectations; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use ReflectionClass; +use stdClass; use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException; use Symfony\Component\HttpKernel\Exception\BadRequestHttpException; @@ -199,6 +201,19 @@ final class RoleProcessorTest extends TestCase self::assertSame($role, $result); } + public function testProcessNonRoleDataThrowsLogicException(): void + { + // Garde-fou contre une misconfiguration : ce processor est wire + // exclusivement sur les operations Role. + $this->persistProcessor->expects(self::never())->method('process'); + $this->removeProcessor->expects(self::never())->method('process'); + + $this->expectException(LogicException::class); + $this->expectExceptionMessage('RoleProcessor attend une instance de'); + + $this->processor->process(new stdClass(), new Patch()); + } + /** * Positionne l'id d'un Role via reflection pour simuler une entite deja * persistee (les mocks d'UnitOfWork n'alimentent pas l'id tout seul). diff --git a/tests/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessorTest.php b/tests/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessorTest.php index 6dce00b..e30dbcb 100644 --- a/tests/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessorTest.php +++ b/tests/Module/Core/Infrastructure/ApiPlatform/State/Processor/UserRbacProcessorTest.php @@ -12,10 +12,12 @@ use App\Module\Core\Domain\Entity\User; use App\Module\Core\Infrastructure\ApiPlatform\State\Processor\UserRbacProcessor; use Doctrine\ORM\EntityManagerInterface; use Doctrine\ORM\UnitOfWork; +use LogicException; use PHPUnit\Framework\Attributes\AllowMockObjectsWithoutExpectations; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use ReflectionClass; +use stdClass; use Symfony\Bundle\SecurityBundle\Security; use Symfony\Component\HttpKernel\Exception\BadRequestHttpException; @@ -196,6 +198,18 @@ final class UserRbacProcessorTest extends TestCase self::assertSame($self, $result); } + public function testProcessNonUserDataThrowsLogicException(): void + { + // Garde-fou contre une misconfiguration : ce processor est wire + // exclusivement sur l'operation user_rbac_patch (cible User). + $this->persistProcessor->expects(self::never())->method('process'); + + $this->expectException(LogicException::class); + $this->expectExceptionMessage('UserRbacProcessor attend une instance de'); + + $this->processor->process(new stdClass(), new Patch()); + } + /** * Construit un User avec un id force via reflection (les mocks * d'UnitOfWork n'alimentent pas l'id tout seul).