lorisleiva / laravel-actions

⚡️ Laravel components that take care of one specific task
https://laravelactions.com
MIT License
2.52k stars 124 forks source link

Guest user policy in authorize function of action #137

Closed Celant closed 3 years ago

Celant commented 3 years ago

Hey, love the package 🙂

Is there any guidance on how to correctly check a policy that has a nullable user argument? My policy check has a nullable user argument, but I can't call it within the authorize function of my Action.

Usually you'd call a policy check something like:

public function authorize(ActionRequest $request, Event $event): bool
{
    return $request->user()->can('view', $event);
}

but if the request is not authenticated, the user returns nil resulting in a Call to a member function can() on null exception.

Another way suggested by laravel would be to use the controller helper:

public function authorize(ActionRequest $request, Event $event): bool
{
    $this->authorize('view', $event);
}

but this also doesn't work, as the Action is not a Controller.

Are there any suggestions on how to call a policy function that takes a nullable user from within an Action?

lorisleiva commented 3 years ago

Hi there 👋

How about using the Gate facade directly? This is what the controller delegates to when using the authorize method.

use Illuminate\Support\Facades\Gate;

public function authorize(ActionRequest $request, Event $event): bool
{
    Gate::authorize('view', $event);
}

Note that using Gate::authorize will automatically throw a Illuminate\Auth\Access\AuthorizationException if the ability is unauthorized instead of going through the validator logic. Which, to be honest, is fine because that's the default behaviour of the validator anyway but if you wanted a way that's more "integrated" with the action's logic, a better approach would be to use Gate::check that will return a boolean instead which is what the action's authorize method is expected to receive.

use Illuminate\Support\Facades\Gate;

public function authorize(ActionRequest $request, Event $event): bool
{
    return Gate::check('view', $event);
}

I hope this helps. 🍀

lorisleiva commented 3 years ago

This should probably be in the Laravel Actions documentation btw so thanks for raising this! 😊

EDIT: Documentation updated. ✅

Celant commented 3 years ago

Thanks for the super quick reply! I think that solves my issue, but there might be a secondary one.

In every case I've tested in my application, the $event parameter passed to the authorize function is always null. If I check the parameter in the handle function, it is correctly present. Just not in the authorize function!

Full code sample is below, and I am truly puzzled 🙂

class ShowEvent
{
    use AsAction;

    public function authorize(ActionRequest $request, Event $event): bool
    {
        dd($event->id);
        return Gate::check('view', $event);
    }

    public function handle(ActionRequest $request, Event $event): Response
    {
        return Inertia::render('Events/Show', [
            'event' => $event,
        ]);
    }

    public function asController(ActionRequest $request, Event $event): Response
    {
        return $this->handle($request, $event);
    }
}

In this case, dd just shows null.

Celant commented 3 years ago

@lorisleiva I suspect the issue here is that inspectAuthorization() in the ValidateActions concern uses $this->resolveAndCallMethod('authorize'), whereas the ControllerDecorator uses $this->resolveFromRouteAndCall().

The former uses the service container to resolve dependencies and then call the method, whereas the latter does the same but also includes $this->route->parametersWithoutNulls(), meaning handle gets the Event object from the route correctly but authorize doesn't.

I don't know enough about Laravel and this library to know whether it's as simple as just including the parametersWithoutNulls in the arguments passed to resolveAndCallMethod, or if some wider change is needed to support this.

If it is the former, I am happy to try and PR that. Otherwise, I'll need your help on this one. Additionally, It looks like the tests cover controllers with implicit route bindings, as well as authorization and validation, but not both. Maybe this is an edge case that requires a test case? 🙂

lorisleiva commented 3 years ago

I've moved this conversation to #140.