Skip to content
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

[9.x] Initial support for multiple providers #1220

Merged
merged 16 commits into from
Apr 28, 2020
Merged

[9.x] Initial support for multiple providers #1220

merged 16 commits into from
Apr 28, 2020

Conversation

billriess
Copy link
Contributor

@billriess billriess commented Apr 17, 2020

This PR adds support to allow passport to use and enforce multiple providers.

The issue:

Currently, Passport only uses the UserProvider passed into it from the RequestGuard. This can be problematic if you have more than one provider (config/auth.php) that needs to be authenticated. Since Passport has no knowledge of other UserProvider you can end up in situations where the wrong user is authenticated and returned.

The solution:

This PR adds support to optionally set the provider that should be used on the Passport\Client. If a provider is defined on the Passport\Client, it will compare it against the UserProvider coming in from the RequestGuard and return the user if they match. This ensures that the user being returned is coming from the right model. If there is a mismatch, no user is returned, thus leaving you with an unauthenticated request. If no provider is defined on the Passport\Client then everything works as it always has.

Breaking changes:

The only big breaking change here is that this requires a schema change to the oauth_clients table to add the provider column.

@driesvints driesvints marked this pull request as draft April 17, 2020 07:41
@driesvints driesvints changed the title Initial support for multiple providers [8.x] Initial support for multiple providers Apr 17, 2020
@driesvints
Copy link
Member

Hey @billriess. Thanks a lot for giving this another go! I've marked the PR as draft because some tests are still failing. Let's get those fixed first before we mark it as ready.

Copy link
Member

@driesvints driesvints left a comment

Choose a reason for hiding this comment

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

Gave this a quick review. In general this looks good. But I believe the PR should definitely go to master because of the breaking changes.

Thanks for your work already!

src/Bridge/Client.php Show resolved Hide resolved
src/Bridge/Client.php Show resolved Hide resolved
src/ClientRepository.php Show resolved Hide resolved
src/ClientRepository.php Show resolved Hide resolved
src/Console/InstallCommand.php Outdated Show resolved Hide resolved
src/Guards/TokenGuard.php Outdated Show resolved Hide resolved
src/Guards/TokenGuard.php Outdated Show resolved Hide resolved
@billriess billriess changed the base branch from 8.x to master April 17, 2020 08:00
@billriess
Copy link
Contributor Author

I fixed DocBlocks and StyleCI suggestions as well as updated the way the provider defaults when using the artisan command.

I will take a look at the failed tests later. I encourage anyone interested in this feature to review it and make suggestions as needed.

@driesvints driesvints changed the title [8.x] Initial support for multiple providers [9.x] Initial support for multiple providers Apr 17, 2020
@driesvints driesvints mentioned this pull request Apr 17, 2020
@taylorotwell
Copy link
Member

Can someone describe what "multi-provider" even means?

@lucadegasperi
Copy link

@taylorotwell inside the config/auth.php you can define multiple providers.
The possibility to choose a provider is not currently exposed by passport.
Since adding request parameters to an oAuth2 call is generally not recommended, one possible way and probably the easiest due to the underlying league package, is to be able to tie a provider to an oauth 2 client.

@billriess
Copy link
Contributor Author

@taylorotwell as @lucadegasperi has explained the current implementation Passport is fairly restrictive when using things like password grants. This PR allows us to define what provider should be used at the oauth_client level. This way, we can have separate oauth_clients for different providers, which trickles down to different models.

This also fixes a potential security concern as Passport defaults to passing in the provider from the request's guard. This means the Authenticable returned by Passport would be whatever middleware guard you had in place on the route - allowing a User token to retrieve a Customer if their IDs match. With the PR it checks the guard's UserProvider with the oauth_client's UserProvider (if set) and does not return the Authenticatable if they are different.

@billriess
Copy link
Contributor Author

I've just pushed another update that fixes some of the tests. I'm a little stumped on the remaining failed tests as they are related to the way the PSR is mocked. If anyone has some ideas on how I can get those last tests to pass I would appreciate it.

@billriess
Copy link
Contributor Author

Fixed the validation on the TokenGuard to fall back properly if a provider is not set on the oauth_client.

@billriess billriess marked this pull request as ready for review April 18, 2020 04:15
Copy link
Member

@driesvints driesvints left a comment

Choose a reason for hiding this comment

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

Think we're almost there. Nice work already 👍

