Merge pull request #403 from mcg-web/access-pre-execute-object-only-i…

…f-needed Execute resolverCallback for access only if needed
overblog · Oct 15, 2018 · cc02858 · cc02858
2 parents 5a586fb + b2ab02d
commit cc02858
Show file tree

Hide file tree

Showing 6 changed files with 76 additions and 23 deletions.
diff --git 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
@@ -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
@@ -22,25 +22,14 @@ 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)
     {
-        // operation is mutation and is mutation field
-        if ($isMutation) {
-            if (!$this->hasAccess($accessChecker, null, $resolveArgs)) {
-                throw new UserError('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;
         }
@@ -57,32 +46,47 @@ 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 */
         $resolveInfo = $resolveArgs[3];
 
         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);

diff --git a/src/Resources/skeleton/OutputFieldConfig.php.skeleton b/src/Resources/skeleton/OutputFieldConfig.php.skeleton
@@ -8,4 +8,5 @@
 <spaces># public and access are custom options managed only by the bundle
 <spaces>'public' => <public>,
 <spaces>'access' => <access>,
+<spaces>'useStrictAccess' => <useStrictAccess>,
 ],
diff --git 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
@@ -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',
+                        ],
+                    ],
                 ],
             ],
         ];