Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions src/Symfony/Bundle/Resources/config/state/security.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,28 +13,39 @@

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'),
service('api_platform.security.resource_access_checker'),
'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',
]);
};
23 changes: 23 additions & 0 deletions src/Symfony/Security/ObjectVariableCheckerInterface.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<?php

/*
* This file is part of the API Platform project.
*
* (c) Kévin Dunglas <[email protected]>
*
* 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<string, mixed> $variables
*/
public function usesObjectVariable(string $expression, array $variables = []): bool;
}
72 changes: 57 additions & 15 deletions src/Symfony/Security/ResourceAccessChecker.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -27,7 +29,7 @@
*
* @author Kévin Dunglas <[email protected]>
*/
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)
{
Expand All @@ -43,32 +45,32 @@ 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());
}

/**
* @copyright Fabien Potencier <[email protected]>
*
* @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
]);
}

/**
Expand All @@ -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<mixed>|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;
}
}
21 changes: 13 additions & 8 deletions src/Symfony/Security/State/AccessCheckerProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -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;
Expand All @@ -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)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if ('pre_read' === $this->event && $this->resourceAccessChecker instanceof ObjectVariableCheckerInterface && $this->resourceAccessChecker->usesObjectVariable($isGranted, $resourceAccessCheckerContext)) {
if ($preRead && $this->resourceAccessChecker instanceof ObjectVariableCheckerInterface && $this->resourceAccessChecker->usesObjectVariable($isGranted, $resourceAccessCheckerContext)) {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, there is likely a missing test, because the missing use statement hasn't been caught.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it did fail on behat :)

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;
}
}
44 changes: 44 additions & 0 deletions tests/Fixtures/TestBundle/ApiResource/IsGrantedTestResource.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
<?php

/*
* This file is part of the API Platform project.
*
* (c) Kévin Dunglas <[email protected]>
*
* 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']),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the test really working? Because I expected object in the expression where provider is called before security check

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this test is working, when no object is present in the security check it'll be called before the provider. If object is present it'll be called after.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, that's on me then. I expected the test to work a little bit differently and I was looking for tests both with and without object in security to see that it behaves correctly in all cases

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh we already have a bunch of tests with object.

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');
}
}
68 changes: 68 additions & 0 deletions tests/Functional/IsGrantedTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
<?php

/*
* This file is part of the API Platform project.
*
* (c) Kévin Dunglas <[email protected]>
*
* 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);
}
}
Loading