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
Client Grant requires users table #869
Comments
I'm not really sure how this can happen. A client credentials grant shouldn't attempt to retrieve a user in the first place I believe? Or am I missing something? |
@driesvints Well it shouldn't, but here is the catch. Laravel requires the I'll make a PR, hope that is OK with you? |
I'm a bit concerned with edge cases and what this will break in people's setups. I think we need to investigate this more first. |
I don't think it will break anything, because when we use the Client Grant there is no In Client Grant scenario we are only concerned if the token is valid, e.g. if it has not expired or if it has not been revoked etc. When we use other scenarios, the Then again, I might have jumped the gun, I was just eager to make my first open source contribution :D I hope I am right and it gets merged :) |
You gotta realize that this method is used for all grant types not just client credentials. |
Yes, I am aware of that, but in all other cases the In the case of Client Grant This method returns an instance of the AccessTokenRepository::getNewToken(ClientEntityInterface $clientEntity, array $scopes, $userIdentifier = null) As you can see the
trait AccessTokenTrait
{
/**
* Generate a JWT from the access token
*
* @param CryptKey $privateKey
*
* @return Token
*/
public function convertToJWT(CryptKey $privateKey)
{
return (new Builder())
->setAudience($this->getClient()->getIdentifier())
->setId($this->getIdentifier(), true)
->setIssuedAt(time())
->setNotBefore(time())
->setExpiration($this->getExpiryDateTime()->getTimestamp())
->setSubject($this->getUserIdentifier())
->set('scopes', $this->getScopes())
->sign(new Sha256(), new Key($privateKey->getKeyPath(), $privateKey->getPassPhrase()))
->getToken();
}
} That's the flow in a nutshell when we create tokens. When we are authenticating the token in the // Return the request with additional attributes
return $request
->withAttribute('oauth_access_token_id', $token->getClaim('jti'))
->withAttribute('oauth_client_id', $token->getClaim('aud'))
->withAttribute('oauth_user_id', $token->getClaim('sub'))
->withAttribute('oauth_scopes', $token->getClaim('scopes')); In Client Grant the $user = $this->provider->retrieveById(
$psr->getAttribute('oauth_user_id')
);
if (! $user) {
return;
} To this:
In all other grant types we will always get the user id since it is the I hope I made it more clear. :) |
I still think this won't work. This way people can send tokens without the sub claim and pass authentication which could lead to security issues. Btw there's a call to the @Sephster do you have any insights here? |
I don't think so because that would result in different hashes and this method would fail
|
@driesvints Totally missed the if (! $userId = $psr->getAttribute('oauth_user_id')) {
return;
}
$user = $this->provider->retrieveById($userId);
if (! $user) {
return;
} This way the Client Grant works without the |
I still don't know if this is the best approach. Don't you need to provide full compatibility with OAuth2 which means providing support for all OAuth2 grants? You still need a users table for the other grants. If you leave it away the other grant types won't work anymore? |
Hi @driesvints - apologies for the delay in responding. As you have intimated, the sub must be set to be compliant with OAuth specs. For the majority of grants, it is usually set to the resource owner which would be the user ID in most cases. However, the client credentials grant is the only exception to this rule. As there is no resource owner, the sub should be set to the client ID instead. It should never be set to null. @sasa-b is right that we shouldn't need to check for a user ID but the assertion that the sub will be null isn't correct. I hope this helps. If I get a chance over the weekend I will see if I can find a quick win solution to this if nothing has been resolved. Happy to help however I can. Cheers |
@Sephster thanks for replying! It seems to me that we first need to retrieve the client, check if it's a client credentials grant and if so just skip the user check? In other cases we'll do need to check the user. |
Hi @driesvints, I am wondering if the issue was introduced as part of this commit: 404b345 I have not had time to test it yet, but I have been able to use the client credentials grant in the past without requiring a user table, but after starting a new project I have now discovered the issue. I can run a few more tests tomorrow in the hope of identifying the issue. |
After a few tests, I don't believe it is due to the commit mentioned above. I will continue digging into it and hopefully find why the issue has suddenly started happening. |
@Sephster Are you sure the return $request
->withAttribute('oauth_access_token_id', $token->getClaim('jti'))
->withAttribute('oauth_client_id', $token->getClaim('aud'))
->withAttribute('oauth_user_id', $token->getClaim('sub'))
->withAttribute('oauth_scopes', $token->getClaim('scopes')); There's already an |
@sasa-b @michael-sbs I've tried digging into this issue but I've come to the conclusion that you shouldn't be using the |
Hi @driesvints, sorry, in work just now so posting this quickly but I think this is correct. There has been dome debate over our usage of the aud claim. It is most likely incorrectly setting it to the client ID when it should instead be setting it to the resource server that would be consuming the JWT. There is a discussion open for it here thephpleague/oauth2-server#857 |
@Sephster I see. Thanks and keep us posted :) |
@sasa-b @michael-sbs going to close this off then. Feel free to respond if you ever find time. |
Currently, the Client Grant implementation requires the presence of the
users
table, otherwise if the table is not present and we try to authorize we get the following exception:Since the Client Grant should be used for machine to machine communication the
users
table should not be mandatory.The issue happens in the
Laravel\Passport\Guards\TokenGuard
:The provider tries to fetch the user and everything blows up. We could easily circumvent this with an additional if statement, namely, if a
user_id/oauth_user_id
is null in theoauth_tokens
table, don't try to fetch the user. Something like this:I'd be happy to make a pull request if that's OK with the maintainers?
The text was updated successfully, but these errors were encountered: