feat: 웹, 어드민 간 refresh token 분리#732
Open
whqtker wants to merge 7 commits into
Open
Conversation
- redis에 ADMIN_REFRESH:{userId} 형태로 토큰 저장
- 관련 테스트 구현
whqtker
requested review from
Gyuhyeok99,
Hexeong,
JAEHEE25,
lsy1307,
sukangpunch and
wibaek
as code owners
Walkthrough이 PR은 관리자 인증 시스템을 구현합니다. 1) 도메인 모델 추가 — Estimated code review effort🎯 4 (Complex) | ⏱️ ~45분 Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PMD (7.25.0)src/main/java/com/example/solidconnection/common/exception/ErrorCode.javaNo java executable found in PATH 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 563c0f1f94
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| public AdminReissueResponse reissue(String requestedAdminRefreshToken) { | ||
| if (!authTokenProvider.isValidAdminRefreshToken(requestedAdminRefreshToken)) { |
There was a problem hiding this comment.
When the admin refresh JWT is actually expired, isValidAdminRefreshToken() calls tokenProvider.parseSubject() before it can return false; JwtTokenProvider catches ExpiredJwtException and raises CustomException(INVALID_TOKEN), so the ADMIN_REFRESH_TOKEN_EXPIRED branch in CustomExceptionHandler is skipped and the admin refresh cookie is not deleted. In that expired-cookie scenario /admin/auth/reissue keeps returning an invalid-token error while leaving the stale cookie in the browser, defeating the new admin-cookie cleanup path.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/example/solidconnection/auth/token/config/TokenProperties.java (1)
33-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick win1)
adminRefresh누락 시Map.of에서 즉시 실패하므로, 명시적 검증으로 원인을 드러내 주세요.Line 34-38 구간은 설정 누락 시 NPE로 터져 원인 파악이 어렵습니다.
Objects.requireNonNull(또는 명시적IllegalStateException)로 키 이름을 포함해 fail-fast 처리하는 편이 안전합니다.수정 예시
+import java.util.Objects; ... `@PostConstruct` public void init() { tokenConfigs = Map.of( - AccessToken.class, access, - RefreshToken.class, refresh, - AdminRefreshToken.class, adminRefresh, - SignUpToken.class, signUp + AccessToken.class, Objects.requireNonNull(access, "token.access is required"), + RefreshToken.class, Objects.requireNonNull(refresh, "token.refresh is required"), + AdminRefreshToken.class, Objects.requireNonNull(adminRefresh, "token.admin-refresh is required"), + SignUpToken.class, Objects.requireNonNull(signUp, "token.sign-up is required") ); }🤖 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 `@src/main/java/com/example/solidconnection/auth/token/config/TokenProperties.java` around lines 33 - 39, In TokenProperties.init(), add an explicit null-check for adminRefresh (and other required token config fields if desired) before calling Map.of so missing configuration throws a clear, fail-fast error; use Objects.requireNonNull(adminRefresh, "adminRefresh must be configured") or throw new IllegalStateException with that message, then populate tokenConfigs = Map.of(...); reference the init() method, the adminRefresh field, tokenConfigs, and the Map.of call when making the change.
🧹 Nitpick comments (3)
src/main/java/com/example/solidconnection/admin/auth/service/AdminAuthService.java (1)
69-76: 🏗️ Heavy lift1) 관리자
reissue는 refresh token 회전(rotate)까지 포함하는 구성이 더 안전합니다.현재는 access token만 갱신되어, 탈취된 admin refresh token의 재사용 가능 시간이 그대로 남습니다. 관리자 경로는 회전 전략(새 refresh 발급 + 기존 refresh 폐기 + 쿠키 교체)으로 줄이는 것을 권장합니다.
🤖 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 `@src/main/java/com/example/solidconnection/admin/auth/service/AdminAuthService.java` around lines 69 - 76, AdminAuthService.reissue currently only issues a new AccessToken but must implement refresh token rotation: after validating requestedAdminRefreshToken via authTokenProvider.isValidAdminRefreshToken and parsing the SiteUser with authTokenProvider.parseSiteUser, generate a new AccessToken and a new RefreshToken (e.g., authTokenProvider.generateRefreshToken(siteUser)), persist or record the new refresh token and revoke/invalidate the old one (e.g., authTokenProvider.revokeRefreshToken(requestedAdminRefreshToken) or authTokenProvider.rotateRefreshToken(old, new)) in an atomic operation, and return the new refresh token to the caller (or ensure AdminReissueResponse.from(newAccessToken) is extended to include the new refresh token so the controller can replace the admin cookie). Ensure you reference AdminAuthService.reissue, authTokenProvider.isValidAdminRefreshToken, authTokenProvider.parseSiteUser, authTokenProvider.generateAccessToken, authTokenProvider.generateRefreshToken/revoke/rotate, and AdminReissueResponse when implementing these changes.src/test/java/com/example/solidconnection/admin/auth/service/AdminAuthServiceTest.java (1)
93-121: ⚡ Quick win1) 분리 목표를 직접 보장하는 부정 케이스 테스트를 추가해 주세요.
현재는 “저장된/삭제된 어드민 리프레시 토큰”만 검증합니다. PR 목표가 웹/어드민 토큰 분리인 만큼,
RefreshToken(웹용)을adminAuthService.reissue(...)에 넣었을 때 실패하는 케이스를 추가하면 회귀를 강하게 막을 수 있습니다.🤖 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 `@src/test/java/com/example/solidconnection/admin/auth/service/AdminAuthServiceTest.java` around lines 93 - 121, Add a negative test in AdminAuthServiceTest that ensures adminAuthService.reissue(...) rejects a web RefreshToken: generate and save a web RefreshToken (use authTokenProvider.generateAndSaveRefreshToken(...) or equivalent that returns a RefreshToken), then call adminAuthService.reissue(refreshToken.token()) and assert it throws AuthException with the ADMIN_REFRESH_TOKEN_EXPIRED message; this verifies that passing a RefreshToken (not AdminRefreshToken) to adminAuthService.reissue is rejected.src/main/java/com/example/solidconnection/security/config/SecurityConfiguration.java (1)
66-68: ⚡ Quick win
/admin/auth/**전체 공개 대신 필요한 엔드포인트만 공개해 주세요.
- Line 66 설정은
sign-out까지 익명 접근 경로에 포함합니다. 현재도 컨트롤러에서 실패 처리되지만, 인증 경계는 보안 설정 레이어에서 먼저 닫는 편이 안전합니다.권한 범위 최소화 제안
- .requestMatchers("/admin/auth/**").permitAll() + .requestMatchers("/admin/auth/sign-in", "/admin/auth/reissue").permitAll() .requestMatchers("/admin/**").hasRole(ADMIN.name())🤖 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 `@src/main/java/com/example/solidconnection/security/config/SecurityConfiguration.java` around lines 66 - 68, The broad permitAll for "/admin/auth/**" in SecurityConfiguration should be narrowed: replace the wildcard with explicit public auth endpoints (e.g. "/admin/auth/sign-in", "/admin/auth/sign-up", "/admin/auth/password-reset" — but NOT the sign-out endpoint) inside the same HttpSecurity configuration method so anonymous access is only granted to those specific handlers; ensure the "/admin/**" matcher with hasRole(ADMIN.name()) remains and that the ordering of requestMatchers keeps admin role checks intact.
🤖 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
`@src/main/java/com/example/solidconnection/auth/service/AuthTokenProvider.java`:
- Around line 80-85: Wrap the call to tokenProvider.parseSubject in
isValidAdminRefreshToken with a try-catch that catches parsing exceptions (e.g.,
IllegalArgumentException or the specific exception thrown by parseSubject) and
returns false on error; if parsing succeeds, proceed to use
tokenStorage.findToken(subject, AdminRefreshToken.class).map(...).orElse(false)
as before so any parse failure is normalized to false without propagating
exceptions.
In `@src/main/resources/config/application-variable.yml`:
- Around line 169-171: The admin refresh cookie is being set as secure
unconditionally which prevents browsers from saving it on http://localhost;
update AdminRefreshTokenCookieManager to set secure=false when running in
local/non-HTTPS environments or when the configured admin-refresh.cookie-domain
equals "localhost" (or is empty), and only set secure=true for HTTPS domains;
alternatively adjust application-variable.yml for local profile to use an empty
cookie-domain or a local-specific profile value and ensure the cookie builder
logic checks the domain/profile before applying secure(true).
In `@src/test/resources/application.yml`:
- Around line 95-98: The test YAML for token.admin-refresh is missing the
cookie-name required by the AdminRefreshTokenCookieProperties contract; update
the test configuration to include cookie-name (e.g., the same value used in
production) under token.admin-refresh so the AdminRefreshTokenCookieProperties
(cookie-name/cookie-domain) contract is satisfied and cookie creation in the
code paths that use AdminRefreshTokenCookieProperties will not receive a null
name.
---
Outside diff comments:
In
`@src/main/java/com/example/solidconnection/auth/token/config/TokenProperties.java`:
- Around line 33-39: In TokenProperties.init(), add an explicit null-check for
adminRefresh (and other required token config fields if desired) before calling
Map.of so missing configuration throws a clear, fail-fast error; use
Objects.requireNonNull(adminRefresh, "adminRefresh must be configured") or throw
new IllegalStateException with that message, then populate tokenConfigs =
Map.of(...); reference the init() method, the adminRefresh field, tokenConfigs,
and the Map.of call when making the change.
---
Nitpick comments:
In
`@src/main/java/com/example/solidconnection/admin/auth/service/AdminAuthService.java`:
- Around line 69-76: AdminAuthService.reissue currently only issues a new
AccessToken but must implement refresh token rotation: after validating
requestedAdminRefreshToken via authTokenProvider.isValidAdminRefreshToken and
parsing the SiteUser with authTokenProvider.parseSiteUser, generate a new
AccessToken and a new RefreshToken (e.g.,
authTokenProvider.generateRefreshToken(siteUser)), persist or record the new
refresh token and revoke/invalidate the old one (e.g.,
authTokenProvider.revokeRefreshToken(requestedAdminRefreshToken) or
authTokenProvider.rotateRefreshToken(old, new)) in an atomic operation, and
return the new refresh token to the caller (or ensure
AdminReissueResponse.from(newAccessToken) is extended to include the new refresh
token so the controller can replace the admin cookie). Ensure you reference
AdminAuthService.reissue, authTokenProvider.isValidAdminRefreshToken,
authTokenProvider.parseSiteUser, authTokenProvider.generateAccessToken,
authTokenProvider.generateRefreshToken/revoke/rotate, and AdminReissueResponse
when implementing these changes.
In
`@src/main/java/com/example/solidconnection/security/config/SecurityConfiguration.java`:
- Around line 66-68: The broad permitAll for "/admin/auth/**" in
SecurityConfiguration should be narrowed: replace the wildcard with explicit
public auth endpoints (e.g. "/admin/auth/sign-in", "/admin/auth/sign-up",
"/admin/auth/password-reset" — but NOT the sign-out endpoint) inside the same
HttpSecurity configuration method so anonymous access is only granted to those
specific handlers; ensure the "/admin/**" matcher with hasRole(ADMIN.name())
remains and that the ordering of requestMatchers keeps admin role checks intact.
In
`@src/test/java/com/example/solidconnection/admin/auth/service/AdminAuthServiceTest.java`:
- Around line 93-121: Add a negative test in AdminAuthServiceTest that ensures
adminAuthService.reissue(...) rejects a web RefreshToken: generate and save a
web RefreshToken (use authTokenProvider.generateAndSaveRefreshToken(...) or
equivalent that returns a RefreshToken), then call
adminAuthService.reissue(refreshToken.token()) and assert it throws
AuthException with the ADMIN_REFRESH_TOKEN_EXPIRED message; this verifies that
passing a RefreshToken (not AdminRefreshToken) to adminAuthService.reissue is
rejected.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 76e10ccb-2b72-4aa3-bcec-dbed0059c206
📒 Files selected for processing (17)
src/main/java/com/example/solidconnection/admin/auth/controller/AdminAuthController.javasrc/main/java/com/example/solidconnection/admin/auth/controller/AdminRefreshTokenCookieManager.javasrc/main/java/com/example/solidconnection/admin/auth/controller/config/AdminRefreshTokenCookieProperties.javasrc/main/java/com/example/solidconnection/admin/auth/dto/AdminReissueResponse.javasrc/main/java/com/example/solidconnection/admin/auth/dto/AdminSignInRequest.javasrc/main/java/com/example/solidconnection/admin/auth/dto/AdminSignInResponse.javasrc/main/java/com/example/solidconnection/admin/auth/dto/AdminSignInResult.javasrc/main/java/com/example/solidconnection/admin/auth/service/AdminAuthService.javasrc/main/java/com/example/solidconnection/auth/domain/AdminRefreshToken.javasrc/main/java/com/example/solidconnection/auth/service/AuthTokenProvider.javasrc/main/java/com/example/solidconnection/auth/token/config/TokenProperties.javasrc/main/java/com/example/solidconnection/common/exception/CustomExceptionHandler.javasrc/main/java/com/example/solidconnection/common/exception/ErrorCode.javasrc/main/java/com/example/solidconnection/security/config/SecurityConfiguration.javasrc/main/resources/config/application-variable.ymlsrc/test/java/com/example/solidconnection/admin/auth/service/AdminAuthServiceTest.javasrc/test/resources/application.yml
Comment on lines
+80
to
+85
| public boolean isValidAdminRefreshToken(String requestedAdminRefreshToken) { | ||
| Subject subject = tokenProvider.parseSubject(requestedAdminRefreshToken); | ||
| return tokenStorage.findToken(subject, AdminRefreshToken.class) | ||
| .map(foundToken -> Objects.equals(foundToken, requestedAdminRefreshToken)) | ||
| .orElse(false); | ||
| } |
There was a problem hiding this comment.
1) isValidAdminRefreshToken은 파싱 실패를 false로 정규화해 주세요.
Line 81에서 파싱 예외가 전파되면, 호출부가 기대한 “유효성 실패(false)” 흐름이 깨질 수 있습니다. 이 메서드는 비정상 입력에 대해 일관되게 false를 반환하도록 고정하는 편이 안전합니다.
수정 예시
public boolean isValidAdminRefreshToken(String requestedAdminRefreshToken) {
- Subject subject = tokenProvider.parseSubject(requestedAdminRefreshToken);
- return tokenStorage.findToken(subject, AdminRefreshToken.class)
- .map(foundToken -> Objects.equals(foundToken, requestedAdminRefreshToken))
- .orElse(false);
+ try {
+ Subject subject = tokenProvider.parseSubject(requestedAdminRefreshToken);
+ return tokenStorage.findToken(subject, AdminRefreshToken.class)
+ .map(foundToken -> Objects.equals(foundToken, requestedAdminRefreshToken))
+ .orElse(false);
+ } catch (RuntimeException e) {
+ return false;
+ }
}📝 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.
Suggested change
| public boolean isValidAdminRefreshToken(String requestedAdminRefreshToken) { | |
| Subject subject = tokenProvider.parseSubject(requestedAdminRefreshToken); | |
| return tokenStorage.findToken(subject, AdminRefreshToken.class) | |
| .map(foundToken -> Objects.equals(foundToken, requestedAdminRefreshToken)) | |
| .orElse(false); | |
| } | |
| public boolean isValidAdminRefreshToken(String requestedAdminRefreshToken) { | |
| try { | |
| Subject subject = tokenProvider.parseSubject(requestedAdminRefreshToken); | |
| return tokenStorage.findToken(subject, AdminRefreshToken.class) | |
| .map(foundToken -> Objects.equals(foundToken, requestedAdminRefreshToken)) | |
| .orElse(false); | |
| } catch (RuntimeException e) { | |
| return false; | |
| } | |
| } |
🤖 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
`@src/main/java/com/example/solidconnection/auth/service/AuthTokenProvider.java`
around lines 80 - 85, Wrap the call to tokenProvider.parseSubject in
isValidAdminRefreshToken with a try-catch that catches parsing exceptions (e.g.,
IllegalArgumentException or the specific exception thrown by parseSubject) and
returns false on error; if parsing succeeds, proceed to use
tokenStorage.findToken(subject, AdminRefreshToken.class).map(...).orElse(false)
as before so any parse failure is normalized to false without propagating
exceptions.
Comment on lines
+169
to
+171
| admin-refresh: | ||
| cookie-name: "adminRefreshToken" | ||
| cookie-domain: "localhost" |
There was a problem hiding this comment.
1) 로컬 프로필에서 어드민 리프레시 쿠키가 저장되지 않을 가능성이 큽니다.
Line 169-171은 localhost 도메인을 주지만, AdminRefreshTokenCookieManager가 쿠키를 항상 secure(true)로 발급해서 HTTP(http://localhost:*) 환경에서는 브라우저가 쿠키를 저장하지 않습니다. 로컬 로그인/재발급 플로우가 실제와 다르게 깨질 수 있습니다.
diff 제안
# src/main/resources/config/application-variable.yml
token:
admin-refresh:
cookie-name: "adminRefreshToken"
cookie-domain: "localhost"
+ cookie-secure: false// src/main/java/com/example/solidconnection/admin/auth/controller/config/AdminRefreshTokenCookieProperties.java
public record AdminRefreshTokenCookieProperties(
String cookieName,
- String cookieDomain
+ String cookieDomain,
+ boolean cookieSecure
) {// src/main/java/com/example/solidconnection/admin/auth/controller/AdminRefreshTokenCookieManager.java
ResponseCookie cookie = ResponseCookie.from(properties.cookieName(), adminRefreshToken)
.httpOnly(true)
- .secure(true)
+ .secure(properties.cookieSecure())
.path(PATH)🤖 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 `@src/main/resources/config/application-variable.yml` around lines 169 - 171,
The admin refresh cookie is being set as secure unconditionally which prevents
browsers from saving it on http://localhost; update
AdminRefreshTokenCookieManager to set secure=false when running in
local/non-HTTPS environments or when the configured admin-refresh.cookie-domain
equals "localhost" (or is empty), and only set secure=true for HTTPS domains;
alternatively adjust application-variable.yml for local profile to use an empty
cookie-domain or a local-specific profile value and ensure the cookie builder
logic checks the domain/profile before applying secure(true).
Hexeong
reviewed
Jun 8, 2026
| .path(PATH) | ||
| .maxAge(maxAge) | ||
| .domain(properties.cookieDomain()) | ||
| .sameSite(SameSite.LAX.attributeValue()) |
Contributor
There was a problem hiding this comment.
기존 LAX 방식이었던 이유가 어떤 거였는지 알려주실 수 있나요?? 인증 정보가 관리지 웹(www.admins.solid-connection.com)과 서비스 웹(www.solid-connection.com)으로 완전 분리되었으니까 Strict를 해도 되지 않을까라는 생각이 들어 의견을 듣고 싶습니다!
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
2 participants
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.
관련 이슈
작업 내용
ADMIN_REFRESH:{userId}형태로 토큰 저장됩니다.특이 사항
리뷰 요구사항 (선택)