Skip to content

feat(symfony): expose voter reasons - #8448

Open
nozarashi20 wants to merge 1 commit into
api-platform:mainfrom
nozarashi20:feat/symfony-voter-reasons
Open

feat(symfony): expose voter reasons#8448
nozarashi20 wants to merge 1 commit into
api-platform:mainfrom
nozarashi20:feat/symfony-voter-reasons

Conversation

@nozarashi20

@nozarashi20 nozarashi20 commented Aug 12, 2026

Copy link
Copy Markdown
Q A
Branch? main
Tickets Closes #7331
License MIT
Doc PR n/a

This PR exposes reasons provided by Symfony voters in denied HTTP responses when %kernel.debug% is enabled. GraphQL behavior is unchanged.

A fresh AccessDecision is captured for every is_granted() call without changing the result of the security expression.

An explicit security message configured on the operation still takes priority. Otherwise, the voter reason is used as the response detail in debug mode. When debug mode is disabled, the response remains generic (Access Denied.), while the voter reason is preserved in the exception message so that it is available to Symfony's exception logging.

ResourceAccessCheckerInterface is unchanged. Existing custom resource access checkers continue to work and fall back to Access Denied. when no captured voter reason is available.

@nozarashi20
nozarashi20 force-pushed the feat/symfony-voter-reasons branch 3 times, most recently from a5d5268 to ba9f7eb Compare August 17, 2026 14:20
@nozarashi20
nozarashi20 force-pushed the feat/symfony-voter-reasons branch from ba9f7eb to 0ed0686 Compare August 24, 2026 14:41

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.

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.

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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Security] Support voter reasons in the "access denied" responses

2 participants