Feature/52 fix security + oAuth setting #53
Conversation
|
Caution Review failedThe pull request is closed. Walkthrough이번 변경 사항은 OAuth2 인증 로직과 보안 설정의 개선에 중점을 둡니다. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SecurityConfig
participant OAuth2SuccessHandler
participant OAuth2FailureHandler
participant OauthTokenService
participant CustomErrorController
Client->>SecurityConfig: OAuth2 로그인 요청
alt 인증 성공
SecurityConfig->>OAuth2SuccessHandler: onAuthenticationSuccess()
OAuth2SuccessHandler->>OauthTokenService: generateTokenResponse(loginId)
OauthTokenService-->>OAuth2SuccessHandler: TokenResponse
OAuth2SuccessHandler-->>Client: JSON 토큰 응답
else 인증 실패
SecurityConfig->>OAuth2FailureHandler: onAuthenticationFailure()
OAuth2FailureHandler-->>Client: JSON 오류 응답 (HTTP 400)
end
Client->>CustomErrorController: /error 호출
CustomErrorController-->>Client: 오류 정보 반환
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
✨ Finishing Touches
🪧 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: 4
🧹 Nitpick comments (5)
src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/oauth/handler/CustomOAuth2AuthenticationFailureHandler.kt (1)
21-26: 응답 형식 개선 및 사용자 친화적인 오류 메시지응답 콘텐츠 타입에 charset을 추가하고 구조화된 JSON 형식으로 오류 응답을 반환하는 것은 좋은 방법입니다. 그러나 하드코딩된 오류 메시지는 국제화에 제한이 있을 수 있습니다.
향후에는 다음과 같이 메시지 소스를 사용하여 국제화를 지원하는 것을 고려해보세요:
- "error" to "구글 로그인 실패", - "message" to (exception.message ?: "올바른 이메일 도메인이 아닙니다..") + "error" to messageSource.getMessage("oauth.login.failure", null, locale), + "message" to (exception.message ?: messageSource.getMessage("email.domain.not.allowed", null, locale))src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/exception/CustomErrorController.kt (1)
20-27: 에러 응답 구조 및 국제화 고려에러 응답 구조가 잘 정의되어 있습니다. 그러나 하드코딩된 한국어 메시지는 국제화에 제한이 있을 수 있습니다. 또한 서버 오류 메시지에는 민감한 정보가 노출되지 않도록 주의해야 합니다.
향후에는 다음과 같은 개선을 고려해보세요:
- 메시지 소스를 사용하여 국제화 지원
- 개발 환경과 프로덕션 환경에서 다른 수준의 오류 세부 정보 제공 (프로덕션에서는 최소한의 정보만 노출)
- val message = request.getAttribute("javax.servlet.error.message")?.toString() - ?: "알 수 없는 오류가 발생했습니다." + val environment = Environment.getActiveProfiles() + val message = if (Arrays.asList(environment).contains("prod")) { + messageSource.getMessage("error.generic", null, locale) + } else { + request.getAttribute("javax.servlet.error.message")?.toString() + ?: messageSource.getMessage("error.unknown", null, locale) + }src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/oauth/handler/CustomOAuth2AuthenticationSuccessHandler.kt (2)
25-27: 타입 체크 및 예외 처리 개선인증 주체(principal)에 대한 타입 체크를 추가하고 명확한 예외를 던지는 것은 좋은 방법입니다. 그러나 다른 오류 메시지와 일관되게 한국어로 작성하는 것이 좋을 것 같습니다.
예외 메시지를 한국어로 변경하여 일관성을 유지하세요:
- ?: throw IllegalStateException("Authentication principal must be CustomOauth2UserDetails") + ?: throw IllegalStateException("인증 주체는 CustomOauth2UserDetails 타입이어야 합니다")
37-41: 응답 작성 방식 개선MediaType 상수를 사용하여 콘텐츠 타입을 설정하는 것은 좋은 방법입니다. 그러나 writer.flush()를 명시적으로 호출하는 것은 불필요할 수 있습니다.
response.writer.close()를 호출하여 자원을 정리하고 자동으로 flush되도록 하는 것이 더 좋은 방법입니다:
response.writer.write(objectMapper.writeValueAsString(tokenResponse)) - response.writer.flush() + response.writer.close()또는 try-with-resources 구문을 사용하여 자원을 안전하게 관리할 수 있습니다.
src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/config/SecurityConfig.kt (1)
60-67: 예외 처리 개선인증 예외 발생 시 JSON 형식의 표준화된 오류 응답을 제공하도록 개선되었습니다. 이는 클라이언트 측에서 오류를 일관되게 처리할 수 있게 해주는 좋은 변경입니다.
다만, 모든 인증 예외를 BAD_REQUEST(400)로 처리하는 것은 일부 상황에서 부적절할 수 있습니다. 인증 관련 오류는 일반적으로 UNAUTHORIZED(401)나 FORBIDDEN(403)으로 처리합니다.
- response.status = HttpServletResponse.SC_BAD_REQUEST + response.status = HttpServletResponse.SC_UNAUTHORIZED
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/main/kotlin/entry/dsm/gitauth/equusgithubauth/domain/user/presentation/dto/response/TokenResponse.kt(1 hunks)src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/config/SecurityConfig.kt(4 hunks)src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/exception/CustomErrorController.kt(1 hunks)src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/oauth/exception/EmailDomainNotAllowedException.kt(1 hunks)src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/oauth/handler/CustomOAuth2AuthenticationFailureHandler.kt(1 hunks)src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/oauth/handler/CustomOAuth2AuthenticationSuccessHandler.kt(1 hunks)src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/oauth/service/GoogleOauthService.kt(1 hunks)src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/oauth/service/component/EmailDomainValidator.kt(1 hunks)src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/oauth/service/component/OauthTokenService.kt(1 hunks)
🔇 Additional comments (10)
src/main/kotlin/entry/dsm/gitauth/equusgithubauth/domain/user/presentation/dto/response/TokenResponse.kt (1)
9-9: 불필요한 후행 쉼표 제거, 코드 스타일 개선불필요한 후행 쉼표(trailing comma)를 제거하여 코드 스타일이 개선되었습니다. 클래스의 기능에는 영향을 주지 않는 변경사항입니다.
src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/oauth/service/GoogleOauthService.kt (1)
50-50: 토큰 생성 로직 리팩터링기존에 GoogleOauthService 내에 있던 토큰 생성 로직이 제거되고, 단순히 사용자 정보와 원본 OAuth2 속성을 반환하도록 변경되었습니다. 이는 토큰 생성 책임을 새로운 OauthTokenService 컴포넌트로 이동시킨 것으로 보이며, 단일 책임 원칙(SRP)을 더 잘 준수하게 되었습니다.
src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/oauth/service/component/OauthTokenService.kt (2)
8-16: 새로운 OauthTokenService 컴포넌트 추가토큰 생성 로직을 담당하는 새로운 컴포넌트가 추가되었습니다. 이 컴포넌트는 JwtTokenProvider를 사용하여 loginId를 기반으로 TokenResponse를 생성합니다. 이런 분리는 관심사의 분리(Separation of Concerns) 원칙을 따르며, 코드의 유지보수성과 테스트 용이성을 향상시킵니다.
13-15: generateTokenResponse 메서드 구현loginId를 받아서 TokenResponse를 반환하는 간단한 메서드입니다. 현재는 JwtTokenProvider의 generateToken 메서드를 그대로 호출하여 반환하고 있습니다. 이후 요구사항 변경으로 토큰 생성 로직이 확장되어야 할 경우 이 메서드 내에서 처리할 수 있어 확장성이 좋습니다.
src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/oauth/handler/CustomOAuth2AuthenticationFailureHandler.kt (2)
3-3: Component 애노테이션과 ObjectMapper 추가에 대한 개선Component 애노테이션과 ObjectMapper를 추가한 것은 좋은 변경입니다. 이를 통해 핸들러를 스프링 컨테이너가 관리할 수 있고, JSON 직렬화를 위한 표준 도구를 사용하게 되었습니다.
Also applies to: 8-10, 13-14
28-28: ObjectMapper를 사용한 응답 작성ObjectMapper를 사용하여 JSON 응답을 작성하는 것은 적절한 방법입니다. 오류 응답 형식이 일관되게 유지됩니다.
src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/oauth/handler/CustomOAuth2AuthenticationSuccessHandler.kt (2)
4-6: OauthTokenService 의존성 주입 개선토큰 생성 로직을 OauthTokenService로 추출한 것은 좋은 설계 결정입니다. 이를 통해 코드 재사용성이 향상되고 단일 책임 원칙을 더 잘 준수할 수 있게 되었습니다.
Also applies to: 17-17
30-36: 토큰 생성 로직 리팩토링토큰 생성 로직을 OauthTokenService로 위임하는 것은 책임 분리의 좋은 예입니다. 코드가 더 간결해지고 각 컴포넌트의 역할이 명확해졌습니다.
src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/config/SecurityConfig.kt (2)
29-30: 의존성 주입 패턴 적용을 통한 코드 개선핸들러를 내부에서 생성하는 대신 의존성 주입을 활용하여 코드의 테스트 용이성과 유지보수성이 향상되었습니다. 이는 스프링의 권장 패턴을 따르는 좋은 변경입니다.
78-79: 핸들러 주입 방식으로 변경내부에서 직접 핸들러를 생성하는 대신 주입된 핸들러를 사용하도록 변경되었습니다. 이는 코드의 결합도를 낮추고 테스트 용이성을 높이는 좋은 변경입니다.
...lin/entry/dsm/gitauth/equusgithubauth/global/oauth/service/component/EmailDomainValidator.kt
Outdated
Show resolved
Hide resolved
...n/entry/dsm/gitauth/equusgithubauth/global/oauth/exception/EmailDomainNotAllowedException.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/exception/CustomErrorController.kt
Show resolved
Hide resolved
| .requestMatchers( | ||
| "/", "/login", "/oauth2/**", "/api/github/auth", "/api/github/auth/**", | ||
| "/oauth2/authorize/**", "/error", "/notice/**", "/notice", "/reports", | ||
| "/oauth2/authorize/**", "/error" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
엔드포인트 접근 제한 변경 확인 필요
엔드포인트 접근 권한 목록이 변경되었습니다. "/notice/", "/notice", "/reports" 엔드포인트가 제거되고 "/oauth2/authorize/"가 추가되었습니다. 이 변경으로 인해 기존 기능에 영향이 없는지 확인해 주세요.
🏁 Script executed:
#!/bin/bash
# 해당 엔드포인트들이 다른 곳에서 사용되고 있는지 확인
echo "Checking for removed endpoints usage in controllers..."
rg -l "(/notice|/reports)" --type=kotlin | grep -v SecurityConfig.ktLength of output: 383
엔드포인트 변경으로 인한 기능 영향 검토 및 수정 필요
보안 설정에서 "/notice/", "/notice", "/reports" 엔드포인트를 제거하고 "/oauth2/authorize/"를 추가한 변경 사항과 관련하여 아래 컨트롤러에서 해당 엔드포인트가 여전히 사용되는 것으로 확인되었습니다:
src/main/kotlin/entry/dsm/gitauth/equusgithubauth/domain/notice/presentation/NoticeController.ktsrc/main/kotlin/entry/dsm/gitauth/equusgithubauth/domain/report/presentation/ReportController.kt
이에 따라, 기존 기능에 영향을 미칠 가능성이 있으므로, 아래 사항을 확인해 주세요:
- 변경이 의도된 사항이라면, 컨트롤러의 엔드포인트 수정이 필요합니다.
- 만약 기존 기능 유지를 원한다면, SecurityConfig의 변경 사항을 재검토해 주세요.
There was a problem hiding this comment.
괜찮아 저건 너무 권한을 오픈 해놔서 지운 거야 어짜피 필요한 엔드포인트는 GET 뿐이라
There was a problem hiding this comment.
@qkrwndnjs1075 확인했습니다! 보안을 강화하기 위해 의도적으로 권한 범위를 줄이신 것이군요. GET 메소드만 허용하고 나머지 작업(POST, PUT, DELETE 등)은 인증이 필요하도록 제한하신 것은 보안 측면에서 매우 적절한 접근법입니다. 이렇게 하면 공개적으로 조회는 가능하되 데이터 수정은 인증된 사용자만 가능하게 되어 보안이 향상됩니다. 👍
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/oauth/handler/CustomOAuth2AuthenticationFailureHandler.kt (2)
3-14: 컴포넌트 주입과 ObjectMapper 초기화에 대한 개선 가능성
ObjectMapper를 직접 생성하는 대신 Spring의@Autowired를 통해 주입받는 것이 더 효율적일 수 있습니다. 이렇게 하면 다른 곳에서 사용하는 ObjectMapper 설정과의 일관성을 유지할 수 있습니다.@Component class CustomOAuth2AuthenticationFailureHandler : AuthenticationFailureHandler { - private val objectMapper = ObjectMapper() + @Autowired + private lateinit var objectMapper: ObjectMapper override fun onAuthenticationFailure(또는 생성자 주입을 사용할 수도 있습니다:
@Component -class CustomOAuth2AuthenticationFailureHandler : AuthenticationFailureHandler { +class CustomOAuth2AuthenticationFailureHandler( + private val objectMapper: ObjectMapper +) : AuthenticationFailureHandler { - private val objectMapper = ObjectMapper() override fun onAuthenticationFailure(
20-29: 에러 응답 구조 개선된 점에러 응답을 구조화하고 한글 메시지를 포함한 것은 사용자 경험 측면에서 좋은 개선입니다. 다만 아래 내용을 고려해보세요:
- 하드코딩된 에러 메시지를 상수로 분리하거나 메시지 소스를 사용하는 것이 유지보수에 더 좋을 수 있습니다.
- 디버깅을 위해 예외 타입이나 추가 정보를 포함하는 것을 고려해보세요.
override fun onAuthenticationFailure( request: HttpServletRequest, response: HttpServletResponse, exception: AuthenticationException ) { response.status = HttpServletResponse.SC_UNAUTHORIZED response.contentType = "application/json; charset=UTF-8" val errorResponse = mapOf( "error" to "구글 로그인 실패", "message" to (exception.message ?: "올바른 이메일 도메인이 아닙니다.."), + "type" to exception.javaClass.simpleName ) response.writer.write(objectMapper.writeValueAsString(errorResponse)) }src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/oauth/handler/CustomOAuth2AuthenticationSuccessHandler.kt (1)
20-41: 인증 성공 로직 리팩토링인증 성공 시 토큰 생성 로직을
OauthTokenService로 위임한 것은 코드 품질 측면에서 좋은 개선입니다. 몇 가지 제안과 관찰 사항이 있습니다:
- 주석은 유용하지만, 코드가 충분히 자명하다면 불필요할 수 있습니다.
- 불필요한 빈 줄이 있습니다.
flush()호출은 좋지만,close()도 고려해볼 수 있습니다.override fun onAuthenticationSuccess( request: HttpServletRequest, response: HttpServletResponse, authentication: Authentication ) { - // CustomOauth2UserDetails에서 loginId 추출 val oauthUser = authentication.principal as? CustomOauth2UserDetails ?: throw IllegalStateException("Authentication principal must be CustomOauth2UserDetails") - val loginId = oauthUser.username - val tokenResponse: TokenResponse = oauthTokenService.generateTokenResponse(loginId) - response.status = HttpServletResponse.SC_OK response.contentType = MediaType.APPLICATION_JSON_VALUE response.characterEncoding = "UTF-8" response.writer.write(objectMapper.writeValueAsString(tokenResponse)) response.writer.flush() + response.writer.close() }src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/exception/CustomErrorController.kt (1)
1-28: 전역 에러 처리 컨트롤러 구현전역 에러 처리를 위한
CustomErrorController구현은 좋은 접근 방식입니다. 다음 개선 사항을 고려해 보세요:
- Spring Boot 2.3 이후로는
ErrorController대신@ControllerAdvice와@ExceptionHandler를 사용하는 것이 권장됩니다.- 에러 메시지와 상태 코드에 더 유연하게 대응할 수 있도록 개선이 가능합니다.
- 로깅을 추가하여 서버 측에서 에러 추적이 용이하도록 할 수 있습니다.
package entry.dsm.gitauth.equusgithubauth.global.exception +import org.slf4j.LoggerFactory import org.springframework.boot.web.servlet.error.ErrorController import org.springframework.http.HttpStatus import org.springframework.http.ResponseEntity import org.springframework.web.bind.annotation.RequestMapping import org.springframework.web.bind.annotation.RestController import jakarta.servlet.http.HttpServletRequest @RestController class CustomErrorController : ErrorController { + private val logger = LoggerFactory.getLogger(CustomErrorController::class.java) + @RequestMapping("/error") fun handleError(request: HttpServletRequest): ResponseEntity<Map<String, Any>> { val status = request.getAttribute("javax.servlet.error.status_code") as? Int ?: HttpStatus.INTERNAL_SERVER_ERROR.value() val message = request.getAttribute("javax.servlet.error.message")?.toString() ?: "알 수 없는 오류가 발생했습니다." + val exception = request.getAttribute("javax.servlet.error.exception") as? Throwable + + logger.error("Error handling request: $message", exception) val errorResponse = mapOf( "error" to "서버 오류", "message" to message, "status" to status ) return ResponseEntity(errorResponse, HttpStatus.valueOf(status)) } }또는 최신 Spring Boot 스타일로 변경하는 것을 고려해보세요:
@RestControllerAdvice class GlobalExceptionHandler { private val logger = LoggerFactory.getLogger(GlobalExceptionHandler::class.java) @ExceptionHandler(Exception::class) fun handleException(ex: Exception): ResponseEntity<Map<String, Any>> { logger.error("Unhandled exception occurred", ex) val errorResponse = mapOf( "error" to "서버 오류", "message" to (ex.message ?: "알 수 없는 오류가 발생했습니다."), "status" to HttpStatus.INTERNAL_SERVER_ERROR.value() ) return ResponseEntity(errorResponse, HttpStatus.INTERNAL_SERVER_ERROR) } // 다른 특정 예외에 대한 핸들러 추가 }src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/config/SecurityConfig.kt (1)
60-67: 인증 예외 처리 로직 추가인증 실패 시 구조화된 JSON 응답을 제공하는 예외 처리 로직이 추가되었습니다. 이는 클라이언트에게 더 명확한 에러 메시지를 제공하여 디버깅 및 사용자 경험을 개선합니다.
하지만 하드코딩된. 에러 메시지를 사용하는 대신 별도의 메시지 소스나 상수를 사용하는 것이 향후 유지보수에 더 좋을 수 있습니다.
- val errorMessage = authException?.message ?: "허용되지 않은 요청입니다." - response.writer.write("""{ "error": "요청 실패", "message": "$errorMessage" }""") + val errorMessage = authException?.message ?: ErrorMessages.UNAUTHORIZED_REQUEST + response.writer.write("""{ "error": ErrorMessages.REQUEST_FAILED, "message": "$errorMessage" }""")별도의 ErrorMessages 클래스를 만들어 에러 메시지를 관리하는 것을 고려해보세요.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/main/kotlin/entry/dsm/gitauth/equusgithubauth/domain/user/presentation/dto/response/TokenResponse.kt(1 hunks)src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/config/SecurityConfig.kt(4 hunks)src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/exception/CustomErrorController.kt(1 hunks)src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/oauth/exception/EmailDomainNotAllowedException.kt(1 hunks)src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/oauth/handler/CustomOAuth2AuthenticationFailureHandler.kt(1 hunks)src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/oauth/handler/CustomOAuth2AuthenticationSuccessHandler.kt(1 hunks)src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/oauth/service/GoogleOauthService.kt(1 hunks)src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/oauth/service/component/EmailDomainValidator.kt(1 hunks)src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/oauth/service/component/OauthTokenService.kt(1 hunks)
🔇 Additional comments (8)
src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/oauth/service/component/OauthTokenService.kt (1)
1-16: 토큰 생성 로직의 분리는 좋은 리팩토링입니다.토큰 생성 로직을 별도의 서비스로 분리해서 재사용성을 높인 것은 좋은 접근 방식입니다. 단일 책임 원칙(SRP)을 따르고 있으며, 다른 컴포넌트에서 쉽게 주입하여 사용할 수 있게 되었습니다.
src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/oauth/handler/CustomOAuth2AuthenticationSuccessHandler.kt (1)
4-18: 컴포넌트 구조와 의존성 주입 개선새로 도입된
OauthTokenService를 생성자 주입을 통해 사용하는 것은 좋은 접근 방식입니다. 이는 테스트 용이성과 결합도 감소에 도움이 됩니다.src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/config/SecurityConfig.kt (4)
3-3: import 변경 적절함HttpServletResponse import가 추가되었고, 불필요한 import가 제거되었습니다. 이는 새로운 예외 처리 구현에 필요한 변경으로 적절합니다.
Also applies to: 9-9
29-30: 의존성 주입 패턴 적용 개선생성자에 custom handler들을 직접 주입받도록 변경한 것은 좋은 접근 방식입니다. 이전에는 내부에서 직접 인스턴스를 생성했을 것으로 보이는데, 이런 의존성 주입 패턴을 사용함으로써 테스트 용이성과 결합도를 개선했습니다.
52-52: error 경로 permitAll에 추가"/error" 경로를 permitAll에 추가한 것은 적절합니다. 이는 새롭게 구현한 에러 처리 메커니즘이 인증되지 않은 사용자에게도 적절한 에러 응답을 반환할 수 있도록 합니다.
78-79: OAuth2 핸들러 의존성 주입 활용OAuth2 로그인 설정에서 주입된 핸들러를 사용하도록 변경한 것은 좋은 개선입니다. 이전에는 직접 인스턴스를 생성했던 것으로 보이는데, 이런 방식은 테스트가 어렵고 결합도가 높아집니다. 스프링의 DI 컨테이너를 활용하는 현재 방식이 더 바람직합니다.
src/main/kotlin/entry/dsm/gitauth/equusgithubauth/domain/user/presentation/dto/response/TokenResponse.kt (1)
9-9: 불필요한 후행 쉼표 제거 - 좋은 정리입니다!후행 쉼표(trailing comma)를 제거하여 코드가 더 깔끔해졌습니다. Kotlin에서는 후행 쉼표가 선택적이지만, 일관된 코드 스타일을 유지하는 것이 좋습니다.
src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/oauth/service/GoogleOauthService.kt (1)
50-50: 토큰 생성 로직 분리 - 좋은 리팩토링입니다!토큰 생성 로직을
GoogleOauthService에서 제거하고 별도의OauthTokenService로 이동한 것으로 보입니다. 이는 단일 책임 원칙(SRP)을 더 잘 따르는 방향으로의 개선입니다. 사용자 정보 로드와 토큰 생성이 분리되어 각 클래스의 책임이 더 명확해졌습니다.PR 설명에 언급된 대로 "oAuth 로직에서 JWT 반환 로직 리팩토링"이 잘 이루어진 것으로 보입니다.
...n/entry/dsm/gitauth/equusgithubauth/global/oauth/exception/EmailDomainNotAllowedException.kt
Outdated
Show resolved
Hide resolved
...lin/entry/dsm/gitauth/equusgithubauth/global/oauth/service/component/EmailDomainValidator.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/oauth/exception/EmailDomainNotAllowedException.kt (1)
5-5: 하드코딩된 에러 메시지에 대한 검토 필요
AuthenticationException을 상속받는 것은 좋은 변경이지만, 에러 메시지가 하드코딩 되어 있습니다. 이전 구현에서는ErrorCode열거형을 사용하여 에러 코드를 표준화했었는데, 현재 방식은 다국어 지원이나 일관된 오류 처리가 어려워질 수 있습니다.PR 설명에서 언급하신 것처럼 오류 처리를 하드코딩한 접근 방식을 수정할 의향이 있으시다면, 아래와 같은 방식을 고려해 보세요:
- class EmailDomainNotAllowedException : AuthenticationException("허용되지 않은 이메일 도메인입니다") + class EmailDomainNotAllowedException( + private val errorCode: ErrorCode = ErrorCode.EMAIL_DOMAIN_IN_NOT_ALLOWED_EXCEPTION + ) : AuthenticationException(errorCode.message)이렇게 하면 Spring Security의 인증 예외 흐름을 유지하면서도 표준화된 오류 코드 시스템을 활용할 수 있습니다.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/exception/CustomErrorController.kt(1 hunks)src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/oauth/exception/EmailDomainNotAllowedException.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/exception/CustomErrorController.kt
🔇 Additional comments (1)
src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/oauth/exception/EmailDomainNotAllowedException.kt (1)
3-3: Spring Security의 AuthenticationException 임포트 적용 완료
org.springframework.security.core.AuthenticationException을 임포트하도록 변경하셨네요. 이 변경은 Spring Security의 인증 흐름과 더 잘 통합되는 올바른 방향입니다.
qkrwndnjs1075
force-pushed
the
feature/52-fix-security
branch
from
8c8aec1 to
8c81902
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/oauth/handler/CustomOAuth2AuthenticationFailureHandler.kt (1)
13-13: ObjectMapper 직접 생성 대신 Bean 주입 고려
private val objectMapper = ObjectMapper()를 통해 직접 생성하기보다는, 스프링에서 제공하는 기본ObjectMapperBean을 주입받으면 공통 설정(직렬화/역직렬화 옵션 등)을 재사용할 수 있어 유지보수성이 향상됩니다.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/exception/CustomErrorController.kt(1 hunks)src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/oauth/exception/EmailDomainNotAllowedException.kt(1 hunks)src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/oauth/handler/CustomOAuth2AuthenticationFailureHandler.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/oauth/exception/EmailDomainNotAllowedException.kt
- src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/exception/CustomErrorController.kt
🔇 Additional comments (4)
src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/oauth/handler/CustomOAuth2AuthenticationFailureHandler.kt (4)
3-3: ObjectMapper import 추가 확인
ObjectMapper를 통해 JSON 직렬화를 수행하려는 의도와 일치합니다. OAuth 인증 실패 시 에러 정보를 쉽게 반환할 수 있어 적절해 보입니다.
8-10: 스프링 Bean 등록 사용
클래스에@Component를 적용해 핸들러를 스프링 Bean으로 등록한 것은 보안 설정에서 주입하기 용이합니다.
18-28: OAuth 인증 예외 처리 로직 점검
응답 상태 코드를 401로 설정하고, JSON 형태로 에러 정보를 반환하는 접근이 명확합니다. 다만 국제화(i18n) 적용이 필요한 경우를 대비해 메시지를 리소스 파일로 관리하거나, JSON 변환 과정에서 발생할 수 있는 예외 상황에 대한 로깅·관리 방안을 함께 고려해볼 수 있습니다.
30-30: 클래스 마무리
핸들러 내부 로직이 간결해 가독성이 좋습니다. 현재 구현 상태를 유지해도 문제없어 보입니다.
| val (statusCode, errorMessage) = | ||
| when (exception) { | ||
| is CustomOauth2AuthenticationException -> { | ||
| val cause = exception.cause | ||
| if (cause is CustomException) { | ||
| cause.statusCode to cause.message | ||
| } else { | ||
| ErrorCode.INTERNAL_SERVER_ERROR.status to "OAuth2 인증 실패" | ||
| } | ||
| } | ||
| is CustomException -> exception.statusCode to exception.message | ||
| else -> ErrorCode.INTERNAL_SERVER_ERROR.status to "OAuth2 인증 실패" | ||
| } | ||
|
|
||
| val errorResponse = | ||
| ErrorResponse( | ||
| statusCode = statusCode, | ||
| message = errorMessage ?: "Unknown error", | ||
| timestamp = LocalDateTime.now(), | ||
| path = request.requestURI, | ||
| method = request.method, | ||
| ) | ||
|
|
||
| response.status = statusCode |
There was a problem hiding this comment.
이렇게 하면 oauth 부분에서 터지는 오류만 잡을수있는거 아닌가요?
There was a problem hiding this comment.
Oauth 흐름이 실패하면 failureHandler로 가기 때문에 failure Handler에 구현했습니다. 선배가 말했던 것처럼 만들어도 에러를 못 잡더라고요
There was a problem hiding this comment.
그래도 oauth부분 말고도 jwt 관련된 곳에서 터질 수 있으니 만들어주세요
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/exception/GlobalExceptionFilter.kt (2)
14-31: 예외 처리 범위를 확장하는 것이 좋습니다.현재 필터는
CustomException만 처리하고 있습니다. 다른 유형의 예외(예: 시스템 예외, 런타임 예외)도 처리하여 클라이언트에 일관된 오류 응답을 제공하는 것이 좋습니다.try { filterChain.doFilter(request, response) } catch (e: CustomException) { val errorResponse = ErrorResponse( statusCode = e.statusCode, message = e.message, timestamp = LocalDateTime.now(), path = request.requestURI, method = request.method ) writeErrorResponse(response, e.statusCode, errorResponse) +} catch (e: Exception) { + val errorResponse = ErrorResponse( + statusCode = HttpStatus.INTERNAL_SERVER_ERROR.value(), + message = "서버 내부 오류가 발생했습니다", + timestamp = LocalDateTime.now(), + path = request.requestURI, + method = request.method + ) + writeErrorResponse(response, HttpStatus.INTERNAL_SERVER_ERROR.value(), errorResponse) }
14-31: 예외 로깅을 추가하는 것을 고려해보세요.예외가 발생했을 때 로그를 남기면 문제 해결에 도움이 됩니다. SLF4J와 같은 로깅 프레임워크를 사용하여 예외 정보를 기록하는 것이 좋습니다.
+private val log = LoggerFactory.getLogger(GlobalExceptionFilter::class.java) override fun doFilterInternal( request: HttpServletRequest, response: HttpServletResponse, filterChain: FilterChain ) { try { filterChain.doFilter(request, response) } catch (e: CustomException) { + log.error("CustomException 발생: ${e.message}", e) val errorResponse = ErrorResponse( statusCode = e.statusCode, message = e.message, timestamp = LocalDateTime.now(), path = request.requestURI, method = request.method ) writeErrorResponse(response, e.statusCode, errorResponse) } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/exception/GlobalExceptionFilter.kt(1 hunks)
🔇 Additional comments (4)
src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/exception/GlobalExceptionFilter.kt (4)
1-10: 패키지 및 imports 선언이 적절합니다.필요한 모든 종속성이 올바르게 가져와졌으며, 패키지 구조가 전역 예외 처리를 위한 필터에 적합합니다.
11-13: 필터 클래스 정의가 잘 구성되었습니다.
OncePerRequestFilter를 상속하는 것은 요청당 한 번만 필터가 실행되도록 보장하는 좋은 방법입니다. 생성자를 통한ObjectMapper주입도 적절합니다.
33-43: writeErrorResponse 메서드가 잘 구현되었습니다.응답 상태, 콘텐츠 타입 및 문자 인코딩을 적절하게 설정하고 있습니다. ObjectMapper를 사용하여 ErrorResponse 객체를 JSON으로 직렬화하는 접근 방식도 적절합니다.
1-44:Details
✅ Verification successful
CustomException 클래스가 코드베이스에 존재하는지 확인하세요.
이 필터는
CustomException클래스에 의존하고 있습니다. 이 클래스가 코드베이스에 정의되어 있고 필요한statusCode및message속성을 가지고 있는지 확인해주세요.
🏁 Script executed:
#!/bin/bash # CustomException 클래스 확인 echo "CustomException 클래스 찾기:" rg -l "class CustomException" --type kotlin # ErrorResponse 클래스 확인 echo "ErrorResponse 클래스 찾기:" rg -l "class ErrorResponse" --type kotlinLength of output: 369
검증 완료: CustomException 및 ErrorResponse 클래스 모두 코드베이스에 존재함
CustomException클래스는src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/exception/CustomException.kt파일에 정의되어 있습니다.ErrorResponse클래스는src/main/kotlin/entry/dsm/gitauth/equusgithubauth/global/exception/ErrorResponse.kt파일에 정의되어 있습니다.필터에서 이 두 클래스를 올바르게 사용하고 있으며, 필요한 속성(
statusCode,message등)도 포함된 것으로 확인되어 코드에 추가 수정 사항은 없습니다.
| request.getAttribute("jakarta.servlet.error.status_code") as? Int | ||
| ?: HttpStatus.INTERNAL_SERVER_ERROR.value() | ||
| val message = | ||
| request.getAttribute("jakarta.servlet.error.message")?.toString() | ||
| ?: "알 수 없는 오류가 발생했습니다." | ||
|
|
||
| val errorResponse = | ||
| mapOf( | ||
| "error" to "서버 오류", | ||
| "message" to message, | ||
| "status" to status, | ||
| ) |
| ErrorCode.INTERNAL_SERVER_ERROR.status to "OAuth2 인증 실패" | ||
| } | ||
| } | ||
| is CustomException -> exception.statusCode to exception.message | ||
| else -> ErrorCode.INTERNAL_SERVER_ERROR.status to "OAuth2 인증 실패" |
#️⃣연관된 이슈
📝작업 내용
💬리뷰 요구사항(선택)
Summary by CodeRabbit
새로운 기능
버그 수정
리팩토링