kkontroll's avatar

About using abort in Policy.

I am currently faced with this pattern of writing policies:

public function update(User $user): bool
{
    if ($user->isDeleted()) {
        abort(403, "Your user has been deleted");
    }

    if (! $user->isActive()) {
        abort(403, "Your user is not activated yet");
    }

    return true;
}

Consequently, I am a bit perplexed and I was hoping for some educated input and comments about this way of writing polices. Good and bad. I would appreciate any feedback.

0 likes
9 replies
tykus's avatar

You can use the deny method on Illuminate\Auth\Access\Response to fail and set a message:

public function update(User $user): bool
{
    if ($user->isDeleted()) {
        return Response::deny("Your user has been deleted");
    }

    if (! $user->isActive()) {
        return Response::deny("Your user is not activated yet");
    }

    return true;
}

https://laravel.com/docs/8.x/authorization#policy-responses

Sinnbeck's avatar

@tykus Your first answer was also correct

$this->deny('Message');

//calls
protected function deny($message = null, $code = null)
    {
        return Response::deny($message, $code);
    }
kkontroll's avatar

Yes, I am aware of this possibility, and that is an alternative. But I was looking for more good and bad about the implementation i referred to.

I appreciate taking the time to give feedback, thank you :)

tykus's avatar

I'm aware @sinnbeck - just edited to be consistent with the docs.

@kkontroll I would expect that you will specifically get an AuthorizationException whenever you use deny method, whereas abort helper will give a more general HttpResponseException; does this matter?

kkontroll's avatar

Well for my part I am unsure. Should it matter? And that is part of what I am trying to figure out. I am not facing one problem or a bug, I think my question is more in terms of best practice and what is proper, and in part how things should be done.

What could the consequences be when throwing HttpException instead of AuthorizationException? In a controller, if we do $this->authorize(...) we would have to catch a different exception. We could just change those and do that, but does it matter? For now all I know is that it appears to be possible, but it feels weird and off.

Would it, assumedly, be more secure to use whitelisting instead of blacklisting?

The method does not really return anything, ever, in cases of "denial". Does it matter? The whole premise feels wrong, but it appears to work in practice.

It is in an API context, if that makes a difference, and the reasoning seems to be that this will automatically return 403 with a predefined message whenever one tries to authorize for this action.

I guess I am a bit biased, but I am honestly just trying to figure this out to iron out an internal issue and to learn something new.

Edit: What would your initial reaction be if you came across a system of policies written in this matter when working on an API?

tykus's avatar

There is no catch in the Controller, everything is handled at framework level because both approaches generate the appropriate response.

My personal opinion would be to stick with the framework conventions unless there is a very specific reason to avoid it. Laravel will generate an appropriate response status code and your message if you use the deny method (just as it will using your abort approach`)

kkontroll's avatar

There is no catch in the Controller, everything is handled at framework level because both approaches generate the appropriate response.

Assuming that we would want to have a different error message depending on where the policy is used, would we not have to try-catch the authorize call then?

From my prior experience with Laravel, if I want to handle the return from a policy, then I would have to catch an AuthorizationException in the controller, adjust the error message, and return the "appropriately" formed response from the controller. That would fail if we are using abort, because that throws a different class of exception. I would imagine that is unfortunate - am I being a stickler?

My personal opinion would be to stick with the framework conventions [...]

I think that is my stance on it as well, but what are those conventions? Does my first example follow those conventions in your opinion? Surely, at least it should be return $this->deny('Your user is not activated yet'), or maybe I misunderstood you here. I apologize if that is the case.

Do you have an opinion on whitelisting/blacklisting in policies? I would be interested in hearing about it if you do.

tykus's avatar

I believe, you would not use the authorize method in that case because it takes over control of flow right through to returning the response; instead you could use something like this in the Controller action:

abort_if (Gate::denies('update', $model), 403, 'Your custom message');

The framework convention (to my understanding) is to deny the action in the Policy method; in my opinion abort is a blunt instrument that, although quickly breaking flow, bypasses the exceptional Authorization behaviour.

As mentioned by @sinnbeck above, you can use either $this->deny() or Response::deny() according to your preference; I changed my original reply only to be consistent with the docs that I linked, but would use $this->deny() myself.

I am not 100% sure what you mean by whitelisting/blacklisting in Policies, maybe you could clarify?

kkontroll's avatar

I would agree that using return false and return true could be changed to $this->deny() or $this->allow() - the doc uses these as well. Either of these would be fine by me, though I am more accustomed to the first-mentioned alternative.

However, in so far as I have managed to understand up until now, using abort inline in the policy for denying the user access to the resources, seems unconventional to say the least. Especially since it throws a different class of exception than a developers experience in the framework would make one assume. My understanding is that you are somewhat of the same opinion, though you might have different reasons for it.

I was not aware of the abort_if example that you gave, I will have to look into that. Thank you for the suggestion!

By whitelisting/blacklisting, I mean that in my original post everyone is assumed to authorized, except those explicitly being denied. I believe that is called "blacklisting", and from what I have found it is frowned upon security-wise and also conventionally in Laravel - as I believe you touched upon. Is it not more "healthy" to assume everyone is denied, other than those you identify to explicitly have access?

I am not sure what I am looking for here. I think I just needed fresh input for an internal discussion which seems to be a bit locked. Hence, I am investigating whether I am unaccepting to new things, or if my concerns are genuine and worthwhile.

Please or to participate in this conversation.