feat: adopt OAuth2-style token fields in authentication responses #347
Conversation
- Replace token response fields with OAuth2-style fields: access_token, token_type, expires_in, refresh_token, and scope - Add a test assertion to verify that refresh_token is present and not empty in the response fix #346 Signed-off-by: Bo-Yi Wu <[email protected]>
WalkthroughThe default JSON responses for login and token refresh in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GinJWTMiddleware
participant AuthService
Client->>GinJWTMiddleware: POST /login (credentials)
GinJWTMiddleware->>AuthService: Validate credentials
AuthService-->>GinJWTMiddleware: Success (user info)
GinJWTMiddleware->>GinJWTMiddleware: Obtain refresh token
alt Refresh token success
GinJWTMiddleware-->>Client: JSON { access_token, token_type, expires_in, refresh_token, scope }
else Refresh token failure
GinJWTMiddleware-->>Client: 401 Unauthorized JSON error
end
Client->>GinJWTMiddleware: POST /refresh (refresh_token)
GinJWTMiddleware->>AuthService: Validate refresh_token
AuthService-->>GinJWTMiddleware: Success (new token)
GinJWTMiddleware->>GinJWTMiddleware: Obtain refresh token
alt Refresh token success
GinJWTMiddleware-->>Client: JSON { access_token, token_type, expires_in, refresh_token, scope }
else Refresh token failure
GinJWTMiddleware-->>Client: 401 Unauthorized JSON error
end
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ 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 aims to standardize the authentication and token refresh response formats to align with the OAuth2 specification. By adopting OAuth2-style fields, the API's token delivery becomes more interoperable and adheres to widely recognized industry standards. The changes include modifications to the default response handlers and an updated test to validate the new output structure.
Highlights
- Authentication Response Standardization: The default
LoginResponseandRefreshResponsehandlers inauth_jwt.gohave been updated to return authentication tokens and related information in an OAuth2-compliant format. This includes replacing the previouscode,token, andexpirefields withaccess_token,token_type(set to 'Bearer'),expires_in(calculated in seconds),refresh_token(initially an empty string), andscope(set to 'create'). - Test Coverage Enhancement: A new assertion has been added to the
TestRefreshHandlerRS256function inauth_jwt_test.go. This assertion specifically verifies that therefresh_tokenfield is present and not empty in the refresh response, ensuring the new OAuth2-style output structure is correctly implemented and returned.
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. ↩
There was a problem hiding this comment.
Code Review
This pull request aligns authentication responses with the OAuth2 standard. The default LoginResponse and RefreshResponse provide an empty refresh_token, which contradicts a new test assertion. The scope field is hardcoded, and a test is broken due to an outdated response format.
auth_jwt_test.go
Outdated
| refreshToken := gjson.Get(r.Body.String(), "refresh_token") | ||
| assert.Equal(t, "refresh successfully", message.String()) | ||
| assert.Equal(t, http.StatusOK, r.Code) | ||
| assert.Equal(t, makeTokenString("RS256", "admin"), cookie.String()) | ||
| assert.NotEmpty(t, refreshToken.String(), "refresh_token should not be empty") |
There was a problem hiding this comment.
The TestRefreshHandlerRS256 test case uses a custom RefreshResponse function (defined on lines 485-498) which returns the old response format and does not include the refresh_token field. As a result, the new assertion assert.NotEmpty() will fail. Either remove the custom RefreshResponse or update it to return the new OAuth2-style fields.
auth_jwt.go
Outdated
| "refresh_token": "", | ||
| "scope": "create", |
There was a problem hiding this comment.
The refresh_token is hardcoded to an empty string, but the new test in auth_jwt_test.go asserts that it should not be empty. This will cause tests to fail. Consider setting refresh_token to the value of token to align with the tests.
| "refresh_token": "", | |
| "scope": "create", | |
| "refresh_token": token, |
auth_jwt.go
Outdated
| "refresh_token": "", | ||
| "scope": "create", |
There was a problem hiding this comment.
The refresh_token is hardcoded to an empty string, which will cause the new test assertion to fail. Consider setting it to the token value.
| "refresh_token": "", | |
| "scope": "create", | |
| "refresh_token": token, |
| "token_type": "Bearer", | ||
| "expires_in": int(time.Until(expire).Seconds()), | ||
| "refresh_token": "", | ||
| "scope": "create", |
There was a problem hiding this comment.
The scope is hardcoded to "create". Since scopes are not configurable, it's better to omit this optional field.
| "token_type": "Bearer", | ||
| "expires_in": int(time.Until(expire).Seconds()), | ||
| "refresh_token": "", | ||
| "scope": "create", |
There was a problem hiding this comment.
The scope is hardcoded to "create". It's better to omit this optional field since scopes are not a supported feature.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| if mw.RefreshResponse == nil { | ||
| mw.RefreshResponse = func(c *gin.Context, code int, token string, expire time.Time) { | ||
| c.JSON(http.StatusOK, gin.H{ | ||
| "code": http.StatusOK, | ||
| "token": token, | ||
| "expire": expire.Format(time.RFC3339), | ||
| "access_token": token, | ||
| "token_type": "Bearer", | ||
| "expires_in": int(time.Until(expire).Seconds()), | ||
| "refresh_token": "", | ||
| "scope": "create", | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Apply the same fixes to RefreshResponse for consistency.
The RefreshResponse function has the same issues as LoginResponse. For consistency and correctness, apply the same fixes here.
Apply this diff to maintain consistency:
if mw.RefreshResponse == nil {
mw.RefreshResponse = func(c *gin.Context, code int, token string, expire time.Time) {
c.JSON(http.StatusOK, gin.H{
"access_token": token,
"token_type": "Bearer",
- "expires_in": int(time.Until(expire).Seconds()),
- "refresh_token": "",
- "scope": "create",
+ "expires_in": int(expire.Sub(mw.TimeFunc()).Seconds()),
+ "refresh_token": token,
+ "scope": "read write",
})
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if mw.RefreshResponse == nil { | |
| mw.RefreshResponse = func(c *gin.Context, code int, token string, expire time.Time) { | |
| c.JSON(http.StatusOK, gin.H{ | |
| "code": http.StatusOK, | |
| "token": token, | |
| "expire": expire.Format(time.RFC3339), | |
| "access_token": token, | |
| "token_type": "Bearer", | |
| "expires_in": int(time.Until(expire).Seconds()), | |
| "refresh_token": "", | |
| "scope": "create", | |
| }) | |
| } | |
| } | |
| if mw.RefreshResponse == nil { | |
| mw.RefreshResponse = func(c *gin.Context, code int, token string, expire time.Time) { | |
| c.JSON(http.StatusOK, gin.H{ | |
| "access_token": token, | |
| "token_type": "Bearer", | |
| "expires_in": int(expire.Sub(mw.TimeFunc()).Seconds()), | |
| "refresh_token": token, | |
| "scope": "read write", | |
| }) | |
| } | |
| } |
🤖 Prompt for AI Agents
In auth_jwt.go around lines 377 to 387, the RefreshResponse function has the
same issues as LoginResponse regarding response structure and data handling.
Update RefreshResponse to match the fixed LoginResponse implementation by
ensuring the response fields are consistent, correctly formatted, and include
all necessary information such as proper token details and expiration handling.
This will maintain consistency and correctness across both response handlers.
| if mw.LoginResponse == nil { | ||
| mw.LoginResponse = func(c *gin.Context, code int, token string, expire time.Time) { | ||
| c.JSON(http.StatusOK, gin.H{ | ||
| "code": http.StatusOK, | ||
| "token": token, | ||
| "expire": expire.Format(time.RFC3339), | ||
| "access_token": token, | ||
| "token_type": "Bearer", | ||
| "expires_in": int(time.Until(expire).Seconds()), | ||
| "refresh_token": "", | ||
| "scope": "create", | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Address inconsistency with refresh_token field and improve scope handling.
The OAuth2 adoption looks good, but there are several issues to address:
- Inconsistent refresh_token: The field is set to empty string, but the test expects it to be non-empty
- Hardcoded scope: The scope is hardcoded to "create" which may not be appropriate for all use cases
- Potential negative expires_in: Using
time.Until(expire).Seconds()could return negative values if expire time is in the past
Apply this diff to fix the refresh_token issue and improve scope handling:
if mw.LoginResponse == nil {
mw.LoginResponse = func(c *gin.Context, code int, token string, expire time.Time) {
c.JSON(http.StatusOK, gin.H{
"access_token": token,
"token_type": "Bearer",
- "expires_in": int(time.Until(expire).Seconds()),
- "refresh_token": "",
- "scope": "create",
+ "expires_in": int(expire.Sub(mw.TimeFunc()).Seconds()),
+ "refresh_token": token,
+ "scope": "read write",
})
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if mw.LoginResponse == nil { | |
| mw.LoginResponse = func(c *gin.Context, code int, token string, expire time.Time) { | |
| c.JSON(http.StatusOK, gin.H{ | |
| "code": http.StatusOK, | |
| "token": token, | |
| "expire": expire.Format(time.RFC3339), | |
| "access_token": token, | |
| "token_type": "Bearer", | |
| "expires_in": int(time.Until(expire).Seconds()), | |
| "refresh_token": "", | |
| "scope": "create", | |
| }) | |
| } | |
| } | |
| if mw.LoginResponse == nil { | |
| mw.LoginResponse = func(c *gin.Context, code int, token string, expire time.Time) { | |
| c.JSON(http.StatusOK, gin.H{ | |
| "access_token": token, | |
| "token_type": "Bearer", | |
| "expires_in": int(expire.Sub(mw.TimeFunc()).Seconds()), | |
| "refresh_token": token, | |
| "scope": "read write", | |
| }) | |
| } | |
| } |
🤖 Prompt for AI Agents
In auth_jwt.go around lines 357 to 367, the refresh_token field is set to an
empty string but should be populated with a valid token to meet test
expectations. Replace the empty string with the actual refresh token value.
Also, avoid hardcoding the scope by dynamically setting it based on the user's
granted scopes or context. Finally, ensure expires_in never returns a negative
value by clamping it to zero if the calculated duration is negative.
auth_jwt_test.go
Outdated
| refreshToken := gjson.Get(r.Body.String(), "refresh_token") | ||
| assert.Equal(t, "refresh successfully", message.String()) | ||
| assert.Equal(t, http.StatusOK, r.Code) | ||
| assert.Equal(t, makeTokenString("RS256", "admin"), cookie.String()) | ||
| assert.NotEmpty(t, refreshToken.String(), "refresh_token should not be empty") | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent test expectation with implementation.
The test expects refresh_token to be non-empty, but the implementation in auth_jwt.go sets it to an empty string. This test will fail.
After fixing the implementation, consider expanding the test to verify all OAuth2 fields:
Run(handler, func(r gofight.HTTPResponse, rq gofight.HTTPRequest) {
message := gjson.Get(r.Body.String(), "message")
cookie := gjson.Get(r.Body.String(), "cookie")
refreshToken := gjson.Get(r.Body.String(), "refresh_token")
+ accessToken := gjson.Get(r.Body.String(), "access_token")
+ tokenType := gjson.Get(r.Body.String(), "token_type")
+ expiresIn := gjson.Get(r.Body.String(), "expires_in")
+ scope := gjson.Get(r.Body.String(), "scope")
+
assert.Equal(t, "refresh successfully", message.String())
assert.Equal(t, http.StatusOK, r.Code)
assert.Equal(t, makeTokenString("RS256", "admin"), cookie.String())
assert.NotEmpty(t, refreshToken.String(), "refresh_token should not be empty")
+ assert.NotEmpty(t, accessToken.String(), "access_token should not be empty")
+ assert.Equal(t, "Bearer", tokenType.String(), "token_type should be Bearer")
+ assert.True(t, expiresIn.Int() > 0, "expires_in should be positive")
+ assert.NotEmpty(t, scope.String(), "scope should not be empty")
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| refreshToken := gjson.Get(r.Body.String(), "refresh_token") | |
| assert.Equal(t, "refresh successfully", message.String()) | |
| assert.Equal(t, http.StatusOK, r.Code) | |
| assert.Equal(t, makeTokenString("RS256", "admin"), cookie.String()) | |
| assert.NotEmpty(t, refreshToken.String(), "refresh_token should not be empty") | |
| }) | |
| Run(handler, func(r gofight.HTTPResponse, rq gofight.HTTPRequest) { | |
| message := gjson.Get(r.Body.String(), "message") | |
| cookie := gjson.Get(r.Body.String(), "cookie") | |
| refreshToken := gjson.Get(r.Body.String(), "refresh_token") | |
| accessToken := gjson.Get(r.Body.String(), "access_token") | |
| tokenType := gjson.Get(r.Body.String(), "token_type") | |
| expiresIn := gjson.Get(r.Body.String(), "expires_in") | |
| scope := gjson.Get(r.Body.String(), "scope") | |
| assert.Equal(t, "refresh successfully", message.String()) | |
| assert.Equal(t, http.StatusOK, r.Code) | |
| assert.Equal(t, makeTokenString("RS256", "admin"), cookie.String()) | |
| assert.NotEmpty(t, refreshToken.String(), "refresh_token should not be empty") | |
| assert.NotEmpty(t, accessToken.String(), "access_token should not be empty") | |
| assert.Equal(t, "Bearer", tokenType.String(), "token_type should be Bearer") | |
| assert.True(t, expiresIn.Int() > 0, "expires_in should be positive") | |
| assert.NotEmpty(t, scope.String(), "scope should not be empty") | |
| }) |
🤖 Prompt for AI Agents
In auth_jwt_test.go around lines 530 to 535, the test expects the
"refresh_token" field to be non-empty, but the current implementation sets it to
an empty string causing the test to fail. Update the implementation in
auth_jwt.go to generate and set a valid non-empty refresh token string. After
that, enhance the test to verify all relevant OAuth2 response fields for
completeness.
- Add error handling and response for refresh token generation failures during middleware initialization - Return the actual refresh token in the response instead of an empty string - Update test claims to include standard JWT fields with current timestamps - Change test code to extract access_token instead of token from responses - Remove redundant test assertions for refresh_token presence Signed-off-by: Bo-Yi Wu <[email protected]>
fix #346
Summary by CodeRabbit
New Features
Bug Fixes