feat: User Service 인증/인가 + CRUD 전체 구현#3
Merged
HellWorlding merged 15 commits intomainfrom Apr 5, 2026
Merged
Conversation
- 회원가입 API (POST /api/users — Keycloak 등록 + DB 저장) - 로그인 API (POST /api/users/login — Keycloak JWT 토큰 발급) - 로그아웃 API (POST /api/users/logout — refresh token 무효화) - Keycloak Admin Client 연동 (KeycloakIdentityProvider, KeycloakProperties) - /me 엔드포인트 (GET /api/users/me) - 사용자 CRUD (단건조회, 목록조회, 수정, 삭제) - User/Courier 엔티티 수정 (@SQLRestriction, @Version, 컬럼 명세 맞춤) - 예외 분리 (UserErrorCode, UserNotFoundException, UserEmailDuplicateException, UserSlackIdDuplicateException) - HubInfo/CompanyInfo 임시 생성자 추가 (Provider 연동 전) Closes #1
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughREADME와 docker-compose-dev.yml이 추가되었고, build.gradle에 GitHub Packages 레포지토리 및 공통 모듈/Keycloak·Spring Cloud·QueryDSL 등 의존성이 도입되었습니다. JPA 엔티티(User, Courier), 값 객체(HubInfo, CompanyInfo, UserType, DeliveryChargeType), 도메인 예외(UserErrorCode 등), 리포지토리 인터페이스 및 JPA 구현체가 추가되었습니다. UserService와 KeycloakIdentityProvider, UserController 및 관련 DTO/응답 모델이 구현되었고 application.yaml에 Keycloak·Eureka·Config 설정이 추가되었으며 .gitignore에 kafka.server.truststore.jks가 추가되었습니다. 애플리케이션 클래스에 Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (10)
src/main/java/com/loopang/userservice/presentation/dto/response/SignupResponseDto.java-21-28 (1)
21-28:⚠️ Potential issue | 🟡 Minor
from(User user)에 null 방어가 없어 예외 원인 파악이 어렵습니다.매핑 시작 전에 명시적 null 검증을 넣어 실패 지점을 고정하는 편이 안전합니다.
🔧 제안 diff
import java.time.LocalDateTime; +import java.util.Objects; import java.util.UUID; @@ public static SignupResponseDto from(User user) { + Objects.requireNonNull(user, "user must not be null"); return SignupResponseDto.builder() .slackId(user.getSlackId())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/loopang/userservice/presentation/dto/response/SignupResponseDto.java` around lines 21 - 28, The static factory method SignupResponseDto.from(User user) lacks null protection and should validate its input before mapping; add an explicit null check for the user parameter (e.g., using Objects.requireNonNull(user, "...") or an if-check that throws IllegalArgumentException/NullPointerException with a clear message) at the start of SignupResponseDto.from so failures are deterministic and the exception message clearly indicates the missing User; keep the rest of the mapping unchanged (slackId, name, role, hubId, createdAt).src/main/java/com/loopang/userservice/presentation/dto/SignupRequestDto.java-19-21 (1)
19-21:⚠️ Potential issue | 🟡 Minor검증 메시지와 제약 조건이 email 필드에 맞지 않음
- 메시지가 "아이디"라고 되어 있지만 필드명은
@Size(max = 10)은 실제 이메일 주소(예:user@example.com)를 담기에 너무 짧습니다.- 이메일 형식 검증을 위해
♻️ 수정 제안
- `@NotBlank`(message = "아이디를 입력해주세요.") - `@Size`(min = 4, max = 10, message = "아이디는 4자 이상 10자 이하로 입력해주세요.") + `@NotBlank`(message = "이메일을 입력해주세요.") + `@Email`(message = "올바른 이메일 형식이 아닙니다.") + `@Size`(max = 50, message = "이메일은 50자 이하로 입력해주세요.") private String email;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/loopang/userservice/presentation/dto/SignupRequestDto.java` around lines 19 - 21, The validation on the email field in SignupRequestDto is incorrect: replace the misleading "아이디" messages and the overly restrictive `@Size`(min=4, max=10) with email-appropriate constraints by updating the annotations on the email field (e.g., remove or relax `@Size`, add javax.validation.constraints.@Email, and change `@NotBlank` message and any `@Size` message to reference "이메일" and allow realistic length), ensuring the field uses `@Email` and a suitable max length (or omit `@Size`) so valid addresses like user@example.com pass validation.README.md-78-78 (1)
78-78:⚠️ Potential issue | 🟡 Minor테이블 컬럼 개수 불일치 (동일 이슈)
위와 동일하게
+ BaseUserEntity행이 테이블 형식에 맞지 않습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 78, The table row containing "+ BaseUserEntity" is misaligned with the table column count; locate the row that includes the literal "+ BaseUserEntity" and either remove the leading "+ " or reformat the row so it matches the table's pipe-separated column structure (same number of cells as the header), ensuring consistent use of '|' separators and cell counts for the entire table.src/main/java/com/loopang/userservice/infrastructure/keycloak/KeycloakIdentityProvider.java-67-67 (1)
67-67:⚠️ Potential issue | 🟡 Minor이메일이 로그에 기록됩니다 — PII 노출 주의.
이메일은 개인식별정보(PII)입니다. 프로덕션 환경에서는 마스킹 처리하거나 로그 레벨을 DEBUG로 변경하는 것을 고려하세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/loopang/userservice/infrastructure/keycloak/KeycloakIdentityProvider.java` at line 67, The log statement in KeycloakIdentityProvider that calls log.info("[Keycloak] 유저 등록 성공: email={}, id={}", email, userId) writes a user's email (PII) to info logs; change this to avoid PII leakage by either masking the email (e.g., show only domain or first/last chars) or lowering the log level to debug and remove the raw email from info-level logs. Locate the log call in the KeycloakIdentityProvider class (the user registration success path) and replace the info log with one that either masks email or logs only non-PII (e.g., "id={}" and maskedEmail) or use log.debug for the detailed message while keeping a non-PII info message.README.md-33-42 (1)
33-42:⚠️ Potential issue | 🟡 Minor문서와 구현 상태가 일치하지 않습니다.
README에서 "유저 (구현 예정)"으로 표시된 엔드포인트들이 실제로는 이미 구현되어 있습니다 (
UserController.java에서 확인). 구현 현황(152-176행)과 API 엔드포인트 섹션(33-42행)의 상태를 동기화해 주세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 33 - 42, The README's "유저 (구현 예정)" table is out of sync with the actual implemented endpoints in UserController (methods: getCurrentUser, getUserById, listUsers, updateUser, deleteUser); update the README to reflect that these endpoints are implemented (change the section title and the Status column from "planned" to "implemented" or similar) and ensure the URL, description, and authentication columns match the annotations/behavior in UserController (confirm method names and auth requirements like MASTER or 로그인 match and adjust the table entries accordingly).src/main/java/com/loopang/userservice/application/service/UserService.java-53-56 (1)
53-56:⚠️ Potential issue | 🟡 Minor
hubName에 빈 문자열 사용 — NOT NULL 컬럼과 불일치.README에 따르면
hub_name은VARCHAR(50) NOT NULL입니다. 빈 문자열("")을 저장하면 데이터 무결성 문제가 발생할 수 있습니다. TODO 주석처럼HubProvider연동 전까지 임시 값을 사용하더라도, 최소한 의미 있는 placeholder나 validation을 추가하는 것이 좋습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/loopang/userservice/application/service/UserService.java` around lines 53 - 56, The HubInfo and CompanyInfo are being constructed with empty hubName/companyName (HubInfo(request.getHubId(), "") and CompanyInfo(request.getCompanyId(), "")) which violates the DB NOT NULL constraint; instead populate a meaningful placeholder or validated fallback (e.g., use request.getHubName() or a constant like "UNKNOWN_HUB"/"UNKNOWN_COMPANY") and/or validate and throw an exception if no name is available before persisting; update the UserService code paths that create HubInfo and CompanyInfo to use the chosen non-empty placeholder or validation logic so NOT NULL constraints are satisfied.src/main/java/com/loopang/userservice/presentation/controller/UserController.java-63-67 (1)
63-67:⚠️ Potential issue | 🟡 Minor
@Valid어노테이션이 누락되었습니다.다른 POST/PATCH 엔드포인트들과 달리
updateUser의@RequestBody에@Valid어노테이션이 없습니다.UserUpdateRequestDto에 validation 어노테이션이 있다면 동작하지 않습니다.🐛 수정 제안
`@PatchMapping`("/{userId}") public CommonResponse<UserResponseDto> updateUser(`@PathVariable` UUID userId, - `@RequestBody` UserUpdateRequestDto request) { + `@Valid` `@RequestBody` UserUpdateRequestDto request) { return CommonResponse.success(userService.updateUser(userId, request), "사용자 정보가 수정되었습니다."); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/loopang/userservice/presentation/controller/UserController.java` around lines 63 - 67, The updateUser endpoint is missing the `@Valid` annotation on the `@RequestBody` parameter so validation on UserUpdateRequestDto won't run; update the method signature in UserController.updateUser to annotate the request parameter with `@Valid` (i.e., change "@RequestBody UserUpdateRequestDto request" to "@Valid `@RequestBody` UserUpdateRequestDto request") and add the appropriate import for javax.validation.Valid or jakarta.validation.Valid consistent with the project.src/main/java/com/loopang/userservice/infrastructure/keycloak/KeycloakIdentityProvider.java-63-74 (1)
63-74:⚠️ Potential issue | 🟡 Minor
locationHeader가null일 경우 NPE 발생 가능.HTTP 201 응답에서도
Location헤더가 없을 수 있습니다. null 체크를 추가하세요.🛡️ 수정 제안
try (Response response = usersResource.create(user)) { if (response.getStatus() == 201) { String locationHeader = response.getHeaderString("Location"); + if (locationHeader == null || !locationHeader.contains("/")) { + throw new InternalServerException("Keycloak 응답에서 유저 ID를 추출할 수 없습니다."); + } String userId = locationHeader.substring(locationHeader.lastIndexOf("/") + 1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/loopang/userservice/infrastructure/keycloak/KeycloakIdentityProvider.java` around lines 63 - 74, In KeycloakIdentityProvider's user creation block, locationHeader may be null causing an NPE when extracting userId from usersResource.create(...) response; add a null/empty check for response.getHeaderString("Location") before calling substring, log a clear error including response.getStatus() and the email when Location is missing, and throw an InternalServerException (or handle appropriately) instead of proceeding to parse a null header so UUID.fromString(...) is never called on invalid input.README.md-67-67 (1)
67-67:⚠️ Potential issue | 🟡 Minor테이블 컬럼 개수가 맞지 않습니다.
+ BaseUserEntity행이 4열 테이블 형식과 일치하지 않아 마크다운 렌더링 시 문제가 발생합니다.📝 수정 제안
-| + BaseUserEntity (createdAt/By, updatedAt/By, deletedAt/By) | +| created_at | TIMESTAMP | | 생성 일시 (BaseUserEntity) | +| created_by | UUID | | 생성자 (BaseUserEntity) | +| updated_at | TIMESTAMP | | 수정 일시 (BaseUserEntity) | +| updated_by | UUID | | 수정자 (BaseUserEntity) | +| deleted_at | TIMESTAMP | | 삭제 일시 (BaseUserEntity) | +| deleted_by | UUID | | 삭제자 (BaseUserEntity) |또는 테이블 아래에 별도 주석으로 표기하는 방식도 고려해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 67, The table row containing "BaseUserEntity (createdAt/By, updatedAt/By, deletedAt/By)" is malformed because it only has one cell while the table expects four columns; update that row so it has exactly four pipe-separated cells (e.g., add empty placeholders or split the description across the appropriate cells) to match the table's column count, or remove the row and instead add a separate note below the table describing BaseUserEntity and its fields; target the markdown row that mentions BaseUserEntity and adjust it accordingly.src/main/java/com/loopang/userservice/infrastructure/keycloak/KeycloakIdentityProvider.java-101-110 (1)
101-110:⚠️ Potential issue | 🟡 Minor응답 본문 또는 필드가
null일 경우 NPE 발생 가능.
response.getBody()가null을 반환하거나 필드가 누락된 경우 NPE가 발생합니다. 또한 raw typeMap사용으로 타입 안전성이 보장되지 않습니다.🛡️ 수정 제안
- ResponseEntity<Map> response = restTemplate.exchange( + ResponseEntity<Map<String, Object>> response = restTemplate.exchange( tokenUrl, HttpMethod.POST, new HttpEntity<>(params, headers), - Map.class + new ParameterizedTypeReference<Map<String, Object>>() {} ); - Map body = response.getBody(); + Map<String, Object> body = response.getBody(); + if (body == null) { + throw new InternalServerException("Keycloak 토큰 응답이 비어있습니다."); + } log.info("[Keycloak] 로그인 성공: {}", email); return TokenResponseDto.builder() - .accessToken((String) body.get("access_token")) + .accessToken(Objects.requireNonNull((String) body.get("access_token"), "access_token 누락"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/loopang/userservice/infrastructure/keycloak/KeycloakIdentityProvider.java` around lines 101 - 110, The code uses raw Map body = response.getBody() and blindly casts fields into TokenResponseDto which can throw NPE or ClassCastException if body is null or fields missing; update the logic in the method that builds TokenResponseDto (reference TokenResponseDto.builder(), response.getBody(), and the Map variable) to: 1) treat response.getBody() as a typed Map<String,Object> and null-check it before accessing; 2) validate each expected key ("access_token","refresh_token","token_type","expires_in","refresh_expires_in") and handle missing values by throwing a clear custom exception or returning an appropriate error, and 3) safely extract numeric fields by checking instanceof Number (or parsing) before calling longValue(); also add logging for null/missing fields rather than letting an NPE propagate.
🧹 Nitpick comments (10)
src/main/java/com/loopang/userservice/domain/service/CompanyProvider.java (1)
8-8: 파라미터 명을storeId대신companyId로 맞추는 것을 권장합니다.
CompanyProvider계약 의미와 호출 측 도메인 의미를 일치시키면 유지보수성이 좋아집니다.🔧 제안 diff
- CompanyInfo get(UUID storeId); + CompanyInfo get(UUID companyId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/loopang/userservice/domain/service/CompanyProvider.java` at line 8, Rename the parameter in the CompanyProvider contract so the method signature reads get(UUID companyId) instead of get(UUID storeId); update the parameter name in the interface declaration (CompanyProvider::get) and any implementing classes/method overrides to use companyId to keep the provider contract and callers consistent.src/main/java/com/loopang/userservice/domain/service/RoleCheck.java (1)
11-11: 복수 역할 체크 시그니처를hasAnyRole(Collection<UserType>)형태로 분리하는 것을 권장합니다.현재 오버로드는 호출 의도 표현이 약하고,
List고정 타입은 계약 유연성을 제한합니다.🔧 제안 diff
-import java.util.List; +import java.util.Collection; import java.util.UUID; @@ - boolean hasRole(List<UserType> types); + boolean hasAnyRole(Collection<UserType> types);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/loopang/userservice/domain/service/RoleCheck.java` at line 11, In the RoleCheck interface, replace the ambiguous overloaded hasRole(List<UserType>) with a clear hasAnyRole(Collection<UserType>) signature: add a new method boolean hasAnyRole(Collection<UserType> types) in RoleCheck (and update any implementing classes to implement this method), stop using the List-typed hasRole overload to increase API flexibility, and either deprecate or remove the old hasRole(List<UserType>) declaration and its implementations to avoid ambiguity.src/main/java/com/loopang/userservice/domain/entity/Courier.java (2)
28-30: Courier 인스턴스 생성 방법이 없습니다.
@AllArgsConstructor(access = AccessLevel.PRIVATE)와@NoArgsConstructor(access = AccessLevel.PROTECTED)조합으로 인해 외부에서 새로운Courier인스턴스를 생성할 방법이 없습니다. JPA 조회 시에는 protected 생성자로 충분하지만, 새로운 배송 담당자를 등록하려면 정적 팩토리 메서드나 빌더가 필요합니다.♻️ 정적 팩토리 메서드 추가 제안
`@AllArgsConstructor`(access = AccessLevel.PRIVATE) `@NoArgsConstructor`(access = AccessLevel.PROTECTED) public class Courier extends BaseUserEntity { + + public static Courier create(User user, DeliveryChargeType type, int deliveryTurn) { + Courier courier = new Courier(); + courier.user = user; + courier.type = type; + courier.deliveryTurn = deliveryTurn; + return courier; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/loopang/userservice/domain/entity/Courier.java` around lines 28 - 30, The Courier class currently has only a private all-args constructor and a protected no-args constructor, so external code cannot create new Courier instances; add a public static factory method (e.g., Courier.create(...) or Courier.of(...)) or a public builder to the Courier class that accepts the necessary fields and returns a new Courier by invoking the private constructor, or alternatively change the AllArgsConstructor access to PUBLIC if appropriate; reference the Courier class and its constructors (the `@AllArgsConstructor`(access = AccessLevel.PRIVATE) and `@NoArgsConstructor`(access = AccessLevel.PROTECTED) annotations) when making the change so new courier registration code can instantiate Courier objects.
51-53:assignDeliveryTurn파라미터 타입 일관성 검토 필요파라미터가
Integer(wrapper)인데 필드는int(primitive)입니다.null이 전달되면NullPointerException이 발생합니다. 의도적으로 nullable을 허용하지 않는다면 파라미터 타입을int로 변경하거나, null 체크를 추가하는 것이 좋습니다.♻️ 파라미터 타입 수정 제안
- public void assignDeliveryTurn(Integer turn) { + public void assignDeliveryTurn(int turn) { this.deliveryTurn = turn; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/loopang/userservice/domain/entity/Courier.java` around lines 51 - 53, The assignDeliveryTurn method accepts an Integer while the Courier.deliveryTurn field is a primitive int, which can cause an NPE if null is passed; fix by changing the method signature in Courier.assignDeliveryTurn from Integer to int so the parameter is non-nullable and assign directly to deliveryTurn, or alternatively add an explicit null check (e.g., Objects.requireNonNull or throw IllegalArgumentException) before assigning to deliveryTurn if you intend to allow callers to pass nullable values.src/main/java/com/loopang/userservice/domain/vo/CompanyInfo.java (1)
14-15: @NoArgsConstructor의 접근 수준을 protected로 제한 권장JPA는 기본 생성자가 필요하지만, public으로 열어두면
companyId와companyName이 null인 불완전한 객체가 생성될 수 있습니다.@AllArgsConstructor가 이미 있으므로, 기본 생성자는 JPA용으로만 사용하도록 protected로 제한하는 것이 좋습니다.♻️ 수정 제안
-@NoArgsConstructor +@NoArgsConstructor(access = AccessLevel.PROTECTED)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/loopang/userservice/domain/vo/CompanyInfo.java` around lines 14 - 15, Change the default no-arg constructor to protected so JPA can use it but callers cannot create incomplete CompanyInfo instances: replace the current `@NoArgsConstructor` on the CompanyInfo class with a protected-access no-args constructor (e.g., `@NoArgsConstructor`(access = AccessLevel.PROTECTED)) while keeping `@AllArgsConstructor`; ensure the lombok AccessLevel symbol is available so companyId and companyName cannot be instantiated as null via a public no-arg constructor.src/main/java/com/loopang/userservice/presentation/controller/UserController.java (2)
52-55:getUser엔드포인트에 권한 체크가 없습니다.README에 따르면 이 엔드포인트는 MASTER 권한이 필요합니다. 현재는 인증 없이 접근 가능합니다. TODO로 남겨진
@PreAuthorize또는RoleCheck연동이 완료되면 적용이 필요합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/loopang/userservice/presentation/controller/UserController.java` around lines 52 - 55, The getUser method in UserController lacks an authorization check and must require MASTER role per README; update the getUser endpoint to enforce role-based access by applying the appropriate security annotation or interceptor (e.g., add `@PreAuthorize`("hasRole('MASTER')") to the getUser method or invoke the existing RoleCheck integration) so only users with MASTER can call userService.getUser(UUID), ensuring import/configuration for the chosen mechanism is present.
47-50:X-User-UUID헤더 누락 시 명확한 에러 메시지가 필요합니다.헤더가 없으면 Spring은 기본적으로
MissingRequestHeaderException을 던지지만, 클라이언트에게 명확한 메시지를 제공하려면required = true(기본값)와 함께 예외 처리기를 추가하거나,@RequestHeader(value = "X-User-UUID", required = true)를 명시적으로 선언하는 것이 좋습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/loopang/userservice/presentation/controller/UserController.java` around lines 47 - 50, The getMe endpoint in UserController uses `@RequestHeader`("X-User-UUID") without explicitly declaring required=true and has no clear handling for MissingRequestHeaderException; update the annotation to `@RequestHeader`(value = "X-User-UUID", required = true) on the getMe(UUID userId) method and add a controller-level (or `@ControllerAdvice`) exception handler for MissingRequestHeaderException that returns a meaningful CommonResponse error (clear message like "X-User-UUID header is required") so clients receive an explicit error when the header is missing.src/main/java/com/loopang/userservice/infrastructure/keycloak/KeycloakIdentityProvider.java (2)
44-44:RestTemplate을 직접 생성하지 말고 Bean으로 주입하세요.
new RestTemplate()을 직접 생성하면 연결 풀링, 타임아웃 설정, 테스트 시 모킹이 어려워집니다.♻️ 수정 제안
별도 Config 클래스 생성:
`@Configuration` public class RestTemplateConfig { `@Bean` public RestTemplate restTemplate() { return new RestTemplateBuilder() .setConnectTimeout(Duration.ofSeconds(5)) .setReadTimeout(Duration.ofSeconds(10)) .build(); } }생성자 주입으로 변경:
- public KeycloakIdentityProvider(KeycloakProperties properties) { + public KeycloakIdentityProvider(KeycloakProperties properties, RestTemplate restTemplate) { this.properties = properties; this.keycloak = KeycloakBuilder.builder() // ... .build(); - this.restTemplate = new RestTemplate(); + this.restTemplate = restTemplate; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/loopang/userservice/infrastructure/keycloak/KeycloakIdentityProvider.java` at line 44, Replace the direct instantiation of RestTemplate in KeycloakIdentityProvider (the this.restTemplate = new RestTemplate() line) with a constructor-injected RestTemplate bean: add a RestTemplate parameter to KeycloakIdentityProvider's constructor and assign it to the restTemplate field. Also add a separate configuration class (e.g., RestTemplateConfig) that defines a `@Bean` RestTemplate using RestTemplateBuilder with sensible connect/read timeouts (setConnectTimeout, setReadTimeout) so the client benefits from pooling, timeouts, and is mockable in tests.
111-114: 모든 예외를 동일하게 처리하면 실제 원인 파악이 어렵습니다.네트워크 오류, Keycloak 서버 오류, 잘못된 자격 증명 등 다양한 원인이 있을 수 있지만, 모두 "이메일 또는 비밀번호가 올바르지 않습니다"로 처리됩니다.
HttpClientErrorException과HttpServerErrorException을 구분하여 처리하는 것이 좋습니다.♻️ 수정 제안
- } catch (Exception e) { - log.error("[Keycloak] 로그인 실패: {}", email, e); - throw new UnAuthorizedException("이메일 또는 비밀번호가 올바르지 않습니다."); + } catch (HttpClientErrorException.Unauthorized | HttpClientErrorException.BadRequest e) { + log.warn("[Keycloak] 로그인 실패 (인증 오류): {}", email); + throw new UnAuthorizedException("이메일 또는 비밀번호가 올바르지 않습니다."); + } catch (RestClientException e) { + log.error("[Keycloak] 로그인 실패 (서버 오류): {}", email, e); + throw new InternalServerException("인증 서버에 연결할 수 없습니다."); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/loopang/userservice/infrastructure/keycloak/KeycloakIdentityProvider.java` around lines 111 - 114, The catch-all Exception block in KeycloakIdentityProvider obscures root causes; update the login/auth flow to catch and handle specific exceptions: catch HttpClientErrorException (from org.springframework.web.client) and when status is 401/400 rethrow UnAuthorizedException with the existing message and log the status and response body, catch HttpServerErrorException to log server-side details and throw an appropriate 5xx-specific runtime exception (or wrap in a custom InternalServerError/ServiceUnavailable), and finally keep a narrow general Exception fallback that logs full details and rethrows a generic runtime exception; locate the try/catch in KeycloakIdentityProvider (the method that currently logs "[Keycloak] 로그인 실패: {}") and replace the single catch(Exception e) with these specific handlers, ensuring logs include error status and response body for HttpClientErrorException/HttpServerErrorException.src/main/java/com/loopang/userservice/application/service/UserService.java (1)
71-77:getMe와getUser가 동일한 로직을 수행합니다.현재 두 메서드 모두
findUserById()를 호출하여 동일한 결과를 반환합니다. 향후getMe에서는 민감한 정보를 포함하고getUser에서는 제외하는 등 차별화가 필요할 수 있습니다. 현재는 문제가 없지만, 의도적인 설계인지 확인이 필요합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/loopang/userservice/application/service/UserService.java` around lines 71 - 77, getMe and getUser both call findUserById and return UserResponseDto.from, which is likely unintentional; decide whether they should differ (e.g., include sensitive fields for the authenticated user). To fix, either consolidate them (remove one and use a single method) or implement explicit differentiation: change getMe to return a DTO that includes private fields (e.g., UserResponseDto.fromPrivate(findUserById(userId)) or UserResponseDto.from(findUserById(userId), true)) and change getUser to return a public view (e.g., UserResponseDto.fromPublic(findUserById(userId)) or UserResponseDto.from(findUserById(userId), false)); update or add corresponding factory methods on UserResponseDto and adjust callers to use the correct method (referencing getMe, getUser, findUserById, and UserResponseDto.from*).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker-compose-dev.yml`:
- Line 3: 현재 docker-compose 이미지가 "image: postgres:latest"로 되어 있어 예기치 않은 버전
업그레이드를 초래할 수 있으니 "image: postgres:latest"를 고정된 메이저.마이너(및 패치/OS) 태그로 변경하세요; 예를 들어
사용 중인 Postgres 호환성에 맞춰 "postgres:X.Y" 또는 더 재현 가능한 "postgres:X.Y-Z-alpine"처럼
명시적으로 버전과 베이스 이미지를 지정하고, 변경한 태그가 CI와 로컬 환경에서 동작하는지(데이터/마이그레이션 호환성) 확인하세요.
In `@src/main/java/com/loopang/userservice/application/service/UserService.java`:
- Around line 100-105: The call user.delete(null) is invalid because User.delete
requires (UUID masterId, RoleCheck roleCheck, IdentityProvider identityProvider)
and null masterId triggers checkMasterId() -> BadRequestException; fix by either
obtaining and passing real security context values (e.g. use SecurityUtil to get
masterId and appropriate RoleCheck and IdentityProvider) when calling
deleteUser(UUID) — update deleteUser to call
user.delete(SecurityUtil.getMasterId(), RoleCheck.NO_CHECK,
IdentityProvider.SYSTEM) or similar — or add a new User.softDelete() (or
deleteWithoutAuth()) method that performs only the soft-delete logic without
masterId validation and call that from deleteUser until full SecurityUtil +
RoleCheck integration is ready.
- Around line 33-61: signup currently registers the user in Keycloak via
identityProvider.register(...) before saving to the DB, which can leave a
Keycloak user dangling if userRepository.save(...) throws; wrap the DB save in a
try-catch that on any exception calls identityProvider.delete(keycloakUserId)
(or the appropriate identityProvider.deleteUser/deleteById method) to undo the
Keycloak registration, log/delete-exception failures appropriately, and then
rethrow the original exception so the transaction can rollback; implement this
change inside the signup method surrounding the call to userRepository.save(...)
and ensure identityProvider.register(...) result (keycloakUserId) is available
to the compensating delete.
In `@src/main/java/com/loopang/userservice/domain/entity/User.java`:
- Around line 79-83: The null-check in User.checkMasterId is faulty because
String.valueOf(masterId) yields "null" and StringUtils.hasText(...) returns
true; change the check to explicitly test the UUID for null (e.g., if (masterId
== null) or Objects.isNull(masterId)) and throw the BadRequestException when
null so a missing masterId is correctly detected.
- Around line 85-93: The delete method can leave DB soft-deleted while Keycloak
still has the user; to fix, call identityProvider.withdraw(id) before performing
super.delete(masterId) inside the same transactional boundary and propagate any
exceptions so the transaction rolls back; specifically, in User.delete(UUID
masterId, RoleCheck roleCheck, IdentityProvider identityProvider) perform
checkMasterId(...) and checkMaster(...), then attempt
identityProvider.withdraw(id) first (handle/propagate errors), and only after
successful withdraw call super.delete(masterId); alternatively implement a
compensation path that re-creates the identity on failure, but prefer the
withdraw-first approach to avoid inconsistency.
In
`@src/main/java/com/loopang/userservice/domain/exception/UserEmailDuplicateException.java`:
- Around line 7-9: The constructor in UserEmailDuplicateException currently
embeds the raw email in the exception message; remove the raw email to avoid
exposing PII and replace the message with a generic text such as "이미 사용중인
이메일입니다." (i.e., update the UserEmailDuplicateException(String email) constructor
to not concatenate the input email into the super message). If you need to
retain any email for internal diagnostics, store it in a private field or log a
masked version via a helper (e.g., maskEmail) but do not return or include the
original email in the exception message or any API-facing output.
In
`@src/main/java/com/loopang/userservice/domain/exception/UserSlackIdDuplicateException.java`:
- Around line 7-9: The constructor of UserSlackIdDuplicateException currently
includes the raw slackId in the exception message
(UserSlackIdDuplicateException(String slackId)); remove the Slack ID literal
from the public message to avoid leaking identifiers by changing the message to
a generic one like "이미 사용중인 SlackID 입니다" and, if you need the actual id for
internal debugging, record it only in a secure/controlled log or include a
masked version (e.g., show only last 4 chars) when creating the exception;
update the constructor to accept slackId but not append it raw to super(...) (or
remove the parameter if unused) so the class no longer exposes the full
identifier.
In `@src/main/java/com/loopang/userservice/domain/service/IdentityProvider.java`:
- Line 3: IdentityProvider (domain service) currently imports
presentation.dto.response.TokenResponseDto, which violates layer boundaries;
remove the presentation DTO dependency by either moving TokenResponseDto into
the application/domain layer or by creating a domain-level response model (e.g.,
IdentityToken or DomainTokenResponse) used by IdentityProvider; update
IdentityProvider method signatures to return the new domain response type and
adapt presentation layer controllers to map that domain response to the original
TokenResponseDto (or use a mapper) so the domain layer no longer depends on
presentation.
In `@src/main/java/com/loopang/userservice/domain/vo/HubInfo.java`:
- Around line 23-33: HubInfo 생성자에서 HubProvider.get(id)의 반환 객체 필드 유효성도 검사하세요:
HubInfo hub = hubProvider.get(id) 이후 hub.getHubId()와 hub.getHubName()가 null 또는 빈
문자열(또는 공백만 있는 경우)인지 검사하고, 컬럼 제약(생성 시점의 Line 16/19 제약과 동일하게)과 맞지 않으면 기존과 동일한 유형의
BadRequestException으로 예외를 던지도록 수정하세요; 즉 HubInfo 생성자 내부에서 hubId/hubName을 읽기 전에 값의
null·빈값 방어를 추가하고 유효하면 this.hubId/this.hubName에 대입합니다.
- Around line 36-39: The HubInfo(UUID hubId, String hubName) constructor lacks
input validation allowing null or overly long values that can break DB
constraints; add precondition checks in the HubInfo constructor (class HubInfo,
constructor HubInfo(UUID hubId, String hubName)) to: 1) reject null hubId (throw
NullPointerException or IllegalArgumentException), 2) reject null/blank hubName
and trim it, and 3) enforce a maximum hubName length (use a constant like
HUB_NAME_MAX_LENGTH matching the DB schema, e.g., 255) and throw
IllegalArgumentException if exceeded; ensure messages explain the violated
constraint.
In
`@src/main/java/com/loopang/userservice/infrastructure/keycloak/KeycloakProperties.java`:
- Around line 10-20: Add startup (fail-fast) validation for Keycloak
configuration by ensuring the KeycloakProperties class performs validation at
application bootstrap: annotate the class with javax/jakarta validation support
(e.g., add `@Validated` to the class) and mark required fields (serverUrl, realm,
username, password, clientId, clientSecret) with `@NotBlank/`@NotNull so Spring
fails binding on missing values; alternatively implement a simple validate() or
`@PostConstruct` method inside KeycloakProperties that checks those same fields
and throws an IllegalStateException with a clear message if any are
blank/missing. Ensure the validation runs during configuration binding so the
app fails fast on startup.
In
`@src/main/java/com/loopang/userservice/presentation/controller/UserController.java`:
- Around line 69-73: The delete endpoint currently calls
userService.deleteUser(userId) but UserService.deleteUser invokes
User.delete(null), causing a signature mismatch or a BadRequestException from
masterId null checks; update UserService.deleteUser to call User.delete with the
correct masterId (e.g., the requesting user's ID or the userId as appropriate)
or refactor User.delete to accept only the intended parameter set, and ensure
null-checks/validation and exceptions align (adjust UserService.deleteUser,
User.delete, and any masterId validation logic so the call site and signature
match and do not pass null).
In `@src/main/java/com/loopang/userservice/presentation/dto/LoginRequestDto.java`:
- Around line 8-16: The DTO currently only has `@NoArgsConstructor`(access =
AccessLevel.PRIVATE) so Jackson cannot instantiate LoginRequestDto during JSON
deserialization; add a public constructor or annotate the class with Lombok's
`@AllArgsConstructor` (or change the no-args constructor to access =
AccessLevel.PUBLIC) so Jackson can create instances; apply the same fix to the
other affected DTOs (e.g., LogoutRequestDto) to ensure runtime deserialization
succeeds.
In
`@src/main/java/com/loopang/userservice/presentation/dto/LogoutRequestDto.java`:
- Around line 8-13: LogoutRequestDto currently uses `@NoArgsConstructor`(access =
AccessLevel.PRIVATE) which prevents Jackson from deserializing the JSON request;
either change the no-args constructor to be public (remove the access override
so Jackson can call the default constructor) or replace the private no-args
constructor with an explicit all-args constructor annotated with `@JsonCreator`
(e.g., add `@AllArgsConstructor` and annotate the constructor with
`@JsonCreator/`@JsonProperty for the refreshToken field) so Jackson can construct
LogoutRequestDto with the refreshToken property.
In
`@src/main/java/com/loopang/userservice/presentation/dto/SignupRequestDto.java`:
- Around line 38-39: The DTO exposes a writable role (SignupRequestDto.role)
allowing clients to set master-level roles; fix by preventing client-controlled
role: either remove the role field from SignupRequestDto or ignore it in
UserService.signup() and always set the new user's role to UserType.USER when
building the User (e.g., in UserService.signup() set role(UserType.USER));
ensure any role elevation happens via a separate admin-controlled flow and not
from signup input.
In
`@src/main/java/com/loopang/userservice/presentation/dto/UserUpdateRequestDto.java`:
- Line 16: The UserUpdateRequestDto exposes a mutable `role` field that is
passed into `User.update()` without authorization checks, enabling privilege
escalation; either remove `role` from UserUpdateRequestDto (and ignore role
updates in `User.update()`), or enforce authorization before applying role
changes by adding a check in `UserController.updateUser()` /
`UserService.updateUser()` that only allows role modifications when the caller
has MASTER privileges (or implement a dedicated endpoint for role changes that
verifies MASTER). Locate `UserUpdateRequestDto.role`, the call sites that pass
the DTO into `User.update()`, and the methods `UserController.updateUser()` and
`UserService.updateUser()` to apply the chosen fix.
- Around line 10-19: UserUpdateRequestDto is missing an all-args constructor
which causes Jackson deserialization to fail; add an `@AllArgsConstructor`
annotation (or explicitly define a public constructor matching the fields) to
the UserUpdateRequestDto class so Jackson can instantiate it during
deserialization, referencing the class name UserUpdateRequestDto and its fields
name, slackId, role, hubId, companyId, approved.
In `@src/main/resources/application.yaml`:
- Around line 24-25: The application.yaml currently hardcodes Keycloak admin
defaults via the keys username and password (username:
${KEYCLOAK_USERNAME:admin}, password: ${KEYCLOAK_PASSWORD:admin}), which exposes
credentials; remove the default fallback values so the app reads only from
environment variables (e.g., change to username: ${KEYCLOAK_USERNAME} and
password: ${KEYCLOAK_PASSWORD}), and add application startup validation (or
fail-fast) that checks KEYCLOAK_USERNAME/KEYCLOAK_PASSWORD are set (or require a
dedicated secret store) to prevent accidental use of weak defaults in
production.
- Line 27: Remove the hardcoded secret value from the client-secret setting in
application.yaml and require the environment variable instead; specifically,
replace the current client-secret default usage of KEYCLOAK_CLIENT_SECRET (the
value showing as "NriuCBLsjGI3erRdMK7DdXXpQIlLrGiu") with a pattern that does
not provide a plaintext fallback (so the app fails startup if
KEYCLOAK_CLIENT_SECRET is not set) or use a non-sensitive placeholder string,
and ensure any real secret found here is rotated immediately; update references
to client-secret and KEYCLOAK_CLIENT_SECRET accordingly and add a startup-time
validation that rejects missing KEYCLOAK_CLIENT_SECRET.
---
Minor comments:
In `@README.md`:
- Line 78: The table row containing "+ BaseUserEntity" is misaligned with the
table column count; locate the row that includes the literal "+ BaseUserEntity"
and either remove the leading "+ " or reformat the row so it matches the table's
pipe-separated column structure (same number of cells as the header), ensuring
consistent use of '|' separators and cell counts for the entire table.
- Around line 33-42: The README's "유저 (구현 예정)" table is out of sync with the
actual implemented endpoints in UserController (methods: getCurrentUser,
getUserById, listUsers, updateUser, deleteUser); update the README to reflect
that these endpoints are implemented (change the section title and the Status
column from "planned" to "implemented" or similar) and ensure the URL,
description, and authentication columns match the annotations/behavior in
UserController (confirm method names and auth requirements like MASTER or 로그인
match and adjust the table entries accordingly).
- Line 67: The table row containing "BaseUserEntity (createdAt/By, updatedAt/By,
deletedAt/By)" is malformed because it only has one cell while the table expects
four columns; update that row so it has exactly four pipe-separated cells (e.g.,
add empty placeholders or split the description across the appropriate cells) to
match the table's column count, or remove the row and instead add a separate
note below the table describing BaseUserEntity and its fields; target the
markdown row that mentions BaseUserEntity and adjust it accordingly.
In `@src/main/java/com/loopang/userservice/application/service/UserService.java`:
- Around line 53-56: The HubInfo and CompanyInfo are being constructed with
empty hubName/companyName (HubInfo(request.getHubId(), "") and
CompanyInfo(request.getCompanyId(), "")) which violates the DB NOT NULL
constraint; instead populate a meaningful placeholder or validated fallback
(e.g., use request.getHubName() or a constant like
"UNKNOWN_HUB"/"UNKNOWN_COMPANY") and/or validate and throw an exception if no
name is available before persisting; update the UserService code paths that
create HubInfo and CompanyInfo to use the chosen non-empty placeholder or
validation logic so NOT NULL constraints are satisfied.
In
`@src/main/java/com/loopang/userservice/infrastructure/keycloak/KeycloakIdentityProvider.java`:
- Line 67: The log statement in KeycloakIdentityProvider that calls
log.info("[Keycloak] 유저 등록 성공: email={}, id={}", email, userId) writes a user's
email (PII) to info logs; change this to avoid PII leakage by either masking the
email (e.g., show only domain or first/last chars) or lowering the log level to
debug and remove the raw email from info-level logs. Locate the log call in the
KeycloakIdentityProvider class (the user registration success path) and replace
the info log with one that either masks email or logs only non-PII (e.g.,
"id={}" and maskedEmail) or use log.debug for the detailed message while keeping
a non-PII info message.
- Around line 63-74: In KeycloakIdentityProvider's user creation block,
locationHeader may be null causing an NPE when extracting userId from
usersResource.create(...) response; add a null/empty check for
response.getHeaderString("Location") before calling substring, log a clear error
including response.getStatus() and the email when Location is missing, and throw
an InternalServerException (or handle appropriately) instead of proceeding to
parse a null header so UUID.fromString(...) is never called on invalid input.
- Around line 101-110: The code uses raw Map body = response.getBody() and
blindly casts fields into TokenResponseDto which can throw NPE or
ClassCastException if body is null or fields missing; update the logic in the
method that builds TokenResponseDto (reference TokenResponseDto.builder(),
response.getBody(), and the Map variable) to: 1) treat response.getBody() as a
typed Map<String,Object> and null-check it before accessing; 2) validate each
expected key
("access_token","refresh_token","token_type","expires_in","refresh_expires_in")
and handle missing values by throwing a clear custom exception or returning an
appropriate error, and 3) safely extract numeric fields by checking instanceof
Number (or parsing) before calling longValue(); also add logging for
null/missing fields rather than letting an NPE propagate.
In
`@src/main/java/com/loopang/userservice/presentation/controller/UserController.java`:
- Around line 63-67: The updateUser endpoint is missing the `@Valid` annotation on
the `@RequestBody` parameter so validation on UserUpdateRequestDto won't run;
update the method signature in UserController.updateUser to annotate the request
parameter with `@Valid` (i.e., change "@RequestBody UserUpdateRequestDto request"
to "@Valid `@RequestBody` UserUpdateRequestDto request") and add the appropriate
import for javax.validation.Valid or jakarta.validation.Valid consistent with
the project.
In
`@src/main/java/com/loopang/userservice/presentation/dto/response/SignupResponseDto.java`:
- Around line 21-28: The static factory method SignupResponseDto.from(User user)
lacks null protection and should validate its input before mapping; add an
explicit null check for the user parameter (e.g., using
Objects.requireNonNull(user, "...") or an if-check that throws
IllegalArgumentException/NullPointerException with a clear message) at the start
of SignupResponseDto.from so failures are deterministic and the exception
message clearly indicates the missing User; keep the rest of the mapping
unchanged (slackId, name, role, hubId, createdAt).
In
`@src/main/java/com/loopang/userservice/presentation/dto/SignupRequestDto.java`:
- Around line 19-21: The validation on the email field in SignupRequestDto is
incorrect: replace the misleading "아이디" messages and the overly restrictive
`@Size`(min=4, max=10) with email-appropriate constraints by updating the
annotations on the email field (e.g., remove or relax `@Size`, add
javax.validation.constraints.@Email, and change `@NotBlank` message and any `@Size`
message to reference "이메일" and allow realistic length), ensuring the field uses
`@Email` and a suitable max length (or omit `@Size`) so valid addresses like
user@example.com pass validation.
---
Nitpick comments:
In `@src/main/java/com/loopang/userservice/application/service/UserService.java`:
- Around line 71-77: getMe and getUser both call findUserById and return
UserResponseDto.from, which is likely unintentional; decide whether they should
differ (e.g., include sensitive fields for the authenticated user). To fix,
either consolidate them (remove one and use a single method) or implement
explicit differentiation: change getMe to return a DTO that includes private
fields (e.g., UserResponseDto.fromPrivate(findUserById(userId)) or
UserResponseDto.from(findUserById(userId), true)) and change getUser to return a
public view (e.g., UserResponseDto.fromPublic(findUserById(userId)) or
UserResponseDto.from(findUserById(userId), false)); update or add corresponding
factory methods on UserResponseDto and adjust callers to use the correct method
(referencing getMe, getUser, findUserById, and UserResponseDto.from*).
In `@src/main/java/com/loopang/userservice/domain/entity/Courier.java`:
- Around line 28-30: The Courier class currently has only a private all-args
constructor and a protected no-args constructor, so external code cannot create
new Courier instances; add a public static factory method (e.g.,
Courier.create(...) or Courier.of(...)) or a public builder to the Courier class
that accepts the necessary fields and returns a new Courier by invoking the
private constructor, or alternatively change the AllArgsConstructor access to
PUBLIC if appropriate; reference the Courier class and its constructors (the
`@AllArgsConstructor`(access = AccessLevel.PRIVATE) and `@NoArgsConstructor`(access
= AccessLevel.PROTECTED) annotations) when making the change so new courier
registration code can instantiate Courier objects.
- Around line 51-53: The assignDeliveryTurn method accepts an Integer while the
Courier.deliveryTurn field is a primitive int, which can cause an NPE if null is
passed; fix by changing the method signature in Courier.assignDeliveryTurn from
Integer to int so the parameter is non-nullable and assign directly to
deliveryTurn, or alternatively add an explicit null check (e.g.,
Objects.requireNonNull or throw IllegalArgumentException) before assigning to
deliveryTurn if you intend to allow callers to pass nullable values.
In `@src/main/java/com/loopang/userservice/domain/service/CompanyProvider.java`:
- Line 8: Rename the parameter in the CompanyProvider contract so the method
signature reads get(UUID companyId) instead of get(UUID storeId); update the
parameter name in the interface declaration (CompanyProvider::get) and any
implementing classes/method overrides to use companyId to keep the provider
contract and callers consistent.
In `@src/main/java/com/loopang/userservice/domain/service/RoleCheck.java`:
- Line 11: In the RoleCheck interface, replace the ambiguous overloaded
hasRole(List<UserType>) with a clear hasAnyRole(Collection<UserType>) signature:
add a new method boolean hasAnyRole(Collection<UserType> types) in RoleCheck
(and update any implementing classes to implement this method), stop using the
List-typed hasRole overload to increase API flexibility, and either deprecate or
remove the old hasRole(List<UserType>) declaration and its implementations to
avoid ambiguity.
In `@src/main/java/com/loopang/userservice/domain/vo/CompanyInfo.java`:
- Around line 14-15: Change the default no-arg constructor to protected so JPA
can use it but callers cannot create incomplete CompanyInfo instances: replace
the current `@NoArgsConstructor` on the CompanyInfo class with a protected-access
no-args constructor (e.g., `@NoArgsConstructor`(access = AccessLevel.PROTECTED))
while keeping `@AllArgsConstructor`; ensure the lombok AccessLevel symbol is
available so companyId and companyName cannot be instantiated as null via a
public no-arg constructor.
In
`@src/main/java/com/loopang/userservice/infrastructure/keycloak/KeycloakIdentityProvider.java`:
- Line 44: Replace the direct instantiation of RestTemplate in
KeycloakIdentityProvider (the this.restTemplate = new RestTemplate() line) with
a constructor-injected RestTemplate bean: add a RestTemplate parameter to
KeycloakIdentityProvider's constructor and assign it to the restTemplate field.
Also add a separate configuration class (e.g., RestTemplateConfig) that defines
a `@Bean` RestTemplate using RestTemplateBuilder with sensible connect/read
timeouts (setConnectTimeout, setReadTimeout) so the client benefits from
pooling, timeouts, and is mockable in tests.
- Around line 111-114: The catch-all Exception block in KeycloakIdentityProvider
obscures root causes; update the login/auth flow to catch and handle specific
exceptions: catch HttpClientErrorException (from org.springframework.web.client)
and when status is 401/400 rethrow UnAuthorizedException with the existing
message and log the status and response body, catch HttpServerErrorException to
log server-side details and throw an appropriate 5xx-specific runtime exception
(or wrap in a custom InternalServerError/ServiceUnavailable), and finally keep a
narrow general Exception fallback that logs full details and rethrows a generic
runtime exception; locate the try/catch in KeycloakIdentityProvider (the method
that currently logs "[Keycloak] 로그인 실패: {}") and replace the single
catch(Exception e) with these specific handlers, ensuring logs include error
status and response body for HttpClientErrorException/HttpServerErrorException.
In
`@src/main/java/com/loopang/userservice/presentation/controller/UserController.java`:
- Around line 52-55: The getUser method in UserController lacks an authorization
check and must require MASTER role per README; update the getUser endpoint to
enforce role-based access by applying the appropriate security annotation or
interceptor (e.g., add `@PreAuthorize`("hasRole('MASTER')") to the getUser method
or invoke the existing RoleCheck integration) so only users with MASTER can call
userService.getUser(UUID), ensuring import/configuration for the chosen
mechanism is present.
- Around line 47-50: The getMe endpoint in UserController uses
`@RequestHeader`("X-User-UUID") without explicitly declaring required=true and has
no clear handling for MissingRequestHeaderException; update the annotation to
`@RequestHeader`(value = "X-User-UUID", required = true) on the getMe(UUID userId)
method and add a controller-level (or `@ControllerAdvice`) exception handler for
MissingRequestHeaderException that returns a meaningful CommonResponse error
(clear message like "X-User-UUID header is required") so clients receive an
explicit error when the header is missing.
🪄 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: 3d149e70-090f-4bc8-80ab-405da8b8cebf
📒 Files selected for processing (37)
.gitignoreREADME.mdbuild.gradledocker-compose-dev.ymlgradlewsettings.gradlesrc/main/java/com/loopang/userservice/UserserviceApplication.javasrc/main/java/com/loopang/userservice/application/service/UserService.javasrc/main/java/com/loopang/userservice/domain/entity/Courier.javasrc/main/java/com/loopang/userservice/domain/entity/User.javasrc/main/java/com/loopang/userservice/domain/exception/UserEmailDuplicateException.javasrc/main/java/com/loopang/userservice/domain/exception/UserErrorCode.javasrc/main/java/com/loopang/userservice/domain/exception/UserNotFoundException.javasrc/main/java/com/loopang/userservice/domain/exception/UserSlackIdDuplicateException.javasrc/main/java/com/loopang/userservice/domain/repository/CourierRepository.javasrc/main/java/com/loopang/userservice/domain/repository/UserRepository.javasrc/main/java/com/loopang/userservice/domain/service/CompanyProvider.javasrc/main/java/com/loopang/userservice/domain/service/HubProvider.javasrc/main/java/com/loopang/userservice/domain/service/IdentityProvider.javasrc/main/java/com/loopang/userservice/domain/service/RoleCheck.javasrc/main/java/com/loopang/userservice/domain/vo/CompanyInfo.javasrc/main/java/com/loopang/userservice/domain/vo/DeliveryChargeType.javasrc/main/java/com/loopang/userservice/domain/vo/HubInfo.javasrc/main/java/com/loopang/userservice/domain/vo/UserType.javasrc/main/java/com/loopang/userservice/infrastructure/keycloak/KeycloakIdentityProvider.javasrc/main/java/com/loopang/userservice/infrastructure/keycloak/KeycloakProperties.javasrc/main/java/com/loopang/userservice/infrastructure/repository/JpaCourierRepository.javasrc/main/java/com/loopang/userservice/infrastructure/repository/JpaUserRepository.javasrc/main/java/com/loopang/userservice/presentation/controller/UserController.javasrc/main/java/com/loopang/userservice/presentation/dto/LoginRequestDto.javasrc/main/java/com/loopang/userservice/presentation/dto/LogoutRequestDto.javasrc/main/java/com/loopang/userservice/presentation/dto/SignupRequestDto.javasrc/main/java/com/loopang/userservice/presentation/dto/UserUpdateRequestDto.javasrc/main/java/com/loopang/userservice/presentation/dto/response/SignupResponseDto.javasrc/main/java/com/loopang/userservice/presentation/dto/response/TokenResponseDto.javasrc/main/java/com/loopang/userservice/presentation/dto/response/UserResponseDto.javasrc/main/resources/application.yaml
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/com/loopang/userservice/domain/entity/User.java`:
- Around line 93-100: The current delete(UUID masterId, RoleCheck roleCheck,
IdentityProvider identityProvider) calls identityProvider.withdraw(id)
(KeycloakIdentityProvider.withdraw) before super.delete(masterId), risking
Keycloak-only deletion if the DB transaction fails; change the flow to perform
the DB soft-delete inside the transactional boundary first (invoke
super.delete(masterId) while still in the transaction), and instead of calling
identityProvider.withdraw synchronously, persist a retryable outbox/event record
(e.g., DeletionRequestedEvent or OutboxEntry) from the same transaction so an
async worker can reliably call identityProvider.withdraw(id) and retry on
failure; ensure the delete method (and symbols delete, super.delete,
identityProvider.withdraw) writes the outbox entry atomically with the DB change
and removes/marks it when the external call succeeds.
- Around line 79-85: The softDelete(UUID deletedBy) path on User bypasses authz
and identity-provider sync; update the service to call the full deletion flow
instead of softDelete: in UserService.deleteUser(...) replace the
user.softDelete(null) call with user.delete(masterId, roleCheck,
identityProvider) (or the equivalent delete(...) overload that performs
SecurityUtil/RoleCheck verification and calls identityProvider.withdraw(...));
ensure the User.delete(...) implementation enforces role checks and invokes
identityProvider.withdraw(...) so external Keycloak withdrawal and MASTER
permission checks run in the actual execution path.
- Around line 45-53: Add unique constraints for the User entity's email and
slackId at both JPA and DB migration layers: update the User class (symbol:
User) to declare email and slackId as unique (e.g., via `@Column`(unique = true)
or adding a `@Table`(uniqueConstraints = ...) referencing the fields email and
slackId) and create/update the database migration that alters the p_user table
to add UNIQUE indexes on email and slack_id (ensure migration contains CREATE
UNIQUE INDEX or ALTER TABLE ADD CONSTRAINT ... UNIQUE for p_user(email) and
p_user(slack_id)). Ensure column names match the entity mappings and include
rollback statements in the migration.
🪄 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: 0e869ce9-4d6f-42ad-b9e5-0d6e61893f83
📒 Files selected for processing (5)
docker-compose-dev.ymlsrc/main/java/com/loopang/userservice/application/service/UserService.javasrc/main/java/com/loopang/userservice/domain/entity/User.javasrc/main/java/com/loopang/userservice/domain/exception/UserEmailDuplicateException.javasrc/main/java/com/loopang/userservice/domain/exception/UserSlackIdDuplicateException.java
✅ Files skipped from review due to trivial changes (1)
- docker-compose-dev.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/com/loopang/userservice/domain/exception/UserSlackIdDuplicateException.java
- src/main/java/com/loopang/userservice/domain/exception/UserEmailDuplicateException.java
- src/main/java/com/loopang/userservice/application/service/UserService.java
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/main/java/com/loopang/userservice/presentation/controller/UserController.java (1)
52-73:⚠️ Potential issue | 🔴 Critical민감 엔드포인트에 권한 검증이 없습니다.
GET /api/users/{userId},GET /api/users,PATCH /api/users/{userId},DELETE /api/users/{userId}는 PR 목표상 MASTER 권한 제약이 필요한데 현재 컨트롤러 레벨 보호가 없습니다. 최소한 메서드 보안(@PreAuthorize) 또는 동등한 정책을 적용해야 합니다.수정 예시
+import org.springframework.security.access.prepost.PreAuthorize; ... `@GetMapping`("/{userId}") + `@PreAuthorize`("hasRole('MASTER')") public CommonResponse<UserResponseDto> getUser(`@PathVariable` UUID userId) { ... `@GetMapping` + `@PreAuthorize`("hasRole('MASTER')") public CommonResponse<List<UserResponseDto>> getUsers(Pageable pageable) { ... `@PatchMapping`("/{userId}") + `@PreAuthorize`("hasRole('MASTER')") public CommonResponse<UserResponseDto> updateUser(`@PathVariable` UUID userId, `@RequestBody` UserUpdateRequestDto request) { ... `@DeleteMapping`("/{userId}") + `@PreAuthorize`("hasRole('MASTER')") public CommonResponse<Void> deleteUser(`@PathVariable` UUID userId) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/loopang/userservice/presentation/controller/UserController.java` around lines 52 - 73, The controller exposes sensitive endpoints without role checks; add method-level or class-level Spring Security pre-authorization to require the MASTER role for getUser, getUsers, updateUser, and deleteUser in UserController (e.g., annotate each of the methods or the controller class with `@PreAuthorize`("hasRole('MASTER')") or the equivalent hasAuthority expression used in your app), and ensure method security is enabled in your security configuration (EnableGlobalMethodSecurity(prePostEnabled = true) or `@EnableMethodSecurity` depending on Spring version); use the exact method names getUser, getUsers, updateUser, deleteUser to locate where to apply the annotations and make sure the role string matches your existing role naming convention.
🤖 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/com/loopang/userservice/application/service/UserService.java`:
- Around line 105-109: The call user.softDelete(null) in deleteUser(UUID) passes
a null actor and breaks audit requirements; instead obtain and pass a concrete
actor id (e.g., a system account or current requester) when deleting. Update
deleteUser to retrieve the actor id (use SecurityUtil or a new constant like
SYSTEM_ACCOUNT_ID until full auth is wired) and call user.softDelete(actorId);
also adjust the TODO comment to indicate replacing the temporary actorId with
RoleCheck/identityProvider integration later. Ensure findUserById and
user.softDelete signatures remain consistent with the non-null actor id.
- Around line 61-64: The catch block swallowing the original DB error when
calling identityProvider.withdraw(keycloakUserId) must be fixed: wrap the
withdraw call in its own try/catch inside the existing catch(Exception e) so any
exception thrown by identityProvider.withdraw is caught and logged (e.g.,
log.error("Compensating rollback failed", rollbackEx, ...) ) but does not
replace or suppress the original exception variable e; after handling/logging
the rollback error rethrow the original exception e. Update the catch block
around the code that currently calls identityProvider.withdraw(...) in
UserService (the catch handling the DB save failure) accordingly.
In
`@src/main/java/com/loopang/userservice/presentation/controller/UserController.java`:
- Around line 47-49: 현재 getMe 메서드는 `@RequestHeader`("X-User-UUID")로 클라이언트가 보낸
UUID를 신원으로 사용해 위조에 취약하므로, 메서드 시그니처에서 RequestHeader를 제거하고 인증된 토큰의
principal(subject)에서 사용자 식별자를 추출하도록 변경하세요: 즉, UserController.getMe(...)에서 클라이언트
헤더 대신 스프링 시큐리티의 인증정보(예: `@AuthenticationPrincipal` 또는
SecurityContextHolder.getContext().getAuthentication().getPrincipal()/getName())로부터
UUID를 얻어 그 값을 userService.getMe(...)에 전달하도록 수정하고 관련 임포트와 예외 처리를 추가하세요.
- Around line 64-66: The updateUser controller method is missing Bean Validation
on the request body so UserUpdateRequestDto constraints won't run; add the
`@Valid` annotation to the UserUpdateRequestDto parameter in the
UserController.updateUser method (i.e., change the parameter from `@RequestBody`
UserUpdateRequestDto request to `@RequestBody` `@Valid` UserUpdateRequestDto
request) and import the appropriate `@Valid` (javax.validation.Valid or
jakarta.validation.Valid) to enable validation and trigger any binding
errors/validation exceptions handled by your existing exception handlers.
---
Duplicate comments:
In
`@src/main/java/com/loopang/userservice/presentation/controller/UserController.java`:
- Around line 52-73: The controller exposes sensitive endpoints without role
checks; add method-level or class-level Spring Security pre-authorization to
require the MASTER role for getUser, getUsers, updateUser, and deleteUser in
UserController (e.g., annotate each of the methods or the controller class with
`@PreAuthorize`("hasRole('MASTER')") or the equivalent hasAuthority expression
used in your app), and ensure method security is enabled in your security
configuration (EnableGlobalMethodSecurity(prePostEnabled = true) or
`@EnableMethodSecurity` depending on Spring version); use the exact method names
getUser, getUsers, updateUser, deleteUser to locate where to apply the
annotations and make sure the role string matches your existing role naming
convention.
🪄 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: f4def49f-5e65-4722-97f1-074d28b65050
📒 Files selected for processing (8)
src/main/java/com/loopang/userservice/application/service/UserService.javasrc/main/java/com/loopang/userservice/domain/service/IdentityProvider.javasrc/main/java/com/loopang/userservice/domain/service/dto/TokenData.javasrc/main/java/com/loopang/userservice/domain/vo/HubInfo.javasrc/main/java/com/loopang/userservice/infrastructure/keycloak/KeycloakIdentityProvider.javasrc/main/java/com/loopang/userservice/infrastructure/keycloak/KeycloakProperties.javasrc/main/java/com/loopang/userservice/presentation/controller/UserController.javasrc/main/java/com/loopang/userservice/presentation/dto/response/TokenResponseDto.java
✅ Files skipped from review due to trivial changes (1)
- src/main/java/com/loopang/userservice/presentation/dto/response/TokenResponseDto.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/com/loopang/userservice/domain/service/IdentityProvider.java
- src/main/java/com/loopang/userservice/domain/vo/HubInfo.java
- src/main/java/com/loopang/userservice/infrastructure/keycloak/KeycloakIdentityProvider.java
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/com/loopang/userservice/domain/entity/User.java`:
- Around line 96-104: The public method delete(UUID masterId, RoleCheck
roleCheck, IdentityProvider identityProvider) currently can NPE when roleCheck
or identityProvider are null; add explicit null checks at the start of delete
(before calling checkMasterId/checkMaster or identityProvider.withdraw) and
throw the appropriate domain exception (not a raw NPE) with clear messages
(e.g., "roleCheck must not be null" / "identityProvider must not be null") so
callers fail with a domain-level error; keep existing calls to checkMasterId,
checkMaster, roleCheck.hasRole and identityProvider.withdraw unchanged
otherwise.
🪄 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: b7894e01-1b7d-428a-a181-2aa36687faaa
📒 Files selected for processing (5)
src/main/java/com/loopang/userservice/application/service/UserService.javasrc/main/java/com/loopang/userservice/domain/entity/User.javasrc/main/java/com/loopang/userservice/domain/vo/HubInfo.javasrc/main/java/com/loopang/userservice/domain/vo/UserType.javasrc/main/java/com/loopang/userservice/presentation/controller/UserController.java
✅ Files skipped from review due to trivial changes (2)
- src/main/java/com/loopang/userservice/domain/vo/UserType.java
- src/main/java/com/loopang/userservice/application/service/UserService.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/com/loopang/userservice/domain/vo/HubInfo.java
- src/main/java/com/loopang/userservice/presentation/controller/UserController.java
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
2 participants
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.
📌 작업 유형
✅ 작업 내용
인증
CRUD
엔티티/예외
코드래빗 리뷰 반영
Closes #1
🧪 테스트 / 확인 방법
👀 리뷰 포인트