[jwt] add refresh tokens support - #19
Merged
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 56 minutes and 1 second. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughThis PR implements a complete JWT refresh token system spanning server and client. The backend introduces refresh token persistence via a new database table, a Service API for token generation/validation/revocation/rotation, and new ChangesJWT Refresh Token Lifecycle
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant API
participant Service
participant Repository
User->>Frontend: Login
Frontend->>API: POST /auth/login
API->>Service: GenerateTokenPair(user)
Service->>Repository: Create(userID, tokenHash, expiresAt)
Service-->>API: accessToken, refreshToken
API-->>Frontend: LoginResponse
Frontend->>Frontend: Store tokens in localStorage
User->>Frontend: Make authenticated request
Frontend->>API: Fetch with accessToken
API-->>Frontend: 401 Unauthorized (token expired)
Frontend->>API: POST /auth/refresh with refreshToken
API->>Service: ValidateRefreshToken(rawToken)
Service->>Repository: FindByHash(tokenHash)
Service->>Service: RotateTokenPair(oldToken, user)
Service->>Repository: RevokeByHash(oldHash)
Service->>Repository: Create(userID, newTokenHash, newExpiry)
Service-->>API: newAccessToken, newRefreshToken
API-->>Frontend: RefreshResponse
Frontend->>Frontend: Update tokens in localStorage
Frontend->>API: Retry original request with newAccessToken
API-->>Frontend: 200 OK
User->>Frontend: Logout
Frontend->>API: POST /auth/logout with refreshToken
API->>Service: RevokeRefreshToken(rawToken)
Service->>Repository: RevokeByHash(tokenHash)
API-->>Frontend: 204 No Content
Frontend->>Frontend: Clear localStorage tokens
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
🤖 Pull request artifacts
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
internal/jwt/service.go (1)
153-160: ⚖️ Poor tradeoffConsider making token rotation atomic.
If
RevokeRefreshTokensucceeds butGenerateRefreshTokenfails (e.g., database write error), the user loses their refresh token with no recovery path except re-authentication. Wrapping both operations in a database transaction would ensure the old token is only revoked if the new one is successfully created.This is acceptable for now since users can re-login, but could cause poor UX under database instability.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/jwt/service.go` around lines 153 - 160, RotateRefreshToken currently calls RevokeRefreshToken then GenerateRefreshToken separately, which can leave the user without a refresh token if generation fails; change RotateRefreshToken to perform both operations inside a single database transaction (use the Service's DB/transaction helper) so that you begin a tx, call revoke and generate using the transaction-aware variants (or pass the tx/context into RevokeRefreshToken and GenerateRefreshToken or inline their DB operations), commit the tx only after GenerateRefreshToken succeeds, and rollback on any error; ensure errors from Begin/Commit/Rollback are handled and propagated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/db/migrations/20260527032544_refresh_tokens.sql`:
- Line 16: Remove the stray standalone '---' line from the migration file
20260527032544_refresh_tokens.sql: open the migration SQL and delete the lone
line containing only '---' so it is not emitted to the DB as SQL; ensure the
rest of the migration content (up/down sections or SQL statements) remains
unchanged and the file still conforms to Goose migration formatting.
In `@internal/jwt/repository.go`:
- Around line 44-57: RevokeByHash should only match tokens that are not already
revoked to avoid race replay; change the update Where clause to include revoked
= false (e.g. .Where("token_hash = ? AND revoked = ?", tokenHash, false)) so the
UPDATE only affects unrevoked rows, keep the existing Set("revoked = ?", true)
and the RowsAffected check, and return ErrRefreshTokenRevoked when RowsAffected
== 0 as before.
In `@internal/server/auth/handler.go`:
- Around line 99-135: Add Swagger annotations to expose the new endpoints by
adding comment blocks above the handler functions: place `@Summary`, `@Description`
(optional), `@Tags` (e.g., Auth), `@Accept` json, `@Produce` json, `@Param` for the
request bodies, `@Success` responses (e.g., 200 for handleRefresh returning
RefreshResponse and 204 for handleLogout), and `@Router` entries for the paths
/auth/refresh [post] and /auth/logout [post]; update the comment blocks located
immediately above the handleRefresh and handleLogout function declarations so
the generator includes these endpoints in the API docs.
- Around line 114-117: The handler currently treats all errors from
h.jwtSvc.RotateTokenPair(...) as internal errors; change it to detect token-auth
errors (e.g., invalid or revoked refresh token) using errors.Is / errors.As
against the JWT service's sentinel errors (such as ErrInvalidRefreshToken or
ErrTokenRevoked / jwt package sentinel errors) and return an HTTP 401
unauthorized result instead of a 500; keep using the same RotateTokenPair call
and variables (accessToken, refreshToken, err) but branch on the error type and
return a mapped 401 "unauthorized" error/response when the error indicates a bad
refresh token, otherwise preserve the existing error handling for other
failures.
In `@requests.http`:
- Around line 29-44: The logout request is still using the original
adminRefreshToken; update it to use the rotated refresh token returned by the
refresh call (the same pattern used for adminAccessToken). Replace the body
value for "refresh_token" in the POST /auth/logout request to reference the
refresh token from the refresh response (e.g., the response variable used for
adminAccessToken, such as adminRefresh.response.body.$.refresh_token) so the
active rotated token is revoked. Ensure any environment variable or shorthand
like adminRefreshToken is reassigned to that response value if other requests
rely on it.
In `@web/static/js/api.js`:
- Around line 19-45: The refreshAccessToken function leaves the module-level
_refreshPromise set after a successful refresh, causing future callers to reuse
a stale resolved promise; update refreshAccessToken so that _refreshPromise is
cleared once the refresh settles successfully (complementing the existing .catch
handler) — for example, inside the inner async IIFE (the Promise assigned to
_refreshPromise) capture the parsed data/access_token into a local, persist
tokens, then set _refreshPromise = null before returning the access token (or
use a try/finally around the await/logic to ensure _refreshPromise is nulled on
both success and failure).
In `@web/static/js/app.js`:
- Around line 116-131: The logout() method calls getRefreshToken() which is not
defined in this scope; fix by either (A) reading the refresh token directly from
localStorage inside logout() (e.g. get the "refresh_token" value and use that as
rt before calling window.apiFetch("/auth/logout", ...)), or (B) export or attach
getRefreshToken from api.js so it is available where logout() runs (make
getRefreshToken accessible and then call that exported/attached function).
Update the reference in logout() (and keep the try/catch around the apiFetch) so
there is no direct call to an undefined identifier.
---
Nitpick comments:
In `@internal/jwt/service.go`:
- Around line 153-160: RotateRefreshToken currently calls RevokeRefreshToken
then GenerateRefreshToken separately, which can leave the user without a refresh
token if generation fails; change RotateRefreshToken to perform both operations
inside a single database transaction (use the Service's DB/transaction helper)
so that you begin a tx, call revoke and generate using the transaction-aware
variants (or pass the tx/context into RevokeRefreshToken and
GenerateRefreshToken or inline their DB operations), commit the tx only after
GenerateRefreshToken succeeds, and rollback on any error; ensure errors from
Begin/Commit/Rollback are handled and propagated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e3038b4-832d-415a-a0a2-6ec13dcaee61
📒 Files selected for processing (17)
internal/config/config.gointernal/config/module.gointernal/db/migrations/20260527032544_refresh_tokens.sqlinternal/jwt/config.gointernal/jwt/domain.gointernal/jwt/errors.gointernal/jwt/models.gointernal/jwt/module.gointernal/jwt/repository.gointernal/jwt/service.gointernal/server/auth/dto.gointernal/server/auth/handler.gointernal/server/docs/docs.gointernal/server/middlewares/jwtauth/jwtauth.gorequests.httpweb/static/js/api.jsweb/static/js/app.js
capcom6
force-pushed
the
jwt/refresh-token
branch
from
May 28, 2026 03:07
8eca748 to
cb60adc
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
/auth/refreshendpoint to obtain new access tokens/auth/logoutendpoint to revoke refresh tokens