-
-
Notifications
You must be signed in to change notification settings - Fork 266
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
Allow using a custom guard for authorization by setting config #265
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the effort, why not using the same auth config without creating a new one?
for example:
'auth' => 'your gate'
then in the controller, we can use something like this:
if (config('larecipe.settings.auth') == true) // normal auth
if (config('larecipe.settings.auth') == false) // without auth
(config('larecipe.settings.auth') != null) // custom gate
what do you think about this?
With strict equality, ie:
I could get behind this. Whatever your preference is, is good by me 😄 |
yes exactly. What do you think I just feel it's redundant to have both configs unless you have a certain case that you might need the two of them? |
is this helping? https://github.com/larecipe/larecipe-docs/pull/8/files |
Hey @saleem-hadad and @kfriars I already implemented this I think in a PR long ago. So really when you set auth to false, you can then fall back on any middleware you want which lets you then use gates. |
I will provide some updated docs in the PR too, I agree it can feel redundant but I usually lean on the side of being blatant. I see no issue with stuffing all of the functionality in the I will update my PR soon and you can have a look. |
haha my bad, that's the exact PR I did and referred to above. Good find there @saleem-hadad |
Thanks for helping out @WillGoldstein! My issue is not the middleware, but the guard configured (and ultimately the UserProvider). The way the package is currently configured will only use the default Guard resolved from the service container, but in my project I have several and I want to select a particular one for docs. If I am missing something, please let me know. |
@kfriars thats awesome and makes sense! Sorry I didn’t see that benefit. Your addition is a supplement and would be great to have. |
Here is my attempt at shoehorning the We would end up with: if (config('larecipe.settings.auth')) {
if (config('larecipe.settings.auth') !== true) {
Auth::shouldUse(config('larecipe.settings.auth'));
}
$this->middleware(['auth']);
} else {
if (config('larecipe.settings.middleware')) {
$this->middleware(config('larecipe.settings.middleware'));
}
} I have also realized a situation where this implementation is less flexible (But would still accommodate my use-case). Imagine inside your Gate::define('viewLarecipe', function ($user, $documentation) {
if ($documentation->title === 'Admin Panel') {
return $user && $user->isAdmin();
}
return true;
}); With the implementation in this comment, this would not be possible, since the auth middleware would fail and would 403 before hitting the gate, if no $user was authenticated. If you would like this version I will update the PR, otherwise please accept as is, or give me some details on further changes. |
@saleem-hadad or @WillGoldstein it would be ideal for me to have this merged before Thursday. I can use my fork for now, but would like to close this out soon. Please let me know if there is anything else you would like in the PR. |
Hey @saleem-hadad and @WillGoldstein ! Just a friendly nudge that this still exists 😄 Again, if there is anything else you would like me to include in the PR, please let me know. |
I actually can’t merge it... but thanks for doing this! Hopefully @saleem-hadad will soon |
@saleem-hadad ☝️ ❓ |
I have come across your lovely package, but my project uses several custom guards which means that
$this->authorize('viewLarecipe', $documentation)
will always fail, as theweb
gaurd's$user
will always benull
.This is a pretty straightforward PR that is non-breaking, but allows people to use custom guards to protect their LaRecipe Docs.