From 72d7bfa0f05b399455969aa1e6285be393c7b6f7 Mon Sep 17 00:00:00 2001 From: Jacob Thomason Date: Wed, 29 Jul 2026 04:07:02 -0400 Subject: [PATCH 1/4] feat: accept PHP callables as #[Security] rules #[Security] gains a `rule` parameter taking a callable that receives a SecurityRuleContext and returns bool. Accepted forms: array callable (a non-static method resolves through the container), invokable object for parameterised rules, and on PHP 8.5 first-class callables and static closures. Unlike an expression, a rule is refactor-safe, debuggable and unit testable on its own. Expressions remain fully supported, and are hardened alongside: they are now parsed at schema build rather than failing on first resolution, and must evaluate to bool like rules do. Adds CallableResolver, which normalises every form to a Closure once at schema build; #[Prefetch] now shares it and accepts invokable objects and Closures too. Claude-Session: https://claude.ai/code/session_0156c2BrkiiJtVZkF5dkPgC6 --- src/Annotations/Prefetch.php | 12 +- src/Annotations/Security.php | 64 ++++- src/CallableResolver.php | 131 +++++++++ src/InvalidCallableRuntimeException.php | 35 +++ .../BadExpressionInSecurityException.php | 13 + .../NonBooleanSecurityResultException.php | 60 ++++ src/Middlewares/SecurityFieldMiddleware.php | 163 +++++++++-- .../SecurityInputFieldMiddleware.php | 149 ++++++++-- src/ParameterizedCallableResolver.php | 48 ++-- src/SchemaFactory.php | 8 +- src/Security/SecurityRuleContext.php | 75 +++++ tests/AbstractQueryProvider.php | 4 +- tests/Annotations/SecurityTest.php | 80 +++++- tests/CallableResolverTest.php | 168 +++++++++++ tests/FieldsBuilderTest.php | 17 +- .../Controllers/SecurityController.php | 107 +++++++ tests/Fixtures/PageSizeWithin.php | 31 +++ tests/Fixtures/SecretIs.php | 26 ++ tests/Integration/EndToEndTest.php | 262 ++++++++++++++++++ tests/Integration/IntegrationTestCase.php | 8 +- .../SecurityFieldMiddlewareTest.php | 243 ++++++++++++++++ tests/ParameterizedCallableResolverTest.php | 45 ++- tests/Security/SecurityRuleContextTest.php | 114 ++++++++ website/docs/attributes-reference.md | 23 +- website/docs/fine-grained-security.mdx | 241 +++++++++++++--- website/docs/implementing-security.md | 9 + website/docs/prefetch-method.mdx | 13 +- 27 files changed, 2002 insertions(+), 147 deletions(-) create mode 100644 src/CallableResolver.php create mode 100644 src/Middlewares/NonBooleanSecurityResultException.php create mode 100644 src/Security/SecurityRuleContext.php create mode 100644 tests/CallableResolverTest.php create mode 100644 tests/Fixtures/PageSizeWithin.php create mode 100644 tests/Fixtures/SecretIs.php create mode 100644 tests/Middlewares/SecurityFieldMiddlewareTest.php create mode 100644 tests/Security/SecurityRuleContextTest.php diff --git a/src/Annotations/Prefetch.php b/src/Annotations/Prefetch.php index f4fa3bbeb7..e65a2d4ac8 100644 --- a/src/Annotations/Prefetch.php +++ b/src/Annotations/Prefetch.php @@ -10,8 +10,16 @@ #[Attribute(Attribute::TARGET_PARAMETER)] class Prefetch implements ParameterAnnotationInterface { - /** @param string|(callable&array{class-string, string}) $callable */ - public function __construct(public readonly string|array $callable) + /** + * @param string|(callable&array{class-string, string})|object $callable The prefetch method: a bare method name on + * the declaring class, an array callable, an + * invokable object, or — from PHP 8.5 — + * first-class callable syntax. A non-static + * method named by the array form is resolved + * through the container; first-class callable + * syntax cannot express that form. + */ + public function __construct(public readonly string|array|object $callable) { } diff --git a/src/Annotations/Security.php b/src/Annotations/Security.php index 13db0c93db..ef5a5b532a 100644 --- a/src/Annotations/Security.php +++ b/src/Annotations/Security.php @@ -6,6 +6,7 @@ use Attribute; use BadMethodCallException; +use TheCodingMachine\GraphQLite\GraphQLRuntimeException; use function array_key_exists; use function is_string; @@ -13,14 +14,26 @@ #[Attribute(Attribute::TARGET_PROPERTY | Attribute::TARGET_METHOD | Attribute::IS_REPEATABLE)] class Security implements MiddlewareAnnotationInterface { - private string $expression; + private string|null $expression = null; + + /** @var array{class-string, string}|object|null */ + private array|object|null $rule = null; + private mixed $failWith; private bool $failWithIsSet = false; private int $statusCode; private string $message; /** - * @param array|string $data data array managed by the Doctrine Annotations library or the expression + * @param array|string $data data array managed by the Doctrine Annotations library or the expression + * @param string|null $expression A security expression, evaluated by Symfony ExpressionLanguage. Suits short, + * local predicates. Prefer $rule for logic worth typing, testing or reusing. + * @param array{class-string, string}|object|null $rule A callable receiving a + * {@see \TheCodingMachine\GraphQLite\Security\SecurityRuleContext} + * and returning bool. Accepts an array callable, an invokable + * object, and — from PHP 8.5 — first-class callable syntax + * or an inline static closure. A non-static method named by + * the array form is resolved through the container. * * @throws BadMethodCallException */ @@ -30,17 +43,33 @@ public function __construct( mixed $failWith = '__fail__with__magic__key__', string|null $message = null, int|null $statusCode = null, + array|object|null $rule = null, ) { if (is_string($data)) { $data = ['expression' => $data]; } $expression = $data['value'] ?? $data['expression'] ?? $expression; + $rule = $data['rule'] ?? $rule; + + // An empty expression has always counted as "no expression"; keep that. if (! $expression) { - throw new BadMethodCallException('The #[Security] attribute must be passed an expression. For instance: "#[Security("is_granted(\'CAN_EDIT_STUFF\')")]"'); + $expression = null; + } + + // Silent precedence would turn a half-finished migration into a security change: whichever + // of the two lost would simply stop being enforced, with nothing in the schema or the + // response saying so. #[Security] is repeatable, so two checks means two attributes. + if ($expression !== null && $rule !== null) { + throw new BadMethodCallException('A #[Security] attribute cannot be passed both an expression and a rule. #[Security] is repeatable: declare one attribute per check.'); + } + + if ($expression === null && $rule === null) { + throw new BadMethodCallException('The #[Security] attribute must be passed an expression or a rule. For instance: "#[Security(rule: [MyRules::class, \'canEditStuff\'])]"'); } $this->expression = $expression; + $this->rule = $rule; if (array_key_exists('failWith', $data)) { $this->failWith = $data['failWith']; @@ -56,11 +85,40 @@ public function __construct( } } + /** + * Whether this annotation carries an expression rather than a rule. + * + * Custom middlewares reading #[Security] annotations should branch on this, or on + * {@see getRule()}, before calling {@see getExpression()}. + */ + public function hasExpression(): bool + { + return $this->expression !== null; + } + + /** @throws GraphQLRuntimeException When this annotation carries a rule instead of an expression. */ public function getExpression(): string { + if ($this->expression === null) { + throw new GraphQLRuntimeException('This #[Security] attribute carries a rule, not an expression. Call getRule() instead, or check hasExpression() first.'); + } + return $this->expression; } + /** + * The callable to invoke, or null when this annotation carries an expression. + * + * The value is returned exactly as it was written in the attribute; turning it into a Closure + * is the middleware's job, so that it happens once, at schema-build time. + * + * @return array{class-string, string}|object|null + */ + public function getRule(): array|object|null + { + return $this->rule; + } + public function isFailWithSet(): bool { return $this->failWithIsSet; diff --git a/src/CallableResolver.php b/src/CallableResolver.php new file mode 100644 index 0000000000..43111d4ebf --- /dev/null +++ b/src/CallableResolver.php @@ -0,0 +1,131 @@ +isPublic()) { + throw InvalidCallableRuntimeException::methodNotPublic($className, $methodName); + } + + if ($refMethod->isStatic()) { + return [$refMethod->getClosure(), $refMethod]; + } + + // A non-static method is resolved through the container at invocation time, so the service + // keeps its normal lifecycle instead of being built while the schema is compiled. + $container = $this->container; + $closure = static fn (mixed ...$args): mixed => $container->get($className)->$methodName(...$args); + + return [$closure, $refMethod]; + } + + /** + * @return array{Closure, ReflectionFunctionAbstract} + * + * @throws InvalidCallableRuntimeException + */ + private static function resolveInvokable(object $callable): array + { + try { + $refMethod = new ReflectionMethod($callable, '__invoke'); + } catch (ReflectionException $e) { + throw InvalidCallableRuntimeException::notInvokable($callable::class, $e); + } + + if (! $refMethod->isPublic()) { + throw InvalidCallableRuntimeException::methodNotPublic($callable::class, '__invoke'); + } + + return [$refMethod->getClosure($callable), $refMethod]; + } + + /** + * Recovers the method a Closure came from, when there is one. + * + * First-class callable syntax (`Foo::bar(...)`) produces a Closure that still reports its + * originating method's name, scope and docblock, so callers that need to reflect the target, + * such as {@see ParameterizedCallableResolver}, keep working exactly as they do for the array + * form. An inline closure has no such origin and reflects as a plain function. + */ + private static function reflectClosure(Closure $closure): ReflectionFunctionAbstract + { + $refFunction = new ReflectionFunction($closure); + $scope = $refFunction->getClosureScopeClass(); + + if ($scope !== null && $scope->hasMethod($refFunction->getName())) { + return $scope->getMethod($refFunction->getName()); + } + + return $refFunction; + } +} diff --git a/src/InvalidCallableRuntimeException.php b/src/InvalidCallableRuntimeException.php index f35006c8c5..744e990910 100644 --- a/src/InvalidCallableRuntimeException.php +++ b/src/InvalidCallableRuntimeException.php @@ -12,4 +12,39 @@ public static function methodNotFound(string $className, string $methodName, Thr { return new self('Method ' . $className . '::' . $methodName . " wasn't found or isn't accessible.", 0, $previous); } + + public static function methodNotPublic(string $className, string $methodName): self + { + return new self( + 'Method ' . $className . '::' . $methodName . ' must be public to be used as a callable. On PHP 8.5 and later, ' + . 'first-class callable syntax written inside the declaring class keeps the class scope and can therefore name a ' + . 'private method: ' . $className . '::' . $methodName . '(...).', + ); + } + + public static function notInvokable(string $className, Throwable|null $previous = null): self + { + return new self( + 'Object of class ' . $className . ' cannot be used as a callable because it has no __invoke() method.', + 0, + $previous, + ); + } + + public static function noClassContext(string $methodName): self + { + return new self( + 'The bare method name "' . $methodName . '" cannot be resolved because no class context was provided. ' + . 'Name the class explicitly instead, for instance [MyRules::class, "' . $methodName . '"].', + ); + } + + public static function notAMethod(): self + { + return new self( + 'The callable must name a real method, because its parameters are mapped to GraphQL arguments and their ' + . 'descriptions are read from the method docblock. An inline closure has no such method. Use an array ' + . 'callable, an invokable object, or first-class callable syntax on PHP 8.5 and later.', + ); + } } diff --git a/src/Middlewares/BadExpressionInSecurityException.php b/src/Middlewares/BadExpressionInSecurityException.php index 31964d0d0c..05a8c2d80c 100644 --- a/src/Middlewares/BadExpressionInSecurityException.php +++ b/src/Middlewares/BadExpressionInSecurityException.php @@ -21,4 +21,17 @@ public static function wrapException(Throwable $e, QueryFieldDescriptor|InputFie return new self($message, $e->getCode(), $e); } + + /** + * Raised while the schema is built, so a malformed expression is a startup error naming the + * field rather than a surprise inside a resolver on some later request. + */ + public static function fromSyntaxError(Throwable $e, QueryFieldDescriptor|InputFieldDescriptor $fieldDescriptor, string $expression): self + { + $originalResolver = $fieldDescriptor->getOriginalResolver(); + $message = 'The expression in the #[Security] attribute of "' . $originalResolver->toString() . '" is not valid: ' + . $e->getMessage() . ' Expression: "' . $expression . '".'; + + return new self($message, $e->getCode(), $e); + } } diff --git a/src/Middlewares/NonBooleanSecurityResultException.php b/src/Middlewares/NonBooleanSecurityResultException.php new file mode 100644 index 0000000000..be066c49f4 --- /dev/null +++ b/src/Middlewares/NonBooleanSecurityResultException.php @@ -0,0 +1,60 @@ +getOriginalResolver()->toString() . '" must return a bool, but returned ' + . get_debug_type($returned) . '.', + ); + } + + public static function fromExpression( + string $expression, + QueryFieldDescriptor|InputFieldDescriptor $fieldDescriptor, + mixed $returned, + ): self + { + return new self( + 'The #[Security] expression guarding "' . $fieldDescriptor->getOriginalResolver()->toString() + . '" must evaluate to a bool, but evaluated to ' . get_debug_type($returned) + . '. Compare explicitly, for instance "user !== null" rather than "user". Expression: "' + . $expression . '".', + ); + } + + /** @param array{class-string, string}|object|null $rule */ + private static function describe(array|object|null $rule): string + { + if (is_array($rule)) { + return implode('::', $rule) . '()'; + } + + return $rule === null ? '(unknown)' : $rule::class; + } +} diff --git a/src/Middlewares/SecurityFieldMiddleware.php b/src/Middlewares/SecurityFieldMiddleware.php index d6cdea541c..8bb7b54fde 100644 --- a/src/Middlewares/SecurityFieldMiddleware.php +++ b/src/Middlewares/SecurityFieldMiddleware.php @@ -4,16 +4,21 @@ namespace TheCodingMachine\GraphQLite\Middlewares; +use Closure; use GraphQL\Type\Definition\FieldDefinition; use GraphQL\Type\Definition\NonNull; use GraphQL\Type\Definition\OutputType; use Symfony\Component\ExpressionLanguage\ExpressionLanguage; +use Symfony\Component\ExpressionLanguage\SyntaxError; use TheCodingMachine\GraphQLite\Annotations\FailWith; use TheCodingMachine\GraphQLite\Annotations\Security; +use TheCodingMachine\GraphQLite\CallableResolver; +use TheCodingMachine\GraphQLite\InputFieldDescriptor; use TheCodingMachine\GraphQLite\Parameters\ParameterInterface; use TheCodingMachine\GraphQLite\QueryFieldDescriptor; use TheCodingMachine\GraphQLite\Security\AuthenticationServiceInterface; use TheCodingMachine\GraphQLite\Security\AuthorizationServiceInterface; +use TheCodingMachine\GraphQLite\Security\SecurityRuleContext; use Throwable; use function array_combine; @@ -21,16 +26,27 @@ use function array_slice; use function assert; use function count; +use function is_bool; /** * A field middleware that reads "Security" Symfony annotations. */ class SecurityFieldMiddleware implements FieldMiddlewareInterface { + /** + * Variable names getVariables() always supplies, regardless of the field's own parameters. + * + * Kept in step with getVariables() by testExpressionCanReferenceEveryProvidedVariable(); they cannot + * be derived from it, because building the real bag would call the authentication service at + * schema-build time. + */ + private const VARIABLE_NAMES = ['user', 'this', 'authorizationService', 'authenticationService']; + public function __construct( private readonly ExpressionLanguage $language, private readonly AuthenticationServiceInterface $authenticationService, private readonly AuthorizationServiceInterface $authorizationService, + private readonly CallableResolver $callableResolver, ) { } @@ -71,6 +87,12 @@ public function process( } } + // Rules are normalized to a Closure here, once, rather than per request: an unresolvable + // callable then fails at schema build with a class and method name in the message, and the + // resolver below never has to branch on the shape the attribute was given. + $this->lintExpressions($securityAnnotations, $queryFieldDescriptor->getParameters(), $queryFieldDescriptor); + $rules = $this->normalizeRules($securityAnnotations); + $resolver = $queryFieldDescriptor->getResolver(); $originalResolver = $queryFieldDescriptor->getOriginalResolver(); @@ -82,19 +104,39 @@ public function process( // source arg before zipping args to parameter names. $injectSource = $queryFieldDescriptor->isInjectSource(); - $queryFieldDescriptor = $queryFieldDescriptor->withResolver(function (object|null $source, ...$args) use ($originalResolver, $securityAnnotations, $resolver, $failWith, $parameters, $queryFieldDescriptor, $injectSource) { - $variables = $this->getVariables( - $args, - $parameters, - $injectSource ? $source : $originalResolver->executionSource($source), - $injectSource, - ); - - foreach ($securityAnnotations as $annotation) { - try { - $authorized = $this->language->evaluate($annotation->getExpression(), $variables); - } catch (Throwable $e) { - throw BadExpressionInSecurityException::wrapException($e, $queryFieldDescriptor); + $queryFieldDescriptor = $queryFieldDescriptor->withResolver(function (object|null $source, ...$args) use ($originalResolver, $securityAnnotations, $rules, $resolver, $failWith, $parameters, $queryFieldDescriptor, $injectSource) { + $executionSource = $injectSource ? $source : $originalResolver->executionSource($source); + $arguments = $this->getArguments($args, $parameters, $injectSource); + + foreach ($securityAnnotations as $index => $annotation) { + $rule = $rules[$index] ?? null; + + if ($rule !== null) { + $authorized = $rule(new SecurityRuleContext( + $this->authenticationService->getUser(), + $executionSource, + $arguments, + $this->authenticationService, + $this->authorizationService, + )); + + // An authorization check silently treating 0, '' or null as denial and the + // string 'false' as permission is not a behavior worth carrying over to rules. + if (! is_bool($authorized)) { + throw NonBooleanSecurityResultException::fromRule($annotation->getRule(), $queryFieldDescriptor, $authorized); + } + } else { + $expression = $annotation->getExpression(); + + try { + $authorized = $this->language->evaluate($expression, $this->getVariables($executionSource, $arguments)); + } catch (Throwable $e) { + throw BadExpressionInSecurityException::wrapException($e, $queryFieldDescriptor); + } + + if (! is_bool($authorized)) { + throw NonBooleanSecurityResultException::fromExpression($expression, $queryFieldDescriptor, $authorized); + } } if (! $authorized) { @@ -116,31 +158,102 @@ public function process( } /** + * Parses every expression while the schema is built. + * + * Without this a malformed expression only fails when the field is first resolved, so a typo + * ships and surfaces as a request-time error for whoever hits that field. The variable names + * handed to the linter are exactly those getVariables() will supply, so what lints here is + * what evaluates later. + * + * @param Security[] $annotations + * @param array $parameters + */ + private function lintExpressions(array $annotations, array $parameters, QueryFieldDescriptor|InputFieldDescriptor $descriptor): void + { + $names = [...self::VARIABLE_NAMES, ...array_keys($parameters)]; + + // Linting initialises the parser, and an ExpressionLanguage refuses further register() calls + // once that has happened. Linting a clone keeps every function the consumer registered + // visible to the linter while leaving the real instance open to registration afterwards, + // which is how it behaved before expressions were checked at build time. + $linter = clone $this->language; + + foreach ($annotations as $annotation) { + if (! $annotation->hasExpression()) { + continue; + } + + $expression = $annotation->getExpression(); + + try { + $linter->lint($expression, $names); + } catch (SyntaxError $e) { + throw BadExpressionInSecurityException::fromSyntaxError($e, $descriptor, $expression); + } + } + } + + /** + * Turns every rule-bearing annotation into a Closure, keyed by its position in $annotations. + * + * @param Security[] $annotations + * + * @return array + */ + private function normalizeRules(array $annotations): array + { + $rules = []; + + foreach ($annotations as $index => $annotation) { + $rule = $annotation->getRule(); + + if ($rule === null) { + continue; + } + + [$rules[$index]] = $this->callableResolver->resolve($rule); + } + + return $rules; + } + + /** + * The field's resolved PHP arguments, keyed by parameter name. + * * @param array $args * @param array $parameters * * @return array */ - private function getVariables(array $args, array $parameters, object|null $source, bool $injectSource = false): array + private function getArguments(array $args, array $parameters, bool $injectSource): array { - $variables = [ - // If a user is not logged, we provide an empty user object to make usage easier - 'user' => $this->authenticationService->getUser(), - 'authorizationService' => $this->authorizationService, // Used by the is_granted expression language function. - 'authenticationService' => $this->authenticationService, // Used by the is_logged expression language function. - 'this' => $source, - ]; - // Strip the source arg prepended by QueryField::fromFieldDescriptor so the remaining // user-supplied args line up positionally with the captured parameter names. The source - // is always exposed via `this`, so there's no loss of information for the expression. + // is always exposed separately, so there's no loss of information. if ($injectSource && count($args) > count($parameters)) { $args = array_slice($args, 1); } $argsName = array_keys($parameters); - $argsByName = $argsName ? array_combine($argsName, $args) : []; - return $variables + $argsByName; + return $argsName ? array_combine($argsName, $args) : []; + } + + /** + * @param array $arguments + * + * @return array + */ + private function getVariables(object|null $source, array $arguments): array + { + $variables = [ + // If a user is not logged, we provide an empty user object to make usage easier + 'user' => $this->authenticationService->getUser(), + 'authorizationService' => $this->authorizationService, // Used by the is_granted expression language function. + 'authenticationService' => $this->authenticationService, // Used by the is_logged expression language function. + 'this' => $source, + ]; + + return $variables + $arguments; } } diff --git a/src/Middlewares/SecurityInputFieldMiddleware.php b/src/Middlewares/SecurityInputFieldMiddleware.php index ab1da69d5a..464b4719e8 100644 --- a/src/Middlewares/SecurityInputFieldMiddleware.php +++ b/src/Middlewares/SecurityInputFieldMiddleware.php @@ -4,19 +4,25 @@ namespace TheCodingMachine\GraphQLite\Middlewares; +use Closure; use Symfony\Component\ExpressionLanguage\ExpressionLanguage; +use Symfony\Component\ExpressionLanguage\SyntaxError; use TheCodingMachine\GraphQLite\Annotations\Security; +use TheCodingMachine\GraphQLite\CallableResolver; use TheCodingMachine\GraphQLite\InputField; use TheCodingMachine\GraphQLite\InputFieldDescriptor; use TheCodingMachine\GraphQLite\Parameters\ParameterInterface; +use TheCodingMachine\GraphQLite\QueryFieldDescriptor; use TheCodingMachine\GraphQLite\Security\AuthenticationServiceInterface; use TheCodingMachine\GraphQLite\Security\AuthorizationServiceInterface; +use TheCodingMachine\GraphQLite\Security\SecurityRuleContext; use Throwable; use function array_combine; use function array_keys; use function array_slice; use function count; +use function is_bool; /** * A field input middleware that reads "Security" Symfony annotations. @@ -24,10 +30,20 @@ */ class SecurityInputFieldMiddleware implements InputFieldMiddlewareInterface { + /** + * Variable names getVariables() always supplies, regardless of the field's own parameters. + * + * Kept in step with getVariables() by testExpressionCanReferenceEveryProvidedVariable(); they cannot + * be derived from it, because building the real bag would call the authentication service at + * schema-build time. + */ + private const VARIABLE_NAMES = ['user', 'this', 'authorizationService', 'authenticationService']; + public function __construct( private readonly ExpressionLanguage $language, private readonly AuthenticationServiceInterface $authenticationService, private readonly AuthorizationServiceInterface $authorizationService, + private readonly CallableResolver $callableResolver, ) { } @@ -41,6 +57,10 @@ public function process(InputFieldDescriptor $inputFieldDescriptor, InputFieldHa return $inputFieldHandler->handle($inputFieldDescriptor); } + // Normalized once, at schema build. See the sibling SecurityFieldMiddleware. + $this->lintExpressions($securityAnnotations, $inputFieldDescriptor->getParameters(), $inputFieldDescriptor); + $rules = $this->normalizeRules($securityAnnotations); + $resolver = $inputFieldDescriptor->getResolver(); $originalResolver = $inputFieldDescriptor->getOriginalResolver(); @@ -50,19 +70,37 @@ public function process(InputFieldDescriptor $inputFieldDescriptor, InputFieldHa // sibling SecurityFieldMiddleware for the detailed comment. $injectSource = $inputFieldDescriptor->isInjectSource(); - $inputFieldDescriptor = $inputFieldDescriptor->withResolver(function (object|null $source, ...$args) use ($originalResolver, $securityAnnotations, $resolver, $parameters, $inputFieldDescriptor, $injectSource) { - $variables = $this->getVariables( - $args, - $parameters, - $injectSource ? $source : $originalResolver->executionSource($source), - $injectSource, - ); - - foreach ($securityAnnotations as $annotation) { - try { - $authorized = $this->language->evaluate($annotation->getExpression(), $variables); - } catch (Throwable $e) { - throw BadExpressionInSecurityException::wrapException($e, $inputFieldDescriptor); + $inputFieldDescriptor = $inputFieldDescriptor->withResolver(function (object|null $source, ...$args) use ($originalResolver, $securityAnnotations, $rules, $resolver, $parameters, $inputFieldDescriptor, $injectSource) { + $executionSource = $injectSource ? $source : $originalResolver->executionSource($source); + $arguments = $this->getArguments($args, $parameters, $injectSource); + + foreach ($securityAnnotations as $index => $annotation) { + $rule = $rules[$index] ?? null; + + if ($rule !== null) { + $authorized = $rule(new SecurityRuleContext( + $this->authenticationService->getUser(), + $executionSource, + $arguments, + $this->authenticationService, + $this->authorizationService, + )); + + if (! is_bool($authorized)) { + throw NonBooleanSecurityResultException::fromRule($annotation->getRule(), $inputFieldDescriptor, $authorized); + } + } else { + $expression = $annotation->getExpression(); + + try { + $authorized = $this->language->evaluate($expression, $this->getVariables($executionSource, $arguments)); + } catch (Throwable $e) { + throw BadExpressionInSecurityException::wrapException($e, $inputFieldDescriptor); + } + + if (! is_bool($authorized)) { + throw NonBooleanSecurityResultException::fromExpression($expression, $inputFieldDescriptor, $authorized); + } } if (! $authorized) { @@ -76,13 +114,87 @@ public function process(InputFieldDescriptor $inputFieldDescriptor, InputFieldHa return $inputFieldHandler->handle($inputFieldDescriptor); } + /** + * Parses every expression while the schema is built. + * + * Without this a malformed expression only fails when the field is first resolved, so a typo + * ships and surfaces as a request-time error for whoever hits that field. The variable names + * handed to the linter are exactly those getVariables() will supply, so what lints here is + * what evaluates later. + * + * @param Security[] $annotations + * @param array $parameters + */ + private function lintExpressions(array $annotations, array $parameters, QueryFieldDescriptor|InputFieldDescriptor $descriptor): void + { + $names = [...self::VARIABLE_NAMES, ...array_keys($parameters)]; + + // Linting initialises the parser, and an ExpressionLanguage refuses further register() calls + // once that has happened. Linting a clone keeps every function the consumer registered + // visible to the linter while leaving the real instance open to registration afterwards, + // which is how it behaved before expressions were checked at build time. + $linter = clone $this->language; + + foreach ($annotations as $annotation) { + if (! $annotation->hasExpression()) { + continue; + } + + $expression = $annotation->getExpression(); + + try { + $linter->lint($expression, $names); + } catch (SyntaxError $e) { + throw BadExpressionInSecurityException::fromSyntaxError($e, $descriptor, $expression); + } + } + } + + /** + * @param Security[] $annotations + * + * @return array + */ + private function normalizeRules(array $annotations): array + { + $rules = []; + + foreach ($annotations as $index => $annotation) { + $rule = $annotation->getRule(); + + if ($rule === null) { + continue; + } + + [$rules[$index]] = $this->callableResolver->resolve($rule); + } + + return $rules; + } + /** * @param array $args * @param array $parameters * * @return array */ - private function getVariables(array $args, array $parameters, object|null $source, bool $injectSource = false): array + private function getArguments(array $args, array $parameters, bool $injectSource): array + { + if ($injectSource && count($args) > count($parameters)) { + $args = array_slice($args, 1); + } + + $argsName = array_keys($parameters); + + return $argsName ? array_combine($argsName, $args) : []; + } + + /** + * @param array $arguments + * + * @return array + */ + private function getVariables(object|null $source, array $arguments): array { $variables = [ // If a user is not logged, we provide an empty user object to make usage easier @@ -92,13 +204,6 @@ private function getVariables(array $args, array $parameters, object|null $sourc 'this' => $source, ]; - if ($injectSource && count($args) > count($parameters)) { - $args = array_slice($args, 1); - } - - $argsName = array_keys($parameters); - $argsByName = $argsName ? array_combine($argsName, $args) : []; - - return $variables + $argsByName; + return $variables + $arguments; } } diff --git a/src/ParameterizedCallableResolver.php b/src/ParameterizedCallableResolver.php index a1abc33d69..fb77277e8f 100644 --- a/src/ParameterizedCallableResolver.php +++ b/src/ParameterizedCallableResolver.php @@ -4,57 +4,47 @@ namespace TheCodingMachine\GraphQLite; -use Psr\Container\ContainerInterface; +use Closure; use ReflectionClass; -use ReflectionException; use ReflectionMethod; use TheCodingMachine\GraphQLite\Parameters\ParameterInterface; -use function assert; -use function is_callable; -use function is_string; - class ParameterizedCallableResolver { public function __construct( private readonly FieldsBuilder $fieldsBuilder, - private readonly ContainerInterface $container, + private readonly CallableResolver $callableResolver, ) { } /** - * @param string|array{class-string, string} $callable + * Resolves a callable and maps the GraphQL parameters it declares. + * + * @param string|array{class-string, string}|object $callable + * @param class-string|ReflectionClass $classContext * - * @return array{callable, array} + * @return array{Closure, array} + * + * @throws InvalidCallableRuntimeException */ - public function resolve(string|array $callable, string|ReflectionClass $classContext, int $skip = 0): array + public function resolve(string|array|object $callable, string|ReflectionClass $classContext, int $skip = 0): array { if ($classContext instanceof ReflectionClass) { $classContext = $classContext->getName(); } - // If string method is given, it's equivalent to [self::class, 'method'] - if (is_string($callable)) { - $callable = [$classContext, $callable]; - } - - try { - $refMethod = new ReflectionMethod($callable[0], $callable[1]); - } catch (ReflectionException $e) { - throw InvalidCallableRuntimeException::methodNotFound($callable[0], $callable[1], $e); - } + [$resolved, $refFunction] = $this->callableResolver->resolve($callable, $classContext); - // If method isn't static, then we should try to resolve the class name through the container. - if (! $refMethod->isStatic()) { - $callable = fn (...$args) => $this->container->get($callable[0])->{$callable[1]}(...$args); + // Parameters become GraphQL arguments and their descriptions come from the method docblock, + // so the target has to be a real method. First-class callable syntax keeps that origin and + // reflects as a ReflectionMethod; an inline closure does not and cannot be mapped. + // Callers add their own context by catching InvalidCallableRuntimeException, the way + // PrefetchParameterMiddleware rewraps it as InvalidPrefetchMethodRuntimeException. + if (! $refFunction instanceof ReflectionMethod) { + throw InvalidCallableRuntimeException::notAMethod(); } - assert(is_callable($callable)); - - // Map all parameters of the callable. - $parameters = $this->fieldsBuilder->getParameters($refMethod, $skip); - - return [$callable, $parameters]; + return [$resolved, $this->fieldsBuilder->getParameters($refFunction, $skip)]; } } diff --git a/src/SchemaFactory.php b/src/SchemaFactory.php index 4aea95bb3e..87056d7298 100644 --- a/src/SchemaFactory.php +++ b/src/SchemaFactory.php @@ -408,6 +408,8 @@ public function createSchema(): Schema $expressionLanguage = $this->expressionLanguage ?: new ExpressionLanguage($symfonyCache); $expressionLanguage->registerProvider(new SecurityExpressionLanguageProvider()); + $callableResolver = new CallableResolver($this->container); + $directiveRegistry = new DirectiveRegistry($annotationReader); $directiveRegistry->discover(); @@ -418,7 +420,7 @@ public function createSchema(): Schema $fieldMiddlewarePipe->pipe($fieldMiddleware); } // TODO: add a logger to the SchemaFactory and make use of it everywhere (and most particularly in SecurityFieldMiddleware) - $fieldMiddlewarePipe->pipe(new SecurityFieldMiddleware($expressionLanguage, $authenticationService, $authorizationService)); + $fieldMiddlewarePipe->pipe(new SecurityFieldMiddleware($expressionLanguage, $authenticationService, $authorizationService, $callableResolver)); $fieldMiddlewarePipe->pipe(new AuthorizationFieldMiddleware($authenticationService, $authorizationService)); $fieldMiddlewarePipe->pipe(new CostFieldMiddleware()); $fieldMiddlewarePipe->pipe(new DirectiveFieldMiddleware($directiveAstBuilder)); @@ -428,7 +430,7 @@ public function createSchema(): Schema $inputFieldMiddlewarePipe->pipe($inputFieldMiddleware); } // TODO: add a logger to the SchemaFactory and make use of it everywhere (and most particularly in SecurityInputFieldMiddleware) - $inputFieldMiddlewarePipe->pipe(new SecurityInputFieldMiddleware($expressionLanguage, $authenticationService, $authorizationService)); + $inputFieldMiddlewarePipe->pipe(new SecurityInputFieldMiddleware($expressionLanguage, $authenticationService, $authorizationService, $callableResolver)); $inputFieldMiddlewarePipe->pipe(new AuthorizationInputFieldMiddleware($authenticationService, $authorizationService)); $inputFieldMiddlewarePipe->pipe(new DirectiveInputFieldMiddleware($directiveAstBuilder)); @@ -492,7 +494,7 @@ classBoundCache: $classBoundCache, $inputFieldMiddlewarePipe, $descriptionResolver, ); - $parameterizedCallableResolver = new ParameterizedCallableResolver($fieldsBuilder, $this->container); + $parameterizedCallableResolver = new ParameterizedCallableResolver($fieldsBuilder, $callableResolver); foreach ($this->parameterMiddlewares as $parameterMapper) { $parameterMiddlewarePipe->pipe($parameterMapper); diff --git a/src/Security/SecurityRuleContext.php b/src/Security/SecurityRuleContext.php new file mode 100644 index 0000000000..e0b852e210 --- /dev/null +++ b/src/Security/SecurityRuleContext.php @@ -0,0 +1,75 @@ + $arguments The field's resolved PHP arguments, keyed by parameter name. + */ + public function __construct( + public readonly object|null $user, + public readonly object|null $source, + public readonly array $arguments, + private readonly AuthenticationServiceInterface $authenticationService, + private readonly AuthorizationServiceInterface $authorizationService, + ) { + } + + /** + * Whether the current user holds $right, optionally against a specific subject. + * + * The rule-facing equivalent of the `is_granted()` expression function. + */ + public function isGranted(string $right, mixed $subject = null): bool + { + return $this->authorizationService->isAllowed($right, $subject); + } + + /** + * Whether anybody is currently logged in. + * + * The rule-facing equivalent of the `is_logged()` expression function. + */ + public function isLogged(): bool + { + return $this->authenticationService->isLogged(); + } + + /** + * The field argument named $name, or null when the field has no such argument. + */ + public function argument(string $name): mixed + { + return $this->arguments[$name] ?? null; + } + + /** + * Whether the field was passed an argument named $name. + * + * Distinguishes an argument that was genuinely null from one that does not exist, which + * {@see argument()} cannot. + */ + public function hasArgument(string $name): bool + { + return array_key_exists($name, $this->arguments); + } +} diff --git a/tests/AbstractQueryProvider.php b/tests/AbstractQueryProvider.php index 2d2c5e794c..31323e618e 100644 --- a/tests/AbstractQueryProvider.php +++ b/tests/AbstractQueryProvider.php @@ -4,6 +4,7 @@ namespace TheCodingMachine\GraphQLite; +use TheCodingMachine\GraphQLite\CallableResolver; use GraphQL\Type\Definition\InputObjectType; use GraphQL\Type\Definition\NamedType; use GraphQL\Type\Definition\ObjectType; @@ -319,6 +320,7 @@ protected function buildFieldsBuilder(): FieldsBuilder $expressionLanguage, new VoidAuthenticationService(), new VoidAuthorizationService(), + new CallableResolver($container), ), ); @@ -336,7 +338,7 @@ protected function buildFieldsBuilder(): FieldsBuilder $fieldMiddlewarePipe, $inputFieldMiddlewarePipe, ); - $parameterizedCallableResolver = new ParameterizedCallableResolver($fieldsBuilder, $container); + $parameterizedCallableResolver = new ParameterizedCallableResolver($fieldsBuilder, new CallableResolver($container)); $parameterMiddlewarePipe->pipe(new ResolveInfoParameterHandler()); $parameterMiddlewarePipe->pipe(new PrefetchParameterMiddleware($parameterizedCallableResolver)); diff --git a/tests/Annotations/SecurityTest.php b/tests/Annotations/SecurityTest.php index 3f82ae129d..23fb9dba1d 100644 --- a/tests/Annotations/SecurityTest.php +++ b/tests/Annotations/SecurityTest.php @@ -4,6 +4,7 @@ use BadMethodCallException; use PHPUnit\Framework\TestCase; +use TheCodingMachine\GraphQLite\GraphQLRuntimeException; class SecurityTest extends TestCase { @@ -11,7 +12,7 @@ class SecurityTest extends TestCase public function testBadParams(): void { $this->expectException(BadMethodCallException::class); - $this->expectExceptionMessage('The #[Security] attribute must be passed an expression. For instance: "#[Security("is_granted(\'CAN_EDIT_STUFF\')")]"'); + $this->expectExceptionMessage('The #[Security] attribute must be passed an expression or a rule. For instance: "#[Security(rule: [MyRules::class, \'canEditStuff\'])]"'); new Security([]); } @@ -21,4 +22,81 @@ public function testIncompatibleParams(): void $this->expectExceptionMessage('A #[Security] attribute that has "failWith" attribute set cannot have a message or a statusCode attribute.'); new Security(['expression'=>'foo', 'failWith'=>null, 'statusCode'=>500]); } + + public function testExpressionAndRuleAreMutuallyExclusive(): void + { + $this->expectException(BadMethodCallException::class); + $this->expectExceptionMessage('A #[Security] attribute cannot be passed both an expression and a rule. #[Security] is repeatable: declare one attribute per check.'); + new Security('foo', rule: ['SomeRules', 'canEditStuff']); + } + + public function testPositionalExpressionStillWorks(): void + { + $security = new Security("is_granted('CAN_EDIT_STUFF')"); + + self::assertTrue($security->hasExpression()); + self::assertSame("is_granted('CAN_EDIT_STUFF')", $security->getExpression()); + self::assertNull($security->getRule()); + } + + public function testDataValueFormStillWorks(): void + { + $security = new Security(['value' => 'foo']); + + self::assertSame('foo', $security->getExpression()); + } + + public function testDataExpressionFormStillWorks(): void + { + $security = new Security(['expression' => 'foo', 'message' => 'Nope', 'statusCode' => 401]); + + self::assertSame('foo', $security->getExpression()); + self::assertSame('Nope', $security->getMessage()); + self::assertSame(401, $security->getStatusCode()); + } + + public function testArrayCallableRuleIsReturnedIntact(): void + { + $security = new Security(rule: ['SomeRules', 'canEditStuff']); + + self::assertFalse($security->hasExpression()); + self::assertSame(['SomeRules', 'canEditStuff'], $security->getRule()); + } + + public function testInvokableObjectRuleIsReturnedIntact(): void + { + $rule = new class { + public function __invoke(): bool + { + return true; + } + }; + $security = new Security(rule: $rule); + + self::assertSame($rule, $security->getRule()); + } + + public function testRuleCanBePassedInTheDataArray(): void + { + $security = new Security(['rule' => ['SomeRules', 'canEditStuff']]); + + self::assertSame(['SomeRules', 'canEditStuff'], $security->getRule()); + } + + public function testGetExpressionThrowsOnARuleBearingAnnotation(): void + { + $security = new Security(rule: ['SomeRules', 'canEditStuff']); + + $this->expectException(GraphQLRuntimeException::class); + $this->expectExceptionMessage('This #[Security] attribute carries a rule, not an expression. Call getRule() instead, or check hasExpression() first.'); + $security->getExpression(); + } + + public function testFailWithIsUnaffectedByRules(): void + { + $security = new Security(rule: ['SomeRules', 'canEditStuff'], failWith: null); + + self::assertTrue($security->isFailWithSet()); + self::assertNull($security->getFailWith()); + } } diff --git a/tests/CallableResolverTest.php b/tests/CallableResolverTest.php new file mode 100644 index 0000000000..2acac7b7d2 --- /dev/null +++ b/tests/CallableResolverTest.php @@ -0,0 +1,168 @@ +resolver()->resolve([Contact::class, 'prefetchTheContacts']); + + self::assertInstanceOf(Closure::class, $closure); + self::assertSame(['test'], $closure(['test'])); + self::assertInstanceOf(ReflectionMethod::class, $reflection); + self::assertSame('prefetchTheContacts', $reflection->getName()); + } + + public function testResolvesBareMethodNameAgainstTheClassContext(): void + { + [$closure, $reflection] = $this->resolver()->resolve('prefetchTheContacts', Contact::class); + + self::assertSame(['test'], $closure(['test'])); + self::assertSame(Contact::class, $reflection->getDeclaringClass()->getName()); + } + + public function testBareMethodNameWithoutClassContextIsRejected(): void + { + $this->expectException(InvalidCallableRuntimeException::class); + $this->expectExceptionMessage('The bare method name "prefetchTheContacts" cannot be resolved because no class context was provided.'); + + $this->resolver()->resolve('prefetchTheContacts'); + } + + /** + * The case Closure::fromCallable() cannot handle on its own: it throws a TypeError for a + * non-static method, so the container has to be consulted instead. + */ + public function testResolvesNonStaticMethodThroughTheContainer(): void + { + $container = $this->createMock(ContainerInterface::class); + $container->expects($this->once()) + ->method('get') + ->with(FooExtendType::class) + ->willReturn(new FooExtendType()); + + [$closure, $reflection] = (new CallableResolver($container))->resolve([FooExtendType::class, 'customExtendedField']); + + self::assertSame('TEST', $closure(new TestObject('test'))); + self::assertSame('customExtendedField', $reflection->getName()); + } + + /** + * The container must not be touched while the schema is being built, only when the rule runs. + */ + public function testContainerIsNotConsultedUntilTheClosureIsInvoked(): void + { + $container = $this->createMock(ContainerInterface::class); + $container->expects($this->never())->method('get'); + + (new CallableResolver($container))->resolve([FooExtendType::class, 'customExtendedField']); + } + + public function testResolvesInvokableObject(): void + { + $invokable = new class { + public function __invoke(string $value): string + { + return 'invoked:' . $value; + } + }; + + [$closure, $reflection] = $this->resolver()->resolve($invokable); + + self::assertInstanceOf(Closure::class, $closure); + self::assertSame('invoked:x', $closure('x')); + self::assertInstanceOf(ReflectionMethod::class, $reflection); + self::assertSame('__invoke', $reflection->getName()); + } + + public function testInvokableObjectKeepsItsConstructorState(): void + { + $invokable = new class ('expected') { + public function __construct(private readonly string $expected) + { + } + + public function __invoke(string $value): bool + { + return $value === $this->expected; + } + }; + + [$closure] = $this->resolver()->resolve($invokable); + + self::assertTrue($closure('expected')); + self::assertFalse($closure('other')); + } + + public function testResolvesClosureUnchanged(): void + { + $closure = static fn (string $value): string => 'closure:' . $value; + + [$resolved, $reflection] = $this->resolver()->resolve($closure); + + self::assertSame($closure, $resolved); + self::assertInstanceOf(ReflectionFunction::class, $reflection); + } + + /** + * A Closure produced by Closure::fromCallable(), which is what first-class callable syntax + * yields on PHP 8.5, still knows the method it came from. Callers that need to reflect the + * target, such as ParameterizedCallableResolver, therefore keep working. + */ + public function testClosureFromACallableReflectsBackToItsMethod(): void + { + $closure = Closure::fromCallable([Contact::class, 'prefetchTheContacts']); + + [, $reflection] = $this->resolver()->resolve($closure); + + self::assertInstanceOf(ReflectionMethod::class, $reflection); + self::assertSame('prefetchTheContacts', $reflection->getName()); + self::assertSame(Contact::class, $reflection->getDeclaringClass()->getName()); + } + + public function testUnknownMethodIsRejectedAtResolutionTime(): void + { + $this->expectException(InvalidCallableRuntimeException::class); + $this->expectExceptionMessage('Method ' . Contact::class . '::doesntExist wasn\'t found or isn\'t accessible.'); + + $this->resolver()->resolve([Contact::class, 'doesntExist']); + } + + public function testNonPublicMethodIsRejectedAtResolutionTime(): void + { + $this->expectException(InvalidCallableRuntimeException::class); + $this->expectExceptionMessage('must be public to be used as a callable'); + + $this->resolver()->resolve([self::class, 'hiddenRule']); + } + + public function testNonInvokableObjectIsRejected(): void + { + $this->expectException(InvalidCallableRuntimeException::class); + $this->expectExceptionMessage('has no __invoke() method'); + + $this->resolver()->resolve(new TestObject('test')); + } + + private function resolver(): CallableResolver + { + return new CallableResolver(new EmptyContainer()); + } + + private static function hiddenRule(): bool + { + return true; + } +} + diff --git a/tests/FieldsBuilderTest.php b/tests/FieldsBuilderTest.php index 454a7a4df0..e386f56be7 100644 --- a/tests/FieldsBuilderTest.php +++ b/tests/FieldsBuilderTest.php @@ -791,23 +791,20 @@ public function testOutputTypeArgumentDescription(): void $this->assertSame('Test argument description', $testField->args[0]->description); } + /** + * A malformed expression is now rejected while the schema is built, rather than on the first + * request that happens to touch the field. + */ public function testSecurityBadQuery(): void { $controller = new TestControllerWithBadSecurity(); $queryProvider = $this->buildFieldsBuilder(); - $queries = $queryProvider->getQueries($controller); - - $this->assertCount(1, $queries); - $query = $queries['testBadSecurity']; - $this->assertSame('testBadSecurity', $query->name); - - $resolve = $query->resolveFn; - $this->expectException(BadExpressionInSecurityException::class); - $this->expectExceptionMessage('An error occurred while evaluating expression in @Security annotation of method "TheCodingMachine\GraphQLite\Fixtures\TestControllerWithBadSecurity::testBadSecurity()": Unexpected token "name" of value "is" around position 6 for expression `this is not valid expression language`.'); - $result = $resolve(new stdClass(), [], null, $this->createMock(ResolveInfo::class)); + $this->expectExceptionMessage('The expression in the #[Security] attribute of "TheCodingMachine\GraphQLite\Fixtures\TestControllerWithBadSecurity::testBadSecurity()" is not valid: Unexpected token "name" of value "is" around position 6 for expression `this is not valid expression language`. Expression: "this is not valid expression language".'); + + $queryProvider->getQueries($controller); } public function testQueryProviderWithNullableArray(): void diff --git a/tests/Fixtures/Integration/Controllers/SecurityController.php b/tests/Fixtures/Integration/Controllers/SecurityController.php index 47c9dc294e..181f7d3d1e 100644 --- a/tests/Fixtures/Integration/Controllers/SecurityController.php +++ b/tests/Fixtures/Integration/Controllers/SecurityController.php @@ -9,6 +9,9 @@ use TheCodingMachine\GraphQLite\Annotations\InjectUser; use TheCodingMachine\GraphQLite\Annotations\Query; use TheCodingMachine\GraphQLite\Annotations\Security; +use TheCodingMachine\GraphQLite\Fixtures\PageSizeWithin; +use TheCodingMachine\GraphQLite\Fixtures\SecretIs; +use TheCodingMachine\GraphQLite\Security\SecurityRuleContext; class SecurityController { @@ -55,6 +58,77 @@ public function getSecretUsingThis(string $secret): string return 'you can see this secret only if isAllowed() returns true'; } + #[Query] + #[Security(rule: [self::class, 'secretIsFoo'], message: 'Wrong secret passed')] + public function getSecretPhraseByRule(string $secret): string + { + return 'you can see this secret only if passed parameter is "foo"'; + } + + #[Query] + #[Security(rule: new SecretIs('foo'), failWith: null)] + public function getNullableSecretPhraseByRule(string $secret): string + { + return 'you can see this secret only if passed parameter is "foo"'; + } + + /** + * The rule's constructor argument is the limit being enforced: 100 here decides whether a + * given "first" is allowed, and nothing else in the attribute carries that number. + */ + #[Query] + #[Security(rule: new PageSizeWithin(100), statusCode: 400, message: 'Page size too large')] + public function getPagedSecret(int $first): string + { + return 'you can see this secret only if first is within the configured limit'; + } + + #[Query] + #[Security(rule: [self::class, 'userBarIs42'])] + public function getSecretUsingUserByRule(): string + { + return 'you can see this secret only if user.bar is set to 42'; + } + + #[Query] + #[Security(rule: [self::class, 'canEditAndIsLogged'])] + public function getSecretUsingIsGrantedByRule(): string + { + return 'you can see this secret only if user has right "CAN_EDIT"'; + } + + #[Query] + #[Security(rule: [self::class, 'sourceAllowsSecret'])] + public function getSecretUsingSourceByRule(string $secret): string + { + return 'you can see this secret only if isAllowed() returns true'; + } + + /** + * Both checks must pass, and they are evaluated in declaration order. + */ + #[Query] + #[Security("secret=='foo'")] + #[Security(rule: [self::class, 'userBarIs42'])] + public function getSecretUsingExpressionAndRule(string $secret): string + { + return 'you can see this secret only if both checks pass'; + } + + #[Query] + #[Security(rule: [self::class, 'truthyString'])] + public function getSecretWithNonBooleanRule(): string + { + return 'never returned'; + } + + #[Query] + #[Security('this.truthyValue()')] + public function getSecretWithNonBooleanExpression(): string + { + return 'never returned'; + } + #[Query] public function getInjectedUser( #[InjectUser] @@ -68,4 +142,37 @@ public function isAllowed(string $secret): bool { return $secret === '42'; } + + /** Deliberately returns a truthy non-bool, exercised through the expression path. */ + public function truthyValue(): string + { + return 'yes'; + } + + public static function secretIsFoo(SecurityRuleContext $context): bool + { + return $context->argument('secret') === 'foo'; + } + + public static function userBarIs42(SecurityRuleContext $context): bool + { + return $context->user !== null && $context->user->bar === 42; + } + + public static function canEditAndIsLogged(SecurityRuleContext $context): bool + { + return $context->isGranted('CAN_EDIT', $context->user) && $context->isLogged(); + } + + /** Reads the source object: the rule equivalent of the `this` expression variable. */ + public static function sourceAllowsSecret(SecurityRuleContext $context): bool + { + return $context->source->isAllowed($context->argument('secret')); + } + + /** Deliberately returns a truthy non-bool, to prove rules reject it rather than accepting it. */ + public static function truthyString(SecurityRuleContext $context): mixed + { + return 'yes'; + } } diff --git a/tests/Fixtures/PageSizeWithin.php b/tests/Fixtures/PageSizeWithin.php new file mode 100644 index 0000000000..ea971757f9 --- /dev/null +++ b/tests/Fixtures/PageSizeWithin.php @@ -0,0 +1,31 @@ +argument('first'); + + return is_int($requested) && $requested <= $this->max; + } +} diff --git a/tests/Fixtures/SecretIs.php b/tests/Fixtures/SecretIs.php new file mode 100644 index 0000000000..c6ddcbbd84 --- /dev/null +++ b/tests/Fixtures/SecretIs.php @@ -0,0 +1,26 @@ +argument('secret') === $this->expected; + } +} diff --git a/tests/Integration/EndToEndTest.php b/tests/Integration/EndToEndTest.php index ad6a4647a7..529e27bd4f 100644 --- a/tests/Integration/EndToEndTest.php +++ b/tests/Integration/EndToEndTest.php @@ -30,6 +30,7 @@ use TheCodingMachine\GraphQLite\Loggers\ExceptionLogger; use TheCodingMachine\GraphQLite\Mappers\CannotMapTypeException; use TheCodingMachine\GraphQLite\Middlewares\MissingAuthorizationException; +use TheCodingMachine\GraphQLite\Middlewares\NonBooleanSecurityResultException; use TheCodingMachine\GraphQLite\Schema; use TheCodingMachine\GraphQLite\SchemaFactory; use TheCodingMachine\GraphQLite\Security\AuthenticationServiceInterface; @@ -1137,6 +1138,197 @@ public function testEndToEndSecurityAnnotation(): void $result->toArray(DebugFlag::RETHROW_INTERNAL_EXCEPTIONS); } + public function testEndToEndSecurityRule(): void + { + $schema = $this->mainContainer->get(Schema::class); + assert($schema instanceof Schema); + + $result = GraphQL::executeQuery($schema, ' + query { + secretPhraseByRule(secret: "foo") + } + '); + + $this->assertSame(['secretPhraseByRule' => 'you can see this secret only if passed parameter is "foo"'], $this->getSuccessResult($result)); + + $result = GraphQL::executeQuery($schema, ' + query { + secretPhraseByRule(secret: "bar") + } + '); + + $this->expectException(MissingAuthorizationException::class); + $this->expectExceptionMessage('Wrong secret passed'); + $result->toArray(DebugFlag::RETHROW_INTERNAL_EXCEPTIONS); + } + + /** + * An invokable object is how a rule is parameterized: attribute arguments are constant + * expressions, so a callable written in an attribute cannot capture or partially apply. + */ + public function testEndToEndSecurityRuleAsInvokableObject(): void + { + $schema = $this->mainContainer->get(Schema::class); + assert($schema instanceof Schema); + + $result = GraphQL::executeQuery($schema, ' + query { + nullableSecretPhraseByRule(secret: "foo") + } + '); + + $this->assertSame(['nullableSecretPhraseByRule' => 'you can see this secret only if passed parameter is "foo"'], $this->getSuccessResult($result)); + + $result = GraphQL::executeQuery($schema, ' + query { + nullableSecretPhraseByRule(secret: "bar") + } + '); + + $this->assertSame(['nullableSecretPhraseByRule' => null], $this->getSuccessResult($result)); + } + + /** + * A rule parameterized by its constructor makes the authorization decision. + * + * #[Security(rule: new PageSizeWithin(100))] guards this field. The two queries below differ + * only in the requested page size, and the 100 passed to the constructor is the only thing + * that separates them: 50 is served, 500 is refused. + */ + public function testEndToEndSecurityRuleParameterizedByItsConstructor(): void + { + $schema = $this->mainContainer->get(Schema::class); + assert($schema instanceof Schema); + + $result = GraphQL::executeQuery($schema, ' + query { + pagedSecret(first: 50) + } + '); + + $this->assertSame( + ['pagedSecret' => 'you can see this secret only if first is within the configured limit'], + $this->getSuccessResult($result), + ); + + $result = GraphQL::executeQuery($schema, ' + query { + pagedSecret(first: 500) + } + '); + + $this->expectException(MissingAuthorizationException::class); + $this->expectExceptionMessage('Page size too large'); + $result->toArray(DebugFlag::RETHROW_INTERNAL_EXCEPTIONS); + } + + /** + * The boundary itself: 100 is allowed, 101 is not. + */ + public function testEndToEndSecurityRuleEnforcesItsConstructorBoundaryExactly(): void + { + $schema = $this->mainContainer->get(Schema::class); + assert($schema instanceof Schema); + + $result = GraphQL::executeQuery($schema, ' + query { + pagedSecret(first: 100) + } + '); + + $this->assertSame( + ['pagedSecret' => 'you can see this secret only if first is within the configured limit'], + $this->getSuccessResult($result), + ); + + $result = GraphQL::executeQuery($schema, ' + query { + pagedSecret(first: 101) + } + '); + + $this->expectException(MissingAuthorizationException::class); + $result->toArray(DebugFlag::RETHROW_INTERNAL_EXCEPTIONS); + } + + /** + * The rule equivalent of the `this` expression variable. + */ + public function testEndToEndSecurityRuleReadingTheSource(): void + { + $schema = $this->mainContainer->get(Schema::class); + assert($schema instanceof Schema); + + $result = GraphQL::executeQuery($schema, ' + query { + secretUsingSourceByRule(secret:"41") + } + '); + + $this->assertSame('Access denied.', $result->toArray(DebugFlag::RETHROW_UNSAFE_EXCEPTIONS)['errors'][0]['message']); + + $result = GraphQL::executeQuery($schema, ' + query { + secretUsingSourceByRule(secret:"42") + } + '); + + $this->assertSame('you can see this secret only if isAllowed() returns true', $result->toArray(DebugFlag::RETHROW_UNSAFE_EXCEPTIONS)['data']['secretUsingSourceByRule']); + } + + public function testEndToEndSecurityRuleDeniesWithoutUser(): void + { + $schema = $this->mainContainer->get(Schema::class); + assert($schema instanceof Schema); + + $result = GraphQL::executeQuery($schema, ' + query { + secretUsingUserByRule + } + '); + + $this->assertSame('Access denied.', $result->toArray(DebugFlag::RETHROW_UNSAFE_EXCEPTIONS)['errors'][0]['message']); + } + + /** + * A rule must return a bool. Expressions are evaluated loosely; rules deliberately are not. + */ + public function testEndToEndSecurityRuleRejectsNonBooleanReturn(): void + { + $schema = $this->mainContainer->get(Schema::class); + assert($schema instanceof Schema); + + $result = GraphQL::executeQuery($schema, ' + query { + secretWithNonBooleanRule + } + '); + + $this->expectException(NonBooleanSecurityResultException::class); + $this->expectExceptionMessage('must return a bool, but returned string'); + $result->toArray(DebugFlag::RETHROW_INTERNAL_EXCEPTIONS); + } + + /** + * Expressions are held to the same contract as rules: a truthy non-bool is rejected rather + * than silently granting access. + */ + public function testEndToEndSecurityExpressionRejectsNonBooleanResult(): void + { + $schema = $this->mainContainer->get(Schema::class); + assert($schema instanceof Schema); + + $result = GraphQL::executeQuery($schema, ' + query { + secretWithNonBooleanExpression + } + '); + + $this->expectException(NonBooleanSecurityResultException::class); + $this->expectExceptionMessage('must evaluate to a bool, but evaluated to string'); + $result->toArray(DebugFlag::RETHROW_INTERNAL_EXCEPTIONS); + } + public function testEndToEndSecurityFailWithAnnotation(): void { $schema = $this->mainContainer->get(Schema::class); @@ -1269,6 +1461,76 @@ public function isAllowed(string $right, $subject = null): bool $this->assertSame('you can see this secret only if user has right "CAN_EDIT"', $result->toArray(DebugFlag::RETHROW_UNSAFE_EXCEPTIONS)['data']['secretUsingIsGranted']); } + /** + * The rule equivalents of `user`, `is_granted()` and `is_logged()`, proving a rule reaches the + * same authentication and authorization services the expression functions do. + */ + public function testEndToEndSecurityRuleWithUserConnected(): void + { + $container = $this->createContainer([ + AuthenticationServiceInterface::class => static function () { + return new class implements AuthenticationServiceInterface { + public function isLogged(): bool + { + return true; + } + + public function getUser(): object|null + { + $user = new stdClass(); + $user->bar = 42; + return $user; + } + }; + }, + AuthorizationServiceInterface::class => static function () { + return new class implements AuthorizationServiceInterface { + public function isAllowed(string $right, $subject = null): bool + { + return $right === 'CAN_EDIT' && $subject->bar === 42; + } + }; + }, + ]); + + $schema = $container->get(Schema::class); + assert($schema instanceof Schema); + + $result = GraphQL::executeQuery($schema, ' + query { + secretUsingUserByRule + } + '); + + $this->assertSame('you can see this secret only if user.bar is set to 42', $result->toArray(DebugFlag::RETHROW_UNSAFE_EXCEPTIONS)['data']['secretUsingUserByRule']); + + $result = GraphQL::executeQuery($schema, ' + query { + secretUsingIsGrantedByRule + } + '); + + $this->assertSame('you can see this secret only if user has right "CAN_EDIT"', $result->toArray(DebugFlag::RETHROW_UNSAFE_EXCEPTIONS)['data']['secretUsingIsGrantedByRule']); + + // #[Security] is repeatable and every annotation must pass, whichever form each one uses. + $result = GraphQL::executeQuery($schema, ' + query { + secretUsingExpressionAndRule(secret: "foo") + } + '); + + $this->assertSame('you can see this secret only if both checks pass', $result->toArray(DebugFlag::RETHROW_UNSAFE_EXCEPTIONS)['data']['secretUsingExpressionAndRule']); + + // The expression half denies. + $result = GraphQL::executeQuery($schema, ' + query { + secretUsingExpressionAndRule(secret: "bar") + } + '); + + $this->assertSame('Access denied.', $result->toArray(DebugFlag::RETHROW_UNSAFE_EXCEPTIONS)['errors'][0]['message']); + } + public function testEndToEndSecurityWithThis(): void { $schema = $this->mainContainer->get(Schema::class); diff --git a/tests/Integration/IntegrationTestCase.php b/tests/Integration/IntegrationTestCase.php index 8db1d26d27..3134e18dd2 100644 --- a/tests/Integration/IntegrationTestCase.php +++ b/tests/Integration/IntegrationTestCase.php @@ -63,6 +63,7 @@ use TheCodingMachine\GraphQLite\Middlewares\SecurityInputFieldMiddleware; use TheCodingMachine\GraphQLite\NamingStrategy; use TheCodingMachine\GraphQLite\NamingStrategyInterface; +use TheCodingMachine\GraphQLite\CallableResolver; use TheCodingMachine\GraphQLite\ParameterizedCallableResolver; use TheCodingMachine\GraphQLite\QueryProviderInterface; use TheCodingMachine\GraphQLite\Reflection\DocBlock\CachedDocBlockFactory; @@ -151,7 +152,7 @@ public function createContainer(array $overloadedServices = []): ContainerInterf $container->get(FieldMiddlewareInterface::class), $container->get(InputFieldMiddlewareInterface::class), ); - $parameterizedCallableResolver = new ParameterizedCallableResolver($fieldsBuilder, $container); + $parameterizedCallableResolver = new ParameterizedCallableResolver($fieldsBuilder, $container->get(CallableResolver::class)); $parameterMiddlewarePipe->pipe(new PrefetchParameterMiddleware($parameterizedCallableResolver)); @@ -181,6 +182,7 @@ public function createContainer(array $overloadedServices = []): ContainerInterf new ExpressionLanguage(new Psr16Adapter(new Psr16Cache(new ArrayAdapter())), [new SecurityExpressionLanguageProvider()]), $container->get(AuthenticationServiceInterface::class), $container->get(AuthorizationServiceInterface::class), + $container->get(CallableResolver::class), ); }, AuthorizationFieldMiddleware::class => static function (ContainerInterface $container) { @@ -194,9 +196,13 @@ public function createContainer(array $overloadedServices = []): ContainerInterf new ExpressionLanguage(new Psr16Adapter(new Psr16Cache(new ArrayAdapter())), [new SecurityExpressionLanguageProvider()]), $container->get(AuthenticationServiceInterface::class), $container->get(AuthorizationServiceInterface::class), + $container->get(CallableResolver::class), ); }, CostFieldMiddleware::class => fn () => new CostFieldMiddleware(), + CallableResolver::class => static function (ContainerInterface $container) { + return new CallableResolver($container); + }, ArgumentResolver::class => static function (ContainerInterface $container) { return new ArgumentResolver(); }, diff --git a/tests/Middlewares/SecurityFieldMiddlewareTest.php b/tests/Middlewares/SecurityFieldMiddlewareTest.php new file mode 100644 index 0000000000..03b95f7b46 --- /dev/null +++ b/tests/Middlewares/SecurityFieldMiddlewareTest.php @@ -0,0 +1,243 @@ +expectException(BadExpressionInSecurityException::class); + $this->expectExceptionMessage('is not valid'); + + $this->process([new Security('this is not valid expression language')]); + } + + /** + * Linting must not gate on GraphQLite's own two functions. Anything the consumer registered on + * the ExpressionLanguage they supplied has to be accepted too. + */ + public function testExpressionMayCallAFunctionRegisteredByTheConsumer(): void + { + $language = $this->language(); + $language->register('is_owner', static fn () => 'true', static fn (array $v) => true); + + $field = $this->process([new Security('is_owner(this)')], $language); + + self::assertNotNull($field); + } + + public function testUnknownFunctionIsStillRejected(): void + { + $this->expectException(BadExpressionInSecurityException::class); + $this->expectExceptionMessage('does not exist'); + + $this->process([new Security('no_such_function(this)')]); + } + + /** + * Pins the lint name list against getVariables(). If a variable is added to the bag without + * being added to VARIABLE_NAMES, expressions using it would lint as unknown and this fails. + */ + #[DataProvider('providedVariableNames')] + public function testExpressionCanReferenceEveryProvidedVariable(string $variable): void + { + $field = $this->process([new Security($variable . ' == null')]); + + self::assertNotNull($field); + } + + public static function providedVariableNames(): iterable + { + yield 'user' => ['user']; + yield 'this' => ['this']; + yield 'authorizationService' => ['authorizationService']; + yield 'authenticationService' => ['authenticationService']; + } + + /** + * Linting initialises the parser, and ExpressionLanguage refuses register() afterwards. The + * middleware lints a clone so a consumer holding the real instance can still register on it, + * exactly as they could before expressions were checked at build time. + */ + public function testBuildingTheSchemaLeavesTheExpressionLanguageRegisterable(): void + { + $language = $this->language(); + + $this->process([new Security("is_granted('FOO')")], $language); + + $language->register('registered_afterwards', static fn () => 'true', static fn (array $v) => true); + + self::assertTrue($language->evaluate('registered_afterwards()', [])); + } + + /** + * A Closure is a supported rule form. First-class callable syntax (`Rules::allow(...)`) is how + * you write one in an attribute on PHP 8.5, but it cannot appear in a fixture here because the + * floor is 8.2 — and it does not need to. What reaches the middleware is a Closure either way, + * and Closure::fromCallable() produces one indistinguishable from what FCC yields. + */ + public function testClosureRuleGrantsAccess(): void + { + $field = $this->process([new Security(rule: static fn (SecurityRuleContext $context): bool => true)]); + + self::assertNotNull($field); + self::assertSame('resolved', ($field->resolveFn)(null)); + } + + public function testClosureRuleDeniesAccess(): void + { + $field = $this->process([ + new Security(rule: static fn (SecurityRuleContext $context): bool => false, message: 'Nope'), + ]); + + self::assertNotNull($field); + + $this->expectException(MissingAuthorizationException::class); + $this->expectExceptionMessage('Nope'); + ($field->resolveFn)(null); + } + + /** The exact shape first-class callable syntax produces. */ + public function testClosureFromACallableActsAsARule(): void + { + $field = $this->process([new Security(rule: Closure::fromCallable([self::class, 'denyEverything']))]); + + self::assertNotNull($field); + + $this->expectException(MissingAuthorizationException::class); + ($field->resolveFn)(null); + } + + public function testClosureRuleIsHandedTheSecurityRuleContext(): void + { + $seen = null; + + $field = $this->process([ + new Security(rule: static function (SecurityRuleContext $context) use (&$seen): bool { + $seen = $context; + + return true; + }), + ]); + + ($field->resolveFn)(null); + + self::assertInstanceOf(SecurityRuleContext::class, $seen); + } + + /** + * First-class callable syntax — `Rules::allow(...)` — written directly in the attribute. + * + * The class is built with eval() on purpose. FCC in a constant expression is a COMPILE-time + * fatal before PHP 8.5, and every .php file under tests/ gets compiled during integration runs + * because SchemaFactory falls back to ComposerFinder, which walks Composer's autoload paths. + * #[RequiresPhp] gates execution, not compilation, so a normal fixture file would kill the 8.2, + * 8.3 and 8.4 cells no matter how it was annotated. eval() defers the compile until after this + * guard has already decided to run. + */ + #[RequiresPhp('>= 8.5')] + public function testFirstClassCallableSyntaxActsAsARule(): void + { + if (! class_exists('FirstClassCallableRuleProbe', false)) { + eval(<<<'PROBE' + class FirstClassCallableRuleProbe + { + #[\TheCodingMachine\GraphQLite\Annotations\Security( + rule: \TheCodingMachine\GraphQLite\Middlewares\SecurityFieldMiddlewareTest::denyEverything(...), + )] + public function guarded(): string + { + return 'never'; + } + } + PROBE); + } + + $rule = (new ReflectionMethod('FirstClassCallableRuleProbe', 'guarded')) + ->getAttributes(Security::class)[0] + ->newInstance() + ->getRule(); + + // FCC yields a genuine Closure that still knows the method it came from. + self::assertInstanceOf(Closure::class, $rule); + self::assertSame('denyEverything', (new ReflectionFunction($rule))->getName()); + + $field = $this->process([new Security(rule: $rule)]); + + self::assertNotNull($field); + + $this->expectException(MissingAuthorizationException::class); + ($field->resolveFn)(null); + } + + public static function denyEverything(SecurityRuleContext $context): bool + { + return false; + } + + /** @param MiddlewareAnnotationInterface[] $annotations */ + private function process(array $annotations, ExpressionLanguage|null $language = null): FieldDefinition|null + { + $resolver = static fn (): string => 'resolved'; + + $descriptor = new QueryFieldDescriptor( + name: 'foo', + type: Type::string(), + resolver: $resolver, + originalResolver: new ServiceResolver([new VoidAuthorizationService(), 'isAllowed']), + middlewareAnnotations: new MiddlewareAnnotations($annotations), + ); + + $middleware = new SecurityFieldMiddleware( + $language ?? $this->language(), + new VoidAuthenticationService(), + new VoidAuthorizationService(), + new CallableResolver(new EmptyContainer()), + ); + + return $middleware->process($descriptor, new class implements FieldHandlerInterface { + public function handle(QueryFieldDescriptor $fieldDescriptor): FieldDefinition|null + { + return new FieldDefinition([ + 'name' => $fieldDescriptor->getName(), + 'resolve' => $fieldDescriptor->getResolver(), + ]); + } + }); + } + + private function language(): ExpressionLanguage + { + $language = new ExpressionLanguage(); + $language->registerProvider(new SecurityExpressionLanguageProvider()); + + return $language; + } +} diff --git a/tests/ParameterizedCallableResolverTest.php b/tests/ParameterizedCallableResolverTest.php index f1643fb99e..a326342af0 100644 --- a/tests/ParameterizedCallableResolverTest.php +++ b/tests/ParameterizedCallableResolverTest.php @@ -24,7 +24,7 @@ public function testResolveReturnsCallableAndParametersFromStaticMethod(): void [$resultingCallable, $resultingParameters] = (new ParameterizedCallableResolver( $fieldsBuilder, - $this->createMock(ContainerInterface::class), + new CallableResolver($this->createMock(ContainerInterface::class)), ))->resolve([Contact::class, 'prefetchTheContacts'], self::class, 123); self::assertSame(['test'], $resultingCallable(['test'])); @@ -42,7 +42,7 @@ public function testResolveReturnsCallableAndParametersFromStaticMethodOnSelf(): [$resultingCallable, $resultingParameters] = (new ParameterizedCallableResolver( $fieldsBuilder, - $this->createMock(ContainerInterface::class), + new CallableResolver($this->createMock(ContainerInterface::class)), ))->resolve('prefetchTheContacts', Contact::class, 123); self::assertSame(['test'], $resultingCallable(['test'])); @@ -66,13 +66,50 @@ public function testResolveReturnsCallableAndParametersFromContainer(): void [$resultingCallable, $resultingParameters] = (new ParameterizedCallableResolver( $fieldsBuilder, - $container, + new CallableResolver($container), ))->resolve([FooExtendType::class, 'customExtendedField'], self::class, 123); self::assertSame('TEST', $resultingCallable(new TestObject('test'))); self::assertSame($expectedParameters, $resultingParameters); } + /** + * A Closure that came from a method keeps that origin, so it can still be parameter-mapped. + * This is what first-class callable syntax produces on PHP 8.5. + */ + public function testResolveAcceptsAClosureBackedByAMethod(): void + { + $expectedParameters = [$this->createStub(ParameterInterface::class)]; + + $fieldsBuilder = $this->createMock(FieldsBuilder::class); + $fieldsBuilder->method('getParameters') + ->with(new IsEqual(new \ReflectionMethod(Contact::class, 'prefetchTheContacts')), 0) + ->willReturn($expectedParameters); + + [$resultingCallable, $resultingParameters] = (new ParameterizedCallableResolver( + $fieldsBuilder, + new CallableResolver($this->createMock(ContainerInterface::class)), + ))->resolve(\Closure::fromCallable([Contact::class, 'prefetchTheContacts']), self::class); + + self::assertSame(['test'], $resultingCallable(['test'])); + self::assertSame($expectedParameters, $resultingParameters); + } + + /** + * An anonymous closure has no originating method, so its parameters cannot become GraphQL + * arguments. The message stays generic: this resolver does not know which attribute called it. + */ + public function testResolveRejectsAClosureWithNoOriginatingMethod(): void + { + $this->expectException(InvalidCallableRuntimeException::class); + $this->expectExceptionMessage('The callable must name a real method, because its parameters are mapped to GraphQL arguments'); + + (new ParameterizedCallableResolver( + $this->createMock(FieldsBuilder::class), + new CallableResolver($this->createMock(ContainerInterface::class)), + ))->resolve(static fn (array $sources): array => $sources, self::class); + } + public function testResolveThrowsInvalidCallableMethodNotFoundException(): void { $this->expectException(InvalidCallableRuntimeException::class); @@ -80,7 +117,7 @@ public function testResolveThrowsInvalidCallableMethodNotFoundException(): void (new ParameterizedCallableResolver( $this->createMock(FieldsBuilder::class), - $this->createMock(ContainerInterface::class), + new CallableResolver($this->createMock(ContainerInterface::class)), ))->resolve('doesntExist', self::class); } } \ No newline at end of file diff --git a/tests/Security/SecurityRuleContextTest.php b/tests/Security/SecurityRuleContextTest.php new file mode 100644 index 0000000000..d8bec35ef0 --- /dev/null +++ b/tests/Security/SecurityRuleContextTest.php @@ -0,0 +1,114 @@ +context($user, $source, ['first' => 10, 'search' => null]); + + self::assertSame($user, $context->user); + self::assertSame($source, $context->source); + self::assertSame(['first' => 10, 'search' => null], $context->arguments); + } + + public function testArgumentReadsByName(): void + { + $context = $this->context(null, null, ['first' => 10]); + + self::assertSame(10, $context->argument('first')); + } + + public function testArgumentIsNullForAnAbsentArgument(): void + { + $context = $this->context(null, null, []); + + self::assertNull($context->argument('missing')); + } + + public function testHasArgumentDistinguishesNullFromAbsent(): void + { + $context = $this->context(null, null, ['explicitlyNull' => null]); + + self::assertTrue($context->hasArgument('explicitlyNull')); + self::assertFalse($context->hasArgument('missing')); + self::assertNull($context->argument('explicitlyNull')); + } + + public function testIsGrantedDelegatesToTheAuthorizationService(): void + { + $subject = new stdClass(); + + $authorization = $this->createMock(AuthorizationServiceInterface::class); + $authorization->expects($this->once()) + ->method('isAllowed') + ->with('DOCUMENT_READ', $subject) + ->willReturn(true); + + $context = new SecurityRuleContext( + null, + null, + [], + $this->createMock(AuthenticationServiceInterface::class), + $authorization, + ); + + self::assertTrue($context->isGranted('DOCUMENT_READ', $subject)); + } + + public function testIsGrantedPassesNoSubjectWhenNoneIsGiven(): void + { + $authorization = $this->createMock(AuthorizationServiceInterface::class); + $authorization->expects($this->once()) + ->method('isAllowed') + ->with('ROLE_ADMIN', null) + ->willReturn(false); + + $context = new SecurityRuleContext( + null, + null, + [], + $this->createMock(AuthenticationServiceInterface::class), + $authorization, + ); + + self::assertFalse($context->isGranted('ROLE_ADMIN')); + } + + public function testIsLoggedDelegatesToTheAuthenticationService(): void + { + $authentication = $this->createMock(AuthenticationServiceInterface::class); + $authentication->expects($this->once()) + ->method('isLogged') + ->willReturn(true); + + $context = new SecurityRuleContext( + null, + null, + [], + $authentication, + $this->createMock(AuthorizationServiceInterface::class), + ); + + self::assertTrue($context->isLogged()); + } + + /** @param array $arguments */ + private function context(object|null $user, object|null $source, array $arguments): SecurityRuleContext + { + return new SecurityRuleContext( + $user, + $source, + $arguments, + $this->createMock(AuthenticationServiceInterface::class), + $this->createMock(AuthorizationServiceInterface::class), + ); + } +} diff --git a/website/docs/attributes-reference.md b/website/docs/attributes-reference.md index 31b50655d9..c6d35ba56c 100644 --- a/website/docs/attributes-reference.md +++ b/website/docs/attributes-reference.md @@ -225,7 +225,7 @@ Marks field parameter to be used for [prefetching](prefetch-method.mdx). Attribute | Compulsory | Type | Definition ------------------------------|------------|----------|-------- -callable | *no* | callable | Name of the prefetch method (in same class) or a full callable, either a static method or regular service from the container +callable | *yes* | callable | Name of the prefetch method (in same class), a full callable naming either a static method or a regular service from the container, an invokable object, or — on PHP 8.5 — first-class callable syntax ## #[Query] @@ -251,16 +251,25 @@ name | *yes* | string | The name of the right. ## #[Security] -The `#[Security]` attribute can be used to check fin-grained access rights. -It is very flexible: it allows you to pass an expression that can contains custom logic. +The `#[Security]` attribute can be used to check fine-grained access rights. +It is very flexible: it allows you to pass a PHP callable containing custom logic. See [the fine grained security page](fine-grained-security.mdx) for more details. **Applies on**: methods or properties annotated with `#[Query]`, `#[Mutation]` or `#[Field]`. - -Attribute | Compulsory | Type | Definition ----------------|------------|--------|-------- -*default* | *yes* | string | The security expression +Repeatable: every `#[Security]` attribute declared on a field must pass. + +Attribute | Compulsory | Type | Definition +---------------|------------|----------------|-------- +expression | see below | string | A security expression, evaluated by Symfony ExpressionLanguage. Also accepted as the attribute's first positional argument: `#[Security("is_granted('X')")]` +rule | see below | array \| object | A callable receiving a `SecurityRuleContext` and returning `bool`. Must be passed by name (`rule:`) — the first positional argument is the expression, so a positional callable is either a `TypeError` or silently read as a legacy data array. Prefer first-class callable syntax (`self::canShow(...)`, PHP 8.5+); `[Rules::class, 'canShow']` works on 8.2+ and is the only form that can name a container-resolved method. A bare method-name string is **not** accepted here, unlike `#[Prefetch]` +failWith | *no* | mixed | Value returned instead of denying. Cannot be combined with *message* or *statusCode* +message | *no* | string | Error message when access is denied. Defaults to `Access denied.` +statusCode | *no* | int | Status code when access is denied. Defaults to `403` + +Exactly one of `expression` and `rule` must be given; passing both throws, as does passing neither. +Both forms are fully supported — see [fine grained security](fine-grained-security.mdx) for when to +use which. ## #[SourceField] diff --git a/website/docs/fine-grained-security.mdx b/website/docs/fine-grained-security.mdx index c27f6dfb43..9a8fad490b 100644 --- a/website/docs/fine-grained-security.mdx +++ b/website/docs/fine-grained-security.mdx @@ -8,7 +8,7 @@ sidebar_label: Fine grained security If the [`#[Logged]` and `#[Right]` attributes](authentication-authorization.mdx#logged-and-right-attributes) are not granular enough for your needs, you can use the advanced `#[Security]` attribute. -Using the `#[Security]` attribute, you can write an *expression* that can contain custom logic. For instance: +Using the `#[Security]` attribute, you can write a *rule* containing custom logic. For instance: - Check that a user can access a given resource - Check that a user has one right or another right @@ -16,36 +16,92 @@ Using the `#[Security]` attribute, you can write an *expression* that can contai ## Using the #[Security] attribute -The `#[Security]` attribute is very flexible: it allows you to pass an expression that can contains custom logic: +A rule is an ordinary PHP callable. It receives a `SecurityRuleContext` and returns a `bool`. The +clearest way to write one is first-class callable syntax, which lets the rule live as a private +method right beside the field it guards: ```php use TheCodingMachine\GraphQLite\Annotations\Security; +use TheCodingMachine\GraphQLite\Security\SecurityRuleContext; -// ... - -#[Query] -#[Security("is_granted('ROLE_ADMIN') or is_granted('POST_SHOW', post)")] -public function getPost(Post $post): array +class PostController { - // ... + #[Query] + #[Security(rule: self::canShow(...))] + public function getPost(Post $post): Post + { + // ... + } + + private static function canShow(SecurityRuleContext $context): bool + { + return $context->isGranted('ROLE_ADMIN') + || $context->isGranted('POST_SHOW', $context->argument('post')); + } } ``` -The *expression* defined in the `#[Security]` attribute must conform to [Symfony's Expression Language syntax](https://symfony.com/doc/4.4/components/expression_language/syntax.html) +Nothing is called when the attribute is read. `self::canShow(...)` produces a `Closure`; GraphQLite +invokes it during resolution, once per field, with the context. + +
+ First-class callable syntax in an attribute requires PHP 8.5. On PHP 8.2, 8.3 + and 8.4 it is a compile-time fatal — Constant expression contains invalid + operations — that no runtime check can catch. If you support those versions, use the + array form, which works everywhere: + #[Security(rule: [PostController::class, 'canShow'])]. +

