[Feat] 로그인 기능 추가 - #11
Open
mimimya wants to merge 65 commits into
Open
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 1 minutes and 52 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughKeycloak 연동 인증 흐름에 필요한 DTO/커맨드가 추가되고 AuthController가 도입되어 /token, /refresh, /logout 엔드포인트가 구현되었으며, 서비스 및 Keycloak 연동 구현은 커맨드 입력과 새로운 Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthController
participant AuthService
participant KeycloakAuthService
participant Keycloak
Client->>AuthController: POST /api/v1/token (TokenRequest)
AuthController->>AuthController: TokenRequest.toCommand()
AuthController->>AuthService: getToken(LoginCommand)
AuthService->>KeycloakAuthService: getToken(LoginCommand)
KeycloakAuthService->>Keycloak: 인증 요청 (email, password)
Keycloak-->>KeycloakAuthService: KeycloakTokenResponse
KeycloakAuthService-->>AuthService: TokenResult.from(response)
AuthService-->>AuthController: TokenResult
AuthController->>AuthController: TokenResponse.from(TokenResult)
AuthController-->>Client: 200 TokenResponse
Client->>AuthController: POST /api/v1/refresh (RefreshTokenRequest)
AuthController->>AuthController: RefreshTokenRequest.toCommand()
AuthController->>AuthService: refreshToken(RefreshTokenCommand)
AuthService->>KeycloakAuthService: refreshToken(RefreshTokenCommand)
KeycloakAuthService->>Keycloak: 리프레시 요청 (refreshToken)
Keycloak-->>KeycloakAuthService: KeycloakTokenResponse
KeycloakAuthService-->>AuthService: TokenResult.from(response)
AuthService-->>AuthController: TokenResult
AuthController-->>Client: 200 TokenResponse
Client->>AuthController: POST /api/v1/logout (RefreshTokenRequest)
AuthController->>AuthService: logout(refreshToken)
AuthController-->>Client: 204 No Content
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
src/main/java/org/iimsa/userservice/presentation/dto/UpdateProfileRequest.java (1)
3-7:slackId형식 검증을 요청 DTO 경계에서 추가해 주세요.Line 6은 현재 포맷 검증이 없어 잘못된 값이 그대로 유입될 수 있습니다. nullable은 유지하되 형식 검증만 거는 편이 안전합니다.
제안 diff
package org.iimsa.userservice.presentation.dto; +import jakarta.validation.constraints.Email; + public record UpdateProfileRequest( // 본인 수정 가능 String name, + `@Email`(message = "슬랙 아이디는 이메일 형식이어야 합니다.") String slackId ) { }Based on learnings, the
slackIdfield in this codebase is intentionally email-format and should be validated as an email-shaped value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/iimsa/userservice/presentation/dto/UpdateProfileRequest.java` around lines 3 - 7, Update the UpdateProfileRequest record to validate slackId as an email-shaped value while keeping it nullable: annotate the slackId component in the UpdateProfileRequest record with the bean-validation email constraint (e.g., `@Email`) and a nullable annotation (e.g., `@Nullable` or javax.annotation.Nullable used across the project), and add the corresponding import(s) so the bean-validator will reject invalid formats at the DTO boundary; ensure controllers that accept UpdateProfileRequest use `@Valid` so this validation is enforced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/iimsa/userservice/application/dto/command/LoginCommand.java`:
- Around line 3-14: LoginCommand currently depends on the presentation DTO
TokenRequest via the static factory method from(TokenRequest), breaking layer
boundaries; remove the TokenRequest import and the from(TokenRequest) method
from LoginCommand so the record remains a pure application-level command
(LoginCommand with fields loginId and password), and implement the TokenRequest
-> LoginCommand mapping inside the controller (or presentation layer) by
constructing new LoginCommand(request.email(), request.password()) where needed;
update any call sites that referenced LoginCommand.from(...) to use the new
controller-side construction.
In
`@src/main/java/org/iimsa/userservice/application/dto/command/RefreshTokenCommand.java`:
- Around line 3-10: RefreshTokenCommand currently depends on presentation DTO
RefreshTokenRequest via the from(RefreshTokenRequest) factory which inverts
layer dependency; remove the import and the from(RefreshTokenRequest) method
from the record and make RefreshTokenCommand presentation-agnostic (keep the
constructor taking String refreshToken), then do the conversion in the
controller by constructing new RefreshTokenCommand(request.refreshToken()) where
RefreshTokenRequest is available; update any callers to use the controller-side
construction instead of RefreshTokenCommand.from(...).
In
`@src/main/java/org/iimsa/userservice/infrastructure/keycloak/KeycloakAuthService.java`:
- Around line 37-41: The logs in KeycloakAuthService are writing the raw email
(loginCommand.loginId()) in log.info and log.error; replace those usages with a
masked or non-identifying value (e.g., call a helper like
maskEmail(loginCommand.loginId()) or hashId(loginCommand.loginId())) and use
that masked value in the log messages around keycloakClient.getToken and
TokenResult.from so no plaintext email is emitted; add the maskEmail/hashId
helper in the class (or utility) and use it in both log.info and log.error
calls.
- Around line 40-43: Update getToken() and refreshToken() to distinguish
Keycloak-auth failures from external/network failures: catch FeignException (or
the client exception used) and if feignException.status() == 401 throw
UnAuthorizedException as before, otherwise map network/5xx/timeouts to a
502-mapped exception (e.g., throw a BadGateway/Proxy exception or the same
InternalServerException used by logout) so callers receive a 502 for external
faults; for non-Feign generic Exceptions also treat them as external faults and
throw the 502-mapped exception. Mirror the logic in
KeycloakClientFallbackFactory.handleException() and ensure you reference
getToken(), refreshToken(), logout(), UnAuthorizedException and
InternalServerException (or the chosen 502 exception) when implementing the
change.
In `@src/main/java/org/iimsa/userservice/presentation/AuthController.java`:
- Line 48: Remove logging of sensitive data in AuthController: do not log
request.email() or raw refresh tokens. Locate the token issuance flow in
AuthController where log.info("인증 토큰 발급 요청: {}", request.email()) and the
refresh-token log (reference the refresh token variable/name used around the
same block) are called; either remove those log statements or replace them with
non-sensitive indicators (e.g., log only an action, user id, or a masked/hashed
email with no full address). Ensure no plaintext refresh token is ever written
to logs and update any log messages to avoid including request.email() or the
refresh token variable.
In `@src/main/java/org/iimsa/userservice/presentation/dto/UpdateRoleRequest.java`:
- Around line 3-10: Add bean-validation to the UpdateRoleRequest record so the
role component is validated as required: annotate the record component role with
`@NotNull` (on UpdateRoleRequest and component name role) and add the
corresponding import for javax.validation.constraints.NotNull (or your project's
validation package) so requests with null role fail fast with 400 at the
boundary.
---
Nitpick comments:
In
`@src/main/java/org/iimsa/userservice/presentation/dto/UpdateProfileRequest.java`:
- Around line 3-7: Update the UpdateProfileRequest record to validate slackId as
an email-shaped value while keeping it nullable: annotate the slackId component
in the UpdateProfileRequest record with the bean-validation email constraint
(e.g., `@Email`) and a nullable annotation (e.g., `@Nullable` or
javax.annotation.Nullable used across the project), and add the corresponding
import(s) so the bean-validator will reject invalid formats at the DTO boundary;
ensure controllers that accept UpdateProfileRequest use `@Valid` so this
validation is enforced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6325c37e-d58c-4b43-a1f8-601d7abf6ca7
📒 Files selected for processing (14)
configssrc/main/java/org/iimsa/userservice/application/dto/TokenResult.javasrc/main/java/org/iimsa/userservice/application/dto/command/LoginCommand.javasrc/main/java/org/iimsa/userservice/application/dto/command/RefreshTokenCommand.javasrc/main/java/org/iimsa/userservice/application/service/AuthService.javasrc/main/java/org/iimsa/userservice/infrastructure/keycloak/KeycloakAuthService.javasrc/main/java/org/iimsa/userservice/presentation/AuthController.javasrc/main/java/org/iimsa/userservice/presentation/UserController.javasrc/main/java/org/iimsa/userservice/presentation/dto/RefreshTokenRequest.javasrc/main/java/org/iimsa/userservice/presentation/dto/TokenRequest.javasrc/main/java/org/iimsa/userservice/presentation/dto/TokenResponse.javasrc/main/java/org/iimsa/userservice/presentation/dto/UpdateProfileRequest.javasrc/main/java/org/iimsa/userservice/presentation/dto/UpdateRoleRequest.javasrc/main/resources/application.yaml
Comment on lines
40
to
43
| } catch (Exception e) { | ||
| log.error("인증 처리 중 오류 발생 (Email: {}): {}", email, e.getMessage()); | ||
| log.error("인증 처리 중 오류 발생 (Email: {}): {}", loginCommand.loginId(), e.getMessage()); | ||
| throw new UnAuthorizedException("이메일 또는 비밀번호가 올바르지 않습니다."); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) KeycloakClient 계약/구현에서 어떤 예외를 던질 수 있는지 확인
fd -i 'KeycloakClient.java'
rg -n -C3 --type=java '\binterface\s+KeycloakClient\b|class\s+.*Keycloak.*Client|getToken\s*\(|logout\s*\('
# 2) 전역 예외 처리에서 각 예외가 어떤 HTTP 상태로 변환되는지 확인
rg -n -C3 --type=java '@ExceptionHandler|ResponseStatusException|UnAuthorizedException|InternalServerException|BadGateway|502'
# 3) 인증 서비스에서 catch-all 패턴 위치 확인
rg -n -C3 --type=java 'class\s+KeycloakAuthService|catch\s*\(Exception\s+e\)|throw\s+new\s+UnAuthorizedException'Repository: apangmsa/user-service
Length of output: 28280
예외 처리를 외부 장애와 인증 실패로 구분하세요.
현재 getToken()과 refreshToken()은 모든 예외를 UnAuthorizedException(401)으로 변환합니다. 하지만 API 문서에는 Keycloak 통신 장애 시 502 응답이 명시되어 있으며, logout() 메서드는 같은 상황에서 InternalServerException을 던집니다.
네트워크 타임아웃, 5xx 오류 등은 401이 아니라 502로 응답되어야 하고, 오직 Keycloak의 401 응답만 클라이언트에게 인증 실패로 전달되어야 합니다. KeycloakClientFallbackFactory의 handleException() 메서드와 같이 FeignException 타입별 처리를 적용하세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/iimsa/userservice/infrastructure/keycloak/KeycloakAuthService.java`
around lines 40 - 43, Update getToken() and refreshToken() to distinguish
Keycloak-auth failures from external/network failures: catch FeignException (or
the client exception used) and if feignException.status() == 401 throw
UnAuthorizedException as before, otherwise map network/5xx/timeouts to a
502-mapped exception (e.g., throw a BadGateway/Proxy exception or the same
InternalServerException used by logout) so callers receive a 502 for external
faults; for non-Feign generic Exceptions also treat them as external faults and
throw the 502-mapped exception. Mirror the logic in
KeycloakClientFallbackFactory.handleException() and ensure you reference
getToken(), refreshToken(), logout(), UnAuthorizedException and
InternalServerException (or the chosen 502 exception) when implementing the
change.
| }) | ||
| @PostMapping("/token") | ||
| public TokenResponse token(@RequestBody @Valid TokenRequest request) { | ||
| log.info("인증 토큰 발급 요청: {}", request.email()); |
There was a problem hiding this comment.
민감정보(이메일/리프레시 토큰) 로그 노출을 제거하세요.
특히 Line 64의 리프레시 토큰 원문 로깅은 즉시 제거가 필요합니다.
🔒 제안 수정안
- log.info("인증 토큰 발급 요청: {}", request.email());
+ log.info("인증 토큰 발급 요청 수신");
...
- log.info("인증 토큰 리프레시 요청: {}", request.refreshToken());
+ log.info("인증 토큰 리프레시 요청 수신");Also applies to: 64-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/iimsa/userservice/presentation/AuthController.java` at line
48, Remove logging of sensitive data in AuthController: do not log
request.email() or raw refresh tokens. Locate the token issuance flow in
AuthController where log.info("인증 토큰 발급 요청: {}", request.email()) and the
refresh-token log (reference the refresh token variable/name used around the
same block) are called; either remove those log statements or replace them with
non-sensitive indicators (e.g., log only an action, user id, or a masked/hashed
email with no full address). Ensure no plaintext refresh token is ever written
to logs and update any log messages to avoid including request.email() or the
refresh token variable.
Comment on lines
+3
to
+10
| import java.util.UUID; | ||
| import org.iimsa.userservice.domain.model.Role; | ||
|
|
||
| public record UpdateRoleRequest( | ||
| // MASTER만 수정 가능 | ||
| Role role, | ||
| UUID hubId, | ||
| UUID companyId |
There was a problem hiding this comment.
role 필드 필수 검증이 누락되었습니다.
Line 8에서 role이 null로 들어와도 경계에서 차단되지 않습니다. 요청 단계에서 400으로 빠르게 실패시키도록 @NotNull을 추가하는 게 필요합니다.
제안 diff
package org.iimsa.userservice.presentation.dto;
+import jakarta.validation.constraints.NotNull;
import java.util.UUID;
import org.iimsa.userservice.domain.model.Role;
public record UpdateRoleRequest(
// MASTER만 수정 가능
+ `@NotNull`(message = "역할(role)은 필수입니다.")
Role role,
UUID hubId,
UUID companyId
) {
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/iimsa/userservice/presentation/dto/UpdateRoleRequest.java`
around lines 3 - 10, Add bean-validation to the UpdateRoleRequest record so the
role component is validated as required: annotate the record component role with
`@NotNull` (on UpdateRoleRequest and component name role) and add the
corresponding import for javax.validation.constraints.NotNull (or your project's
validation package) so requests with null role fail fast with 400 at the
boundary.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/org/iimsa/userservice/infrastructure/keycloak/KeycloakAuthService.java (1)
70-73:⚠️ Potential issue | 🟠 Major
refreshToken에서 일반 FeignException 처리가 누락되었습니다.
getToken()과 달리refreshToken()은UnAuthorizedException만 처리하고 있습니다. Keycloak 통신 오류(네트워크 장애, 5xx)가 발생하면 예외가 처리되지 않고 전파됩니다. TODO 주석이 있는 것으로 보아 인지하고 계신 것 같습니다.🔧 수정 제안
} catch (UnAuthorizedException e) { // TODO: 500 처리됨 log.error("토큰 갱신 실패: {}", e.getMessage(), e); throw new SessionExpiredException(); + } catch (Exception e) { + log.error("토큰 갱신 중 통신 오류: {}", e.getMessage()); + throw new CustomException("현재 인증 서버를 사용할 수 없습니다. 잠시 후 다시 시도해주세요.", HttpStatus.BAD_GATEWAY); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/iimsa/userservice/infrastructure/keycloak/KeycloakAuthService.java` around lines 70 - 73, refreshToken() currently only catches UnAuthorizedException so FeignException (network/5xx) bubbles up; update refreshToken() to also catch FeignException, log the full error (message and exception) and translate it into an appropriate domain exception: if the FeignException indicates 401/unauthorized throw SessionExpiredException (same as UnAuthorizedException path), otherwise throw a KeycloakCommunicationException (or a suitable remote/temporary failure exception) so calling code gets a handled, logged error; reference the refreshToken() method, UnAuthorizedException, FeignException, SessionExpiredException and inspect FeignException.status() to decide mapping.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/iimsa/userservice/infrastructure/keycloak/KeycloakAuthService.java`:
- Line 112: The call md.digest(email.getBytes()) in KeycloakAuthService uses the
platform default charset which can produce inconsistent hashes; replace it with
an explicit charset by calling email.getBytes(StandardCharsets.UTF_8) and add
the necessary import for java.nio.charset.StandardCharsets so the digest is
stable across environments.
---
Duplicate comments:
In
`@src/main/java/org/iimsa/userservice/infrastructure/keycloak/KeycloakAuthService.java`:
- Around line 70-73: refreshToken() currently only catches UnAuthorizedException
so FeignException (network/5xx) bubbles up; update refreshToken() to also catch
FeignException, log the full error (message and exception) and translate it into
an appropriate domain exception: if the FeignException indicates
401/unauthorized throw SessionExpiredException (same as UnAuthorizedException
path), otherwise throw a KeycloakCommunicationException (or a suitable
remote/temporary failure exception) so calling code gets a handled, logged
error; reference the refreshToken() method, UnAuthorizedException,
FeignException, SessionExpiredException and inspect FeignException.status() to
decide mapping.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4ccbb40c-18b0-4771-85bb-bffa1a367265
📒 Files selected for processing (5)
src/main/java/org/iimsa/userservice/application/dto/command/LoginCommand.javasrc/main/java/org/iimsa/userservice/domain/exception/LoginFailedException.javasrc/main/java/org/iimsa/userservice/domain/exception/SessionExpiredException.javasrc/main/java/org/iimsa/userservice/infrastructure/keycloak/KeycloakAuthService.javasrc/main/java/org/iimsa/userservice/infrastructure/keycloak/client/KeycloakClient.java
✅ Files skipped from review due to trivial changes (1)
- src/main/java/org/iimsa/userservice/infrastructure/keycloak/client/KeycloakClient.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/iimsa/userservice/application/dto/command/LoginCommand.java
| private String hashEmail(String email) { | ||
| try { | ||
| MessageDigest md = MessageDigest.getInstance("SHA-256"); | ||
| byte[] digest = md.digest(email.getBytes()); |
There was a problem hiding this comment.
getBytes()에 명시적 charset을 지정하세요.
email.getBytes()는 플랫폼 기본 인코딩을 사용하므로 환경에 따라 해시 값이 달라질 수 있습니다.
🔧 수정 제안
+import java.nio.charset.StandardCharsets;
...
- byte[] digest = md.digest(email.getBytes());
+ byte[] digest = md.digest(email.getBytes(StandardCharsets.UTF_8));📝 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
| byte[] digest = md.digest(email.getBytes()); | |
| import java.nio.charset.StandardCharsets; | |
| // ... other code ... | |
| byte[] digest = md.digest(email.getBytes(StandardCharsets.UTF_8)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/iimsa/userservice/infrastructure/keycloak/KeycloakAuthService.java`
at line 112, The call md.digest(email.getBytes()) in KeycloakAuthService uses
the platform default charset which can produce inconsistent hashes; replace it
with an explicit charset by calling email.getBytes(StandardCharsets.UTF_8) and
add the necessary import for java.nio.charset.StandardCharsets so the digest is
stable across environments.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Issue Number
📝 작업 내역
AuthController 구현
💡 PR 특이사항
💡 명세서 수정
API 명세서 최신화 완료
Summary by CodeRabbit
New Features
Improvement
New