Adding the ability to use package with service-to-service Passport client

[SuperDJ]

No description provided.

[drbyte]

Thanks for all your work on this!

[SuperDJ]

@parallels999 & @drbyte perhaps one of you can help with this. Been testing this locally but run into a problem where the (permission)middleware always returns 403. The Auth::guard($guard) returns Illuminate\Auth\SessionGuard instead of Laravel\Passport\Guards\TokenGuard. Could this be because the route is in the client middleware as well?

[
  'client' => \Laravel\Passport\Http\Middleware\CheckClientCredentials::class,
];

For example:

Route::group(['middleware' => 'client'], function() {
   Route::get(...)->middleware('permission:edit-posts');
});

Almost as if the client or another middleware changes it.

[drbyte]

Regarding the 403, I'll have to wait til tomorrow to spend some time looking in detail.

Regarding the currently-failing tests, it's because you changed (rearranged) the order in which the test-case roles/permissions were created, so the id numbers are out of sync with the tests. We can make those tests less rigid by querying the ID numbers before testing against them, but don't need to do that inside this PR, because it's unrelated. You could put them back in original order and we can clean them up in another PR.

[SuperDJ]

@drbyte, @erikn69, @parallels999 I did some more digging around and testing. I found that when using Passport v11 and providing Auth::guard('api) did return the expected TokenGuard.

But when making api requests the provided $guard = null and so the Auth::guard is wrong.

[parallels999]

If someone uses a guard name different than api, that won't work

I found that when using Passport v11 and providing Auth::guard('api) did return the expected TokenGuard

Did you set Route::middleware(['auth:api']) on your routes?

[SuperDJ]

@parallels999 for the client credentials grant it shouldn't be used:

If you are using the client credentials grant, you should use the client middleware to protect your routes instead of the auth:api middleware.

Also when I do add the auth:api middleware to the routes it fails(403). Also see laravel/passport#898.

Would it be possible to do something like the following:

if($bearerToken && !$guard) {
  Auth::guard('api'); // Or a configuration setting
}

And later we could reset it to Auth::guard($guard) if required/ as a fallback.

[angeljqv]

For example:

Route::group(['middleware' => 'client'], function() {
  Route::get(...)->middleware('permission:edit-posts');
});

What about scopes? client-credentials-grant-tokens

Route::get('/orders', function (Request $request) {
    ...
})->middleware('client:role-name-1,role-name-2');

[SuperDJ]

@angeljqv what exactly do you mean? We don't change the working the client middleware.

[angeljqv]

We don't change the working the client middleware.

I have never said that you should change the behavior of the client middleware
I say that scopes already exists, when the token is generated, read documentation again

What about scopes?

[SuperDJ]

@angeljqv I prefer consistency. And I'm already using laravel-permission package for permissions on the web part of my project. So to me it only seems logical to also apply the permissions to the API part. That way I only have to deal with one "system" for permissions.

[angeljqv]

ok, i got it

[drbyte]

@SuperDJ This is looking good. Do you have any remaining concerns about it?

[drbyte]

Also, are you comfortable squashing the commits into one and then force-pushing that commit back to this branch?

[erikn69]

@drbyte you can use squash merge PR and change the commit message

[drbyte]

@drbyte you can use squash merge PR and change the commit message

Agreed.

[SuperDJ]

Do you have any remaining concerns about it?

@drbyte the only thing I'm not sure of, is what I mentioned here. Are the checks specific enough to only work for the Client Credentials Grant and not the other Passport Grants. Unfortunatly I'm currently unable to check this.

[drbyte]

Unfortunately it's not an area where I have much experience. Does your own app need a solution to that? Or can we safely document it as a limitation for which help is welcomed?

[SuperDJ]

@drbyte currently my app only needs to use the Client Credentials Grant. And that is working with the changes made in this PR. But we could add something to the docs indeed.

Or maybe add another check after line 95 of Guard.php perhaps on line 100 of if(!$client->fistParty()) {return null;}. That should atleast prevent 2 other Grants from being affected. Perhaps @erikn69 can shed some light on it?

[erikn69]

Password Grant should just use the User model

if the token uses User model, $authGuard->user() gets the model and getPassportClient is never gonna be called
I think it should be enough as it is, but I can't check it, I don't use passport

$user = $authGuard->user();
// For machine-to-machine Passport clients
if (! $user && $request->bearerToken()) {
    $user = Guard::getPassportClient($guard);
}

[SuperDJ]

Isn't there some sort of beta release so things like this can be tested?

[drbyte]

Isn't there some sort of beta release so things like this can be tested?

We haven't usually released "betas" on this package.
But you can even now simply composer require spatie/laravel-permission:^6.0-dev to use it prior to official release. Anything merged into the package's master branch will be incorporated.

[SuperDJ]

@drbyte made it an opt-in feature. That way is up to the end user to decide if they want to use it.

[drbyte]

@SuperDJ thanks for all your work on this.

I'm going to merge it so it can be used in dev as beta etc. And if it is found that adjustments are required, those can be done via PR.