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] Adds on-demand gate authorization #39789

Merged
merged 14 commits into from Dec 2, 2021
Merged

[8.x] Adds on-demand gate authorization #39789

merged 14 commits into from Dec 2, 2021

Conversation

DarkGhostHunter
Copy link
Contributor

@DarkGhostHunter DarkGhostHunter commented Nov 26, 2021

What?

Allows the gate to permit or forbid a procedure by a condition, instead of using the gate to register a one-time ability or using verbose checks. Rework of #39778.

Before

use Illuminate\Auth\Access\AuthorizationException;
use App\Models\Wallet;

$wallet = Wallet::find(1);

if ($wallet->overQuota()) {
    throw new AuthorizationException("This action exceeds your cuota");
}

After

use Illuminate\Support\Facades\Gate;
use App\Models\Wallet;

$wallet = Wallet::find(1);

// Forbids an action...
Gate::forbid($wallet->overQuota(), "This action exceeds your cuota");

// or permits an action.
Gate::permit($wallet->underQuota(), "This action exceeds your cuota");

This is intended is only for one-time checks. Developers are encouraged to register abilities when these can be reused.

Why?

Because you can bypass registering an action in the Gate, without abandoning the Gate powers. For example, you can also issue callbacks to retrieve the currently authenticated user and even set a message...

use Illuminate\Support\Facades\Gate;
use App\Models\Wallet;

Gate::forbid(fn ($user) => $user->wallet->overQuota(), "You exceed your cuota", 409);

... or hijack the callback with a Response instance anytime.

use Illuminate\Auth\Access\Response;use Illuminate\Support\Facades\Gate;
use App\Models\Wallet;

Gate::forbid(function ($user) {
    if ($user->can('bypassQuota')) {
        // This takes precedence. 
        return Response::allow('Quota limits are not applied to you.');
    }
    
    return $user->wallet->overQuota();
});

BC?

Nope, only additive.

@DarkGhostHunter
Copy link
Contributor Author

Help pls Docker decided to die.

@DarkGhostHunter
Copy link
Contributor Author

Can someone help me re-run the jobs?

@driesvints
Copy link
Member

@DarkGhostHunter done

@DarkGhostHunter
Copy link
Contributor Author

Tx

@taylorotwell
Copy link
Member

taylorotwell commented Dec 1, 2021

@DarkGhostHunter

Renamed to allowIf and denyIf.

I have a concern with the behavior of the callback in regards to the currently authenticated user. Currently, it functions differently than typical gates. Typically, a gate will automatically throw an authorization exception if no user is authenticated unless the given gate or policy method has a nullable type hint on the user.

However, this PR will just pass null into the callback if the user is not authenticated.

image

IMO it should function exactly like a typical gate if you pass a callback to the allowIf or denyIf methods.

@taylorotwell
Copy link
Member

Converting to draft while you potentially address this. Please mark as ready for review when you would like me to look at this PR again.

@taylorotwell taylorotwell marked this pull request as draft December 1, 2021 14:10
@DarkGhostHunter
Copy link
Contributor Author

DarkGhostHunter commented Dec 1, 2021

I reworked the method to always throw an exception if the callback requires the authenticated user and there is none.

I thought a AuthenticationException would make more sense, but since the user requirement is part of the callback itself, I think it's fine to even respect the same message and code issued (if any).

This will return an AuthorizationException

// No user authenticated
Gate::allowIf(function (Authenticatable $user) {
    return true;
}, 'Do not');

// AuthorizationException: "Do not".

@DarkGhostHunter DarkGhostHunter marked this pull request as ready for review December 1, 2021 19:54
@taylorotwell
Copy link
Member

I find passing an $allow flag as the last parameter of allowIf utterly confusing. Please remove that and just duplicate some code if you have to between the two methods. It makes them both much easier to read.

@laravel laravel locked and limited conversation to collaborators Dec 2, 2021
@laravel laravel unlocked this conversation Dec 2, 2021
@taylorotwell taylorotwell marked this pull request as draft December 2, 2021 14:00
@DarkGhostHunter
Copy link
Contributor Author

DarkGhostHunter commented Dec 2, 2021 via email

@DarkGhostHunter DarkGhostHunter marked this pull request as ready for review December 2, 2021 18:05
@taylorotwell taylorotwell merged commit 8eb1827 into laravel:8.x Dec 2, 2021
@taylorotwell
Copy link
Member

Thanks for contributing to Laravel! ❤️

@DarkGhostHunter
Copy link
Contributor Author

DarkGhostHunter commented Dec 2, 2021 via email

@DarkGhostHunter DarkGhostHunter deleted the feat/authorize-callback branch December 3, 2021 02:21
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

3 participants