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

Parse time claims as Number and convert to Long. #759

Merged
merged 1 commit into from
Jan 17, 2024

Conversation

nelsongraca
Copy link
Contributor

When integrating with Jetbrains Hub as an OpenID provider, found that it returns the time fields in the JWT as seconds since epoch and miliseconds in the decimal part.

Example:

"exp": 1712762861.532,
"iat": 1705105035.125,
"auth_time": 1704986861.532,

This was causing a org.jose4j.jwt.MalformedClaimException to be thrown as Double can't be cast to Long at first sight this seems a bug in the OpenID provider, but the spec is vague about the definition.

A JSON numeric value representing the number of seconds from
1970-01-01T00:00:00Z UTC until the specified UTC date/time,
ignoring leap seconds. This is equivalent to the IEEE Std 1003.1,
2013 Edition [POSIX.1] definition "Seconds Since the Epoch", in
which each day is accounted for by exactly 86400 seconds, other
than that non-integer values can be represented. See RFC 3339
[RFC3339] for details regarding date/times in general and UTC in
particular.

There is no specific mention that the value has to be Long.

This PR aims to prevent such cases by getting the Claim as Number and then returning it as a Long

Further information can be found on the Zulip thread: https://quarkusio.zulipchat.com/#narrow/stream/187030-users/topic/The.20value.20of.20the.20'exp'.20claim.20is.20not.20the.20expected.20type

Tests not added yet as I'm unsure how I can write such test.

@gastaldi
Copy link
Contributor

Follow-up to #758

@nelsongraca nelsongraca marked this pull request as ready for review January 14, 2024 15:14
@Skyllarr
Copy link
Collaborator

Skyllarr commented Jan 15, 2024

Tests not added yet as I'm unsure how I can write such test.

@nelsongraca You can take a look at the DefaultJWTCallerPrincipalTest class. You can try to write a test by adding a claim updated_at in the new format

@nelsongraca
Copy link
Contributor Author

@nelsongraca You can take a look at the DefaultJWTCallerPrincipalTest class. You can try to write a test by adding a claim updated_at in the new format

I looked into it but seems like I'll have to write a full JWT JSON to parse, or did I miss anything?

@sberyozkin
Copy link
Contributor

@sberyozkin
Copy link
Contributor

@nelsongraca Ignore my comment please, easier to copy one of the test resources, indeed

@nelsongraca nelsongraca force-pushed the decimal-claims branch 5 times, most recently from b2bc818 to 73aac69 Compare January 16, 2024 15:50
@nelsongraca
Copy link
Contributor Author

@Skyllarr and @sberyozkin added test for some of the time claims, only question is regarding best practice/convention, should I use static time, or dynamically with current time, both ways are in the code (one of the commented out), LMK which should be used.

@sberyozkin
Copy link
Contributor

sberyozkin commented Jan 16, 2024

@nelsongraca Thanks, this is probably not testing the fix the best way.

Rather than modifying this legacy test code, can you please copy DefaultJWTParserTest into say TimeClaimsTest, and then create a dedicated simple JSON with the issuer, upn and the time claims only and have a single test confirming that retrieving iat, exp as individual claims works (there is no dedicated getter for the auth time in the current JsonWebToken interface but you can cast it to long)

@nelsongraca
Copy link
Contributor Author

@sberyozkin yep can do it.

@nelsongraca
Copy link
Contributor Author

Still need to move it to a new class, can't load from JSON though TokenUtils.signClaims will ignore the time claims in the JSON file and either use provided ones in the signClaims method (there's an overloaded version with those).

This is the original reason I'm not using a JSON with the claims, if there's something I'm missing or I'm not clear enough let me know.

@sberyozkin
Copy link
Contributor

@nelsongraca Token verification step can be skipped for the test, since it is about asserting the parser correctly handles the JSON containing such claims, the verification has already happened at this stage. TokenUtils is an MP JWT helper only, it is not great for the general testing support, JWT Build API we ship is much easier and covers all the variations. Though with George's @gastaldi fix such claim values are converted to long but JWT Build API can be used to load an existing JSON too before signing it...

Copy link
Contributor

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

The test can be made simpler, but it covers the fix, thanks @nelsongraca

@sberyozkin sberyozkin merged commit 6320d93 into smallrye:main Jan 17, 2024
5 checks passed
@sberyozkin sberyozkin added this to the 4.5.0 milestone Jan 17, 2024
@nelsongraca
Copy link
Contributor Author

is there a date set when a version with this fix will be released?

@nelsongraca nelsongraca deleted the decimal-claims branch February 12, 2024 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants