[5.9] Consider request validation before authorization

[owenconti]

Description:

A long, long time ago, a PR was merged that changed the order of operations for the ValidatesWhenResolvedTrait: #6407

I'm creating this issue to see if there would be an opportunity to either:

• reverse the change (potential large breaking change)
• add an option on the trait which individual request classes could override

The reason I believe the order should be: validate input -> authorize is because there are often times where the authorization logic needs to reference the request input. If the input has not yet been validated (as is current functionality), there are two problems:

• developer has to manually validate inside the authorize method
• user making the request is returned a 403 instead of the more-correct 422 response

Taking a slightly modified example from the Laravel documentation:

public function authorize()
{
    $comment = Comment::find($this->input('comment_id'));

    return $comment && $this->user()->can('update', $comment);
}

public function rules()
{
    return [
        'comment_id' => 'required|int|exists:comments,id'
    ];
}

If the user making this request specifies a nonexistent comment_id, I think it can be argued that the correct response is 422.

In addition, if the request input validation happened prior to the authorization, the authorize method could be cleaned up, since we're 100% sure the comment exists:

public function authorize()
{
    return $this->user()->can('update', Comment::find($this->input('comment_id')));
}

I'm more than willing to submit the PR for this, but I'd like to know if there's a chance it will be accepted (and which direction would be preferred).

[derekmd]

Proposals should be sent to https://github.com/laravel/ideas/issues since the framework Issues are meant for bug reports: https://laravel.com/docs/5.8/contributions#bug-reports

<?php

namespace App\Http\Requests;

trait AuthorizesAfterValidation
{
    public function authorize()
    {
        return true;
    }

    public function withValidator($validator)
    {
        $validator->after(function ($validator) {
            if (! $validator->failed() && ! $this->authorizeValidated()) {
                $this->failedAuthorization();
            }
        });
    }

    abstract public function authorizeValidated();
}

class UpdateCommentRequest extends FormRequest
{
    use AuthorizesAfterValidation;

    public function authorizeValidated()
    {
        return $this->user()->can('update', Comment::find($this->input('comment_id')));
    }

    public function rules()
    {
        return [
            'comment_id' => 'required|int|exists:comments,id', // runs a duplicate database query
        ];
    }
}

For your example, the most appropriate response is a HTTP 404 status code if the comment isn't found. The resource to authorize doesn't exist. Laravel handles this case by using route model binding, no FormRequest necessary:

class CommentsController
{
    /**
     * PUT /comments/{comment}
     */
    public function update(Comment $comment)
    {
        $this->authorize('update', $comment);


    }
}

Also have a look at the once() memoization helper ( https://github.com/spatie/once ) that can be used in FormRequest objects to fetch models that aren't cached using route model binding.

class UpdateCommentRequest extends FormRequest
{
    public function authorize()
    {
        return $this->user()->can('update', $this->comment());
    }

    public function rules()
    {
        return [
            // no exists rule needed
        ];
    }

    // $request->comment() in the controller won't hit the database again
    public function comment()
    {
        return once(function () {
            // ModelNotFoundException thrown
            return Comment::findOrFail($this->input('comment_id'));
        });
    }
}

Either way, further discussion should be sent to the "laravel/ideas" Issues.

[driesvints]

Heya, thanks for submitting this. This seems like a feature request or an improvement. It's best to post these at the laravel/ideas repository to get support for your idea. After that you may send a PR to the framework. Please only use this issue tracker to report bugs and issues with the framework.

[owenconti]

Thanks @derekmd - My example was a bit too simplistic. I have a hard time imagining there'd be much support to change this anyways, so I'll stick to a trait-based approach as you recommended.

Thanks.