Skip to content
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

ISSUE-10 JWT Util 클래스 구현 #11

Merged
merged 6 commits into from
Dec 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions buildSrc/src/main/kotlin/Versions.kt
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,6 @@ object Versions {
const val SPRING_BOOT = "3.2.5"
const val SPRING_DEPENDENCY_MANAGEMENT = "1.1.5"
const val MYSQL = "8.0.33"
const val JJWT = "0.12.5"
const val JJWT = "0.12.6"
const val JDK = 17
}
}
1 change: 1 addition & 0 deletions core/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ plugins {

dependencies {
api(project(":domain"))
implementation(project(":support:jwt"))

implementation("org.springframework.boot:spring-boot-starter:${Versions.SPRING_BOOT}")
compileOnly("org.springframework.boot:spring-boot-starter-jdbc:${Versions.SPRING_BOOT}")
Expand Down
16 changes: 11 additions & 5 deletions core/src/main/kotlin/org/doorip/core/auth/JwtService.kt
Original file line number Diff line number Diff line change
@@ -1,13 +1,19 @@
package org.doorip.core.auth

import org.doorip.domain.UserId
import org.springframework.stereotype.Component
import org.doorip.support.jwt.JwtProvider
import org.doorip.support.jwt.JwtSecretKey
import org.springframework.stereotype.Service

@Component
internal class JwtService : AccessTokenUseCase {
@Service
internal class JwtService(
private val jwtProvider: JwtProvider,
private val jwtSecretKey: JwtSecretKey,
) : AccessTokenUseCase {

override fun getUserId(token: String): UserId? {
// TODO
return null
val subject = jwtProvider.validateAndGetSubject(token, jwtSecretKey) ?: return null

return UserId(subject.toLong())
Comment on lines 14 to +17
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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
}
}

}
}
18 changes: 18 additions & 0 deletions core/src/main/kotlin/org/doorip/core/config/JwtConfig.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package org.doorip.core.config

import org.doorip.support.jwt.JwtProvider
import org.doorip.support.jwt.JwtSecretKey
import org.springframework.boot.context.properties.EnableConfigurationProperties
import org.springframework.context.annotation.Bean
import org.springframework.context.annotation.Configuration

@EnableConfigurationProperties(JwtProperties::class)
@Configuration
internal class JwtConfig {

@Bean
fun jwtProvider(): JwtProvider = JwtProvider()

@Bean
fun jwtSecretKey(jwtProperties: JwtProperties): JwtSecretKey = JwtSecretKey(jwtProperties.secret)
}
8 changes: 8 additions & 0 deletions core/src/main/kotlin/org/doorip/core/config/JwtProperties.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package org.doorip.core.config

import org.springframework.boot.context.properties.ConfigurationProperties

@ConfigurationProperties(prefix = "jwt")
data class JwtProperties(
val secret: String,
)
Comment on lines +7 to +8
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

secret 값의 보안에 주의해 주세요

secret 값이 외부에 노출되지 않도록 보안에 유의하시기 바랍니다. 필요하다면 암호화나 키 관리 시스템을 도입해 주세요.

2 changes: 2 additions & 0 deletions core/src/main/resources/application-jwt.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
jwt:
secret: ${JWT_SECRET_KEY}
11 changes: 2 additions & 9 deletions domain/src/main/kotlin/org/doorip/domain/DooripException.kt
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,8 @@ sealed class ClientException(
message: String,
) : DooripException(code, message)

class UnauthorizedException(
code: String,
message: String,
) : ClientException(code, message)

class UnauthenticatedException(
code: String,
message: String,
) : ClientException(code, message)
data object UnauthorizedException : ClientException("e4030", "인가 과정에서 예외가 발생했습니다.") { private fun readResolve(): Any = UnauthorizedException }
data object UnauthenticatedException : ClientException("e4010", "인증 과정에서 예외가 발생했습니다. (ex. AT 만료 or RT 만료)") { private fun readResolve(): Any = UnauthenticatedException }
Comment on lines +15 to +16
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Suggested change
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 }


