laravel / passport

Laravel Passport provides OAuth2 server support to Laravel.
https://laravel.com/docs/passport
MIT License
3.29k stars 780 forks source link

[13.x] Configure the user provider for PAT #1768

Closed hafezdivandari closed 4 months ago

hafezdivandari commented 4 months ago

We have documented how to add a guard with 'passport' as driver and set its 'provider'. So it's common practice to have several guards and providers. This PR makes PAT factory respect the guard provider when issuing personal access tokens.

Changes

github-actions[bot] commented 4 months ago

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

driesvints commented 4 months ago

@hafezdivandari forgive me but I don't really understand why all of this is necessary. Passport should only issue tokens for one client? Why offer support for different guards? Could you give some practical use cases?

hafezdivandari commented 4 months ago

@driesvints thank you for asking. We already support different guards and user providers not only on Passport but on the entire Laravel auth system.

On Passport, the Client model has provider property and the token guard uses this provider to retrieve the user. You can add HasApiTokens trait to any authenticatable model, define a customized auth guard / provider and it works!

Sanctum has the same ability, that you can use HasApiTokens trait to any authenticatable model, but handles the auth differently - it has tokenable property on its PAT model, that means you can issue tokens for different models / providers / guards.

What this PR does is that it fixes a bug on Passport PAT when using customized user provider. But the support for different guards is not about Passport or something that this PR offer.

driesvints commented 4 months ago

Thanks @hafezdivandari. I think I might not fully understand yet. For me, there should only be the current id and secret keys on personal_access_client and nothing else. I don't yet understand how custom user providers come into play. How can there ever be a use case when we need to issue access tokens for a different ID and secret than that of the base one? All PAT should originate from the same one imo since it represents the main app.

hafezdivandari commented 4 months ago

@driesvints let me explain that, assume that you have different guards / user providers like this one. Then you issue token for each one:

$user->createToken('user token');                  // uses default client to issue token
$businessUser->createToken('business user token'); // also uses the default client to issue token!

both these tokens have the same oauth_client_id that has users as provider. So when a business user tries to login, the TokenGuards retrieves a normal user instead and logins a wrong entity!

driesvints commented 4 months ago

@hafezdivandari thanks for that. I fully understand now. I think the PR is okay in this case but just got one remark:

So for each user provider you need to run php artisan passport:client --personal and create a separate one?

Shouldn't the config then look like:

'personal_access_client' => [
    'users' => [
        'id' => env('PASSPORT_PERSONAL_ACCESS_CLIENT_ID'),
        'secret' => env('PASSPORT_PERSONAL_ACCESS_CLIENT_SECRET'),
    ],

    'customers' => [
        'id' => env('PASSPORT_PERSONAL_ACCESS_CLIENT_CUSTOMER_ID'),
        'secret' => env('PASSPORT_PERSONAL_ACCESS_CLIENT_CUSTOMER_SECRET'),
    ],
],
hafezdivandari commented 4 months ago

@driesvints for backward compatibility, it first look for the specified provider credentials, if not found it fallbacks to personal_access_client.id and personal_access_client.secret like before, for those who just use default guard / provider. So we don't need any upgrade guide entry for this change.

driesvints commented 4 months ago

Thanks for your work @hafezdivandari