Skip to content

feat(symfony): expose voter reasons #8448

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nozarashi20 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(symfony): expose voter reasons #8448

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion src/Symfony/Bundle/Resources/config/security.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');

Expand Down
9 changes: 6 additions & 3 deletions src/Symfony/Bundle/Resources/config/state/security.php
View file
Open in desktop

@soyuka soyuka Sep 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wiring %kernel.debug% into four provider definitions duplicates a decision we already make in exactly one place: ErrorProvider (src/State/ErrorProvider.php) already receives $debug and already scrubs the detail so we don't leak internals in prod.

If a null detail on the exception means "no developer-configured message, safe to scrub", the whole debug gate fits as one extra rule there and none of these providers needs to know about kernel.debug. The two strings on the exception are unavoidable either way, since a configured securityMessage has to stay visible in production — but the flag doesn't have to travel through the security providers.

Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,17 @@
->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)
->args([
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)
Expand All @@ -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%');
};
3 changes: 2 additions & 1 deletion src/Symfony/Bundle/Resources/config/state/security_validator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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%');
};
47 changes: 47 additions & 0 deletions src/Symfony/Security/AccessDecisionCapturingAuthorizationChecker.php
View file
Open in desktop

@soyuka soyuka Sep 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this whole class can go away. We already own the is_granted expression function — ApiPlatform\Symfony\Security\Core\Authorization\ExpressionLanguageProvider is tagged security.expression_language_provider, and Symfony's ExpressionLanguage prepends its own provider "to let users override it easily", so ours wins. Forwarding the third argument there is enough:

// evaluator
static fn (array $variables, $attributes, $object = null) => $variables['auth_checker']->isGranted($attributes, $object, $variables['access_decision'] ?? null)
// compiler
static fn ($attributes, $object = 'null'): string => sprintf('$auth_checker->isGranted(%s, %s, $access_decision ?? null)', $attributes, $object)

with getVariables() exposing 'access_decision' => $accessDecision. The ?? null matters: that provider is also used for Symfony's own access_control and ExpressionVoter expressions, which won't define the variable — AuthorizationChecker::isGranted() then falls back to its own accessDecisionStack, i.e. exactly today's behaviour.

That removes this class and its 242-line test. One behaviour delta worth calling out: with a single decision shared per expression, is_granted('A') or is_granted('B') with both denied reports both reasons, since AccessDecisionManager appends to $accessDecision->votes and getMessage() filters by the final verdict. I think that's better output than keeping only the last reason, but it does mean testItSelectsOnlyTheLastIndependentDeniedDecision has to be rewritten.

Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
<?php

/*
* This file is part of the API Platform project.
*
* (c) Kévin Dunglas <dunglas@gmail.com>
*
* 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();
}
}
22 changes: 22 additions & 0 deletions src/Symfony/Security/AccessDeniedMessageProviderInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
<?php

/*
* This file is part of the API Platform project.
*
* (c) Kévin Dunglas <dunglas@gmail.com>
*
* 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;
}
37 changes: 34 additions & 3 deletions src/Symfony/Security/Exception/AccessDeniedException.php
View file
Open in desktop

@soyuka soyuka Sep 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This class is deprecated since 4.4, so I'd rather not grow it with ProblemExceptionInterface and a new detail argument. SecurityParameterProvider already prefers ApiPlatform\Metadata\Exception\AccessDeniedException when it exists — new behaviour belongs there, and this one should keep only the deprecation shim.

Original file line number Diff line number Diff line change
Expand Up @@ -15,22 +15,53 @@

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);
}

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;
Expand Down
32 changes: 27 additions & 5 deletions src/Symfony/Security/ResourceAccessChecker.php
View file
Open in desktop

@soyuka soyuka Sep 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for this — exposing voter reasons is definitely something we want, and the contract you describe (explicit securityMessage wins, generic message in prod) is the right one. My concerns are all about how the reason travels from the checker to the provider.

The PR does two separable things: (1) capture the AccessDecision out of is_granted(), and (2) transport it from here to AccessCheckerProvider. I'd like both solved differently.

On the transport: getAccessDeniedMessage() + reset() + the kernel.reset tag turn this class into a shared mutable value holder, and it's a singleton used from six places — AbstractItemNormalizer (property security, called many times during serialization), SecurityParameterProvider, both AccessCheckerProviders, and the JSON-LD/HAL/JSON:API/GraphQL (de)normalizers. It happens to be correct today because you reset at the top of isGranted(), but correctness then depends on nobody ever calling the checker between the failing check and the throw. That's the kind of temporal coupling I'd rather not add to a service with that many callers.

I'd prefer the caller to own the decision, the way Symfony itself did it. Keep ResourceAccessCheckerInterface::isGranted(): bool untouched and add a separate opt-in interface, exactly the way ObjectVariableCheckerInterface already sits next to it:

interface AccessDecisionAwareResourceAccessCheckerInterface
{
    public function decide(string $resourceClass, string $expression, array $extraVariables = []): AccessDecision;
}

isGranted() then becomes $this->decide(...)->isGranted, and AccessCheckerProvider does the same instanceof dance it already does for ObjectVariableCheckerInterface, falling back to the bool for third-party and Laravel checkers. No state, no reset(), no kernel.reset tag, no cross-request leak in worker runtimes, and SecurityParameterProvider can adopt it later for free — which the shared-slot design can't do consistently.

One trap to guard: AccessDecision::$isGranted has no default value, so an expression that never reaches auth_checker (object.owner == user) leaves it uninitialized and getMessage() will throw. Needs an isset() or $votes check.

Original file line number Diff line number Diff line change
Expand Up @@ -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 <dunglas@gmail.com>
*/
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.');
}
Expand All @@ -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
Expand All @@ -59,15 +81,15 @@ 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());
}

/**
* @copyright Fabien Potencier <fabien@symfony.com>
*
* @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();
Expand All @@ -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
]);
}

Expand Down
17 changes: 15 additions & 2 deletions src/Symfony/Security/State/AccessCheckerProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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)
{
}

Expand Down Expand Up @@ -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;
Expand Down
15 changes: 15 additions & 0 deletions src/Symfony/Tests/Bundle/DependencyInjection/ApiPlatformExtensionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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']);
Expand All @@ -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;
Expand Down
26 changes: 26 additions & 0 deletions src/Symfony/Tests/Security/Exception/AccessDeniedExceptionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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());
}
}
1 change: 1 addition & 0 deletions tests/Functional/IsGrantedTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading
Loading