+ One difference matters when you downgrade: the array form is resolved by reflection, so the + method must be public. A rule kept private under + self::canShow(...) has to be widened to public static, or moved to a + shared rules class. Leave it private and the schema fails to build with + "must be public to be used as a callable". + See ways to write a rule. +
+ +Because a rule is plain PHP, it is found by "find usages", renamed safely by an IDE, steppable in a +debugger, and unit testable without building a schema or executing a query. + +Note that static analysis checks the *reference* to the rule, not its body: `SecurityRuleContext` +exposes `$user` and `$source` as `object|null` and `argument()` as `mixed`, so reaching through them +is unchecked. Narrow them yourself when it matters — an `instanceof` or a typed local — the same way +you would with any `mixed` input.
- If you are a Symfony user, you might already be used to the #[Security] attribute. Most of the inspiration - of this attribute comes from Symfony. Warning though! GraphQLite's #[Security] attribute and - Symfony's #[Security] attribute are slightly different. Especially, the two attributes do not live - in the same namespace! + #[Security] also accepts an expression string, evaluated by Symfony's + Expression Language. Both forms are fully supported. Reach for an expression when the check is a + short predicate you want to read at the field; reach for a rule when the logic is worth typing, + testing, reusing or parameterizing. See the expression form.
-## The `is_granted` function +### Ways to write a rule + +Every form below receives the same `SecurityRuleContext` and must return `bool`. They differ only in +what they can express and which PHP version accepts them. -Use the `is_granted` function to check if a user has a special right. +| Form | PHP | Use it when | +|---|---|---| +| `self::canShow(...)` | 8.5+ | **Preferred.** The rule belongs to the class it guards. Written inside the class body it keeps class scope, so the method can stay `private` — no new public surface. Only this form can reach a private method. | +| `PostRules::canShow(...)` | 8.5+ | **Preferred** when the rule is shared across controllers. Refactor-safe: renaming the method updates the attribute. | +| `[PostRules::class, 'canShow']` | 8.2+ | You support PHP below 8.5, **or** the rule needs collaborators — a non-static method here is resolved through the container. Resolved by reflection, so the method must be **public**. | +| `new PageSizeWithin(100)` | 8.2+ | The rule is parameterized by a constant. See [parameterizing a rule](#parameterizing-a-rule). | +| `static function (SecurityRuleContext $c) { ... }` | 8.5+ | A genuine one-off. Must be `static`; arrow functions and `use (...)` are rejected in attributes. | + +Two limits worth knowing before you choose: + +* **First-class callable syntax cannot name a container-resolved method.** `Service::method(...)` on a + non-static method compiles and then throws when the attribute is read. A rule that needs injected + dependencies must use the array form — that is not a legacy fallback, it is the only syntax that + can express it. +* **Nothing in an attribute can capture a variable.** Attribute arguments are constant expressions on + every PHP version, so to parameterize a rule you construct it — see below. + +## Checking rights + +Use `isGranted()` to check whether the current user holds a right. ```php -#[Security("is_granted('ROLE_ADMIN')")] +$context->isGranted('ROLE_ADMIN') ``` is similar to @@ -54,43 +110,89 @@ is similar to #[Right("ROLE_ADMIN")] ``` -In addition, the `is_granted` function accepts a second optional parameter: the "scope" of the right. +For a global, subject-free permission check, prefer `#[Right]` — it says what it means with less +ceremony. Reach for `#[Security]` when the decision depends on the field's arguments or its source +object, which `#[Right]` structurally cannot see. + +`isGranted()` accepts a second optional parameter: the "scope" of the right. ```php #[Query] -#[Security("is_granted('POST_SHOW', post)")] -public function getPost(Post $post): array +#[Security(rule: self::canShow(...))] +public function getPost(Post $post): Post { // ... } ``` -In the example above, the `getPost` method can be called only if the logged user has the 'POST_SHOW' permission on the -`$post` object. You can notice that the `$post` object comes from the parameters. +In the example above, `getPost` can be called only if the logged user has the `POST_SHOW` permission +on the `$post` object, which the rule reads with `$context->argument('post')`. ## Accessing method parameters -All parameters passed to the method can be accessed in the `#[Security]` expression. +All parameters passed to the method are available on the context, by name, already resolved to their +PHP values. ```php #[Query] -#[Security(expression: "startDate < endDate", statusCode: 400, message: "End date must be after start date")] +#[Security(rule: self::startsBeforeItEnds(...), statusCode: 400, message: "End date must be after start date")] public function getPosts(DateTimeImmutable $startDate, DateTimeImmutable $endDate): array { // ... } + +private static function startsBeforeItEnds(SecurityRuleContext $context): bool +{ + return $context->argument('startDate') < $context->argument('endDate'); +} ``` In the example above, we tweak a bit the Security attribute purpose to do simple input validation. +Use `hasArgument()` when you need to tell an argument that was genuinely `null` from one that was +not supplied at all. + +## Parameterizing a rule + +PHP attribute arguments are constant expressions, so a callable written inside an attribute cannot +capture a variable or partially apply — on any PHP version. Rather than writing one method per +constant, use an invokable object: + +```php +final class PageSizeWithin +{ + public function __construct(private readonly int $max) + { + } + + public function __invoke(SecurityRuleContext $context): bool + { + return $context->argument('first') <= $this->max; + } +} + +#[Query] +#[Security(rule: new PageSizeWithin(100), statusCode: 400, message: 'Page size too large')] +public function getPosts(int $first): array +{ + // ... +} +``` + +`new` in an attribute argument has been legal since PHP 8.1, so this form works on every supported +version — and the constructor call is type-checked by static analysis like any other. + +This is the one job first-class callable syntax cannot do: `self::pageSizeWithin(...)` has nowhere to +put the `100`. Construct the rule instead of naming it. + ## Setting HTTP code and error message You can use the `statusCode` and `message` attributes to set the HTTP code and GraphQL error message. ```php #[Query] -#[Security(expression: "is_granted('POST_SHOW', post)", statusCode: 404, message: "Post not found (let's pretend the post does not exists!)")] -public function getPost(Post $post): array +#[Security(rule: self::canShow(...), statusCode: 404, message: "Post not found (let's pretend the post does not exists!)")] +public function getPost(Post $post): Post { // ... } @@ -107,7 +209,7 @@ to set a default value. ```php #[Query] -#[Security(expression: "is_granted('CAN_SEE_MARGIN', this)", failWith: null)] +#[Security(rule: self::canSeeMargin(...), failWith: null)] public function getMargin(): float { // ... @@ -121,28 +223,32 @@ You cannot use the `failWith` attribute along `statusCode` or `message` attribut ## Accessing the user -You can use the `user` variable to access the currently logged user. -You can use the `is_logged()` function to check if a user is logged or not. - +Use `$context->user` to access the currently logged user, and `$context->isLogged()` to check +whether anybody is logged in at all. ```php #[Query] -#[Security("is_logged() && user.age > 18")] +#[Security(rule: self::isAdult(...))] public function getNSFWImages(): array { // ... } + +private static function isAdult(SecurityRuleContext $context): bool +{ + return $context->isLogged() && $context->user->age > 18; +} ``` ## Accessing the current object -You can use the `this` variable to access any (public) property / method of the current class. +Use `$context->source` to access the object the field is being resolved on. ```php class Post { #[Field] - #[Security("this.canAccessBody(user)")] - public function getBody(): array + #[Security(rule: self::userCanAccessBody(...))] + public function getBody(): string { // ... } @@ -151,20 +257,43 @@ class Post { { // Some custom logic here } + + private static function userCanAccessBody(SecurityRuleContext $context): bool + { + return $context->source->canAccessBody($context->user); + } } ``` +## Combining several checks + +`#[Security]` is repeatable, and every attribute must pass. Declare one attribute per check rather +than building one large rule: + +```php +#[Query] +#[Security(rule: self::canShow(...))] +#[Security(rule: new PageSizeWithin(100), statusCode: 400)] +public function getPosts(Post $post, int $first): array +{ + // ... +} +``` + +Passing both a rule and an expression to the *same* `#[Security]` attribute is an error, so that a +half-finished migration can never silently drop one of the two checks. + ## Available scope The `#[Security]` attribute can be used in any query, mutation or field, so anywhere you have a `#[Query]`, `#[Mutation]` -or `#[Field]` attribute. +or `#[Field]` attribute. It also applies to input fields. ## How to restrict access to a given resource -The `is_granted` method can be used to restrict access to a specific resource. +`isGranted()` can be used to restrict access to a specific resource. ```php -#[Security("is_granted('POST_SHOW', post)")] +$context->isGranted('POST_SHOW', $context->argument('post')) ``` If you are wondering how to configure these fine-grained permissions, this is not something that GraphQLite handles @@ -174,6 +303,42 @@ If you are using Symfony, you will [create a custom voter](https://symfony.com/d If you are using Laravel, you will [create a Gate or a Policy](https://laravel.com/docs/6.x/authorization). -If you are using another framework, you need to know that the `is_granted` function simply forwards the call to -the `isAllowed` method of the configured `AuthorizationSerice`. See [Connecting GraphQLite to your framework's security module +If you are using another framework, you need to know that `isGranted()` simply forwards the call to +the `isAllowed` method of the configured `AuthorizationService`. See [Connecting GraphQLite to your framework's security module ](implementing-security.md) for more details + +## The expression form + +`#[Security]` also accepts a string evaluated by +[Symfony's Expression Language](https://symfony.com/doc/current/components/expression_language/syntax.html): + +```php +#[Query] +#[Security("is_granted('ROLE_ADMIN') or is_granted('POST_SHOW', post)")] +public function getPost(Post $post): array +{ + // ... +} +``` + +This is the terser option for a short predicate, and it keeps the check readable at the field it +guards. The available variables and functions map one to one onto the rule context: + +| Expression | Rule | +|---|---| +| `user` | `$context->user` | +| `this` | `$context->source` | +| a field argument, for example `post` | `$context->argument('post')` | +| `is_granted('X', y)` | `$context->isGranted('X', $y)` | +| `is_logged()` | `$context->isLogged()` | + +Two things to know about expressions specifically: + +* They are parsed when the schema is built, so a malformed expression is a startup error naming the + field rather than a surprise on the first request that touches it. +* They must evaluate to a `bool`, exactly like rules. Write `user !== null`, not `user` — an + authorization decision should not ride on PHP's truthiness table. + +An expression is evaluated inside Symfony's Expression Language, which does not run under +`declare(strict_types=1)`. A method called from an expression therefore receives coerced arguments +where a rule would raise a `TypeError`. If a check depends on argument types being exact, use a rule. diff --git a/website/docs/implementing-security.md b/website/docs/implementing-security.md index 88bbd8f7f2..b45e129333 100644 --- a/website/docs/implementing-security.md +++ b/website/docs/implementing-security.md @@ -16,6 +16,15 @@ To plug GraphQLite to your framework's security mechanism, you will have to prov * `TheCodingMachine\GraphQLite\Security\AuthenticationServiceInterface` * `TheCodingMachine\GraphQLite\Security\AuthorizationServiceInterface` +
+ These two interfaces are the implementer's surface. If you are writing a + + #[Security] rule, call SecurityRuleContext::isGranted() and + SecurityRuleContext::isLogged() rather than depending on these interfaces directly. + They give a rule the same reach through a surface GraphQLite controls, so your rules keep + compiling as these interfaces evolve. +
+ Those two interfaces act as adapters between GraphQLite and your framework: ```php diff --git a/website/docs/prefetch-method.mdx b/website/docs/prefetch-method.mdx index a12cb50891..82724f4f1e 100644 --- a/website/docs/prefetch-method.mdx +++ b/website/docs/prefetch-method.mdx @@ -72,8 +72,19 @@ class PostType { When a `#[Prefetch]` attribute is detected on a parameter of `#[Field]` attribute, the method is called automatically. The prefetch callable must be one of the following: - a static method in the same class: `#[Prefetch('prefetchMethod')]` - - a static method in a different class: `#[Prefetch([OtherClass::class, 'prefetchMethod')]` + - a static method in a different class: `#[Prefetch([OtherClass::class, 'prefetchMethod'])]` - a non-static method in a different class, resolvable through the container: `#[Prefetch([OtherService::class, 'prefetchMethod'])]` + - an invokable object: `#[Prefetch(new CommentLoader())]` + - on PHP 8.5 and later, first-class callable syntax: `#[Prefetch(OtherClass::prefetchMethod(...))]` + +First-class callable syntax **cannot** name a non-static method resolved through the container: it +compiles, and then fails when the attribute is read. Keep using the array form for that case — it is +not a workaround for older PHP, it is the only syntax that can express it. + +The callable must name a real method, because its own parameters become GraphQL arguments and their +descriptions are read from the method docblock. An inline closure has no such method and is rejected +when the schema is built. + The first argument of the method is always an array of instances of the main type. It can return absolutely anything (mixed). ## Input arguments From e65afa05205a8a41f6668244343c1b7bca09ee7d Mon Sep 17 00:00:00 2001 From: Jacob Thomason Date: Wed, 29 Jul 2026 17:07:21 -0400 Subject: [PATCH 2/4] fix: validate #[Security] expressions with parse() rather than lint() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ExpressionLanguage::lint() only exists from Symfony 6.1, so the --prefer-lowest CI cells — which resolve symfony/expression-language down to 4.4 — died with "Call to undefined method". parse() has been available since 2.4 and rejects the same three things: bad syntax, unknown variables, unknown functions. Verified identical messages on 4.4.9 and 8.0.8, so the exception text is unchanged. Claude-Session: https://claude.ai/code/session_0156c2BrkiiJtVZkF5dkPgC6 --- src/Middlewares/SecurityFieldMiddleware.php | 14 +++++++++----- src/Middlewares/SecurityInputFieldMiddleware.php | 14 +++++++++----- 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/src/Middlewares/SecurityFieldMiddleware.php b/src/Middlewares/SecurityFieldMiddleware.php index 8bb7b54fde..030702c765 100644 --- a/src/Middlewares/SecurityFieldMiddleware.php +++ b/src/Middlewares/SecurityFieldMiddleware.php @@ -172,10 +172,14 @@ private function lintExpressions(array $annotations, array $parameters, QueryFie { $names = [...self::VARIABLE_NAMES, ...array_keys($parameters)]; - // Linting initialises the parser, and an ExpressionLanguage refuses further register() calls - // once that has happened. Linting a clone keeps every function the consumer registered - // visible to the linter while leaving the real instance open to registration afterwards, - // which is how it behaved before expressions were checked at build time. + // parse(), not lint(): lint() only exists from Symfony 6.1, and the --prefer-lowest CI cells + // resolve symfony/expression-language down to 4.4. parse() has been there since 2.4 and + // rejects exactly the same things — bad syntax, unknown variables, unknown functions. + // + // Parsing initialises the parser, and an ExpressionLanguage refuses further register() calls + // once that has happened. Parsing a clone keeps every function the consumer registered + // visible while leaving the real instance open to registration afterwards, which is how it + // behaved before expressions were checked at build time. $linter = clone $this->language; foreach ($annotations as $annotation) { @@ -186,7 +190,7 @@ private function lintExpressions(array $annotations, array $parameters, QueryFie $expression = $annotation->getExpression(); try { - $linter->lint($expression, $names); + $linter->parse($expression, $names); } catch (SyntaxError $e) { throw BadExpressionInSecurityException::fromSyntaxError($e, $descriptor, $expression); } diff --git a/src/Middlewares/SecurityInputFieldMiddleware.php b/src/Middlewares/SecurityInputFieldMiddleware.php index 464b4719e8..93840daf70 100644 --- a/src/Middlewares/SecurityInputFieldMiddleware.php +++ b/src/Middlewares/SecurityInputFieldMiddleware.php @@ -129,10 +129,14 @@ private function lintExpressions(array $annotations, array $parameters, QueryFie { $names = [...self::VARIABLE_NAMES, ...array_keys($parameters)]; - // Linting initialises the parser, and an ExpressionLanguage refuses further register() calls - // once that has happened. Linting a clone keeps every function the consumer registered - // visible to the linter while leaving the real instance open to registration afterwards, - // which is how it behaved before expressions were checked at build time. + // parse(), not lint(): lint() only exists from Symfony 6.1, and the --prefer-lowest CI cells + // resolve symfony/expression-language down to 4.4. parse() has been there since 2.4 and + // rejects exactly the same things — bad syntax, unknown variables, unknown functions. + // + // Parsing initialises the parser, and an ExpressionLanguage refuses further register() calls + // once that has happened. Parsing a clone keeps every function the consumer registered + // visible while leaving the real instance open to registration afterwards, which is how it + // behaved before expressions were checked at build time. $linter = clone $this->language; foreach ($annotations as $annotation) { @@ -143,7 +147,7 @@ private function lintExpressions(array $annotations, array $parameters, QueryFie $expression = $annotation->getExpression(); try { - $linter->lint($expression, $names); + $linter->parse($expression, $names); } catch (SyntaxError $e) { throw BadExpressionInSecurityException::fromSyntaxError($e, $descriptor, $expression); } From 82eb963e2565d7161955cc134243501fcee88036 Mon Sep 17 00:00:00 2001 From: Jacob Thomason Date: Wed, 29 Jul 2026 20:02:02 -0400 Subject: [PATCH 3/4] feat: extract SecurityRuleContextInterface Rules could only be written against the final SecurityRuleContext, so a rule was usable inside a GraphQLite field resolution and nowhere else. The interface declares the context contract, letting a rule be unit tested against a fake and reused from an HTTP middleware, a console command or a message handler. The readonly properties stay on the concrete class and gain getUser/getSource/getArguments accessors on the contract, since an interface cannot declare properties before the 8.4 property hooks this library predates. No middleware change is needed: rules are invoked without a declared parameter type, so an interface-typed rule accepts the concrete instance by subtyping. A middleware test pins that. --- src/Security/SecurityRuleContext.php | 38 ++++++- src/Security/SecurityRuleContextInterface.php | 53 ++++++++++ .../Controllers/SecurityController.php | 17 ++++ tests/Integration/EndToEndTest.php | 34 +++++++ .../SecurityFieldMiddlewareTest.php | 24 +++++ tests/Security/SecurityRuleContextTest.php | 99 +++++++++++++++++++ website/docs/attributes-reference.md | 2 +- website/docs/fine-grained-security.mdx | 47 ++++++++- website/docs/implementing-security.md | 5 + 9 files changed, 315 insertions(+), 4 deletions(-) create mode 100644 src/Security/SecurityRuleContextInterface.php diff --git a/src/Security/SecurityRuleContext.php b/src/Security/SecurityRuleContext.php index e0b852e210..4feeb6b99b 100644 --- a/src/Security/SecurityRuleContext.php +++ b/src/Security/SecurityRuleContext.php @@ -17,8 +17,12 @@ * information is added as new readonly properties or new methods so that existing rules keep * compiling; the constructor is called only by GraphQLite's own Security middlewares and should be * treated as internal. + * + * A rule needing nothing GraphQL-specific can type-hint {@see SecurityRuleContextInterface} + * instead, which is this same surface expressed as a contract, so the rule can be unit tested + * against a fake context and reused outside a field resolution. */ -final class SecurityRuleContext +final class SecurityRuleContext implements SecurityRuleContextInterface { /** * @param object|null $user The currently authenticated user, or null when nobody is logged in. @@ -34,6 +38,38 @@ public function __construct( ) { } + /** + * The currently authenticated user, or null when nobody is logged in. + * + * The contract's equivalent of the `$user` property. + */ + public function getUser(): object|null + { + return $this->user; + } + + /** + * The object the field is being resolved on, if there is one. + * + * The contract's equivalent of the `$source` property. + */ + public function getSource(): object|null + { + return $this->source; + } + + /** + * The field's resolved PHP arguments, keyed by parameter name. + * + * The contract's equivalent of the `$arguments` property. + * + * @return array + */ + public function getArguments(): array + { + return $this->arguments; + } + /** * Whether the current user holds $right, optionally against a specific subject. * diff --git a/src/Security/SecurityRuleContextInterface.php b/src/Security/SecurityRuleContextInterface.php new file mode 100644 index 0000000000..61c50d6477 --- /dev/null +++ b/src/Security/SecurityRuleContextInterface.php @@ -0,0 +1,53 @@ + + */ + public function getArguments(): array; + + /** Whether the current user holds $right, optionally against a specific subject. */ + public function isGranted(string $right, mixed $subject = null): bool; + + /** Whether anybody is currently logged in. */ + public function isLogged(): bool; + + /** The value named $name, or null when there is no such value. */ + public function argument(string $name): mixed; + + /** + * Whether a value named $name was supplied. + * + * Distinguishes a value that was genuinely null from one that does not exist, which + * {@see argument()} cannot. + */ + public function hasArgument(string $name): bool; +} diff --git a/tests/Fixtures/Integration/Controllers/SecurityController.php b/tests/Fixtures/Integration/Controllers/SecurityController.php index 181f7d3d1e..a5afaa1c2c 100644 --- a/tests/Fixtures/Integration/Controllers/SecurityController.php +++ b/tests/Fixtures/Integration/Controllers/SecurityController.php @@ -12,6 +12,7 @@ use TheCodingMachine\GraphQLite\Fixtures\PageSizeWithin; use TheCodingMachine\GraphQLite\Fixtures\SecretIs; use TheCodingMachine\GraphQLite\Security\SecurityRuleContext; +use TheCodingMachine\GraphQLite\Security\SecurityRuleContextInterface; class SecurityController { @@ -65,6 +66,17 @@ public function getSecretPhraseByRule(string $secret): string return 'you can see this secret only if passed parameter is "foo"'; } + /** + * The rule type-hints the contract rather than the concrete context, and reads the arguments + * through the accessor an interface can declare on PHP 8.2. + */ + #[Query] + #[Security(rule: [self::class, 'secretIsFooByContract'], message: 'Wrong secret passed')] + public function getSecretPhraseByContractRule(string $secret): string + { + return 'you can see this secret only if passed parameter is "foo"'; + } + #[Query] #[Security(rule: new SecretIs('foo'), failWith: null)] public function getNullableSecretPhraseByRule(string $secret): string @@ -154,6 +166,11 @@ public static function secretIsFoo(SecurityRuleContext $context): bool return $context->argument('secret') === 'foo'; } + public static function secretIsFooByContract(SecurityRuleContextInterface $context): bool + { + return ($context->getArguments()['secret'] ?? null) === 'foo'; + } + public static function userBarIs42(SecurityRuleContext $context): bool { return $context->user !== null && $context->user->bar === 42; diff --git a/tests/Integration/EndToEndTest.php b/tests/Integration/EndToEndTest.php index 529e27bd4f..08b6ab66aa 100644 --- a/tests/Integration/EndToEndTest.php +++ b/tests/Integration/EndToEndTest.php @@ -1162,6 +1162,40 @@ public function testEndToEndSecurityRule(): void $result->toArray(DebugFlag::RETHROW_INTERNAL_EXCEPTIONS); } + /** + * A rule type-hinting SecurityRuleContextInterface rather than the concrete context is invoked + * exactly like any other, through a real schema and a real query. + * + * The rule reads its arguments with getArguments(), the accessor that exists because an + * interface cannot declare the readonly properties on PHP 8.2. + */ + public function testEndToEndSecurityRuleTypeHintingTheContract(): void + { + $schema = $this->mainContainer->get(Schema::class); + assert($schema instanceof Schema); + + $result = GraphQL::executeQuery($schema, ' + query { + secretPhraseByContractRule(secret: "foo") + } + '); + + $this->assertSame( + ['secretPhraseByContractRule' => 'you can see this secret only if passed parameter is "foo"'], + $this->getSuccessResult($result), + ); + + $result = GraphQL::executeQuery($schema, ' + query { + secretPhraseByContractRule(secret: "bar") + } + '); + + $this->expectException(MissingAuthorizationException::class); + $this->expectExceptionMessage('Wrong secret passed'); + $result->toArray(DebugFlag::RETHROW_INTERNAL_EXCEPTIONS); + } + /** * An invokable object is how a rule is parameterized: attribute arguments are constant * expressions, so a callable written in an attribute cannot capture or partially apply. diff --git a/tests/Middlewares/SecurityFieldMiddlewareTest.php b/tests/Middlewares/SecurityFieldMiddlewareTest.php index 03b95f7b46..6b4c6cca12 100644 --- a/tests/Middlewares/SecurityFieldMiddlewareTest.php +++ b/tests/Middlewares/SecurityFieldMiddlewareTest.php @@ -19,6 +19,7 @@ use TheCodingMachine\GraphQLite\QueryFieldDescriptor; use TheCodingMachine\GraphQLite\Security\SecurityExpressionLanguageProvider; use TheCodingMachine\GraphQLite\Security\SecurityRuleContext; +use TheCodingMachine\GraphQLite\Security\SecurityRuleContextInterface; use TheCodingMachine\GraphQLite\Security\VoidAuthenticationService; use TheCodingMachine\GraphQLite\Security\VoidAuthorizationService; @@ -152,6 +153,29 @@ public function testClosureRuleIsHandedTheSecurityRuleContext(): void self::assertInstanceOf(SecurityRuleContext::class, $seen); } + /** + * A rule may type-hint the contract rather than the concrete context. + * + * The middleware declares no parameter type on the rule, so this needs nothing from the library + * beyond SecurityRuleContext implementing the interface. The test pins that, since a rule + * written this way is what makes the rule unit testable and reusable outside a resolution. + */ + public function testRuleMayTypeHintTheContract(): void + { + $seen = null; + + $field = $this->process([ + new Security(rule: static function (SecurityRuleContextInterface $context) use (&$seen): bool { + $seen = $context; + + return $context->getArguments() === []; + }), + ]); + + self::assertSame('resolved', ($field->resolveFn)(null)); + self::assertInstanceOf(SecurityRuleContext::class, $seen); + } + /** * First-class callable syntax — `Rules::allow(...)` — written directly in the attribute. * diff --git a/tests/Security/SecurityRuleContextTest.php b/tests/Security/SecurityRuleContextTest.php index d8bec35ef0..fa65f9df2b 100644 --- a/tests/Security/SecurityRuleContextTest.php +++ b/tests/Security/SecurityRuleContextTest.php @@ -5,6 +5,8 @@ use PHPUnit\Framework\TestCase; use stdClass; +use function array_key_exists; + class SecurityRuleContextTest extends TestCase { public function testExposesUserSourceAndArguments(): void @@ -19,6 +21,32 @@ public function testExposesUserSourceAndArguments(): void self::assertSame(['first' => 10, 'search' => null], $context->arguments); } + /** + * The contract is what a rule may type-hint, so the context GraphQLite actually builds has to + * satisfy it. + */ + public function testSatisfiesTheContract(): void + { + self::assertInstanceOf(SecurityRuleContextInterface::class, $this->context(null, null, [])); + } + + /** + * The accessors exist because an interface cannot declare properties on PHP 8.2, so they must + * report exactly what the readonly properties hold. A rule reads one or the other and must not + * be able to tell which it got. + */ + public function testAccessorsAgreeWithTheReadonlyProperties(): void + { + $user = new stdClass(); + $source = new stdClass(); + + $context = $this->context($user, $source, ['first' => 10, 'search' => null]); + + self::assertSame($context->user, $context->getUser()); + self::assertSame($context->source, $context->getSource()); + self::assertSame($context->arguments, $context->getArguments()); + } + public function testArgumentReadsByName(): void { $context = $this->context(null, null, ['first' => 10]); @@ -100,6 +128,77 @@ public function testIsLoggedDelegatesToTheAuthenticationService(): void self::assertTrue($context->isLogged()); } + /** + * The reason the contract exists. + * + * A rule written against it is decided by whatever context the caller supplies, so it runs with + * no schema, no query and no security services in play, and can be reused anywhere a caller can + * name a user, a subject and a set of values. + */ + public function testRuleTypeHintingTheContractRunsAgainstAnyImplementation(): void + { + $rule = static fn (SecurityRuleContextInterface $context): bool => $context->isLogged() + && $context->argument('secret') === 'foo'; + + self::assertTrue($rule($this->fakeContext(true, ['secret' => 'foo']))); + self::assertFalse($rule($this->fakeContext(true, ['secret' => 'bar']))); + self::assertFalse($rule($this->fakeContext(false, ['secret' => 'foo']))); + } + + /** + * A hand-written implementation, deliberately not a mock: it proves the contract asks for + * nothing a caller outside GraphQLite cannot provide. + * + * @param array $arguments + */ + private function fakeContext(bool $isLogged, array $arguments): SecurityRuleContextInterface + { + return new class ($isLogged, $arguments) implements SecurityRuleContextInterface { + /** @param array $arguments */ + public function __construct( + private readonly bool $isLogged, + private readonly array $arguments, + ) { + } + + public function getUser(): object|null + { + return null; + } + + public function getSource(): object|null + { + return null; + } + + /** @return array */ + public function getArguments(): array + { + return $this->arguments; + } + + public function isGranted(string $right, mixed $subject = null): bool + { + return false; + } + + public function isLogged(): bool + { + return $this->isLogged; + } + + public function argument(string $name): mixed + { + return $this->arguments[$name] ?? null; + } + + public function hasArgument(string $name): bool + { + return array_key_exists($name, $this->arguments); + } + }; + } + /** @param array $arguments */ private function context(object|null $user, object|null $source, array $arguments): SecurityRuleContext { diff --git a/website/docs/attributes-reference.md b/website/docs/attributes-reference.md index c6d35ba56c..0f3cadcafa 100644 --- a/website/docs/attributes-reference.md +++ b/website/docs/attributes-reference.md @@ -262,7 +262,7 @@ Repeatable: every `#[Security]` attribute declared on a field must pass. Attribute | Compulsory | Type | Definition ---------------|------------|----------------|-------- expression | see below | string | A security expression, evaluated by Symfony ExpressionLanguage. Also accepted as the attribute's first positional argument: `#[Security("is_granted('X')")]` -rule | see below | array \| object | A callable receiving a `SecurityRuleContext` and returning `bool`. Must be passed by name (`rule:`) — the first positional argument is the expression, so a positional callable is either a `TypeError` or silently read as a legacy data array. Prefer first-class callable syntax (`self::canShow(...)`, PHP 8.5+); `[Rules::class, 'canShow']` works on 8.2+ and is the only form that can name a container-resolved method. A bare method-name string is **not** accepted here, unlike `#[Prefetch]` +rule | see below | array \| object | A callable receiving a `SecurityRuleContext`, or the `SecurityRuleContextInterface` it implements, and returning `bool`. Must be passed by name (`rule:`) — the first positional argument is the expression, so a positional callable is either a `TypeError` or silently read as a legacy data array. Prefer first-class callable syntax (`self::canShow(...)`, PHP 8.5+); `[Rules::class, 'canShow']` works on 8.2+ and is the only form that can name a container-resolved method. A bare method-name string is **not** accepted here, unlike `#[Prefetch]` failWith | *no* | mixed | Value returned instead of denying. Cannot be combined with *message* or *statusCode* message | *no* | string | Error message when access is denied. Defaults to `Access denied.` statusCode | *no* | int | Status code when access is denied. Defaults to `403` diff --git a/website/docs/fine-grained-security.mdx b/website/docs/fine-grained-security.mdx index 9a8fad490b..53d7d45f86 100644 --- a/website/docs/fine-grained-security.mdx +++ b/website/docs/fine-grained-security.mdx @@ -76,8 +76,9 @@ you would with any `mixed` input. ### Ways to write a rule -Every form below receives the same `SecurityRuleContext` and must return `bool`. They differ only in -what they can express and which PHP version accepts them. +Every form below receives the same `SecurityRuleContext` and must return `bool`. (A rule may also +type-hint the interface that class implements; see [the rule contract](#the-rule-contract).) They +differ only in what they can express and which PHP version accepts them. | Form | PHP | Use it when | |---|---|---| @@ -185,6 +186,48 @@ version — and the constructor call is type-checked by static analysis like any This is the one job first-class callable syntax cannot do: `self::pageSizeWithin(...)` has nowhere to put the `100`. Construct the rule instead of naming it. +## The rule contract + +Everything the context offers is declared by +`TheCodingMachine\GraphQLite\Security\SecurityRuleContextInterface`, which `SecurityRuleContext` +implements. A rule may type-hint the interface instead of the class: + +```php +use TheCodingMachine\GraphQLite\Security\SecurityRuleContextInterface; + +final class PostRules +{ + public static function canShow(SecurityRuleContextInterface $context): bool + { + return $context->isGranted('ROLE_ADMIN') + || $context->isGranted('POST_SHOW', $context->argument('post')); + } +} +``` + +GraphQLite passes the same object either way, so nothing about how the rule is invoked changes. +What changes is what else can call it: + +* **The rule is unit testable with a fake context.** Implement the interface over the user, subject + and values a case needs, then call the rule directly. No schema to build, no query to execute and + no authentication or authorization service to stand up. +* **The rule is reusable outside GraphQL.** An HTTP middleware, a console command or a message + handler can implement the interface over what it already has and reach the same decision, so the + rule stays the single place that check is written. + +A PHP interface cannot declare properties before 8.4 and GraphQLite supports 8.2, so the three +pieces of data the context carries appear on the interface as methods: + +| On `SecurityRuleContext` | On the interface | +|---|---| +| `$context->user` | `$context->getUser()` | +| `$context->source` | `$context->getSource()` | +| `$context->arguments` | `$context->getArguments()` | + +`isGranted()`, `isLogged()`, `argument()` and `hasArgument()` are identical on both. The readonly +properties stay on `SecurityRuleContext`, so a rule already type-hinting the concrete class needs no +change. + ## Setting HTTP code and error message You can use the `statusCode` and `message` attributes to set the HTTP code and GraphQL error message. diff --git a/website/docs/implementing-security.md b/website/docs/implementing-security.md index b45e129333..483f51bbc0 100644 --- a/website/docs/implementing-security.md +++ b/website/docs/implementing-security.md @@ -23,6 +23,11 @@ To plug GraphQLite to your framework's security mechanism, you will have to prov SecurityRuleContext::isLogged() rather than depending on these interfaces directly. They give a rule the same reach through a surface GraphQLite controls, so your rules keep compiling as these interfaces evolve. +

+ That surface is itself declared by SecurityRuleContextInterface, in the same + namespace. Type-hint it in a rule and the rule can be unit tested against a fake context and + called from outside GraphQL, where an HTTP middleware or a console command supplies the + context. See the rule contract. Those two interfaces act as adapters between GraphQLite and your framework: From 4b486283cf44f5d4eab66337e403f9e13f81e873 Mon Sep 17 00:00:00 2001 From: Jacob Thomason Date: Thu, 30 Jul 2026 03:33:40 -0400 Subject: [PATCH 4/4] feat: let a #[Security] rule state its own refusal message A rule is reusable, but its refusal message was not: every call site had to repeat the same sentence, and a rule could not explain why it refused. SecurityRuleMessageInterface lets a rule supply that message. Precedence is explicit message, then the rule's, then "Access denied.", so the feature is opt in and no existing attribute changes behavior. Security now stores the message as given and resolves the default on read, which is what lets the middleware tell an absent message from an explicit one. --- src/Annotations/Security.php | 36 +++- src/Middlewares/SecurityFieldMiddleware.php | 28 ++- .../SecurityInputFieldMiddleware.php | 24 ++- src/Security/SecurityRuleMessageInterface.php | 50 +++++ tests/Annotations/SecurityTest.php | 79 ++++++++ .../Controllers/SecurityController.php | 11 ++ tests/Fixtures/PageSizeWithin.php | 11 +- tests/Integration/EndToEndTest.php | 33 ++++ .../SecurityFieldMiddlewareTest.php | 94 ++++++++++ .../SecurityInputFieldMiddlewareTest.php | 171 ++++++++++++++++++ website/docs/attributes-reference.md | 4 +- website/docs/fine-grained-security.mdx | 75 ++++++++ 12 files changed, 608 insertions(+), 8 deletions(-) create mode 100644 src/Security/SecurityRuleMessageInterface.php create mode 100644 tests/Middlewares/SecurityInputFieldMiddlewareTest.php diff --git a/src/Annotations/Security.php b/src/Annotations/Security.php index ef5a5b532a..e52cbb8597 100644 --- a/src/Annotations/Security.php +++ b/src/Annotations/Security.php @@ -22,7 +22,16 @@ class Security implements MiddlewareAnnotationInterface private mixed $failWith; private bool $failWithIsSet = false; private int $statusCode; - private string $message; + + /** + * The message exactly as it was written in the attribute, or null when none was written. + * + * The default is applied by {@see getMessage()} rather than here, so that "no message given" + * stays distinguishable from "a message was given, and it happens to read like the default". + * A middleware with a better message available, such as one the rule states itself, can only + * prefer it over the default if it can tell those two apart. See {@see hasMessage()}. + */ + private string|null $message; /** * @param array|string $data data array managed by the Doctrine Annotations library or the expression @@ -78,8 +87,11 @@ public function __construct( $this->failWith = $failWith; $this->failWithIsSet = true; } - $this->message = $message ?? $data['message'] ?? 'Access denied.'; + $this->message = $message ?? $data['message'] ?? null; $this->statusCode = $statusCode ?? $data['statusCode'] ?? 403; + // Deliberately reads the raw arguments rather than $this->message: an attribute passing + // failWith together with a message has always been an error, and storing the message + // unresolved must not change which combinations are rejected. if ($this->failWithIsSet === true && (($message || isset($data['message'])) || ($statusCode || isset($data['statusCode'])))) { throw new BadMethodCallException('A #[Security] attribute that has "failWith" attribute set cannot have a message or a statusCode attribute.'); } @@ -134,8 +146,26 @@ public function getStatusCode(): int return $this->statusCode; } + /** + * Whether a message was written in the attribute. + * + * A middleware holding a better message than the default, such as one a rule states itself, + * branches on this: an explicit message is the field author's decision and always wins, while + * an absent one leaves the middleware free to supply its own before falling back to + * {@see getMessage()}. + */ + public function hasMessage(): bool + { + return $this->message !== null; + } + + /** + * The message to deny with, defaulting to "Access denied." when the attribute gave none. + * + * Always a string, so a middleware that has nothing better to offer can call this alone. + */ public function getMessage(): string { - return $this->message; + return $this->message ?? 'Access denied.'; } } diff --git a/src/Middlewares/SecurityFieldMiddleware.php b/src/Middlewares/SecurityFieldMiddleware.php index 030702c765..32e98d9546 100644 --- a/src/Middlewares/SecurityFieldMiddleware.php +++ b/src/Middlewares/SecurityFieldMiddleware.php @@ -19,6 +19,7 @@ use TheCodingMachine\GraphQLite\Security\AuthenticationServiceInterface; use TheCodingMachine\GraphQLite\Security\AuthorizationServiceInterface; use TheCodingMachine\GraphQLite\Security\SecurityRuleContext; +use TheCodingMachine\GraphQLite\Security\SecurityRuleMessageInterface; use Throwable; use function array_combine; @@ -147,7 +148,7 @@ public function process( return $failWith->getValue(); } - throw new MissingAuthorizationException($annotation->getMessage(), $annotation->getStatusCode()); + throw new MissingAuthorizationException($this->resolveMessage($annotation), $annotation->getStatusCode()); } } @@ -221,6 +222,31 @@ private function normalizeRules(array $annotations): array return $rules; } + /** + * The message a denied field is reported with. + * + * A message written on the attribute is the field author's decision about this field, so it + * always wins. Failing that, a rule stating its own message supplies one; failing that too, + * getMessage() returns "Access denied.", which keeps the default written in a single place. + * + * The rule is read back from the annotation rather than from the normalized Closure. Rules are + * normalized to Closures at schema build, and a Closure can be called but not asked anything; + * the annotation still holds the rule exactly as the attribute wrote it, which is where an + * invokable rule object remains reachable. Nothing about the normalization needs to change. + */ + private function resolveMessage(Security $annotation): string + { + if (! $annotation->hasMessage()) { + $rule = $annotation->getRule(); + + if ($rule instanceof SecurityRuleMessageInterface) { + return $rule->getRefusalMessage(); + } + } + + return $annotation->getMessage(); + } + /** * The field's resolved PHP arguments, keyed by parameter name. * diff --git a/src/Middlewares/SecurityInputFieldMiddleware.php b/src/Middlewares/SecurityInputFieldMiddleware.php index 93840daf70..7899c790c0 100644 --- a/src/Middlewares/SecurityInputFieldMiddleware.php +++ b/src/Middlewares/SecurityInputFieldMiddleware.php @@ -16,6 +16,7 @@ use TheCodingMachine\GraphQLite\Security\AuthenticationServiceInterface; use TheCodingMachine\GraphQLite\Security\AuthorizationServiceInterface; use TheCodingMachine\GraphQLite\Security\SecurityRuleContext; +use TheCodingMachine\GraphQLite\Security\SecurityRuleMessageInterface; use Throwable; use function array_combine; @@ -104,7 +105,7 @@ public function process(InputFieldDescriptor $inputFieldDescriptor, InputFieldHa } if (! $authorized) { - throw new MissingAuthorizationException($annotation->getMessage(), $annotation->getStatusCode()); + throw new MissingAuthorizationException($this->resolveMessage($annotation), $annotation->getStatusCode()); } } @@ -176,6 +177,27 @@ private function normalizeRules(array $annotations): array return $rules; } + /** + * The message a denied input field is reported with. See the sibling SecurityFieldMiddleware. + * + * A message written on the attribute always wins; failing that, a rule stating its own message + * supplies one; failing that too, getMessage() returns the default. The rule is read back from + * the annotation because normalization has replaced it with a Closure by this point, and only + * the annotation still holds the object the attribute wrote. + */ + private function resolveMessage(Security $annotation): string + { + if (! $annotation->hasMessage()) { + $rule = $annotation->getRule(); + + if ($rule instanceof SecurityRuleMessageInterface) { + return $rule->getRefusalMessage(); + } + } + + return $annotation->getMessage(); + } + /** * @param array $args * @param array $parameters diff --git a/src/Security/SecurityRuleMessageInterface.php b/src/Security/SecurityRuleMessageInterface.php new file mode 100644 index 0000000000..d3a0e8402d --- /dev/null +++ b/src/Security/SecurityRuleMessageInterface.php @@ -0,0 +1,50 @@ +argument('first') <= $this->max; + * } + * + * public function getRefusalMessage(): string + * { + * return 'Page size must be at most ' . $this->max . '.'; + * } + * } + */ +interface SecurityRuleMessageInterface +{ + /** + * The message the field is denied with when this rule refuses. + * + * Read only on refusal, and only when the attribute gave no message of its own. It reaches the + * client, so it should say what a caller is allowed to know and nothing more. + */ + public function getRefusalMessage(): string; +} diff --git a/tests/Annotations/SecurityTest.php b/tests/Annotations/SecurityTest.php index 23fb9dba1d..ee3b916fab 100644 --- a/tests/Annotations/SecurityTest.php +++ b/tests/Annotations/SecurityTest.php @@ -99,4 +99,83 @@ public function testFailWithIsUnaffectedByRules(): void self::assertTrue($security->isFailWithSet()); self::assertNull($security->getFailWith()); } + + /** The message is stored unresolved, but reading it must still yield what it always has. */ + public function testMessageDefaultsToAccessDenied(): void + { + $security = new Security('foo'); + + self::assertFalse($security->hasMessage()); + self::assertSame('Access denied.', $security->getMessage()); + } + + public function testExplicitMessageIsReportedAsGiven(): void + { + $security = new Security('foo', message: 'Nope'); + + self::assertTrue($security->hasMessage()); + self::assertSame('Nope', $security->getMessage()); + } + + /** + * The distinction the feature rests on: a message reading exactly like the default is still a + * message the field author wrote, so a middleware must not treat it as absent and substitute + * something of its own. + */ + public function testMessageWrittenOutAsTheDefaultStillCountsAsGiven(): void + { + $security = new Security('foo', message: 'Access denied.'); + + self::assertTrue($security->hasMessage()); + self::assertSame('Access denied.', $security->getMessage()); + } + + public function testMessageInTheDataArrayCountsAsGiven(): void + { + $security = new Security(['expression' => 'foo', 'message' => 'Nope']); + + self::assertTrue($security->hasMessage()); + self::assertSame('Nope', $security->getMessage()); + } + + public function testFailWithAndMessageAreStillMutuallyExclusive(): void + { + $this->expectException(BadMethodCallException::class); + $this->expectExceptionMessage('A #[Security] attribute that has "failWith" attribute set cannot have a message or a statusCode attribute.'); + new Security('foo', failWith: null, message: 'Nope'); + } + + public function testFailWithAndDataArrayMessageAreStillMutuallyExclusive(): void + { + $this->expectException(BadMethodCallException::class); + $this->expectExceptionMessage('A #[Security] attribute that has "failWith" attribute set cannot have a message or a statusCode attribute.'); + new Security(['expression' => 'foo', 'failWith' => null, 'message' => 'Nope']); + } + + /** + * failWith on its own has never been an error, and the message it reports has always been the + * default. Storing the message unresolved must not turn either of those into a change. + */ + public function testFailWithAloneIsStillAccepted(): void + { + $security = new Security('foo', failWith: null); + + self::assertTrue($security->isFailWithSet()); + self::assertFalse($security->hasMessage()); + self::assertSame('Access denied.', $security->getMessage()); + } + + /** + * Pins the exclusion's exact trigger. It tests the raw argument for truthiness, so an empty + * message has never fired it, quirk and all. Testing the stored message instead would have + * silently widened the rule, since an empty string is a message that was written. + */ + public function testFailWithAndAnEmptyMessageAreStillAccepted(): void + { + $security = new Security('foo', failWith: null, message: ''); + + self::assertTrue($security->isFailWithSet()); + self::assertTrue($security->hasMessage()); + self::assertSame('', $security->getMessage()); + } } diff --git a/tests/Fixtures/Integration/Controllers/SecurityController.php b/tests/Fixtures/Integration/Controllers/SecurityController.php index a5afaa1c2c..5997dae63e 100644 --- a/tests/Fixtures/Integration/Controllers/SecurityController.php +++ b/tests/Fixtures/Integration/Controllers/SecurityController.php @@ -95,6 +95,17 @@ public function getPagedSecret(int $first): string return 'you can see this secret only if first is within the configured limit'; } + /** + * The same rule, with no message written in the attribute: the refusal is reported with the + * one the rule states, which quotes the limit only the rule knows. + */ + #[Query] + #[Security(rule: new PageSizeWithin(10))] + public function getPagedSecretUsingTheRuleMessage(int $first): string + { + return 'you can see this secret only if first is within the configured limit'; + } + #[Query] #[Security(rule: [self::class, 'userBarIs42'])] public function getSecretUsingUserByRule(): string diff --git a/tests/Fixtures/PageSizeWithin.php b/tests/Fixtures/PageSizeWithin.php index ea971757f9..6a7802f113 100644 --- a/tests/Fixtures/PageSizeWithin.php +++ b/tests/Fixtures/PageSizeWithin.php @@ -5,6 +5,7 @@ namespace TheCodingMachine\GraphQLite\Fixtures; use TheCodingMachine\GraphQLite\Security\SecurityRuleContext; +use TheCodingMachine\GraphQLite\Security\SecurityRuleMessageInterface; use function is_int; @@ -15,8 +16,11 @@ * capture or partially apply on any PHP version. Constructing the rule in the attribute can, which * is why a threshold like this needs no "extra arguments" array: `new PageSizeWithin(100)` carries * the limit, and static analysis checks the constructor call. + * + * It also states its own refusal message, which is the case for the contract: the limit is known + * here and nowhere else, so no field guarded by this rule has to repeat it in a `message:`. */ -final class PageSizeWithin +final class PageSizeWithin implements SecurityRuleMessageInterface { public function __construct(private readonly int $max) { @@ -28,4 +32,9 @@ public function __invoke(SecurityRuleContext $context): bool return is_int($requested) && $requested <= $this->max; } + + public function getRefusalMessage(): string + { + return 'Page size must be at most ' . $this->max . '.'; + } } diff --git a/tests/Integration/EndToEndTest.php b/tests/Integration/EndToEndTest.php index 08b6ab66aa..42d44e80bc 100644 --- a/tests/Integration/EndToEndTest.php +++ b/tests/Integration/EndToEndTest.php @@ -1285,6 +1285,39 @@ public function testEndToEndSecurityRuleEnforcesItsConstructorBoundaryExactly(): $result->toArray(DebugFlag::RETHROW_INTERNAL_EXCEPTIONS); } + /** + * A rule stating its own refusal message is denied with it, with no `message:` on the field. + * + * The same rule guards `pagedSecret`, which does write a `message:` and is refused with that + * one instead: the field author's message outranks the rule's. + */ + public function testEndToEndSecurityRuleSuppliesItsOwnRefusalMessage(): void + { + $schema = $this->mainContainer->get(Schema::class); + assert($schema instanceof Schema); + + $result = GraphQL::executeQuery($schema, ' + query { + pagedSecretUsingTheRuleMessage(first: 5) + } + '); + + $this->assertSame( + ['pagedSecretUsingTheRuleMessage' => 'you can see this secret only if first is within the configured limit'], + $this->getSuccessResult($result), + ); + + $result = GraphQL::executeQuery($schema, ' + query { + pagedSecretUsingTheRuleMessage(first: 11) + } + '); + + $this->expectException(MissingAuthorizationException::class); + $this->expectExceptionMessage('Page size must be at most 10.'); + $result->toArray(DebugFlag::RETHROW_INTERNAL_EXCEPTIONS); + } + /** * The rule equivalent of the `this` expression variable. */ diff --git a/tests/Middlewares/SecurityFieldMiddlewareTest.php b/tests/Middlewares/SecurityFieldMiddlewareTest.php index 6b4c6cca12..475ddcec47 100644 --- a/tests/Middlewares/SecurityFieldMiddlewareTest.php +++ b/tests/Middlewares/SecurityFieldMiddlewareTest.php @@ -20,6 +20,7 @@ use TheCodingMachine\GraphQLite\Security\SecurityExpressionLanguageProvider; use TheCodingMachine\GraphQLite\Security\SecurityRuleContext; use TheCodingMachine\GraphQLite\Security\SecurityRuleContextInterface; +use TheCodingMachine\GraphQLite\Security\SecurityRuleMessageInterface; use TheCodingMachine\GraphQLite\Security\VoidAuthenticationService; use TheCodingMachine\GraphQLite\Security\VoidAuthorizationService; @@ -226,6 +227,99 @@ public static function denyEverything(SecurityRuleContext $context): bool return false; } + /** + * A rule stating its own message is denied with it, with nothing written in the attribute. + * + * This is what the contract buys: the reason lives beside the check that produces it, so no + * field guarded by the rule repeats it. + */ + public function testRuleSuppliesTheRefusalMessageWhenTheAttributeGivesNone(): void + { + $field = $this->process([new Security(rule: $this->refusingRule('Page size must be at most 100.'))]); + + self::assertNotNull($field); + + $this->expectException(MissingAuthorizationException::class); + $this->expectExceptionMessage('Page size must be at most 100.'); + ($field->resolveFn)(null); + } + + /** A message on the attribute is about this field specifically, so it outranks the rule's. */ + public function testExplicitMessageWinsOverTheRuleSuppliedOne(): void + { + $field = $this->process([ + new Security(rule: $this->refusingRule('From the rule'), message: 'From the attribute'), + ]); + + self::assertNotNull($field); + + $this->expectException(MissingAuthorizationException::class); + $this->expectExceptionMessage('From the attribute'); + ($field->resolveFn)(null); + } + + /** + * Including when what the attribute wrote happens to be the default word for word. Resolving + * the default in the annotation's constructor made this case indistinguishable from an absent + * message, which is what stopped a rule from supplying one at all. + */ + public function testExplicitMessageWinsEvenWhenItReadsLikeTheDefault(): void + { + $field = $this->process([ + new Security(rule: $this->refusingRule('From the rule'), message: 'Access denied.'), + ]); + + self::assertNotNull($field); + + $this->expectException(MissingAuthorizationException::class); + $this->expectExceptionMessage('Access denied.'); + ($field->resolveFn)(null); + } + + /** Opt in: a rule that does not implement the contract is denied exactly as it always was. */ + public function testRuleWithoutTheContractIsDeniedWithTheDefaultMessage(): void + { + $field = $this->process([new Security(rule: static fn (SecurityRuleContext $context): bool => false)]); + + self::assertNotNull($field); + + $this->expectException(MissingAuthorizationException::class); + $this->expectExceptionMessage('Access denied.'); + ($field->resolveFn)(null); + } + + /** An expression has no object to ask, so it keeps the default too. */ + public function testExpressionIsDeniedWithTheDefaultMessage(): void + { + $field = $this->process([new Security('user != null')]); + + self::assertNotNull($field); + + $this->expectException(MissingAuthorizationException::class); + $this->expectExceptionMessage('Access denied.'); + ($field->resolveFn)(null); + } + + /** A rule that refuses everything, stating $message as the reason. */ + private function refusingRule(string $message): SecurityRuleMessageInterface + { + return new class ($message) implements SecurityRuleMessageInterface { + public function __construct(private readonly string $message) + { + } + + public function __invoke(SecurityRuleContextInterface $context): bool + { + return false; + } + + public function getRefusalMessage(): string + { + return $this->message; + } + }; + } + /** @param MiddlewareAnnotationInterface[] $annotations */ private function process(array $annotations, ExpressionLanguage|null $language = null): FieldDefinition|null { diff --git a/tests/Middlewares/SecurityInputFieldMiddlewareTest.php b/tests/Middlewares/SecurityInputFieldMiddlewareTest.php new file mode 100644 index 0000000000..dcea2cdae4 --- /dev/null +++ b/tests/Middlewares/SecurityInputFieldMiddlewareTest.php @@ -0,0 +1,171 @@ +process([new Security(rule: static fn (SecurityRuleContext $context): bool => true)]); + + self::assertNotNull($field); + self::assertSame('resolved', $this->resolveField($field)); + } + + /** A rule stating its own message is denied with it, with nothing written in the attribute. */ + public function testRuleSuppliesTheRefusalMessageWhenTheAttributeGivesNone(): void + { + $field = $this->process([new Security(rule: $this->refusingRule('Page size must be at most 100.'))]); + + self::assertNotNull($field); + + $this->expectException(MissingAuthorizationException::class); + $this->expectExceptionMessage('Page size must be at most 100.'); + $this->resolveField($field); + } + + /** A message on the attribute is about this input field specifically, so it outranks the rule's. */ + public function testExplicitMessageWinsOverTheRuleSuppliedOne(): void + { + $field = $this->process([ + new Security(rule: $this->refusingRule('From the rule'), message: 'From the attribute'), + ]); + + self::assertNotNull($field); + + $this->expectException(MissingAuthorizationException::class); + $this->expectExceptionMessage('From the attribute'); + $this->resolveField($field); + } + + /** Including when what the attribute wrote happens to be the default word for word. */ + public function testExplicitMessageWinsEvenWhenItReadsLikeTheDefault(): void + { + $field = $this->process([ + new Security(rule: $this->refusingRule('From the rule'), message: 'Access denied.'), + ]); + + self::assertNotNull($field); + + $this->expectException(MissingAuthorizationException::class); + $this->expectExceptionMessage('Access denied.'); + $this->resolveField($field); + } + + /** Opt in: a rule that does not implement the contract is denied exactly as it always was. */ + public function testRuleWithoutTheContractIsDeniedWithTheDefaultMessage(): void + { + $field = $this->process([new Security(rule: static fn (SecurityRuleContext $context): bool => false)]); + + self::assertNotNull($field); + + $this->expectException(MissingAuthorizationException::class); + $this->expectExceptionMessage('Access denied.'); + $this->resolveField($field); + } + + /** An expression has no object to ask, so it keeps the default too. */ + public function testExpressionIsDeniedWithTheDefaultMessage(): void + { + $field = $this->process([new Security('user != null')]); + + self::assertNotNull($field); + + $this->expectException(MissingAuthorizationException::class); + $this->expectExceptionMessage('Access denied.'); + $this->resolveField($field); + } + + /** A rule that refuses everything, stating $message as the reason. */ + private function refusingRule(string $message): SecurityRuleMessageInterface + { + return new class ($message) implements SecurityRuleMessageInterface { + public function __construct(private readonly string $message) + { + } + + public function __invoke(SecurityRuleContextInterface $context): bool + { + return false; + } + + public function getRefusalMessage(): string + { + return $this->message; + } + }; + } + + /** @param MiddlewareAnnotationInterface[] $annotations */ + private function process(array $annotations): InputField|null + { + $descriptor = new InputFieldDescriptor( + name: 'foo', + type: Type::string(), + resolver: static fn (): string => 'resolved', + originalResolver: new ServiceResolver([new VoidAuthorizationService(), 'isAllowed']), + middlewareAnnotations: new MiddlewareAnnotations($annotations), + ); + + $language = new ExpressionLanguage(); + $language->registerProvider(new SecurityExpressionLanguageProvider()); + + $middleware = new SecurityInputFieldMiddleware( + $language, + new VoidAuthenticationService(), + new VoidAuthorizationService(), + new CallableResolver(new EmptyContainer()), + ); + + return $middleware->process($descriptor, new class implements InputFieldHandlerInterface { + public function handle(InputFieldDescriptor $inputFieldDescriptor): InputField|null + { + return new InputField( + name: $inputFieldDescriptor->getName(), + type: $inputFieldDescriptor->getType(), + arguments: ['foo' => new SourceParameter()], + originalResolver: $inputFieldDescriptor->getOriginalResolver(), + resolver: $inputFieldDescriptor->getResolver(), + forConstructorHydration: false, + description: null, + isUpdate: false, + hasDefaultValue: false, + defaultValue: null, + ); + } + }); + } + + private function resolveField(InputField $field): mixed + { + return $field->getResolve()(new stdClass(), [], null, $this->createStub(ResolveInfo::class)); + } +} diff --git a/website/docs/attributes-reference.md b/website/docs/attributes-reference.md index 0f3cadcafa..201488e0a0 100644 --- a/website/docs/attributes-reference.md +++ b/website/docs/attributes-reference.md @@ -264,12 +264,12 @@ Attribute | Compulsory | Type | Definition expression | see below | string | A security expression, evaluated by Symfony ExpressionLanguage. Also accepted as the attribute's first positional argument: `#[Security("is_granted('X')")]` rule | see below | array \| object | A callable receiving a `SecurityRuleContext`, or the `SecurityRuleContextInterface` it implements, and returning `bool`. Must be passed by name (`rule:`) — the first positional argument is the expression, so a positional callable is either a `TypeError` or silently read as a legacy data array. Prefer first-class callable syntax (`self::canShow(...)`, PHP 8.5+); `[Rules::class, 'canShow']` works on 8.2+ and is the only form that can name a container-resolved method. A bare method-name string is **not** accepted here, unlike `#[Prefetch]` failWith | *no* | mixed | Value returned instead of denying. Cannot be combined with *message* or *statusCode* -message | *no* | string | Error message when access is denied. Defaults to `Access denied.` +message | *no* | string | Error message when access is denied. Always wins when given. When omitted, the message the rule states if it implements `SecurityRuleMessageInterface` (opt in), otherwise `Access denied.` statusCode | *no* | int | Status code when access is denied. Defaults to `403` Exactly one of `expression` and `rule` must be given; passing both throws, as does passing neither. Both forms are fully supported — see [fine grained security](fine-grained-security.mdx) for when to -use which. +use which, and for [a rule that states its own message](fine-grained-security.mdx#a-rule-that-states-its-own-message). ## #[SourceField] diff --git a/website/docs/fine-grained-security.mdx b/website/docs/fine-grained-security.mdx index 53d7d45f86..953e59f0b4 100644 --- a/website/docs/fine-grained-security.mdx +++ b/website/docs/fine-grained-security.mdx @@ -228,6 +228,9 @@ pieces of data the context carries appear on the interface as methods: properties stay on `SecurityRuleContext`, so a rule already type-hinting the concrete class needs no change. +That interface describes what a rule is *given*. A second, equally optional one describes what a +rule *says* when it refuses: see [a rule that states its own message](#a-rule-that-states-its-own-message). + ## Setting HTTP code and error message You can use the `statusCode` and `message` attributes to set the HTTP code and GraphQL error message. @@ -245,6 +248,78 @@ Note: since a single GraphQL call contain many errors, 2 errors might have confl The resulting status code is up to the GraphQL middleware you use. Most of the time, the status code with the higher error code will be returned. +### A rule that states its own message + +A rule usually knows why it refuses better than the field it guards does. Repeating that reason in a +`message:` on every field the rule guards is what makes the two drift apart: the check is edited in +one place and the sentence explaining it in ten. A rule can state the message itself by implementing +`SecurityRuleMessageInterface`, which is the [`PageSizeWithin` rule above](#parameterizing-a-rule) +with one method added: + +```php +use TheCodingMachine\GraphQLite\Security\SecurityRuleContextInterface; +use TheCodingMachine\GraphQLite\Security\SecurityRuleMessageInterface; + +final class PageSizeWithin implements SecurityRuleMessageInterface +{ + public function __construct(private readonly int $max) + { + } + + public function __invoke(SecurityRuleContextInterface $context): bool + { + return $context->argument('first') <= $this->max; + } + + public function getRefusalMessage(): string + { + return "Page size must be at most {$this->max}."; + } +} +``` + +```php +#[Query] +#[Security(rule: new PageSizeWithin(100), statusCode: 400)] +public function getPosts(int $first): array +{ + // ... +} +``` + +Denying that field reports `Page size must be at most 100.` with no `message:` written anywhere. +The limit is stated once, and so is the sentence quoting it. + +The message is chosen in this order: + +1. the `message:` written on the `#[Security]` attribute, whenever there is one; +2. the message the rule states, when the rule implements `SecurityRuleMessageInterface`; +3. `Access denied.` + +An explicit `message:` therefore always wins, so a single field can still say something the shared +rule has no way to know: + +```php +#[Query] +#[Security(rule: new PageSizeWithin(100), statusCode: 400, message: 'This report is capped at 100 rows')] +public function getReportRows(int $first): array +{ + // ... +} +``` + +Three things to know: + +* **It is opt in.** A field guarded by a rule that does not implement the interface is denied with + the attribute's message, or with `Access denied.` when the attribute wrote none, exactly as before. +* **A rule states a message, not a status.** `statusCode` is untouched by the interface, and + `failWith` is unaffected too: a field with `failWith` returns a value instead of denying, so no + message is read at all. +* **Only a rule that is an object can carry a message.** An array callable names a static method and + first-class callable syntax produces a `Closure`, and neither has an instance to ask. Write the + rule as an [invokable object](#parameterizing-a-rule), which is also what lets the message quote + the value the rule was constructed with. + ## Setting a default value If you do not want an error to be thrown when the security condition is not met, you can use the `failWith` attribute