[IS 5.11] Fix bugs related to private key JWT when prevent token reuse is disable related flows#165
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes multiple bugs related to private key JWT authentication when the "prevent token reuse" option is disabled. The changes address three distinct issues: a reversed comparison operator in the JTI validity period check, incorrect JDBC method usage losing date precision, and unnecessary/redundant database calls in the JTI validation flow.
Changes:
- Fixed the
checkJTIValidityPeriodcomparison logic inJWTValidatorwhich was inverted (allowing expired tokens and rejecting valid ones), and refactoredvalidateJTIto eliminate redundant DB calls and correctly place thepersistJWTIDcall only for new JTI entries. - Changed
rs.getTime()tors.getTimestamp()inJWTStorageManager.getJwtFromDB()to correctly read full datetime values instead of only the time portion. - Updated the test expectation for JWT replay with
preventTokenReuse=falseto expecttrue(pass) when the token is still within its validity period, consistent with the logic fix.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
component/client-handler/src/main/java/.../JWTValidator.java |
Fixed inverted comparison in checkJTIValidityPeriod, refactored validateJTI to remove redundant validateJWTInDataBase method, moved persistJWTID to only persist for new entries, and corrected error messages |
component/client-handler/src/main/java/.../JWTStorageManager.java |
Changed getTime() to getTimestamp() for correct datetime retrieval from DB |
component/client-handler/src/test/java/.../JWTValidatorTest.java |
Updated test expectation for JWT replay with preventTokenReuse=false from false to true to match corrected logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...ava/org/wso2/carbon/identity/oauth2/token/handler/clientauth/jwt/validator/JWTValidator.java
Outdated
Show resolved
Hide resolved
...org/wso2/carbon/identity/oauth2/token/handler/clientauth/jwt/validator/JWTValidatorTest.java
Show resolved
Hide resolved
…y/oauth2/token/handler/clientauth/jwt/validator/JWTValidator.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| long timeStampSkewMillis) throws OAuthClientAuthnException { | ||
|
|
||
| if (currentTimeInMillis + timeStampSkewMillis > jwtExpiryTimeMillis) { | ||
| if (currentTimeInMillis + timeStampSkewMillis < jwtExpiryTimeMillis) { |
There was a problem hiding this comment.
The improvement referenced from the master fix
https://github.com/wso2-extensions/identity-oauth-addons/pull/103/changes#diff-c1fefc87f2cf0485b2f5e352367c515cb1036459a5232698108ff48d4d50a5e1R297
There was a problem hiding this comment.
When JWT token reuse is enabled, The token should be able to reuse before the expiry, not after the expiry. The previous logic is incorrect.
| if (rs.next()) { | ||
| long exp = rs.getTime(1, Calendar.getInstance(TimeZone.getTimeZone(Constants.UTC))).getTime(); | ||
| long created = rs.getTime(2, Calendar.getInstance(TimeZone.getTimeZone(Constants.UTC))).getTime(); | ||
| long exp = rs.getTimestamp(1, Calendar.getInstance(TimeZone.getTimeZone(Constants.UTC))).getTime(); |
There was a problem hiding this comment.
The change reference from https://github.com/wso2-extensions/identity-oauth-addons/pull/103/changes#diff-8197856955fc0b0fa42d9961e51a93ed883a2617dad9c0689828764a6b2c3010R135
Without this change the error response doesn't have the epoch time correctly.
Related Issues
preventTokenReuse=falsefunctionality broken due to expiry enforcement logic and DB constraint violations wso2/product-is#26705The suggested improvement is a behaviour change as explained
Previously, the documentation stated that a JWT could be reused after its expiry. However, this is incorrect. A JWT must be reused before the token expires.
Even if a client attempts to reuse a JWT after it has expired, the request will fail because the corresponding JTI is already registered in the IDN_OIDC_JTI table. Therefore, the previously documented behavior would not have worked in practice.
Additionally, there is a JTI cleanup script that periodically removes expired JTIs from the IDN_OIDC_JTI table. Once these entries are cleaned, the previously used JTIs are removed from the table. However, this still does not allow the reuse of the JWT, since the token itself is already expired and will be rejected during validation.
Therefore, regardless of the cleanup process, an expired JWT cannot be used again. This confirms that the previously documented flow was not functionally valid. As a result, the proposed improvement can be introduced without requiring any additional configuration, since it aligns with the actual behavior of the system.
Purpose
This pull request refactors the JWT replay and expiry validation logic to improve correctness and clarity. The main changes include adjustments to how JWT expiry and creation times are retrieved from the database, updates to the replay prevention logic, and corrections to the expiry validation condition. Test cases have also been updated to reflect the new logic.
JWT Replay and Expiry Validation Improvements:
JWTStorageManager.javafromgetTimetogetTimestampto ensure accurate time handling for JWT entries.JWTValidator.java:validateJWTInDataBasemethod and integrating its logic directly.checkJTIValidityPeriodto use<instead of>, ensuring that JWTs are only considered valid within their allowed expiry time.Test Updates:
JWTValidatorTest.javato align with the new validation logic, ensuring tests accurately reflect the expected outcomes for JWT replay and expiry scenarios.