Skip to content
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.7] Authorize Middleware Doesn't Accept String Parameters #25763

Merged

Conversation

damiankloip
Copy link
Contributor

@damiankloip damiankloip commented Sep 24, 2018

When using authorization gates, they can be used with arbitrary parameters that aren't necessarily models. E.g.

In blade templates:

@can('ability', 'some_value')

And, as middleware via authorize in controllers:

$this->authorize('ability', 'some_value')

However, when trying to do that at the route middleware level, the Authorize middleware always assumes the parameter will be a model. So it's not possible to do the following:

Route::middleware('can:ability,some_value')

This breaks currently as getGateArguments (and then getModel) assumes the parameter will be a model (or class name) and tries to resolve it from route parameters as so. So the above example would be turned into an array of arguments like [null] instead.

This can be solved quite simply by passing the $model value as the default value to the route() call.

It doesn't seem like this should cause any existing breakage, as it currently returns an array of null values in this case. Tests also don't fail with the change - only with the new test case added here (before the fix). This also makes the behaviour more consist across all usage.

@damiankloip damiankloip force-pushed the authorize-middleware-string-params branch from 868afcb to ee642c7 Compare September 24, 2018 18:53
@damiankloip damiankloip force-pushed the authorize-middleware-string-params branch from ee642c7 to 73168c9 Compare September 24, 2018 19:05
@damiankloip damiankloip changed the base branch from master to 5.7 September 24, 2018 19:05
@damiankloip damiankloip changed the title Authorize middleware string params Authorize Middleware Doesn't Accept String Parameters Sep 24, 2018
@damiankloip damiankloip changed the title Authorize Middleware Doesn't Accept String Parameters [5.7] Authorize Middleware Doesn't Accept String Parameters Sep 24, 2018
@@ -86,6 +86,24 @@ public function testSimpleAbilityAuthorized()
$this->assertEquals($response->content(), 'success');
}

public function testSimpleAbilityWithStringParameter()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add test like:

  1. The route has parameter document
  2. You pass the value document for some_value

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @TBlindaruk - do you mean like 2aa094d ? So we pass a parameter, get that back and is available in the ability callback.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, like this, but I`m not sure if it is good behavior

Copy link
Contributor Author

@damiankloip damiankloip Sep 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behaviour already exists though? If you had a parameter on the route, the existing code calling route() would already return this anyway. The new test method added doesn't really test the change with the default value but the current logic.


$response = $this->router->dispatch(Request::create('dashboard', 'GET'));

$this->assertEquals($response->content(), 'success');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is actually the other way round ;-)
$this->assertEquals('success', $response->content());

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to change this, every other assertion like this in the file is $this->assertEquals($response->content(), 'success') though.


$response = $this->router->dispatch(Request::create('dashboard/true', 'GET'));

$this->assertEquals($response->content(), 'success');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

@taylorotwell taylorotwell merged commit 942fc31 into laravel:5.7 Sep 27, 2018
@emiliopedrollo
Copy link
Contributor

This change broke our code.

We had something like this:

Route::get('/{forum}/{post}/{comment}')->middleware(['can:view,comment,forum,post']);
Route::get('/allforums/{post}/{comment}')->middleware(['can:view,comment,null,post']);

and on our Policy:

public function view(User $user, Comment $comment, Forum $forum = null, Post $post = null) {
    ...
}

And then when someone accessed the second rout the forum parameter would be null and now it becomes a string 'null' instead triggering a FatalThrowableError exception as the argument accepts either a Forum model or a null value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants