laratrust icon indicating copy to clipboard operation
laratrust copied to clipboard

Multiple Guards on a Single Route

Open derekrprice opened this issue 3 years ago • 7 comments

  • 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 avatar Feb 01 '21 21:02 derekrprice

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 avatar Feb 01 '21 21:02 derekrprice

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.

derekrprice avatar Feb 10 '21 12:02 derekrprice

@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?

santigarcor avatar Feb 11 '21 14:02 santigarcor

@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.

derekrprice avatar Feb 11 '21 16:02 derekrprice

@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.

connecteev avatar Jul 02 '23 14:07 connecteev

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.

derekrprice avatar Jul 03 '23 20:07 derekrprice

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

connecteev avatar Jul 03 '23 21:07 connecteev