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

Allow adding more claims to OIDC JWT authentication token #34883

Merged

Conversation

sberyozkin
Copy link
Member

@sberyozkin sberyozkin commented Jul 20, 2023

Replaces #34686.

@lordvlad, can you please check this PR ?

I thought first I'd start cherry-picking from your PR but then decided not to since having a Map as we agreed in #34686 makes code changes different. I thought it would be simpler to recreate a PR due to the merge issues in your original PR

@quarkus-bot
Copy link

quarkus-bot bot commented Jul 20, 2023

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@lordvlad
Copy link
Contributor

This looks really good. Much appreciated

@@ -367,6 +368,11 @@ public static String signJwtWithKey(OidcCommonConfig oidcConfig, String tokenReq
}
}

@SuppressWarnings({ "unchecked", "rawtypes" })
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is only introduced so you don't have to annotate signJwtWithKey with SuppressWarnings, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

@lordvlad Yes, it is guaranteed Quarkus Config will init an empty or non empty Map with string value. When dealing directly with the JWT build api, Map can contains complex claims like arrays or objects

@sberyozkin
Copy link
Member Author

Thanks @lordvlad for your original PR and the discussion which has led us to introducing a map, users should now be able to add whatever simple custom string claims they need. Support for complex claims, non String types can be reviewed as needed

@sberyozkin
Copy link
Member Author

@geoand @gastaldi Can you please have a quick review ? @lordvlad who created the original PR has approved so it should be good to go

@sberyozkin sberyozkin merged commit 14d455e into quarkusio:main Jul 20, 2023
@quarkus-bot quarkus-bot bot added this to the 3.3 - main milestone Jul 20, 2023
@sberyozkin sberyozkin deleted the oidc_credentials_jwt_extra_claims branch July 20, 2023 22:47
@lordvlad
Copy link
Contributor

@sberyozkin quick question, to understand the release process: why was this not included in 3.2.2.Final? Cause the scope for 3.2.2 was already set? Will it be included in 3.2.3?

@gastaldi
Copy link
Contributor

It will be included in 3.3.0.CR1, due in August 9th, see https://github.com/quarkusio/quarkus/wiki/Release-Planning

@sberyozkin
Copy link
Member Author

@lordvlad Sometimes we can request backports for bug fixes - but this one can be considered a new feature, which is why it is planned for 3.3.0.CR1, I hope you'll have no problems migrating

@lordvlad
Copy link
Contributor

lordvlad commented Jul 29, 2023 via email

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

Successfully merging this pull request may close these issues.

3 participants