From 6be99cebd8c08ce988b6c595eeab74d9dd697b3d Mon Sep 17 00:00:00 2001 From: Jeremiah VALERIE Date: Fri, 12 Oct 2018 17:30:03 +0200 Subject: [PATCH 1/2] Execute resolverCallback for access only if needed --- .../ConfigProcessor/AclConfigProcessor.php | 3 +- src/Generator/TypeGenerator.php | 13 ++++++++ src/Resolver/AccessResolver.php | 11 +++++-- .../skeleton/OutputFieldConfig.php.skeleton | 1 + .../config/access/mapping/access.types.yml | 7 +++- tests/Functional/Security/AccessTest.php | 33 ++++++++++++++++++- 6 files changed, 61 insertions(+), 7 deletions(-) diff --git a/src/Definition/ConfigProcessor/AclConfigProcessor.php b/src/Definition/ConfigProcessor/AclConfigProcessor.php index 72f6828d4..dfb3ed457 100644 --- a/src/Definition/ConfigProcessor/AclConfigProcessor.php +++ b/src/Definition/ConfigProcessor/AclConfigProcessor.php @@ -34,9 +34,8 @@ public static function acl(array $fields, AccessResolver $accessResolver, callab } elseif (\is_callable($accessChecker)) { $field['resolve'] = function ($value, $args, $context, ResolveInfo $info) use ($field, $accessChecker, $accessResolver, $defaultResolver) { $resolverCallback = self::findFieldResolver($field, $info, $defaultResolver); - $isMutation = 'mutation' === $info->operation->operation && $info->parentType === $info->schema->getMutationType(); - return $accessResolver->resolve($accessChecker, $resolverCallback, [$value, $args, $context, $info], $isMutation); + return $accessResolver->resolve($accessChecker, $resolverCallback, [$value, $args, $context, $info], $field['useStrictAccess']); }; } } diff --git a/src/Generator/TypeGenerator.php b/src/Generator/TypeGenerator.php index 5979cf522..bdc5ba1b4 100644 --- a/src/Generator/TypeGenerator.php +++ b/src/Generator/TypeGenerator.php @@ -7,6 +7,7 @@ use Overblog\GraphQLBundle\Definition\Argument; use Overblog\GraphQLBundle\Definition\Type\CustomScalarType; use Overblog\GraphQLGenerator\Generator\TypeGenerator as BaseTypeGenerator; +use Symfony\Component\ExpressionLanguage\Expression; use Symfony\Component\Filesystem\Filesystem; class TypeGenerator extends BaseTypeGenerator @@ -192,6 +193,18 @@ protected function generateParentClassName(array $config) } } + protected function generateUseStrictAccess(array $value) + { + $useStrictAccess = 'true'; + if (null !== $this->getExpressionLanguage() && $this->arrayKeyExistsAndIsNotNull($value, 'access') && $value['access'] instanceof Expression) { + $parsedExpression = $this->getExpressionLanguage()->parse($value['access'], ['value', 'args', 'context', 'info', 'object']); + $serializedNode = \str_replace("\n", '//', (string) $parsedExpression->getNodes()); + $useStrictAccess = false === \strpos($serializedNode, 'NameNode(name: \'object\')') ? 'true' : 'false'; + } + + return $useStrictAccess; + } + public function compile($mode) { $cacheDir = $this->getCacheDir(); diff --git a/src/Resolver/AccessResolver.php b/src/Resolver/AccessResolver.php index 7eb360db8..217e5e128 100644 --- a/src/Resolver/AccessResolver.php +++ b/src/Resolver/AccessResolver.php @@ -22,12 +22,17 @@ public function __construct(PromiseAdapter $promiseAdapter) $this->promiseAdapter = $promiseAdapter; } - public function resolve(callable $accessChecker, callable $resolveCallback, array $resolveArgs = [], $isMutation = false) + public function resolve(callable $accessChecker, callable $resolveCallback, array $resolveArgs = [], $useStrictAccess = false) { + /** @var ResolveInfo $info */ + $info = $resolveArgs[3]; // operation is mutation and is mutation field - if ($isMutation) { + $isMutation = 'mutation' === $info->operation->operation && $info->parentType === $info->schema->getMutationType(); + + if ($isMutation || $useStrictAccess) { if (!$this->hasAccess($accessChecker, null, $resolveArgs)) { - throw new UserError('Access denied to this field.'); + $exceptionClassName = $isMutation ? UserError::class : UserWarning::class; + throw new $exceptionClassName('Access denied to this field.'); } $result = \call_user_func_array($resolveCallback, $resolveArgs); diff --git a/src/Resources/skeleton/OutputFieldConfig.php.skeleton b/src/Resources/skeleton/OutputFieldConfig.php.skeleton index 1dcee2bb8..fb2cd0e00 100644 --- a/src/Resources/skeleton/OutputFieldConfig.php.skeleton +++ b/src/Resources/skeleton/OutputFieldConfig.php.skeleton @@ -8,4 +8,5 @@ # public and access are custom options managed only by the bundle 'public' => , 'access' => , +'useStrictAccess' => , ], diff --git a/tests/Functional/App/config/access/mapping/access.types.yml b/tests/Functional/App/config/access/mapping/access.types.yml index 5be91d1c2..30e0589c4 100644 --- a/tests/Functional/App/config/access/mapping/access.types.yml +++ b/tests/Functional/App/config/access/mapping/access.types.yml @@ -41,11 +41,16 @@ User: type: Boolean resolve: true friends: - # replace by strict equality after fix compilation expression language bug in sf 4.0 + # TODO(mcg-web): replace by strict equality after fix compilation expression language bug in sf 4.0 access: "@=object == 1" type: friendConnection argsBuilder: ConnectionArgs resolve: '@=resolver("friends", [value, args])' + friendsAsArray: + # TODO(mcg-web): replace by strict equality after fix compilation expression language bug in sf 4.0 + access: "@=object != 2" + type: '[Int]' + resolve: [1, 2, 3] forbidden: access: false type: String! diff --git a/tests/Functional/Security/AccessTest.php b/tests/Functional/Security/AccessTest.php index fd5097f2a..d5a2fca85 100644 --- a/tests/Functional/Security/AccessTest.php +++ b/tests/Functional/Security/AccessTest.php @@ -210,6 +210,19 @@ public function testUserAccessToUserFriends() $this->assertResponse($this->userFriendsQuery, $expected, static::USER_ADMIN, 'access'); } + public function testUserAccessToUserFriendsAsArray() + { + $expected = [ + 'data' => [ + 'user' => [ + 'friendsAsArray' => [1, null, 3], + ], + ], + ]; + + $this->assertResponse('query { user { friendsAsArray } }', $expected, static::USER_ADMIN, 'access'); + } + public function testMutationAllowedUser() { $result = 123; @@ -287,7 +300,25 @@ private function expectedFailedUserRoles() return [ 'data' => [ 'user' => [ - 'roles' => [0 => null], + 'roles' => null, + ], + ], + 'extensions' => [ + 'warnings' => [ + [ + 'message' => 'Access denied to this field.', + 'category' => 'user', + 'locations' => [ + [ + 'line' => 1, + 'column' => 16, + ], + ], + 'path' => [ + 'user', + 'roles', + ], + ], ], ], ]; From b2ab02d58bb18f7aa1035da80aa6a8c99087a78e Mon Sep 17 00:00:00 2001 From: Jeremiah VALERIE Date: Sun, 14 Oct 2018 17:52:44 +0200 Subject: [PATCH 2/2] Refactor access resolver --- src/Resolver/AccessResolver.php | 45 ++++++++++++++++----------------- 1 file changed, 22 insertions(+), 23 deletions(-) diff --git a/src/Resolver/AccessResolver.php b/src/Resolver/AccessResolver.php index 217e5e128..5be94d965 100644 --- a/src/Resolver/AccessResolver.php +++ b/src/Resolver/AccessResolver.php @@ -24,28 +24,12 @@ public function __construct(PromiseAdapter $promiseAdapter) public function resolve(callable $accessChecker, callable $resolveCallback, array $resolveArgs = [], $useStrictAccess = false) { - /** @var ResolveInfo $info */ - $info = $resolveArgs[3]; - // operation is mutation and is mutation field - $isMutation = 'mutation' === $info->operation->operation && $info->parentType === $info->schema->getMutationType(); - - if ($isMutation || $useStrictAccess) { - if (!$this->hasAccess($accessChecker, null, $resolveArgs)) { - $exceptionClassName = $isMutation ? UserError::class : UserWarning::class; - throw new $exceptionClassName('Access denied to this field.'); - } - - $result = \call_user_func_array($resolveCallback, $resolveArgs); - } else { - $result = $this->filterResultUsingAccess($accessChecker, $resolveCallback, $resolveArgs); + if ($useStrictAccess || self::isMutationRootField($resolveArgs[3])) { + return $this->checkAccessForStrictMode($accessChecker, $resolveCallback, $resolveArgs); } - return $result; - } - - private function filterResultUsingAccess(callable $accessChecker, callable $resolveCallback, array $resolveArgs = []) - { $result = \call_user_func_array($resolveCallback, $resolveArgs); + if ($result instanceof Promise) { $result = $result->adoptedPromise; } @@ -62,6 +46,21 @@ function ($result) use ($accessChecker, $resolveArgs) { return $this->processFilter($result, $accessChecker, $resolveArgs); } + private static function isMutationRootField(ResolveInfo $info) + { + return 'mutation' === $info->operation->operation && $info->parentType === $info->schema->getMutationType(); + } + + private function checkAccessForStrictMode(callable $accessChecker, callable $resolveCallback, array $resolveArgs = []) + { + if (!$this->hasAccess($accessChecker, $resolveArgs)) { + $exceptionClassName = self::isMutationRootField($resolveArgs[3]) ? UserError::class : UserWarning::class; + throw new $exceptionClassName('Access denied to this field.'); + } + + return \call_user_func_array($resolveCallback, $resolveArgs); + } + private function processFilter($result, $accessChecker, $resolveArgs) { /** @var ResolveInfo $resolveInfo */ @@ -69,25 +68,25 @@ private function processFilter($result, $accessChecker, $resolveArgs) if (self::isIterable($result) && $resolveInfo->returnType instanceof ListOfType) { foreach ($result as $i => $object) { - $result[$i] = $this->hasAccess($accessChecker, $object, $resolveArgs) ? $object : null; + $result[$i] = $this->hasAccess($accessChecker, $resolveArgs, $object) ? $object : null; } } elseif ($result instanceof Connection) { $result->edges = \array_map( function (Edge $edge) use ($accessChecker, $resolveArgs) { - $edge->node = $this->hasAccess($accessChecker, $edge->node, $resolveArgs) ? $edge->node : null; + $edge->node = $this->hasAccess($accessChecker, $resolveArgs, $edge->node) ? $edge->node : null; return $edge; }, $result->edges ); - } elseif (!$this->hasAccess($accessChecker, $result, $resolveArgs)) { + } elseif (!$this->hasAccess($accessChecker, $resolveArgs, $result)) { throw new UserWarning('Access denied to this field.'); } return $result; } - private function hasAccess(callable $accessChecker, $object, array $resolveArgs = []) + private function hasAccess(callable $accessChecker, array $resolveArgs = [], $object = null) { $resolveArgs[] = $object; $access = (bool) \call_user_func_array($accessChecker, $resolveArgs);