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

Correct assumption that OIDC access token is always a JWT #8250

Closed
wants to merge 1 commit into from

Conversation

11xor6
Copy link
Member

@11xor6 11xor6 commented Jun 10, 2021

There is an assumption that the OAuth2/OIDC access token is always a JWT, this is not always the case as the OAuth2 and OIDC specifications both specify that it's implementation is undefined.

See: https://datatracker.ietf.org/doc/html/rfc6749#section-1.4 (OIDC defers to the OAuth2 definition of access tokens here: https://openid.net/specs/openid-connect-core-1_0.html#Terminology)

@cla-bot cla-bot bot added the cla-signed label Jun 10, 2021
@11xor6 11xor6 force-pushed the fix-access-token-assumption branch 3 times, most recently from 76d9b06 to 0895543 Compare June 10, 2021 09:57
@11xor6 11xor6 force-pushed the fix-access-token-assumption branch from 0895543 to 299b40c Compare June 11, 2021 00:45
this.service = requireNonNull(service, "service is null");
this.principalField = config.getPrincipalField();
Copy link
Member

Choose a reason for hiding this comment

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

redundant this

{
return jwtParser.parseClaimsJws(jws);
return Optional.ofNullable(jwtParser.parseClaimsJws(token)
Copy link
Member

Choose a reason for hiding this comment

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

I think that moving jwtParser.. part to the next line would create an additional indent and thus improve readability slightly.

@11xor6
Copy link
Member Author

11xor6 commented Aug 3, 2021

Closing this in favor of #8641 which superceeds this.

@11xor6 11xor6 closed this Aug 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants