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.

Sign up for GitHub

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

Jump to bottom

OIDC make token iat claim optional via config property #39249

Merged
merged 1 commit into from
Mar 8, 2024
Merged

OIDC make token iat claim optional via config property #39249

merged 1 commit into from
Mar 8, 2024

Conversation

wesleysalimansdvb
Copy link
Contributor

@wesleysalimansdvb wesleysalimansdvb commented Mar 7, 2024 •
edited by geoand
Loading

@quarkus-bot quarkus-bot bot added the area/oidc label Mar 7, 2024
@michalvavrik michalvavrik requested a review from sberyozkin March 7, 2024 10:55
sberyozkin
sberyozkin reviewed Mar 7, 2024
View reviewed changes
Copy link
Member

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

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

@wesleysalimansdvb Thanks for the PR, I'd like to propose to have it named everywhere as issuedAtRequired and also document that this property will be ignored for the ID token verification - in OidcIdentityProvider there must be a false value passed when ID token is being verified.

Also, please update OidcRecorder to throw a configuration exception if this property is true and the quarkus.oidc.token-age is also true.

Thanks

@wesleysalimansdvb
Copy link
Contributor Author

@sberyozkin I processed your feedback :)

@wesleysalimansdvb
Copy link
Contributor Author

@sberyozkin thanks for the good feedback, I learned a lot by contributing to the quarkus codebase!

sberyozkin
sberyozkin approved these changes Mar 7, 2024
View reviewed changes
Copy link
Member

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

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

Thanks @wesleysalimansdvb, I'll suggest a few more tiny updates, and we'll merge, please squash the commits if you don't mind

sberyozkin
sberyozkin approved these changes Mar 7, 2024
View reviewed changes
Copy link
Member

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

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

@wesleysalimansdvb Very last (I promise :-)) 3 suggestions are proposed, thanks

@wesleysalimansdvb
oidc make token age optional via config property
4d825af 
update configuration exception and rename variable

Rewording documentation and always check id token iat

oidc rename variable name throw configuration property error
@wesleysalimansdvb wesleysalimansdvb force-pushed the 39215-oidc-make-age-optional branch from 8b9f85c to 4d825af Compare March 8, 2024 12:42
@wesleysalimansdvb
Copy link
Contributor Author

wesleysalimansdvb commented Mar 8, 2024 •
edited
Loading

@sberyozkin Squashed the commits in to 1 commit, I hope everything is alright like this. If not, I'll see :p

Thanks!

@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Mar 8, 2024 •
edited by github-actions bot
Loading

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 4d825af.

✅ 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.

You can consult the Develocity build scans.

@sberyozkin sberyozkin merged commit fb3c0b8 into quarkusio:main Mar 8, 2024
22 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.9 - main milestone Mar 8, 2024
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Mar 8, 2024
@sberyozkin
Copy link
Member

Proposing a backport to 3.8 following a request from @madocx at #39887, as the changes do not seem to be sensitive

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sberyozkin sberyozkin sberyozkin approved these changes

Assignees
No one assigned
Labels
area/oidc kind/enhancement New feature or request triage/backport-3.8
Projects
None yet
Milestone
3.9.0.CR1
Development

Successfully merging this pull request may close these issues.

No way to configure iat claim as optional in quarkus-oidc
2 participants
@wesleysalimansdvb @sberyozkin