data object InvalidRequestValueException : ClientException("e4000", "잘못된 요청입니다.") { private fun readResolve(): Any = InvalidRequestValueException }
data object MethodNotAllowedException : ClientException("e4050", "잘못된 HTTP method 요청입니다.") { private fun readResolve(): Any = MethodNotAllowedException }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ internal class TokenAuthenticationFilter(
val authentication = TokenAuthentication(token, userId, details)

val securityContext = SecurityContextHolder.getContext()

securityContext.authentication = authentication
}

Expand Down
1 change: 1 addition & 0 deletions settings.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,5 @@ include(
"domain",
"gateway:rdb",
"support:yaml",
"support:jwt",
)
12 changes: 12 additions & 0 deletions support/jwt/build.gradle.kts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import org.springframework.boot.gradle.tasks.bundling.BootJar

dependencies {
implementation("io.jsonwebtoken:jjwt-api:${Versions.JJWT}")
runtimeOnly("io.jsonwebtoken:jjwt-impl:${Versions.JJWT}")
runtimeOnly("io.jsonwebtoken:jjwt-jackson:${Versions.JJWT}")
Comment on lines +4 to +6
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

💡 Codebase verification

JJWT 라이브러리 버전 업데이트가 필요해요! 🔄

현재 프로젝트에서 사용 중인 JJWT 버전(0.12.5)이 최신 버전(0.12.6)보다 낮네요. 보안과 안정성을 위해 업데이트를 추천드립니다!

buildSrc/src/main/kotlin/Versions.kt에서 JJWT 버전을 "0.12.6"으로 업데이트해주세요.

추가로 제안드린 BOM 방식의 의존성 관리도 고려해보시면 좋을 것 같아요. 버전 관리가 더욱 편리해질 거예요! 😊

🔗 Analysis chain

의존성 버전 관리에 대한 제안

안녕하세요! JJWT 라이브러리 의존성 설정에 대해 몇 가지 조언을 드리고 싶어요. 📚

JJWT 버전의 호환성과 보안 업데이트를 확인해볼까요?

의존성 선언을 조금 더 명확하게 정리하면 어떨까요?

val jjwtVersion = Versions.JJWT

dependencies {
    implementation(platform("io.jsonwebtoken:jjwt-bom:$jjwtVersion"))
    implementation("io.jsonwebtoken:jjwt-api")
    runtimeOnly("io.jsonwebtoken:jjwt-impl")
    runtimeOnly("io.jsonwebtoken:jjwt-jackson")
}

이렇게 하면 BOM(Bill of Materials)을 통해 버전을 일관성 있게 관리할 수 있어요! 😊

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# JJWT 최신 버전 및 보안 권고사항 확인
curl -s "https://search.maven.org/solrsearch/select?q=g:io.jsonwebtoken+AND+a:jjwt-api&rows=5&wt=json" | jq '.response.docs[0].latestVersion'

Length of output: 152


Script:

#!/bin/bash
# Check current JJWT version in the project
fd "buildSrc|gradle.properties" --type f --exec cat {} \; | grep -i "JJWT"

# Check if BOM is already used somewhere in the project
fd "build.gradle.kts$" --type f --exec grep -l "jjwt-bom" {} \;

Length of output: 144


Script:

#!/bin/bash
# Try to find Versions.kt or similar files that might contain version definitions
fd "Versions.kt" --type f --exec cat {} \;

# Look for JJWT version definition in any Kotlin or Gradle files
rg "JJWT.*=.*\".*\"" --type kotlin --type gradle

Length of output: 450

}

tasks {
withType<Jar> { enabled = true }
withType<BootJar> { enabled = false }
}
Comment on lines +9 to +12
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Gradle 태스크 구성에 대한 제안

태스크 구성이 잘 되어있네요! 혹시 추가로 다음과 같은 설정도 고려해보시면 어떨까요?

tasks {
    withType<Jar> {
        enabled = true
        manifest {
            attributes(
                "Implementation-Title" to project.name,
                "Implementation-Version" to project.version
            )
        }
    }
    withType<BootJar> { enabled = false }
}

