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

Global Gate::before() checks affect Laravel policy checks outside of Statamic #10832

Open
jesseleite opened this issue Sep 24, 2024 · 5 comments

Comments

@jesseleite
Copy link
Member

jesseleite commented Sep 24, 2024

Bug description

A user reported that these global Gate:: calls affect Laravel policy checks outside of Statamic.

More specifically, this super user check will return true in policies outside of Statamic, which can lead to some unexpected behaviour when installing Statamic into existing Laravel apps...

Gate::before(function ($user, $ability) {
    return optional(User::fromUser($user))->isSuper() ? true : null;
});

After some internal discussion, maybe we could look at a way to opt-out of these global Gate calls in favour of a middleware-based approach that can be selectively applied to routes / route groups?

Maybe that middleware-based approach becomes the new default in a major version as well (technically this is breaking change for anyone relying on these gates in addons, etc)?

Also, the above approach could work for normal web requests, but what about commands, jobs, etc.?

Related: #8337

@DanielCoulbourne
Copy link

Second this, I was the "user" in question. Spent an hour and a half debugging to the ends of the earth with a client why we couldn't get a specific policy in his Laravel app to fail. Turns out the user he was logged in as was a statamic superuser.

I think that installing Statamic into a laravel app and making a user a "superuser" should not make them immune to all the extant policies in the laravel app.

I think the middleware idea is ideal. That way statamic can apply it to all their routes, and if users want to opt in to have statamic SU's be Godmode in their app, they can just add the middleware to their stack.

@jasonvarga
Copy link
Member

I don't think a middleware would really work, since we might need access to the gates outside of the context of a request, like a command, job, tinkerwell, etc.

Another idea we came up with was to confirm if the ability the gate is checking for is one that is intended for Statamic, otherwise leave it alone.

I think that installing Statamic into a laravel app and making a user a "superuser" should not make them immune to all the extant policies in the laravel app.

Agree 100%. This really just seems like an oversight.

@ryanmitchell
Copy link
Contributor

Could it work to check if the $user passed in the gate is a Statamic User, and only run the gate check if they are, as outside of Statamic it would just be a user model?

@jasonvarga
Copy link
Member

That's probably even better, unless there's a gotcha somewhere I can't think of right now. 😄

@jasonvarga
Copy link
Member

jasonvarga commented Sep 26, 2024

@DanielCoulbourne In your app are you using DB or file based users? Doesn't matter really.

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

Successfully merging a pull request may close this issue.

5 participants