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

[5.6] Fix FormRequest class authorization validation priority #24369

Merged
merged 1 commit into from May 30, 2018

Conversation

Projects
None yet
7 participants
@brunogaspar
Contributor

brunogaspar commented May 30, 2018

This pull request fixes an issue where the validation rules were being triggered before the authorization, which was leading to issues on some tests i have, where i was not passing some values (on purpose), and the rules were relying on them, however, for that particular test case, i was testing the authorization only and not determining if the provided data was valid.

So now the validation rules only gets triggered if the authorize() passes, which seems the proper way.

Note: I couldn't figure out a good way to add a unit test, but removing the rules from the stub class was failing prior to this change and now passes. If anyone has an idea to have a way to test it, let me know and i update the pull request.

Thanks!

fix: FormRequest class authorization validation priority
Signed-off-by: Bruno Gaspar <brunofgaspar1@gmail.com>
@sisve

This comment has been minimized.

Show comment
Hide comment
@sisve

sisve May 30, 2018

Contributor

This would be a change in behavior and should target the master branch (next release). This will affect people that uses request data within the authorization callback; they will no longer have the data validated when called, and would need to validate it themselves.

Contributor

sisve commented May 30, 2018

This would be a change in behavior and should target the master branch (next release). This will affect people that uses request data within the authorization callback; they will no longer have the data validated when called, and would need to validate it themselves.

@brunogaspar

This comment has been minimized.

Show comment
Hide comment
@brunogaspar

brunogaspar May 30, 2018

Contributor

That's a good point @sisve and thanks for taking the time to look at it.

I can be wrong though, but the data doesn't seem to be validated that early (at least that's what i'm seeing on my end), it just calls the rules() to get the, you know, the rules, however, it only validates these rules, later, when the $instance->passes() is called, which happens after the authorization validation which in turn causes the issue i mentioned on the OP.

Contributor

brunogaspar commented May 30, 2018

That's a good point @sisve and thanks for taking the time to look at it.

I can be wrong though, but the data doesn't seem to be validated that early (at least that's what i'm seeing on my end), it just calls the rules() to get the, you know, the rules, however, it only validates these rules, later, when the $instance->passes() is called, which happens after the authorization validation which in turn causes the issue i mentioned on the OP.

@sisve

This comment has been minimized.

Show comment
Hide comment
@sisve

sisve May 30, 2018

Contributor

You're right, I totally misread that code. The authorization is always done before validating the data, this change will just slightly delay calling getValidatorInstance until we know that the authorization succeeded.

Contributor

sisve commented May 30, 2018

You're right, I totally misread that code. The authorization is always done before validating the data, this change will just slightly delay calling getValidatorInstance until we know that the authorization succeeded.

@taylorotwell taylorotwell merged commit 1ec69a8 into laravel:5.6 May 30, 2018

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@brunogaspar brunogaspar deleted the brunogaspar:fix/form-request-authorization-priority branch May 31, 2018

@shadoWalker89

This comment has been minimized.

Show comment
Hide comment
@shadoWalker89

shadoWalker89 Jun 26, 2018

Contributor

@sisve @taylorotwell @brunogaspar Something like this shouldn't have been pushed into the current branch, this broke a part of my app where i'm storing the user that i'm updating within an attribute of the FormRequest class.
That was done on the rules() method because it is called before the authorize() method.
Any way i moved the code to the authorize() method now

Contributor

shadoWalker89 commented Jun 26, 2018

@sisve @taylorotwell @brunogaspar Something like this shouldn't have been pushed into the current branch, this broke a part of my app where i'm storing the user that i'm updating within an attribute of the FormRequest class.
That was done on the rules() method because it is called before the authorize() method.
Any way i moved the code to the authorize() method now

@brunogaspar

This comment has been minimized.

Show comment
Hide comment
@brunogaspar

brunogaspar Jun 26, 2018

Contributor

@shadoWalker89 Please paste your FormRequest class so i can see why it broke for you.

This PR actually fixed a logic bug as i mentioned on the OP.

Contributor

brunogaspar commented Jun 26, 2018

@shadoWalker89 Please paste your FormRequest class so i can see why it broke for you.

This PR actually fixed a logic bug as i mentioned on the OP.

@shadoWalker89

This comment has been minimized.

Show comment
Hide comment
@shadoWalker89

shadoWalker89 Jun 26, 2018

Contributor

I'm assigning the property userBeingUpdated value on the rules() method and then using it in the authorize() method.

When the order of the methods calls changed it broke my app.
The fix is pretty simple as i said before, it's just moving the userBeingUpdated to the authorize()

class UpdateUserRequest extends FormRequest
{
    protected $userBeingUpdated;

    public function authorize()
    {
        return $this->user()->can('users.edit', $this->userBeingUpdated);
    }

    public function rules()
    {
    	// This being set here and the used in the authorize()
        $this->userBeingUpdated = $this->route()->parameter('user');

        // My rules over here
    }
}
Contributor

shadoWalker89 commented Jun 26, 2018

I'm assigning the property userBeingUpdated value on the rules() method and then using it in the authorize() method.

When the order of the methods calls changed it broke my app.
The fix is pretty simple as i said before, it's just moving the userBeingUpdated to the authorize()

class UpdateUserRequest extends FormRequest
{
    protected $userBeingUpdated;

    public function authorize()
    {
        return $this->user()->can('users.edit', $this->userBeingUpdated);
    }

    public function rules()
    {
    	// This being set here and the used in the authorize()
        $this->userBeingUpdated = $this->route()->parameter('user');

        // My rules over here
    }
}
@BrandonSurowiec

This comment has been minimized.

Show comment
Hide comment
@BrandonSurowiec

BrandonSurowiec Jun 27, 2018

Contributor

@shadoWalker89 Thanks for sharing! You could put both of those lines in the same method to prevent breakage. Or even inline it if that is the only place you're using that variable. If you want to use it in multiple places, adding your own getter method could work too so it doesn't depend on the order of the method calls.

    public function authorize()
    {
        $this->userBeingUpdated = $this->route()->parameter('user');
        return $this->user()->can('users.edit', $this->userBeingUpdated);
    }
	
    // or

    public function authorize()
    {
        return $this->user()->can('users.edit', $this->route()->parameter('user'));
    }
Contributor

BrandonSurowiec commented Jun 27, 2018

@shadoWalker89 Thanks for sharing! You could put both of those lines in the same method to prevent breakage. Or even inline it if that is the only place you're using that variable. If you want to use it in multiple places, adding your own getter method could work too so it doesn't depend on the order of the method calls.

    public function authorize()
    {
        $this->userBeingUpdated = $this->route()->parameter('user');
        return $this->user()->can('users.edit', $this->userBeingUpdated);
    }
	
    // or

    public function authorize()
    {
        return $this->user()->can('users.edit', $this->route()->parameter('user'));
    }
@okdewit

This comment has been minimized.

Show comment
Hide comment
@okdewit

okdewit Jul 11, 2018

@taylorotwell I'm happy there are solutions documented here, but I do think it's still very disruptive to merge a breaking change like this into a minor version.

okdewit commented Jul 11, 2018

@taylorotwell I'm happy there are solutions documented here, but I do think it's still very disruptive to merge a breaking change like this into a minor version.

@sisve

This comment has been minimized.

Show comment
Hide comment
@sisve

sisve Jul 11, 2018

Contributor

I'm usually very paranoid about breaking changes, but not in this case. If I understand it correctly, the rules method was used in this context for something other that it was intended. Not only did it return validation rules, but read route parameters and set flags and thus having side effects.

Imagine having a workflow of 1) eating breakfast and 2) go to work. Imagine that you need to put on pants somewhere, and you decide to add that to the breakfast step. This works for you, and pants-and-breakfast becomes a regular maneuver ... until your employer starts a "have breakfast at work"-movement.

The proper solution depends on the architecture. Couldn't a simple getUserBeingUpdated() method suffice, a method that read from the route directly? That way there would be no weird side-effects from code not meant to have side effects.

Contributor

sisve commented Jul 11, 2018

I'm usually very paranoid about breaking changes, but not in this case. If I understand it correctly, the rules method was used in this context for something other that it was intended. Not only did it return validation rules, but read route parameters and set flags and thus having side effects.

Imagine having a workflow of 1) eating breakfast and 2) go to work. Imagine that you need to put on pants somewhere, and you decide to add that to the breakfast step. This works for you, and pants-and-breakfast becomes a regular maneuver ... until your employer starts a "have breakfast at work"-movement.

The proper solution depends on the architecture. Couldn't a simple getUserBeingUpdated() method suffice, a method that read from the route directly? That way there would be no weird side-effects from code not meant to have side effects.

@devcircus

This comment has been minimized.

Show comment
Hide comment
@devcircus

devcircus Jul 11, 2018

Contributor

There are also hook methods available in form requests to allow for any extra logic that needs to be run. "validationData()" allows changing any of the data that needs to be validated and "prepareForValidation()" ( provided by the validatesWhenResolved trait), allows running code before validation starts.

Contributor

devcircus commented Jul 11, 2018

There are also hook methods available in form requests to allow for any extra logic that needs to be run. "validationData()" allows changing any of the data that needs to be validated and "prepareForValidation()" ( provided by the validatesWhenResolved trait), allows running code before validation starts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment