-
Notifications
You must be signed in to change notification settings - Fork 88
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
Apis with client_credentilas token: bad credentials response. #207
Comments
I traced it down to this change in league/oauth2-server From there the 'sub' in the jwt is the client identifier, which is used as the user identifier. Before this change, the sub was empty, which the OAuth2Authenticator in this library turned into a NullUser. Now the userIdentifier is not 'empty' anymore, so it tries to load the clientIdentifier as a user via the userProvider. I don't think that is correct as the ClientGrant does not identify a user as identified by the userproviders. |
I've been able to reproduce the I'm already working on a fix for this issue. |
I don't think we should override getSubjectIdentifier (#208) as the sub could be the client identifier according to the RFC. Therefore I think #209 is more correct as it handles how the sub should be interpreted. By checking if the sub equals the aud and then prefering the aud, we guard for the possible attack where a users grants client roles while registering with the client id as a username. |
I agree with @RubenKruiswijk, I find the #209 solution more elegant. There is a possible risk if the user id is the same than the oauth2 client id but it looks like pretty negligible |
I aggree with @RubenKruiswijk and @fxbt here. @chalasr could you review this issue and the related PRs? Thanks. |
Thanks everyone for the report, discussion and reproducer. And thanks a lot for the fix @ajgarlag, release coming. |
version: 0.9.0
It was tested on symfony: 7.1, 7.2
On league/oauth2-server-bundle version 0.9.0. if apis are used with client_credentilas token, it always returns "Bad Credentials".
This error doesn't occur in version 0.8.0.
The text was updated successfully, but these errors were encountered: