ISSUE-10 JWT Util 클래스 구현 #11
Conversation
Walkthrough이 변경 사항은 JWT(JSON Web Token) 기능을 지원하기 위해 여러 파일에 대한 수정 및 추가를 포함합니다. Changes
Assessment against linked issues
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (14)
core/build.gradle.kts(1 hunks)core/src/main/kotlin/org/doorip/core/auth/JwtService.kt(1 hunks)core/src/main/kotlin/org/doorip/core/config/JwtConfig.kt(1 hunks)core/src/main/kotlin/org/doorip/core/config/JwtProperties.kt(1 hunks)core/src/main/resources/application-jwt.yml(1 hunks)domain/src/main/kotlin/org/doorip/domain/DooripException.kt(1 hunks)presentation/support/auth/src/main/kotlin/org/doorip/presentation/support/auth/TokenAuthenticationFilter.kt(0 hunks)settings.gradle.kts(1 hunks)support/jwt/build.gradle.kts(1 hunks)support/jwt/src/main/kotlin/org/doorip/support/jwt/Extensions.kt(1 hunks)support/jwt/src/main/kotlin/org/doorip/support/jwt/JwtProvider.kt(1 hunks)support/jwt/src/main/kotlin/org/doorip/support/jwt/JwtSecretKey.kt(1 hunks)support/jwt/src/main/kotlin/org/doorip/support/jwt/RSAPublicKey.kt(1 hunks)support/jwt/src/main/kotlin/org/doorip/support/jwt/SignatureKey.kt(1 hunks)
💤 Files with no reviewable changes (1)
- presentation/support/auth/src/main/kotlin/org/doorip/presentation/support/auth/TokenAuthenticationFilter.kt
🧰 Additional context used
🪛 detekt (1.23.7)
support/jwt/src/main/kotlin/org/doorip/support/jwt/JwtProvider.kt
[warning] 58-58: The caught exception is too generic. Prefer catching specific exceptions to the case that is currently handled.
(detekt.exceptions.TooGenericExceptionCaught)
[warning] 54-54: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 58-58: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 71-71: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (6)
core/src/main/resources/application-jwt.yml (1)
2-2: 환경 변수 설정을 확인해 주세요
JWT_SECRET_KEY 환경 변수가 설정되어 있는지 확인해 주세요. 설정되지 않으면 애플리케이션이 실행되지 않을 수 있습니다.
settings.gradle.kts (1)
8-8: 모듈 의존성 순서를 확인해 주세요
support:jwt 모듈이 다른 모듈에 의존성이 없는지 확인해 주세요. 의존성 순서에 따라 빌드 문제가 발생할 수 있습니다.
core/build.gradle.kts (1)
9-9: JWT 모듈 의존성이 올바르게 추가되었네요! 👍
프로젝트 구조에 맞게 support:jwt 모듈을 잘 참조하고 계십니다.
core/src/main/kotlin/org/doorip/core/auth/JwtService.kt (1)
8-12: @service 어노테이션 사용이 적절해요! 🎯
비즈니스 로직을 담당하는 서비스 계층임을 명확하게 표현하셨네요. 의존성 주입도 깔끔하게 처리되었습니다.
core/src/main/kotlin/org/doorip/core/config/JwtConfig.kt (2)
9-11: 설정 클래스 구조가 깔끔하네요! ✨
@EnableConfigurationProperties와 @Configuration 어노테이션을 적절히 사용하셨습니다.
16-17: 🧹 Nitpick (assertive)
비밀키 보안 강화를 고려해보세요 🔐
JWT 비밀키는 매우 중요한 보안 정보입니다. 다음 사항들을 고려해보시면 좋을 것 같아요:
- 키 길이가 충분한지 검증
- 환경 변수나 보안 저장소 사용
- 키 순환(rotation) 정책 수립
비밀키 설정을 확인해볼까요?
✅ Verification successful
JWT 시크릿 키 설정이 잘 되어있네요! 👍
환경 변수(${JWT_SECRET_KEY})를 통해 시크릿 키를 관리하고 계신 것을 확인했어요. 이는 보안 모범 사례를 잘 따르고 있는 것이랍니다. 다만 추가로 다음 사항들을 고려해보시면 더 좋을 것 같아요:
- 운영 환경에서 시크릿 키가 최소 256비트(32바이트) 이상의 길이를 가지도록 설정해주세요
- 정기적인 키 순환(rotation) 정책을 수립하시는 것을 추천드려요
- 가능하다면 AWS Secrets Manager나 HashiCorp Vault 같은 시크릿 관리 서비스 도입을 고려해보세요
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check JWT secret key configuration in application properties
# Look for JWT secret key configuration
rg -l "jwt\.secret|JWT_SECRET" --type yaml
Length of output: 88
Script:
#!/bin/bash
# Check the content of JWT configuration file
cat core/src/main/resources/application-jwt.yml
Length of output: 84
| val decoder = Base64.getUrlDecoder() | ||
| val decoded = String(decoder.decode(header)) | ||
| return objectMapper.readValue(decoded, typeReference) | ||
| } catch (e: DatabindException) { |
There was a problem hiding this comment.
예외 처리를 개선해 주세요
DatabindException을 캡처했지만 예외를 무시하고 있어 원래 예외 정보가 손실될 수 있습니다. 예외를 로깅하거나 적절한 처리를 추가해 주세요.
🧰 Tools
🪛 detekt (1.23.7)
[warning] 54-54: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
| return null | ||
| } catch (e: JsonParseException) { | ||
| return null | ||
| } catch (e: IndexOutOfBoundsException) { |
There was a problem hiding this comment.
예외 처리를 개선해 주세요
JsonParseException을 캡처했지만 예외를 무시하고 있어 원래 예외 정보가 손실될 수 있습니다. 예외를 로깅하거나 적절한 처리를 추가해 주세요.
🧰 Tools
🪛 detekt (1.23.7)
[warning] 58-58: The caught exception is too generic. Prefer catching specific exceptions to the case that is currently handled.
(detekt.exceptions.TooGenericExceptionCaught)
[warning] 58-58: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🛠️ Refactor suggestion
너무 일반적인 예외를 캡처하고 있습니다
IndexOutOfBoundsException 대신 더 구체적인 예외를 캡처하는 것이 좋습니다. 이는 예상하지 못한 예외를 놓칠 수 있기 때문입니다.
🧰 Tools
🪛 detekt (1.23.7)
[warning] 58-58: The caught exception is too generic. Prefer catching specific exceptions to the case that is currently handled.
(detekt.exceptions.TooGenericExceptionCaught)
[warning] 58-58: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
| else -> return null | ||
| } | ||
| return parser.parseSignedClaims(token).payload | ||
| } catch (e: JwtException) { |
There was a problem hiding this comment.
예외 처리를 개선해 주세요
JwtException을 캡처했지만 예외를 무시하고 있어 원래 예외 정보가 손실될 수 있습니다. 예외를 로깅하거나 적절한 처리를 추가해 주세요.
🧰 Tools
🪛 detekt (1.23.7)
[warning] 71-71: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
| is SecretKey -> generateParser(signatureKey) | ||
| is PublicKey -> generateParser(signatureKey) | ||
| else -> return null |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
when 구문의 가독성을 높여 주세요
when 구문에서 else 대신 명시적으로 모든 경우를 처리해 주세요. 키의 타입이 SecretKey나 PublicKey가 아닌 경우를 대비해 예외를 처리해 주세요.
| interface SignatureKey { | ||
| val key: Key | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
SignatureKey 인터페이스의 역할을 명확히 해 주세요
SignatureKey 인터페이스에 key 프로퍼티만 존재합니다. 추후 확장을 고려하여 추가 메서드나 프로퍼티를 정의해 두는 것이 좋습니다.
| override fun getUserId(token: String): UserId? { | ||
| // TODO | ||
| return null | ||
| val subject = jwtProvider.validateAndGetSubject(token, jwtSecretKey) ?: return null | ||
|
|
||
| return UserId(subject.toLong()) |
There was a problem hiding this comment.
toLong() 변환 시 예외 처리가 필요해 보여요 🤔
subject를 Long으로 변환하는 과정에서 NumberFormatException이 발생할 수 있습니다. 안전한 처리를 위해 예외 처리를 추가하면 좋을 것 같아요.
다음과 같은 방식으로 개선해보는 건 어떨까요?
override fun getUserId(token: String): UserId? {
val subject = jwtProvider.validateAndGetSubject(token, jwtSecretKey) ?: return null
- return UserId(subject.toLong())
+ return try {
+ UserId(subject.toLong())
+ } catch (e: NumberFormatException) {
+ null
+ }
}📝 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.
| override fun getUserId(token: String): UserId? { | |
| // TODO | |
| return null | |
| val subject = jwtProvider.validateAndGetSubject(token, jwtSecretKey) ?: return null | |
| return UserId(subject.toLong()) | |
| override fun getUserId(token: String): UserId? { | |
| val subject = jwtProvider.validateAndGetSubject(token, jwtSecretKey) ?: return null | |
| return try { | |
| UserId(subject.toLong()) | |
| } catch (e: NumberFormatException) { | |
| null | |
| } | |
| } |
| package org.doorip.support.jwt | ||
|
|
||
| import java.math.BigInteger | ||
| import java.security.Key | ||
| import java.security.KeyFactory | ||
| import java.security.spec.RSAPublicKeySpec | ||
| import java.util.* | ||
|
|
||
| class RSAPublicKey( | ||
| private val n: String, | ||
| private val e: String, | ||
| private val type: String, | ||
| ) : SignatureKey { | ||
| override val key: Key by lazy { | ||
| val decoder = Base64.getUrlDecoder() | ||
|
|
||
| val nBytes = decoder.decode(n) | ||
| val eBytes = decoder.decode(e) | ||
|
|
||
| val n = BigInteger(1, nBytes) | ||
| val e = BigInteger(1, eBytes) | ||
|
|
||
| val rsaPublicKeySpec = RSAPublicKeySpec(n, e) | ||
|
|
||
| val keyFactory = KeyFactory.getInstance(type) | ||
|
|
||
| keyFactory.generatePublic(rsaPublicKeySpec) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
보안 관련 추가 제안사항이 있습니다 🔒
- 키 생성 후에는 입력으로 사용된 민감한 문자열들(
n,e)을 메모리에서 제거하는 것이 좋습니다. SignatureKey인터페이스에 키 삭제 메서드를 추가하는 것을 고려해보세요.
| override val key: Key by lazy { | ||
| val decoder = Base64.getUrlDecoder() | ||
|
|
||
| val nBytes = decoder.decode(n) | ||
| val eBytes = decoder.decode(e) | ||
|
|
||
| val n = BigInteger(1, nBytes) | ||
| val e = BigInteger(1, eBytes) | ||
|
|
||
| val rsaPublicKeySpec = RSAPublicKeySpec(n, e) | ||
|
|
||
| val keyFactory = KeyFactory.getInstance(type) | ||
|
|
||
| keyFactory.generatePublic(rsaPublicKeySpec) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
예외 처리를 개선하면 좋을 것 같아요! 🛡️
현재 구현에서는 키 생성 과정에서 발생할 수 있는 예외들이 그대로 전파됩니다. 보안 관련 컴포넌트이므로 더 명확한 예외 처리가 필요해요.
다음과 같이 개선하는 것을 제안드립니다:
override val key: Key by lazy {
- val decoder = Base64.getUrlDecoder()
-
- val nBytes = decoder.decode(n)
- val eBytes = decoder.decode(e)
-
- val n = BigInteger(1, nBytes)
- val e = BigInteger(1, eBytes)
-
- val rsaPublicKeySpec = RSAPublicKeySpec(n, e)
-
- val keyFactory = KeyFactory.getInstance(type)
-
- keyFactory.generatePublic(rsaPublicKeySpec)
+ try {
+ val decoder = Base64.getUrlDecoder()
+
+ val nBytes = decoder.decode(n)
+ val eBytes = decoder.decode(e)
+
+ val n = BigInteger(1, nBytes)
+ val e = BigInteger(1, eBytes)
+
+ val rsaPublicKeySpec = RSAPublicKeySpec(n, e)
+ val keyFactory = KeyFactory.getInstance(type)
+
+ keyFactory.generatePublic(rsaPublicKeySpec)
+ } catch (e: IllegalArgumentException) {
+ throw IllegalArgumentException("잘못된 Base64 형식입니다", e)
+ } catch (e: Exception) {
+ throw IllegalStateException("공개키 생성 중 오류가 발생했습니다", e)
+ }
}📝 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.
| override val key: Key by lazy { | |
| val decoder = Base64.getUrlDecoder() | |
| val nBytes = decoder.decode(n) | |
| val eBytes = decoder.decode(e) | |
| val n = BigInteger(1, nBytes) | |
| val e = BigInteger(1, eBytes) | |
| val rsaPublicKeySpec = RSAPublicKeySpec(n, e) | |
| val keyFactory = KeyFactory.getInstance(type) | |
| keyFactory.generatePublic(rsaPublicKeySpec) | |
| } | |
| override val key: Key by lazy { | |
| try { | |
| val decoder = Base64.getUrlDecoder() | |
| val nBytes = decoder.decode(n) | |
| val eBytes = decoder.decode(e) | |
| val n = BigInteger(1, nBytes) | |
| val e = BigInteger(1, eBytes) | |
| val rsaPublicKeySpec = RSAPublicKeySpec(n, e) | |
| val keyFactory = KeyFactory.getInstance(type) | |
| keyFactory.generatePublic(rsaPublicKeySpec) | |
| } catch (e: IllegalArgumentException) { | |
| throw IllegalArgumentException("잘못된 Base64 형식입니다", e) | |
| } catch (e: Exception) { | |
| throw IllegalStateException("공개키 생성 중 오류가 발생했습니다", e) | |
| } | |
| } |
| class RSAPublicKey( | ||
| private val n: String, | ||
| private val e: String, | ||
| private val type: String, | ||
| ) : SignatureKey { |
There was a problem hiding this comment.
생성자 매개변수 유효성 검사가 필요해요! 🔍
현재 구현에서는 입력값 n, e, type에 대한 유효성 검사가 없습니다. Base64 디코딩 과정에서 잘못된 입력값으로 인한 예외가 발생할 수 있어요.
다음과 같이 생성자에서 유효성 검사를 추가하는 것을 추천드립니다:
class RSAPublicKey(
private val n: String,
private val e: String,
private val type: String,
-) : SignatureKey {
+) : SignatureKey {
+ init {
+ require(n.isNotBlank()) { "modulus는 비어있을 수 없습니다" }
+ require(e.isNotBlank()) { "exponent는 비어있을 수 없습니다" }
+ require(type == "RSA") { "현재는 RSA 알고리즘만 지원합니다" }
+ }📝 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.
| class RSAPublicKey( | |
| private val n: String, | |
| private val e: String, | |
| private val type: String, | |
| ) : SignatureKey { | |
| class RSAPublicKey( | |
| private val n: String, | |
| private val e: String, | |
| private val type: String, | |
| ) : SignatureKey { | |
| init { | |
| require(n.isNotBlank()) { "modulus는 비어있을 수 없습니다" } | |
| require(e.isNotBlank()) { "exponent는 비어있을 수 없습니다" } | |
| require(type == "RSA") { "현재는 RSA 알고리즘만 지원합니다" } | |
| } |
| data object UnauthorizedException : ClientException("e4030", "인가 과정에서 예외가 발생했습니다.") { private fun readResolve(): Any = UnauthorizedException } | ||
| data object UnauthenticatedException : ClientException("e4010", "인증 과정에서 예외가 발생했습니다. (ex. AT 만료 or RT 만료)") { private fun readResolve(): Any = UnauthenticatedException } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
예외 처리 구조에 대한 제안이 있습니다! 🏗️
JWT 관련 예외들을 위한 별도의 sealed 클래스를 만드는 것을 고려해보세요. 이렇게 하면 JWT 관련 예외들을 더 체계적으로 관리할 수 있습니다.
예시 구조:
sealed class JwtException(
code: String,
message: String
) : ClientException(code, message)
data object TokenExpiredException : JwtException("e4011", "인증 토큰이 만료되었습니다.")
data object InvalidTokenException : JwtException("e4012", "유효하지 않은 토큰입니다.")🧹 Nitpick (assertive)
예외 메시지를 더 명확하게 개선하면 좋을 것 같아요! 📝
현재 예외 메시지가 다소 모호할 수 있습니다. JWT 컨텍스트에서 더 구체적인 정보를 제공하면 좋을 것 같아요.
다음과 같이 메시지를 개선하는 것을 제안드립니다:
-data object UnauthorizedException : ClientException("e4030", "인가 과정에서 예외가 발생했습니다.") { private fun readResolve(): Any = UnauthorizedException }
+data object UnauthorizedException : ClientException("e4030", "해당 리소스에 대한 접근 권한이 없습니다.") { private fun readResolve(): Any = UnauthorizedException }
-data object UnauthenticatedException : ClientException("e4010", "인증 과정에서 예외가 발생했습니다. (ex. AT 만료 or RT 만료)") { private fun readResolve(): Any = UnauthenticatedException }
+data object UnauthenticatedException : ClientException("e4010", "인증이 필요하거나 제공된 인증 토큰이 만료되었습니다.") { private fun readResolve(): Any = UnauthenticatedException }📝 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.
| data object UnauthorizedException : ClientException("e4030", "인가 과정에서 예외가 발생했습니다.") { private fun readResolve(): Any = UnauthorizedException } | |
| data object UnauthenticatedException : ClientException("e4010", "인증 과정에서 예외가 발생했습니다. (ex. AT 만료 or RT 만료)") { private fun readResolve(): Any = UnauthenticatedException } | |
| data object UnauthorizedException : ClientException("e4030", "해당 리소스에 대한 접근 권한이 없습니다.") { private fun readResolve(): Any = UnauthorizedException } | |
| data object UnauthenticatedException : ClientException("e4010", "인증이 필요하거나 제공된 인증 토큰이 만료되었습니다.") { private fun readResolve(): Any = UnauthenticatedException } |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (4)
buildSrc/src/main/kotlin/Versions.kt(1 hunks)support/jwt/src/main/kotlin/org/doorip/support/jwt/JwtSecretKey.kt(1 hunks)support/jwt/src/main/kotlin/org/doorip/support/jwt/RSAPublicKey.kt(1 hunks)support/jwt/src/main/kotlin/org/doorip/support/jwt/SignatureKey.kt(1 hunks)
🧰 Additional context used
🪛 detekt (1.23.7)
support/jwt/src/main/kotlin/org/doorip/support/jwt/RSAPublicKey.kt
[warning] 30-30: The caught exception is too generic. Prefer catching specific exceptions to the case that is currently handled.
(detekt.exceptions.TooGenericExceptionCaught)
[warning] 30-30: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (3)
buildSrc/src/main/kotlin/Versions.kt (1)
8-8: JJWT 버전 업데이트에 대한 검토가 필요합니다.
JJWT 라이브러리의 버전을 0.12.6으로 업데이트하신 것 확인했습니다. 안전한 업데이트를 위해 몇 가지 확인이 필요해 보입니다.
✅ Verification successful
JJWT 0.12.6 버전 업데이트는 안전하게 진행하셔도 좋습니다! 👍
검토 결과 다음과 같은 긍정적인 내용을 확인했습니다:
- 0.12.6 버전은 메모리 누수 문제를 해결하고 BouncyCastle을 1.78로 업그레이드하는 등 중요한 개선사항을 포함하고 있어요.
- 현재 프로젝트에서 사용 중인 JJWT API 패턴(
Jwts.builder(),Jwts.parser())은 0.12.6 버전과 호환됩니다. - GitHub 보안 취약점 데이터베이스에서 이 버전에 대한 알려진 취약점이 없음을 확인했습니다.
프로젝트의 JWT 구현이 support/jwt 모듈에 잘 격리되어 있어 마이그레이션도 안전하게 진행될 것으로 보입니다.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: JJWT 버전 관련 정보 확인
# 최신 릴리즈 노트 및 변경 사항 확인
curl -s "https://api.github.com/repos/jwtk/jjwt/releases/tags/0.12.6" | jq -r '.body'
# 보안 취약점 확인
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: MAVEN, package: "io.jsonwebtoken:jjwt") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
# Spring Boot 3.2.5와의 호환성 확인
rg -l "io.jsonwebtoken" | xargs rg "springBootVersion|SPRING_BOOT"
Length of output: 1513
Script:
#!/bin/bash
# 프로젝트의 build.gradle 파일들에서 JJWT 의존성 사용 확인
fd -e gradle.kts -e gradle | xargs rg "io\.jsonwebtoken"
# Spring Boot 버전 확인
fd -e gradle.kts -e gradle -e properties | xargs rg "spring(-|\.)boot\.version|springBootVersion"
# JJWT 관련 코드 사용 패턴 확인
rg -A 3 "Jwts\."
Length of output: 1561
support/jwt/src/main/kotlin/org/doorip/support/jwt/JwtSecretKey.kt (1)
6-12:
보안 강화가 필요해요 🔒
이전 리뷰에서 제안된 보안 강화 사항이 아직 반영되지 않았네요. 다시 한번 강조드립니다:
- 시크릿 키의 최소 길이 검증
- CharArray 사용으로 메모리 보안 강화
- UTF-8 인코딩 명시
추가로 다음 사항도 고려해 주세요:
- 키 생성 실패 시의 예외 처리
- 키의 유효성 검증 로직
다음과 같이 개선하는 것을 추천드립니다:
class JwtSecretKey(
- private val secret: String,
+ private val secret: CharArray,
) : SignatureKey {
+ companion object {
+ private const val MIN_SECRET_LENGTH = 32
+ }
+
+ init {
+ require(secret.size >= MIN_SECRET_LENGTH) {
+ "시크릿 키는 최소 ${MIN_SECRET_LENGTH}자 이상이어야 합니다"
+ }
+ }
+
- override val key: Key by lazy {
- Keys.hmacShaKeyFor(secret.toByteArray())
+ override val key: Key by lazy {
+ try {
+ Keys.hmacShaKeyFor(String(secret).toByteArray(Charset.forName("UTF-8")))
+ } catch (e: Exception) {
+ throw IllegalStateException("HMAC 키 생성 중 오류가 발생했습니다", e)
+ }
}
+
+ fun clear() {
+ secret.fill('\u0000')
+ }
}support/jwt/src/main/kotlin/org/doorip/support/jwt/RSAPublicKey.kt (1)
9-13: 생성자 매개변수 검증이 필요해요 ✨
이전 리뷰에서 제안된 생성자 매개변수 유효성 검사가 아직 반영되지 않았네요. 입력값 검증은 보안에 매우 중요합니다!
| const val JJWT = "0.12.6" | ||
| const val JDK = 17 | ||
| } No newline at end of file | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
마이그레이션 가이드가 필요할 수 있습니다.
JJWT 버전 업데이트와 관련하여 프로젝트 내 JWT 관련 코드의 마이그레이션이 필요할 수 있습니다. 특히 새로운 버전에서 변경된 API나 deprecated된 기능들이 있는지 확인해주시면 좋겠습니다.
마이그레이션 가이드나 체크리스트가 필요하시다면 제가 도움을 드릴 수 있습니다. 필요하신가요?
| interface SignatureKey { | ||
| val key: Key? | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
인터페이스 설계를 개선해 주세요 🎨
안녕하세요! SignatureKey 인터페이스와 관련해서 몇 가지 개선사항을 제안드립니다:
key프로퍼티가 nullable인 이유가 명확하지 않아요. 키가 없는 상황이 실제로 필요한지 검토해 주세요.- 인터페이스의 목적과 사용법을 설명하는 KDoc 문서를 추가하면 좋겠어요.
- 키의 유효성을 검증하는 메서드도 추가하면 좋을 것 같아요.
다음과 같이 개선하는 것을 제안드립니다:
+/**
+ * JWT 서명에 사용되는 키를 제공하는 인터페이스입니다.
+ */
interface SignatureKey {
- val key: Key?
+ val key: Key
+
+ /**
+ * 키의 유효성을 검증합니다.
+ * @throws IllegalStateException 키가 유효하지 않은 경우
+ */
+ fun validate()
}📝 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.
| interface SignatureKey { | |
| val key: Key? | |
| } | |
| /** | |
| * JWT 서명에 사용되는 키를 제공하는 인터페이스입니다. | |
| */ | |
| interface SignatureKey { | |
| val key: Key | |
| /** | |
| * 키의 유효성을 검증합니다. | |
| * @throws IllegalStateException 키가 유효하지 않은 경우 | |
| */ | |
| fun validate() | |
| } |
There was a problem hiding this comment.
key가 nullable인 이유는 RSAPublicKey에서 키 생성 예외가 발생한 경우 예외를 던지지 않고 null을 저장해서 관리하기 위함이긴 해
There was a problem hiding this comment.
@SunwoongH, 이해했습니다. RSAPublicKey에서 키 생성 중 예외가 발생할 경우 예외를 던지지 않고 null로 관리하기 위해 key 프로퍼티를 nullable로 정의하신 것이군요. 설명 감사합니다!
✏️ Learnings added
Learnt from: SunwoongH
PR: Team-Going/doorip-server#11
File: support/jwt/src/main/kotlin/org/doorip/support/jwt/SignatureKey.kt:5-7
Timestamp: 2024-12-05T12:34:59.044Z
Learning: `support/jwt/src/main/kotlin/org/doorip/support/jwt/SignatureKey.kt`의 `SignatureKey` 인터페이스에서 `key` 프로퍼티는 nullable로 정의되어 있습니다. 이는 `RSAPublicKey` 구현에서 키 생성 시 예외가 발생하면 예외를 던지지 않고 `null`을 저장하여 관리하기 때문입니다.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| override val key: Key? by lazy { | ||
| try { | ||
| val decoder = Base64.getUrlDecoder() | ||
|
|
||
| val nBytes = decoder.decode(n) | ||
| val eBytes = decoder.decode(e) | ||
|
|
||
| val n = BigInteger(1, nBytes) | ||
| val e = BigInteger(1, eBytes) | ||
|
|
||
| val rsaPublicKeySpec = RSAPublicKeySpec(n, e) | ||
|
|
||
| val keyFactory = KeyFactory.getInstance(type) | ||
|
|
||
| keyFactory.generatePublic(rsaPublicKeySpec) | ||
| } catch (e: Exception) { | ||
| null | ||
| } | ||
| } |
There was a problem hiding this comment.
예외 처리를 개선해 주세요 🛡️
현재 구현에는 몇 가지 개선이 필요한 부분이 있어요:
- 모든 예외를
null로 처리하는 것은 위험할 수 있어요 - 구체적인 예외 타입별로 처리가 필요해요
- 의미 있는 에러 메시지가 있으면 좋겠어요
다음과 같이 개선하는 것을 추천드립니다:
- override val key: Key? by lazy {
+ override val key: Key by lazy {
try {
val decoder = Base64.getUrlDecoder()
val nBytes = decoder.decode(n)
val eBytes = decoder.decode(e)
val n = BigInteger(1, nBytes)
val e = BigInteger(1, eBytes)
val rsaPublicKeySpec = RSAPublicKeySpec(n, e)
val keyFactory = KeyFactory.getInstance(type)
keyFactory.generatePublic(rsaPublicKeySpec)
- } catch (e: Exception) {
- null
+ } catch (e: IllegalArgumentException) {
+ throw IllegalArgumentException("잘못된 Base64 형식입니다", e)
+ } catch (e: NoSuchAlgorithmException) {
+ throw IllegalArgumentException("지원하지 않는 알고리즘입니다: $type", e)
+ } catch (e: InvalidKeySpecException) {
+ throw IllegalArgumentException("잘못된 키 형식입니다", e)
+ } catch (e: Exception) {
+ throw IllegalStateException("공개키 생성 중 예기치 않은 오류가 발생했습니다", e)
}
}📝 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.
| override val key: Key? by lazy { | |
| try { | |
| val decoder = Base64.getUrlDecoder() | |
| val nBytes = decoder.decode(n) | |
| val eBytes = decoder.decode(e) | |
| val n = BigInteger(1, nBytes) | |
| val e = BigInteger(1, eBytes) | |
| val rsaPublicKeySpec = RSAPublicKeySpec(n, e) | |
| val keyFactory = KeyFactory.getInstance(type) | |
| keyFactory.generatePublic(rsaPublicKeySpec) | |
| } catch (e: Exception) { | |
| null | |
| } | |
| } | |
| override val key: Key by lazy { | |
| try { | |
| val decoder = Base64.getUrlDecoder() | |
| val nBytes = decoder.decode(n) | |
| val eBytes = decoder.decode(e) | |
| val n = BigInteger(1, nBytes) | |
| val e = BigInteger(1, eBytes) | |
| val rsaPublicKeySpec = RSAPublicKeySpec(n, e) | |
| val keyFactory = KeyFactory.getInstance(type) | |
| keyFactory.generatePublic(rsaPublicKeySpec) | |
| } catch (e: IllegalArgumentException) { | |
| throw IllegalArgumentException("잘못된 Base64 형식입니다", e) | |
| } catch (e: NoSuchAlgorithmException) { | |
| throw IllegalArgumentException("지원하지 않는 알고리즘입니다: $type", e) | |
| } catch (e: InvalidKeySpecException) { | |
| throw IllegalArgumentException("잘못된 키 형식입니다", e) | |
| } catch (e: Exception) { | |
| throw IllegalStateException("공개키 생성 중 예기치 않은 오류가 발생했습니다", e) | |
| } | |
| } |
🧰 Tools
🪛 detekt (1.23.7)
[warning] 30-30: The caught exception is too generic. Prefer catching specific exceptions to the case that is currently handled.
(detekt.exceptions.TooGenericExceptionCaught)
[warning] 30-30: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
There was a problem hiding this comment.
support:jwt 모듈에서 예외 클래스를 가진 domain 모듈을 의존하지 않게 하기 위함이라 예외보단 null로 관리해서 해당 key를 사용하는 클래스에서 예외 핸들링을 해주는게 맞는거 같아
There was a problem hiding this comment.
@SunwoongH, 이해했습니다. support:jwt 모듈에서 domain 모듈을 의존하지 않기 위해 예외 대신 null로 처리하고, 해당 키를 사용하는 클래스에서 예외 처리를 하시는 것이 좋다는 점 숙지하겠습니다. 의견 주셔서 감사합니다.
(_/)
( •_•)
💧
✏️ Learnings added
Learnt from: SunwoongH
PR: Team-Going/doorip-server#11
File: support/jwt/src/main/kotlin/org/doorip/support/jwt/RSAPublicKey.kt:15-33
Timestamp: 2024-12-05T12:36:53.828Z
Learning: `support:jwt` 모듈에서 예외 클래스를 가진 `domain` 모듈을 의존하지 않기 위해 예외 대신 `null`로 처리하고, 예외 처리는 해당 키를 사용하는 클래스에서 수행한다.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Related Issue ✔
close #10
Description ✔