Don't conceal errors in resolvers for canFind, canModel and canQuery directives (#2686)

Author: MarcEspiard

src/Auth/BaseCanDirective.php

@@ -3,6 +3,7 @@
 namespace Nuwave\Lighthouse\Auth;
 
 use Illuminate\Contracts\Auth\Access\Gate;
+use Nuwave\Lighthouse\Auth\Contracts\ResolvesDuringAuthorization;
 use Nuwave\Lighthouse\Exceptions\AuthorizationException;
 use Nuwave\Lighthouse\Execution\ResolveInfo;
 use Nuwave\Lighthouse\Schema\Directives\BaseDirective;
@@ -86,9 +87,18 @@ public function handleField(FieldValue $fieldValue): void
             $gate = $this->gate->forUser($context->user());
             $checkArguments = $this->buildCheckArguments($args);
             $authorizeModel = fn (mixed $model) => $this->authorizeModel($gate, $ability, $model, $checkArguments);
+            $hasResolved = false;
+            $trackedResolver = function () use (&$hasResolved, $resolver) {
+                $hasResolved = true;
+
+                return $resolver(...func_get_args());
+            };
 
             try {
-                return $this->authorizeRequest($root, $args, $context, $resolveInfo, $resolver, $authorizeModel);
+                $resolved = $this->authorizeRequest($root, $args, $context, $resolveInfo, $trackedResolver, $authorizeModel);
+                if ($hasResolved) {
+                    return $resolved;
+                }
             } catch (\Throwable $throwable) {
                 $action = $this->directiveArgValue('action');
                 if ($action === 'EXCEPTION_NOT_AUTHORIZED') {
@@ -101,11 +111,18 @@ public function handleField(FieldValue $fieldValue): void
 
                 throw $throwable;
             }
+
+            // Try to resolve the field outside the authorization try-catch block to avoid catching resolver exceptions.
+            return $resolver($root, $args, $context, $resolveInfo);
         });
     }
 
     /**
-     * Authorizes request and resolves the field.
+     * Authorizes request and optionally resolves the field.
+     *
+     * This method is called inside the authorization try-catch block.
+     * Resolving the field here is optional, it will be done outside the try-catch block if not resolved here.
+     * Resolving inside this method means any exceptions thrown by the resolver will be caught and handled according to the `action` argument.
      *
      * @phpstan-import-type Resolver from \Nuwave\Lighthouse\Schema\Values\FieldValue as Resolver
      *

src/Auth/CanFindDirective.php

@@ -65,7 +65,7 @@ protected function authorizeRequest(mixed $root, array $args, GraphQLContext $co
             $authorize($model);
         }
 
-        return $resolver($root, $args, $context, $resolveInfo);
+        return null;
     }
 
     /**

src/Auth/CanModelDirective.php

@@ -33,6 +33,6 @@ protected function authorizeRequest(mixed $root, array $args, GraphQLContext $co
     {
         $authorize($this->getModelClass());
 
-        return $resolver($root, $args, $context, $resolveInfo);
+        return null;
     }
 }

src/Auth/CanQueryDirective.php

@@ -45,6 +45,6 @@ protected function authorizeRequest(mixed $root, array $args, GraphQLContext $co
             $authorize($model);
         }
 
-        return $resolver($root, $args, $context, $resolveInfo);
+        return null;
     }
 }

src/Auth/CanRootDirective.php

@@ -27,6 +27,6 @@ protected function authorizeRequest(mixed $root, array $args, GraphQLContext $co
     {
         $authorize($root);
 
-        return $resolver($root, $args, $context, $resolveInfo);
+        return null;
     }
 }

tests/Integration/Auth/CanFindDirectiveDBTest.php

@@ -10,6 +10,7 @@
 use Tests\Utils\Models\Post;
 use Tests\Utils\Models\Task;
 use Tests\Utils\Models\User;
+use Tests\Utils\Mutations\ThrowWhenInvoked;
 use Tests\Utils\Policies\UserPolicy;
 
 final class CanFindDirectiveDBTest extends DBTestCase
@@ -210,6 +211,37 @@ public function testFailsToFindSpecificModelConcealException(): void
         ]);
     }
 
+    public function testDoesntConcealResolverException(): void
+    {
+        $admin = new User();
+        $admin->name = UserPolicy::ADMIN;
+        $this->be($admin);
+
+        $user = factory(User::class)->create();
+        assert($user instanceof User);
+
+        $this->schema = /** @lang GraphQL */ '
+        type Mutation {
+            throwWhenInvoked(id: ID!): User
+                @canFind(ability: "view", find: "id", action: EXCEPTION_NOT_AUTHORIZED)
+        }
+
+        type User {
+            name: String!
+        }
+        ' . self::PLACEHOLDER_QUERY;
+
+        $this->graphQL(/** @lang GraphQL */ '
+        mutation ($id: ID!) {
+            throwWhenInvoked(id: $id) {
+                name
+            }
+        }
+        ', [
+            'id' => $user->getKey(),
+        ])->assertGraphQLErrorMessage(ThrowWhenInvoked::ERROR_MESSAGE);
+    }
+
     public function testFailsToFindSpecificModelWithFindOrFailFalse(): void
     {
         $user = new User();

tests/Integration/Auth/CanModelDirectiveDBTest.php

@@ -0,0 +1,37 @@
+<?php declare(strict_types=1);
+
+namespace Integration\Auth;
+
+use Tests\DBTestCase;
+use Tests\Utils\Models\User;
+use Tests\Utils\Mutations\ThrowWhenInvoked;
+use Tests\Utils\Policies\UserPolicy;
+
+final class CanModelDirectiveDBTest extends DBTestCase
+{
+    public function testDoesntConcealResolverException(): void
+    {
+        $admin = new User();
+        $admin->name = UserPolicy::ADMIN;
+        $this->be($admin);
+
+        $this->schema = /** @lang GraphQL */ '
+        type Mutation {
+            throwWhenInvoked: Task
+                @canModel(ability: "adminOnly", action: EXCEPTION_NOT_AUTHORIZED)
+        }
+
+        type Task {
+            name: String!
+        }
+        ' . self::PLACEHOLDER_QUERY;
+
+        $this->graphQL(/** @lang GraphQL */ '
+        mutation {
+            throwWhenInvoked {
+                name
+            }
+        }
+        ')->assertGraphQLErrorMessage(ThrowWhenInvoked::ERROR_MESSAGE);
+    }
+}

tests/Integration/Auth/CanQueryDirectiveDBTest.php

@@ -6,6 +6,7 @@
 use Tests\Utils\Models\Post;
 use Tests\Utils\Models\Task;
 use Tests\Utils\Models\User;
+use Tests\Utils\Mutations\ThrowWhenInvoked;
 use Tests\Utils\Policies\UserPolicy;
 
 final class CanQueryDirectiveDBTest extends DBTestCase
@@ -83,6 +84,37 @@ public function testFailsToFindSpecificModelWithQuery(): void
         ]);
     }
 
+    public function testDoesntConcealResolverException(): void
+    {
+        $admin = new User();
+        $admin->name = UserPolicy::ADMIN;
+        $this->be($admin);
+
+        $user = factory(User::class)->create();
+        assert($user instanceof User);
+
+        $this->schema = /** @lang GraphQL */ '
+        type Mutation {
+            throwWhenInvoked(id: ID!): User
+                @canQuery(ability: "view", action: EXCEPTION_NOT_AUTHORIZED)
+        }
+
+        type User {
+            name: String!
+        }
+        ' . self::PLACEHOLDER_QUERY;
+
+        $this->graphQL(/** @lang GraphQL */ '
+        mutation ($id: ID!) {
+            throwWhenInvoked(id: $id) {
+                name
+            }
+        }
+        ', [
+            'id' => $user->getKey(),
+        ])->assertGraphQLErrorMessage(ThrowWhenInvoked::ERROR_MESSAGE);
+    }
+
     public function testHandleMultipleModelsWithQuery(): void
     {
         $admin = new User();

tests/Integration/Auth/CanRootDirectiveDBTest.php

@@ -0,0 +1,36 @@
+<?php declare(strict_types=1);
+
+namespace Integration\Auth;
+
+use Tests\DBTestCase;
+use Tests\Utils\Models\User;
+use Tests\Utils\Mutations\ThrowWhenInvoked;
+use Tests\Utils\Policies\UserPolicy;
+
+final class CanRootDirectiveDBTest extends DBTestCase
+{
+    public function testDoesntConcealResolverException(): void
+    {
+        $admin = new User();
+        $admin->name = UserPolicy::ADMIN;
+        $this->be($admin);
+
+        $this->schema = /** @lang GraphQL */ '
+        type Mutation {
+            throwWhenInvoked: Task
+        }
+
+        type Task {
+            name: String! @canRoot(ability: "adminOnly", action: EXCEPTION_NOT_AUTHORIZED)
+        }
+        ' . self::PLACEHOLDER_QUERY;
+
+        $this->graphQL(/** @lang GraphQL */ '
+        mutation {
+            throwWhenInvoked {
+                name
+            }
+        }
+        ')->assertGraphQLErrorMessage(ThrowWhenInvoked::ERROR_MESSAGE);
+    }
+}

tests/Utils/Mutations/ThrowWhenInvoked.php

@@ -0,0 +1,20 @@
+<?php declare(strict_types=1);
+
+namespace Tests\Utils\Mutations;
+
+use Nuwave\Lighthouse\Exceptions\AuthorizationException;
+
+final class ThrowWhenInvoked
+{
+    public const ERROR_MESSAGE = 'Custom error message from ThrowWhenInvoked mutation.';
+
+    /**
+     * @param  array<string, mixed>  $args
+     *
+     * @return array<string, mixed>
+     */
+    public function __invoke(mixed $root, array $args): array
+    {
+        throw new AuthorizationException(self::ERROR_MESSAGE);
+    }
+}