이렇게 하면 JAR 파일에 유용한 메타데이터가 포함되어 버전 관리가 더욱 수월해질 거예요! 😊

10 changes: 10 additions & 0 deletions support/jwt/src/main/kotlin/org/doorip/support/jwt/Extensions.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package org.doorip.support.jwt

import java.time.LocalDateTime
import java.time.ZoneId
import java.util.*

fun LocalDateTime.toDate(): Date {
val instant = this.atZone(ZoneId.systemDefault()).toInstant()
return Date.from(instant)
}
Comment on lines +7 to +10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

시스템 기본 시간대 사용에 대한 고려사항

친절하게 말씀드리자면, 시스템의 기본 시간대를 사용하시는 것이 향후 문제가 될 수 있어요. 특히 여러 서버에서 실행되거나 다른 시간대의 사용자들이 있는 경우에 예기치 않은 동작이 발생할 수 있답니다. 😊

다음과 같은 개선을 제안드립니다:

-fun LocalDateTime.toDate(): Date {
-    val instant = this.atZone(ZoneId.systemDefault()).toInstant()
-    return Date.from(instant)
+fun LocalDateTime.toDate(zoneId: ZoneId = ZoneId.of("UTC")): Date {
+    val instant = this.atZone(zoneId).toInstant()
+    return Date.from(instant)
}

이렇게 하면 다음과 같은 이점이 있어요:

  1. UTC를 기본값으로 사용하여 일관성 있는 시간 처리가 가능해요
  2. 필요한 경우 다른 시간대도 지정할 수 있어요
  3. 분산 시스템에서도 안정적으로 동작해요
📝 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.

Suggested change
fun LocalDateTime.toDate(): Date {
val instant = this.atZone(ZoneId.systemDefault()).toInstant()
return Date.from(instant)
}
fun LocalDateTime.toDate(zoneId: ZoneId = ZoneId.of("UTC")): Date {
val instant = this.atZone(zoneId).toInstant()
return Date.from(instant)
}

87 changes: 87 additions & 0 deletions support/jwt/src/main/kotlin/org/doorip/support/jwt/JwtProvider.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
package org.doorip.support.jwt

import com.fasterxml.jackson.core.JsonParseException
import com.fasterxml.jackson.core.type.TypeReference
import com.fasterxml.jackson.databind.DatabindException
import com.fasterxml.jackson.databind.ObjectMapper
import io.jsonwebtoken.Claims
import io.jsonwebtoken.JwtException
import io.jsonwebtoken.JwtParser
import io.jsonwebtoken.Jwts
import java.security.PublicKey
import java.time.LocalDateTime
import java.time.LocalDateTime.now
import java.util.*
import javax.crypto.SecretKey

class JwtProvider {
private val objectMapper = ObjectMapper()
private val typeReference = object : TypeReference<Map<String, String>>() {}

fun generate(id: String, expiration: LocalDateTime, key: SignatureKey): String {
return Jwts.builder()
.issuedAt(now().toDate())
.expiration(expiration.toDate())
.subject(id)
.signWith(key.key)
.compact()
}

fun validateAndGetSubject(token: String, key: SignatureKey): String? {
val payload = parsePayload(token, key) ?: return null
return payload.subject
}

fun validateAndGetSubject(token: String, key: SignatureKey, iss: String, aud: String): String? {
val payload = parsePayload(token, key) ?: return null
if (payload.issuer == iss && payload.audience.contains(aud)) {
return payload.subject
}
return null
}

fun validateAndGetClaim(token: String, key: SignatureKey, claimName: String): String? {
val payload = parsePayload(token, key) ?: return null
return payload.get(claimName, String::class.java)
}

fun parseHeader(token: String): Map<String, String>? {
try {
val header = token.split(".")[0]
val decoder = Base64.getUrlDecoder()
val decoded = String(decoder.decode(header))
return objectMapper.readValue(decoded, typeReference)
} catch (e: DatabindException) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

예외 처리를 개선해 주세요

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

예외 처리를 개선해 주세요

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)

return null
}
}

