refactor: 불필요한 Authentication 상속 관계 통합 및 이름 변경 #388
Conversation
|
""" Walkthrough
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
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. 🪧 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 (
|
- '스프링 시큐리티의 용어'에 대해 주석으로 추가 설명을 한다. - 의미상 중요한 것을 위에 위치시킨다.
- JwtAuthentication -> TokenAuthentication - Jwt라는 구체 기술을 드러내지 않도록 한다.
- JwtAuthenticationFilter -> TokenAuthenticationFilter - Jwt라는 구체 기술을 드러내지 않도록 한다.
- 불필요하게 상속 관계였던 클래스를 하나로 통합한다. - ExpiredTokenAuthentication와 SiteUserAuthentication을 다형성을 이용해 처리하기 위해 상속 관계를 만들었지만, ExpiredTokenAuthentication의 삭제(solid-connection#308 PR)에 따라, 상속 관계가 필요하지 않게 되었으므로 통합한다.
- SiteUserAuthentication 테스트 코드의 내용을 토대로 작성한다.
- authentication 객체와 이를 처리하는 provider가 같은 패키지에 오도록 패키지 이동
- "외부의 구체 기술"관련 코드를 모으는 패키지의 관례적인 이름인 'infrastructure'를 사용한다.
nayonsoso
force-pushed
the
refactor/unify-unnecessary-authentication-inheritance
branch
from
b34fa59 to
3620a1a
Compare
nayonsoso
requested review from
Gyuhyeok99,
lsy1307,
whqtker and
wibaek
as code owners
![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/test/java/com/example/solidconnection/common/resolver/AuthorizedUserResolverTest.java (1)
6-6: 사용되지 않는 import 문을 정리해주세요! 🧹
assertAll에 대한 static import가 추가되었지만 현재 코드에서 사용되지 않고 있습니다. 불필요한 import는 제거하는 것이 좋겠습니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/main/java/com/example/solidconnection/security/authentication/SiteUserAuthentication.java(0 hunks)src/main/java/com/example/solidconnection/security/authentication/TokenAuthentication.java(1 hunks)src/main/java/com/example/solidconnection/security/authentication/TokenAuthenticationProvider.java(2 hunks)src/main/java/com/example/solidconnection/security/config/AuthenticationManagerConfig.java(2 hunks)src/main/java/com/example/solidconnection/security/config/SecurityConfiguration.java(3 hunks)src/main/java/com/example/solidconnection/security/filter/SignOutCheckFilter.java(1 hunks)src/main/java/com/example/solidconnection/security/filter/TokenAuthenticationFilter.java(3 hunks)src/main/java/com/example/solidconnection/security/infrastructure/AuthorizationHeaderParser.java(1 hunks)src/test/java/com/example/solidconnection/common/resolver/AuthorizedUserResolverTest.java(2 hunks)src/test/java/com/example/solidconnection/security/authentication/SiteUserAuthenticationTest.java(0 hunks)src/test/java/com/example/solidconnection/security/authentication/TokenAuthenticationProviderTest.java(5 hunks)src/test/java/com/example/solidconnection/security/authentication/TokenAuthenticationTest.java(1 hunks)src/test/java/com/example/solidconnection/security/filter/TokenAuthenticationFilterTest.java(4 hunks)src/test/java/com/example/solidconnection/security/infrastructure/AuthorizationHeaderParserTest.java(1 hunks)
💤 Files with no reviewable changes (2)
- src/main/java/com/example/solidconnection/security/authentication/SiteUserAuthentication.java
- src/test/java/com/example/solidconnection/security/authentication/SiteUserAuthenticationTest.java
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/main/java/com/example/solidconnection/security/filter/TokenAuthenticationFilter.java (1)
src/main/java/com/example/solidconnection/security/authentication/TokenAuthentication.java (1)
TokenAuthentication(7-42)
src/test/java/com/example/solidconnection/security/filter/TokenAuthenticationFilterTest.java (1)
src/main/java/com/example/solidconnection/security/authentication/TokenAuthentication.java (1)
TokenAuthentication(7-42)
src/test/java/com/example/solidconnection/security/authentication/TokenAuthenticationTest.java (1)
src/main/java/com/example/solidconnection/security/userdetails/SiteUserDetails.java (1)
SiteUserDetails(9-56)
src/test/java/com/example/solidconnection/common/resolver/AuthorizedUserResolverTest.java (1)
src/main/java/com/example/solidconnection/security/authentication/TokenAuthentication.java (1)
TokenAuthentication(7-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (33)
src/main/java/com/example/solidconnection/security/infrastructure/AuthorizationHeaderParser.java (1)
1-1: 패키지 이동이 적절합니다! 👍
AuthorizationHeaderParser를security.filter에서security.infrastructure로 이동한 것이 아키텍처 관점에서 매우 좋은 결정입니다. 이 클래스는 필터에 특화된 로직이 아니라 인프라 레벨의 토큰 파싱 유틸리티이므로, 여러 필터에서 재사용할 수 있는 인프라 컴포넌트로 분류하는 것이 맞습니다.src/test/java/com/example/solidconnection/security/infrastructure/AuthorizationHeaderParserTest.java (1)
1-1: 테스트 패키지 이동이 일관성 있게 처리되었습니다! ✅
AuthorizationHeaderParserTest를 메인 클래스와 동일한infrastructure패키지로 이동한 것이 좋습니다. 테스트 클래스는 대상 클래스와 같은 패키지 구조를 따라야 하므로 이런 일관성 있는 처리가 중요합니다.src/main/java/com/example/solidconnection/security/filter/SignOutCheckFilter.java (1)
6-6: import 문 업데이트가 정확합니다! 📦
AuthorizationHeaderParser의 패키지 이동에 따라 import 문을security.infrastructure로 업데이트한 것이 올바릅니다. 필터에서 여전히 동일한 방식으로 토큰 파싱 기능을 사용하고 있어 일관성이 유지되고 있습니다.src/test/java/com/example/solidconnection/common/resolver/AuthorizedUserResolverTest.java (1)
11-11: TokenAuthentication으로의 전환이 완벽합니다! 🔄
SiteUserAuthentication에서TokenAuthentication으로 import 변경createAuthenticationWithUser메서드에서 새로운TokenAuthentication생성자 사용리팩토링의 일관성이 잘 유지되고 있으며,
TokenAuthentication(String token, Object principal)생성자를 올바르게 사용하고 있습니다.Also applies to: 91-94
src/test/java/com/example/solidconnection/security/filter/TokenAuthenticationFilterTest.java (1)
8-8: 필터 클래스 리팩토링이 완벽하게 처리되었습니다! 🎯다음 변경사항들이 일관성 있게 적용되었습니다:
- Import 업데이트:
SiteUserAuthentication→TokenAuthentication- 클래스명 변경:
JwtAuthenticationFilterTest→TokenAuthenticationFilterTest- 필드명 변경:
jwtAuthenticationFilter→tokenAuthenticationFilter- 메서드 호출 업데이트: 모든
doFilterInternal호출이 새로운 필터 인스턴스를 사용- Assertion 타입 변경:
TokenAuthentication.class를 기대하도록 수정JWT 특화 클래스에서 일반적인 토큰 기반 인증으로의 전환이 테스트 코드에서도 완벽하게 반영되었습니다.
Also applies to: 28-31, 56-56, 71-71, 75-75
src/main/java/com/example/solidconnection/security/config/SecurityConfiguration.java (3)
8-8: 1. 임포트 변경 - JWT 특정 네이밍에서 범용 토큰 네이밍으로 전환JWT 기술에 의존적인
JwtAuthenticationFilter에서 보다 범용적인TokenAuthenticationFilter로 임포트가 변경되었습니다. 이는 PR 목표와 일치하는 좋은 변경사항입니다.
33-33: 2. 필드 주입 변경 - 필터 클래스명 일관성 유지필터 인스턴스 필드명이
jwtAuthenticationFilter에서tokenAuthenticationFilter로 변경되어 새로운 클래스명과 일관성을 유지하고 있습니다.
72-73: 3. 필터 체인 구성 업데이트 - 올바른 순서 유지필터 체인 구성이 다음과 같이 업데이트되었습니다:
-tokenAuthenticationFilter가BasicAuthenticationFilter전에 위치
-signOutCheckFilter가TokenAuthenticationFilter전에 위치필터 실행 순서가 올바르게 유지되어 인증 흐름에 문제가 없을 것으로 보입니다.
src/main/java/com/example/solidconnection/security/config/AuthenticationManagerConfig.java (3)
3-3: 1. 임포트 변경 - 인증 프로바이더 네이밍 일관성
SiteUserAuthenticationProvider에서TokenAuthenticationProvider로 임포트가 변경되어 토큰 기반 인증 접근 방식과 일관성을 유지하고 있습니다.
14-14: 2. 필드 주입 변경 - 프로바이더 인스턴스 업데이트필드명이
siteUserAuthenticationProvider에서tokenAuthenticationProvider로 변경되어 새로운 클래스명과 일치합니다.
19-19: 3. AuthenticationManager 빈 구성 업데이트
ProviderManager가 이제tokenAuthenticationProvider를 사용하도록 구성되어 리팩토링된 인증 구조와 일관성을 유지합니다.src/test/java/com/example/solidconnection/security/authentication/TokenAuthenticationProviderTest.java (3)
1-1: 1. 패키지 위치 변경 - 인증 컴포넌트 재구성테스트 클래스가
security.provider에서security.authentication패키지로 이동되어 프로바이더의 새로운 위치와 일치합니다.
28-28: 2. 클래스명 변경 - 일관된 네이밍 적용
SiteUserAuthenticationProviderTest에서TokenAuthenticationProviderTest로 클래스명이 변경되어 테스트 대상 클래스와 일관성을 유지합니다.
31-31: 3. 인증 클래스 참조 업데이트 - 전체적인 일관성테스트 전반에 걸쳐 다음과 같은 변경사항이 적용되었습니다:
-SiteUserAuthenticationProvider→TokenAuthenticationProvider
-SiteUserAuthentication→TokenAuthentication모든 테스트 메서드에서 새로운 클래스들을 올바르게 사용하고 있으며, 기존 테스트 시나리오와 로직이 그대로 유지되어 테스트 커버리지가 보장됩니다.
Also applies to: 49-49, 63-63, 82-82, 93-94, 107-107
src/main/java/com/example/solidconnection/security/filter/TokenAuthenticationFilter.java (3)
3-4: 1. 임포트 변경 - 패키지 재구성 및 네이밍 일관성두 가지 중요한 임포트 변경사항이 있습니다:
-JwtAuthentication→TokenAuthentication: JWT 특정 네이밍에서 범용 토큰 네이밍으로 전환
-AuthorizationHeaderParser패키지 이동:filter에서infrastructure로 이동하여 역할을 더 명확히 반영
22-22: 2. 클래스명 변경 - 기술 중립적 네이밍
JwtAuthenticationFilter에서TokenAuthenticationFilter로 클래스명이 변경되어 JWT 기술에 의존하지 않는 범용적인 이름으로 개선되었습니다.
37-37: 3. TokenAuthentication 생성자 사용 - 올바른 인증 플로우
TokenAuthentication이 토큰 문자열만으로 생성되어 미인증 상태로 초기화됩니다. 이는AuthenticationManager가 이후 인증 프로세스를 통해 principal을 설정하여 인증된 상태로 변환하는 올바른 플로우입니다.src/test/java/com/example/solidconnection/security/authentication/TokenAuthenticationTest.java (4)
1-14: 1. 새로운 테스트 클래스 생성 - TokenAuthentication 검증
TokenAuthentication클래스를 위한 새로운 테스트 클래스가 생성되었습니다. 패키지 위치와 클래스명이 테스트 대상과 일관성을 유지하고 있습니다.
16-47: 2. 인증 정보 반환 테스트 - 토큰 및 사용자 정보 검증인증 정보 반환 기능을 위한 테스트가 잘 구성되어 있습니다:
- 토큰 반환 테스트:getToken()메서드 동작 확인
- 사용자 정보 반환 테스트:getPrincipal()메서드로SiteUserDetails반환 확인테스트에서 객체 추출 체이닝을 사용하여 중첩된 속성을 효과적으로 검증하고 있습니다.
49-70: 3. 인증 상태 테스트 - 생성자별 인증 상태 확인
TokenAuthentication의 두 생성자에 대한 인증 상태 테스트가 포괄적으로 구성되어 있습니다:
- 토큰만으로 생성 시: 미인증 상태 (isAuthenticated() = false)
- 토큰과 사용자 정보로 생성 시: 인증된 상태 (isAuthenticated() = true)이는
TokenAuthentication클래스의 설계 의도와 완벽히 일치합니다.
72-81: 4. 테스트 헬퍼 메서드 - 테스트 데이터 생성
createSiteUser()헬퍼 메서드가 테스트에 필요한SiteUser인스턴스를 생성합니다. 모든 필수 속성이 적절한 값으로 설정되어 있어 테스트 실행에 문제가 없을 것으로 보입니다.src/main/java/com/example/solidconnection/security/authentication/TokenAuthenticationProvider.java (5)
1-1: 패키지 이동이 적절하게 반영되었습니다.
security.provider에서security.authentication으로 패키지를 이동한 것은 클래스의 역할과 책임을 더 명확하게 표현합니다.
14-14: 클래스명 변경이 일관성 있게 적용되었습니다.
SiteUserAuthenticationProvider에서TokenAuthenticationProvider로 변경하여 JWT 기술에 종속되지 않는 일반적인 토큰 기반 인증을 표현하고 있습니다.
21-22: 타입 캐스팅과 토큰 추출이 안전하게 구현되었습니다.
supports()메서드에서TokenAuthentication타입을 확인하므로 안전한 캐스팅이 보장됩니다.
26-26: 인증된 TokenAuthentication 인스턴스 생성이 올바르게 구현되었습니다.토큰과 사용자 세부정보를 모두 포함한 생성자를 사용하여 인증된 상태의
TokenAuthentication객체를 생성하고 있습니다.
31-31: supports 메서드가 새로운 인증 타입을 올바르게 지원합니다.
TokenAuthentication.class.isAssignableFrom(authentication)을 통해 타입 호환성을 정확히 확인하고 있습니다.src/main/java/com/example/solidconnection/security/authentication/TokenAuthentication.java (7)
7-7: 추상 클래스에서 구체 클래스로의 전환이 적절합니다.상속 구조를 제거하고 단일 구체 클래스로 통합하여 코드 복잡성을 줄였습니다.
9-11: 필드 순서가 Spring Security 규칙에 맞게 조정되었습니다.
principal(인증 주체)credentials(증명 수단)순서로 정렬하여 Spring Security의 일반적인 패턴을 따릅니다.
13-18: 미인증 상태 생성자가 올바르게 구현되었습니다.토큰만 받는 생성자에서는:
- 빈 권한 목록으로 초기화
- principal을 null로 설정
- credentials에 토큰 저장
- authenticated를 false로 설정이는 Spring Security의 미인증 토큰 패턴을 정확히 따릅니다.
20-27: 인증된 상태 생성자가 적절하게 구현되었습니다.토큰과 principal을 받는 생성자에서는:
- UserDetails 인스턴스인 경우 권한 목록을 추출
- 그렇지 않은 경우 빈 권한 목록 사용
- authenticated를 true로 설정이는 인증 완료 후 상태를 올바르게 표현합니다.
30-32: getPrincipal 메서드가 올바르게 구현되었습니다.필드 순서 변경에 따라 principal 필드를 반환하도록 수정되었습니다.
35-37: getCredentials 메서드가 올바르게 구현되었습니다.필드 순서 변경에 따라 credentials 필드(토큰)를 반환하도록 수정되었습니다.
39-41: getToken 편의 메서드가 일관성 있게 유지되었습니다.
getCredentials()를 String으로 캐스팅하여 반환하는 방식으로 기존 API 호환성을 유지합니다.
whqtker
left a comment
whqtker
left a comment
There was a problem hiding this comment.
적어주신 내용 모두 좋은 변경이라고 생각합니다 ! 작업 내용에 없는 간단한 리팩터링 사항 모두 동의합니다.
그 외 딱히 문제되는 부분은 찾지 못했고, 궁금한 점만 코멘트 남겼습니다. 고생하셨습니다 !! 👍
혹시 컨트롤러에서 SiteUser 를 인자로 받지 않도록 하는 부분은 추가로 진행하시는 건가요 ?
| ExchangeStatus.CONSIDERING, | ||
| Role.MENTEE | ||
| ); | ||
| } |
There was a problem hiding this comment.
siteUserFixture 를 통해 사용자를 생성하지 않고 따로 메서드를 구현하신 이유가 있나요 ?
There was a problem hiding this comment.
아니 이게 이렇게 나오네요 ... createSiteUser 메서드입니다
There was a problem hiding this comment.
혹시 컨트롤러에서 SiteUser 를 인자로 받지 않도록 하는 부분은 추가로 진행하시는 건가요 ?
네, 이 이후에 바로 진행하려 했습니다! file change가 너무 커질 것 같아서요😅
siteUserFixture 를 통해 사용자를 생성하지 않고 따로 메서드를 구현하신 이유가 있나요 ?
siteUserFixture를 사용하려면 빈 주입이 필요합니다.
그럼 이 테스트는 TokenAuthentication라는 POJO에 대한 단위 테스트가 아니라, SpringBootTest로 바뀌어야 합니다.
빈 주입을 위해서요.
SiteUser 를 생성하는 것은 이 테스트의 핵심 관심사가 아닌데,
그것을 위해 단위 테스트가 가능한 상태에서 SpringBootTest로 바뀌는게 이상하다고 생각했습니다
There was a problem hiding this comment.
오 ... 그렇군요 지금보니 POJO는 전부 @TestContainerSpringBootTest 어노테이션이 없네요
이해했습니다 !
Gyuhyeok99
left a comment
Gyuhyeok99
left a comment
There was a problem hiding this comment.
미루고 미뤘던 작업이 하나 끝났네요 😊
고생하셨습니다!!
| package com.example.solidconnection.security.infrastructure; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
| import static org.junit.jupiter.api.Assertions.assertAll; |
There was a problem hiding this comment.
AuthorizationHeaderParserTest에 @DisplayName이 빠져있던데 이거만 추가해주실 수 있나요
사실 다른 몇몇 곳에도 안붙어 있긴 합니다 😅
4 participants
관련 이슈
작업 내용
과거에는 Jwt 토큰으로 요청할 수 있는 api들이
"정상 토큰으로 호출하는 api"와 "토큰이 만료되었으나 호출할 수 있는 api"가 모두 존재했습니다.
두가지 api 를 다형성을 활용해, 중복 코드 없이 처리하기 위해
JwtAuthentication (부모) - ExpiredTokenAuthentication(자식1) / SiteUserAuthentication(자식2)
와 같은 상속 관계를 만들어주었습니다.
(cf. 스프링 시큐리팅는 사용자의 인증 정보를 Authentication에 담아 관리합니다)
그런데 후자가 (#308 PR)에서 제거되어, 더 이상 이런 상속 관계가 필요하지 않게 되었습니다.
따라서 JwtAuthentication와 SiteUserAuthentication을 통합하여 하나의 클래스로 만듭니다.
또한 기존의 JwtAuthentication라는 이름은 "Jwt"라는 구체 기술을 드러내므로,
TokenAuthentication 으로 이름을 변경했습니다.
비슷한 맥락으로 Bearer 토큰을 추출하는 클래스 AuthorizationHeaderParser도 구체 기술에 대한 클래스라 생각하여,
security.infrastructure 패키지 하위로 옮겨주었습니다.