From 19c5d0654568706261fc0773af6d9876cb3f6078 Mon Sep 17 00:00:00 2001 From: soyuka Date: Mon, 3 Nov 2025 10:28:39 +0100 Subject: [PATCH] feat(symfony): isGranted before provider --- .../Resources/config/state/security.php | 17 ++++- .../ObjectVariableCheckerInterface.php | 23 ++++++ .../Security/ResourceAccessChecker.php | 72 +++++++++++++++---- .../Security/State/AccessCheckerProvider.php | 21 +++--- .../ApiResource/IsGrantedTestResource.php | 44 ++++++++++++ tests/Functional/IsGrantedTest.php | 68 ++++++++++++++++++ 6 files changed, 219 insertions(+), 26 deletions(-) create mode 100644 src/Symfony/Security/ObjectVariableCheckerInterface.php create mode 100644 tests/Fixtures/TestBundle/ApiResource/IsGrantedTestResource.php create mode 100644 tests/Functional/IsGrantedTest.php diff --git a/src/Symfony/Bundle/Resources/config/state/security.php b/src/Symfony/Bundle/Resources/config/state/security.php index a0f82cb10f5..993212eb09c 100644 --- a/src/Symfony/Bundle/Resources/config/state/security.php +++ b/src/Symfony/Bundle/Resources/config/state/security.php @@ -13,17 +13,20 @@ namespace Symfony\Component\DependencyInjection\Loader\Configurator; +use ApiPlatform\State\Provider\SecurityParameterProvider; +use ApiPlatform\Symfony\Security\State\AccessCheckerProvider; + return function (ContainerConfigurator $container) { $services = $container->services(); - $services->set('api_platform.state_provider.access_checker', 'ApiPlatform\Symfony\Security\State\AccessCheckerProvider') + $services->set('api_platform.state_provider.access_checker', AccessCheckerProvider::class) ->decorate('api_platform.state_provider.read', null, 0) ->args([ service('api_platform.state_provider.access_checker.inner'), service('api_platform.security.resource_access_checker'), ]); - $services->set('api_platform.state_provider.access_checker.post_deserialize', 'ApiPlatform\Symfony\Security\State\AccessCheckerProvider') + $services->set('api_platform.state_provider.access_checker.post_deserialize', AccessCheckerProvider::class) ->decorate('api_platform.state_provider.deserialize', null, 0) ->args([ service('api_platform.state_provider.access_checker.post_deserialize.inner'), @@ -31,10 +34,18 @@ 'post_denormalize', ]); - $services->set('api_platform.state_provider.security_parameter', 'ApiPlatform\State\Provider\SecurityParameterProvider') + $services->set('api_platform.state_provider.security_parameter', SecurityParameterProvider::class) ->decorate('api_platform.state_provider.access_checker', null, 0) ->args([ service('api_platform.state_provider.security_parameter.inner'), service('api_platform.security.resource_access_checker'), ]); + + $services->set('api_platform.state_provider.access_checker.pre_read', AccessCheckerProvider::class) + ->decorate('api_platform.state_provider.read', null, 10) + ->args([ + service('api_platform.state_provider.access_checker.pre_read.inner'), + service('api_platform.security.resource_access_checker'), + 'pre_read', + ]); }; diff --git a/src/Symfony/Security/ObjectVariableCheckerInterface.php b/src/Symfony/Security/ObjectVariableCheckerInterface.php new file mode 100644 index 00000000000..0f7d8ba9ed6 --- /dev/null +++ b/src/Symfony/Security/ObjectVariableCheckerInterface.php @@ -0,0 +1,23 @@ + + * + * 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; + +interface ObjectVariableCheckerInterface +{ + /** + * @param string $expression a Expression Language string + * @param array $variables + */ + public function usesObjectVariable(string $expression, array $variables = []): bool; +} diff --git a/src/Symfony/Security/ResourceAccessChecker.php b/src/Symfony/Security/ResourceAccessChecker.php index 7a317c3669a..e12087a3a7e 100644 --- a/src/Symfony/Security/ResourceAccessChecker.php +++ b/src/Symfony/Security/ResourceAccessChecker.php @@ -15,6 +15,8 @@ use ApiPlatform\Metadata\ResourceAccessCheckerInterface; use Symfony\Component\ExpressionLanguage\ExpressionLanguage; +use Symfony\Component\ExpressionLanguage\Node\NameNode; +use Symfony\Component\ExpressionLanguage\Node\Node; use Symfony\Component\Security\Core\Authentication\AuthenticationTrustResolverInterface; use Symfony\Component\Security\Core\Authentication\Token\NullToken; use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface; @@ -27,7 +29,7 @@ * * @author Kévin Dunglas */ -final class ResourceAccessChecker implements ResourceAccessCheckerInterface +final class ResourceAccessChecker implements ResourceAccessCheckerInterface, ObjectVariableCheckerInterface { 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) { @@ -43,18 +45,12 @@ 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.'); } - $variables = array_merge($extraVariables, [ - 'trust_resolver' => $this->authenticationTrustResolver, - 'auth_checker' => $this->authorizationChecker, // needed for the is_granted expression function - ]); - - if (null === $token = $this->tokenStorage->getToken()) { - $token = new NullToken(); - } - - $variables = array_merge($variables, $this->getVariables($token)); + return (bool) $this->expressionLanguage->evaluate($expression, $this->getVariables($extraVariables)); + } - return (bool) $this->expressionLanguage->evaluate($expression, $variables); + public function usesObjectVariable(string $expression, array $variables = []): bool + { + return $this->hasObjectVariable($this->expressionLanguage->parse($expression, array_keys($this->getVariables($variables)))->getNodes()->toArray()); } /** @@ -62,13 +58,19 @@ public function isGranted(string $resourceClass, string $expression, array $extr * * @see https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Security/Core/Authorization/Voter/ExpressionVoter.php */ - private function getVariables(TokenInterface $token): array + private function getVariables(array $variables): array { - return [ + if (null === $token = $this->tokenStorage->getToken()) { + $token = new NullToken(); + } + + return array_merge($variables, [ 'token' => $token, 'user' => $token->getUser(), 'roles' => $this->getEffectiveRoles($token), - ]; + 'trust_resolver' => $this->authenticationTrustResolver, + 'auth_checker' => $this->authorizationChecker, // needed for the is_granted expression function + ]); } /** @@ -82,4 +84,44 @@ private function getEffectiveRoles(TokenInterface $token): array return $this->roleHierarchy->getReachableRoleNames($token->getRoleNames()); } + + /** + * Recursively checks if a variable named 'object' is present in the expression AST. + * + * @param Node|array|null $nodeOrNodes the ExpressionLanguage Node instance or an array of nodes/values + */ + private function hasObjectVariable(Node|array|null $nodeOrNodes): bool + { + if ($nodeOrNodes instanceof NameNode) { + if ('object' === $nodeOrNodes->attributes['name'] || 'previous_object' === $nodeOrNodes->attributes['name']) { + return true; + } + + return false; + } + + if ($nodeOrNodes instanceof Node) { + foreach ($nodeOrNodes->nodes as $childNode) { + if ($this->hasObjectVariable($childNode)) { + return true; + } + } + + return false; + } + + if (\is_array($nodeOrNodes)) { + foreach ($nodeOrNodes as $element) { + if (\is_string($element)) { + continue; + } + + if ($this->hasObjectVariable($element)) { + return true; + } + } + } + + return false; + } } diff --git a/src/Symfony/Security/State/AccessCheckerProvider.php b/src/Symfony/Security/State/AccessCheckerProvider.php index 7313d7d125d..af86387c1f9 100644 --- a/src/Symfony/Security/State/AccessCheckerProvider.php +++ b/src/Symfony/Security/State/AccessCheckerProvider.php @@ -21,6 +21,7 @@ use ApiPlatform\Metadata\ResourceAccessCheckerInterface; use ApiPlatform\State\ProviderInterface; use ApiPlatform\Symfony\Security\Exception\AccessDeniedException; +use ApiPlatform\Symfony\Security\ObjectVariableCheckerInterface; use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException; /** @@ -59,15 +60,15 @@ public function provide(Operation $operation, array $uriVariables = [], array $c $message = $operation->getSecurityMessage(); } - $body = $this->decorated->provide($operation, $uriVariables, $context); - if (null === $isGranted) { - return $body; + if ( + null === $isGranted + // On a GraphQl QueryCollection we want to perform security stage only on the top-level query + || ($operation instanceof QueryCollection && null !== ($context['source'] ?? null)) + ) { + return $this->decorated->provide($operation, $uriVariables, $context); } - // On a GraphQl QueryCollection we want to perform security stage only on the top-level query - if ($operation instanceof QueryCollection && null !== ($context['source'] ?? null)) { - return $body; - } + $body = 'pre_read' === $this->event ? null : $this->decorated->provide($operation, $uriVariables, $context); if ($operation instanceof HttpOperation) { $request = $context['request'] ?? null; @@ -84,10 +85,14 @@ public function provide(Operation $operation, array $uriVariables = [], array $c ]; } + if ('pre_read' === $this->event && $this->resourceAccessChecker instanceof ObjectVariableCheckerInterface && $this->resourceAccessChecker->usesObjectVariable($isGranted, $resourceAccessCheckerContext)) { + 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.'); } - return $body; + return 'pre_read' === $this->event ? $this->decorated->provide($operation, $uriVariables, $context) : $body; } } diff --git a/tests/Fixtures/TestBundle/ApiResource/IsGrantedTestResource.php b/tests/Fixtures/TestBundle/ApiResource/IsGrantedTestResource.php new file mode 100644 index 00000000000..51942b2991a --- /dev/null +++ b/tests/Fixtures/TestBundle/ApiResource/IsGrantedTestResource.php @@ -0,0 +1,44 @@ + + * + * 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\Fixtures\TestBundle\ApiResource; + +use ApiPlatform\Metadata\ApiResource; +use ApiPlatform\Metadata\Get; +use ApiPlatform\Metadata\Operation; + +#[ApiResource( + operations: [ + new Get(uriTemplate: 'is_granted_tests/{id}', security: 'is_granted("ROLE_ADMIN")', uriVariables: ['id'], provider: [self::class, 'provide']), + new Get(uriTemplate: 'is_granted_test_call_provider/{id}', uriVariables: ['id'], security: 'is_granted("ROLE_ADMIN")', provider: [self::class, 'provideShouldNotBeCalled']), + ] +)] +class IsGrantedTestResource +{ + private ?int $id = null; + + public function getId(): ?int + { + return $this->id; + } + + public static function provide(Operation $operation, array $uriVariables = [], array $context = []) + { + return new self(); + } + + public static function provideShouldNotBeCalled(Operation $operation, array $uriVariables = [], array $context = []) + { + throw new \RuntimeException('provider should not get called'); + } +} diff --git a/tests/Functional/IsGrantedTest.php b/tests/Functional/IsGrantedTest.php new file mode 100644 index 00000000000..5f446512269 --- /dev/null +++ b/tests/Functional/IsGrantedTest.php @@ -0,0 +1,68 @@ + + * + * 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\Functional; + +use ApiPlatform\Symfony\Bundle\Test\ApiTestCase; +use ApiPlatform\Tests\Fixtures\TestBundle\ApiResource\IsGrantedTestResource; +use ApiPlatform\Tests\SetupClassResourcesTrait; +use Symfony\Component\Security\Core\User\InMemoryUser; + +final class IsGrantedTest extends ApiTestCase +{ + use SetupClassResourcesTrait; + + protected static ?bool $alwaysBootKernel = false; + + /** + * @return class-string[] + */ + public static function getResources(): array + { + return [IsGrantedTestResource::class]; + } + + public function testGetIsGrantedAsAdmin(): void + { + $client = self::createClient(); + $client->loginUser(new InMemoryUser('admin', 'password', ['ROLE_ADMIN'])); + + $client->request('GET', '/is_granted_tests/1'); + $this->assertResponseIsSuccessful(); + } + + public function testGetIsGrantedAsUser(): void + { + $client = self::createClient(); + $client->loginUser(new InMemoryUser('user', 'password', ['ROLE_USER'])); + + $client->request('GET', '/is_granted_tests/1'); + $this->assertResponseStatusCodeSame(403); + } + + public function testGetIsGrantedAsAnonymous(): void + { + $client = self::createClient(); + + $client->request('GET', '/is_granted_tests/1'); + $this->assertResponseStatusCodeSame(401); + } + + public function testGetIsGrantedShouldNotCallProvider(): void + { + $client = self::createClient(); + + $client->request('GET', '/is_granted_test_call_provider/1'); + $this->assertResponseStatusCodeSame(401); + } +}