src/Client.php Outdated Show resolved Hide resolved
src/ClientRepository.php Outdated Show resolved Hide resolved
src/Bridge/Client.php Outdated Show resolved Hide resolved
src/ClientRepository.php Outdated Show resolved Hide resolved
src/Console/InstallCommand.php Show resolved Hide resolved
src/Guards/TokenGuard.php Outdated Show resolved Hide resolved
src/Guards/TokenGuard.php Outdated Show resolved Hide resolved
src/Guards/TokenGuard.php Outdated Show resolved Hide resolved
src/Guards/TokenGuard.php Outdated Show resolved Hide resolved
src/Client.php Outdated Show resolved Hide resolved
@taylorotwell
Copy link
Member

taylorotwell commented Apr 28, 2020

I'm confused. Where in this code does it actually use the Client model's provider to pull the model from the database. I see the PassportUserProvider provider name is compared to the clients. But PassportUserProvider's provider is always set to $config['provider']... which appears to be some static value and not controlled by the client being used at all. I guess I'm lost?

What is my actual auth configuration file supposed to look like when using this? I guess I am supposed to define a different guard in my auth configuration file each type of client provider I have?

@billriess
Copy link
Contributor Author

I'm confused. Where in this code does it actually use the Client model's provider to pull the model from the database. I see the PassportUserProvider provider name is compared to the clients. But PassportUserProvider's provider is always set to $config['provider']... which appears to be some static value and not controlled by the client being used at all. I guess I'm lost?

If the requested provider matches the one on the client then we know we can let them in and return the user. If there is a mismatch, we know that the token they are using is not setup for whatever provider you're trying to guard against.

config/auth.php

...
    'guards' => [
        'web' => [
            'driver' => 'session',
            'provider' => 'users',
        ],
        'api' => [
            'driver' => 'passport',
            'provider' => 'users'
        ],
        'api:customers' => [
            'driver' => 'passport',
            'provider' => 'customers'
        ],
    ],

    'providers' => [
        'users' => [
            'driver' => 'eloquent',
            'model' => App\User::class,
        ],
        'customers' => [
            'driver' => 'eloquent',
            'model' => App\Customer::class,
        ],
    ],

routes/api.php

// This should only be accessible by Users...
Route::middleware('auth:api')->get('/user', function (Request $request) {
    return $request->user();
});

// This should only be accessible by Customers...
Route::middleware('auth:api:customers')->get('/customer', function (Request $request) {
    return $request->user();
});

@taylorotwell
Copy link
Member

Gotcha.

@billriess
Copy link
Contributor Author

#161 and #982 go more into detail to the problem and various ways its been discussed to resolve it.

@taylorotwell taylorotwell merged commit e52ceba into laravel:master Apr 28, 2020
@taylorotwell
Copy link
Member

Thanks. We'll want some documentation around this.

@driesvints
Copy link
Member

Thanks @billriess for all your work on this!

@ziming
Copy link

ziming commented May 7, 2020

For my existing oauth_clients do I need to put in a value of 'users' in the provider column for them? Since the default provider in config/auth.php is 'users'

@driesvints
Copy link
Member

@ziming no, the column is nullable (see upgrade guide) and will take the default provider if it's set as null.

@sajadsholi
Copy link

how can we add this change to our project?

@sajadsholi
Copy link

sajadsholi commented May 10, 2020

@driesvints
I update the laravel/passport to v9.1 and add the customer provider to auth.php config and in the customer's routes if I run the

dd($request->user());

it returns the customer's info and for user's routes it works fine and it return the user's info too but I think there is a bug about tokens
if I use user token for customer's routes it doesn't return this response :

{
    "error": "Unauthenticated."
}
// 401 status 

It return customer's info again

@billriess
Copy link
Contributor Author

Did you set the provider on the client?

@sajadsholi
Copy link

sajadsholi commented May 11, 2020

@billriess
actually my user types are : user , admin , courier and I set these config for auth.php :

    'guards' => [
        'web' => [
            'driver' => 'session',
            'provider' => 'users',
        ],
        'admin' => [
            'driver' => 'session',
            'provider' => 'admins',
        ],
        'api' => [
            'driver' => 'passport',
            'provider' => 'users',
            'hash' => false,
        ],
        'api:couriers' => [
            'driver' => 'passport',
            'provider' => 'couriers',
            'hash' => false,
        ],
    ],

    'providers' => [
        'users' => [
            'driver' => 'eloquent',
            'model' => App\User::class,
        ],
        'admins' => [
            'driver' => 'eloquent',
            'model' => App\Admin::class,
        ],
        'couriers' => [
            'driver' => 'eloquent',
            'model' => App\Courier::class,
        ],
    ],

