feat(symfony): expose voter reasons - #8448
Conversation
a5d5268 to
ba9f7eb
Compare
ba9f7eb to
0ed0686
Compare
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This PR exposes reasons provided by Symfony voters in denied HTTP responses when
%kernel.debug%is enabled. GraphQL behavior is unchanged.A fresh
AccessDecisionis captured for everyis_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.ResourceAccessCheckerInterfaceis unchanged. Existing custom resource access checkers continue to work and fall back toAccess Denied.when no captured voter reason is available.