[6.x] Re-work super user authorization check #11516
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tatamic permission
Otherwise, when the `Authorize` middleware checks if the user has access to the CP, `Permission:all()` in the `Gate::before()` closure won't return any permissions as they haven't been booted yet.
Until I figure out a better solution. 🤔
Since there's no `DroidsClass` policy, I've updated this test to authorize using a permission instead, which'll work.
duncanmcclean
commented
Feb 28, 2025
| @@ -84,7 +85,13 @@ public function boot() | |||
| }); | |||
|
|
|||
| Gate::before(function ($user, $ability) { | |||
| return optional(User::fromUser($user))->isSuper() ? true : null; | |||
| Permission::boot(); | |||
There was a problem hiding this comment.
While I think this approach is broadly on the right track, I don't really want to be booting permissions here. Although, I'm not sure what the best alternative is... 🤔
- Without booting permissions here, users can't access the CP because the
Authorizemiddleware checks if the user has the relevant permissions, however it happens before theBootPermissionsmiddleware is called. - We can't reorder the middleware, otherwise it might cause issues with translations in permission labels, which Ability to defer permission and utility registration for translations #7343 solved.
There was a problem hiding this comment.
What's wrong with booting in there? It only happens when the closure is executed.
There was a problem hiding this comment.
I guess I was just thinking it was weird since we're already booting them in a middleware, and that trying to boot them multiple times would hurt performance?
Maybe I'm wrong though, and it's fine?
There was a problem hiding this comment.
We could make Permissions::boot() return early and do nothing if it's already been booted once.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request attempts to fix an issue with how super users are authorized in Statamic.
Currently, when an authorization check is performed against a super user, authorization will be granted. This is obviously far from ideal, especially when you're integrating Statamic as part of an existing Laravel application.
This PR aims to fix this issue by only granting authorization when we know the ability/permission "belongs" to Statamic.
Closes #8337.
Closes #10832.