-
Notifications
You must be signed in to change notification settings - Fork 0
[FEAT] jwt 작성 #7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughJWT 기반 인증 도입: 토큰 생성·발급·검증 컴포넌트(Generator/Provider/Validator/KeyProvider), 필터·엔트리포인트·접근거부 핸들러 추가, SecurityConfig에 필터/예외처리 통합, 오류 코드 확장 및 빌드 의존성(jjwt) 추가. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Filter as JwtAuthenticationFilter
participant Provider as JwtProvider
participant Validator as JwtValidator
participant KeyProv as KeyProvider
participant SecCtx as SecurityContext
participant Chain as FilterChain
Client->>Filter: HTTP 요청 (Authorization: Bearer <token>)
activate Filter
Filter->>Filter: resolveToken()
alt 토큰 존재
Filter->>Provider: getAuthentication(token)
activate Provider
Provider->>Validator: validateAndParseAccessToken(token)
activate Validator
Validator->>KeyProv: getSigningKey()
activate KeyProv
KeyProv-->>Validator: signing key
deactivate KeyProv
Validator-->>Provider: claims / 예외
deactivate Validator
Provider->>Provider: claims 검증 -> Authentication 생성
Provider-->>Filter: Authentication / 예외
deactivate Provider
alt 인증 성공
Filter->>SecCtx: setAuthentication(auth)
else 인증 실패
Filter->>SecCtx: clearContext()
Filter->>Filter: request.setAttribute(EXCEPTION_KEY, ex)
end
else 토큰 없음
Filter->>SecCtx: clearContext()
end
Filter->>Chain: doFilter(request, response)
deactivate Filter
sequenceDiagram
autonumber
participant Client
participant Controller as AuthController
participant Generator as JwtGenerator
participant KeyProv as KeyProvider
participant Store as TokenStore
Client->>Controller: 로그인 요청 (자격증명)
Controller->>Generator: generateAccessToken(userId, roles)
activate Generator
Generator->>KeyProv: getSigningKey()
KeyProv-->>Generator: signing key
Generator-->>Controller: GeneratedToken(access, expire)
deactivate Generator
Controller->>Generator: generateRefreshToken()
activate Generator
Generator->>KeyProv: getSigningKey()
KeyProv-->>Generator: signing key
Generator-->>Controller: GeneratedToken(refresh, expire)
deactivate Generator
Controller->>Store: refresh 토큰 저장
Controller-->>Client: TokenDto 반환 (access, refresh, 만료시간)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Nitpick comments (6)
src/main/kotlin/org/appjam/smashing/global/exception/ErrorCode.kt (1)
21-25: JWT 인증 관련 에러 코드 추가 확인에러 코드들이 적절하게 정의되어 있습니다. 다만, 주석이
// Member로 되어 있는데, 해당 에러 코드들은 인증(AUTH) 관련이므로// Auth또는// JWT로 변경하는 것이 더 명확할 것 같습니다.src/main/kotlin/org/appjam/smashing/domain/auth/jwt/Token.kt (1)
7-12: 팩토리 메서드가 불필요합니다.
Token.of(accessToken, refreshToken)는Token(accessToken, refreshToken)와 동일한 기능을 합니다. Kotlin data class는 이미 명확한 생성자를 제공하므로, 추가적인 로직이 없다면 팩토리 메서드를 제거하고 직접 생성자를 사용하는 것이 더 간결합니다.🔎 권장 수정안
data class Token( val accessToken: String, val refreshToken: String, -) { - companion object { - fun of( - accessToken: String, - refreshToken: String, - ) = Token(accessToken, refreshToken) - } -} +)JwtProvider.kt에서도 변경이 필요합니다:
- fun issueToken(userId: Long): Token = Token.of( + fun issueToken(userId: Long): Token = Token( jwtGenerator.generateToken(userId, TokenType.ACCESS_TOKEN), jwtGenerator.generateToken(userId, TokenType.REFRESH_TOKEN) )src/main/kotlin/org/appjam/smashing/domain/auth/jwt/JwtProperties.kt (1)
6-12: 설정 바인딩 방식 개선을 권장합니다.현재
@Component와@ConfigurationProperties를 함께 사용하고 있는데, Spring Boot 2.2+에서는@ConfigurationPropertiesScan또는@EnableConfigurationProperties를 사용하는 것이 더 권장됩니다.또한,
accessTokenExpireTime과refreshTokenExpireTime의 기본값이0이므로, 설정이 누락되면 토큰이 즉시 만료됩니다. 애플리케이션 시작 시 유효성 검증을 추가하는 것을 권장합니다.🔎 권장 수정안
package org.appjam.smashing.domain.auth.jwt +import jakarta.annotation.PostConstruct import org.springframework.boot.context.properties.ConfigurationProperties -import org.springframework.stereotype.Component -@Component @ConfigurationProperties(prefix = "jwt") class JwtProperties { lateinit var secret: String var accessTokenExpireTime: Long = 0 var refreshTokenExpireTime: Long = 0 + + @PostConstruct + fun validate() { + require(secret.isNotBlank()) { "JWT secret must not be blank" } + require(accessTokenExpireTime > 0) { "accessTokenExpireTime must be positive" } + require(refreshTokenExpireTime > 0) { "refreshTokenExpireTime must be positive" } + } }그리고 메인 애플리케이션 클래스 또는 설정 클래스에 추가:
@EnableConfigurationProperties(JwtProperties::class)src/main/kotlin/org/appjam/smashing/domain/auth/jwt/JwtValidator.kt (2)
27-31: JwtParser 캐싱을 권장합니다.
getJwtParser()가 호출될 때마다 새로운JwtParser인스턴스를 생성합니다.JwtParser는 thread-safe하므로 한 번 생성하여 재사용하는 것이 성능상 유리합니다.🔎 권장 수정안
@Component class JwtValidator( private val keyProvider: KeyProvider ) { + private val jwtParser: JwtParser by lazy { + Jwts.parserBuilder() + .setSigningKey(keyProvider.getSigningKey()) + .build() + } + fun validateRefreshToken(refreshToken: String, storedRefreshToken: String) { if (refreshToken != storedRefreshToken) { throw CustomException(ErrorCode.INVALID_REFRESH_TOKEN) } } fun parseToken(token: String): Jws<Claims> = try { - getJwtParser().parseClaimsJws(token) + jwtParser.parseClaimsJws(token) } catch (e: ExpiredJwtException) { throw CustomException(ErrorCode.EXPIRED_ACCESS_TOKEN) } catch (e: Exception) { throw CustomException(ErrorCode.INVALID_ACCESS_TOKEN) } - - private fun getJwtParser(): JwtParser = - Jwts.parserBuilder() - .setSigningKey(keyProvider.getSigningKey()) - .build() }
23-24: 일반 Exception 캐치 시 로깅 추가를 권장합니다.
Exception을 일괄적으로 catch하면 예상치 못한 에러(예:NullPointerException,IllegalStateException)가INVALID_ACCESS_TOKEN으로 처리되어 디버깅이 어려울 수 있습니다. 최소한 로깅을 추가하는 것을 권장합니다.🔎 권장 수정안
+import org.slf4j.LoggerFactory + @Component class JwtValidator( private val keyProvider: KeyProvider ) { + private val logger = LoggerFactory.getLogger(JwtValidator::class.java) + // ... fun parseToken(token: String): Jws<Claims> = try { getJwtParser().parseClaimsJws(token) } catch (e: ExpiredJwtException) { throw CustomException(ErrorCode.EXPIRED_ACCESS_TOKEN) } catch (e: Exception) { + logger.warn("Token parsing failed", e) throw CustomException(ErrorCode.INVALID_ACCESS_TOKEN) }src/main/kotlin/org/appjam/smashing/domain/auth/filter/JwtAuthenticationFilter.kt (1)
43-50: 빈 권한 목록 사용에 대한 검토 필요
emptyList()로 권한을 설정하면 향후 역할 기반 접근 제어(RBAC)를 적용하기 어려울 수 있습니다. 현재 요구사항에 역할/권한이 필요 없다면 괜찮지만, 추후 확장 가능성을 고려해주세요.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
build.gradle.ktssrc/main/kotlin/org/appjam/smashing/domain/auth/filter/JwtAuthenticationFilter.ktsrc/main/kotlin/org/appjam/smashing/domain/auth/jwt/JwtGenerator.ktsrc/main/kotlin/org/appjam/smashing/domain/auth/jwt/JwtProperties.ktsrc/main/kotlin/org/appjam/smashing/domain/auth/jwt/JwtProvider.ktsrc/main/kotlin/org/appjam/smashing/domain/auth/jwt/JwtValidator.ktsrc/main/kotlin/org/appjam/smashing/domain/auth/jwt/KeyProvider.ktsrc/main/kotlin/org/appjam/smashing/domain/auth/jwt/Token.ktsrc/main/kotlin/org/appjam/smashing/domain/auth/jwt/TokenType.ktsrc/main/kotlin/org/appjam/smashing/global/config/SecurityConfig.ktsrc/main/kotlin/org/appjam/smashing/global/exception/ErrorCode.kt
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/kotlin/org/appjam/smashing/domain/auth/filter/JwtAuthenticationFilter.kt (1)
src/main/kotlin/org/appjam/smashing/SmashingApplication.kt (1)
@SpringBootApplication(7-9)
src/main/kotlin/org/appjam/smashing/global/exception/ErrorCode.kt (1)
src/main/kotlin/org/appjam/smashing/global/exception/CustomException.kt (1)
errorCode(3-6)
🔇 Additional comments (8)
src/main/kotlin/org/appjam/smashing/domain/auth/jwt/TokenType.kt (1)
1-5: LGTM!토큰 타입을 명확하게 구분하는 간결한 enum 구현입니다.
src/main/kotlin/org/appjam/smashing/domain/auth/jwt/JwtGenerator.kt (1)
14-28: LGTM!JWT 토큰 생성 로직이 적절하게 구현되어 있습니다.
subject,issuedAt,expiration을 포함하고 HS256 알고리즘으로 서명하는 표준적인 구현입니다.선택적으로,
issuer클레임(iss)을 추가하면 토큰의 출처를 검증하는 데 도움이 될 수 있습니다.src/main/kotlin/org/appjam/smashing/domain/auth/jwt/JwtProvider.kt (1)
19-25: LGTM! 코드 구현이 적절합니다.
getUserId()로직이 올바르게 구현되어 있습니다. 현재 프로젝트에서 사용 중인 jjwt 버전은 0.11.5이며, 이 버전에서는jws.body가 표준 API입니다.jws.body의 deprecation은 jjwt 0.12.x 이후에 적용되므로, 현재 환경에서는 문제가 없습니다.src/main/kotlin/org/appjam/smashing/domain/auth/filter/JwtAuthenticationFilter.kt (2)
35-41: LGTM!
resolveAccessToken메서드의 구현이 적절합니다. null 헤더 처리, prefix 검증, 공백 제거 및 빈 문자열 체크가 잘 되어 있습니다.
52-54: LGTM!상수를 companion object에서 관리하는 것은 Kotlin의 관용적인 패턴입니다.
src/main/kotlin/org/appjam/smashing/global/config/SecurityConfig.kt (3)
39-44:anyRequestpermitAll 설정으로 인해 JWT 필터가 실질적으로 선택적 동작현재
anyRequest가permitAll로 설정되어 있어 JWT 필터가 인증 정보를 설정하더라도 실제로 인증이 강제되지 않습니다. TODO 주석처럼 추후authenticated로 변경이 필요합니다.JWT 필터와 함께 동작하려면 다음과 같이 수정해야 합니다:
authorize(anyRequest, authenticated)현재 개발 단계라면 괜찮지만, 프로덕션 전에 반드시 수정이 필요합니다.
61-65:/api/v1/member/profile엔드포인트가 PERMIT_ALL에 포함된 것이 의도된 것인지 확인 필요일반적으로 프로필 조회 엔드포인트는 인증된 사용자만 접근 가능해야 합니다. 이 엔드포인트가 공개 API로 설계된 것이 맞는지 확인해주세요.
만약 로그인 후 프로필 조회라면 PERMIT_ALL에서 제거하는 것이 적절합니다.
19-21: LGTM!
JwtProvider를 생성자 주입 방식으로 받아JwtAuthenticationFilter에 전달하는 구조가 적절합니다.addFilterBefore로UsernamePasswordAuthenticationFilter앞에 JWT 필터를 배치한 것도 올바른 위치입니다.Also applies to: 42-44
src/main/kotlin/org/appjam/smashing/domain/auth/filter/JwtAuthenticationFilter.kt
Show resolved
Hide resolved
src/main/kotlin/org/appjam/smashing/domain/auth/jwt/KeyProvider.kt
Outdated
Show resolved
Hide resolved
kyoooooong
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
기한이 촉박해서 힘들었을텐데, 역시 나의 짱서버 너무너무 수고 많았습니다!
항상... 난 참 복이 많은 사람이구나....
제 인생에 감사하고 있습니다....
| "/actuator/**", | ||
|
|
||
| // API | ||
| "/api/v1/member/login", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
혹시 인증/인가 관련된 api의 엔드포인트의 경우 api/v1/auth/로 분리하면 어떨까요? 이 경우 둘다 각각의 장단점이 있을 것같아서 유빈님의 의견이 궁금합니다 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
인증,인가 관련 api는 auth로 두는 게 좋을 것 같네요! 이건 수정해두도록 하겠습니다!
|
|
||
| class JwtAuthenticationFilter( | ||
| private val jwtProvider: JwtProvider | ||
| ) : OncePerRequestFilter() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오호 OncePerRequestFilter() 상속 너무너무 좋습니다 👏
| ) { | ||
| val token = resolveAccessToken(request) | ||
|
|
||
| if (token == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
token이 null일 경우를 대비하신 점 너무 좋습니다! 다만 지금 로직의 경우, token이 있으면 바로 getuserid()를 통해 가져오고 있는데, 이 경우 만료 등의 예외가 날 시 별도의 처리가 되지 않아 있어 500이 발생할 수 있습니다. 따라서 try/catch로 잡아서 SecurityContext clear 후, 인증 실패 흐름(401)로 보내는 부분이 필요할 것같아요.
필터에서 response를 바로 내려 체인을 중단하는 방법도 사용할 수 있지만, entrypoint에서 처리할 수 있도록 진행하시는 부분도 추천드리고 싶습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 살짝 헷갈리는 부분이 현재 Validator에서 CustomException을 터트리고 있지만 아직 컨트롤러를 들어가기 전이라 GlobalExceptionHandler에서 잡지 못한다는 뜻 맞을까요..?
그러면 entrypoint에서 처리할 수 있도록 수정하겠습니다!
덕분에 알아가요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8e7cc90 보시면 제가 구현한 부분 확인하실 수 있어요!
참고로 필터 영역에서 ResponseEntity를 반환하면 직렬화에 실패할 수 있다고 하여 ApiResponse에 errorBody 함수를 추가로 구현하였는데 괜찮을지 확인 부탁드려요!
fun errorBody(errorCode: ErrorCode): ApiResponse<Unit> =
ApiResponse(
status = Status.ERROR,
statusCode = errorCode.httpStatus.value(),
message = errorCode.message,
errorCode = errorCode.errorCode,
errorName = errorCode.name,
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋은 코드 너무너무 잘봤습니다!
그런데 지금 현재 코드를 보면, ExceptionHandlerFilter와 JwtAuthenticationEntryPoint가 모두 JSON 응답을 직접 작성하고 있어 인증 실패 응답 흐름이 이중화될 수 있을 것같아요.
필터는 응답을 쓰지 않고(request attribute로 예외만 전달), 최종 응답은 EntryPoint/AccessDeniedHandler에서 HandlerExceptionResolver로 GlobalExceptionHandler를 태워 포맷을 한 군데로 통일하는 방식으로 가져가는 방식으로 정리해 에러 응답 생성 지점을 하나로 정리하면 이후에 유지보수/디버깅이 훨씬 쉬워질 수 있을 것 같습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이런 방법이 있었군요!!!!!! 많이 배워갑니다...
HandlerExceptionResolve를 사용하는 방향으로 수정했습니다!
| return header.substring(prefix.length).trim().takeIf { it.isNotBlank() } | ||
| } | ||
|
|
||
| private fun setAuthentication(userId: Long) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 principal이 단순 userId라 요청 처리 중 사용자 부가정보(권한, timezone 등)가 필요할 때 매번 DB 조회가 강제될 수 있을 것같습니다! CustomUserDetails을 도입하고 해당 함수를 필터의 프라이빗 함수가 아니라, provider의 내용으로 분리해 provider에서 getAuthentication() 으로 authentication 정보를 가져오면 ,그것으로 securitycontextholder에 인증 정보를 설정하는 방향으로 분리하는 것도 좋을 것같아요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오오 생각치 못한 방법이네요!! 좋은 것 같아요! a836a84에 구현해뒀습니당
아직 다른 부가 정보들은 넣지 않았는데 나중에 필요해지면 추가하는 방향으로 갈게요..!!
| filterChain.doFilter(request, response) | ||
| } | ||
|
|
||
| private fun resolveAccessToken(request: HttpServletRequest): String? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 함수 네이밍을 resolveToken 으로 바꾸면 어떨까요? access/refresh 구분은 provider 쪽 책임으로 보여서, 이 함수는 단순히 header에서 token 문자열을 추출하는 역할이라 resolveToken 네이밍이 더 명확할 수 있을 것같아요
| @Component | ||
| @ConfigurationProperties(prefix = "jwt") | ||
| class JwtProperties { | ||
| lateinit var secret: String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
혹시 이부분 lateinit 으로 하고 각각 기본값 0 을 주신 이유가 있으실까요? 궁급합니당
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이렇게 뒀던 이유는 값 주입 시점이 객체 생성 이후인데 String은 null일 수 없기 때문에 var secret: String으로만 두먼 컴파일 에러가 발생하여 lateinit을 넣어줬었습니다!
또한 나머지 변수들을 기본값 0으로 둔 이유는 Long타입은 lateinit 선언이 불가하기 때문에 0으로 우선 넣어두고 나중에 yml파일에서 값을 덮어씌우려고 했습니다!
근데 다시 생각을 해보니..... data class를 이용해서 넣으면 초기화되지 않은 상태로 객체가 생성되는 걸 막을 수 있을 것 같아서 그렇게 바꿔도 괜찮을 것 같네욤..
@ConfigurationProperties(prefix = "jwt")
data class JwtProperties(
val secret: String,
val accessTokenExpireTime: Long,
val refreshTokenExpireTime: Long,
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오! 말씀해주신 방향 너무 좋은 것같아요. @ConfigurationProperties를 data class로 두고 넣어줘야하는 필수값들을 기본값 0이 아닌 주입해서 넣어주는 방식으로 하면 그냥 class를 생성자 주입으로 쓰는 것보다 의도가 더 드러날 수 있어서 좋은 방식인 것같습니다
| private val jwtValidator: JwtValidator, | ||
| ) { | ||
| fun issueToken(userId: Long): Token = Token.of( | ||
| jwtGenerator.generateToken(userId, TokenType.ACCESS_TOKEN), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
말씀드린 것처럼 access / refresh는 역할이 달라서, TokenType enum으로 분기하기보다는 전용 생성 메서드로 로직을 분리하는 방식이 더 명확할 수 있을 것같아요! (access / refresh 생성 책임이 코드상에서 바로 드러나는 장점이 있을 것 같습니다)
| } | ||
| } | ||
|
|
||
| fun parseToken(token: String): Jws<Claims> = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refresh 검증을 분리한 점 너무 좋습니다! 👍
다만 parseToken()이 AccessToken을 파싱하여 Claims를 반환하는 것에 목적이 있는지, 아니면 AccessToken의 유효성 검증에 목적이 있는지 역할이 조금 애매해 보입니다. 두 부분의 기능을 분리하고, AccessToken의 유효성 검증에서는 더 많은 예외를 각각 알맞게 분리해주어 만료, 위조, 보안 예외 등을 모두 각각 처리해주어야할 것같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
정말 예리하시네요....👍
제가 두 역할을 분리하려고 아래처럼 작성해보았는데요!
// 유효성 검증
fun validateAccessToken(token: String) {
try { getJwtParser().parseClaimsJws(token) }
catch (e: ExpiredJwtException) { }
}
// 파싱하여 반환
fun parseClaims(token: String): Claims {
return getJwtParser()
.parseClaimsJws(token)
.body
}이렇게 되면 같은 로직을 두 번 반복하는 것 같더라구요
그래서 차라리 하나로 합치고 함수명을 상세하게 적는 건 어떨까 제안해봅니다..! f741f9c
fun validateAndParseAccessToken(token: String): Claims =
try {
getJwtParser().parseClaimsJws(token).body
} catch (e: ExpiredJwtException) {
throw CustomException(ErrorCode.EXPIRED_ACCESS_TOKEN)
} |
|
||
| /* ========== 도메인 ========== */ | ||
| // Member | ||
| INVALID_REFRESH_TOKEN(HttpStatus.UNAUTHORIZED, "AUTH", "유효하지 않은 리프레시 토큰입니다."), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
도메인의 에러코드의 경우 각각 다르게 auth1과 같이 코드를 따로 만들어주면 좋을 것같아요!
| class KeyProvider( | ||
| private val jwtProperties: JwtProperties | ||
| ) { | ||
| fun getSigningKey(): Key = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
KeyProvider에서 secret을 Base64로 인코딩한 뒤 문자열 바이트를 키로 사용하고 있는데, 이 방식은 실제 Base64 디코딩된 raw key bytes를 사용하는 것과 의미가 달라질 수 있습니다.
jwt.secret을 Base64 인코딩된 값으로 관리하고, Decoders.BASE64.decode(secret) → Keys.hmacShaKeyFor(bytes) 형태로 키를 생성하면 의도가 더 명확하고 JJWT 권장 방식에도 맞을 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WOWOWOWOW......이걸 놓쳤네요... 민경언니는 신이야... 수정했습니다!!!! 5ce02e6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main/kotlin/org/appjam/smashing/domain/auth/exception/ExceptionHandlerFilter.kt (1)
26-30: 예외 발생 시 로깅 추가를 권장합니다.현재 예외가 발생해도 로깅 없이 응답만 반환하고 있어, 운영 환경에서 문제 추적이 어려울 수 있습니다. 특히
INTERNAL_SERVER_ERROR로 처리되는 일반 예외의 경우 원인 파악이 중요합니다.🔎 로깅 추가 제안
+import org.slf4j.LoggerFactory + @Component class ExceptionHandlerFilter( private val objectMapper: ObjectMapper, ) : OncePerRequestFilter() { + + private val log = LoggerFactory.getLogger(this::class.java) override fun doFilterInternal( request: HttpServletRequest, response: HttpServletResponse, filterChain: FilterChain ) { try { filterChain.doFilter(request, response) } catch (e: CustomException) { + log.warn("CustomException occurred: ${e.errorCode}", e) handleException(response, e.errorCode) } catch (e: Exception) { + log.error("Unexpected exception occurred", e) handleException(response, ErrorCode.INTERNAL_SERVER_ERROR) } }src/main/kotlin/org/appjam/smashing/domain/auth/exception/JwtAuthenticationEntryPoint.kt (1)
18-29: 인증 예외 로깅 추가를 권장합니다.
authException파라미터가 사용되지 않고 있어, 인증 실패 원인을 파악하기 어려울 수 있습니다. 디버깅과 보안 모니터링을 위해 로깅을 추가하는 것이 좋습니다.🔎 로깅 추가 제안
+import org.slf4j.LoggerFactory + @Component class JwtAuthenticationEntryPoint( private val objectMapper: ObjectMapper, ) : AuthenticationEntryPoint { + + private val log = LoggerFactory.getLogger(this::class.java) override fun commence( request: HttpServletRequest, response: HttpServletResponse, authException: AuthenticationException, ) { + log.warn("Authentication failed for ${request.requestURI}: ${authException.message}") val errorCode = ErrorCode.UNAUTHORIZED // ... } }src/main/kotlin/org/appjam/smashing/global/config/SecurityConfig.kt (1)
21-25:JwtAuthenticationFilter인스턴스화 방식 불일치
ExceptionHandlerFilter는 의존성 주입을 통해 주입받고 있지만,JwtAuthenticationFilter는 인라인으로 생성되고 있습니다. 일관성을 위해JwtAuthenticationFilter도@Component로 등록하고 주입받는 것을 고려해 보세요.🔎 일관성 개선 제안
class SecurityConfig( private val jwtProvider: JwtProvider, private val exceptionHandlerFilter: ExceptionHandlerFilter, private val jwtAuthenticationEntryPoint: JwtAuthenticationEntryPoint, + private val jwtAuthenticationFilter: JwtAuthenticationFilter, ) { // ... - addFilterBefore<UsernamePasswordAuthenticationFilter>(JwtAuthenticationFilter(jwtProvider)) + addFilterBefore<UsernamePasswordAuthenticationFilter>(jwtAuthenticationFilter) }Also applies to: 51-51
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/kotlin/org/appjam/smashing/domain/auth/exception/ExceptionHandlerFilter.ktsrc/main/kotlin/org/appjam/smashing/domain/auth/exception/JwtAuthenticationEntryPoint.ktsrc/main/kotlin/org/appjam/smashing/global/common/dto/ApiResponse.ktsrc/main/kotlin/org/appjam/smashing/global/config/SecurityConfig.kt
🔇 Additional comments (2)
src/main/kotlin/org/appjam/smashing/global/common/dto/ApiResponse.kt (1)
81-91: LGTM!
errorBody함수가 필터/EntryPoint에서ResponseEntity없이 에러 응답 바디를 직접 생성할 수 있도록 잘 구현되었습니다. 기존error()함수와 일관된 구조를 유지하고 있습니다.src/main/kotlin/org/appjam/smashing/global/config/SecurityConfig.kt (1)
43-43: TODO 주석: 현재 모든 요청이permitAll로 설정되어 있습니다.보안상
authorize(anyRequest, permitAll)설정은 개발 단계에서만 사용해야 합니다. 추후authenticated로 변경하여 JWT 인증이 필요한 엔드포인트를 보호해 주세요.
| addFilterBefore<UsernamePasswordAuthenticationFilter>(exceptionHandlerFilter) | ||
| addFilterBefore<UsernamePasswordAuthenticationFilter>(JwtAuthenticationFilter(jwtProvider)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
cat -n src/main/kotlin/org/appjam/smashing/global/config/SecurityConfig.ktRepository: TEAM-SMASHING/SMASHING-SERVER
Length of output: 3235
🏁 Script executed:
# Verify Spring Security's addFilterBefore behavior with multiple calls
# by checking if there are any test files or documentation references
rg -n "addFilterBefore" --type kotlin -A 2 -B 2Repository: TEAM-SMASHING/SMASHING-SERVER
Length of output: 718
🏁 Script executed:
# Search for any filter tests or documentation that might clarify the order
find . -type f -name "*.kt" | xargs grep -l "addFilterBefore\|FilterChain" | head -5Repository: TEAM-SMASHING/SMASHING-SERVER
Length of output: 312
🏁 Script executed:
# Check the ExceptionHandlerFilter and JwtAuthenticationFilter implementations
echo "=== ExceptionHandlerFilter ===" && cat -n src/main/kotlin/org/appjam/smashing/domain/auth/exception/ExceptionHandlerFilter.kt && echo -e "\n=== JwtAuthenticationFilter ===" && cat -n src/main/kotlin/org/appjam/smashing/domain/auth/filter/JwtAuthenticationFilter.ktRepository: TEAM-SMASHING/SMASHING-SERVER
Length of output: 4021
필터 체인 순서를 정정해야 합니다.
addFilterBefore를 동일한 참조 위치에 순차적으로 두 번 호출할 때, 마지막에 추가된 필터가 체인의 첫 번째로 실행됩니다. 현재 코드에서는:
- 51번 줄의
JwtAuthenticationFilter가 마지막에 추가되어 첫 번째로 실행 - 50번 줄의
ExceptionHandlerFilter가 두 번째로 실행
이는 JwtAuthenticationFilter의 예외(jwtProvider.getUserId 호출 시)를 ExceptionHandlerFilter가 포착할 수 없다는 의미입니다.
해결책: 두 addFilterBefore 호출 순서를 바꾸세요:
addFilterBefore<UsernamePasswordAuthenticationFilter>(JwtAuthenticationFilter(jwtProvider))
addFilterBefore<UsernamePasswordAuthenticationFilter>(exceptionHandlerFilter)이렇게 하면 ExceptionHandlerFilter가 먼저 실행되어 JwtAuthenticationFilter의 모든 예외를 올바르게 처리합니다.
🤖 Prompt for AI Agents
In src/main/kotlin/org/appjam/smashing/global/config/SecurityConfig.kt around
lines 50-51, the two addFilterBefore calls are ordered such that
JwtAuthenticationFilter is added last and therefore executes first, preventing
ExceptionHandlerFilter from catching exceptions thrown by
JwtAuthenticationFilter; fix this by swapping the two addFilterBefore calls so
the ExceptionHandlerFilter is added after the JwtAuthenticationFilter call
(i.e., reverse their order) ensuring ExceptionHandlerFilter runs first and can
handle JwtAuthenticationFilter exceptions.
|
모든 코드리뷰를 반영했습니당🥹🥹 확인 부탁드려요!! @kyoooooong |
kyoooooong
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고 너무너무너무너무 많으셨습니다 ㅜㅜㅜ 🥺👏
| filterChain: FilterChain, | ||
| ) { | ||
| try { | ||
| val token = resolveToken(request) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JwtAuthenticationFilter에서 예외를 attribute에 담고 다시 throw하면 예외 처리 흐름이 시큐리티 내부 구현에 의존해 401/500이 케이스별로 갈릴 수 있을 것같아요. catch에서는 SecurityContextHolder.clearContext() 후 CustomException만 attribute로 전달하고, 응답 생성은 EntryPoint(Resolver)에서 일관되게 처리되도록 throw 없이 흐름을 정리해주시는 건 혹시 어떠실까요?
또, 토큰 체크 시 token == null 분기 대신 !token.isNullOrBlank() && validateToken(token) 패턴을 사용하면 null/empty/blank 케이스를 한 번에 방어할 수 있어 조건이 더 명확해질 수 있을 것같아서 이렇게 정리하시는 방향도 추천드리고 싶습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
어떤 흐름을 말씀하신지 이해했습니다!!
반영했는데 확인부탁드려요! 98ec65e
| response: HttpServletResponse, | ||
| authException: AuthenticationException, | ||
| ) { | ||
| val exception = request.getAttribute(EXCEPTION_KEY) as? Exception ?: authException |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EntryPoint에서 attribute를 Exception 전체로 받으면 범위가 너무 넓어 JWT와 무관한 예외까지 섞일 수 있습니다. attribute는 CustomException만 받도록 제한하고, 없으면 UNAUTHORIZED로 통일해 응답 정책을 명확히 하면 좋을 것같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CustomException으로 받는 거를 간과했네요ㅜㅜ 수정하겠습니다!
InsufficientAuthenticationException를 사용하면 UNAUTHORIZED로 명시해줄 수 있다고 해서 사용해주었습니다!
| @ExceptionHandler(org.springframework.security.core.AuthenticationException::class) | ||
| fun handleAuthenticationException(exception: AuthenticationException): ResponseEntity<ApiResponse<Unit>> { | ||
| return ApiResponse.error(ErrorCode.UNAUTHORIZED) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 401 응답은 JwtAuthenticationEntryPoint에서 HandlerExceptionResolver로 위임하는 구조라, GlobalExceptionHandler의 @ExceptionHandler(AuthenticationException)는 역할이 겹칠 수 있어 보여요.
인증 실패는 Security 필터 체인에서 발생하는 케이스가 대부분이라, 401 처리 책임은 EntryPoint로 일원화하고(Global에서는 제거), GlobalExceptionHandler는 컨트롤러/도메인 예외 처리에만 집중하는 쪽이 흐름이 더 명확할 수 있을 것같아요.
인증, 인가와 관련된 부분은 GlobalExceptionHandler에서 직접 처리하기보다, 각각 AuthenticationEntryPoint / AccessDeniedHandler에서 받아 HandlerExceptionResolver로 넘기는 방식으로 통일하는 게 역할 분리와 응답 일관성 측면에서 더 적절할 수 있을 것같은데 혹시 어떠실까요..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
각각 AuthenticationEntryPoint / AccessDeniedHandler에서 처리하도록 수정했습니다! f62822b
HandlerExceptionResolver에 대해서는 더 공부해보는 걸로 하겠습니닷..
| authorize(anyRequest, permitAll) // TODO: 추후 로그인 기능 구현 시 수정 | ||
| } | ||
|
|
||
| exceptionHandling { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exceptionHandlerFilter 제거하고 JwtAuthenticationFilter만 남긴 방향은 너무 좋습니다! 덕분에 응답 생성 지점이 줄어서 예외 흐름이 단순해졌어요.
다만 예외 처리 순서/책임 관점에서 보면, Spring Security는 인증 실패(401)와 인가 실패(403)를 서로 다른 출구로 처리합니다.
401(인증 필요/인증 실패): AuthenticationEntryPoint가 호출됨
403(인증은 됐지만 권한 부족): AccessDeniedHandler가 호출됨
현재는 authenticationEntryPoint만 지정되어 있어 401 흐름은 의도대로 통일되지만, 추후 authenticated() / @PreAuthorize 등 인가 규칙이 들어가면 403이 발생했을 때는 기본 AccessDeniedHandler로 처리될 수 있어 응답 포맷/코드가 401과 다르게 나갈 여지가 있습니다.
그래서 예외 처리 흐름을 완전히 통일하려면 아래처럼 AccessDeniedHandler(403)도 함께 등록해서 401/403 모두 Security 단계에서 받아 HandlerExceptionResolver로 공통 포맷으로 넘기도록 맞추는 게 좋을 것 같습니다 😊
exceptionHandling { authenticationEntryPoint = jwtAuthenticationEntryPoint accessDeniedHandler = jwtAccessDeniedHandler }
src/main/kotlin/org/appjam/smashing/domain/auth/jwt/CustomUserDetails.kt
Outdated
Show resolved
Hide resolved
| @Component | ||
| @ConfigurationProperties(prefix = "jwt") | ||
| class JwtProperties { | ||
| lateinit var secret: String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오! 말씀해주신 방향 너무 좋은 것같아요. @ConfigurationProperties를 data class로 두고 넣어줘야하는 필수값들을 기본값 0이 아닌 주입해서 넣어주는 방식으로 하면 그냥 class를 생성자 주입으로 쓰는 것보다 의도가 더 드러날 수 있어서 좋은 방식인 것같습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In
@src/main/kotlin/org/appjam/smashing/domain/auth/exception/JwtAccessDeniedHandler.kt:
- Around line 11-15: SecurityConfig is not wiring the JwtAccessDeniedHandler
bean, so 403 errors bypass it; import and inject JwtAccessDeniedHandler into the
SecurityConfig constructor, then inside the HttpSecurity exceptionHandling block
set authenticationEntryPoint = jwtAuthenticationEntryPoint and
accessDeniedHandler = jwtAccessDeniedHandler (ensure you reference the
JwtAccessDeniedHandler type and the injected jwtAccessDeniedHandler instance).
- Around line 17-23: Add explicit handling for
org.springframework.security.access.AccessDeniedException in
GlobalExceptionHandler so AccessDeniedException delegated from
JwtAccessDeniedHandler returns HTTP 403 instead of 500; either add a new
@ExceptionHandler(AccessDeniedException::class) method that returns
ApiResponse.error(ErrorCode.FORBIDDEN) (mirroring the
AuthorizationDeniedException handler) or merge AccessDeniedException into the
existing authorization handler to handle both exceptions and return FORBIDDEN.
src/main/kotlin/org/appjam/smashing/domain/auth/exception/JwtAccessDeniedHandler.kt
Show resolved
Hide resolved
src/main/kotlin/org/appjam/smashing/domain/auth/exception/JwtAccessDeniedHandler.kt
Show resolved
Hide resolved
|
마무리 했습니다..! 혹시 더 수정 필요한 부분이 있는 것 같다면 언제든 말씀해주세요!! @kyoooooong |
kyoooooong
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
너무너무 고생 많았습니다.... 🥺
| response: HttpServletResponse, | ||
| accessDeniedException: AccessDeniedException, | ||
| ) { | ||
| resolver.resolveException(request, response, null, accessDeniedException) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AccessDeniedHandler에서 accessDeniedException을 그대로 resolver로 넘기면, 전역 핸들러에서 ErrorCode.FORBIDDEN으로 매핑되지 않아 응답 포맷/코드가 달라질 수 있을 것같아요! 403은 CustomException(ErrorCode.FORBIDDEN)로 감싸서 resolver로 넘기면 401/403 모두 일관된 포맷으로 통일될 수 있을 것같습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아아 Spring Security 기본 예외인 accessDeniedException을 그대로 넘기면 안 되는 군요ㅜㅜ CustomException을 넘기는 것으로 수정했습니다!
| } | ||
|
|
||
| val subject = claims.subject ?: throw CustomException(ErrorCode.INVALID_ACCESS_TOKEN_SUBJECT) | ||
| val role = listOf(SimpleGrantedAuthority("ROLE_USER")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 ROLE_USER가 Provider에서 고정되어 있는데, 추후 권한 확장(예: ADMIN, MANAGER 등)을 고려하면 토큰 claim 기반으로 authorities를 구성하도록 열어두면 더 유연할 것 같아요!
예를 들어 토큰에 roles: ["USER", "ADMIN"] 같은 claim이 들어온다면, 인증 생성 시점에서 아래처럼 GrantedAuthority로 변환해줄 수 있을 것 같습니다.
'roles.map { SimpleGrantedAuthority("ROLE_$it") }'
이렇게 해두면 권한 정책 변경 시 CustomUserDetails나 인증 구조를 수정하지 않고도 토큰 claim만으로 권한을 확장할 수 있어서 유지보수 측면에서도 이점이 있을 것 같습니다 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
어떤 흐름인지 이해했습니다! 해당 내용 9db7d7d에 반영했는데 이 부분을 의도해주신 거 맞을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @src/main/kotlin/org/appjam/smashing/domain/auth/jwt/JwtGenerator.kt:
- Around line 3-5: Upgrade the JJWT dependency to 0.13.0 to remediate
CVE-2024-31033 and update JwtGenerator.kt to use the new safe key APIs: change
the project dependency version for io.jsonwebtoken to 0.13.0, replace any uses
of setSigningKey() or signWith(rawSecretString, ...) with
Keys.hmacShaKeyFor(byte[]) to produce a java.security.Key and call signWith(key)
or signWith(key, SignatureAlgorithm) in the JwtGenerator class, update imports
(e.g., add io.jsonwebtoken.security.Keys and remove deprecated overloads), and
then rebuild/run tests to ensure token creation still works.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/kotlin/org/appjam/smashing/domain/auth/jwt/JwtGenerator.ktsrc/main/kotlin/org/appjam/smashing/domain/auth/jwt/JwtProvider.ktsrc/main/kotlin/org/appjam/smashing/domain/auth/jwt/Token.kt
🔇 Additional comments (5)
src/main/kotlin/org/appjam/smashing/domain/auth/jwt/Token.kt (1)
3-34: 깔끔한 구조와 과거 피드백 반영이 훌륭합니다!토큰 문자열과 만료 시간을 중첩된 데이터 클래스로 분리한 구조가 명확하고, 팩토리 메서드를 통한 객체 생성 방식도 적절합니다. 과거 리뷰에서 언급된 클라이언트의 만료 시간 요구사항을 잘 반영하셨습니다.
src/main/kotlin/org/appjam/smashing/domain/auth/jwt/JwtProvider.kt (2)
16-26: 토큰 발급 로직이 명확하게 구현되었습니다.access/refresh 토큰을 별도로 생성하고, 만료 시간과 함께 TokenDto로 조립하는 흐름이 깔끔합니다.
28-49: 인증 추출 로직이 적절하게 구현되었습니다.토큰 타입 검증, subject 추출, CustomUserDetails 생성 및 Authentication 반환 흐름이 올바르게 구현되어 있습니다. 예외 처리도 적절합니다.
Line 37의 hard-coded ROLE_USER는 과거 리뷰에서 이미 논의된 사항으로, 향후 권한 확장 시 claim 기반으로 전환하는 방안이 고려되었음을 확인했습니다.
src/main/kotlin/org/appjam/smashing/domain/auth/jwt/JwtGenerator.kt (2)
19-37: Access 토큰 생성 로직이 올바르게 구현되었습니다.userId를 subject로 사용하고, TokenType을 문자열로 저장하는 방식이 적절합니다. GeneratedToken을 통해 토큰과 만료 시간을 함께 반환하는 구조도 깔끔합니다.
39-57: Refresh 토큰 생성 로직이 올바르게 구현되었습니다.UUID를 subject로 사용하여 access 토큰과 명확히 구분되는 구조가 잘 구현되었습니다. 과거 리뷰에서 제기된 토큰 혼용 우려를 효과적으로 해결했습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/kotlin/org/appjam/smashing/domain/auth/jwt/JwtGenerator.kt (1)
3-5: JJWT 라이브러리 보안 취약점 관련 - 이전 리뷰 참고이전 리뷰에서 JJWT 0.11.5 버전의 CVE-2024-31033 취약점이 이미 지적되었습니다. 해당 이슈를 참고하여 0.13.0 버전으로 업그레이드를 검토해 주세요.
🧹 Nitpick comments (1)
src/main/kotlin/org/appjam/smashing/domain/auth/jwt/JwtProvider.kt (1)
35-38: roles 리스트 내 null 요소 처리를 고려해 주세요.
roles가List<*>타입으로 캐스팅되므로 리스트 내에 null 요소가 포함될 수 있습니다. 현재it.toString()은 null에 대해"null"문자열을 반환하여"ROLE_null"이라는 의도치 않은 권한이 생성될 수 있습니다.🔎 제안하는 수정
- val authorities = roles.map { - SimpleGrantedAuthority(ROLE + it.toString()) - } + val authorities = roles.filterNotNull().map { + SimpleGrantedAuthority(ROLE + it.toString()) + }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/kotlin/org/appjam/smashing/domain/auth/exception/JwtAccessDeniedHandler.ktsrc/main/kotlin/org/appjam/smashing/domain/auth/jwt/JwtGenerator.ktsrc/main/kotlin/org/appjam/smashing/domain/auth/jwt/JwtProvider.ktsrc/main/kotlin/org/appjam/smashing/global/exception/ErrorCode.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/kotlin/org/appjam/smashing/domain/auth/exception/JwtAccessDeniedHandler.kt
🔇 Additional comments (7)
src/main/kotlin/org/appjam/smashing/domain/auth/jwt/JwtGenerator.kt (3)
9-12: LGTM!토큰과 만료 시간을 함께 반환하는 깔끔한 데이터 클래스입니다.
19-41: LGTM! 이전 리뷰 피드백이 잘 반영되었습니다.
TokenType.ACCESS_TOKEN.name으로 enum을 문자열로 저장하여 파싱 시 타입 캐스팅 문제를 방지했습니다.- roles claim을 통해 권한 정보를 토큰에 포함시켜 확장성을 확보했습니다.
43-61: LGTM! Refresh 토큰 분리가 적절히 구현되었습니다.이전 리뷰에서 언급된 대로 refresh 토큰은 userId 대신 UUID를 subject로 사용하여 access 토큰과 명확히 구분됩니다. 이를 통해 refresh 토큰으로 userId를 추출하는 실수를 방지할 수 있습니다.
src/main/kotlin/org/appjam/smashing/domain/auth/jwt/JwtProvider.kt (3)
17-30: LGTM!토큰 발급 로직이 깔끔하게 구현되었습니다. 기본값으로 빈 리스트를 사용하여 roles 파라미터의 유연성을 확보한 점이 좋습니다.
40-43: 토큰 타입 검증 로직이 적절합니다.Access 토큰인지 명시적으로 검증하여 refresh 토큰으로 인증을 시도하는 것을 방지합니다. 이전 리뷰에서 언급된 보안 우려사항이 잘 해결되었습니다.
47-56: LGTM!CustomUserDetails 생성 및 Authentication 객체 반환 로직이 올바르게 구현되었습니다. 이전 리뷰에서 권고한 대로 토큰 claim 기반으로 authorities를 구성하여 권한 확장에 유연하게 대응할 수 있습니다.
src/main/kotlin/org/appjam/smashing/global/exception/ErrorCode.kt (1)
21-40: LGTM! 에러 코드가 체계적으로 잘 정리되었습니다.이전 리뷰에서 권고한 대로 도메인별 에러 코드(
AUTH-001~AUTH-017)가 적용되었습니다. Access 토큰과 Refresh 토큰 에러가 명확히 구분되어 있어 디버깅 및 클라이언트 에러 처리에 용이합니다.
|
항상 수고가 많으십니다.... 마지막까지 파이팅이에요!!!!!💪 @kyoooooong |
kyoooooong
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
정말 너무너무 수고 많으셨습니다 🥺 코멘트 한번씩만 봐주시고, 완성되면 바로 머지해주세요! 고생 너무너무 많았습니다 👏
| package org.appjam.smashing.domain.auth.jwt | ||
|
|
||
| data class TokenDto( | ||
| val token: Token, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
너무너무 좋은 코드 잘보았습니다! 그런데 토큰마다의 값과 만료시각으로 묶어보면 더 좋을 수 있을 것같아서 혹시 아래와 같은 형태는 어떠실까요..? 수고 많으셨습니다 👍
`data class TokenDto(
val accessToken: Token,
val refreshToken: Token,
) {
companion object {
fun create(
accessToken: Token,
refreshToken: Token,
) = TokenDto(
accessToken = accessToken,
refreshToken = refreshToken
)
}
data class Token(
val token: String,
val expiredAt: Long,
) {}`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋습니다! b58dce2
| response: HttpServletResponse, | ||
| authException: AuthenticationException, | ||
| ) { | ||
| val exception = request.getAttribute(EXCEPTION_KEY) as? CustomException ?: InsufficientAuthenticationException(ErrorCode.UNAUTHORIZED.message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EntryPoint에서 fallback 예외가 InsufficientAuthenticationException으로 되어 있는데, 전역 응답 포맷을 CustomException(ErrorCode) 기반으로 통일하려면 fallback도 CustomException(ErrorCode.UNAUTHORIZED)로 맞추는 게 더 일관될 수 있을 것같다는 생각이 듭니다!
| import org.springframework.stereotype.Component | ||
| import java.util.* | ||
|
|
||
| data class GeneratedToken( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이부분 이미 tokendto -> token 정보와 똑같이 존재해서 이걸로 사용해주면 좋을 것같아요!
| refreshToken: String, | ||
| refreshTokenExpiredAt: Long, | ||
| ): TokenDto = TokenDto( | ||
| accessToken = Token( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
token 생성하는 부분도 정팩메로 분리하면 좋을 것같습니다!
| @@ -0,0 +1,29 @@ | |||
| package org.appjam.smashing.domain.auth.jwt | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 jwt 패키지 안에 있는 파일들을 좀 더 세분화해서 components, dto, enums 등과 같이 나누어 분리하면 좋을 것같습니다! 👍
| @ConfigurationProperties(prefix = "jwt") | ||
| data class JwtProperties( | ||
| val secret: String, | ||
| val accessTokenExpireTime: Long, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 저희 .yml 파일을 보니 환경 변수값이 더 추가된 것같아요! 맞춰서 .sample.env도 추가 부탁드립니다
📌 Related Issue
#️⃣ 요약 설명
SecurityConfig에서 JWT 필터를 거칠 수 있도록 수정해 주었습니다.JwtAuthenticationFilter에서는 매 요청마다 "Bearer"를 추출한 후 검사하도록 했습니다.JwtProvider는 JWT 관련 기능을 진입하게 해주는 역할을 하고, 토큰 생성은JwtGenerator로, 토큰 검증은JwtValidator로 역할을 분리하였습니다.KeyProvider와JwtProperties는 서명 키를 관리하였습니다.📝 작업 내용
👍 동작 확인
💬 리뷰 요구사항(선택)
Summary by CodeRabbit
새로운 기능
개선 사항
✏️ Tip: You can customize this high-level summary in your review settings.