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

Add CheckScopes and CheckForAnyScope Middleware #310

Merged
merged 5 commits into from Oct 13, 2021

Conversation

filippofortino
Copy link
Contributor

This PR add the CheckScopes middleware and CheckForAnyScopes middleware from Passport.
The code is basically copy-pasted from Passport.

This idea was discussed earlier today by @masterix21 and @themsaid on Twitter.

https://twitter.com/masterix21/status/1448212135578935296

@taylorotwell taylorotwell merged commit 2576f93 into laravel:2.x Oct 13, 2021
@driesvints
Copy link
Member

@filippofortino I just released this. Can you also PR this to the docs of Sanctum? Thanks!

@filippofortino
Copy link
Contributor Author

@filippofortino I just released this. Can you also PR this to the docs of Sanctum? Thanks!

Sure, don't know if i'm gonna be able to do that today, but will do it by tomorrow.

I was thinking however, since Sanctum refers to scopes as abilities, would it be better to rename this two middlewares to CheckAbility and CheckForAnyAbilities?

Of course you can already register them like so in app/Http/Kernel.php and use it your app, but idk, i just thought it was wort mentioning.

'abilities' => \Laravel\Sanctum\Http\Middleware\CheckScopes::class,
'ability' => \Laravel\Sanctum\Http\Middleware\CheckForAnyScope::class

Let me know what you think and i can send a PR in case you agree.

@driesvints
Copy link
Member

You can always try to see if Taylor would accept it 👍

@masterix21
Copy link

masterix21 commented Oct 20, 2021

I was thinking however, since Sanctum refers to scopes as abilities, would it be better to rename this two middlewares to CheckAbility and CheckForAnyAbilities?

I think it would break a lot of applications.

@driesvints
Copy link
Member

@masterix21 this PR is just a week old

@masterix21
Copy link

@driesvints I'm talking about the idea to replace CheckScopes and CheckForAnyScope with *Ability.

@filippofortino
Copy link
Contributor Author

@driesvints I'm talking about the idea to replace CheckScopes and CheckForAnyScope with *Ability.

This will only replace the newly added middlewares in Sanctum, if you're using the ones in Passport they won't be affected.

However, I'll send a PR later today and we'll see.

@driesvints
Copy link
Member

@filippofortino a BC solution for now would be to move the functionality to the new traits and then re-use those traits in the old ones. Then add a @deprecated annotation to the old traits to point people to the new ones. We can then remove them in the next major version.

@filippofortino
Copy link
Contributor Author

@driesvints alright I'll do that!

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.

None yet

4 participants