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

[8.x] Allow can method to be chained onto route for quick authorization #39464

Merged
merged 1 commit into from
Nov 4, 2021

Conversation

taylorotwell
Copy link
Member

Passing entire class names in the can middleware can be a bit cumbersome and annoying since you can't click through to the class definition.

This adds a very simple helper for adding the can middleware to a given route.

@hdogan
Copy link

hdogan commented Nov 3, 2021

IMHO, authorize might be better for consistent naming (in conjunction with controller authorize method provided by AuthorizesRequests)

@Jubeki
Copy link
Contributor

Jubeki commented Nov 3, 2021

Just an idea: What about updating/viewing resources?

e.g.

Route::post('posts/{post}')->can('update')->withModel('post');

// or

Route::post('posts/{post}')->can('update', Route::binding('post'));

// or

Route::post('posts/{post}')->canModel('update', 'post');

Here Route::binding('post') is an example for lazy retrieving of the model which is used in the route.

Though I am not really happy with any of my ideas.
I just wanted to show them.

@tontonsb
Copy link
Contributor

tontonsb commented Nov 3, 2021

If I get it correctly, it allows specifying additional parameters using this syntax: can('create', [Flight::class, Passenger::class]) and it would produce a comma separated list of all the given terms.

I propose that not requiring the wrapping array would provide a more natural syntax and also make the implementation simpler:

    public function can(...$terms)
    {
        return $this->middleware(['can:'.implode(',', $terms)]);
    }

And then the developer does can('create', Flight::class, Passenger::class, 'other-stuff', 'whatever arg') and so on.

This would also allow type hinting if that's something desired in the framework: public function can(string ...$terms).

@MasterRO94
Copy link
Contributor

You can install Laravel Idea plugin and you will get completions almost for all strings such as configs, validation rules, gate abilities, request data and more.

Just try a free monthly trial and you won't be able to work without it.

P.S. This is not an Ad. I just want to support the author)

@ugorur
Copy link
Contributor

ugorur commented Nov 3, 2021

IMHO, authorize might be better for consistent naming (in conjunction with controller authorize method provided by AuthorizesRequests)

because of we used to use can as middleware, authorize can be confusing.

@timacdonald
Copy link
Member

timacdonald commented Nov 3, 2021

I obviously have feelings / opinions about this stuff haha - but putting that to the side and looking at this objectively - I like this in isolation for sure.

Wondering about scalability + consistency though. These might not be issues - but just some thoughts.

Ending up with a mixture of ->middleware() + ->can() calls.

Route::stuff()
    ->middleware(['auth'])
    ->can('create', Model::class);

Having a first party named method for one, but not another...

Route::stuff()
    ->middleware(['throttle:1,2,3'])
    ->can('create', Model::class);

@ugorur
Copy link
Contributor

ugorur commented Nov 3, 2021

If I get it correctly, it allows specifying additional parameters using this syntax: can('create', [Flight::class, Passenger::class]) and it would produce a comma separated list of all the given terms.

I propose that not requiring the wrapping array would provide a more natural syntax and also make the implementation simpler:

    public function can(...$terms)
    {
        return $this->middleware(['can:'.implode(',', $terms)]);
    }

And then the developer does can('create', Flight::class, Passenger::class, 'other-stuff', 'whatever arg') and so on.

This would also allow type hinting if that's something desired in the framework: public function can(string ...$terms).

string $ability, ...$terms will be much better.

@GrahamCampbell GrahamCampbell changed the title Allow can method to be chained onto route for quick authorization [8.x] Allow can method to be chained onto route for quick authorization Nov 3, 2021
@taylorotwell
Copy link
Member Author

taylorotwell commented Nov 4, 2021

@timacdonald I had the same thought - this whole thing was somewhat inspired by thinking about your has-parameters package. I was looking at current middleware that require cumbersome argument inputs and landed on authorize and throttle. However, the throttle middleware's cumbersome syntax is no longer documented and instead we encourage named throttlers: https://laravel.com/docs/8.x/routing#rate-limiting

So, that left the authorize middleware. And, it just hit me to make it a method like the withTrashed stuff that was recently added to individual route definitions. I do agree with you though this opens the door for a throttle method directly on routes as well.

I agree there are implications there as far as what middleware do we offer methods for. Obviously we can't do this for every middleware in Laravel, but my goal was to mainly think about it for the more cumbersome ones to define.

This was also spurred by a recent PR that allows you to pass stringable objects as middleware, which got my juices flowing on this but ultimately decided on a dedicated method: #39439

@imanghafoori1
Copy link
Contributor

imanghafoori1 commented Nov 4, 2021

I think it should be clear whether this can stuff is a must for the route to get matched or it is a middleware that is checked after the route is matched.

If it is a middleware, then what happens if the condition does not match? Where to change the response or the reaction of the system in case of failure.

@taylorotwell
Copy link
Member Author

@imanghafoori1 those are questions regarding the Authorize middleware in general and not really relevant to this PR I don't think. This is just a helper for adding that existing middleware onto your route.

@DarkGhostHunter
Copy link
Contributor

Instead of adding helpers, it could pass any non-existing method as a middleware using the arguments as parameters.

This way you can use can('create', Flight::class), receive with magic, and pass it as can:create,\App\Models\Flight if the method name exists as a middleware. Otherwise, a method not found exception is thrown.

I believe the Router registrar is Macroable, so macros should come first, allowing the user to override before passing it as middleware.

I'm gonna make a PR for this don't stop me.

@taylorotwell
Copy link
Member Author

I love magic but that's probably a bit too far for me 😆

@DarkGhostHunter
Copy link
Contributor

DarkGhostHunter commented Nov 4, 2021

I love magic but that's probably a bit too far for me 😆

The whole framework is magic.

PR is done 90%. Hold 10 min and you will see it.

@DarkGhostHunter
Copy link
Contributor

#39483

@taylorotwell
Copy link
Member Author

@Jubeki you just do ->can('view', 'post') just like the can middleware.

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.

9 participants