Lighthouse: Policies that require a model instance

[acro5piano]

Thank you for the great work!

I use v2.2-beta.1 and I found @can directive throws an error.

Describe the bug

When I use policy, lighthouse always throws error.

Expected behavior
can Delete a post if the user granted in policy

Schema

schema:

type Mutation {
    deletePost(id: ID!): User 
        @can(if: "update", model: "App\\Post") 
        @delete(model: "App\\Post")
}

query:

mutation deletePost {
  deletePost(id: 1) {
    id
  }
}

Output/Logs

throws this error:

Too few arguments to function App\\Policies\\PostPolicy::update(), 
1 passed in /var/www/html/vendor/laravel/framework/src/Illuminate/Auth/Access/Gate.php on line 481 
and exactly 2 expected

Environment

Lighthouse Version: v2.2-beta.1
Laravel Version: 5.5.41
PHP Version: 7.2.0

Additional context

I researched what is happening, and found the following code may cause the problem.

// src/Schema/Directives/Fields/CanDirective.php

// ...
                $model = $this->getModelClass() ?: get_class($args[0]);
                $can = collect($policies)->reduce(function ($allowed, $policy) use ($model) {
                    if (! app('auth')->user()->can($policy, $model)) {
                        return false;
                    }
// ...

https://github.com/nuwave/lighthouse/blob/master/src/Schema/Directives/Fields/CanDirective.php#L38-L43

Actually the above code pass model "class" string instead of model "instance" to the policy.

So if we change this behavior to use find and pass the instance to the policy just like other directive, it will be fixed.
How do you think?

            $model = $this->getModelClass()::find($id);

https://github.com/nuwave/lighthouse/blob/master/src/Schema/Directives/Fields/DeleteDirective.php#L49

[spawnia]

Right now, the @can directive only supports authorization that are not specific to a single model instance https://laravel.com/docs/5.6/authorization#methods-without-models

I am working on a PR that refines the current behaviour a bit further and am adding test cases as well as documentation for it. Will update here once i got it done.

Not quite sure on how we can solve authorization on specific models. My main problem is that we do have multiple ways of resolving model instances, so just querying by ID might not work for a lot of use cases. On the other hand, we can not really utilize the Model instance after it has been resolved, because in the case of mutations, it may already have changed.

Tough one.

[acro5piano]

@spawnia
Thank you for your quick response, I understand what is going on.
As you say there is a lot of query method like in, where, and so on.

In my use case, CRUD authorization is enough, just as users cannot update other's posts, so single instance policy works.
I would like to create my custom directive, hoping to publish to packagist as a plugin for Lighthouse.

Thanks!

[acro5piano]

For those who would like to apply a policy to single instance, try this directive.

<?php

namespace App\GraphQL\Directives;

use GraphQL\Error\Error;
use Nuwave\Lighthouse\Schema\Values\FieldValue;
use Nuwave\Lighthouse\Schema\Directives\BaseDirective;
use Nuwave\Lighthouse\Support\Contracts\FieldMiddleware;

class PolicyDirective extends BaseDirective implements FieldMiddleware
{
    /**
     * Name of the directive.
     *
     * @return string
     */
    public function name()
    {
        return 'policy';
    }

    /**
     * Resolve the field directive.
     *
     * @param FieldValue $value
     * @param \Closure    $next
     *
     * @return FieldValue
     */
    public function handleField(FieldValue $value, \Closure $next)
    {
        $policies = $this->directiveArgValue('if');
        $resolver = $value->getResolver();

        return $next($value->setResolver(
            function ($root, $args) use ($policies, $resolver) {
                $model = $this->getModelClass()::find($args['id']);

                $can = collect($policies)->reduce(function ($allowed, $policy) use ($model) {
                    if (! app('auth')->user()->can($policy, $model)) {
                        return false;
                    }

                    return $allowed;
                }, true);

                if (! $can) {
                    throw new Error('Not authorized to access resource');
                }

                return call_user_func_array($resolver, func_get_args());
            }
        ));
    }
}

type Mutation {
    deletePost(id: ID!): Post 
        @policy(if: "update", model: "App\\Post") 
        @delete(model: "App\\Post")
}

Note that this implementation does not work for multiple instances, or other situation. Hope this helps!

[spawnia]

Sweet, your custom directive looks great. I will leave this issue open for now as the basis for discussion around how we can apply Policies to specific model instances. Would love to get some more feedback/hear a bunch more use cases.

Maybe we can settle on a generalized solution that fits most use-cases. If so, i would be up for including it in Lighthouse.

[yaquawa]

@spawnia @acro5piano
interesting.

for more generic usage, how about add a keyName argument.
if such a keyName was given, try to get the model from the arguments.
if keyName was not given, fallback to Model::getKeyName().
The only problem for this is that it'll find the model twice with the same id…

type Mutation {
    deletePost(my_id: ID!): Post 
        @can(if: "update", model: "App\\Post", keyName: "my_id") 
        @delete(model: "App\\Post")
}

[carelloqwerty]

This is closed issue, @spawnia, but still i hope this message willn't be ignored. The problem with @can is: when you pass like in documentation @can(ability: "update", find: "id"), @can directive works before @update, but @can goes to DB, finds model by key and check it, loads relations if they need to be checked by policy and that's all. Then Update directive goes to DB, finds model by key, makes editional requests to load relations if they needed. So as you see the problem is that @can directive doesn't check the same instance as @update is going to modifer, not to mention loads of sql requests

[spawnia]

@carelloqwerty i see how that is inefficient. Feel free to open a PR to improve that aspect of Lighthouse.

[SagarNaliyapara]

@spawnia how we can use it for delete? I'm getting error

Too few arguments to function App\\Policies\\Post::delete(), 1 passed in vendor/laravel/framework/src/Illuminate/Auth/Access/Gate.php on line 710 and exactly 2 expected

I've used it as follow,

deletePost(id: ID @eq): Post @can(ability: "delete")

Is I'm doing something wrong here? @eriktisme @spawnia

[lorado]

You have to define where @can should take the ID of the model. This should work

deletePost(id: ID! @eq): Post @can(ability: "delete", find: "id")

id in find references attribute name of your field

[SagarNaliyapara]

Thanks @lorado for quick response it works!

[sharif-paksa]

@lorado this should be in the docs

[spawnia]

@lorado this should be in the docs

https://lighthouse-php.com/master/api-reference/directives.html#can