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

Multiple Guards on a Single Route #488

Open
derekrprice opened this issue Feb 1, 2021 · 7 comments
Open

Multiple Guards on a Single Route #488

derekrprice opened this issue Feb 1, 2021 · 7 comments

Comments

@derekrprice
Copy link

derekrprice commented Feb 1, 2021

  • Laravel Version: 6.8
  • Laratrust Version: 5.2.9

Describe the bug
Laravel handles this auth statement just fine, such that 3 user types, each with their own guards can share a single route. If any of the three guards allow access, then access to the route is allowed:

Route::middleware([
    'auth:api,client,its'
])->group(function () {
    Route::get('/stuff', 'StuffController@all');
});

Unfortunately, when I try to add a Laratrust permission check, only the first guard that I list is checked:

Route::middleware([
    'auth:api,client,its',
    'permission:read-asset,guard:api|guard:client|guard:its'
])->group(function () {
    Route::get('/stuff', 'StuffController@all');
});

This seems to be an explicit assumption of LaratrustMiddleware::extractGuard(), as it uses ->first() during the extraction:

protected function extractGuard($string)
{
    $options = Collection::make(explode('|', $string));

    return $options->reject(function ($option) {
        return strpos($option, 'guard:') === false;
    })->map(function ($option) {
        return explode(':', $option)[1];
    })->first();
}

To Reproduce
See above.

Question
Is there some way to manage multiple guards on the same route?

@derekrprice
Copy link
Author

Yeah, I made this work fine by hacking extractGuard() and assignRealValuesTo() to return a list of guards, then looping over that in authorization(). I'll try to put a PR together in the morning.

derekrprice added a commit to PartechGSS/laratrust that referenced this issue Feb 2, 2021
derekrprice added a commit to PartechGSS/laratrust that referenced this issue Feb 2, 2021
@derekrprice
Copy link
Author

Nudge. @santigarcor, does anyone look at these? The attached PR includes tests and adds implied functionality without any breaking changes. I wasn't totally sure about the configuration syntax I chose, but it should be close.

@santigarcor
Copy link
Owner

@derekrprice I've been a bit busy lately, I could take a deep look at it this weekend.
I gave a quick look to it but I didn't find any custom configuration syntax, When you said that you weren't totally sure about the config syntax what were you referring to?

@derekrprice
Copy link
Author

derekrprice commented Feb 11, 2021

@santigarcor I just took what was previously supported and extended it with additional guards separated by pipes (e.g. permission:read-asset,guard:api|guard:client|guard:its). It seemed a fairly intuitive leap and was the first thing I tried after reading the docs. It also required very little code. The other syntax I was considering was a little more compact but I didn't know if it had other implications: permission:read-asset,guard:api,client,its. That syntax would require more code too. That's all.

@connecteev
Copy link

@santigarcor @derekrprice Has this old issue been resolved?

I am trying to decide between this package and https://github.com/spatie/laravel-permission, and while I like spatie's packages because of how well / often they are maintained, I do like the teams functionality that comes with laratrust.

Is this package being actively maintained, because I am seeing some old issues that are yet to be resolved.

@derekrprice
Copy link
Author

No. Per my comment which closed PR #489:

This has been outstanding a long time. I still like the syntax, but I don't think my implementation was correct. This allowed the multiple guards but would sometimes find the wrong user when there was an ID overlap. In any case, I'm no longer working with the code that I needed this for, so I am just going to close this PR.

@connecteev
Copy link

In that case, it would be great to get a fix for this if possible @santigarcor!

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 a pull request may close this issue.

3 participants