chore: migrate to golang-jwt v5 and update token error handling #348
Conversation
- Upgrade github.com/golang-jwt/jwt dependency from v4 to v5 - Update token expiration error handling to use new v5 error types - Add logging for tokens with invalid claims - Adjust test assertions to match updated error messages from jwt v5 Signed-off-by: Bo-Yi Wu <[email protected]>
|
Warning Rate limit exceeded@appleboy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 52 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
WalkthroughThe changes update the JWT library dependency from version v4 to v5, revise error handling logic for token expiration and claim validation in the authentication code, and adjust related tests to reflect stricter and more precise error reporting for the Changes
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @appleboy, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request primarily focuses on migrating the project's JWT dependency to the latest major version (golang-jwt/jwt v5). This upgrade necessitated changes to how token expiration and invalid claims are handled, including updating error checks and introducing new logging for better diagnostics. The associated test suite has also been updated to reflect these changes and ensure continued functionality.
Highlights
- Dependency Upgrade: I've upgraded the
github.com/golang-jwt/jwtlibrary from version 4 to version 5. This is a major version bump and introduces significant changes to the API, particularly around error handling. - Error Handling Refinement: I've updated the token expiration error handling logic to align with the new error types introduced in
golang-jwt/jwtv5. This now useserrors.Isto check forjwt.ErrTokenExpiredandjwt.ErrTokenInvalidClaims. - Improved Logging: I've added a new logging statement that will print a message when a token is found to have invalid claims, which should help in debugging authentication issues.
- Test Suite Adjustments: I've modified existing test assertions to reflect the updated error messages and behaviors from the
golang-jwt/jwtv5 library, ensuring the test suite remains accurate and passes with the new dependency.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
auth_jwt.go(2 hunks)auth_jwt_test.go(2 hunks)go.mod(1 hunks)
🔇 Additional comments (6)
auth_jwt_test.go (3)
17-17: Import upgrade looks good.The JWT library import has been correctly updated to v5, aligning with the dependency upgrade.
589-593: Token creation change aligns with v5 behavior.The removal of the explicit
expclaim setting is appropriate. The token will rely on the JWT library's default expiration handling, which is consistent with how v5 processes expired tokens within the MaxRefresh window.
1245-1245: Error message assertion updated for JWT v5.The test correctly updates the expected error message to match the new format returned by JWT v5 library when the
expclaim has an invalid type. The case-insensitive comparison handles potential variations in error message formatting.auth_jwt.go (3)
7-7: Log import added for enhanced error logging.The log import supports the new logging functionality for invalid token claims, which improves debugging capabilities.
14-14: JWT library import upgraded correctly.The import has been properly updated to v5, consistent with the dependency upgrade in go.mod.
649-656: Improved error handling with JWT v5 idioms.The error handling has been correctly updated to use
errors.Iscomparisons against the new JWT v5 error constants:
jwt.ErrTokenExpiredfor expired tokens (allowing refresh within MaxRefresh window)jwt.ErrTokenInvalidClaimsfor invalid claims with enhanced loggingThis approach is more robust and idiomatic than the previous type assertion method, and maintains the same logical behavior while adding better logging for debugging invalid claims.
There was a problem hiding this comment.
Code Review
This pull request migrates the golang-jwt dependency from v4 to v5 and updates the error handling for expired tokens. Identified two areas for improvement: configurable logger in auth_jwt.go and restoring the exp claim in a key test case in auth_jwt_test.go. Also, the error message in TestExpiredField has changed due to the library update, so ensure this new message is stable and won't change with future library updates.
| claims := token.Claims.(jwt.MapClaims) | ||
| claims["identity"] = "admin" | ||
| claims["exp"] = time.Now().Add(-time.Minute).Unix() | ||
| claims["orig_iat"] = time.Now().Add(-time.Hour).Unix() |
There was a problem hiding this comment.
The exp claim was removed from this test, but this test is intended to verify that an expired token can be refreshed if it's within the MaxRefresh window. By removing the exp claim, the token is no longer expired, and the test no longer covers the intended scenario. Please restore the exp claim to ensure the new error handling logic in CheckIfTokenExpire is correctly tested for expired tokens.
| claims["orig_iat"] = time.Now().Add(-time.Hour).Unix() | |
| claims["exp"] = time.Now().Add(-time.Minute).Unix() |
auth_jwt.go
Outdated
| if errors.Is(err, jwt.ErrTokenInvalidClaims) { | ||
| log.Println("Token has invalid claims, exp is invalid") | ||
| return nil, err |
There was a problem hiding this comment.
Consider using a configurable logger instead of the standard log package, to allow users to control logging behavior. Also, suggest logging the error itself for better debugging, since jwt.ErrTokenInvalidClaims can be triggered by various invalid claims.
log.Printf("Token has invalid claims: %v", err)
auth_jwt_test.go
Outdated
| message := gjson.Get(r.Body.String(), "message") | ||
|
|
||
| assert.Equal(t, ErrExpiredToken.Error(), strings.ToLower(message.String())) | ||
| assert.Equal(t, "token has invalid claims: invalid type for claim: exp is invalid", strings.ToLower(message.String())) |
There was a problem hiding this comment.
The error message has changed due to the library update. It's good to update the assertion, but ensure this new message is stable and won't change with future library updates. If the message is not stable, consider asserting on a more general error condition or code.
- Improve handling of JWT exp claim errors, distinguishing between expired, missing, and invalid type cases - Ensure ParseOptions always include WithTimeFunc for correct time validation - Fix typo in exp claim comment - Add a check for missing exp claim for backwards compatibility - Update tests to verify correct error messages and status codes for missing or invalid exp claims - Add new tests for required exp claim and invalid exp format scenarios Signed-off-by: Bo-Yi Wu <[email protected]>
- Remove unused log import - Eliminate redundant error handling for invalid JWT claims in CheckIfTokenExpire Signed-off-by: Bo-Yi Wu <[email protected]>
- Set the "exp" claim to one minute in the past in TestExpiredTokenWithinMaxRefreshOnRefreshHandler to simulate an expired token. Signed-off-by: Bo-Yi Wu <[email protected]>
- Bump github.com/golang-jwt/jwt/v5 dependency version from 5.2.0 to 5.2.3 Signed-off-by: Bo-Yi Wu <[email protected]>
fixed: #332
Co-Author: @gblandinkingland
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Tests
Chores