fix: 토큰 재발급 로직 수정 #289
fix: 토큰 재발급 로직 수정 #289
Conversation
- subject의 기준이 SiteUser의 id가 아니게 바뀌더라도 변경 부분이 최소화되게 한다. - SiteUser의 subject를 사용하는 곳은 accessToken과 refreshToken를 관리하는 AuthTokenProvider 의 영역이므로 이 위치에 함수를 생성한다.
- parseSubject 가 parseSubjectIgnoringExpiration 보다 먼저 읽히도록
- subject가 아니라 엑세스 토큰으로 리프레시 토큰을 찾도록
Walkthrough
✨ Finishing Touches
🪧 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 (
|
| public ReissueResponse reissue(String subject) { | ||
| public ReissueResponse reissue(String accessToken) { | ||
| // 리프레시 토큰 만료 확인 | ||
| String subject = parseSubject(accessToken, jwtProperties.secret()); | ||
| Optional<String> optionalRefreshToken = authTokenProvider.findRefreshToken(subject); | ||
| if (optionalRefreshToken.isEmpty()) { | ||
| throw new CustomException(REFRESH_TOKEN_EXPIRED); | ||
| } | ||
| // 액세스 토큰 재발급 | ||
| String newAccessToken = authTokenProvider.generateAccessToken(subject); |
There was a problem hiding this comment.
컨트롤러에서 accessToken을 authService.reissue() 함수의 인자로 넘겨줬는데,
reissue 서비스 코드에서는 String subject 라는 파라미터 이름을 사용해서,
마치 컨트롤러에서 subject를 바로 넘겨받은 것으로 해석해서 로직을 짰었습니다 🥲
변경된 코드에서는 파라미터 이름을 accessToken 으로 바꿔 오해를 방지하고,
토큰에서 subject를 추출하여 이후 로직을 수행하도록 했습니다.
wibaek
left a comment
wibaek
left a comment
There was a problem hiding this comment.
아 이런 문제가 있었군요~
말씀대로 Token 타입이 따로 있었으면 알아차리기 쉬웠을 것 같네요!
분리해서 커밋해주셔서 PR 읽기 매우 편했습니다🙂
| String accessToken = authentication.getCredentials().toString(); | ||
| if (accessToken == null) { |
There was a problem hiding this comment.
궁금한 점이 생겼습니다..!
지금 만료된 엑세스 토큰으로 리프레시토큰을 찾아와서 새 엑세스토큰을 발급해주는 거로 이해를 했습니다!
- 이러면 클라이언트에서 리프레시 토큰을 보관할 이유가 없어진 거 같은데 맞나요?
- 그래서 저는 클라이언트에서 만료된 엑세스 토큰 에러를 받으면 리프레시 토큰을 서버로 전송해서 그게 맞는지 검증하고 엑세스 토큰을 발급해주는 게 맞다고 생각했는데 혹시 이거 관해서 얘기한 게 있었나요? 제가 놓친 거 같아요 🥲
(지금 방식이면 누군가 만료된 토큰을 탈취해도 계속 엑세스 토큰을 발급받는 거 아닌가란 생각이 들어서요!)
There was a problem hiding this comment.
어... 맞습니다 저는 저 accessToken이라는 이름으로 되있는게 refreshToken이라 생각했네요.
reissue API로 전송하는건 refresh token인걸로 알고 있어서요.
저도 전체 코드 다시 한번 읽어보고 말씀드리겠습니다
There was a problem hiding this comment.
아하 그렇네요...! 제가 뭐랑 헷갈린걸까요... 😭😭
refreshToken을 POST 요청으로 받아,
Redis에 존재하는지 보고 존재한다면 새로운 accessToken 을 발급하도록 수정하면 될까요?
There was a problem hiding this comment.
이 방식이나 헤더로 받는 방식 두 개가 있을 거 같은데 전 둘 다 좋은 거 같아요!
이건 정말 좋은 아이디어같습니다! 캡슐화도 되고 타입 안정성도 생기니 도입 안할 이유가 없는 거 같네요 ㅎㅎ |
| } | ||
| // 액세스 토큰 재발급 | ||
| String newAccessToken = authTokenProvider.generateAccessToken(subject); | ||
| String newAccessToken = authTokenProvider.generateAccessToken(accessToken); |
There was a problem hiding this comment.
엑세스 토큰 재발급은
authTokenProvider.generateAccessToken(accessToken);
가 아니라
authTokenProvider.generateAccessToken(subject);일 거 같은데 맞나요?
There was a problem hiding this comment.
ㅋㅎㅎㅎ... 이런 실수 방지하려면 Token 클래스가 정말 필요하겠네요..
| String accessToken = authentication.getCredentials().toString(); | ||
| if (accessToken == null) { |
There was a problem hiding this 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: 0
🧹 Nitpick comments (1)
src/main/java/com/example/solidconnection/auth/service/AuthService.java (1)
51-62: 향후 개선 제안: Token 클래스 도입PR 설명과 이전 리뷰 코멘트에서 언급된 것처럼, Token 클래스를 도입하면 이러한 오류를 더욱 방지할 수 있을 것입니다:
- 토큰과 subject를 명확히 구분
- 타입 안전성 제공
- JwtAuthenticationFilter에서 Token 객체 생성 및 subject 파싱
- JwtUtils.parseSubject() 호출을 줄임
이 리팩토링은 코드 안정성과 유지보수성을 더욱 향상시킬 것입니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/com/example/solidconnection/auth/controller/AuthController.java(2 hunks)src/main/java/com/example/solidconnection/auth/domain/TokenType.java(1 hunks)src/main/java/com/example/solidconnection/auth/dto/ReissueRequest.java(1 hunks)src/main/java/com/example/solidconnection/auth/service/AuthService.java(2 hunks)src/test/java/com/example/solidconnection/auth/service/AuthServiceTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (16)
src/main/java/com/example/solidconnection/auth/controller/AuthController.java (2)
6-6: 필요한 ReissueRequest import를 추가했습니다.컨트롤러에서 ReissueRequest DTO를 받기 위해 필요한 import를 올바르게 추가했습니다.
117-121: 토큰 재발급 로직이 개선되었습니다.다음과 같은 개선 사항이 있습니다:
- Authentication 객체 대신 ReissueRequest DTO를 직접 받아 처리하도록 변경
- 컨트롤러에서 토큰 추출 로직을 제거하고 서비스 계층에 위임
- 코드가 간결해지고 책임 분리가 명확해짐
이는 이전 코드에서 발생했던 혼동(Authentication에서 토큰을 추출해 subject로 처리하는 과정)을 해결하여 더욱 명확한 구조를 제공합니다.
src/main/java/com/example/solidconnection/auth/dto/ReissueRequest.java (1)
1-8: 적절한 DTO 구조 설계다음과 같은 긍정적인 측면이 있습니다:
- 리프레시 토큰을 위한 전용 DTO를 만들어 코드 가독성 향상
- Java Record를 사용하여 간결하고 불변 객체로 설계
- @notblank 어노테이션으로 유효성 검증 추가
- 적절한 오류 메시지 제공
이 DTO는 서비스 계층에서 리프레시 토큰을 명확하게 처리할 수 있도록 해줍니다. 좋은 설계입니다!
src/main/java/com/example/solidconnection/auth/domain/TokenType.java (2)
8-9: 토큰 만료 시간 타입 및 값 변경다음과 같은 변경사항이 있습니다:
- ACCESS 토큰 만료 시간 계산에 long 타입 리터럴 사용 (1000L)
- REFRESH 토큰 만료 시간이 7일에서 90일로 크게 증가
리프레시 토큰의 만료 시간을 90일로 늘린 것은 사용자 경험 측면에서 큰 변화입니다. 이로 인해 사용자가 로그인을 자주 해야 하는 불편함이 크게 줄어들 것입니다.
11-11: expireTime 필드 타입 변경다음과 같은 변경사항이 있습니다:
- expireTime 필드 타입을 int에서 long으로 변경
- 생성자 파라미터 타입도 long으로 변경
- SIGN_UP 토큰 시간 계산에 long 타입 리터럴 사용
long 타입을 사용함으로써 더 큰 시간 값(90일과 같은)을 안전하게 처리할 수 있게 되었습니다. int 타입의 최대값(약 21억 밀리초, 약 24.8일)을 초과하는 값을 다룰 때 발생할 수 있는 오버플로우 문제를 방지합니다.
Also applies to: 15-17
src/main/java/com/example/solidconnection/auth/service/AuthService.java (3)
4-7: 필요한 import 추가 및 변경다음과 같은 변경사항이 있습니다:
- ReissueRequest DTO import 추가
- JwtProperties import 추가
- Objects 유틸리티 import 추가
- JwtUtils의 parseSubject 메소드 import 변경
- JwtProperties 의존성 추가
이러한 변경은 리프레시 토큰 검증 로직에 필요한 의존성을 명확히 하고, 안전한 객체 비교를 위한 Objects.equals 사용을 가능하게 합니다.
Also applies to: 14-18, 25-25
46-50: 메소드 주석 개선리프레시 토큰 재발급 로직의 변경사항을 명확히 반영하도록 주석이 업데이트되었습니다:
- 이제 요청된 리프레시 토큰과 저장된 토큰이 일치하는지 확인
- 일치하지 않을 경우 예외 발생 명시
주석이 실제 구현 코드와 일치하게 되어 코드 이해도가 향상되었습니다.
51-58: 토큰 재발급 로직 개선다음과 같은 중요한 개선사항이 있습니다:
- 이제 ReissueRequest에서 리프레시 토큰을 명시적으로 추출
- JwtUtils.parseSubject를 사용하여 토큰에서 subject 추출
- 저장된 리프레시 토큰과 요청된 토큰이 정확히 일치하는지 확인
- Objects.equals를 사용하여 null 안전 비교 구현
이전 구현에서는 파라미터가 'subject'로 명명되어 혼란을 야기했으나, 실제로는 액세스 토큰을 처리하고 있었습니다. 이제 코드가 명확해져 이해하기 쉽고 오류 가능성이 줄어들었습니다.
src/test/java/com/example/solidconnection/auth/service/AuthServiceTest.java (8)
1-24: 패키지 및 임포트 구성이 깔끔합니다!테스트 클래스를 위한 필요한 임포트들을 잘 구성하셨습니다. JUnit 5의 기능들과 AssertJ 검증 라이브러리를 효과적으로 활용하는 것이 좋습니다.
25-40: 테스트 클래스 설정이 명확합니다!
@DisplayName을 한글로 사용하여 테스트의 목적을 명확히 표현했습니다.@TestContainerSpringBootTest커스텀 애노테이션을 사용하여 테스트 환경을 일관되게 설정하셨습니다.- 필요한 의존성들(
AuthService,AuthTokenProvider,SiteUserRepository,JwtProperties)을 명확하게 주입하셨습니다.
41-51: 로그아웃 테스트가 간결하고 명확합니다!
- 테스트 메소드명을 한글로 직관적으로 표현했습니다.
- given-when-then 패턴을 사용하여 테스트 구조가 명확합니다.
- 로그아웃 기능이 액세스 토큰을 블랙리스트에 제대로 추가하는지 검증합니다.
53-64: 탈퇴 테스트 구현이 적절합니다!
- 사용자 탈퇴 기능을 테스트하는 메소드가 간결하게 구현되었습니다.
- 탈퇴 시
quitedAt필드가 내일 날짜로 설정되는지 확인하는 검증 로직이 명확합니다.- given-when-then 패턴으로 테스트 가독성이 좋습니다.
66-96: 토큰 재발급 테스트가 PR 목적에 맞게 잘 구현되었습니다!
@Nested클래스를 사용하여 관련 테스트를 논리적으로 그룹화했습니다.- 성공 케이스:
- 리프레시 토큰이 저장되어 있고 값이 일치할 때 액세스 토큰 재발급 검증
- 재발급된 액세스 토큰의 subject가 리프레시 토큰의 subject와 일치하는지 확인
- 실패 케이스:
- 리프레시 토큰이 저장되어 있지 않을 때 예외 발생 검증
REFRESH_TOKEN_EXPIRED에러 메시지 확인PR 목적인 "토큰 재발급 로직 수정"에 맞게 테스트가 잘 구성되었습니다.
98-107: 유틸리티 메소드가 유용하게 구현되었습니다!테스트에 필요한
SiteUser객체를 생성하는 헬퍼 메소드가 코드 중복을 줄이고 테스트 가독성을 높여줍니다. 필요한 모든 필드를 포함하여 일관된 테스트 데이터를 제공합니다.
76-82: 토큰 재발급 로직의 핵심 검증 부분입니다!이 부분은 PR에서 언급된 토큰 재발급 로직 수정의 핵심입니다. 재발급된 액세스 토큰의 subject가 리프레시 토큰의 subject와 일치하는지 확인하여 토큰이 너무 빨리 만료되는 문제를 해결하고 있습니다. 이전 코드에서는 이 부분에서 오류가 있었을 가능성이 높습니다.
1-108: 전체적인 테스트 구성이 훌륭합니다!
- PR의 핵심 목적인 "토큰 재발급 로직 수정"에 대한 테스트가 잘 구현되었습니다.
- 모든 테스트가 given-when-then 패턴을 따라 가독성이 좋습니다.
- 한글 메소드명을 사용하여 테스트의 의도가 명확합니다.
- 성공과 실패 케이스를 모두 테스트하여 코드 신뢰성을 높였습니다.
PR에서 제안한 향후 개선사항인 Token 클래스 도입도 좋은 아이디어로 보입니다. 현재 테스트에서도 토큰과 subject를 문자열로 처리하는 부분이 있는데, Token 클래스를 도입하면 타입 안전성이 향상되고 이와 같은 테스트 작성도 더 명확해질 것입니다.
| import jakarta.validation.constraints.NotBlank; | ||
|
|
||
| public record ReissueRequest( | ||
| @NotBlank(message = "리프레시 토큰과 함께 요청해주세요.") |
There was a problem hiding this comment.
사소하지만 ㅎㅎ.. 이 컨벤션이 늘 헷갈렸는데 제가 들어오기전 솔커 컨벤션에선 dto에서 어노테이션 쓸 때 한 칸 개행하고 썼더라구요! 나중에 개행한 거 다 없앨까요?
There was a problem hiding this comment.
아하 지금 어노테이션이 안붙어있는 값들은 개행이 안되어있고,
어노테이션이 붙어있는 것들만 시작 개행이 되어있는 상태군요 🫢
둘중에 어느것으로든 통일하면 좋겠는데.. 규혁님은 둘 다 개행 안한게 더 좋으신가요?
저는 둘 중 어느것으로 가도 큰 상관 없습니다!
There was a problem hiding this comment.
읽는 측면에서 큰 차이는 없는 거 같아서 저도 큰 상관 없지만, 어노테이션 앞에 시작개행이 불필요한 빈 줄이라면 없애는 게 더 나은 거 같네요!
There was a problem hiding this comment.
그럼 시작 개행 안하도록 통일하는게 좋겠네요 ㅎㅎ
3 participants
관련 이슈
작업 내용
분명 엑세스 토큰 재발급 로직이 있을텐데 왜 사용자가 만료가 빠르다고 느끼는가 싶어 코드를 봤더니
엑세스 로직에서 실수가 있었습니다 🤕
코멘트로 설명하겠습니다!
그리고 몇가지 작은 리팩터링은 커밋을 따라가면 이해하시기 편하실겁니다~
앞으로의 계획 - 피드백 부탁!
토큰도 Sting, subject도 String 형식이라 이런 실수가 발생했습니다.
Token 이라는 클래스를 만들면 이런 실수를 사전에 방지할 수 있을 것 같습니다.
그리고 Token 클래스가 필드로 subject를 가지게 하고,
JwtAuthenticationFilter에서 Token 객체를 생성할 때 subject를 초기화한다면여러곳에서 JwtUtils.parseSubject() 를 호출하고있는 지금의 코드를 개선할 수 있을 것 같습니다!
(지금은 parseSubject 함수가 19군데에서 호출되고 있습니다.)