private fun parsePayload(token: String, key: SignatureKey): Claims? {
try {
val parser = when (val signatureKey = key.key) {
is SecretKey -> generateParser(signatureKey)
is PublicKey -> generateParser(signatureKey)
else -> return null
Comment on lines +66 to +68
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

when 구문의 가독성을 높여 주세요

when 구문에서 else 대신 명시적으로 모든 경우를 처리해 주세요. 키의 타입이 SecretKeyPublicKey가 아닌 경우를 대비해 예외를 처리해 주세요.

}
return parser.parseSignedClaims(token).payload
} catch (e: JwtException) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

예외 처리를 개선해 주세요

JwtException을 캡처했지만 예외를 무시하고 있어 원래 예외 정보가 손실될 수 있습니다. 예외를 로깅하거나 적절한 처리를 추가해 주세요.

🧰 Tools
🪛 detekt (1.23.7)

[warning] 71-71: The caught exception is swallowed. The original exception could be lost.

(detekt.exceptions.SwallowedException)

return null
}
}

private fun generateParser(key: SecretKey): JwtParser {
return Jwts.parser()
.verifyWith(key)
.build()
}

private fun generateParser(key: PublicKey): JwtParser {
return Jwts.parser()
.verifyWith(key)
.build()
}
}
13 changes: 13 additions & 0 deletions support/jwt/src/main/kotlin/org/doorip/support/jwt/JwtSecretKey.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package org.doorip.support.jwt

import io.jsonwebtoken.security.Keys
import java.security.Key

class JwtSecretKey(
private val secret: String,
) : SignatureKey {

override val key: Key by lazy {
Keys.hmacShaKeyFor(secret.toByteArray())
}
}
34 changes: 34 additions & 0 deletions support/jwt/src/main/kotlin/org/doorip/support/jwt/RSAPublicKey.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
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 {
Comment on lines +9 to +13
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

생성자 매개변수 유효성 검사가 필요해요! 🔍

현재 구현에서는 입력값 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.

Suggested change
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 알고리즘만 지원합니다" }
}


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
}
}
Comment on lines +15 to +33
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

예외 처리를 개선해 주세요 🛡️

현재 구현에는 몇 가지 개선이 필요한 부분이 있어요:

  1. 모든 예외를 null로 처리하는 것은 위험할 수 있어요
  2. 구체적인 예외 타입별로 처리가 필요해요
  3. 의미 있는 에러 메시지가 있으면 좋겠어요

다음과 같이 개선하는 것을 추천드립니다:

-    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.

Suggested change
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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

support:jwt 모듈에서 예외 클래스를 가진 domain 모듈을 의존하지 않게 하기 위함이라 예외보단 null로 관리해서 해당 key를 사용하는 클래스에서 예외 핸들링을 해주는게 맞는거 같아

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package org.doorip.support.jwt

import java.security.Key

interface SignatureKey {
val key: Key?
}
Comment on lines +5 to +7
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

인터페이스 설계를 개선해 주세요 🎨

안녕하세요! SignatureKey 인터페이스와 관련해서 몇 가지 개선사항을 제안드립니다:

  1. key 프로퍼티가 nullable인 이유가 명확하지 않아요. 키가 없는 상황이 실제로 필요한지 검토해 주세요.
  2. 인터페이스의 목적과 사용법을 설명하는 KDoc 문서를 추가하면 좋겠어요.
  3. 키의 유효성을 검증하는 메서드도 추가하면 좋을 것 같아요.

다음과 같이 개선하는 것을 제안드립니다:

+/**
+ * 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.

Suggested change
interface SignatureKey {
val key: Key?
}
/**
* JWT 서명에 사용되는 키를 제공하는 인터페이스입니다.
*/
interface SignatureKey {
val key: Key
/**
* 키의 유효성을 검증합니다.
* @throws IllegalStateException 키가 유효하지 않은 경우
*/
fun validate()
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

key가 nullable인 이유는 RSAPublicKey에서 키 생성 예외가 발생한 경우 예외를 던지지 않고 null을 저장해서 관리하기 위함이긴 해

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Loading