From 6ada08d365e0c0f1b405f1f9763bc1f754033d11 Mon Sep 17 00:00:00 2001 From: Nozarashi <15169250+nozarashi20@users.noreply.github.com> Date: Mon, 24 Aug 2026 16:12:24 +0200 Subject: [PATCH 1/2] feat(symfony): expose voter reasons --- .../Bundle/Resources/config/security.php | 3 +- .../Resources/config/state/security.php | 9 +- .../config/state/security_validator.php | 3 +- ...sDecisionCapturingAuthorizationChecker.php | 47 ++++ .../AccessDeniedMessageProviderInterface.php | 22 ++ .../Exception/AccessDeniedException.php | 37 ++- .../Security/ResourceAccessChecker.php | 32 ++- .../Security/State/AccessCheckerProvider.php | 17 +- .../ApiPlatformExtensionTest.php | 15 ++ .../Exception/AccessDeniedExceptionTest.php | 26 ++ tests/Functional/IsGrantedTest.php | 1 + ...isionCapturingAuthorizationCheckerTest.php | 242 ++++++++++++++++++ .../Security/ResourceAccessCheckerTest.php | 204 ++++++++++++++- .../State/AccessCheckerProviderTest.php | 111 +++++++- 14 files changed, 749 insertions(+), 20 deletions(-) create mode 100644 src/Symfony/Security/AccessDecisionCapturingAuthorizationChecker.php create mode 100644 src/Symfony/Security/AccessDeniedMessageProviderInterface.php create mode 100644 tests/Symfony/Security/AccessDecisionCapturingAuthorizationCheckerTest.php diff --git a/src/Symfony/Bundle/Resources/config/security.php b/src/Symfony/Bundle/Resources/config/security.php index 0976d4b6a10..2f3e0c4fdde 100644 --- a/src/Symfony/Bundle/Resources/config/security.php +++ b/src/Symfony/Bundle/Resources/config/security.php @@ -29,7 +29,8 @@ service('security.role_hierarchy')->nullOnInvalid(), service('security.token_storage')->nullOnInvalid(), service('security.authorization_checker')->nullOnInvalid(), - ]); + ]) + ->tag('kernel.reset', ['method' => 'reset']); $services->alias(ResourceAccessCheckerInterface::class, 'api_platform.security.resource_access_checker'); diff --git a/src/Symfony/Bundle/Resources/config/state/security.php b/src/Symfony/Bundle/Resources/config/state/security.php index ee24ded92e8..2a31a863eb7 100644 --- a/src/Symfony/Bundle/Resources/config/state/security.php +++ b/src/Symfony/Bundle/Resources/config/state/security.php @@ -24,7 +24,8 @@ ->args([ service('api_platform.state_provider.access_checker.inner'), service('api_platform.security.resource_access_checker'), - ]); + ]) + ->arg('$debug', '%kernel.debug%'); $services->set('api_platform.state_provider.access_checker.post_deserialize', AccessCheckerProvider::class) ->decorate('api_platform.state_provider.deserialize', null, 0) @@ -32,7 +33,8 @@ service('api_platform.state_provider.access_checker.post_deserialize.inner'), service('api_platform.security.resource_access_checker'), 'post_denormalize', - ]); + ]) + ->arg('$debug', '%kernel.debug%'); $services->set('api_platform.state_provider.security_parameter', SecurityParameterProvider::class) ->decorate('api_platform.state_provider.access_checker', null, 0) @@ -47,5 +49,6 @@ service('api_platform.state_provider.access_checker.pre_read.inner'), service('api_platform.security.resource_access_checker'), 'pre_read', - ]); + ]) + ->arg('$debug', '%kernel.debug%'); }; diff --git a/src/Symfony/Bundle/Resources/config/state/security_validator.php b/src/Symfony/Bundle/Resources/config/state/security_validator.php index 1b515348651..91d3a397978 100644 --- a/src/Symfony/Bundle/Resources/config/state/security_validator.php +++ b/src/Symfony/Bundle/Resources/config/state/security_validator.php @@ -24,5 +24,6 @@ service('api_platform.state_provider.access_checker.post_validate.inner'), service('api_platform.security.resource_access_checker'), 'post_validate', - ]); + ]) + ->arg('$debug', '%kernel.debug%'); }; diff --git a/src/Symfony/Security/AccessDecisionCapturingAuthorizationChecker.php b/src/Symfony/Security/AccessDecisionCapturingAuthorizationChecker.php new file mode 100644 index 00000000000..f5b2a5a80a1 --- /dev/null +++ b/src/Symfony/Security/AccessDecisionCapturingAuthorizationChecker.php @@ -0,0 +1,47 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace ApiPlatform\Symfony\Security; + +use Symfony\Component\Security\Core\Authorization\AccessDecision; +use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; + +/** + * @internal + */ +final class AccessDecisionCapturingAuthorizationChecker implements AuthorizationCheckerInterface +{ + private ?AccessDecision $accessDecision = null; + + public function __construct(private readonly AuthorizationCheckerInterface $decorated) + { + } + + public function isGranted(mixed $attribute, mixed $subject = null, ?AccessDecision $accessDecision = null): bool + { + $accessDecision ??= new AccessDecision(); + $accessDecision->isGranted = $this->decorated->isGranted($attribute, $subject, $accessDecision); + $this->accessDecision = $accessDecision; + + return $accessDecision->isGranted; + } + + public function getAccessDeniedMessage(): ?string + { + if (null === $this->accessDecision || $this->accessDecision->isGranted) { + return null; + } + + return $this->accessDecision->getMessage(); + } +} diff --git a/src/Symfony/Security/AccessDeniedMessageProviderInterface.php b/src/Symfony/Security/AccessDeniedMessageProviderInterface.php new file mode 100644 index 00000000000..4fd0bf4dc5e --- /dev/null +++ b/src/Symfony/Security/AccessDeniedMessageProviderInterface.php @@ -0,0 +1,22 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace ApiPlatform\Symfony\Security; + +/** + * Exposes the applicable denial message from the latest completed access check. + */ +interface AccessDeniedMessageProviderInterface +{ + public function getAccessDeniedMessage(): ?string; +} diff --git a/src/Symfony/Security/Exception/AccessDeniedException.php b/src/Symfony/Security/Exception/AccessDeniedException.php index 88349e501f2..e2383d15f45 100644 --- a/src/Symfony/Security/Exception/AccessDeniedException.php +++ b/src/Symfony/Security/Exception/AccessDeniedException.php @@ -15,15 +15,21 @@ use ApiPlatform\Metadata\Exception\AccessDeniedException as MetadataAccessDeniedException; use ApiPlatform\Metadata\Exception\HttpExceptionInterface; +use ApiPlatform\Metadata\Exception\ProblemExceptionInterface; use Symfony\Component\Security\Core\Exception\AccessDeniedException as ExceptionAccessDeniedException; /** * @deprecated since API Platform 4.4, use {@see MetadataAccessDeniedException} instead */ -final class AccessDeniedException extends ExceptionAccessDeniedException implements HttpExceptionInterface +final class AccessDeniedException extends ExceptionAccessDeniedException implements HttpExceptionInterface, ProblemExceptionInterface { - public function __construct(string $message = 'Access Denied.', ?\Throwable $previous = null, int $code = 403, bool $triggerDeprecation = true) - { + public function __construct( + string $message = 'Access Denied.', + ?\Throwable $previous = null, + int $code = 403, + bool $triggerDeprecation = true, + private readonly ?string $detail = null, + ) { if ($triggerDeprecation) { trigger_deprecation('api-platform/core', '4.4', 'The "%s" class is deprecated, use "%s" instead.', self::class, MetadataAccessDeniedException::class); } @@ -31,6 +37,31 @@ public function __construct(string $message = 'Access Denied.', ?\Throwable $pre parent::__construct($message, $previous, $code); } + public function getType(): string + { + return '/errors/403'; + } + + public function getTitle(): string + { + return 'An error occurred'; + } + + public function getStatus(): int + { + return 403; + } + + public function getDetail(): string + { + return $this->detail ?? $this->getMessage(); + } + + public function getInstance(): ?string + { + return null; + } + public function getStatusCode(): int { return 403; diff --git a/src/Symfony/Security/ResourceAccessChecker.php b/src/Symfony/Security/ResourceAccessChecker.php index 657245fd693..0be7c1e5529 100644 --- a/src/Symfony/Security/ResourceAccessChecker.php +++ b/src/Symfony/Security/ResourceAccessChecker.php @@ -24,20 +24,25 @@ use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; use Symfony\Component\Security\Core\Role\RoleHierarchyInterface; +use Symfony\Contracts\Service\ResetInterface; /** * Checks if the logged user has sufficient permissions to access the given resource. * * @author Kévin Dunglas */ -final class ResourceAccessChecker implements ResourceAccessCheckerInterface, ObjectVariableCheckerInterface +final class ResourceAccessChecker implements ResourceAccessCheckerInterface, ObjectVariableCheckerInterface, AccessDeniedMessageProviderInterface, ResetInterface { + private ?string $accessDeniedMessage = null; + public function __construct(private readonly ?ExpressionLanguage $expressionLanguage = null, private readonly ?AuthenticationTrustResolverInterface $authenticationTrustResolver = null, private readonly ?RoleHierarchyInterface $roleHierarchy = null, private readonly ?TokenStorageInterface $tokenStorage = null, private readonly ?AuthorizationCheckerInterface $authorizationChecker = null) { } public function isGranted(string $resourceClass, string $expression, array $extraVariables = []): bool { + $this->reset(); + if (null === $this->tokenStorage || null === $this->authenticationTrustResolver) { throw new \LogicException('The "symfony/security" library must be installed to use the "security" attribute.'); } @@ -46,7 +51,24 @@ public function isGranted(string $resourceClass, string $expression, array $extr throw new \LogicException('The "symfony/expression-language" library must be installed to use the "security" attribute.'); } - return (bool) $this->expressionLanguage->evaluate($expression, $this->getVariables($extraVariables)); + $authorizationChecker = null === $this->authorizationChecker ? null : new AccessDecisionCapturingAuthorizationChecker($this->authorizationChecker); + $granted = (bool) $this->expressionLanguage->evaluate($expression, $this->getVariables($extraVariables, $authorizationChecker)); + + if (!$granted && null !== $authorizationChecker) { + $this->accessDeniedMessage = $authorizationChecker->getAccessDeniedMessage(); + } + + return $granted; + } + + public function getAccessDeniedMessage(): ?string + { + return $this->accessDeniedMessage; + } + + public function reset(): void + { + $this->accessDeniedMessage = null; } public function usesObjectVariable(string $expression, array $variables = []): bool @@ -59,7 +81,7 @@ public function usesObjectVariable(string $expression, array $variables = []): b throw new RuntimeException('The "symfony/expression-language" library must be installed to use the "security" attribute.'); } - return $this->hasObjectVariable($this->expressionLanguage->parse($expression, array_keys($this->getVariables($variables)))->getNodes()->toArray()); + return $this->hasObjectVariable($this->expressionLanguage->parse($expression, array_keys($this->getVariables($variables, $this->authorizationChecker)))->getNodes()->toArray()); } /** @@ -67,7 +89,7 @@ public function usesObjectVariable(string $expression, array $variables = []): b * * @see https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Security/Core/Authorization/Voter/ExpressionVoter.php */ - private function getVariables(array $variables): array + private function getVariables(array $variables, ?AuthorizationCheckerInterface $authorizationChecker): array { if (null === $token = $this->tokenStorage->getToken()) { $token = new NullToken(); @@ -78,7 +100,7 @@ private function getVariables(array $variables): array 'user' => $token->getUser(), 'roles' => $this->getEffectiveRoles($token), 'trust_resolver' => $this->authenticationTrustResolver, - 'auth_checker' => $this->authorizationChecker, // needed for the is_granted expression function + 'auth_checker' => $authorizationChecker, // needed for the is_granted expression function ]); } diff --git a/src/Symfony/Security/State/AccessCheckerProvider.php b/src/Symfony/Security/State/AccessCheckerProvider.php index fa3509767b7..e864b53beb7 100644 --- a/src/Symfony/Security/State/AccessCheckerProvider.php +++ b/src/Symfony/Security/State/AccessCheckerProvider.php @@ -20,6 +20,7 @@ use ApiPlatform\Metadata\Operation; use ApiPlatform\Metadata\ResourceAccessCheckerInterface; use ApiPlatform\State\ProviderInterface; +use ApiPlatform\Symfony\Security\AccessDeniedMessageProviderInterface; use ApiPlatform\Symfony\Security\Exception\AccessDeniedException; use ApiPlatform\Symfony\Security\ObjectVariableCheckerInterface; use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException; @@ -32,7 +33,7 @@ */ final class AccessCheckerProvider implements ProviderInterface { - public function __construct(private readonly ProviderInterface $decorated, private readonly ResourceAccessCheckerInterface $resourceAccessChecker, private readonly ?string $event = null) + public function __construct(private readonly ProviderInterface $decorated, private readonly ResourceAccessCheckerInterface $resourceAccessChecker, private readonly ?string $event = null, private readonly bool $debug = false) { } @@ -98,7 +99,19 @@ public function provide(Operation $operation, array $uriVariables = [], array $c } if (!$this->resourceAccessChecker->isGranted($operation->getClass(), $isGranted, $resourceAccessCheckerContext)) { - $operation instanceof GraphQlOperation ? throw new AccessDeniedHttpException($message ?? 'Access Denied.') : throw new AccessDeniedException($message ?? 'Access Denied.', null, 403, false); + if ($operation instanceof GraphQlOperation) { + throw new AccessDeniedHttpException($message ?? 'Access Denied.'); + } + + $voterMessage = null; + if (null === $message && $this->resourceAccessChecker instanceof AccessDeniedMessageProviderInterface) { + $voterMessage = $this->resourceAccessChecker->getAccessDeniedMessage(); + } + + $publicDetail = $message ?? ($this->debug ? $voterMessage : null) ?? 'Access Denied.'; + $message ??= $voterMessage ?? 'Access Denied.'; + + throw new AccessDeniedException($message, triggerDeprecation: false, detail: $publicDetail); } return 'pre_read' === $this->event ? $this->decorated->provide($operation, $uriVariables, $context) : $body; diff --git a/src/Symfony/Tests/Bundle/DependencyInjection/ApiPlatformExtensionTest.php b/src/Symfony/Tests/Bundle/DependencyInjection/ApiPlatformExtensionTest.php index 689b17faa70..6d44fc8acc0 100644 --- a/src/Symfony/Tests/Bundle/DependencyInjection/ApiPlatformExtensionTest.php +++ b/src/Symfony/Tests/Bundle/DependencyInjection/ApiPlatformExtensionTest.php @@ -253,6 +253,7 @@ public function testCommonConfiguration(): void $this->assertServiceHasTags('api_platform.serializer.normalizer.item', ['serializer.normalizer']); $this->assertServiceHasTags('api_platform.serializer_locator', ['container.service_locator']); $this->assertServiceHasTags('api_platform.filter_locator', ['container.service_locator']); + $this->assertServiceHasTags('api_platform.security.resource_access_checker', ['kernel.reset']); // api.xml $this->assertServiceHasTags('api_platform.route_loader', ['routing.loader']); @@ -277,6 +278,20 @@ public function testCommonConfiguration(): void $this->assertTrue($this->container->getParameter('api_platform.enable_head_request_optimization')); } + public function testHttpAccessCheckerProvidersUseKernelDebugToExposeVoterReasons(): void + { + (new ApiPlatformExtension())->load(self::DEFAULT_CONFIG, $this->container); + + foreach ([ + 'api_platform.state_provider.access_checker', + 'api_platform.state_provider.access_checker.post_deserialize', + 'api_platform.state_provider.access_checker.post_validate', + 'api_platform.state_provider.access_checker.pre_read', + ] as $serviceId) { + $this->assertSame('%kernel.debug%', $this->container->getDefinition($serviceId)->getArgument('$debug')); + } + } + public function testSwaggerUiDisabledConfiguration(): void { $config = self::DEFAULT_CONFIG; diff --git a/src/Symfony/Tests/Security/Exception/AccessDeniedExceptionTest.php b/src/Symfony/Tests/Security/Exception/AccessDeniedExceptionTest.php index f10d0fa3c85..3e423bcbd67 100644 --- a/src/Symfony/Tests/Security/Exception/AccessDeniedExceptionTest.php +++ b/src/Symfony/Tests/Security/Exception/AccessDeniedExceptionTest.php @@ -13,6 +13,8 @@ namespace ApiPlatform\Tests\Symfony\Security\Exception; +use ApiPlatform\Metadata\Exception\ProblemExceptionInterface; +use ApiPlatform\State\ApiResource\Error; use ApiPlatform\Symfony\Security\Exception\AccessDeniedException; use PHPUnit\Framework\Attributes\IgnoreDeprecations; use PHPUnit\Framework\TestCase; @@ -38,4 +40,28 @@ public function testKeepsBaseExceptionBehavior(): void $this->assertSame(403, $exception->getStatusCode()); $this->assertSame([], $exception->getHeaders()); } + + public function testExposesASeparatePublicProblemDetail(): void + { + $exception = new AccessDeniedException( + 'Access Denied. Voter reason.', + triggerDeprecation: false, + detail: 'Access Denied.', + ); + + $this->assertInstanceOf(ProblemExceptionInterface::class, $exception); + $this->assertSame('Access Denied. Voter reason.', $exception->getMessage()); + $this->assertSame('Access Denied.', $exception->getDetail()); + $this->assertSame('/errors/403', $exception->getType()); + $this->assertSame('An error occurred', $exception->getTitle()); + $this->assertSame(403, $exception->getStatus()); + $this->assertNull($exception->getInstance()); + + $error = Error::createFromException($exception, 403); + + $this->assertSame('Access Denied.', $error->getDetail()); + $this->assertSame('/errors/403', $error->getType()); + $this->assertSame('An error occurred', $error->getTitle()); + $this->assertSame(403, $error->getStatus()); + } } diff --git a/tests/Functional/IsGrantedTest.php b/tests/Functional/IsGrantedTest.php index 72c1cd10412..c8d8b339088 100644 --- a/tests/Functional/IsGrantedTest.php +++ b/tests/Functional/IsGrantedTest.php @@ -48,6 +48,7 @@ public function testGetIsGrantedAsUser(): void $client->request('GET', '/is_granted_tests/1'); $this->assertResponseStatusCodeSame(403); + $this->assertJsonContains(['detail' => "Access Denied. The user doesn't have ROLE_ADMIN."]); } public function testGetIsGrantedAsAnonymous(): void diff --git a/tests/Symfony/Security/AccessDecisionCapturingAuthorizationCheckerTest.php b/tests/Symfony/Security/AccessDecisionCapturingAuthorizationCheckerTest.php new file mode 100644 index 00000000000..ff77af28e1a --- /dev/null +++ b/tests/Symfony/Security/AccessDecisionCapturingAuthorizationCheckerTest.php @@ -0,0 +1,242 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace ApiPlatform\Tests\Symfony\Security; + +use ApiPlatform\Symfony\Security\AccessDecisionCapturingAuthorizationChecker; +use PHPUnit\Framework\TestCase; +use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorage; +use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; +use Symfony\Component\Security\Core\Authorization\AccessDecision; +use Symfony\Component\Security\Core\Authorization\AccessDecisionManager; +use Symfony\Component\Security\Core\Authorization\AuthorizationChecker; +use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; +use Symfony\Component\Security\Core\Authorization\Strategy\ConsensusStrategy; +use Symfony\Component\Security\Core\Authorization\Strategy\UnanimousStrategy; +use Symfony\Component\Security\Core\Authorization\Voter\Vote; +use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface; + +final class AccessDecisionCapturingAuthorizationCheckerTest extends TestCase +{ + public function testItPreservesArgumentsAndResult(): void + { + $subject = new \stdClass(); + $decorated = self::createAuthorizationChecker(function (mixed $attribute, mixed $actualSubject, ?AccessDecision $accessDecision) use ($subject): bool { + $this->assertSame('ATTRIBUTE', $attribute); + $this->assertSame($subject, $actualSubject); + $this->assertInstanceOf(AccessDecision::class, $accessDecision); + + return true; + }); + + $checker = new AccessDecisionCapturingAuthorizationChecker($decorated); + + $this->assertTrue($checker->isGranted('ATTRIBUTE', $subject)); + $this->assertNull($checker->getAccessDeniedMessage()); + } + + public function testItCreatesOneFreshDecisionPerInvocation(): void + { + $decisions = []; + $decorated = self::createAuthorizationChecker(static function (mixed $attribute, mixed $subject, ?AccessDecision $accessDecision) use (&$decisions): bool { + $decisions[] = $accessDecision; + + return false; + }); + + $checker = new AccessDecisionCapturingAuthorizationChecker($decorated); + $checker->isGranted('A'); + $checker->isGranted('B'); + + $this->assertCount(2, $decisions); + $this->assertNotSame($decisions[0], $decisions[1]); + } + + public function testItUsesAnExplicitlySuppliedDecision(): void + { + $decision = new AccessDecision(); + $decorated = self::createAuthorizationChecker(function (mixed $attribute, mixed $subject, ?AccessDecision $actualDecision) use ($decision): bool { + $this->assertSame($decision, $actualDecision); + + return false; + }); + + $checker = new AccessDecisionCapturingAuthorizationChecker($decorated); + + $this->assertFalse($checker->isGranted('ATTRIBUTE', null, $decision)); + $this->assertFalse($decision->isGranted); + $this->assertSame('Access Denied.', $checker->getAccessDeniedMessage()); + } + + public function testItUsesSymfonyToFormatReasonsFromMatchingVotes(): void + { + $authorizationChecker = self::createSymfonyAuthorizationChecker([ + self::createVoter(VoterInterface::ACCESS_DENIED, 'First reason.'), + self::createVoter(VoterInterface::ACCESS_GRANTED, 'Granted reason.'), + self::createVoter(VoterInterface::ACCESS_DENIED, 'Second reason.'), + ], new ConsensusStrategy(false, false)); + + $checker = new AccessDecisionCapturingAuthorizationChecker($authorizationChecker); + + $this->assertFalse($checker->isGranted('ATTRIBUTE')); + $this->assertSame('Access Denied. First reason. Second reason.', $checker->getAccessDeniedMessage()); + } + + public function testItSelectsOnlyTheLastIndependentDeniedDecision(): void + { + $decorated = self::createAuthorizationChecker(static function (mixed $attribute, mixed $subject, ?AccessDecision $accessDecision): bool { + $accessDecision->votes[] = self::createVote(VoterInterface::ACCESS_DENIED, $attribute.' reason.'); + + return false; + }); + + $checker = new AccessDecisionCapturingAuthorizationChecker($decorated); + $checker->isGranted('A'); + $checker->isGranted('B'); + + $this->assertSame('Access Denied. B reason.', $checker->getAccessDeniedMessage()); + } + + public function testALaterGrantLeavesNoDeniedMessage(): void + { + $decorated = self::createAuthorizationChecker(static function (mixed $attribute, mixed $subject, ?AccessDecision $accessDecision): bool { + $granted = 'B' === $attribute; + $accessDecision->votes[] = self::createVote($granted ? VoterInterface::ACCESS_GRANTED : VoterInterface::ACCESS_DENIED, $attribute.' reason.'); + + return $granted; + }); + + $checker = new AccessDecisionCapturingAuthorizationChecker($decorated); + $checker->isGranted('A'); + $checker->isGranted('B'); + + $this->assertNull($checker->getAccessDeniedMessage()); + } + + public function testItRecordsTheResultWhenACustomCheckerIgnoresTheDecision(): void + { + $checker = new AccessDecisionCapturingAuthorizationChecker(self::createAuthorizationChecker(static fn (): bool => false)); + + $this->assertFalse($checker->isGranted('ATTRIBUTE')); + $this->assertSame('Access Denied.', $checker->getAccessDeniedMessage()); + } + + public function testNestedSymfonyAuthorizationUsesTheTopLevelDecision(): void + { + $outerVoter = new class implements VoterInterface { + private AuthorizationCheckerInterface $authorizationChecker; + + public function setAuthorizationChecker(AuthorizationCheckerInterface $authorizationChecker): void + { + $this->authorizationChecker = $authorizationChecker; + } + + public function vote(TokenInterface $token, mixed $subject, array $attributes, ?Vote $vote = null): int + { + if ('OUTER' !== $attributes[0]) { + return self::ACCESS_ABSTAIN; + } + + $this->authorizationChecker->isGranted('INNER'); + $vote?->addReason('Outer reason.'); + + return self::ACCESS_DENIED; + } + }; + $innerVoter = self::createAttributeVoter([ + 'INNER' => [VoterInterface::ACCESS_DENIED, 'Inner reason.'], + ]); + $authorizationChecker = self::createSymfonyAuthorizationChecker([$outerVoter, $innerVoter], new UnanimousStrategy()); + $outerVoter->setAuthorizationChecker($authorizationChecker); + + $checker = new AccessDecisionCapturingAuthorizationChecker($authorizationChecker); + + $this->assertFalse($checker->isGranted('OUTER')); + $this->assertSame('Access Denied. Inner reason. Outer reason.', $checker->getAccessDeniedMessage()); + } + + private static function createAuthorizationChecker(\Closure $callback): AuthorizationCheckerInterface + { + return new class($callback) implements AuthorizationCheckerInterface { + public function __construct(private readonly \Closure $callback) + { + } + + public function isGranted(mixed $attribute, mixed $subject = null, ?AccessDecision $accessDecision = null): bool + { + return ($this->callback)($attribute, $subject, $accessDecision); + } + }; + } + + /** + * @param list $voters + */ + private static function createSymfonyAuthorizationChecker(array $voters, ConsensusStrategy|UnanimousStrategy $strategy): AuthorizationCheckerInterface + { + return new AuthorizationChecker(new TokenStorage(), new AccessDecisionManager($voters, $strategy)); + } + + private static function createVoter(int $result, string $reason): VoterInterface + { + return new class($result, $reason) implements VoterInterface { + public function __construct(private readonly int $result, private readonly string $reason) + { + } + + public function vote(TokenInterface $token, mixed $subject, array $attributes, ?Vote $vote = null): int + { + $vote?->addReason($this->reason); + + return $this->result; + } + }; + } + + /** + * @param array $votes + */ + private static function createAttributeVoter(array $votes): VoterInterface + { + return new class($votes) implements VoterInterface { + /** + * @param array $votes + */ + public function __construct(private readonly array $votes) + { + } + + public function vote(TokenInterface $token, mixed $subject, array $attributes, ?Vote $vote = null): int + { + if (!isset($this->votes[$attributes[0]])) { + return self::ACCESS_ABSTAIN; + } + + [$result, $reason] = $this->votes[$attributes[0]]; + $vote?->addReason($reason); + + return $result; + } + }; + } + + private static function createVote(int $result, string $reason): Vote + { + $vote = new Vote(); + $vote->voter = self::class; + $vote->result = $result; + $vote->addReason($reason); + + return $vote; + } +} diff --git a/tests/Symfony/Security/ResourceAccessCheckerTest.php b/tests/Symfony/Security/ResourceAccessCheckerTest.php index a8aa66f7e4e..18447154a0c 100644 --- a/tests/Symfony/Security/ResourceAccessCheckerTest.php +++ b/tests/Symfony/Security/ResourceAccessCheckerTest.php @@ -21,11 +21,17 @@ use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\PhpUnit\ProphecyTrait; +use Symfony\Component\Security\Core\Authentication\AuthenticationTrustResolver; use Symfony\Component\Security\Core\Authentication\AuthenticationTrustResolverInterface; +use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorage; use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; +use Symfony\Component\Security\Core\Authorization\AccessDecision; use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; use Symfony\Component\Security\Core\Authorization\ExpressionLanguage; +use Symfony\Component\Security\Core\Authorization\Voter\Vote; +use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface; +use Symfony\Contracts\Service\ResetInterface; /** * @author Kévin Dunglas @@ -121,6 +127,202 @@ public function testWithoutAuthenticationToken(): void $tokenStorageProphecy->getToken()->willReturn(null); $checker = new ResourceAccessChecker($expressionLanguageProphecy->reveal(), $authenticationTrustResolverProphecy->reveal(), null, $tokenStorageProphecy->reveal(), $authorizationCheckerProphecy->reveal()); - self::assertTrue($checker->isGranted(Dummy::class, 'is_granted("ROLE_ADMIN")')); + $this->assertTrue($checker->isGranted(Dummy::class, 'is_granted("ROLE_ADMIN")')); + } + + public function testCapturesASingleDeniedAuthorizationMessage(): void + { + $checker = self::createResourceAccessChecker([ + 'A' => [false, [[VoterInterface::ACCESS_DENIED, 'Reason A.']]], + ]); + + $this->assertFalse($checker->isGranted(Dummy::class, "is_granted('A', object)", ['object' => new \stdClass()])); + $this->assertSame('Access Denied. Reason A.', $checker->getAccessDeniedMessage()); + } + + public function testAndShortCircuitsAfterTheFirstDenial(): void + { + $calls = []; + $checker = self::createResourceAccessChecker([ + 'A' => [false, [[VoterInterface::ACCESS_DENIED, 'Reason A.']]], + 'B' => [false, [[VoterInterface::ACCESS_DENIED, 'Reason B.']]], + ], $calls); + + $this->assertFalse($checker->isGranted(Dummy::class, "is_granted('A', object) && is_granted('B', object)", ['object' => new \stdClass()])); + $this->assertSame(['A'], $calls); + $this->assertSame('Access Denied. Reason A.', $checker->getAccessDeniedMessage()); + } + + public function testAndSelectsTheSecondDecisionWhenTheFirstGrants(): void + { + $calls = []; + $checker = self::createResourceAccessChecker([ + 'A' => [true, [ + [VoterInterface::ACCESS_DENIED, 'A minority denial.'], + [VoterInterface::ACCESS_GRANTED, 'A grant.'], + ]], + 'B' => [false, [[VoterInterface::ACCESS_DENIED, 'Reason B.']]], + ], $calls); + + $this->assertFalse($checker->isGranted(Dummy::class, "is_granted('A', object) && is_granted('B', object)", ['object' => new \stdClass()])); + $this->assertSame(['A', 'B'], $calls); + $this->assertSame('Access Denied. Reason B.', $checker->getAccessDeniedMessage()); + } + + public function testOrSelectsTheLastDecisionWhenBothDeny(): void + { + $calls = []; + $checker = self::createResourceAccessChecker([ + 'A' => [false, [[VoterInterface::ACCESS_DENIED, 'Reason A.']]], + 'B' => [false, [[VoterInterface::ACCESS_DENIED, 'Reason B.']]], + ], $calls); + + $this->assertFalse($checker->isGranted(Dummy::class, "is_granted('A', object) || is_granted('B', object)", ['object' => new \stdClass()])); + $this->assertSame(['A', 'B'], $calls); + $this->assertSame('Access Denied. Reason B.', $checker->getAccessDeniedMessage()); + } + + public function testNegatedGrantExposesNoDeniedMessage(): void + { + $checker = self::createResourceAccessChecker([ + 'A' => [true, [[VoterInterface::ACCESS_GRANTED, 'Reason A.']]], + ]); + + $this->assertFalse($checker->isGranted(Dummy::class, "!is_granted('A', object)", ['object' => new \stdClass()])); + $this->assertNull($checker->getAccessDeniedMessage()); + } + + public function testNonAuthorizationConditionAfterAGrantExposesNoDeniedMessage(): void + { + $checker = self::createResourceAccessChecker([ + 'A' => [true, [[VoterInterface::ACCESS_GRANTED, 'Reason A.']]], + ]); + $object = new class { + public bool $enabled = false; + }; + + $this->assertFalse($checker->isGranted(Dummy::class, "is_granted('A', object) && object.enabled", ['object' => $object])); + $this->assertNull($checker->getAccessDeniedMessage()); + } + + public function testAuthorizationDenialBeforeAnObjectConditionExposesItsMessage(): void + { + $calls = []; + $checker = self::createResourceAccessChecker([ + 'A' => [false, [[VoterInterface::ACCESS_DENIED, 'Reason A.']]], + ], $calls); + $object = new class { + public bool $enabled = false; + }; + + $this->assertFalse($checker->isGranted(Dummy::class, "is_granted('A', object) && object.enabled", ['object' => $object])); + $this->assertSame(['A'], $calls); + $this->assertSame('Access Denied. Reason A.', $checker->getAccessDeniedMessage()); + } + + public function testPureNonAuthorizationDenialExposesNoDeniedMessage(): void + { + $calls = []; + $checker = self::createResourceAccessChecker([], $calls); + $object = new class { + public bool $enabled = false; + }; + + $this->assertFalse($checker->isGranted(Dummy::class, 'object.enabled', ['object' => $object])); + $this->assertSame([], $calls); + $this->assertNull($checker->getAccessDeniedMessage()); + } + + public function testDeniedDecisionWithoutReasonExposesTheGenericMessage(): void + { + $checker = self::createResourceAccessChecker([ + 'A' => [false, []], + ]); + + $this->assertFalse($checker->isGranted(Dummy::class, "is_granted('A')")); + $this->assertSame('Access Denied.', $checker->getAccessDeniedMessage()); + } + + public function testCapturedMessageIsResetBetweenEvaluations(): void + { + $checker = self::createResourceAccessChecker([ + 'A' => [false, [[VoterInterface::ACCESS_DENIED, 'Reason A.']]], + ]); + $object = new class { + public bool $enabled = false; + }; + + $this->assertFalse($checker->isGranted(Dummy::class, "is_granted('A')")); + $this->assertSame('Access Denied. Reason A.', $checker->getAccessDeniedMessage()); + + $this->assertFalse($checker->isGranted(Dummy::class, 'object.enabled', ['object' => $object])); + $this->assertNull($checker->getAccessDeniedMessage()); + + $this->assertTrue($checker->isGranted(Dummy::class, 'true')); + $this->assertNull($checker->getAccessDeniedMessage()); + } + + public function testResetClearsTheCapturedMessage(): void + { + $checker = self::createResourceAccessChecker([ + 'A' => [false, [[VoterInterface::ACCESS_DENIED, 'Reason A.']]], + ]); + + $this->assertInstanceOf(ResetInterface::class, $checker); + $this->assertFalse($checker->isGranted(Dummy::class, "is_granted('A')")); + $this->assertSame('Access Denied. Reason A.', $checker->getAccessDeniedMessage()); + + $checker->reset(); + + $this->assertNull($checker->getAccessDeniedMessage()); + } + + /** + * @param array}> $decisions + * @param list $calls + */ + private static function createResourceAccessChecker(array $decisions, array &$calls = []): ResourceAccessChecker + { + $authorizationChecker = self::createAuthorizationChecker(static function (mixed $attribute, mixed $subject, ?AccessDecision $accessDecision) use ($decisions, &$calls): bool { + $calls[] = $attribute; + [$granted, $votes] = $decisions[$attribute]; + + foreach ($votes as [$result, $reason]) { + $accessDecision->votes[] = self::createVote($result, $reason); + } + + return $granted; + }); + + return self::createResourceAccessCheckerWithAuthorizationChecker($authorizationChecker); + } + + private static function createResourceAccessCheckerWithAuthorizationChecker(?AuthorizationCheckerInterface $authorizationChecker): ResourceAccessChecker + { + return new ResourceAccessChecker(new ExpressionLanguage(), new AuthenticationTrustResolver(), null, new TokenStorage(), $authorizationChecker); + } + + private static function createAuthorizationChecker(\Closure $callback): AuthorizationCheckerInterface + { + return new class($callback) implements AuthorizationCheckerInterface { + public function __construct(private readonly \Closure $callback) + { + } + + public function isGranted(mixed $attribute, mixed $subject = null, ?AccessDecision $accessDecision = null): bool + { + return ($this->callback)($attribute, $subject, $accessDecision); + } + }; + } + + private static function createVote(int $result, string $reason): Vote + { + $vote = new Vote(); + $vote->voter = self::class; + $vote->result = $result; + $vote->addReason($reason); + + return $vote; } } diff --git a/tests/Symfony/Security/State/AccessCheckerProviderTest.php b/tests/Symfony/Security/State/AccessCheckerProviderTest.php index 39f1ce2c505..da8be7ef6b6 100644 --- a/tests/Symfony/Security/State/AccessCheckerProviderTest.php +++ b/tests/Symfony/Security/State/AccessCheckerProviderTest.php @@ -17,6 +17,7 @@ use ApiPlatform\Metadata\GraphQl\Query; use ApiPlatform\Metadata\ResourceAccessCheckerInterface; use ApiPlatform\State\ProviderInterface; +use ApiPlatform\Symfony\Security\AccessDeniedMessageProviderInterface; use ApiPlatform\Symfony\Security\Exception\AccessDeniedException; use ApiPlatform\Symfony\Security\ObjectVariableCheckerInterface; use ApiPlatform\Symfony\Security\State\AccessCheckerProvider; @@ -126,9 +127,6 @@ public function testPreReadSkipsSecurityWhenObjectVariableIsUsed(): void public function testCheckAccessDenied(): void { - $this->expectException(AccessDeniedException::class); - $this->expectExceptionMessage('hello'); - $obj = new \stdClass(); $operation = new Get(class: DummyEntity::class, security: 'hi', securityMessage: 'hello'); $decorated = $this->createMock(ProviderInterface::class); @@ -136,7 +134,14 @@ public function testCheckAccessDenied(): void $resourceAccessChecker = $this->createMock(ResourceAccessCheckerInterface::class); $resourceAccessChecker->expects($this->once())->method('isGranted')->with(DummyEntity::class, 'hi', ['object' => $obj, 'previous_object' => null, 'request' => null])->willReturn(false); $accessChecker = new AccessCheckerProvider($decorated, $resourceAccessChecker); - $accessChecker->provide($operation, [], []); + + try { + $accessChecker->provide($operation, [], []); + $this->fail('An access denied exception should have been thrown.'); + } catch (AccessDeniedException $exception) { + $this->assertSame('hello', $exception->getMessage()); + $this->assertSame('hello', $exception->getDetail()); + } } public function testCheckAccessDeniedWithGraphQl(): void @@ -153,8 +158,106 @@ public function testCheckAccessDeniedWithGraphQl(): void $accessChecker = new AccessCheckerProvider($decorated, $resourceAccessChecker); $accessChecker->provide($operation, [], []); } + + public function testPropagatesCapturedAccessDeniedMessage(): void + { + $obj = new \stdClass(); + $operation = new Get(class: DummyEntity::class, security: 'hi'); + $decorated = $this->createStub(ProviderInterface::class); + $decorated->method('provide')->willReturn($obj); + $resourceAccessChecker = $this->createMock(ResourceAccessCheckerWithDeniedMessageInterface::class); + $resourceAccessChecker->expects($this->once())->method('isGranted')->willReturn(false); + $resourceAccessChecker->expects($this->once())->method('getAccessDeniedMessage')->willReturn('Access Denied. Voter reason.'); + $accessChecker = new AccessCheckerProvider($decorated, $resourceAccessChecker, debug: true); + + try { + $accessChecker->provide($operation, [], []); + $this->fail('An access denied exception should have been thrown.'); + } catch (AccessDeniedException $exception) { + $this->assertSame('Access Denied. Voter reason.', $exception->getMessage()); + $this->assertSame('Access Denied. Voter reason.', $exception->getDetail()); + } + } + + public function testKeepsCapturedAccessDeniedMessageInternalWhenDebugIsDisabled(): void + { + $obj = new \stdClass(); + $operation = new Get(class: DummyEntity::class, security: 'hi'); + $decorated = $this->createStub(ProviderInterface::class); + $decorated->method('provide')->willReturn($obj); + $resourceAccessChecker = $this->createMock(ResourceAccessCheckerWithDeniedMessageInterface::class); + $resourceAccessChecker->expects($this->once())->method('isGranted')->willReturn(false); + $resourceAccessChecker->expects($this->once())->method('getAccessDeniedMessage')->willReturn('Access Denied. Voter reason.'); + $accessChecker = new AccessCheckerProvider($decorated, $resourceAccessChecker); + + try { + $accessChecker->provide($operation, [], []); + $this->fail('An access denied exception should have been thrown.'); + } catch (AccessDeniedException $exception) { + $this->assertSame('Access Denied. Voter reason.', $exception->getMessage()); + $this->assertSame('Access Denied.', $exception->getDetail()); + } + } + + public function testConfiguredEmptyMessageTakesPrecedence(): void + { + $obj = new \stdClass(); + $operation = new Get(class: DummyEntity::class, security: 'hi', securityMessage: ''); + $decorated = $this->createStub(ProviderInterface::class); + $decorated->method('provide')->willReturn($obj); + $resourceAccessChecker = $this->createMock(ResourceAccessCheckerWithDeniedMessageInterface::class); + $resourceAccessChecker->expects($this->once())->method('isGranted')->willReturn(false); + $resourceAccessChecker->expects($this->never())->method('getAccessDeniedMessage'); + $accessChecker = new AccessCheckerProvider($decorated, $resourceAccessChecker, debug: true); + + try { + $accessChecker->provide($operation, [], []); + $this->fail('An access denied exception should have been thrown.'); + } catch (AccessDeniedException $exception) { + $this->assertSame('', $exception->getMessage()); + } + } + + public function testFallsBackToGenericMessageWhenCapturedMessageIsNull(): void + { + $operation = new Get(class: DummyEntity::class, security: 'hi'); + $decorated = $this->createStub(ProviderInterface::class); + $decorated->method('provide')->willReturn(new \stdClass()); + $resourceAccessChecker = $this->createMock(ResourceAccessCheckerWithDeniedMessageInterface::class); + $resourceAccessChecker->expects($this->once())->method('isGranted')->willReturn(false); + $resourceAccessChecker->expects($this->once())->method('getAccessDeniedMessage')->willReturn(null); + $accessChecker = new AccessCheckerProvider($decorated, $resourceAccessChecker, debug: true); + + try { + $accessChecker->provide($operation, [], []); + $this->fail('An access denied exception should have been thrown.'); + } catch (AccessDeniedException $exception) { + $this->assertSame('Access Denied.', $exception->getMessage()); + } + } + + public function testPlainCustomResourceAccessCheckerKeepsGenericFallback(): void + { + $operation = new Get(class: DummyEntity::class, security: 'hi'); + $decorated = $this->createStub(ProviderInterface::class); + $decorated->method('provide')->willReturn(new \stdClass()); + $resourceAccessChecker = $this->createMock(ResourceAccessCheckerInterface::class); + $resourceAccessChecker->expects($this->once())->method('isGranted')->willReturn(false); + $accessChecker = new AccessCheckerProvider($decorated, $resourceAccessChecker, debug: true); + + try { + $accessChecker->provide($operation, [], []); + $this->fail('An access denied exception should have been thrown.'); + } catch (AccessDeniedException $exception) { + $this->assertSame('Access Denied.', $exception->getMessage()); + } + } } interface ResourceAccessCheckerWithObjectVariableInterface extends ResourceAccessCheckerInterface, ObjectVariableCheckerInterface { } + +interface ResourceAccessCheckerWithDeniedMessageInterface extends ResourceAccessCheckerInterface, AccessDeniedMessageProviderInterface +{ +} From 36ac4623bcdd6a0c2a9fc77370ba2da92c17137a Mon Sep 17 00:00:00 2001 From: Nozarashi <15169250+nozarashi20@users.noreply.github.com> Date: Fri, 4 Sep 2026 20:51:42 +0200 Subject: [PATCH 2/2] refactor(symfony): propagate voter access decisions --- src/Laravel/ApiResource/Error.php | 4 +- .../Tests/Unit/ApiResource/ErrorTest.php | 56 ++++ src/Mcp/Server/Handler.php | 19 +- .../Exception/AccessDeniedException.php | 32 ++- .../Exception/AccessDeniedExceptionTest.php | 42 +++ src/Serializer/AbstractItemNormalizer.php | 2 +- .../Tests/AbstractItemNormalizerTest.php | 18 +- src/State/ErrorProvider.php | 29 +++ .../Provider/SecurityParameterProvider.php | 7 +- src/State/Tests/ErrorProviderTest.php | 58 +++++ .../SecurityParameterProviderTest.php | 13 +- .../Bundle/Resources/config/security.php | 3 +- .../Resources/config/state/security.php | 9 +- .../config/state/security_validator.php | 3 +- ...ionAwareResourceAccessCheckerInterface.php | 25 ++ ...sDecisionCapturingAuthorizationChecker.php | 47 ---- .../AccessDeniedMessageProviderInterface.php | 22 -- .../ExpressionLanguageProvider.php | 2 +- .../Exception/AccessDeniedException.php | 37 +-- .../Security/ResourceAccessChecker.php | 38 +-- .../Security/State/AccessCheckerProvider.php | 26 +- .../ApiPlatformExtensionTest.php | 15 -- .../ExpressionLanguageProviderTest.php | 73 ++++++ .../Exception/AccessDeniedExceptionTest.php | 26 -- ...isionCapturingAuthorizationCheckerTest.php | 242 ------------------ .../Security/ResourceAccessCheckerTest.php | 113 ++++---- .../State/AccessCheckerProviderTest.php | 102 +++++--- 27 files changed, 539 insertions(+), 524 deletions(-) create mode 100644 src/Laravel/Tests/Unit/ApiResource/ErrorTest.php create mode 100644 src/Metadata/Tests/Exception/AccessDeniedExceptionTest.php create mode 100644 src/Symfony/Security/AccessDecisionAwareResourceAccessCheckerInterface.php delete mode 100644 src/Symfony/Security/AccessDecisionCapturingAuthorizationChecker.php delete mode 100644 src/Symfony/Security/AccessDeniedMessageProviderInterface.php create mode 100644 src/Symfony/Tests/Security/Core/Authorization/ExpressionLanguageProviderTest.php delete mode 100644 tests/Symfony/Security/AccessDecisionCapturingAuthorizationCheckerTest.php diff --git a/src/Laravel/ApiResource/Error.php b/src/Laravel/ApiResource/Error.php index a85c3c35793..c2845a2fcc0 100644 --- a/src/Laravel/ApiResource/Error.php +++ b/src/Laravel/ApiResource/Error.php @@ -100,7 +100,7 @@ class Error extends \Exception implements ProblemExceptionInterface, HttpExcepti */ public function __construct( private readonly string $title, - private readonly string $detail, + private readonly ?string $detail, #[ApiProperty(identifier: true)] private int $status, array $originalTrace = [], private readonly ?string $instance = null, @@ -127,7 +127,7 @@ public function getOriginalTrace(): array } #[SerializedName('description')] - public function getDescription(): string + public function getDescription(): ?string { return $this->detail; } diff --git a/src/Laravel/Tests/Unit/ApiResource/ErrorTest.php b/src/Laravel/Tests/Unit/ApiResource/ErrorTest.php new file mode 100644 index 00000000000..0c91ff142db --- /dev/null +++ b/src/Laravel/Tests/Unit/ApiResource/ErrorTest.php @@ -0,0 +1,56 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace ApiPlatform\Laravel\Tests\Unit\ApiResource; + +use ApiPlatform\Laravel\ApiResource\Error; +use ApiPlatform\Metadata\Exception\ProblemExceptionInterface; +use PHPUnit\Framework\TestCase; + +final class ErrorTest extends TestCase +{ + public function testProblemWithoutDetailDoesNotExposeTheExceptionMessage(): void + { + $exception = new class('Internal message') extends \Exception implements ProblemExceptionInterface { + public function getType(): string + { + return '/errors/400'; + } + + public function getTitle(): string + { + return 'Invalid request'; + } + + public function getStatus(): int + { + return 400; + } + + public function getDetail(): ?string + { + return null; + } + + public function getInstance(): ?string + { + return null; + } + }; + + $error = Error::createFromException($exception, 400); + + $this->assertNull($error->getDetail()); + $this->assertNull($error->getDescription()); + } +} diff --git a/src/Mcp/Server/Handler.php b/src/Mcp/Server/Handler.php index e6ea7dab268..d38d8f13817 100644 --- a/src/Mcp/Server/Handler.php +++ b/src/Mcp/Server/Handler.php @@ -14,6 +14,7 @@ namespace ApiPlatform\Mcp\Server; use ApiPlatform\Mcp\State\ToolProvider; +use ApiPlatform\Metadata\Exception\AccessDeniedException; use ApiPlatform\Metadata\Exception\HttpExceptionInterface; use ApiPlatform\Metadata\HttpOperation; use ApiPlatform\Metadata\Operation\Factory\OperationMetadataFactoryInterface; @@ -139,7 +140,7 @@ public function handle(Request $request, SessionInterface $session): Response|Er try { $body = $this->provider->provide($operation, $uriVariables, $context); } catch (HttpExceptionInterface $e) { - return Error::forInternalError($e->getMessage(), $request->getId()); + return Error::forInternalError($this->getPublicErrorMessage($e), $request->getId()); } if (!$isResource && null !== ($httpRequest = $context['request'] ?? null)) { @@ -160,7 +161,21 @@ public function handle(Request $request, SessionInterface $session): Response|Er try { return $this->processor->process($body, $operation, $uriVariables, $context); } catch (HttpExceptionInterface $e) { - return Error::forInternalError($e->getMessage(), $request->getId()); + return Error::forInternalError($this->getPublicErrorMessage($e), $request->getId()); } } + + private function getPublicErrorMessage(\Throwable $exception): string + { + $current = $exception; + while (null !== $current) { + if ($current instanceof AccessDeniedException) { + return $current->getDetail() ?? 'Access Denied.'; + } + + $current = $current->getPrevious(); + } + + return $exception->getMessage(); + } } diff --git a/src/Metadata/Exception/AccessDeniedException.php b/src/Metadata/Exception/AccessDeniedException.php index 2bbe90fba1c..6d6429e36fa 100644 --- a/src/Metadata/Exception/AccessDeniedException.php +++ b/src/Metadata/Exception/AccessDeniedException.php @@ -15,8 +15,38 @@ use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException; -final class AccessDeniedException extends AccessDeniedHttpException implements HttpExceptionInterface +final class AccessDeniedException extends AccessDeniedHttpException implements HttpExceptionInterface, ProblemExceptionInterface { + public function __construct(string $message = '', ?\Throwable $previous = null, int $code = 0, array $headers = [], private readonly ?string $detail = null) + { + parent::__construct($message, $previous, $code, $headers); + } + + public function getType(): string + { + return '/errors/403'; + } + + public function getTitle(): string + { + return 'An error occurred'; + } + + public function getStatus(): int + { + return 403; + } + + public function getDetail(): ?string + { + return $this->detail; + } + + public function getInstance(): ?string + { + return null; + } + public function getStatusCode(): int { return 403; diff --git a/src/Metadata/Tests/Exception/AccessDeniedExceptionTest.php b/src/Metadata/Tests/Exception/AccessDeniedExceptionTest.php new file mode 100644 index 00000000000..b8b5326fd4b --- /dev/null +++ b/src/Metadata/Tests/Exception/AccessDeniedExceptionTest.php @@ -0,0 +1,42 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace ApiPlatform\Metadata\Tests\Exception; + +use ApiPlatform\Metadata\Exception\AccessDeniedException; +use ApiPlatform\Metadata\Exception\ProblemExceptionInterface; +use PHPUnit\Framework\TestCase; + +final class AccessDeniedExceptionTest extends TestCase +{ + public function testKeepsTheDefaultMessageForBackwardCompatibility(): void + { + $this->assertSame('', (new AccessDeniedException())->getMessage()); + } + + public function testKeepsTheInternalMessageSeparateFromThePublicDetail(): void + { + $exception = new AccessDeniedException('Access Denied. Voter reason.', detail: 'Access Denied.'); + + $this->assertInstanceOf(ProblemExceptionInterface::class, $exception); + $this->assertSame('Access Denied. Voter reason.', $exception->getMessage()); + $this->assertSame('Access Denied.', $exception->getDetail()); + } + + public function testKeepsAMissingPublicDetailNull(): void + { + $exception = new AccessDeniedException('Access Denied. Voter reason.'); + + $this->assertNull($exception->getDetail()); + } +} diff --git a/src/Serializer/AbstractItemNormalizer.php b/src/Serializer/AbstractItemNormalizer.php index 21aa1de0fe0..06fd361a2de 100644 --- a/src/Serializer/AbstractItemNormalizer.php +++ b/src/Serializer/AbstractItemNormalizer.php @@ -329,7 +329,7 @@ public function denormalize(mixed $data, string $type, ?string $format = null, a if (!$this->canAccessAttributePostDenormalize($object, $previousObject, $attribute, $context)) { if ($throwOnPropertyAccessDenied) { - throw new AccessDeniedException($securityMessage ?? 'Access denied'); + throw new AccessDeniedException($securityMessage ?? 'Access denied', detail: $securityMessage); } if (null !== $previousObject) { $this->setValue($object, $attribute, $this->propertyAccessor->getValue($previousObject, $attribute)); diff --git a/src/Serializer/Tests/AbstractItemNormalizerTest.php b/src/Serializer/Tests/AbstractItemNormalizerTest.php index bc17aae5005..3f24bff2a66 100644 --- a/src/Serializer/Tests/AbstractItemNormalizerTest.php +++ b/src/Serializer/Tests/AbstractItemNormalizerTest.php @@ -573,14 +573,20 @@ public function testDenormalizeWithSecuredPropertyAndThrowOnAccessDeniedExtraPro $normalizer = new class($propertyNameCollectionFactoryProphecy->reveal(), $propertyMetadataFactoryProphecy->reveal(), $iriConverterProphecy->reveal(), $resourceClassResolverProphecy->reveal(), $propertyAccessorProphecy->reveal(), null, null, [], null, $resourceAccessChecker->reveal()) extends AbstractItemNormalizer {}; $normalizer->setSerializer($serializerProphecy->reveal()); - $this->expectException(AccessDeniedException::class); - $this->expectExceptionMessage('Custom access denied message'); - $operation = new Patch(securityMessage: 'Custom access denied message', extraProperties: ['throw_on_access_denied' => true]); - $normalizer->denormalize($data, SecuredDummy::class, 'json', [ - 'operation' => $operation, - ]); + $exception = null; + try { + $normalizer->denormalize($data, SecuredDummy::class, 'json', [ + 'operation' => $operation, + ]); + } catch (\Throwable $caughtException) { + $exception = $caughtException; + } + + $this->assertInstanceOf(AccessDeniedException::class, $exception); + $this->assertSame('Custom access denied message', $exception->getMessage()); + $this->assertSame('Custom access denied message', $exception->getDetail()); } public function testDenormalizeWithSecuredPropertyAndThrowOnAccessDeniedExtraPropertyInOperationThrowsAccessDeniedException(): void diff --git a/src/State/ErrorProvider.php b/src/State/ErrorProvider.php index d8e13853d0a..c89ddca3ded 100644 --- a/src/State/ErrorProvider.php +++ b/src/State/ErrorProvider.php @@ -14,6 +14,7 @@ namespace ApiPlatform\State; use ApiPlatform\Metadata\ErrorResourceInterface; +use ApiPlatform\Metadata\Exception\AccessDeniedException; use ApiPlatform\Metadata\HttpOperation; use ApiPlatform\Metadata\Operation; use ApiPlatform\Metadata\Resource\Factory\ResourceMetadataCollectionFactoryInterface; @@ -74,6 +75,20 @@ public function provide(Operation $operation, array $uriVariables = [], array $c $status = $operation->getStatus() ?? 500; $cl = is_a($operation->getClass(), ErrorResourceInterface::class, true) ? $operation->getClass() : Error::class; $error = $cl::createFromException($exception, $status); + if (null !== ($accessDeniedException = $this->findAccessDeniedException($exception))) { + if ($status !== $accessDeniedException->getStatus()) { + if (method_exists($error, 'setStatus')) { + $error->setStatus($status); + } + if (method_exists($error, 'setType')) { + $error->setType("/errors/$status"); + } + } + + if (method_exists($error, 'setDetail')) { + $error->setDetail($accessDeniedException->getDetail() ?? ($this->debug ? $accessDeniedException->getMessage() : 'Access Denied.')); + } + } if (!$this->debug && $status >= 500 && method_exists($error, 'setDetail')) { $error->setDetail('Internal Server Error'); } @@ -94,4 +109,18 @@ private function renderError(int $status, string $text): Response HTML); } + + private function findAccessDeniedException(\Throwable $exception): ?AccessDeniedException + { + $current = $exception; + while (null !== $current) { + if ($current instanceof AccessDeniedException) { + return $current; + } + + $current = $current->getPrevious(); + } + + return null; + } } diff --git a/src/State/Provider/SecurityParameterProvider.php b/src/State/Provider/SecurityParameterProvider.php index 9b84c76d423..784f9faaf1f 100644 --- a/src/State/Provider/SecurityParameterProvider.php +++ b/src/State/Provider/SecurityParameterProvider.php @@ -109,7 +109,12 @@ class_exists(AccessDeniedException::class, true) => AccessDeniedException::class default => AccessDeniedHttpException::class, }; - throw new ($exception)($parameter->getSecurityMessage() ?? 'Access Denied.'); + $securityMessage = $parameter->getSecurityMessage(); + if (MetadataAccessDeniedException::class === $exception) { + throw new $exception($securityMessage ?? 'Access Denied.', detail: $securityMessage); + } + + throw new $exception($securityMessage ?? 'Access Denied.'); } } diff --git a/src/State/Tests/ErrorProviderTest.php b/src/State/Tests/ErrorProviderTest.php index 22095a5cc8f..9762f318c06 100644 --- a/src/State/Tests/ErrorProviderTest.php +++ b/src/State/Tests/ErrorProviderTest.php @@ -13,6 +13,7 @@ namespace ApiPlatform\State\Tests; +use ApiPlatform\Metadata\Exception\AccessDeniedException; use ApiPlatform\Metadata\Get; use ApiPlatform\State\ApiResource\Error; use ApiPlatform\State\ErrorProvider; @@ -44,4 +45,61 @@ public function testErrorProviderProduction(): void $error = $provider->provide(new Get(), [], ['request' => $request]); $this->assertEquals('Internal Server Error', $error->getDetail()); } + + public function testAccessDeniedReasonIsExposedInDebugMode(): void + { + $error = self::provideError(new AccessDeniedException('Access Denied. Voter reason.'), true); + + $this->assertSame('Access Denied. Voter reason.', $error->getDetail()); + } + + public function testAccessDeniedReasonIsHiddenInProduction(): void + { + $error = self::provideError(new AccessDeniedException('Access Denied. Voter reason.'), false); + + $this->assertSame('Access Denied.', $error->getDetail()); + } + + public function testConfiguredAccessDeniedDetailIsPreservedInProduction(): void + { + $error = self::provideError(new AccessDeniedException('Internal message', detail: 'Public message'), false); + + $this->assertSame('Public message', $error->getDetail()); + } + + public function testConfiguredEmptyAccessDeniedDetailIsPreservedInProduction(): void + { + $error = self::provideError(new AccessDeniedException('Internal message', detail: ''), false); + + $this->assertSame('', $error->getDetail()); + } + + public function testUsesTheResolvedErrorOperationStatusForAccessDeniedProblem(): void + { + $error = self::provideError(new AccessDeniedException('Internal message', detail: 'Public message'), false, 404); + + $this->assertSame(404, $error->getStatus()); + $this->assertSame('/errors/404', $error->getType()); + $this->assertSame('Public message', $error->getDetail()); + } + + public function testFindsTheAccessDeniedProblemInTheExceptionChain(): void + { + $problem = new AccessDeniedException('Access Denied. Voter reason.'); + $exception = new \RuntimeException('Wrapper exception', previous: $problem); + $error = self::provideError($exception, false); + + $this->assertSame('Access Denied.', $error->getDetail()); + } + + private static function provideError(\Throwable $exception, bool $debug, int $status = 403): Error + { + $request = Request::create('/'); + $request->attributes->set('exception', $exception); + + $error = (new ErrorProvider(debug: $debug))->provide(new Get(status: $status), [], ['request' => $request]); + self::assertInstanceOf(Error::class, $error); + + return $error; + } } diff --git a/src/State/Tests/Provider/SecurityParameterProviderTest.php b/src/State/Tests/Provider/SecurityParameterProviderTest.php index bccc26be323..d2b9c2a3c15 100644 --- a/src/State/Tests/Provider/SecurityParameterProviderTest.php +++ b/src/State/Tests/Provider/SecurityParameterProviderTest.php @@ -13,6 +13,7 @@ namespace ApiPlatform\State\Tests\Provider; +use ApiPlatform\Metadata\Exception\AccessDeniedException; use ApiPlatform\Metadata\GetCollection; use ApiPlatform\Metadata\Link; use ApiPlatform\Metadata\Parameters; @@ -63,9 +64,6 @@ public function testIsNotGrantedLink(): void public function testSecurityMessageLink(): void { - $this->expectException(AccessDeniedHttpException::class); - $this->expectExceptionMessage('You are not admin.'); - $obj = new \stdClass(); $barObj = new \stdClass(); $operation = new GetCollection(uriVariables: [ @@ -77,6 +75,13 @@ public function testSecurityMessageLink(): void $resourceAccessChecker = $this->createMock(ResourceAccessCheckerInterface::class); $resourceAccessChecker->expects($this->once())->method('isGranted')->with('Bar', 'is_granted("some_voter", "bar")', ['object' => $obj, 'previous_object' => null, 'request' => $request, 'bar' => $barObj, 'barId' => 1, 'operation' => $operation])->willReturn(false); $accessChecker = new SecurityParameterProvider($decorated, $resourceAccessChecker); - $accessChecker->provide($operation, ['barId' => 1], ['request' => $request]); + + try { + $accessChecker->provide($operation, ['barId' => 1], ['request' => $request]); + self::fail('An AccessDeniedException should have been thrown.'); + } catch (AccessDeniedException $exception) { + $this->assertSame('You are not admin.', $exception->getMessage()); + $this->assertSame('You are not admin.', $exception->getDetail()); + } } } diff --git a/src/Symfony/Bundle/Resources/config/security.php b/src/Symfony/Bundle/Resources/config/security.php index 2f3e0c4fdde..0976d4b6a10 100644 --- a/src/Symfony/Bundle/Resources/config/security.php +++ b/src/Symfony/Bundle/Resources/config/security.php @@ -29,8 +29,7 @@ service('security.role_hierarchy')->nullOnInvalid(), service('security.token_storage')->nullOnInvalid(), service('security.authorization_checker')->nullOnInvalid(), - ]) - ->tag('kernel.reset', ['method' => 'reset']); + ]); $services->alias(ResourceAccessCheckerInterface::class, 'api_platform.security.resource_access_checker'); diff --git a/src/Symfony/Bundle/Resources/config/state/security.php b/src/Symfony/Bundle/Resources/config/state/security.php index 2a31a863eb7..ee24ded92e8 100644 --- a/src/Symfony/Bundle/Resources/config/state/security.php +++ b/src/Symfony/Bundle/Resources/config/state/security.php @@ -24,8 +24,7 @@ ->args([ service('api_platform.state_provider.access_checker.inner'), service('api_platform.security.resource_access_checker'), - ]) - ->arg('$debug', '%kernel.debug%'); + ]); $services->set('api_platform.state_provider.access_checker.post_deserialize', AccessCheckerProvider::class) ->decorate('api_platform.state_provider.deserialize', null, 0) @@ -33,8 +32,7 @@ service('api_platform.state_provider.access_checker.post_deserialize.inner'), service('api_platform.security.resource_access_checker'), 'post_denormalize', - ]) - ->arg('$debug', '%kernel.debug%'); + ]); $services->set('api_platform.state_provider.security_parameter', SecurityParameterProvider::class) ->decorate('api_platform.state_provider.access_checker', null, 0) @@ -49,6 +47,5 @@ service('api_platform.state_provider.access_checker.pre_read.inner'), service('api_platform.security.resource_access_checker'), 'pre_read', - ]) - ->arg('$debug', '%kernel.debug%'); + ]); }; diff --git a/src/Symfony/Bundle/Resources/config/state/security_validator.php b/src/Symfony/Bundle/Resources/config/state/security_validator.php index 91d3a397978..1b515348651 100644 --- a/src/Symfony/Bundle/Resources/config/state/security_validator.php +++ b/src/Symfony/Bundle/Resources/config/state/security_validator.php @@ -24,6 +24,5 @@ service('api_platform.state_provider.access_checker.post_validate.inner'), service('api_platform.security.resource_access_checker'), 'post_validate', - ]) - ->arg('$debug', '%kernel.debug%'); + ]); }; diff --git a/src/Symfony/Security/AccessDecisionAwareResourceAccessCheckerInterface.php b/src/Symfony/Security/AccessDecisionAwareResourceAccessCheckerInterface.php new file mode 100644 index 00000000000..2f1ca6a0fef --- /dev/null +++ b/src/Symfony/Security/AccessDecisionAwareResourceAccessCheckerInterface.php @@ -0,0 +1,25 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace ApiPlatform\Symfony\Security; + +use Symfony\Component\Security\Core\Authorization\AccessDecision; + +interface AccessDecisionAwareResourceAccessCheckerInterface +{ + /** + * @param class-string $resourceClass + * @param array $extraVariables + */ + public function decide(string $resourceClass, string $expression, array $extraVariables = []): AccessDecision; +} diff --git a/src/Symfony/Security/AccessDecisionCapturingAuthorizationChecker.php b/src/Symfony/Security/AccessDecisionCapturingAuthorizationChecker.php deleted file mode 100644 index f5b2a5a80a1..00000000000 --- a/src/Symfony/Security/AccessDecisionCapturingAuthorizationChecker.php +++ /dev/null @@ -1,47 +0,0 @@ - - * - * For the full copyright and license information, please view the LICENSE - * file that was distributed with this source code. - */ - -declare(strict_types=1); - -namespace ApiPlatform\Symfony\Security; - -use Symfony\Component\Security\Core\Authorization\AccessDecision; -use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; - -/** - * @internal - */ -final class AccessDecisionCapturingAuthorizationChecker implements AuthorizationCheckerInterface -{ - private ?AccessDecision $accessDecision = null; - - public function __construct(private readonly AuthorizationCheckerInterface $decorated) - { - } - - public function isGranted(mixed $attribute, mixed $subject = null, ?AccessDecision $accessDecision = null): bool - { - $accessDecision ??= new AccessDecision(); - $accessDecision->isGranted = $this->decorated->isGranted($attribute, $subject, $accessDecision); - $this->accessDecision = $accessDecision; - - return $accessDecision->isGranted; - } - - public function getAccessDeniedMessage(): ?string - { - if (null === $this->accessDecision || $this->accessDecision->isGranted) { - return null; - } - - return $this->accessDecision->getMessage(); - } -} diff --git a/src/Symfony/Security/AccessDeniedMessageProviderInterface.php b/src/Symfony/Security/AccessDeniedMessageProviderInterface.php deleted file mode 100644 index 4fd0bf4dc5e..00000000000 --- a/src/Symfony/Security/AccessDeniedMessageProviderInterface.php +++ /dev/null @@ -1,22 +0,0 @@ - - * - * For the full copyright and license information, please view the LICENSE - * file that was distributed with this source code. - */ - -declare(strict_types=1); - -namespace ApiPlatform\Symfony\Security; - -/** - * Exposes the applicable denial message from the latest completed access check. - */ -interface AccessDeniedMessageProviderInterface -{ - public function getAccessDeniedMessage(): ?string; -} diff --git a/src/Symfony/Security/Core/Authorization/ExpressionLanguageProvider.php b/src/Symfony/Security/Core/Authorization/ExpressionLanguageProvider.php index feb2238a900..74b9cad28b8 100644 --- a/src/Symfony/Security/Core/Authorization/ExpressionLanguageProvider.php +++ b/src/Symfony/Security/Core/Authorization/ExpressionLanguageProvider.php @@ -26,7 +26,7 @@ final class ExpressionLanguageProvider implements ExpressionFunctionProviderInte public function getFunctions(): array { return [ - new ExpressionFunction('is_granted', static fn ($attributes, $object = 'null'): string => \sprintf('$auth_checker->isGranted(%s, %s)', $attributes, $object), static fn (array $variables, $attributes, $object = null) => $variables['auth_checker']->isGranted($attributes, $object)), + new ExpressionFunction('is_granted', static fn ($attributes, $object = 'null'): string => \sprintf('$auth_checker->isGranted(%s, %s, $access_decision ?? null)', $attributes, $object), static fn (array $variables, $attributes, $object = null) => $variables['auth_checker']->isGranted($attributes, $object, $variables['access_decision'] ?? null)), ]; } } diff --git a/src/Symfony/Security/Exception/AccessDeniedException.php b/src/Symfony/Security/Exception/AccessDeniedException.php index e2383d15f45..88349e501f2 100644 --- a/src/Symfony/Security/Exception/AccessDeniedException.php +++ b/src/Symfony/Security/Exception/AccessDeniedException.php @@ -15,21 +15,15 @@ use ApiPlatform\Metadata\Exception\AccessDeniedException as MetadataAccessDeniedException; use ApiPlatform\Metadata\Exception\HttpExceptionInterface; -use ApiPlatform\Metadata\Exception\ProblemExceptionInterface; use Symfony\Component\Security\Core\Exception\AccessDeniedException as ExceptionAccessDeniedException; /** * @deprecated since API Platform 4.4, use {@see MetadataAccessDeniedException} instead */ -final class AccessDeniedException extends ExceptionAccessDeniedException implements HttpExceptionInterface, ProblemExceptionInterface +final class AccessDeniedException extends ExceptionAccessDeniedException implements HttpExceptionInterface { - public function __construct( - string $message = 'Access Denied.', - ?\Throwable $previous = null, - int $code = 403, - bool $triggerDeprecation = true, - private readonly ?string $detail = null, - ) { + public function __construct(string $message = 'Access Denied.', ?\Throwable $previous = null, int $code = 403, bool $triggerDeprecation = true) + { if ($triggerDeprecation) { trigger_deprecation('api-platform/core', '4.4', 'The "%s" class is deprecated, use "%s" instead.', self::class, MetadataAccessDeniedException::class); } @@ -37,31 +31,6 @@ public function __construct( parent::__construct($message, $previous, $code); } - public function getType(): string - { - return '/errors/403'; - } - - public function getTitle(): string - { - return 'An error occurred'; - } - - public function getStatus(): int - { - return 403; - } - - public function getDetail(): string - { - return $this->detail ?? $this->getMessage(); - } - - public function getInstance(): ?string - { - return null; - } - public function getStatusCode(): int { return 403; diff --git a/src/Symfony/Security/ResourceAccessChecker.php b/src/Symfony/Security/ResourceAccessChecker.php index 0be7c1e5529..6b35dddcd85 100644 --- a/src/Symfony/Security/ResourceAccessChecker.php +++ b/src/Symfony/Security/ResourceAccessChecker.php @@ -22,27 +22,28 @@ use Symfony\Component\Security\Core\Authentication\Token\NullToken; use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; +use Symfony\Component\Security\Core\Authorization\AccessDecision; use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; use Symfony\Component\Security\Core\Role\RoleHierarchyInterface; -use Symfony\Contracts\Service\ResetInterface; /** * Checks if the logged user has sufficient permissions to access the given resource. * * @author Kévin Dunglas */ -final class ResourceAccessChecker implements ResourceAccessCheckerInterface, ObjectVariableCheckerInterface, AccessDeniedMessageProviderInterface, ResetInterface +final class ResourceAccessChecker implements ResourceAccessCheckerInterface, ObjectVariableCheckerInterface, AccessDecisionAwareResourceAccessCheckerInterface { - private ?string $accessDeniedMessage = null; - public function __construct(private readonly ?ExpressionLanguage $expressionLanguage = null, private readonly ?AuthenticationTrustResolverInterface $authenticationTrustResolver = null, private readonly ?RoleHierarchyInterface $roleHierarchy = null, private readonly ?TokenStorageInterface $tokenStorage = null, private readonly ?AuthorizationCheckerInterface $authorizationChecker = null) { } public function isGranted(string $resourceClass, string $expression, array $extraVariables = []): bool { - $this->reset(); + return $this->decide($resourceClass, $expression, $extraVariables)->isGranted; + } + public function decide(string $resourceClass, string $expression, array $extraVariables = []): AccessDecision + { if (null === $this->tokenStorage || null === $this->authenticationTrustResolver) { throw new \LogicException('The "symfony/security" library must be installed to use the "security" attribute.'); } @@ -51,24 +52,10 @@ public function isGranted(string $resourceClass, string $expression, array $extr throw new \LogicException('The "symfony/expression-language" library must be installed to use the "security" attribute.'); } - $authorizationChecker = null === $this->authorizationChecker ? null : new AccessDecisionCapturingAuthorizationChecker($this->authorizationChecker); - $granted = (bool) $this->expressionLanguage->evaluate($expression, $this->getVariables($extraVariables, $authorizationChecker)); + $decision = new AccessDecision(); + $decision->isGranted = (bool) $this->expressionLanguage->evaluate($expression, $this->getVariables($extraVariables, $decision)); - if (!$granted && null !== $authorizationChecker) { - $this->accessDeniedMessage = $authorizationChecker->getAccessDeniedMessage(); - } - - return $granted; - } - - public function getAccessDeniedMessage(): ?string - { - return $this->accessDeniedMessage; - } - - public function reset(): void - { - $this->accessDeniedMessage = null; + return $decision; } public function usesObjectVariable(string $expression, array $variables = []): bool @@ -81,7 +68,7 @@ public function usesObjectVariable(string $expression, array $variables = []): b throw new RuntimeException('The "symfony/expression-language" library must be installed to use the "security" attribute.'); } - return $this->hasObjectVariable($this->expressionLanguage->parse($expression, array_keys($this->getVariables($variables, $this->authorizationChecker)))->getNodes()->toArray()); + return $this->hasObjectVariable($this->expressionLanguage->parse($expression, array_keys($this->getVariables($variables)))->getNodes()->toArray()); } /** @@ -89,7 +76,7 @@ public function usesObjectVariable(string $expression, array $variables = []): b * * @see https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Security/Core/Authorization/Voter/ExpressionVoter.php */ - private function getVariables(array $variables, ?AuthorizationCheckerInterface $authorizationChecker): array + private function getVariables(array $variables, ?AccessDecision $accessDecision = null): array { if (null === $token = $this->tokenStorage->getToken()) { $token = new NullToken(); @@ -100,7 +87,8 @@ private function getVariables(array $variables, ?AuthorizationCheckerInterface $ 'user' => $token->getUser(), 'roles' => $this->getEffectiveRoles($token), 'trust_resolver' => $this->authenticationTrustResolver, - 'auth_checker' => $authorizationChecker, // needed for the is_granted expression function + 'auth_checker' => $this->authorizationChecker, // needed for the is_granted expression function + 'access_decision' => $accessDecision, ]); } diff --git a/src/Symfony/Security/State/AccessCheckerProvider.php b/src/Symfony/Security/State/AccessCheckerProvider.php index e864b53beb7..917f6b88dc3 100644 --- a/src/Symfony/Security/State/AccessCheckerProvider.php +++ b/src/Symfony/Security/State/AccessCheckerProvider.php @@ -13,6 +13,7 @@ namespace ApiPlatform\Symfony\Security\State; +use ApiPlatform\Metadata\Exception\AccessDeniedException as MetadataAccessDeniedException; use ApiPlatform\Metadata\Exception\RuntimeException; use ApiPlatform\Metadata\GraphQl\Operation as GraphQlOperation; use ApiPlatform\Metadata\GraphQl\QueryCollection; @@ -20,7 +21,7 @@ use ApiPlatform\Metadata\Operation; use ApiPlatform\Metadata\ResourceAccessCheckerInterface; use ApiPlatform\State\ProviderInterface; -use ApiPlatform\Symfony\Security\AccessDeniedMessageProviderInterface; +use ApiPlatform\Symfony\Security\AccessDecisionAwareResourceAccessCheckerInterface; use ApiPlatform\Symfony\Security\Exception\AccessDeniedException; use ApiPlatform\Symfony\Security\ObjectVariableCheckerInterface; use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException; @@ -33,7 +34,7 @@ */ final class AccessCheckerProvider implements ProviderInterface { - public function __construct(private readonly ProviderInterface $decorated, private readonly ResourceAccessCheckerInterface $resourceAccessChecker, private readonly ?string $event = null, private readonly bool $debug = false) + public function __construct(private readonly ProviderInterface $decorated, private readonly ResourceAccessCheckerInterface $resourceAccessChecker, private readonly ?string $event = null) { } @@ -98,20 +99,25 @@ public function provide(Operation $operation, array $uriVariables = [], array $c return $this->decorated->provide($operation, $uriVariables, $context); } - if (!$this->resourceAccessChecker->isGranted($operation->getClass(), $isGranted, $resourceAccessCheckerContext)) { + $decision = null; + if ($this->resourceAccessChecker instanceof AccessDecisionAwareResourceAccessCheckerInterface) { + $decision = $this->resourceAccessChecker->decide($operation->getClass(), $isGranted, $resourceAccessCheckerContext); + $granted = $decision->isGranted; + } else { + $granted = $this->resourceAccessChecker->isGranted($operation->getClass(), $isGranted, $resourceAccessCheckerContext); + } + + if (!$granted) { if ($operation instanceof GraphQlOperation) { throw new AccessDeniedHttpException($message ?? 'Access Denied.'); } - $voterMessage = null; - if (null === $message && $this->resourceAccessChecker instanceof AccessDeniedMessageProviderInterface) { - $voterMessage = $this->resourceAccessChecker->getAccessDeniedMessage(); - } + $detail = $message; + $message ??= $decision?->getMessage() ?? 'Access Denied.'; - $publicDetail = $message ?? ($this->debug ? $voterMessage : null) ?? 'Access Denied.'; - $message ??= $voterMessage ?? 'Access Denied.'; + $problem = new MetadataAccessDeniedException($message, detail: $detail); - throw new AccessDeniedException($message, triggerDeprecation: false, detail: $publicDetail); + throw new AccessDeniedException($message, $problem, triggerDeprecation: false); } return 'pre_read' === $this->event ? $this->decorated->provide($operation, $uriVariables, $context) : $body; diff --git a/src/Symfony/Tests/Bundle/DependencyInjection/ApiPlatformExtensionTest.php b/src/Symfony/Tests/Bundle/DependencyInjection/ApiPlatformExtensionTest.php index 6d44fc8acc0..689b17faa70 100644 --- a/src/Symfony/Tests/Bundle/DependencyInjection/ApiPlatformExtensionTest.php +++ b/src/Symfony/Tests/Bundle/DependencyInjection/ApiPlatformExtensionTest.php @@ -253,7 +253,6 @@ public function testCommonConfiguration(): void $this->assertServiceHasTags('api_platform.serializer.normalizer.item', ['serializer.normalizer']); $this->assertServiceHasTags('api_platform.serializer_locator', ['container.service_locator']); $this->assertServiceHasTags('api_platform.filter_locator', ['container.service_locator']); - $this->assertServiceHasTags('api_platform.security.resource_access_checker', ['kernel.reset']); // api.xml $this->assertServiceHasTags('api_platform.route_loader', ['routing.loader']); @@ -278,20 +277,6 @@ public function testCommonConfiguration(): void $this->assertTrue($this->container->getParameter('api_platform.enable_head_request_optimization')); } - public function testHttpAccessCheckerProvidersUseKernelDebugToExposeVoterReasons(): void - { - (new ApiPlatformExtension())->load(self::DEFAULT_CONFIG, $this->container); - - foreach ([ - 'api_platform.state_provider.access_checker', - 'api_platform.state_provider.access_checker.post_deserialize', - 'api_platform.state_provider.access_checker.post_validate', - 'api_platform.state_provider.access_checker.pre_read', - ] as $serviceId) { - $this->assertSame('%kernel.debug%', $this->container->getDefinition($serviceId)->getArgument('$debug')); - } - } - public function testSwaggerUiDisabledConfiguration(): void { $config = self::DEFAULT_CONFIG; diff --git a/src/Symfony/Tests/Security/Core/Authorization/ExpressionLanguageProviderTest.php b/src/Symfony/Tests/Security/Core/Authorization/ExpressionLanguageProviderTest.php new file mode 100644 index 00000000000..85fb38ef590 --- /dev/null +++ b/src/Symfony/Tests/Security/Core/Authorization/ExpressionLanguageProviderTest.php @@ -0,0 +1,73 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace ApiPlatform\Tests\Symfony\Security\Core\Authorization; + +use ApiPlatform\Symfony\Security\Core\Authorization\ExpressionLanguageProvider; +use PHPUnit\Framework\TestCase; +use Symfony\Component\ExpressionLanguage\ExpressionLanguage; +use Symfony\Component\Security\Core\Authorization\AccessDecision; +use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; + +final class ExpressionLanguageProviderTest extends TestCase +{ + public function testPassesTheAccessDecisionToTheAuthorizationChecker(): void + { + $decision = new AccessDecision(); + $authorizationChecker = new RecordingAuthorizationChecker(); + + $result = self::createExpressionLanguage()->evaluate('is_granted("A")', [ + 'access_decision' => $decision, + 'auth_checker' => $authorizationChecker, + ]); + + $this->assertTrue($result); + $this->assertSame($decision, $authorizationChecker->accessDecision); + } + + public function testDefaultsToNoAccessDecisionOutsideApiPlatform(): void + { + $authorizationChecker = new RecordingAuthorizationChecker(); + + $result = self::createExpressionLanguage()->evaluate('is_granted("A")', [ + 'auth_checker' => $authorizationChecker, + ]); + + $this->assertTrue($result); + $this->assertNull($authorizationChecker->accessDecision); + } + + public function testCompilerPassesTheOptionalAccessDecision(): void + { + $compiled = self::createExpressionLanguage()->compile('is_granted("A")', ['access_decision', 'auth_checker']); + + $this->assertStringContainsString('$auth_checker->isGranted("A", null, $access_decision ?? null)', $compiled); + } + + private static function createExpressionLanguage(): ExpressionLanguage + { + return new ExpressionLanguage(null, [new ExpressionLanguageProvider()]); + } +} + +final class RecordingAuthorizationChecker implements AuthorizationCheckerInterface +{ + public ?AccessDecision $accessDecision = null; + + public function isGranted(mixed $attribute, mixed $subject = null, ?AccessDecision $accessDecision = null): bool + { + $this->accessDecision = $accessDecision; + + return true; + } +} diff --git a/src/Symfony/Tests/Security/Exception/AccessDeniedExceptionTest.php b/src/Symfony/Tests/Security/Exception/AccessDeniedExceptionTest.php index 3e423bcbd67..f10d0fa3c85 100644 --- a/src/Symfony/Tests/Security/Exception/AccessDeniedExceptionTest.php +++ b/src/Symfony/Tests/Security/Exception/AccessDeniedExceptionTest.php @@ -13,8 +13,6 @@ namespace ApiPlatform\Tests\Symfony\Security\Exception; -use ApiPlatform\Metadata\Exception\ProblemExceptionInterface; -use ApiPlatform\State\ApiResource\Error; use ApiPlatform\Symfony\Security\Exception\AccessDeniedException; use PHPUnit\Framework\Attributes\IgnoreDeprecations; use PHPUnit\Framework\TestCase; @@ -40,28 +38,4 @@ public function testKeepsBaseExceptionBehavior(): void $this->assertSame(403, $exception->getStatusCode()); $this->assertSame([], $exception->getHeaders()); } - - public function testExposesASeparatePublicProblemDetail(): void - { - $exception = new AccessDeniedException( - 'Access Denied. Voter reason.', - triggerDeprecation: false, - detail: 'Access Denied.', - ); - - $this->assertInstanceOf(ProblemExceptionInterface::class, $exception); - $this->assertSame('Access Denied. Voter reason.', $exception->getMessage()); - $this->assertSame('Access Denied.', $exception->getDetail()); - $this->assertSame('/errors/403', $exception->getType()); - $this->assertSame('An error occurred', $exception->getTitle()); - $this->assertSame(403, $exception->getStatus()); - $this->assertNull($exception->getInstance()); - - $error = Error::createFromException($exception, 403); - - $this->assertSame('Access Denied.', $error->getDetail()); - $this->assertSame('/errors/403', $error->getType()); - $this->assertSame('An error occurred', $error->getTitle()); - $this->assertSame(403, $error->getStatus()); - } } diff --git a/tests/Symfony/Security/AccessDecisionCapturingAuthorizationCheckerTest.php b/tests/Symfony/Security/AccessDecisionCapturingAuthorizationCheckerTest.php deleted file mode 100644 index ff77af28e1a..00000000000 --- a/tests/Symfony/Security/AccessDecisionCapturingAuthorizationCheckerTest.php +++ /dev/null @@ -1,242 +0,0 @@ - - * - * For the full copyright and license information, please view the LICENSE - * file that was distributed with this source code. - */ - -declare(strict_types=1); - -namespace ApiPlatform\Tests\Symfony\Security; - -use ApiPlatform\Symfony\Security\AccessDecisionCapturingAuthorizationChecker; -use PHPUnit\Framework\TestCase; -use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorage; -use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; -use Symfony\Component\Security\Core\Authorization\AccessDecision; -use Symfony\Component\Security\Core\Authorization\AccessDecisionManager; -use Symfony\Component\Security\Core\Authorization\AuthorizationChecker; -use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; -use Symfony\Component\Security\Core\Authorization\Strategy\ConsensusStrategy; -use Symfony\Component\Security\Core\Authorization\Strategy\UnanimousStrategy; -use Symfony\Component\Security\Core\Authorization\Voter\Vote; -use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface; - -final class AccessDecisionCapturingAuthorizationCheckerTest extends TestCase -{ - public function testItPreservesArgumentsAndResult(): void - { - $subject = new \stdClass(); - $decorated = self::createAuthorizationChecker(function (mixed $attribute, mixed $actualSubject, ?AccessDecision $accessDecision) use ($subject): bool { - $this->assertSame('ATTRIBUTE', $attribute); - $this->assertSame($subject, $actualSubject); - $this->assertInstanceOf(AccessDecision::class, $accessDecision); - - return true; - }); - - $checker = new AccessDecisionCapturingAuthorizationChecker($decorated); - - $this->assertTrue($checker->isGranted('ATTRIBUTE', $subject)); - $this->assertNull($checker->getAccessDeniedMessage()); - } - - public function testItCreatesOneFreshDecisionPerInvocation(): void - { - $decisions = []; - $decorated = self::createAuthorizationChecker(static function (mixed $attribute, mixed $subject, ?AccessDecision $accessDecision) use (&$decisions): bool { - $decisions[] = $accessDecision; - - return false; - }); - - $checker = new AccessDecisionCapturingAuthorizationChecker($decorated); - $checker->isGranted('A'); - $checker->isGranted('B'); - - $this->assertCount(2, $decisions); - $this->assertNotSame($decisions[0], $decisions[1]); - } - - public function testItUsesAnExplicitlySuppliedDecision(): void - { - $decision = new AccessDecision(); - $decorated = self::createAuthorizationChecker(function (mixed $attribute, mixed $subject, ?AccessDecision $actualDecision) use ($decision): bool { - $this->assertSame($decision, $actualDecision); - - return false; - }); - - $checker = new AccessDecisionCapturingAuthorizationChecker($decorated); - - $this->assertFalse($checker->isGranted('ATTRIBUTE', null, $decision)); - $this->assertFalse($decision->isGranted); - $this->assertSame('Access Denied.', $checker->getAccessDeniedMessage()); - } - - public function testItUsesSymfonyToFormatReasonsFromMatchingVotes(): void - { - $authorizationChecker = self::createSymfonyAuthorizationChecker([ - self::createVoter(VoterInterface::ACCESS_DENIED, 'First reason.'), - self::createVoter(VoterInterface::ACCESS_GRANTED, 'Granted reason.'), - self::createVoter(VoterInterface::ACCESS_DENIED, 'Second reason.'), - ], new ConsensusStrategy(false, false)); - - $checker = new AccessDecisionCapturingAuthorizationChecker($authorizationChecker); - - $this->assertFalse($checker->isGranted('ATTRIBUTE')); - $this->assertSame('Access Denied. First reason. Second reason.', $checker->getAccessDeniedMessage()); - } - - public function testItSelectsOnlyTheLastIndependentDeniedDecision(): void - { - $decorated = self::createAuthorizationChecker(static function (mixed $attribute, mixed $subject, ?AccessDecision $accessDecision): bool { - $accessDecision->votes[] = self::createVote(VoterInterface::ACCESS_DENIED, $attribute.' reason.'); - - return false; - }); - - $checker = new AccessDecisionCapturingAuthorizationChecker($decorated); - $checker->isGranted('A'); - $checker->isGranted('B'); - - $this->assertSame('Access Denied. B reason.', $checker->getAccessDeniedMessage()); - } - - public function testALaterGrantLeavesNoDeniedMessage(): void - { - $decorated = self::createAuthorizationChecker(static function (mixed $attribute, mixed $subject, ?AccessDecision $accessDecision): bool { - $granted = 'B' === $attribute; - $accessDecision->votes[] = self::createVote($granted ? VoterInterface::ACCESS_GRANTED : VoterInterface::ACCESS_DENIED, $attribute.' reason.'); - - return $granted; - }); - - $checker = new AccessDecisionCapturingAuthorizationChecker($decorated); - $checker->isGranted('A'); - $checker->isGranted('B'); - - $this->assertNull($checker->getAccessDeniedMessage()); - } - - public function testItRecordsTheResultWhenACustomCheckerIgnoresTheDecision(): void - { - $checker = new AccessDecisionCapturingAuthorizationChecker(self::createAuthorizationChecker(static fn (): bool => false)); - - $this->assertFalse($checker->isGranted('ATTRIBUTE')); - $this->assertSame('Access Denied.', $checker->getAccessDeniedMessage()); - } - - public function testNestedSymfonyAuthorizationUsesTheTopLevelDecision(): void - { - $outerVoter = new class implements VoterInterface { - private AuthorizationCheckerInterface $authorizationChecker; - - public function setAuthorizationChecker(AuthorizationCheckerInterface $authorizationChecker): void - { - $this->authorizationChecker = $authorizationChecker; - } - - public function vote(TokenInterface $token, mixed $subject, array $attributes, ?Vote $vote = null): int - { - if ('OUTER' !== $attributes[0]) { - return self::ACCESS_ABSTAIN; - } - - $this->authorizationChecker->isGranted('INNER'); - $vote?->addReason('Outer reason.'); - - return self::ACCESS_DENIED; - } - }; - $innerVoter = self::createAttributeVoter([ - 'INNER' => [VoterInterface::ACCESS_DENIED, 'Inner reason.'], - ]); - $authorizationChecker = self::createSymfonyAuthorizationChecker([$outerVoter, $innerVoter], new UnanimousStrategy()); - $outerVoter->setAuthorizationChecker($authorizationChecker); - - $checker = new AccessDecisionCapturingAuthorizationChecker($authorizationChecker); - - $this->assertFalse($checker->isGranted('OUTER')); - $this->assertSame('Access Denied. Inner reason. Outer reason.', $checker->getAccessDeniedMessage()); - } - - private static function createAuthorizationChecker(\Closure $callback): AuthorizationCheckerInterface - { - return new class($callback) implements AuthorizationCheckerInterface { - public function __construct(private readonly \Closure $callback) - { - } - - public function isGranted(mixed $attribute, mixed $subject = null, ?AccessDecision $accessDecision = null): bool - { - return ($this->callback)($attribute, $subject, $accessDecision); - } - }; - } - - /** - * @param list $voters - */ - private static function createSymfonyAuthorizationChecker(array $voters, ConsensusStrategy|UnanimousStrategy $strategy): AuthorizationCheckerInterface - { - return new AuthorizationChecker(new TokenStorage(), new AccessDecisionManager($voters, $strategy)); - } - - private static function createVoter(int $result, string $reason): VoterInterface - { - return new class($result, $reason) implements VoterInterface { - public function __construct(private readonly int $result, private readonly string $reason) - { - } - - public function vote(TokenInterface $token, mixed $subject, array $attributes, ?Vote $vote = null): int - { - $vote?->addReason($this->reason); - - return $this->result; - } - }; - } - - /** - * @param array $votes - */ - private static function createAttributeVoter(array $votes): VoterInterface - { - return new class($votes) implements VoterInterface { - /** - * @param array $votes - */ - public function __construct(private readonly array $votes) - { - } - - public function vote(TokenInterface $token, mixed $subject, array $attributes, ?Vote $vote = null): int - { - if (!isset($this->votes[$attributes[0]])) { - return self::ACCESS_ABSTAIN; - } - - [$result, $reason] = $this->votes[$attributes[0]]; - $vote?->addReason($reason); - - return $result; - } - }; - } - - private static function createVote(int $result, string $reason): Vote - { - $vote = new Vote(); - $vote->voter = self::class; - $vote->result = $result; - $vote->addReason($reason); - - return $vote; - } -} diff --git a/tests/Symfony/Security/ResourceAccessCheckerTest.php b/tests/Symfony/Security/ResourceAccessCheckerTest.php index 18447154a0c..a7d33451a86 100644 --- a/tests/Symfony/Security/ResourceAccessCheckerTest.php +++ b/tests/Symfony/Security/ResourceAccessCheckerTest.php @@ -14,6 +14,8 @@ namespace ApiPlatform\Tests\Symfony\Security; use ApiPlatform\Metadata\Exception\RuntimeException; +use ApiPlatform\Symfony\Security\AccessDecisionAwareResourceAccessCheckerInterface; +use ApiPlatform\Symfony\Security\Core\Authorization\ExpressionLanguageProvider; use ApiPlatform\Symfony\Security\ResourceAccessChecker; use ApiPlatform\Tests\Fixtures\Serializable; use ApiPlatform\Tests\Fixtures\TestBundle\Entity\Dummy; @@ -31,7 +33,6 @@ use Symfony\Component\Security\Core\Authorization\ExpressionLanguage; use Symfony\Component\Security\Core\Authorization\Voter\Vote; use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface; -use Symfony\Contracts\Service\ResetInterface; /** * @author Kévin Dunglas @@ -136,8 +137,11 @@ public function testCapturesASingleDeniedAuthorizationMessage(): void 'A' => [false, [[VoterInterface::ACCESS_DENIED, 'Reason A.']]], ]); - $this->assertFalse($checker->isGranted(Dummy::class, "is_granted('A', object)", ['object' => new \stdClass()])); - $this->assertSame('Access Denied. Reason A.', $checker->getAccessDeniedMessage()); + $this->assertInstanceOf(AccessDecisionAwareResourceAccessCheckerInterface::class, $checker); + $decision = $checker->decide(Dummy::class, "is_granted('A', object)", ['object' => new \stdClass()]); + + $this->assertFalse($decision->isGranted); + $this->assertSame('Access Denied. Reason A.', $decision->getMessage()); } public function testAndShortCircuitsAfterTheFirstDenial(): void @@ -148,12 +152,14 @@ public function testAndShortCircuitsAfterTheFirstDenial(): void 'B' => [false, [[VoterInterface::ACCESS_DENIED, 'Reason B.']]], ], $calls); - $this->assertFalse($checker->isGranted(Dummy::class, "is_granted('A', object) && is_granted('B', object)", ['object' => new \stdClass()])); + $decision = $checker->decide(Dummy::class, "is_granted('A', object) && is_granted('B', object)", ['object' => new \stdClass()]); + + $this->assertFalse($decision->isGranted); $this->assertSame(['A'], $calls); - $this->assertSame('Access Denied. Reason A.', $checker->getAccessDeniedMessage()); + $this->assertSame('Access Denied. Reason A.', $decision->getMessage()); } - public function testAndSelectsTheSecondDecisionWhenTheFirstGrants(): void + public function testAndAggregatesDeniedVotesAcrossAuthorizationChecks(): void { $calls = []; $checker = self::createResourceAccessChecker([ @@ -164,12 +170,14 @@ public function testAndSelectsTheSecondDecisionWhenTheFirstGrants(): void 'B' => [false, [[VoterInterface::ACCESS_DENIED, 'Reason B.']]], ], $calls); - $this->assertFalse($checker->isGranted(Dummy::class, "is_granted('A', object) && is_granted('B', object)", ['object' => new \stdClass()])); + $decision = $checker->decide(Dummy::class, "is_granted('A', object) && is_granted('B', object)", ['object' => new \stdClass()]); + + $this->assertFalse($decision->isGranted); $this->assertSame(['A', 'B'], $calls); - $this->assertSame('Access Denied. Reason B.', $checker->getAccessDeniedMessage()); + $this->assertSame('Access Denied. A minority denial. Reason B.', $decision->getMessage()); } - public function testOrSelectsTheLastDecisionWhenBothDeny(): void + public function testOrAggregatesBothDecisionsWhenBothDeny(): void { $calls = []; $checker = self::createResourceAccessChecker([ @@ -177,9 +185,11 @@ public function testOrSelectsTheLastDecisionWhenBothDeny(): void 'B' => [false, [[VoterInterface::ACCESS_DENIED, 'Reason B.']]], ], $calls); - $this->assertFalse($checker->isGranted(Dummy::class, "is_granted('A', object) || is_granted('B', object)", ['object' => new \stdClass()])); + $decision = $checker->decide(Dummy::class, "is_granted('A', object) || is_granted('B', object)", ['object' => new \stdClass()]); + + $this->assertFalse($decision->isGranted); $this->assertSame(['A', 'B'], $calls); - $this->assertSame('Access Denied. Reason B.', $checker->getAccessDeniedMessage()); + $this->assertSame('Access Denied. Reason A. Reason B.', $decision->getMessage()); } public function testNegatedGrantExposesNoDeniedMessage(): void @@ -188,8 +198,10 @@ public function testNegatedGrantExposesNoDeniedMessage(): void 'A' => [true, [[VoterInterface::ACCESS_GRANTED, 'Reason A.']]], ]); - $this->assertFalse($checker->isGranted(Dummy::class, "!is_granted('A', object)", ['object' => new \stdClass()])); - $this->assertNull($checker->getAccessDeniedMessage()); + $decision = $checker->decide(Dummy::class, "!is_granted('A', object)", ['object' => new \stdClass()]); + + $this->assertFalse($decision->isGranted); + $this->assertSame('Access Denied.', $decision->getMessage()); } public function testNonAuthorizationConditionAfterAGrantExposesNoDeniedMessage(): void @@ -201,8 +213,10 @@ public function testNonAuthorizationConditionAfterAGrantExposesNoDeniedMessage() public bool $enabled = false; }; - $this->assertFalse($checker->isGranted(Dummy::class, "is_granted('A', object) && object.enabled", ['object' => $object])); - $this->assertNull($checker->getAccessDeniedMessage()); + $decision = $checker->decide(Dummy::class, "is_granted('A', object) && object.enabled", ['object' => $object]); + + $this->assertFalse($decision->isGranted); + $this->assertSame('Access Denied.', $decision->getMessage()); } public function testAuthorizationDenialBeforeAnObjectConditionExposesItsMessage(): void @@ -215,12 +229,14 @@ public function testAuthorizationDenialBeforeAnObjectConditionExposesItsMessage( public bool $enabled = false; }; - $this->assertFalse($checker->isGranted(Dummy::class, "is_granted('A', object) && object.enabled", ['object' => $object])); + $decision = $checker->decide(Dummy::class, "is_granted('A', object) && object.enabled", ['object' => $object]); + + $this->assertFalse($decision->isGranted); $this->assertSame(['A'], $calls); - $this->assertSame('Access Denied. Reason A.', $checker->getAccessDeniedMessage()); + $this->assertSame('Access Denied. Reason A.', $decision->getMessage()); } - public function testPureNonAuthorizationDenialExposesNoDeniedMessage(): void + public function testPureNonAuthorizationDenialReturnsAnInitializedDecision(): void { $calls = []; $checker = self::createResourceAccessChecker([], $calls); @@ -228,53 +244,57 @@ public function testPureNonAuthorizationDenialExposesNoDeniedMessage(): void public bool $enabled = false; }; - $this->assertFalse($checker->isGranted(Dummy::class, 'object.enabled', ['object' => $object])); + $decision = $checker->decide(Dummy::class, 'object.enabled', ['object' => $object]); + + $this->assertFalse($decision->isGranted); $this->assertSame([], $calls); - $this->assertNull($checker->getAccessDeniedMessage()); + $this->assertSame('Access Denied.', $decision->getMessage()); } - public function testDeniedDecisionWithoutReasonExposesTheGenericMessage(): void + public function testNonAuthorizationConditionCanGrantAfterAnAuthorizationDenial(): void { + $calls = []; $checker = self::createResourceAccessChecker([ - 'A' => [false, []], - ]); + 'A' => [false, [[VoterInterface::ACCESS_DENIED, 'Reason A.']]], + ], $calls); + $object = new class { + public bool $owner = true; + }; + + $decision = $checker->decide(Dummy::class, "is_granted('A', object) || object.owner", ['object' => $object]); - $this->assertFalse($checker->isGranted(Dummy::class, "is_granted('A')")); - $this->assertSame('Access Denied.', $checker->getAccessDeniedMessage()); + $this->assertTrue($decision->isGranted); + $this->assertSame(['A'], $calls); + $this->assertSame('Access Granted.', $decision->getMessage()); } - public function testCapturedMessageIsResetBetweenEvaluations(): void + public function testDeniedDecisionWithoutReasonExposesTheGenericMessage(): void { $checker = self::createResourceAccessChecker([ - 'A' => [false, [[VoterInterface::ACCESS_DENIED, 'Reason A.']]], + 'A' => [false, []], ]); - $object = new class { - public bool $enabled = false; - }; - $this->assertFalse($checker->isGranted(Dummy::class, "is_granted('A')")); - $this->assertSame('Access Denied. Reason A.', $checker->getAccessDeniedMessage()); + $decision = $checker->decide(Dummy::class, "is_granted('A')"); - $this->assertFalse($checker->isGranted(Dummy::class, 'object.enabled', ['object' => $object])); - $this->assertNull($checker->getAccessDeniedMessage()); - - $this->assertTrue($checker->isGranted(Dummy::class, 'true')); - $this->assertNull($checker->getAccessDeniedMessage()); + $this->assertFalse($decision->isGranted); + $this->assertSame('Access Denied.', $decision->getMessage()); } - public function testResetClearsTheCapturedMessage(): void + public function testKeepsIndependentDeniedDecisionsSeparate(): void { $checker = self::createResourceAccessChecker([ 'A' => [false, [[VoterInterface::ACCESS_DENIED, 'Reason A.']]], + 'B' => [false, [[VoterInterface::ACCESS_DENIED, 'Reason B.']]], ]); - $this->assertInstanceOf(ResetInterface::class, $checker); - $this->assertFalse($checker->isGranted(Dummy::class, "is_granted('A')")); - $this->assertSame('Access Denied. Reason A.', $checker->getAccessDeniedMessage()); + $first = $checker->decide(Dummy::class, "is_granted('A')"); + $second = $checker->decide(Dummy::class, "is_granted('B')"); - $checker->reset(); - - $this->assertNull($checker->getAccessDeniedMessage()); + $this->assertNotSame($first, $second); + $this->assertFalse($first->isGranted); + $this->assertFalse($second->isGranted); + $this->assertSame('Access Denied. Reason A.', $first->getMessage()); + $this->assertSame('Access Denied. Reason B.', $second->getMessage()); } /** @@ -299,7 +319,7 @@ private static function createResourceAccessChecker(array $decisions, array &$ca private static function createResourceAccessCheckerWithAuthorizationChecker(?AuthorizationCheckerInterface $authorizationChecker): ResourceAccessChecker { - return new ResourceAccessChecker(new ExpressionLanguage(), new AuthenticationTrustResolver(), null, new TokenStorage(), $authorizationChecker); + return new ResourceAccessChecker(new ExpressionLanguage(null, [new ExpressionLanguageProvider()]), new AuthenticationTrustResolver(), null, new TokenStorage(), $authorizationChecker); } private static function createAuthorizationChecker(\Closure $callback): AuthorizationCheckerInterface @@ -316,6 +336,9 @@ public function isGranted(mixed $attribute, mixed $subject = null, ?AccessDecisi }; } + /** + * @param VoterInterface::ACCESS_* $result + */ private static function createVote(int $result, string $reason): Vote { $vote = new Vote(); diff --git a/tests/Symfony/Security/State/AccessCheckerProviderTest.php b/tests/Symfony/Security/State/AccessCheckerProviderTest.php index da8be7ef6b6..5830c164377 100644 --- a/tests/Symfony/Security/State/AccessCheckerProviderTest.php +++ b/tests/Symfony/Security/State/AccessCheckerProviderTest.php @@ -13,17 +13,21 @@ namespace ApiPlatform\Tests\Symfony\Security\State; +use ApiPlatform\Metadata\Exception\AccessDeniedException as MetadataAccessDeniedException; use ApiPlatform\Metadata\Get; use ApiPlatform\Metadata\GraphQl\Query; use ApiPlatform\Metadata\ResourceAccessCheckerInterface; use ApiPlatform\State\ProviderInterface; -use ApiPlatform\Symfony\Security\AccessDeniedMessageProviderInterface; +use ApiPlatform\Symfony\Security\AccessDecisionAwareResourceAccessCheckerInterface; use ApiPlatform\Symfony\Security\Exception\AccessDeniedException; use ApiPlatform\Symfony\Security\ObjectVariableCheckerInterface; use ApiPlatform\Symfony\Security\State\AccessCheckerProvider; use ApiPlatform\Tests\Fixtures\DummyEntity; use PHPUnit\Framework\TestCase; use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException; +use Symfony\Component\Security\Core\Authorization\AccessDecision; +use Symfony\Component\Security\Core\Authorization\Voter\Vote; +use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface; class AccessCheckerProviderTest extends TestCase { @@ -140,7 +144,9 @@ public function testCheckAccessDenied(): void $this->fail('An access denied exception should have been thrown.'); } catch (AccessDeniedException $exception) { $this->assertSame('hello', $exception->getMessage()); - $this->assertSame('hello', $exception->getDetail()); + $problem = $exception->getPrevious(); + $this->assertInstanceOf(MetadataAccessDeniedException::class, $problem); + $this->assertSame('hello', $problem->getDetail()); } } @@ -159,44 +165,40 @@ public function testCheckAccessDeniedWithGraphQl(): void $accessChecker->provide($operation, [], []); } - public function testPropagatesCapturedAccessDeniedMessage(): void + public function testPropagatesTheAccessDecisionMessageInternally(): void { $obj = new \stdClass(); $operation = new Get(class: DummyEntity::class, security: 'hi'); $decorated = $this->createStub(ProviderInterface::class); $decorated->method('provide')->willReturn($obj); - $resourceAccessChecker = $this->createMock(ResourceAccessCheckerWithDeniedMessageInterface::class); - $resourceAccessChecker->expects($this->once())->method('isGranted')->willReturn(false); - $resourceAccessChecker->expects($this->once())->method('getAccessDeniedMessage')->willReturn('Access Denied. Voter reason.'); - $accessChecker = new AccessCheckerProvider($decorated, $resourceAccessChecker, debug: true); + $resourceAccessChecker = $this->createMock(ResourceAccessCheckerWithDecisionInterface::class); + $resourceAccessChecker->expects($this->never())->method('isGranted'); + $resourceAccessChecker->expects($this->once())->method('decide')->willReturn(self::createDecision(false, 'Voter reason.')); + $accessChecker = new AccessCheckerProvider($decorated, $resourceAccessChecker); try { $accessChecker->provide($operation, [], []); $this->fail('An access denied exception should have been thrown.'); } catch (AccessDeniedException $exception) { $this->assertSame('Access Denied. Voter reason.', $exception->getMessage()); - $this->assertSame('Access Denied. Voter reason.', $exception->getDetail()); + $problem = $exception->getPrevious(); + $this->assertInstanceOf(MetadataAccessDeniedException::class, $problem); + $this->assertNull($problem->getDetail()); } } - public function testKeepsCapturedAccessDeniedMessageInternalWhenDebugIsDisabled(): void + public function testUsesTheDecisionResultInsteadOfCallingIsGranted(): void { $obj = new \stdClass(); $operation = new Get(class: DummyEntity::class, security: 'hi'); $decorated = $this->createStub(ProviderInterface::class); $decorated->method('provide')->willReturn($obj); - $resourceAccessChecker = $this->createMock(ResourceAccessCheckerWithDeniedMessageInterface::class); - $resourceAccessChecker->expects($this->once())->method('isGranted')->willReturn(false); - $resourceAccessChecker->expects($this->once())->method('getAccessDeniedMessage')->willReturn('Access Denied. Voter reason.'); + $resourceAccessChecker = $this->createMock(ResourceAccessCheckerWithDecisionInterface::class); + $resourceAccessChecker->expects($this->never())->method('isGranted'); + $resourceAccessChecker->expects($this->once())->method('decide')->willReturn(self::createDecision(true)); $accessChecker = new AccessCheckerProvider($decorated, $resourceAccessChecker); - try { - $accessChecker->provide($operation, [], []); - $this->fail('An access denied exception should have been thrown.'); - } catch (AccessDeniedException $exception) { - $this->assertSame('Access Denied. Voter reason.', $exception->getMessage()); - $this->assertSame('Access Denied.', $exception->getDetail()); - } + $this->assertSame($obj, $accessChecker->provide($operation, [], [])); } public function testConfiguredEmptyMessageTakesPrecedence(): void @@ -205,34 +207,38 @@ public function testConfiguredEmptyMessageTakesPrecedence(): void $operation = new Get(class: DummyEntity::class, security: 'hi', securityMessage: ''); $decorated = $this->createStub(ProviderInterface::class); $decorated->method('provide')->willReturn($obj); - $resourceAccessChecker = $this->createMock(ResourceAccessCheckerWithDeniedMessageInterface::class); - $resourceAccessChecker->expects($this->once())->method('isGranted')->willReturn(false); - $resourceAccessChecker->expects($this->never())->method('getAccessDeniedMessage'); - $accessChecker = new AccessCheckerProvider($decorated, $resourceAccessChecker, debug: true); + $resourceAccessChecker = $this->createMock(ResourceAccessCheckerWithDecisionInterface::class); + $resourceAccessChecker->expects($this->once())->method('decide')->willReturn(self::createDecision(false, 'Voter reason.')); + $accessChecker = new AccessCheckerProvider($decorated, $resourceAccessChecker); try { $accessChecker->provide($operation, [], []); $this->fail('An access denied exception should have been thrown.'); } catch (AccessDeniedException $exception) { $this->assertSame('', $exception->getMessage()); + $problem = $exception->getPrevious(); + $this->assertInstanceOf(MetadataAccessDeniedException::class, $problem); + $this->assertSame('', $problem->getDetail()); } } - public function testFallsBackToGenericMessageWhenCapturedMessageIsNull(): void + public function testFallsBackToGenericMessageWhenTheDecisionHasNoReason(): void { $operation = new Get(class: DummyEntity::class, security: 'hi'); $decorated = $this->createStub(ProviderInterface::class); $decorated->method('provide')->willReturn(new \stdClass()); - $resourceAccessChecker = $this->createMock(ResourceAccessCheckerWithDeniedMessageInterface::class); - $resourceAccessChecker->expects($this->once())->method('isGranted')->willReturn(false); - $resourceAccessChecker->expects($this->once())->method('getAccessDeniedMessage')->willReturn(null); - $accessChecker = new AccessCheckerProvider($decorated, $resourceAccessChecker, debug: true); + $resourceAccessChecker = $this->createMock(ResourceAccessCheckerWithDecisionInterface::class); + $resourceAccessChecker->expects($this->once())->method('decide')->willReturn(self::createDecision(false)); + $accessChecker = new AccessCheckerProvider($decorated, $resourceAccessChecker); try { $accessChecker->provide($operation, [], []); $this->fail('An access denied exception should have been thrown.'); } catch (AccessDeniedException $exception) { $this->assertSame('Access Denied.', $exception->getMessage()); + $problem = $exception->getPrevious(); + $this->assertInstanceOf(MetadataAccessDeniedException::class, $problem); + $this->assertNull($problem->getDetail()); } } @@ -243,21 +249,57 @@ public function testPlainCustomResourceAccessCheckerKeepsGenericFallback(): void $decorated->method('provide')->willReturn(new \stdClass()); $resourceAccessChecker = $this->createMock(ResourceAccessCheckerInterface::class); $resourceAccessChecker->expects($this->once())->method('isGranted')->willReturn(false); - $accessChecker = new AccessCheckerProvider($decorated, $resourceAccessChecker, debug: true); + $accessChecker = new AccessCheckerProvider($decorated, $resourceAccessChecker); try { $accessChecker->provide($operation, [], []); $this->fail('An access denied exception should have been thrown.'); } catch (AccessDeniedException $exception) { $this->assertSame('Access Denied.', $exception->getMessage()); + $problem = $exception->getPrevious(); + $this->assertInstanceOf(MetadataAccessDeniedException::class, $problem); + $this->assertNull($problem->getDetail()); } } + + public function testGraphQlDoesNotExposeTheAccessDecisionMessage(): void + { + $operation = new Query(class: DummyEntity::class, security: 'hi'); + $decorated = $this->createStub(ProviderInterface::class); + $decorated->method('provide')->willReturn(new \stdClass()); + $resourceAccessChecker = $this->createMock(ResourceAccessCheckerWithDecisionInterface::class); + $resourceAccessChecker->expects($this->once())->method('decide')->willReturn(self::createDecision(false, 'Voter reason.')); + $accessChecker = new AccessCheckerProvider($decorated, $resourceAccessChecker); + + $this->expectException(AccessDeniedHttpException::class); + $this->expectExceptionMessage('Access Denied.'); + + $accessChecker->provide($operation, [], []); + } + + private static function createDecision(bool $granted, ?string $reason = null): AccessDecision + { + $decision = new AccessDecision(); + $decision->isGranted = $granted; + + if (null === $reason) { + return $decision; + } + + $vote = new Vote(); + $vote->voter = self::class; + $vote->result = $granted ? VoterInterface::ACCESS_GRANTED : VoterInterface::ACCESS_DENIED; + $vote->addReason($reason); + $decision->votes[] = $vote; + + return $decision; + } } interface ResourceAccessCheckerWithObjectVariableInterface extends ResourceAccessCheckerInterface, ObjectVariableCheckerInterface { } -interface ResourceAccessCheckerWithDeniedMessageInterface extends ResourceAccessCheckerInterface, AccessDeniedMessageProviderInterface +interface ResourceAccessCheckerWithDecisionInterface extends ResourceAccessCheckerInterface, AccessDecisionAwareResourceAccessCheckerInterface { }