-
Notifications
You must be signed in to change notification settings - Fork 11.4k
[12.x] Introducing authorizeAfterValidation
#55798
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 12.x
Are you sure you want to change the base?
[12.x] Introducing authorizeAfterValidation
#55798
Conversation
Fun fact: When this trait was introduced, on Laravel 5.0, validation was performed before authorization. Then PR #6407 changed that order on a patch release (by then, patch releases were actually minor, old times...) Since then, some people suggested reverting the order, or adding an option, but I couldn't find a PR trying to actually do something. On my own projects, I created this trait: <?php
namespace App\Http\Requests\Concerns;
trait AuthorizeAfterValidating
{
public function validateResolved(): void
{
$this->prepareForValidation();
$instance = $this->getValidatorInstance();
if ($instance->fails()) {
$this->failedValidation($instance);
}
$this->passedValidation();
if (! $this->passesAuthorization()) {
$this->failedAuthorization();
}
}
} Which I add to the Although, I must admit, it is very rare to happen to use it. But when I do use it, the reason is the same as outlined by OP: to avoid running Business Logic when there are simple validation rules that can be checked beforehand. |
This assumes that
While this is an easy solution, when one of the conditions doesn't apply, this doesn't actually solve that. However, having a smart solution that automagically determines the best order (validation first if authentication uses database driver and no database rule, otherwise authentication first). If we choose the easy solution, it would be a good idea to add the above points to the docs, just in case. |
I still prefer a declarative solution, e.g. explicitly opt-in for the behavior change, such as how it is proposed by this PR, or by manually adding a trait like I already do on some projects of mine. That makes it very clear the developer is aware of the behavior change. Also, not all cases of rules containing database rules would make them desirable to be executed last. It all depends on the authorization logic. |
@rodrigopedra My approach was that we want to make it intuitive, so that it doesn't depend on someone accidentally setting the wrong flag, because the problem ( Instead of a method, this could also be an attribute 🤔 #[AuthorizeAfterValidation]
class UpdatePostRequest extends FormRequest or #[Authorization(enabled: true, after: true)] // both `authorize()` and `authorizeAfterValidation()`
class UpdatePostRequest extends FormRequest or #[Authorization(enabled: true, order: 'after' /* FormRequestAuthorizationOrder::After->value */)] // both `authorize()` and `authorizeAfterValidation()`
class UpdatePostRequest extends FormRequest
This could be done with the automatic approach as well
That sounds like a less custom to me. Maybe, we could even provide stubs for either option. |
You are both right in your words. @shaedrich, there are cases where we already have several hits in the database, one more or one less won't make a difference, but there are cases where we don't have any interaction with the database - either because we don't interact with the authenticated user, or because we don't use validation rules that interact with the database, e.g. @rodrigopedra , regarding replicating what the trait does, although for us this is easy, for others it might not be, especially those new to Laravel, so I believe that a method like the one proposed in the PR would continue to follow the ease proposed by other methods, such as Finally, talking about using an attribute, such as |
This is a place where it could be implemented framework/src/Illuminate/Foundation/Providers/FormRequestServiceProvider.php Lines 29 to 31 in 62b41f6
however, I do understand why you went with the solution at hand 👍🏻 |
I would say that if the user is not even authorized to update the resource then there's no need to show validation errors. Consider this:
The user's time was just wasted fixing the input when they couldn't even update it. |
I agree with you! In fact, I would say that there is room for two, or even more, situations like this. However, without this PR we have no way of offering something like: "authorize only after validate". With this PR we will give the choice to the developer, about the way he wants to do it: authorize before OR after the validation. IMHO, we cannot consider just one way, one format or one behavior, because each application can be unique in its business rules. |
Following good practices in the Laravel ecosystem, it is common to concentrate validation logic in FormRequest classes, which is highly recommended.
As we know, FormRequest provides the
authorize
method that allows you to authorize the request before executing the validation rules. In this method, you can return boolean values, use hard-coded rules directly, or even perform checks based on the authenticated user coming from the database, as the spatie/laravel-permission package proposes.The issue is that
authorize
runs before validation. This means that even if the validation fails afterward, it’s likely that a database hit was executed unnecessarily.With this in mind, this PR proposes the addition of a new method:
This method allows us to invert the default order, making validation occur before authorization. This way, we ensure that authorization — and possible database hits — are performed only if validation is successful.
Example
If the above validation fails due to
title
not matching the rules, we will not get any database hits.