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/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 1c124aff438..bebda009b38 100644 --- a/src/Serializer/AbstractItemNormalizer.php +++ b/src/Serializer/AbstractItemNormalizer.php @@ -327,7 +327,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 ff54a251511..3428cd0b188 100644 --- a/src/Serializer/Tests/AbstractItemNormalizerTest.php +++ b/src/Serializer/Tests/AbstractItemNormalizerTest.php @@ -514,14 +514,17 @@ 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, - ]); + try { + $normalizer->denormalize($data, SecuredDummy::class, 'json', [ + 'operation' => $operation, + ]); + self::fail('An AccessDeniedException should have been thrown.'); + } catch (AccessDeniedException $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..514e52bd13d 100644 --- a/src/State/Tests/ErrorProviderTest.php +++ b/src/State/Tests/ErrorProviderTest.php @@ -13,9 +13,11 @@ namespace ApiPlatform\State\Tests; +use ApiPlatform\Metadata\Exception\AccessDeniedException; use ApiPlatform\Metadata\Get; use ApiPlatform\State\ApiResource\Error; use ApiPlatform\State\ErrorProvider; +use ApiPlatform\Symfony\Security\Exception\AccessDeniedException as SymfonyAccessDeniedException; use ApiPlatform\Validator\Exception\ValidationException; use PHPUnit\Framework\TestCase; use Symfony\Component\HttpFoundation\Request; @@ -44,4 +46,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 SymfonyAccessDeniedException('Access Denied. Voter reason.', $problem, triggerDeprecation: false); + $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/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/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/ResourceAccessChecker.php b/src/Symfony/Security/ResourceAccessChecker.php index 657245fd693..6b35dddcd85 100644 --- a/src/Symfony/Security/ResourceAccessChecker.php +++ b/src/Symfony/Security/ResourceAccessChecker.php @@ -22,6 +22,7 @@ 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; @@ -30,13 +31,18 @@ * * @author Kévin Dunglas */ -final class ResourceAccessChecker implements ResourceAccessCheckerInterface, ObjectVariableCheckerInterface +final class ResourceAccessChecker implements ResourceAccessCheckerInterface, ObjectVariableCheckerInterface, AccessDecisionAwareResourceAccessCheckerInterface { 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 + { + 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.'); @@ -46,7 +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.'); } - return (bool) $this->expressionLanguage->evaluate($expression, $this->getVariables($extraVariables)); + $decision = new AccessDecision(); + $decision->isGranted = (bool) $this->expressionLanguage->evaluate($expression, $this->getVariables($extraVariables, $decision)); + + return $decision; } public function usesObjectVariable(string $expression, array $variables = []): bool @@ -67,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): array + private function getVariables(array $variables, ?AccessDecision $accessDecision = null): array { if (null === $token = $this->tokenStorage->getToken()) { $token = new NullToken(); @@ -79,6 +88,7 @@ private function getVariables(array $variables): array 'roles' => $this->getEffectiveRoles($token), 'trust_resolver' => $this->authenticationTrustResolver, '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 fa3509767b7..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,6 +21,7 @@ use ApiPlatform\Metadata\Operation; use ApiPlatform\Metadata\ResourceAccessCheckerInterface; use ApiPlatform\State\ProviderInterface; +use ApiPlatform\Symfony\Security\AccessDecisionAwareResourceAccessCheckerInterface; use ApiPlatform\Symfony\Security\Exception\AccessDeniedException; use ApiPlatform\Symfony\Security\ObjectVariableCheckerInterface; use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException; @@ -97,8 +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)) { - $operation instanceof GraphQlOperation ? throw new AccessDeniedHttpException($message ?? 'Access Denied.') : throw new AccessDeniedException($message ?? 'Access Denied.', null, 403, false); + $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.'); + } + + $detail = $message; + $message ??= $decision?->getMessage() ?? 'Access Denied.'; + + $problem = new MetadataAccessDeniedException($message, detail: $detail); + + 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/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/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/ResourceAccessCheckerTest.php b/tests/Symfony/Security/ResourceAccessCheckerTest.php index a8aa66f7e4e..b30bd767756 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; @@ -21,11 +23,16 @@ 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; /** * @author Kévin Dunglas @@ -121,6 +128,221 @@ 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->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 + { + $calls = []; + $checker = self::createResourceAccessChecker([ + 'A' => [false, [[VoterInterface::ACCESS_DENIED, 'Reason A.']]], + 'B' => [false, [[VoterInterface::ACCESS_DENIED, 'Reason B.']]], + ], $calls); + + $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.', $decision->getMessage()); + } + + public function testAndAggregatesDeniedVotesAcrossAuthorizationChecks(): 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); + + $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. A minority denial. Reason B.', $decision->getMessage()); + } + + public function testOrAggregatesBothDecisionsWhenBothDeny(): void + { + $calls = []; + $checker = self::createResourceAccessChecker([ + 'A' => [false, [[VoterInterface::ACCESS_DENIED, 'Reason A.']]], + 'B' => [false, [[VoterInterface::ACCESS_DENIED, 'Reason B.']]], + ], $calls); + + $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 A. Reason B.', $decision->getMessage()); + } + + public function testNegatedGrantExposesNoDeniedMessage(): void + { + $checker = self::createResourceAccessChecker([ + 'A' => [true, [[VoterInterface::ACCESS_GRANTED, 'Reason A.']]], + ]); + + $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 + { + $checker = self::createResourceAccessChecker([ + 'A' => [true, [[VoterInterface::ACCESS_GRANTED, 'Reason A.']]], + ]); + $object = new class { + public bool $enabled = false; + }; + + $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 + { + $calls = []; + $checker = self::createResourceAccessChecker([ + 'A' => [false, [[VoterInterface::ACCESS_DENIED, 'Reason A.']]], + ], $calls); + $object = new class { + public bool $enabled = false; + }; + + $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.', $decision->getMessage()); + } + + public function testPureNonAuthorizationDenialReturnsAnInitializedDecision(): void + { + $calls = []; + $checker = self::createResourceAccessChecker([], $calls); + $object = new class { + public bool $enabled = false; + }; + + $decision = $checker->decide(Dummy::class, 'object.enabled', ['object' => $object]); + + $this->assertFalse($decision->isGranted); + $this->assertSame([], $calls); + $this->assertSame('Access Denied.', $decision->getMessage()); + } + + public function testNonAuthorizationConditionCanGrantAfterAnAuthorizationDenial(): void + { + $calls = []; + $checker = self::createResourceAccessChecker([ + '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->assertTrue($decision->isGranted); + $this->assertSame(['A'], $calls); + $this->assertSame('Access Granted.', $decision->getMessage()); + } + + public function testDeniedDecisionWithoutReasonExposesTheGenericMessage(): void + { + $checker = self::createResourceAccessChecker([ + 'A' => [false, []], + ]); + + $decision = $checker->decide(Dummy::class, "is_granted('A')"); + + $this->assertFalse($decision->isGranted); + $this->assertSame('Access Denied.', $decision->getMessage()); + } + + public function testKeepsIndependentDeniedDecisionsSeparate(): void + { + $checker = self::createResourceAccessChecker([ + 'A' => [false, [[VoterInterface::ACCESS_DENIED, 'Reason A.']]], + 'B' => [false, [[VoterInterface::ACCESS_DENIED, 'Reason B.']]], + ]); + + $first = $checker->decide(Dummy::class, "is_granted('A')"); + $second = $checker->decide(Dummy::class, "is_granted('B')"); + + $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()); + } + + /** + * @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(null, [new ExpressionLanguageProvider()]), 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..5830c164377 100644 --- a/tests/Symfony/Security/State/AccessCheckerProviderTest.php +++ b/tests/Symfony/Security/State/AccessCheckerProviderTest.php @@ -13,16 +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\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 { @@ -126,9 +131,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 +138,16 @@ 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()); + $problem = $exception->getPrevious(); + $this->assertInstanceOf(MetadataAccessDeniedException::class, $problem); + $this->assertSame('hello', $problem->getDetail()); + } } public function testCheckAccessDeniedWithGraphQl(): void @@ -153,8 +164,142 @@ public function testCheckAccessDeniedWithGraphQl(): void $accessChecker = new AccessCheckerProvider($decorated, $resourceAccessChecker); $accessChecker->provide($operation, [], []); } + + 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(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()); + $problem = $exception->getPrevious(); + $this->assertInstanceOf(MetadataAccessDeniedException::class, $problem); + $this->assertNull($problem->getDetail()); + } + } + + 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(ResourceAccessCheckerWithDecisionInterface::class); + $resourceAccessChecker->expects($this->never())->method('isGranted'); + $resourceAccessChecker->expects($this->once())->method('decide')->willReturn(self::createDecision(true)); + $accessChecker = new AccessCheckerProvider($decorated, $resourceAccessChecker); + + $this->assertSame($obj, $accessChecker->provide($operation, [], [])); + } + + 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(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 testFallsBackToGenericMessageWhenTheDecisionHasNoReason(): void + { + $operation = new Get(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)); + $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 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); + + 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 ResourceAccessCheckerWithDecisionInterface extends ResourceAccessCheckerInterface, AccessDecisionAwareResourceAccessCheckerInterface +{ +}