Skip to content

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

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

Merged
merged 4 commits into from
Jul 5, 2024

Conversation

hafezdivandari
Copy link
Contributor

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

  • Add feature tests.

  • Require user_id on PersonalAccessGrant.

  • Add getProvider() method to HasApiTokens to get the user provider name.

  • Make PersonalAccessTokenFactory to check for config('passport.personal_access_client.{provider}') before getting default PAT credentials from config('passport.personal_access_client'). For example, having customers auth provider:

    // config/passport.php
    
    'personal_access_client' => [
        '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'),
        ]
    ],

Copy link

github-actions bot commented Jul 4, 2024

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.

@hafezdivandari hafezdivandari marked this pull request as ready for review July 4, 2024 17:20
@driesvints
Copy link
Member

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

@driesvints driesvints marked this pull request as draft July 5, 2024 07:57
@hafezdivandari
Copy link
Contributor Author

@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
Copy link
Member

driesvints commented Jul 5, 2024

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
Copy link
Contributor Author

hafezdivandari commented Jul 5, 2024

@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
Copy link
Member

driesvints commented Jul 5, 2024

@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
Copy link
Contributor Author

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


if (! $config) {
if (! $client || ($client->provider && $client->provider !== $provider)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be the same as the check on TokenGuard:

if (! $client ||
($client->provider &&
$client->provider !== $this->provider->getProviderName())) {

@driesvints driesvints marked this pull request as ready for review July 5, 2024 09:14
@driesvints
Copy link
Member

Thanks for your work @hafezdivandari

@taylorotwell taylorotwell merged commit ef640ef into laravel:13.x Jul 5, 2024
7 checks passed
@hafezdivandari hafezdivandari deleted the 13.x-pat-guard-provider branch July 5, 2024 17:27
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 this pull request may close these issues.

3 participants