laratrust

laratrust copied to clipboard

Fixes #488.

@derekrprice Can we make it to work like this: permission:edit-post|edit-user,guard:api|web so we don't have to put guard: multiple times?

The parsing would be easy enough, but how do you want me to make that work with the "require_all" flag? That is documented here as being separated from "guard:" specs by a pipe. Can I assume that require_all will always come first? If it appears like this, "guard:api|require_all", the syntax you suggested will interpret "require_all" as a guard, and that might be a breaking change for existing implementations. Similarly, requiring a comma between require_all and guard: could disambiguate it, but that would also be a breaking change.

I think that one is separated by the comma as you can see here: role:admin|root,my-awesome-team,require_all so in theory we wouldn't need to monkey with it.

I was worried about this one: ability:admin|owner,create-post|edit-user,require_all|guard:web. Looking at the current parser, commas between require_all and guard: would have worked fine, but anyone working off this doc and using a pipe may end up surprised.

I made this change locally and, in fact, you have several tests that use the guard:api|require_all syntax already. The first one that fails is Laratrust\Tests\Middleware\LaratrustPermissionTest::testHandle_IsLoggedInWithNoPermission_ShouldAbort403, but there are several others as well. After fixing the two tests I added to use commas to separate require_all from guard: with a comma, 7 tests fail with messages like this:

7) Laratrust\Tests\Middleware\MiddlewareLaratrustAbilityTest::testHandle_IsLoggedInWithNoAbility_ShouldAbort403
Mockery\Exception\NoMatchingExpectationException: No matching handler found for Mockery_4_Illuminate_Auth_AuthManager::guard('require_all'). Either the method was unexpected or its arguments matched no expected argument list for this method

/home/derek/Code/laratrust/vendor/mockery/mockery/library/Mockery/ExpectationDirector.php:92
/home/derek/Code/laratrust/vendor/laravel/framework/src/Illuminate/Support/Facades/Facade.php:261
/home/derek/Code/laratrust/src/Middleware/LaratrustAbility.php:35
/home/derek/Code/laratrust/tests/Middleware/MiddlewareLaratrustAbilityTest.php:93

I suppose that I could just add a special case to the parser and assume that require_all isn't a guard regardless of where it appears, for backwards compatibility. Is that what you want me to do?

Done. Any of these syntaxes work correctly now:

require_all|guard:api|web
guard:api|web|require_all
require_all,guard:api|web
guard:api|web,require_all

Also should be resilient to stupid mistakes like the following, (silently) falling back on the default guard in all cases:

require_all,guard:
guard:|
guard:require_all

Sorry @derekrprice I think your first option was the best one. Because the third set after the second comma are the extra options for the middleware so it's better to repeat guard:... multiple times in that third set of arguments

Shoot. I force-pushed the change. Is there a way to revert a single commit after an git commit --amend followed by a force push? Would like to avoid re-implementing.

Maybe you can close the PR, delete the remote branch and in your local revert to the previous commit and then push it again and open the PR again

The commit --amend overwrote the local commit. I'll figure something out.

There you go. Didn't have to redo too much.

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.