and for the user and courier's model I did like this :

namespace App;
use Laravel\Passport\HasApiTokens;
use Illuminate\Foundation\Auth\User as Authenticatable;

class Courier extends Authenticatable
{
    use  HasApiTokens;
}

and the api.php :

Route::prefix('courier')->name('courier.')->middleware('auth:api:couriers')->group(function () {

    Route::get('/' , function($request){
        dd($request->user());
    });

});

Route::prefix('user')->name('user.')->middleware('auth:api')->group(function () {

    Route::get('/' , function($request){
        dd($request->user());
    });

});

everything is fine but in postman i can use courier token for user's routes and use user token for courier's routes too

@billriess
Copy link
Contributor Author

In your database oauth_clients is the provider column set?

@sajadsholi
Copy link

no the provider column does not exist in the table and I ran the

php artisan migrate:fresh --seed

and it didn't add to table by migration

@driesvints
Copy link
Member

@sajad2176 please read the upgrade guide: https://github.com/laravel/passport/blob/9.x/UPGRADE.md#support-for-multiple-guards

@heyaj19
Copy link

heyaj19 commented Jun 12, 2020

@billriess @driesvints I still think there is an issue - I have my providers set in auth.php, the provider column added.

On LoginController with no middleware set, it uses users model. This is working as expected.

The issue starts here. When I change the guard to use

            $this->middleware('guest:managersapi')->except('logout'); 

It's still using the users model not the managers model.

In the auth.php file I have a guard


    'managersapi' => [  //CURRENT GUARD ROUTE
                'driver' => 'passport',
                 'provider' => 'managers'
            ],

Am I missing something. I created a new client-id, client-secret with a provider column pointing to managers.

Edit:
Adding more info if I use the client_id, client_secret that is for managers - the user model does not work. So this part is at least working as expected - the issue is login for a different provider

@billriess
Copy link
Contributor Author

@heyaj19

Some things to note:

This PR covers password grants for bearer tokens. If you're not using that flow, it won't do anything new than it did previously.

You must have the provider defined to force that provider to be used. In your case, managers should be used in your oauth_clients table.

@heyaj19
Copy link

heyaj19 commented Jun 12, 2020

@billriess I am using bearer tokens. This is my guzzle request after the client calls the login route

     $response = $client->post(config('app.PASSPORT_URL'), [
            'form_params' => [
               'client_id' => request('client_id'),
               'client_secret' => request('client_secret'),
                'scope' => '*',
               'username'=>request('email'),
               'password'=>request('password'),
               'grant_type' => request('grant_type')
           ],
        ]);

And like I mentioned above, I do have a client_id, client_secret and provider for the managers table. The login is not working for a different provider other than the USER model

or are you saying the Login route wont work but everything after the bearer token has been issued will

EDIT: Its working

@billriess
Copy link
Contributor Author

@heyaj19 Glad to see you got it working.

@gilles6
Copy link

gilles6 commented Jul 15, 2020

This feature is working great. Thank you very much for making it.

However, from what I understand, it only works with password grant tokens access type (which is now deprecated).

Is there any chance that it is extended to the standard OAuth2 authorization code redirect flow ?

@billriess
Copy link
Contributor Author

@gilles6 Thanks, glad to see it's working well for you!

I don't see why this couldn't be extended to work with any other part of Passport. Given that the OAuth2 spec is moving away from Password grants we could see authorization code flow become more used. I don't have a lot of free time at the moment but I can keep this on my things to look at once I get some time.

@gilles6
Copy link

gilles6 commented Jul 16, 2020

@billriess That would be great :-) Let me know, I'd be one of your first testers!

@guianzollin
Copy link

api:customers

How can I do in my login method? Because Auth::guard('api:customers')->attempt($validated) and Auth::guard('api:customers')->user()->createToken('authToken') do not work.

"message": "Method Illuminate\Auth\RequestGuard::attempt does not exist.",

@ktmsulaim
Copy link

@guianzollin Auth::attempt only works with driver set to session in auth.php

@gilles6
Copy link

gilles6 commented Jan 12, 2021

@billriess
Is there any update on implementing multiple providers with other part of Laravel Passport than Password Grant Token ?

@tiggidoo
Copy link

I have same problem. multi-auth only work with session.If that the token does not work.

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.