[Refactor] #59 Refresh Token 로테이션 적용#61
Conversation
📝 WalkthroughWalkthroughThe PR modifies the authentication token refresh mechanism to rotate both access and refresh tokens instead of just the access token. A new test fixture is added, and comprehensive test coverage is introduced for the updated token refresh functionality. Changes
Sequence DiagramsequenceDiagram
participant Test as Test/Client
participant AuthUsecase as AuthUsecase
participant TokenGen as Token Generator
participant AuthEntity as Auth Entity
participant AuthRepo as Auth Repository
Test->>AuthUsecase: reissueAccessToken(refreshToken)
AuthUsecase->>AuthUsecase: validateRefreshToken()
alt Token Valid
AuthUsecase->>TokenGen: generateAccessToken()
TokenGen-->>AuthUsecase: newAccessToken
AuthUsecase->>TokenGen: generateRefreshToken()
TokenGen-->>AuthUsecase: newRefreshToken
AuthUsecase->>AuthEntity: updateTokens(newAccessToken, newRefreshToken)
AuthEntity-->>AuthUsecase: updated
AuthUsecase->>AuthRepo: save(authEntity)
AuthRepo-->>AuthUsecase: saved
AuthUsecase-->>Test: TokenResponse
else Token Invalid/Expired
AuthUsecase-->>Test: CommonException/JwtAuthenticationException
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![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
🧹 Nitpick comments (2)
src/main/java/tave/crezipsa/crezipsa/application/auth/usecase/AuthUsecaseImpl.java (2)
35-37: Rotation is implemented correctly; consider adding refresh token reuse detection.The core rotation logic is sound — both tokens are regenerated and persisted atomically inside the existing
@Transactionalboundary. No issues with the change itself.One security gap worth tracking as a follow-up: the current implementation provides no token-reuse detection. The typical attack scenario is: a refresh token is stolen, the attacker exchanges it for a new one before the legitimate user does, the server issues a new refresh token to the attacker and invalidates the old one — the legitimate user is then locked out. Without reuse detection, this scenario enables an attacker to maintain persistence while the degraded UX is attributed to an application bug rather than active compromise.
Automatic reuse detection works by having the server recognize that a previously-used (now-superseded) refresh token is being presented; since it cannot determine whether the legitimate user or an attacker is holding the current token, it invalidates the entire token family just to be safe. This would mean: on receiving a refresh request with a token that is cryptographically valid but no longer in the DB (i.e., already rotated), all sessions for that
userIdshould be revoked rather than just returningINVALID_REFRESH_TOKEN.This is a meaningful enhancement that can be a follow-up issue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/tave/crezipsa/crezipsa/application/auth/usecase/AuthUsecaseImpl.java` around lines 35 - 37, In AuthUsecaseImpl where refresh tokens are rotated (calling tokenProvider.generateAccessToken / generateRefreshToken and auth.updateTokens), add refresh-token-reuse detection: when a refresh request presents a cryptographically valid token that does not match the current stored refresh token for the auth record, treat that as reuse and revoke the entire token family for that userId (e.g., call a new method on the auth repository/service such as revokeAllSessions(userId) or invalidateAllTokens(userId)) instead of merely returning INVALID_REFRESH_TOKEN; implement the lookup/compare before generating new tokens and ensure the revoke-all operation is atomic/transactional alongside existing token updates.
35-37: Remove the dead code methodAuth.updateAccessToken().The
updateAccessToken()method is no longer called anywhere in the codebase after the transition toupdateTokens(). This method can be safely removed from theAuthentity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/tave/crezipsa/crezipsa/application/auth/usecase/AuthUsecaseImpl.java` around lines 35 - 37, Remove the unused/dead method Auth.updateAccessToken() from the Auth entity: locate the Auth class and delete the updateAccessToken(...) method signature and body, ensure no remaining references (the code now uses updateTokens(...) as in AuthUsecaseImpl where newAccessToken/newRefreshToken are set), run a project-wide search for updateAccessToken to confirm it’s not used and update any tests or interfaces that referenced it (replace calls with updateTokens if found).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/test/java/tave/crezipsa/crezipsa/application/auth/usecase/AuthUsecaseImplTest.java`:
- Around line 51-72: Add an assertion for the userId in the
reissueAccessToken_success_rotatesBothTokens test: after invoking
authUsecase.reissueAccessToken(request) assert that response.userId() equals the
expected userId (the local variable userId = 1L) to ensure TokenResponse
includes the correct user identifier; update the test method
reissueAccessToken_success_rotatesBothTokens to include this check alongside the
existing accessToken and refreshToken assertions.
---
Nitpick comments:
In
`@src/main/java/tave/crezipsa/crezipsa/application/auth/usecase/AuthUsecaseImpl.java`:
- Around line 35-37: In AuthUsecaseImpl where refresh tokens are rotated
(calling tokenProvider.generateAccessToken / generateRefreshToken and
auth.updateTokens), add refresh-token-reuse detection: when a refresh request
presents a cryptographically valid token that does not match the current stored
refresh token for the auth record, treat that as reuse and revoke the entire
token family for that userId (e.g., call a new method on the auth
repository/service such as revokeAllSessions(userId) or
invalidateAllTokens(userId)) instead of merely returning INVALID_REFRESH_TOKEN;
implement the lookup/compare before generating new tokens and ensure the
revoke-all operation is atomic/transactional alongside existing token updates.
- Around line 35-37: Remove the unused/dead method Auth.updateAccessToken() from
the Auth entity: locate the Auth class and delete the updateAccessToken(...)
method signature and body, ensure no remaining references (the code now uses
updateTokens(...) as in AuthUsecaseImpl where newAccessToken/newRefreshToken are
set), run a project-wide search for updateAccessToken to confirm it’s not used
and update any tests or interfaces that referenced it (replace calls with
updateTokens if found).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/tave/crezipsa/crezipsa/application/auth/usecase/AuthUsecaseImpl.javasrc/test/java/tave/crezipsa/crezipsa/application/auth/usecase/AuthUsecaseImplTest.javasrc/test/java/tave/crezipsa/crezipsa/fixture/AuthFixture.java
| void reissueAccessToken_success_rotatesBothTokens() { | ||
| // Given | ||
| Long userId = 1L; | ||
| String oldRefreshToken = "old-refresh-token"; | ||
| String newAccessToken = "new-access-token"; | ||
| String newRefreshToken = "new-refresh-token"; | ||
|
|
||
| Auth auth = createAuth(userId, "old-access-token", oldRefreshToken); | ||
| RefreshTokenRequest request = new RefreshTokenRequest(userId, oldRefreshToken); | ||
|
|
||
| when(authRepository.findByUserIdAndRefreshToken(userId, oldRefreshToken)) | ||
| .thenReturn(Optional.of(auth)); | ||
| when(tokenProvider.generateAccessToken(userId)).thenReturn(newAccessToken); | ||
| when(tokenProvider.generateRefreshToken(userId)).thenReturn(newRefreshToken); | ||
|
|
||
| // When | ||
| TokenResponse response = authUsecase.reissueAccessToken(request); | ||
|
|
||
| // Then | ||
| assertThat(response.accessToken()).isEqualTo(newAccessToken); | ||
| assertThat(response.refreshToken()).isEqualTo(newRefreshToken); | ||
| } |
There was a problem hiding this comment.
Missing userId assertion in the rotation success test.
The test verifies that both accessToken and refreshToken are rotated correctly, but does not assert the userId field on the TokenResponse. Given that TokenResponse.from(auth) populates userId from the entity, and userId is included in the API response (visible in the PR screenshot), the test's coverage of the response shape is incomplete.
✅ Proposed addition
// Then
+ assertThat(response.userId()).isEqualTo(userId);
assertThat(response.accessToken()).isEqualTo(newAccessToken);
assertThat(response.refreshToken()).isEqualTo(newRefreshToken);📝 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.
| void reissueAccessToken_success_rotatesBothTokens() { | |
| // Given | |
| Long userId = 1L; | |
| String oldRefreshToken = "old-refresh-token"; | |
| String newAccessToken = "new-access-token"; | |
| String newRefreshToken = "new-refresh-token"; | |
| Auth auth = createAuth(userId, "old-access-token", oldRefreshToken); | |
| RefreshTokenRequest request = new RefreshTokenRequest(userId, oldRefreshToken); | |
| when(authRepository.findByUserIdAndRefreshToken(userId, oldRefreshToken)) | |
| .thenReturn(Optional.of(auth)); | |
| when(tokenProvider.generateAccessToken(userId)).thenReturn(newAccessToken); | |
| when(tokenProvider.generateRefreshToken(userId)).thenReturn(newRefreshToken); | |
| // When | |
| TokenResponse response = authUsecase.reissueAccessToken(request); | |
| // Then | |
| assertThat(response.accessToken()).isEqualTo(newAccessToken); | |
| assertThat(response.refreshToken()).isEqualTo(newRefreshToken); | |
| } | |
| void reissueAccessToken_success_rotatesBothTokens() { | |
| // Given | |
| Long userId = 1L; | |
| String oldRefreshToken = "old-refresh-token"; | |
| String newAccessToken = "new-access-token"; | |
| String newRefreshToken = "new-refresh-token"; | |
| Auth auth = createAuth(userId, "old-access-token", oldRefreshToken); | |
| RefreshTokenRequest request = new RefreshTokenRequest(userId, oldRefreshToken); | |
| when(authRepository.findByUserIdAndRefreshToken(userId, oldRefreshToken)) | |
| .thenReturn(Optional.of(auth)); | |
| when(tokenProvider.generateAccessToken(userId)).thenReturn(newAccessToken); | |
| when(tokenProvider.generateRefreshToken(userId)).thenReturn(newRefreshToken); | |
| // When | |
| TokenResponse response = authUsecase.reissueAccessToken(request); | |
| // Then | |
| assertThat(response.userId()).isEqualTo(userId); | |
| assertThat(response.accessToken()).isEqualTo(newAccessToken); | |
| assertThat(response.refreshToken()).isEqualTo(newRefreshToken); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/tave/crezipsa/crezipsa/application/auth/usecase/AuthUsecaseImplTest.java`
around lines 51 - 72, Add an assertion for the userId in the
reissueAccessToken_success_rotatesBothTokens test: after invoking
authUsecase.reissueAccessToken(request) assert that response.userId() equals the
expected userId (the local variable userId = 1L) to ensure TokenResponse
includes the correct user identifier; update the test method
reissueAccessToken_success_rotatesBothTokens to include this check alongside the
existing accessToken and refreshToken assertions.
1 participant
📌 작업한 내용
AccessToken의 만료로 인하여 재발급 요청 시 , RefreshToken의 탈취를 방지하기 위하여
같이 재발급 되게끔 로테이션을 적용하였습니다.
코드는 클로드코드가 작성하였으나 ,TDD기반으로
case 1)유효한 RefreshToken으로 요청하면 AccessToken과 RefreshToken이 모두 갱신된다
case 2)DB에 존재하지 않는 RefreshToken으로 요청하면 INVALID_REFRESH_TOKEN 예외가 발생한다
의 검증이 통과하는지를 확인하였습니다.
🔍 참고 사항
🖼️ 스크린샷
🔗 관련 이슈
✅ 체크리스트
Summary by CodeRabbit
Release Notes
New Features
Tests