-
Notifications
You must be signed in to change notification settings - Fork 784
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
Passport Multi-Auth (Implementation) #982
Comments
I agree that this issue might be a good place to restart the discussion on how to implement this. I currently don't have time myself to look into this but feel free to provide thoughts here. Warning: I'll be removing all comments which aren't related to looking for an actual implementation to prevent this issue from de-railing like the other one. Please only discuss the possible implementation for this. |
I think to get this started we should first determine if the multi-auth implementation should be enforced or not. Meaning, should the gate simply return the user against its own provider or should the client have a provider that enforces the user must come from a specific provider? |
Or to make it more dynamic, the provider can be supplied in the request and the guard check can be made against that request parameter to make it compatible with as many models as required |
Hello everyone. |
Personally, I think each |
But isn't that the point of having multi-auth, to have the flexibility of choosing a provider but at the same time, using fallbacks for default behavior, like using the User model as is the case now. Personally, I too think that each We should probably gather more opinions on this and implement the consensus then. |
Please remember to keep this discussion on topic. |
@billriess When can we expect a PR with the functionality that you have implemented? |
I was hoping for some more feedback as to what way would make the most sense for everyone. I can put a PR together later this week with my implementation and see how that goes. |
I think the provider should be based on the routes... therefore different routes will have different providers. // Admin passport routes
Passport::routes(
null,
[
'as' => 'admin.',
'prefix' => 'admin/oauth',
'middleware' => 'passport.guard:admin',
]
); Here I am assigning a middleware(which needs to be implemented accordingly) to the the routes as to tell passport to use the |
I think that if you're using Passport then it should be relatively safe to assume that you're not a beginner developer, and for that reason it should be as flexible as possible even if that means that a little work is required of the developer. I think it makes sense to have separate endpoints per your suggestion @nikugogoi for the various models so long as there is flexibility in the way those models are authenticated -- some models may not use email / password for example, we may use uuid/id and secret key or some such authentication. As long as the implementation allows us to attach multiple guards to a single api endpoint, e.g. if i wanted either users, or admins, or whoever to be able to access the resource. |
@zagreusinoz for customizing the way how the models are authenticated, there is a way here https://laravel.com/docs/5.8/passport#customizing-the-username-field |
The implementation I have in place locally in my project is that |
@billriess are you able to share even a draft PR with your current implementation? This would solve some issues I'm having in a project. I agree that tying an |
@tmcnicholls I've been a little busy recently with a project but I will put something together and submit it. I know PRs are viewed pretty closely here so I want to make sure the implementation I suggest and the way I implement it will be the desired approach. The way I got this working locally was changing the TokenGuard's construct to accept a |
@billriess I'd like to see you go ahead with your current implementation around each +1 for your implementation. I think to get things moving a fully fledged PR would go along-way. Also be very appreciated by the community. |
So I started a PR for this but one thing I'm stuck on is how to retrieve the string name of the Otherwise, the only way around it is to pass the |
@billriess Would it be possible to see your work so far please? I checked your forked but can't seem to find your work related to this issue :/ I think keeping the option open to specify the provider dynamically would be nice, to allow a token request like this:
|
IntroI'm currently working on something very closely related to this. And will be working on testing some changes locally that may help resolve this; #1094 and #1006 I apologize that this post is a bit long and goes slightly off topic / in depth; but figured this may end up being a great solution for others beyond just myself. But thought I'd share what I'm thinking of trying in the event anyone has any improvements or recommendations on how to make this solution better. (or even if there are any issues I'm not seeing) (Devs/Mods if I should create a separate issue please let me know, but thought it would be best to post this here instead of creating a new issue and linking it) BackgroundI was originally using two guards, and setting a cookie on login for my "admin" guard, and setting a header value for my "api-admin" guard. After talking to one of the developers at T/T they recommended extending the In my models I've overrode the public function getAuthIdentifier()
{
return get_class($this)."_".$this->{$this->getAuthIdentifierName()};
} And the "driver" I'm using for the provider looks like this: <?php
namespace App\Providers;
use Illuminate\Auth\EloquentUserProvider;
use Illuminate\Contracts\Hashing\Hasher as HasherContract;
class UserProvider extends EloquentUserProvider
{
/**
* The array of models we want to be able to authenticate against
*
* @var array
*/
protected $models = [
'App\Models\Admin',
'App\Models\User'
];
/**
* Create a new database user provider.
*
* @param \Illuminate\Contracts\Hashing\Hasher $hasher
* @return void
*/
public function __construct(HasherContract $hasher)
{
$this->hasher = $hasher;
}
/**
* Retrieve a user by their unique identifier.
*
* @param mixed $identifier
* @return \Illuminate\Contracts\Auth\Authenticatable|null
*/
public function retrieveById($identifier)
{
$split = explode('_',$identifier);
$this->model = $split[0];
$identifier = $split[1];
return parent::retrieveById($identifier);
}
/**
* Retrieve a user by their unique identifier and "remember me" token.
*
* @param mixed $identifier
* @param string $token
* @return \Illuminate\Contracts\Auth\Authenticatable|null
*/
public function retrieveByToken($identifier, $token)
{
$split = explode('_',$identifier);
$this->model = $split[0];
$identifier = $split[1];
return parent::retrieveByToken($identifier, $token);
}
/**
* Retrieve a user by the given credentials.
*
* @param array $credentials
* @return \Illuminate\Contracts\Auth\Authenticatable|null
*/
public function retrieveByCredentials(array $credentials)
{
foreach($this->models as $model)
{
$this->model = $model;
$user = parent::retrieveByCredentials($credentials);
if($user != null)
return $user;
}
return null;
}
} Registered in the Auth::provider('tenantAware', function ($app, array $config) {
return new UserProvider($app['hash']);
}); And finally the config looks like this: 'guards' => [
'web' => [
'driver' => 'session',
'provider' => 'tenantAware',
],
'api' => [
'driver' => 'passport',
'provider' => 'tenantAware',
],
],
'providers' => [
'tenantAware' => [
'driver' => 'tenantAware'
],
], With these changes normal non-api related login and viewing work perfect. As soon as I use passport users aren't selected properly. Thought ProcessInstead of passport storing everything as a "HasMany" or "BelongsTo" relationship, I'm going to change these to polymorphic relationships. This will allow the different models being used for authentication to be stored in the same database and not risk a user getting inappropriate access due to a guard not being set. I have to look further into the code (Hopefully someone else can provide better guidance here); but then the model wouldn't need to be pulled from the config as it would be stored in the record relating to the token being used. With knowing the model, and the ID; the What this achievesUsing this type of solution, there would be no need to pass anything additional with the request in order to specify which guard or provider to use as all models would be using the same guard and provider. This also decreases the amount of logic needed on the frontend dividing out and creating additional login forms for different providers. Notes / Possible IssuesThis solution assumes that all providers are using eloquent models for authentication, (can't say it would work with multiple different provider types) I haven't tested password resets or anything beyond basic login/logout. Because I'm not actually storing the |
I think we can agree that @lucadegasperi is on the right track. Feel free to send in a PR that addresses that use case. |
We will probably never accept a client that's in the request since that's not secure. |
I didn't used any other package for this issue. First when i came here in this discussion i got a solution in @hamedov93 and @binarygeotech comments. But i thought why should i install any extra dependency if there is some logical implementations which can do something for me and it works. i got a solution after identifying the behaviour of passport driver is just about to authenticate token but how i get token for any other Model if i already declared
and in my login api i just used super telented |
Can you give a detailed description of how you did it step by step? Thanks. |
@lucadegasperi I read the changes made in your fork. |
@PeterPan73 sure let me share some steps with you Step 1:
Step 2:
Step 3:
Step 4:
In
for crew apis i did same as i did for contractor apis. if it authenticate the crew he will able to access that particular api. i hope i cleared everything and i think these code snippets are clearly understood |
@billriess @lucadegasperi |
I've taken another stab at implementing this, it seems like some of the tests failed although locally it seemed okay - I'll take a look at those tomorrow. |
For anyone still interested in this feature please take a look at the pr above and provide feedback on it. Thanks. |
I updated the PR, all tests are passing now. |
Closing this issue now that this has been merged into master. |
You all can thank @billriess for getting this in. |
Thanks @billriess amazing job! |
Great work @billriess |
So What should I actually do then. Is it only supported for password grant token ? because provider is |
Can you please also update the docs? I've created 2 guards:
with the following providers:
now when I try to use it:
returns true even if I pass the user token. I've tried to create a new client for Admin model with the artisan command: |
|
@driesvints is it only creates password grant token. cuz in the personal access token provider is still |
You set the provider when you create the oauth_client, it doesn't set the provider afterward. The purpose of this is to make sure only 1 provider can use the oauth_client. If you don't need this functionality you can leave it null. This is for password grants. |
@billriess is it possible to have this functionality for personal access token |
That doesn't work for me. I'm logged in as user but Auth::guard() tells me that I'm an admin...maybe some docs on this. How can we use this to check which guard is logged in on same route handler? |
This is for password grants not personal access tokens. |
@billriess what's the use case for this? |
@billriess will it work with |
how to return |
Since #161 has been locked we are no longer able to discuss the issue. Before I submit a PR I want to make sure we have a common ground on what the expected behavior is for multi-provider support in Passport. More specifically, should the provider be defined at the oauth client level? This would limit that client id/secret pair to always return from a specific provider. If the gate requested another provider than what the client has defined it would return a 401.
This is the functionality I have built into my current project and I would be more than willing to issue a PR for it but I want to make sure that is what everyone is expecting.
EDIT: I should note, that if no provider is set on the oauth client then it will follow the same logic as what is in place now, so it should be backward compatible outside of the migration.
The text was updated successfully, but these errors were encountered: