refactor: 문자열 토큰을 Token 클래스로 #297
Conversation
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ 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 (
|
![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 (4)
src/main/java/com/example/solidconnection/auth/service/Subject.java (1)
1-6: 새로운 Subject 레코드 클래스 추가가 훌륭합니다!이 클래스는 토큰의 주체(subject)를 캡슐화하여 문자열 대신 타입 안전성을 제공하는 좋은 예입니다. 레코드를 사용하여 간결하게 구현한 점이 인상적입니다.
다음과 같은 개선 사항을 고려해 보시기 바랍니다:
클래스 용도에 대한 설명 추가
- Subject가 어떤 역할을 하는지 명확히 하는 JavaDoc 주석을 추가하면 더 좋을 것 같습니다
유효성 검사 고려
- value가 null이나 빈 문자열이 아닌지 확인하는 검증 로직을 추가하는 것이 좋을 수 있습니다
public record Subject(String value) { public Subject { Objects.requireNonNull(value, "Subject value must not be null"); if (value.isBlank()) { throw new IllegalArgumentException("Subject value must not be blank"); } } }src/main/java/com/example/solidconnection/auth/service/BlacklistChecker.java (1)
1-6: 인터페이스 도입으로 의존성 역전 원칙(DIP)을 잘 적용했습니다!이 인터페이스는 토큰의 블랙리스트 확인 책임을 명확히 분리하여 단일 책임 원칙(SRP)을 잘 따르고 있습니다.
개선을 위한 제안:
인터페이스 목적에 대한 문서화
- 인터페이스의 용도와 인증 흐름에서의 역할을 설명하는 JavaDoc을 추가하면 좋을 것 같습니다
/** * 토큰이 블랙리스트에 등록되었는지 확인하는 인터페이스입니다. * 이 인터페이스는 필터와 서비스 레이어 간의 의존성을 분리하기 위해 사용됩니다. */ public interface BlacklistChecker {타입 안전성 강화 고려
- 현재 String 타입을 매개변수로 받고 있지만, 전체적인 리팩토링 방향성에 맞춰 AccessToken 타입을 사용하는 것도 고려해볼 수 있습니다
boolean isTokenBlacklisted(AccessToken token);src/test/java/com/example/solidconnection/custom/security/filter/SignOutCheckFilterTest.java (1)
62-63: 블랙리스트 키-값 저장 방식 개선이 잘 반영되었습니다!블랙리스트 토큰을 저장하는 방식이 변경되어 테스트가 적절히 수정되었습니다. 이제 키는 주체(subject)를 기반으로 하고, 값은 토큰 자체를 사용합니다.
개선을 위한 제안:
새로운 저장 방식에 대한 설명 추가
- 다음과 같은 주석을 추가하여 저장 방식의 변경 이유를 명확히 하면 좋을 것 같습니다
// 토큰 주체(subject)를 키로 사용하고 토큰 자체를 값으로 저장하는 방식으로 변경 // 이는 동일 사용자의 여러 토큰을 효과적으로 관리하기 위함입니다 String refreshTokenKey = BLACKLIST.addPrefix(subject); redisTemplate.opsForValue().set(refreshTokenKey, token);다양한 주체(subject) 값에 대한 테스트 케이스 추가
- 여러 subject 값에 대해 블랙리스트 저장 및 확인이 정상 작동하는지 검증하는 테스트를 추가하면 더 견고해질 것 같습니다
src/main/java/com/example/solidconnection/auth/dto/SignInResponse.java (1)
10-13: AccessToken과 RefreshToken 객체를 활용한 팩토리 메서드 추가가 훌륭합니다!이 정적 팩토리 메서드는 도메인 특화 토큰 객체에서 SignInResponse를 생성하는 좋은 방법입니다. 이를 통해 코드 전반에 걸쳐 타입 안전성이 향상됩니다.
개선을 위한 제안:
메서드 목적에 대한 문서화 추가
/** * AccessToken과 RefreshToken 객체로부터 SignInResponse를 생성합니다. * 이 메서드는 도메인 특화 토큰 객체에서 문자열 토큰을 추출하여 응답 객체를 생성합니다. * * @param accessToken 액세스 토큰 객체 * @param refreshToken 리프레시 토큰 객체 * @return 로그인 응답 객체 */ public static SignInResponse of(AccessToken accessToken, RefreshToken refreshToken) {일관된 네이밍 고려
- 'of' 대신 'from'을 사용하는 것이 변환 작업의 의도를 더 명확히 할 수 있습니다
public static SignInResponse from(AccessToken accessToken, RefreshToken refreshToken) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/main/java/com/example/solidconnection/auth/controller/AuthController.java(2 hunks)src/main/java/com/example/solidconnection/auth/dto/ReissueResponse.java(1 hunks)src/main/java/com/example/solidconnection/auth/dto/SignInResponse.java(1 hunks)src/main/java/com/example/solidconnection/auth/service/AccessToken.java(1 hunks)src/main/java/com/example/solidconnection/auth/service/AuthService.java(2 hunks)src/main/java/com/example/solidconnection/auth/service/AuthTokenProvider.java(1 hunks)src/main/java/com/example/solidconnection/auth/service/BlacklistChecker.java(1 hunks)src/main/java/com/example/solidconnection/auth/service/RefreshToken.java(1 hunks)src/main/java/com/example/solidconnection/auth/service/SignInService.java(1 hunks)src/main/java/com/example/solidconnection/auth/service/Subject.java(1 hunks)src/main/java/com/example/solidconnection/custom/security/filter/SignOutCheckFilter.java(3 hunks)src/test/java/com/example/solidconnection/auth/service/AuthServiceTest.java(2 hunks)src/test/java/com/example/solidconnection/auth/service/AuthTokenProviderTest.java(1 hunks)src/test/java/com/example/solidconnection/auth/service/SignInServiceTest.java(4 hunks)src/test/java/com/example/solidconnection/custom/security/filter/SignOutCheckFilterTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (30)
src/main/java/com/example/solidconnection/auth/dto/SignInResponse.java (1)
3-5: 토큰 관련 클래스 import 추가가 적절합니다!도메인 특화 토큰 클래스를 사용하기 위한 import 문이 잘 추가되었습니다.
src/main/java/com/example/solidconnection/auth/dto/ReissueResponse.java (2)
3-3: 새 AccessToken 클래스 임포트 추가타입 안전성을 높이기 위한 좋은 변경입니다. 문자열 토큰 대신 강타입(strongly typed) 객체를 사용하는 접근 방식으로 전환하는 데 필요한 기본 구성 요소입니다.
9-11: 정적 팩토리 메서드 도입으로 응답 생성 패턴 개선AccessToken 객체를 인자로 받아 ReissueResponse 인스턴스를 생성하는 팩토리 메서드를 추가하셨네요. 이 변경은 다음과 같은 장점이 있습니다:
- 토큰 문자열 추출 로직을 DTO 내부로 캡슐화하여 코드 가독성 향상
- AccessToken 객체에서 ReissueResponse 객체로의 변환 과정을 표준화
- 서비스 계층에서 문자열 대신 도메인 객체를 사용할 수 있게 지원
정적 팩토리 메서드 패턴의 적절한 활용으로 코드 품질이 향상되었습니다.
src/main/java/com/example/solidconnection/auth/service/SignInService.java (1)
18-21: 토큰 생성 로직의 타입 안전성 개선SignIn 메서드의 변경 사항에 대해 다음과 같은 개선점이 있습니다:
- SiteUser를 Subject 객체로 변환하는 과정이 명시적으로 추가되어 토큰의 주체(subject)에 대한 표현이 명확해졌습니다
- 문자열 기반 토큰 대신 AccessToken과 RefreshToken 객체를 사용하여 타입 안전성이 향상되었습니다
- 응답 생성에 정적 팩토리 메서드를 활용하여 코드의 의도가 더 명확해졌습니다
이러한 변경은 인증 관련 로직의 객체지향적 설계를 강화하고, JWT 세부 구현 사항을 외부 코드로부터 숨기는 캡슐화 원칙을 잘 적용한 사례입니다.
src/main/java/com/example/solidconnection/auth/service/AccessToken.java (1)
1-11: AccessToken 레코드 클래스 도입으로 토큰 관리 개선새로 추가된 AccessToken 레코드에 대한 리뷰입니다:
- Subject와 token을 필드로 갖는 레코드 타입으로 정의하여 불변성(immutability)을 보장
- 기본 생성자와 함께 문자열 기반 생성자를 오버로딩하여 다양한 생성 방식 지원
- 토큰과 관련된 데이터를 하나의 객체로 응집시켜 캡슐화 향상
이러한 접근 방식은 인증 토큰의 표현과 처리를 더 객체지향적으로 만들고, 문자열 기반 토큰 관리의 한계를 극복하는 좋은 해결책입니다. 도메인 로직에 특화된 클래스를 사용함으로써 코드의 가독성과 유지보수성이 향상될 것입니다.
src/main/java/com/example/solidconnection/auth/service/RefreshToken.java (1)
1-11: RefreshToken 레코드 클래스 도입으로 리프레시 토큰 관리 개선새로 추가된 RefreshToken 레코드에 대한 리뷰입니다:
- Subject와 token을 필드로 갖는 불변 레코드 타입으로 설계되어 안전한 토큰 관리 지원
- 문자열 기반 생성자는 package-private 가시성을 가져 동일 패키지 내에서만 생성 가능하도록 제한
- AccessToken과 유사한 구조를 가지지만 용도에 맞게 접근 제한자를 조정하여 적절한 캡슐화 제공
한 가지 주목할 점은 AccessToken의 보조 생성자는 public인 반면, RefreshToken의 보조 생성자는 패키지 수준의 접근 제한을 갖습니다. 이는 RefreshToken 인스턴스가 서비스 패키지 내에서만 생성되어야 함을 의도한 것으로 보이며, 토큰 생성에 대한 통제력을 높이는 좋은 설계 결정입니다.
src/main/java/com/example/solidconnection/auth/controller/AuthController.java (3)
12-12: AccessToken 클래스 임포트 추가Token 객체 모델을 사용하는 방향으로 리팩토링하면서 필요한 클래스 임포트가 적절히 추가되었습니다.
101-102: 인증 객체 타입 검증 개선문자열 토큰 대신 AccessToken 객체를 사용하여 타입 안전성을 높인 좋은 변경입니다.
- instanceof 패턴 매칭을 활용해 간결하게 타입 검증과 변수 할당을 동시에 처리하고 있습니다.
- 타입이 일치하지 않을 경우 명확한 예외 처리를 제공합니다.
104-104: 타입 안전한 파라미터 전달문자열 대신 AccessToken 객체를 서비스 계층에 전달함으로써 타입 안전성을 보장하고 있습니다. 이는 PR 목표에 명시된 "인증 관련 로직에서 Java 객체 사용을 강제하는 객체지향적 설계"와 일치합니다.
src/test/java/com/example/solidconnection/auth/service/SignInServiceTest.java (5)
3-3: TokenType 임포트 추가토큰 타입에 따른 관리를 위해 TokenType 열거형을 적절히 임포트했습니다.
16-17: RedisTemplate 의존성 추가Redis에 직접 접근하여 토큰을 조회하기 위한 의존성이 추가되었습니다. 이는 리팩토링된 토큰 저장 방식에 맞게 테스트 코드를 업데이트한 것으로 적절합니다.
39-40: RedisTemplate 주입Redis 저장소에 직접 접근하기 위한 RedisTemplate 주입이 적절히 이루어졌습니다.
60-60: 토큰 조회 방식 개선토큰 조회 방식이 개선되었습니다:
- 이전의 Optional 반환 방식에서 직접 Redis에서 값을 조회하는 방식으로 변경되었습니다.
- TokenType.REFRESH.addPrefix() 메서드를 사용하여 표준화된 키 형식을 적용했습니다.
이는 PR 목표에 명시된 "블랙리스트 저장 방식 변경"과 일관된 접근 방식입니다.
64-64: 검증 방식 단순화Optional 검증 대신 직접 문자열 비교를 통해 테스트 검증을 단순화했습니다. 이는 코드의 가독성과 직관성을 높이는 변경입니다.
src/main/java/com/example/solidconnection/custom/security/filter/SignOutCheckFilter.java (3)
3-3: BlacklistChecker 인터페이스 임포트 추가의존성 역전 원칙(DIP)을 적용하기 위해 필요한 인터페이스가 적절히 임포트되었습니다.
23-23: 의존성 개선AuthTokenProvider에 대한 직접적인 의존성을 BlacklistChecker 인터페이스로 대체했습니다:
- 의존성 역전 원칙(DIP)을 적용하여 추상화에 의존하도록 개선했습니다.
- 이를 통해 필터와 서비스 계층 간의 결합도가 낮아졌습니다.
이 변경은 PR 목표에 명시된 "Spring Security 필터와 서비스 계층의 분리"를 정확히 구현한 것입니다.
37-37: 블랙리스트 확인 로직 개선블랙리스트 확인 방식이 명확하게 개선되었습니다:
- 이전의 Optional 확인 방식에서 boolean 반환 메서드로 변경되었습니다.
- 메서드 이름이 직관적으로 변경되어 코드의 가독성이 향상되었습니다.
이는 PR 목표에 명시된 "메서드 시그니처 변경"과 일치하는 변경입니다.
src/test/java/com/example/solidconnection/auth/service/AuthServiceTest.java (5)
38-38: Subject 및 AccessToken 클래스 사용문자열 대신 도메인 객체를 사용하여 테스트 코드의 타입 안전성이 향상되었습니다. todo 주석은 #296 이슈와 관련된 것으로 보이며, 별도로 처리될 예정입니다.
44-44: 블랙리스트 검증 방식 개선블랙리스트 검증 로직이 개선되었습니다:
- Optional 확인 방식에서 boolean 반환 메서드로 변경되었습니다.
- 메서드 이름이 명확하게 변경되어 코드의 의도가 더 잘 드러납니다.
64-68: RefreshToken 객체 사용문자열 토큰 대신 RefreshToken 객체를 생성하고 활용하여 타입 안전성을 높였습니다. 이는 토큰 관리를 객체지향적으로 개선한 PR 목표와 일치합니다.
73-75: Subject 추출 로직 개선토큰에서 Subject를 추출하는 방식이 개선되었습니다:
- JwtUtils 정적 메서드 대신 authTokenProvider의 메서드를 사용합니다.
- 이를 통해 토큰 처리 로직을 한 곳에 집중시키고 토큰의 내부 구현 방식을 캡슐화했습니다.
81-82: 유효하지 않은 토큰 테스트 케이스 개선유효하지 않은 RefreshToken 테스트를 위해 AccessToken을 사용하는 방식으로 변경했습니다. 이는 실제 환경에서 발생할 수 있는 상황을 더 정확하게 모델링하는 좋은 테스트 케이스입니다.
src/main/java/com/example/solidconnection/auth/service/AuthService.java (2)
25-27: 1) 로그아웃 시 이미 블랙리스트에 등록된 토큰 처리 누락동일 토큰이 블랙리스트에 중복 저장돼도 큰 문제는 없지만, Redis
SETNX혹은exists()체크를 통해 idempotent 하게 만드는 것이 유지‧운영 단계에서 불필요한 I/O 를 줄여줍니다.
[ suggest_optional_refactor ]
42-55: 2)isValidRefreshToken예외 전파 시, 커스텀 예외로 래핑 필요
isValidRefreshToken()내부에서 JWT 파싱 예외가 발생하면 boolean 대신 런타임 예외가 그대로 전파되어REFRESH_TOKEN_EXPIRED로 매핑되지 않습니다.
앞서 제안드린 try-catch 보강이 선행되면 본 메서드의 흐름도 자연스럽게 정리됩니다.
[ flag_critical_issue ]src/test/java/com/example/solidconnection/auth/service/AuthTokenProviderTest.java (3)
25-30: 1) Subject 생성값 하드코딩 대신 Test Fixture 상수 사용 권장여러 테스트에서 동일 문자열
"subject123"을 반복 사용하고 있습니다. 테스트 가독성을 위해private static final Subject SUBJECT = new Subject("subject123");형태로 추출하면 유지보수가 쉬워집니다.
[ suggest_nitpick ]
64-70: 2)fakeRefreshToken변수명 혼동
AccessToken으로 생성된 토큰을fakeRefreshToken으로 명명하면 독자가 혼란을 겪습니다. 변수명을otherToken정도로 수정하면 의도가 더 명확해집니다.
[ suggest_nitpick ]
91-97: 3) TODO 주석 → 테스트 격리 전략 구체화 필요
JwtUtils를 Mock 으로 대체하려는 계획이 명시돼 있지만, 구체적 스프링 컨텍스트 설정이 없어 추후 미루기 쉽습니다.@Import(MockitoJwtConfig.class)같은 별도 설정 클래스를 마련해 두면 실수로 TODO 가 방치되는 것을 예방할 수 있습니다.
[ request_verification ]src/main/java/com/example/solidconnection/auth/service/AuthTokenProvider.java (3)
44-49: 1) 유효성 검사 중 파싱 예외가 발생하면 false 반환하도록 수정 필요지금은
JwtUtils.parseSubject()가 예외를 던지면 상위 계층까지 전파됩니다. boolean 메서드 시그니처와 맞지 않으므로, 예외를 캐치해false를 반환하는 편이 정책에 부합합니다.
아래와 같이 try-catch 를 추가해보세요.- public boolean isValidRefreshToken(String requestedRefreshToken) { - String subject = JwtUtils.parseSubject(requestedRefreshToken, jwtProperties.secret()); - String refreshTokenKey = TokenType.REFRESH.addPrefix(subject); - String foundRefreshToken = redisTemplate.opsForValue().get(refreshTokenKey); - return Objects.equals(requestedRefreshToken, foundRefreshToken); + public boolean isValidRefreshToken(String requestedRefreshToken) { + try { + String subject = JwtUtils.parseSubject(requestedRefreshToken, jwtProperties.secret()); + String refreshTokenKey = TokenType.REFRESH.addPrefix(subject); + String foundRefreshToken = redisTemplate.opsForValue().get(refreshTokenKey); + return Objects.equals(requestedRefreshToken, foundRefreshToken); + } catch (io.jsonwebtoken.JwtException | IllegalArgumentException e) { + return false; + } }[ flag_critical_issue ]
57-62: 2) 블랙리스트 확인 로직도 동일한 예외 처리 필요
isTokenBlacklisted()역시 JWT 파싱 오류로 인해 필터 단계에서 500 오류를 유발할 수 있습니다. 위와 동일한 방식으로 try-catch 후false반환을 권장합니다.
[ suggest_essential_refactor ]
69-71: 3)toSubject()메서드 위치 재고
SiteUser→Subject변환은 Domain Converter 성격에 가깝습니다. 별도SubjectMapper유틸 클래스로 분리하면AuthTokenProvider의 역할(토큰 발급/검증)과 명확히 구분되어 SRP 를 지킬 수 있습니다.
[ suggest_optional_refactor ]
|
그림까지 있어서 이해가 더 잘되네요. 그림은 PPT로 그리신건가요? 저는 스프링 지식이 부족해 PR을 조금 더 잘게 자르면 더 이해가 잘 될 것 같다고 생각했어요🙃 (ex: 토큰객체 생성/DIP 구현) |
| throw new CustomException(ErrorCode.AUTHENTICATION_FAILED, "토큰이 없습니다."); | ||
| } | ||
| authService.signOut(token); | ||
| authService.signOut(accessToken); |
There was a problem hiding this comment.
현재 로그아웃시에 refreshToken은 무효화가 안되고 있는 것으로 보았는데 맞을까요?
subject가 제가 알기로 userId로 알고있는데, 위와 같은 경우에 TO-BE 방식은 key가 중복되서 1시간 내에 2번 로그아웃할 경우에 문제가 발생할 수 있을 것 같네요 |
| public void addToBlacklist(AccessToken accessToken) { | ||
| String blackListKey = TokenType.BLACKLIST.addPrefix(accessToken.token()); | ||
| redisTemplate.opsForValue().set(blackListKey, "signOut"); |
There was a problem hiding this comment.
놓친부분 짚어주셔서 감사합니다 🥹
엑세스 토큰자체를 저장하도록 바꿨습니다!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/main/java/com/example/solidconnection/auth/service/AuthTokenProvider.java (3)
40-50: 리프레시 토큰 검증 로직 개선기존 Optional 반환 방식에서 boolean 값을 반환하도록 변경한 것이 좋습니다! 이렇게 하면 API가 더 명확해지고, 호출하는 측에서 사용하기 쉬워집니다.
메서드 내부 로직에서 몇 가지 개선 사항을 제안합니다:
- JwtUtils.parseSubject가 실패할 경우 예외 처리를 문서화하면 좋겠습니다.
- foundRefreshToken이 null일 경우에 대한 처리가 Objects.equals로 잘 되어 있습니다.
리프레시 토큰 유효성 검증 시 주석에 검증 단계를 명확히 설명하면 더 좋을 것 같습니다:
/* * 유효한 리프레시 토큰인지 확인한다. * - 요청된 토큰과 같은 subject 의 리프레시 토큰을 조회한다. * - 조회된 리프레시 토큰과 요청된 토큰이 같은지 비교한다. +* - 토큰이 만료되었거나 형식이 잘못된 경우 JwtUtils.parseSubject에서 예외가 발생합니다. */
58-61: 토큰에서 Subject 추출 로직 캡슐화문자열 토큰으로부터 Subject 객체를 추출하는 로직을 캡슐화한 것이 좋습니다. 이로써 토큰 처리 로직의 재사용성이 높아지고 코드 중복이 줄어듭니다.
parseSubject 메서드에 대한 간단한 JavaDoc을 추가하면 더 좋을 것 같습니다:
+/** +* 토큰 문자열에서 Subject 객체를 추출합니다. +* @param token 파싱할 JWT 토큰 +* @return 토큰에서 추출한 subject를 담고 있는 Subject 객체 +* @throws CustomException 토큰이 유효하지 않을 경우 +*/ public Subject parseSubject(String token) { String subject = JwtUtils.parseSubject(token, jwtProperties.secret()); return new Subject(subject); }
63-65: SiteUser에서 Subject 변환 로직 추가SiteUser 엔티티에서 Subject 객체로 변환하는 메서드를 추가한 것이 좋습니다. 이는 객체 지향적인 접근법으로, 문자열 대신 타입 안전한 객체를 사용함으로써 코드의 안정성과 가독성을 높입니다.
여기서도 간단한 JavaDoc을 추가하면 메서드의 용도와 반환값에 대한 이해가 더 쉬워질 것 같습니다.
+/** +* SiteUser 엔티티로부터 Subject 객체를 생성합니다. +* @param siteUser 사용자 엔티티 +* @return 사용자 ID를 기반으로 한 Subject 객체 +*/ public Subject toSubject(SiteUser siteUser) { return new Subject(siteUser.getId().toString()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/example/solidconnection/auth/service/AuthTokenProvider.java(1 hunks)src/test/java/com/example/solidconnection/auth/service/AuthTokenProviderTest.java(1 hunks)src/test/java/com/example/solidconnection/custom/security/filter/SignOutCheckFilterTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/java/com/example/solidconnection/custom/security/filter/SignOutCheckFilterTest.java
- src/test/java/com/example/solidconnection/auth/service/AuthTokenProviderTest.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/com/example/solidconnection/auth/service/AuthTokenProvider.java (2)
src/main/java/com/example/solidconnection/custom/security/filter/SignOutCheckFilter.java (1)
Component(19-39)src/main/java/com/example/solidconnection/util/JwtUtils.java (1)
Component(14-68)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (6)
src/main/java/com/example/solidconnection/auth/service/AuthTokenProvider.java (6)
6-6: 자바 유틸리티 클래스를 활용하는 깔끔한 임포트 추가좋은 리팩토링 방향이네요! JwtUtils와 Objects 클래스를 활용하여 코드를 더 깔끔하게 정리하셨습니다. 특히 Objects.equals()를 사용하면 NullPointerException을 방지할 수 있어 안전한 코드 작성에 도움이 됩니다.
Also applies to: 10-10
13-13: BlacklistChecker 인터페이스 구현으로 DIP 준수클래스가 BlacklistChecker 인터페이스를 구현하도록 변경한 점이 좋습니다. 이는 PR 목표에서 언급된 의존성 역전 원칙(DIP)을 준수하여 SignOutCheckFilter가 구체 클래스에 의존하지 않고 인터페이스에 의존하게 만드는 변경사항입니다. 이렇게 하면 결합도를 낮추고 테스트 용이성을 높일 수 있습니다.
19-22: AccessToken 객체 생성 로직 개선문자열 기반 토큰에서 객체 지향적인 접근으로 변경한 것이 좋습니다! 이제 AccessToken 객체가 Subject와 토큰 문자열을 함께 캡슐화하여 관리함으로써 타입 안전성이 향상되었습니다.
24-28: RefreshToken 생성 및 저장 로직 통합RefreshToken을 생성하고 바로 저장하는 방식으로 변경하여 책임을 명확히 하였습니다. 이전에는 토큰 생성과 저장이 분리되어 있었을 가능성이 높은데, 이를 하나의 메서드로 통합함으로써 사용자 측에서의 실수 가능성을 줄였습니다.
30-38: blacklist 관리 방식에 대한 고려 필요블랙리스트 저장 방식에 주의가 필요합니다. PR 코멘트에서 언급된 문제점을 검토해보세요:
- 현재 구현: BLACKLIST:{accessToken}을 키로 사용
- 기존 우려사항: 같은 사용자가 1시간 내에 두 번 로그아웃하면 키가 중복될 수 있음
현재 키 값으로 토큰 자체를 사용하고 있어 각 토큰마다 별도의 블랙리스트 항목이 생성되므로 이 문제는 발생하지 않을 것 같습니다. 그러나 이 설계가 PR 설명에서 언급한 "BLACKLIST:{subject}"를 키로 사용하는 방식과 일치하는지 확인해보세요.
52-56: blacklist 확인 로직의 명확한 인터페이스 구현BlacklistChecker 인터페이스를 구현하여 블랙리스트 토큰 확인 로직을 캡슐화했습니다. 이는 SignOutCheckFilter와의 결합도를 낮추는 좋은 접근법입니다.
메서드 이름이 isTokenBlacklisted로 변경된 것도 기존의 findBlackListToken보다 명확하고 의미가 직관적입니다.
Gyuhyeok99
left a comment
Gyuhyeok99
left a comment
There was a problem hiding this comment.
고생하셨습니다! 전반적으로 코드가 아주 깔끔해졌네요! 전 AuthTokenProvider 대신 TokenProvider 주입받는 것만 생각했는데 BlacklistChecker를 주입받는 거로 하니 훨씬 의도가 분명해지네요! 좋은 거 같습니다!
4/ 블랙리스트 저장 방법 변경 (!피드백 필요!)
관련은 위백님이 잘 말씀해주신 거 같습니다!
궁금한 점은 밑에 리뷰 남겨놨습니다! 추가로 로그아웃 시에는 @Authorizeduser 대신 Authentication authentication로 받는 이유는 디비 접근 최소화를 위해 그랬던건거였죠?
| * */ | ||
| public ReissueResponse reissue(ReissueRequest reissueRequest) { | ||
| // 리프레시 토큰 확인 | ||
| String requestedRefreshToken = reissueRequest.refreshToken(); |
There was a problem hiding this comment.
토큰 객체를 만들었는데 결국 바디로 String refreshToken을 받아서 사용하고 있는게 쪼끔 일관성이 떨어지는 거 같다는 생각이 들었는데 나중 pr에서 클라이언트는 그대로
{
"refreshToken": "refresh-token-string"
}
로 보내고 ArgumentResolver를 활용해서 서버에선 RefreshToken refreshToken으로 받도록 하는건 어떤가요?
작성해놓고보니 좀 비효율적인 거 같다는 생각도 드네요 이건 😅
| ) { | ||
| String token = authentication.getCredentials().toString(); | ||
| if (token == null) { | ||
| if (!(authentication.getCredentials() instanceof AccessToken accessToken)) { // null or AccessToken 로 형변환 실패 |
There was a problem hiding this comment.
authentication.getCredentials() 여기에는 String type의 엑세스 토큰이 들어있지 않나요?
지금 방식이면 모든 토큰이 여기서 걸려서 예외가 터질 거 같아서요!
한 번 로그 찍어봤는데
Credentials Type: java.lang.String
Is String: true
Is AccessToken: false
이러고 바로 커스텀 예외 터지네요!
There was a problem hiding this comment.
간단하게 그냥
Object credentials = authentication.getCredentials();
System.out.println("Credentials Type: " + (credentials != null ? credentials.getClass().getName() : "null"));
System.out.println("Is String: " + (credentials instanceof String));
System.out.println("Is AccessToken: " + (credentials instanceof AccessToken));
if (!(authentication.getCredentials() instanceof AccessToken accessToken)) { // null or AccessToken 로 형변환 실패
throw new CustomException(ErrorCode.AUTHENTICATION_FAILED, "토큰이 없습니다.");
}
이렇게 찍어봤었어요
There was a problem hiding this comment.
제가 커밋은 했었는데 푸쉬를 안했었네요😭
꼼꼼한 확인 감사합니다!
There was a problem hiding this comment.
이 코드에 대해 추가 설명드리자면,
이 코드는 제가 “시큐리티에서 사용되는 JwtAuthentication 자체가 AccessToken 을 가지고 있게 하면 어떻게 될까?”
를 생각하며 작성했던 코드입니다. 😅
그런데 그렇게 하지 않은 이유는,
filter 레이어가 auth의 도메인격인 AccessToken을 직접 참조하는 것은 좋은 구조가 아니라고 판단했기 때문입니다.
(물론 지금도 JwtAuthentication가 SiteUser를 직접 참조하고있긴 한데, 짜피 이건 리팩러팅 대상이니깐요🤫 #299)
nayonsoso
left a comment
nayonsoso
left a comment
There was a problem hiding this comment.
토큰 객체를 만들었는데 결국 바디로 String refreshToken을 받아서 사용하고 있는게 쪼끔 일관성이 떨어지는 거 같다는 생각이 들었는데
사실 저도 그 생각을 했었었는데요😅
그런데 ArgumentResolver로 만들기에는, AccessToken이나 RefreshToken이 사용되는 위치가 auth 패키지 뿐이라 중복 코드를 크게 개선하진 못하겠더라고요.
ArgumentResolver를 사용하는건 너무 큰 리팩터링이라 생각했어요.
더 중복이 생기면 우선은 private 메서드로 만들어보고,
이후에 다른 패키지에서도 중복이 생기면 ArgumentResolver를 고려해볼게요!
저도 이부분때문에 괜히 얘기한 거 같다고 말씀드렸던 거 같아요 🥲
|
3 participants
관련 이슈
작업 내용
1/ Subject, AccessToken, RefreshToken 객체 생성
어떤 종류인지에 따라서 명확하게 관리 로직이 달라지고, 토큰의 종류도 늘어날 것 같지 않아 각각 만들어주었다.
우리가 사용하는 토큰이 jwt 토큰이라는 구체적인 기술에 대한 정보를 숨기고, 자바 객체를 사용하게 하여 객체지향적인 코드로 만들었다.
2/ AuthTokenProvider의 함수 시그니처 변경
Optional<String> findRefreshToken → boolean isValidRefreshToken()Optional<String> findBlackListoken → boolean isTokenBlacklisted()3/ 스프링 시큐리티 필터가 서비스단을 의존하는 것 해결
DIP위반을 해결하기 위해, 블랙리스트 체크를 위한 인터페이스를 만들고 거기에 의존하도록 바꿨다.
4/ 블랙리스트 저장 방법 변경 (!피드백 필요!)
특이 사항
왜 사람들이 상속이나 util이 아니라 조합을 쓰라는지 확실히 알게되었습니다 👵🏻
리뷰 요구사항 (선택)