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

Apis with client_credentilas token: bad credentials response. #207

Closed
facundososalopez opened this issue Jan 17, 2025 · 7 comments · Fixed by #209
Closed

Apis with client_credentilas token: bad credentials response. #207

facundososalopez opened this issue Jan 17, 2025 · 7 comments · Fixed by #209

Comments

@facundososalopez
Copy link

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.

@RubenKruiswijk
Copy link

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.

@ajgarlag
Copy link
Contributor

I've been able to reproduce the Bad credentials problem using the reproducer app provided by @fxbt in #191 (comment)

I'm already working on a fix for this issue.

@ajgarlag
Copy link
Contributor

I've opened 2 different PRs to fix the issue: #208 and #209. Let me know which one you prefer, and why.

@RubenKruiswijk
Copy link

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.

@fxbt
Copy link

fxbt commented Jan 22, 2025

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

@ajgarlag
Copy link
Contributor

ajgarlag commented Jan 30, 2025

I aggree with @RubenKruiswijk and @fxbt here.

@chalasr could you review this issue and the related PRs? Thanks.

@chalasr chalasr closed this as completed in 161ba05 Feb 1, 2025
@chalasr
Copy link
Member

chalasr commented Feb 1, 2025

Thanks everyone for the report, discussion and reproducer. And thanks a lot for the fix @ajgarlag, release coming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants