feat: 프로필 업데이트 구현#146
Merged
Merged
Conversation
|
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:
📝 WalkthroughWalkthrough이미지 확장자와 저장 prefix를 공통 계약으로 분리하고, 로컬/S3 저장소가 키 검증과 업로드를 수행하도록 바꿨습니다. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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)
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/com/codeit/team5/mopl/content/controller/ContentControllerTest.java (1)
1-263: 📐 Maintainability & Code Quality | 🔵 Trivial주석 처리된 import 갱신은 사실상 효과가 없고, 테스트 파일 전체가 비활성 상태로 남아 있습니다.
13번 라인의 import 경로를
global.config로 맞춰주신 건 일관성 측면에서 좋지만, 파일 전체(1~263)가 주석 처리되어 있어 이 갱신은 컴파일·실행 어디에도 반영되지 않는 no-op입니다. 더 본질적인 문제는, 이번 PR에서ContentController가 multipart/FileResource흐름으로 바뀌었음에도 해당 컨트롤러 테스트 전체가 비활성 상태라는 점입니다. 회귀를 잡아줄 안전망이 없는 상태로 머지되는 셈이라 권장되지 않습니다.선택지를 정리하면:
- (권장) 활성화 후 변경된 시그니처에 맞춰 갱신: 73번
create(any(...), any())등 새 multipart 계약에 맞게 수정해 복구. 장점은 커버리지 회복, 단점은 본 PR 범위가 늘어남.- 후속 PR로 위임하되 추적: PR 설명대로 테스트를 follow-up으로 미루는 경우. 장점은 본 PR이 가벼워짐, 단점은 비활성 코드가 방치될 위험 → 이슈로 명시적으로 추적하는 편이 안전합니다.
- 삭제: 후속에서 새로 작성할 계획이라면, 주석 덩어리로 남겨 혼란을 주기보다 제거하는 것도 한 방법입니다.
원하시면 변경된 multipart 흐름에 맞춰 이 테스트를 복구하는 초안을 만들어 드리거나, 후속 추적용 이슈를 열어 드릴 수 있습니다. 진행할까요?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/codeit/team5/mopl/content/controller/ContentControllerTest.java` around lines 1 - 263, The entire ContentControllerTest is currently commented out, so the updated import change is a no-op and the controller has no active coverage. Either restore the test class and update it to the new multipart/FileResource contract used by ContentController and ContentService#create, or delete the dead commented block if this test is intentionally deferred. Make sure the restored assertions and mocks match the current create(any(...), any()) signature and related multipart request handling so the test compiles and runs again.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/codeit/team5/mopl/binarycontent/BinaryContentStorage.java`:
- Around line 10-22: `BinaryContentStorage.generateKey` and `validateImageKey`
currently enforce different extension rules, so align the contract by either
fail-fast rejecting unsupported or missing extensions at key generation time or
clearly documenting that `generateKey` may emit extensionless keys; update
`LocalBinaryContentStorage.store` and any callers such as
`MultipartFiles.toImageResource`/`ContentService`/`UserService` to match the
chosen behavior, and use the existing `ImageExtension.from(...)`,
`InvalidImageExtensionException`, and `generateKey` symbols to keep the rule
consistent.
In
`@src/main/java/com/codeit/team5/mopl/binarycontent/support/MultipartFiles.java`:
- Around line 24-29: `MultipartFiles.toFileResource`에서 `FileResource`의
contentType을 클라이언트가 보낸 `file.getContentType()`으로 채우는 부분을 수정하세요. 이미
`ImageExtension.from(extension)`으로 확장자를 검증하고 있으니,
`ImageExtension.contentType()`처럼 검증된 값으로 contentType을 일원화해 `FileResource`에 넣고,
`S3BinaryContentStorage.store`와의 불일치가 없도록 맞춰주세요.
`InvalidImageExtensionException`, `FileResource`, `ImageExtension.from(...)`를
기준으로 위치를 찾아 반영하면 됩니다.
- Around line 28-33: `MultipartFiles.toFileResource`의 `IOException`을 삼키고 `null`을
반환하는 동작을 수정해 fail-fast가 실제로 유지되도록 하세요. 바이트 읽기 실패 시 `FileResource` 생성 실패를 호출부에
명확히 전파하도록, `IOException`을 업로드 실패 계열의 도메인 예외로 감싸 던지거나 기존 예외 처리 흐름에 맞는 예외를 던지도록
바꾸세요. 이렇게 하면 `UserService.update`와 `ContentService.create` 같은 호출부에서 실패가 “미첨부”로
오해되지 않고 즉시 처리됩니다.
In `@src/main/java/com/codeit/team5/mopl/global/config/StorageConfig.java`:
- Around line 24-33: The S3Client bean setup in StorageConfig needs explicit
validation for partially missing credentials and region before building the
client. In the S3Client creation path, verify that accessKey and secretKey are
either both present or both absent, and validate that region is set before
calling AwsBasicCredentials.create(...) or Region.of(...). Use clear
precondition checks in the StorageConfig bean method so failures point directly
to the missing setting instead of surfacing as generic runtime exceptions.
In `@src/main/java/com/codeit/team5/mopl/global/dto/FileResource.java`:
- Around line 7-12: `FileResource` record의 `bytes`는 배열이라 자동 생성되는
`equals`/`hashCode`가 내용 비교가 아니라 참조 비교로 동작합니다. `FileResource`의 `bytes`,
`filename`, `contentType`를 기준으로 향후 비교나 로깅에 쓰일 가능성을 고려해, 필요하다면
`equals`/`hashCode`/`toString`을 명시적으로 재정의하거나 배열 내용을 기준으로 비교/출력하도록 `FileResource`
자체에 방어 로직을 추가해 주세요.
In `@src/main/java/com/codeit/team5/mopl/user/controller/UserController.java`:
- Around line 54-65: The UserController.updateUser endpoint currently trusts the
path userId without any ownership check, so add a first-line authorization guard
before calling userService.update. Use an authentication-based check such as
`@AuthenticationPrincipal` or a `@PreAuthorize` rule on updateUser, and ensure the
authenticated user matches the requested userId before allowing the PATCH
/api/users/{userId} update to proceed. If the check fails, return an appropriate
forbidden/unauthorized response rather than passing the request through to
MultipartFiles.toImageResource and userService.update.
In `@src/main/java/com/codeit/team5/mopl/user/service/UserService.java`:
- Around line 74-86: Update the profile image flow in UserService so the user
does not get a URL until the upload is actually usable: right now
`BinaryContent.pending(...)` is saved, `user.updateProfile(...)` immediately
points to that URL, and `UserMapper.toDto` exposes it even if
`BinaryContentUploadListener` later marks the upload as failed. Fix this by
making the response/profile mapping in `UserMapper.toDto` or the profile update
path check the upload state before exposing `profileImageUrl`, and fall back to
the existing or default image when the related `BinaryContent` is not
`COMPLETED`; if you prefer stronger consistency, add compensation in the upload
failure path to revert or clean up the pending profile image reference.
- Around line 74-81: Image replacement in UserService leaves the previous
profile BinaryContent and storage object behind, creating orphaned records;
update the profile image flow so that when a new image is set via the existing
profileImage/binaryContentRepository/eventPublisher path, the old
user.getProfileImage() is scheduled for cleanup after the upload succeeds (for
example on BinaryContentUploadEvent completion). Use the existing UserService
logic around profileImage replacement and BinaryContentUploadEvent to locate the
change, and add async deletion for the prior BinaryContent plus its stored
object once the new upload is confirmed.
- Around line 70-72: The UserService.update flow currently only loads the target
user via getUser(userId) and does not verify that the caller is the same user,
so add an authorization guard before any profile changes. Compare the
authenticated user ID from the security context (for example via
SecurityContextHolder or `@AuthenticationPrincipal`) with the requested userId,
and throw a 403-style exception when they differ. Keep the check close to update
so the contract documented by UserApi is enforced at runtime.
In `@src/test/java/com/codeit/team5/mopl/content/service/ContentServiceTest.java`:
- Line 87: The `generateKey` stubbing in `ContentServiceTest` is too loose
because it uses `any()` for the first two arguments, so it won’t catch incorrect
prefix or ownerId regressions in the thumbnail key path. Tighten the stub around
the `binaryContentStorage.generateKey(...)` call by fixing the first argument to
`StoragePrefix.THUMBNAIL`, and ideally also verify the ownerId passed from the
`ContentService` logic matches `content.getId()`. If you want stronger coverage,
capture the arguments for `generateKey` and assert both the prefix and owner
value explicitly.
---
Outside diff comments:
In
`@src/test/java/com/codeit/team5/mopl/content/controller/ContentControllerTest.java`:
- Around line 1-263: The entire ContentControllerTest is currently commented
out, so the updated import change is a no-op and the controller has no active
coverage. Either restore the test class and update it to the new
multipart/FileResource contract used by ContentController and
ContentService#create, or delete the dead commented block if this test is
intentionally deferred. Make sure the restored assertions and mocks match the
current create(any(...), any()) signature and related multipart request handling
so the test compiles and runs again.
🪄 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: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b8d47a0b-d2e6-4080-ba14-94e61e65c2f2
📒 Files selected for processing (28)
build.gradlesrc/main/java/com/codeit/team5/mopl/binarycontent/BinaryContentStorage.javasrc/main/java/com/codeit/team5/mopl/binarycontent/ImageExtension.javasrc/main/java/com/codeit/team5/mopl/binarycontent/StoragePrefix.javasrc/main/java/com/codeit/team5/mopl/binarycontent/exception/InvalidImageExtensionException.javasrc/main/java/com/codeit/team5/mopl/binarycontent/storage/local/LocalBinaryContentStorage.javasrc/main/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3BinaryContentStorage.javasrc/main/java/com/codeit/team5/mopl/binarycontent/support/MultipartFiles.javasrc/main/java/com/codeit/team5/mopl/config/StorageConfig.javasrc/main/java/com/codeit/team5/mopl/content/controller/ContentController.javasrc/main/java/com/codeit/team5/mopl/content/service/ContentService.javasrc/main/java/com/codeit/team5/mopl/global/config/AsyncConfig.javasrc/main/java/com/codeit/team5/mopl/global/config/JpaAuditingConfig.javasrc/main/java/com/codeit/team5/mopl/global/config/SecurityConfig.javasrc/main/java/com/codeit/team5/mopl/global/config/StorageConfig.javasrc/main/java/com/codeit/team5/mopl/global/config/SwaggerConfig.javasrc/main/java/com/codeit/team5/mopl/global/dto/FileResource.javasrc/main/java/com/codeit/team5/mopl/user/controller/UserController.javasrc/main/java/com/codeit/team5/mopl/user/controller/api/UserApi.javasrc/main/java/com/codeit/team5/mopl/user/dto/request/UserUpdateRequest.javasrc/main/java/com/codeit/team5/mopl/user/entity/User.javasrc/main/java/com/codeit/team5/mopl/user/service/UserService.javasrc/main/resources/application-prod.ymlsrc/test/java/com/codeit/team5/mopl/binarycontent/storage/local/LocalBinaryContentStorageTest.javasrc/test/java/com/codeit/team5/mopl/content/controller/ContentControllerTest.javasrc/test/java/com/codeit/team5/mopl/content/service/ContentServiceTest.javasrc/test/java/com/codeit/team5/mopl/user/controller/UserControllerTest.javasrc/test/java/com/codeit/team5/mopl/user/repository/UserRepositoryTest.java
💤 Files with no reviewable changes (1)
- src/main/java/com/codeit/team5/mopl/config/StorageConfig.java
4 tasks
dstle
force-pushed
the
feature/15-update-profile
branch
from
fb2da67 to
a81ef60
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/test/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3BinaryContentStorageTest.java`:
- Around line 35-43: `store_success`는 putObject 호출 여부만 검증해서 bucket/key 회귀를 놓칠 수
있습니다. `S3BinaryContentStorageTest.store_success`에서
`verify(s3Client).putObject(any(...), any(...))`를 `ArgumentCaptor` 기반 검증으로 바꿔,
`S3BinaryContentStorage.store`가 전달한 `PutObjectRequest`의 bucket이 `mopl-bucket`이고
key가 `profiles/abc.jpg`인지 직접 단언하세요. 필요하면 `RequestBody`는 존재 여부만 확인하고, 실제 인자 검증은
`PutObjectRequest` 중심으로 하세요.
In `@src/test/java/com/codeit/team5/mopl/user/service/UserServiceTest.java`:
- Around line 203-217: The `UserServiceTest` stubs
`binaryContentStorage.generateKey(...)` too loosely with `any()` for the first
two arguments, so it cannot catch regressions in the profile key contract.
Update the test around `userService.update(...)` to verify the `generateKey`
call uses the correct `StoragePrefix.PROFILE` and the expected owner id
(`user.getId()`), either by replacing the first argument matcher with
`eq(StoragePrefix.PROFILE)` or by capturing the arguments and asserting both
values explicitly. Keep the existing assertions and
`binaryContentRepository.save` / `eventPublisher.publishEvent` verifications
unchanged.
🪄 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: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f1ff3bd6-897d-42d4-9e27-cd1c722fccfe
📒 Files selected for processing (10)
src/main/java/com/codeit/team5/mopl/binarycontent/support/MultipartFiles.javasrc/main/java/com/codeit/team5/mopl/global/config/StorageConfig.javasrc/main/java/com/codeit/team5/mopl/global/dto/FileResource.javasrc/main/java/com/codeit/team5/mopl/user/service/UserService.javasrc/test/java/com/codeit/team5/mopl/binarycontent/storage/local/LocalBinaryContentStorageTest.javasrc/test/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3BinaryContentStorageTest.javasrc/test/java/com/codeit/team5/mopl/content/service/ContentServiceTest.javasrc/test/java/com/codeit/team5/mopl/user/controller/UserControllerIntegrationTest.javasrc/test/java/com/codeit/team5/mopl/user/controller/UserControllerTest.javasrc/test/java/com/codeit/team5/mopl/user/service/UserServiceTest.java
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/com/codeit/team5/mopl/binarycontent/support/MultipartFiles.java`:
- Around line 24-29: The current MultipartFiles validation only checks the
filename extension in MultipartFiles and can be bypassed by disguised uploads.
Update the file handling logic in MultipartFiles (especially the code around
StringUtils.getFilenameExtension, ImageExtension.from, and the FileResource
creation) to verify the actual binary signature or decode the content before
accepting it, and reject files whose real content does not match an image format
even if the extension is allowed.
In `@src/main/java/com/codeit/team5/mopl/global/config/SecurityConfig.java`:
- Around line 41-50: The security rule in SecurityConfig is too permissive
because any PATCH profile update falls through to anyRequest().permitAll().
Update the authorizeHttpRequests chain so the user profile update path handled
by PATCH /api/users/{userId} is protected (prefer a default authenticated policy
via anyRequest().authenticated(), while keeping only sign-in/sign-out,
registration, and Swagger explicitly public). Also ensure the profile update
flow enforces ownership checks in the user update service/controller so only the
authenticated user can modify their own profile.
- Line 1: Update the stale SecurityConfig imports in the affected tests:
AuthControllerTest and UserControllerTest still reference
com.codeit.team5.mopl.config.SecurityConfig, but the class now lives under
com.codeit.team5.mopl.global.config.SecurityConfig. Replace the old import with
the new package wherever SecurityConfig is referenced so the test sources
compile against the relocated config class.
In `@src/main/java/com/codeit/team5/mopl/global/config/StorageConfig.java`:
- Around line 27-35: StorageConfig의 자격증명 검증이 비대칭이라 secretKey만 설정된 부분 설정 케이스가
DefaultCredentialsProvider로 숨겨집니다. hasAccessKey 판단과 함께 secretKey만 존재하는 경우도 명시적으로
검사해 accessKey/secretKey가 둘 다 있거나 둘 다 비어 있어야 한다는 검증을 추가하고, 기존
IllegalStateException 패턴에 맞춰 credentialsProvider 선택 전에 차단되도록 수정하세요.
In `@src/main/java/com/codeit/team5/mopl/global/dto/FileResource.java`:
- Around line 11-14: The FileResource record currently exposes the byte[]
directly, so callers can mutate the payload after construction. Update
FileResource to use defensive copying in the canonical constructor and accessor
for bytes, while keeping filename unchanged, so the stored upload data cannot be
altered through shared array references.
In `@src/main/java/com/codeit/team5/mopl/user/controller/api/UserApi.java`:
- Around line 63-84: The PATCH /api/users/{userId} endpoint is missing ownership
authorization, so anyone can submit another userId and update that profile. Add
a guard in the update flow using the UserApi.updateUser and UserService.update
paths, either with a declarative check like `@PreAuthorize` against
authentication.principal.id or an explicit service-level comparison between the
authenticated user and the path userId. If they do not match, reject with 403
before any profile changes are applied.
In
`@src/test/java/com/codeit/team5/mopl/user/controller/UserControllerIntegrationTest.java`:
- Around line 244-245: The integration test is still asserting the obsolete
$.exceptionName field for the INVALID_INPUT response, while the matching case in
UserControllerIntegrationTest already uses $.exceptionType. Update this
assertion to use the same response key as the other UserController tests and the
controller’s error payload shape, so the shared invalid-name scenario is
verified against the correct field.
In `@src/test/java/com/codeit/team5/mopl/user/controller/UserControllerTest.java`:
- Around line 348-349: The new invalid-input test is asserting the old response
field name, so it fails against the current API contract. Update the
`UserControllerTest` case that checks the `INVALID_INPUT` response to use
`$.exceptionType` instead of `$.exceptionName`, matching the other assertions in
this test class and the actual serialized error payload.
🪄 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: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ef88baf8-fdcc-484d-a213-48471e5bf86b
📒 Files selected for processing (32)
build.gradlesrc/main/java/com/codeit/team5/mopl/binarycontent/BinaryContentStorage.javasrc/main/java/com/codeit/team5/mopl/binarycontent/ImageExtension.javasrc/main/java/com/codeit/team5/mopl/binarycontent/StoragePrefix.javasrc/main/java/com/codeit/team5/mopl/binarycontent/exception/InvalidImageExtensionException.javasrc/main/java/com/codeit/team5/mopl/binarycontent/storage/local/LocalBinaryContentStorage.javasrc/main/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3BinaryContentStorage.javasrc/main/java/com/codeit/team5/mopl/binarycontent/support/MultipartFiles.javasrc/main/java/com/codeit/team5/mopl/config/StorageConfig.javasrc/main/java/com/codeit/team5/mopl/content/controller/ContentController.javasrc/main/java/com/codeit/team5/mopl/content/service/ContentService.javasrc/main/java/com/codeit/team5/mopl/global/config/AsyncConfig.javasrc/main/java/com/codeit/team5/mopl/global/config/JpaAuditingConfig.javasrc/main/java/com/codeit/team5/mopl/global/config/JwtPropertiesConfig.javasrc/main/java/com/codeit/team5/mopl/global/config/PasswordEncoderConfig.javasrc/main/java/com/codeit/team5/mopl/global/config/SecurityConfig.javasrc/main/java/com/codeit/team5/mopl/global/config/StorageConfig.javasrc/main/java/com/codeit/team5/mopl/global/config/SwaggerConfig.javasrc/main/java/com/codeit/team5/mopl/global/dto/FileResource.javasrc/main/java/com/codeit/team5/mopl/user/controller/UserController.javasrc/main/java/com/codeit/team5/mopl/user/controller/api/UserApi.javasrc/main/java/com/codeit/team5/mopl/user/dto/request/UserUpdateRequest.javasrc/main/java/com/codeit/team5/mopl/user/entity/User.javasrc/main/java/com/codeit/team5/mopl/user/service/UserService.javasrc/main/resources/application-prod.ymlsrc/test/java/com/codeit/team5/mopl/binarycontent/storage/local/LocalBinaryContentStorageTest.javasrc/test/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3BinaryContentStorageTest.javasrc/test/java/com/codeit/team5/mopl/content/service/ContentServiceTest.javasrc/test/java/com/codeit/team5/mopl/user/controller/UserControllerIntegrationTest.javasrc/test/java/com/codeit/team5/mopl/user/controller/UserControllerTest.javasrc/test/java/com/codeit/team5/mopl/user/repository/UserRepositoryTest.javasrc/test/java/com/codeit/team5/mopl/user/service/UserServiceTest.java
💤 Files with no reviewable changes (1)
- src/main/java/com/codeit/team5/mopl/config/StorageConfig.java
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/com/codeit/team5/mopl/binarycontent/support/MultipartFiles.java`:
- Around line 24-29: The current MultipartFiles validation only checks the
filename extension in MultipartFiles and can be bypassed by disguised uploads.
Update the file handling logic in MultipartFiles (especially the code around
StringUtils.getFilenameExtension, ImageExtension.from, and the FileResource
creation) to verify the actual binary signature or decode the content before
accepting it, and reject files whose real content does not match an image format
even if the extension is allowed.
In `@src/main/java/com/codeit/team5/mopl/global/config/SecurityConfig.java`:
- Around line 41-50: The security rule in SecurityConfig is too permissive
because any PATCH profile update falls through to anyRequest().permitAll().
Update the authorizeHttpRequests chain so the user profile update path handled
by PATCH /api/users/{userId} is protected (prefer a default authenticated policy
via anyRequest().authenticated(), while keeping only sign-in/sign-out,
registration, and Swagger explicitly public). Also ensure the profile update
flow enforces ownership checks in the user update service/controller so only the
authenticated user can modify their own profile.
- Line 1: Update the stale SecurityConfig imports in the affected tests:
AuthControllerTest and UserControllerTest still reference
com.codeit.team5.mopl.config.SecurityConfig, but the class now lives under
com.codeit.team5.mopl.global.config.SecurityConfig. Replace the old import with
the new package wherever SecurityConfig is referenced so the test sources
compile against the relocated config class.
In `@src/main/java/com/codeit/team5/mopl/global/config/StorageConfig.java`:
- Around line 27-35: StorageConfig의 자격증명 검증이 비대칭이라 secretKey만 설정된 부분 설정 케이스가
DefaultCredentialsProvider로 숨겨집니다. hasAccessKey 판단과 함께 secretKey만 존재하는 경우도 명시적으로
검사해 accessKey/secretKey가 둘 다 있거나 둘 다 비어 있어야 한다는 검증을 추가하고, 기존
IllegalStateException 패턴에 맞춰 credentialsProvider 선택 전에 차단되도록 수정하세요.
In `@src/main/java/com/codeit/team5/mopl/global/dto/FileResource.java`:
- Around line 11-14: The FileResource record currently exposes the byte[]
directly, so callers can mutate the payload after construction. Update
FileResource to use defensive copying in the canonical constructor and accessor
for bytes, while keeping filename unchanged, so the stored upload data cannot be
altered through shared array references.
In `@src/main/java/com/codeit/team5/mopl/user/controller/api/UserApi.java`:
- Around line 63-84: The PATCH /api/users/{userId} endpoint is missing ownership
authorization, so anyone can submit another userId and update that profile. Add
a guard in the update flow using the UserApi.updateUser and UserService.update
paths, either with a declarative check like `@PreAuthorize` against
authentication.principal.id or an explicit service-level comparison between the
authenticated user and the path userId. If they do not match, reject with 403
before any profile changes are applied.
In
`@src/test/java/com/codeit/team5/mopl/user/controller/UserControllerIntegrationTest.java`:
- Around line 244-245: The integration test is still asserting the obsolete
$.exceptionName field for the INVALID_INPUT response, while the matching case in
UserControllerIntegrationTest already uses $.exceptionType. Update this
assertion to use the same response key as the other UserController tests and the
controller’s error payload shape, so the shared invalid-name scenario is
verified against the correct field.
In `@src/test/java/com/codeit/team5/mopl/user/controller/UserControllerTest.java`:
- Around line 348-349: The new invalid-input test is asserting the old response
field name, so it fails against the current API contract. Update the
`UserControllerTest` case that checks the `INVALID_INPUT` response to use
`$.exceptionType` instead of `$.exceptionName`, matching the other assertions in
this test class and the actual serialized error payload.
🪄 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: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ef88baf8-fdcc-484d-a213-48471e5bf86b
📒 Files selected for processing (32)
build.gradlesrc/main/java/com/codeit/team5/mopl/binarycontent/BinaryContentStorage.javasrc/main/java/com/codeit/team5/mopl/binarycontent/ImageExtension.javasrc/main/java/com/codeit/team5/mopl/binarycontent/StoragePrefix.javasrc/main/java/com/codeit/team5/mopl/binarycontent/exception/InvalidImageExtensionException.javasrc/main/java/com/codeit/team5/mopl/binarycontent/storage/local/LocalBinaryContentStorage.javasrc/main/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3BinaryContentStorage.javasrc/main/java/com/codeit/team5/mopl/binarycontent/support/MultipartFiles.javasrc/main/java/com/codeit/team5/mopl/config/StorageConfig.javasrc/main/java/com/codeit/team5/mopl/content/controller/ContentController.javasrc/main/java/com/codeit/team5/mopl/content/service/ContentService.javasrc/main/java/com/codeit/team5/mopl/global/config/AsyncConfig.javasrc/main/java/com/codeit/team5/mopl/global/config/JpaAuditingConfig.javasrc/main/java/com/codeit/team5/mopl/global/config/JwtPropertiesConfig.javasrc/main/java/com/codeit/team5/mopl/global/config/PasswordEncoderConfig.javasrc/main/java/com/codeit/team5/mopl/global/config/SecurityConfig.javasrc/main/java/com/codeit/team5/mopl/global/config/StorageConfig.javasrc/main/java/com/codeit/team5/mopl/global/config/SwaggerConfig.javasrc/main/java/com/codeit/team5/mopl/global/dto/FileResource.javasrc/main/java/com/codeit/team5/mopl/user/controller/UserController.javasrc/main/java/com/codeit/team5/mopl/user/controller/api/UserApi.javasrc/main/java/com/codeit/team5/mopl/user/dto/request/UserUpdateRequest.javasrc/main/java/com/codeit/team5/mopl/user/entity/User.javasrc/main/java/com/codeit/team5/mopl/user/service/UserService.javasrc/main/resources/application-prod.ymlsrc/test/java/com/codeit/team5/mopl/binarycontent/storage/local/LocalBinaryContentStorageTest.javasrc/test/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3BinaryContentStorageTest.javasrc/test/java/com/codeit/team5/mopl/content/service/ContentServiceTest.javasrc/test/java/com/codeit/team5/mopl/user/controller/UserControllerIntegrationTest.javasrc/test/java/com/codeit/team5/mopl/user/controller/UserControllerTest.javasrc/test/java/com/codeit/team5/mopl/user/repository/UserRepositoryTest.javasrc/test/java/com/codeit/team5/mopl/user/service/UserServiceTest.java
💤 Files with no reviewable changes (1)
- src/main/java/com/codeit/team5/mopl/config/StorageConfig.java
🛑 Comments failed to post (8)
src/main/java/com/codeit/team5/mopl/binarycontent/support/MultipartFiles.java (1)
24-29: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
확장자만 검증하면 위장 파일 업로드를 차단하지 못합니다.
현재는
.jpg같은 이름만 맞으면 통과합니다. 실제 바이너리 시그니처 검증이 없어 비이미지/악성 파일이 저장될 수 있습니다.
선택지:
- 권장: 매직넘버(파일 시그니처) 검증 추가(보안↑, 구현 난이도 낮음)
- 대안: 디코딩 기반 검증(ImageIO 등, 포맷별 제약/호환성 이슈 존재)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/codeit/team5/mopl/binarycontent/support/MultipartFiles.java` around lines 24 - 29, The current MultipartFiles validation only checks the filename extension in MultipartFiles and can be bypassed by disguised uploads. Update the file handling logic in MultipartFiles (especially the code around StringUtils.getFilenameExtension, ImageExtension.from, and the FileResource creation) to verify the actual binary signature or decode the content before accepting it, and reject files whose real content does not match an image format even if the extension is allowed.src/main/java/com/codeit/team5/mopl/global/config/SecurityConfig.java (2)
1-1: 🩺 Stability & Availability | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # 옛 config 패키지를 import하는 모든 잔존 참조 탐색 rg -nP '\bimport\s+com\.codeit\.team5\.mopl\.config\.' --type=javaRepository: sb10-part4-team5/sb10-mopl-team5
Length of output: 170
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '== SecurityConfig locations ==\n' git ls-files | rg 'SecurityConfig\.java$|AuthControllerTest\.java$|UserControllerTest\.java$|SecurityConfig' || true printf '\n== Package/import references ==\n' rg -n --hidden --glob '!**/target/**' --glob '!**/build/**' \ 'com\.codeit\.team5\.mopl\.(config|global\.config)\.SecurityConfig|package com\.codeit\.team5\.mopl\.global\.config|package com\.codeit\.team5\.mopl\.config' . printf '\n== Test file snippets ==\n' for f in \ src/test/java/com/codeit/team5/mopl/auth/controller/AuthControllerTest.java \ src/test/java/com/codeit/team5/mopl/user/controller/UserControllerTest.java \ src/main/java/com/codeit/team5/mopl/global/config/SecurityConfig.java do if [ -f "$f" ]; then echo "--- $f ---" sed -n '1,120p' "$f" else echo "--- missing: $f ---" fi doneRepository: sb10-part4-team5/sb10-mopl-team5
Length of output: 14781
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path paths = [ Path('src/test/java/com/codeit/team5/mopl/auth/controller/AuthControllerTest.java'), Path('src/test/java/com/codeit/team5/mopl/user/controller/UserControllerTest.java'), Path('src/main/java/com/codeit/team5/mopl/global/config/SecurityConfig.java'), Path('src/main/java/com/codeit/team5/mopl/config/SecurityConfig.java'), ] for p in paths: print(p, 'exists=' + str(p.exists())) PYRepository: sb10-part4-team5/sb10-mopl-team5
Length of output: 504
옛
SecurityConfigimport를 새 패키지로 바꿔야 합니다.
src/test/java/com/codeit/team5/mopl/auth/controller/AuthControllerTest.java와src/test/java/com/codeit/team5/mopl/user/controller/UserControllerTest.java가 아직com.codeit.team5.mopl.config.SecurityConfig를 import합니다. 현재 클래스는com.codeit.team5.mopl.global.config.SecurityConfig만 있으므로, 이대로면 테스트 컴파일이 깨집니다. 패키지 이동 시 관련 참조를 함께 갱신하는 편이 안전합니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/codeit/team5/mopl/global/config/SecurityConfig.java` at line 1, Update the stale SecurityConfig imports in the affected tests: AuthControllerTest and UserControllerTest still reference com.codeit.team5.mopl.config.SecurityConfig, but the class now lives under com.codeit.team5.mopl.global.config.SecurityConfig. Replace the old import with the new package wherever SecurityConfig is referenced so the test sources compile against the relocated config class.
41-50: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
anyRequest().permitAll()때문에 프로필 수정 엔드포인트가 인증 없이 노출됩니다.이 PR의 목표(
#15)는 "사용자가 자신의 프로필을 수정"하는 것인데, 추가된PATCH /api/users/{userId}는 현재 인증 보호를 전혀 받지 못합니다.왜 그런가:
.requestMatchers("/api/users")는 정확히/api/users경로(회원가입 POST)만 매칭하고/api/users/{userId}같은 하위 경로는 매칭하지 않습니다. 따라서 프로필 수정 요청은 마지막의.anyRequest().permitAll()로 떨어져 누구나 임의의userId프로필을 수정할 수 있게 됩니다. 이는 인증·인가 우회로, 본인 외 타인 프로필 변조가 가능한 심각한 보안 결함입니다.대안 비교:
- 옵션 A (권장): 기본 거부 정책.
.anyRequest().permitAll()을.anyRequest().authenticated()로 바꾸고, 공개가 필요한 경로만 명시적으로permitAll. 장점: 새 엔드포인트가 추가돼도 기본적으로 보호됨(가장 안전). 단점: 기존에 암묵적으로 열려 있던 경로를 빠짐없이 화이트리스트로 옮겨야 함.- 옵션 B: 프로필 경로만 명시적 보호.
.requestMatchers(HttpMethod.PATCH, "/api/users/*").authenticated()추가. 장점: 변경 범위 최소. 단점: 여전히 "기본 허용" 정책이라 향후 새 엔드포인트가 또 노출될 위험이 남음.추가로, 본인 프로필만 수정 가능해야 하므로
userId와 인증 주체의 소유권 검증(서비스/메서드 레벨)도 함께 필요합니다.🔒️ 옵션 A 기반 제안
.authorizeHttpRequests(auth -> auth .requestMatchers("/api/auth/sign-out").authenticated() .requestMatchers("/api/users").permitAll() .requestMatchers("/api/auth/sign-in").permitAll() // Swagger, actuator 필요하면 추가 .requestMatchers("/swagger-ui/**", "/v3/api-docs/**").permitAll() - .anyRequest().permitAll() + .anyRequest().authenticated() )📝 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..authorizeHttpRequests(auth -> auth .requestMatchers("/api/auth/sign-out").authenticated() .requestMatchers("/api/users").permitAll() .requestMatchers("/api/auth/sign-in").permitAll() // Swagger, actuator 필요하면 추가 .requestMatchers("/swagger-ui/**", "/v3/api-docs/**").permitAll() .anyRequest().authenticated() )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/codeit/team5/mopl/global/config/SecurityConfig.java` around lines 41 - 50, The security rule in SecurityConfig is too permissive because any PATCH profile update falls through to anyRequest().permitAll(). Update the authorizeHttpRequests chain so the user profile update path handled by PATCH /api/users/{userId} is protected (prefer a default authenticated policy via anyRequest().authenticated(), while keeping only sign-in/sign-out, registration, and Swagger explicitly public). Also ensure the profile update flow enforces ownership checks in the user update service/controller so only the authenticated user can modify their own profile.src/main/java/com/codeit/team5/mopl/global/config/StorageConfig.java (1)
27-35: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
자격증명 “부분 설정” 케이스가 한쪽만 막혀 있습니다.
accessKey만 있는 경우는 막지만,secretKey만 있는 경우는 현재 통과되어DefaultCredentialsProvider로 떨어집니다. 설정 실수 시 원인 파악이 어려워집니다.
accessKey/secretKey는 둘 다 존재하거나 둘 다 비어야만 하도록 대칭 검증을 추가해주세요.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/codeit/team5/mopl/global/config/StorageConfig.java` around lines 27 - 35, StorageConfig의 자격증명 검증이 비대칭이라 secretKey만 설정된 부분 설정 케이스가 DefaultCredentialsProvider로 숨겨집니다. hasAccessKey 판단과 함께 secretKey만 존재하는 경우도 명시적으로 검사해 accessKey/secretKey가 둘 다 있거나 둘 다 비어 있어야 한다는 검증을 추가하고, 기존 IllegalStateException 패턴에 맞춰 credentialsProvider 선택 전에 차단되도록 수정하세요.src/main/java/com/codeit/team5/mopl/global/dto/FileResource.java (1)
11-14: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
byte[]참조가 그대로 노출되어 업로드 페이로드 무결성이 깨질 수 있습니다.
record필드/접근자가 배열 참조를 그대로 전달해서, 이후 변경 시 비동기 업로드 데이터가 변질될 수 있습니다.
선택지:
- 권장: 생성 시/조회 시 방어적 복사(무결성↑, 비용 매우 작음)
- 대안: 현재처럼 참조 공유(성능 미세 이점, 안전성↓)
예시 수정안
public record FileResource( byte[] bytes, String filename ) { + public FileResource { + bytes = (bytes == null) ? null : Arrays.copyOf(bytes, bytes.length); + } + + `@Override` + public byte[] bytes() { + return (bytes == null) ? null : Arrays.copyOf(bytes, bytes.length); + }📝 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.public record FileResource( byte[] bytes, String filename ) { public FileResource { bytes = (bytes == null) ? null : Arrays.copyOf(bytes, bytes.length); } `@Override` public byte[] bytes() { return (bytes == null) ? null : Arrays.copyOf(bytes, bytes.length); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/codeit/team5/mopl/global/dto/FileResource.java` around lines 11 - 14, The FileResource record currently exposes the byte[] directly, so callers can mutate the payload after construction. Update FileResource to use defensive copying in the canonical constructor and accessor for bytes, while keeping filename unchanged, so the stored upload data cannot be altered through shared array references.src/main/java/com/codeit/team5/mopl/user/controller/api/UserApi.java (1)
63-84: 🔒 Security & Privacy | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # updateUser/update 경로에 인가(소유자) 검사가 존재하는지 확인 fd -e java 'UserController.java|UserService.java' src/main \ --exec sh -c 'echo "== $1 =="; cat -n "$1"' _ {} # 메서드 보안/소유자 비교 패턴 탐색 rg -nP '`@PreAuthorize`|`@PostAuthorize`|principal|getId\(\)\s*==|equals\(\s*userId' src/main/java/com/codeit/team5/mopl/user -C2 # 시큐리티 설정에서 /api/users PATCH 인가 규칙 확인 fd -e java 'SecurityConfig.java' src/main --exec cat -n {}Repository: sb10-part4-team5/sb10-mopl-team5
Length of output: 12099
PATCH /api/users/{userId}인가를 추가해야 합니다
UserApi.java:63-84의 403 문구와 달리, 현재UserService.update()에는 본인 확인 로직이 없고SecurityConfig도 해당 경로를 막지 않아 타인userId로 프로필을 수정할 수 있습니다.
@PreAuthorize("#userId== authentication.principal.id")처럼 선언적으로 막거나, 서비스 진입부에서 인증 주체와userId를 비교해 불일치 시 403을 던지도록 넣어야 합니다. 전자는 간결하지만 principal 구조에 의존하고, 후자는 명시적이어서 테스트가 쉽습니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/codeit/team5/mopl/user/controller/api/UserApi.java` around lines 63 - 84, The PATCH /api/users/{userId} endpoint is missing ownership authorization, so anyone can submit another userId and update that profile. Add a guard in the update flow using the UserApi.updateUser and UserService.update paths, either with a declarative check like `@PreAuthorize` against authentication.principal.id or an explicit service-level comparison between the authenticated user and the path userId. If they do not match, reject with 403 before any profile changes are applied.src/test/java/com/codeit/team5/mopl/user/controller/UserControllerIntegrationTest.java (1)
244-245: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
여기서도
$.exceptionName잔재로 통합 테스트가 깨집니다.같은 파일의
createUser_missingName_returnsBadRequest(Line 118)는 동일한INVALID_INPUT응답을$.exceptionType으로 검증하고 있어, 이 케이스의$.exceptionName은 존재하지 않는 경로를 단언합니다. 결과적으로 "이름 공백 → 400 + 미반영" 시나리오 검증이 실패로 귀결됩니다. UserControllerTest의 동일 케이스와 함께 키를 통일해 주세요.🔧 제안 수정
- .andExpect(jsonPath("$.exceptionName").value("INVALID_INPUT")); + .andExpect(jsonPath("$.exceptionType").value("INVALID_INPUT"));📝 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..andExpect(status().isBadRequest()) .andExpect(jsonPath("$.exceptionType").value("INVALID_INPUT"));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/codeit/team5/mopl/user/controller/UserControllerIntegrationTest.java` around lines 244 - 245, The integration test is still asserting the obsolete $.exceptionName field for the INVALID_INPUT response, while the matching case in UserControllerIntegrationTest already uses $.exceptionType. Update this assertion to use the same response key as the other UserController tests and the controller’s error payload shape, so the shared invalid-name scenario is verified against the correct field.src/test/java/com/codeit/team5/mopl/user/controller/UserControllerTest.java (1)
348-349: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
$.exceptionName은 마이그레이션에서 누락된 잔재로, 이 테스트는 실패합니다.같은 파일의 다른 검증 케이스들(예: Line 124, 147, 211)은 모두
$.exceptionType로 옮겨졌는데, 이 신규 케이스만 옛 필드명$.exceptionName을 단언하고 있습니다.INVALID_INPUT응답은exceptionType키로 내려오므로 존재하지 않는 경로를 단언하게 되어 케이스가 깨집니다. 즉 "이름 공백이면 400" 회귀를 보호하려던 의도와 달리, 매번 실패하거나 잘못된 신호를 줍니다.수정 방향은 단순합니다 — 파일 전반의 컨벤션에 맞춰 키를 통일하면 됩니다.
🔧 제안 수정
- .andExpect(jsonPath("$.exceptionName").value("INVALID_INPUT")); + .andExpect(jsonPath("$.exceptionType").value("INVALID_INPUT"));📝 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..andExpect(status().isBadRequest()) .andExpect(jsonPath("$.exceptionType").value("INVALID_INPUT"));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/codeit/team5/mopl/user/controller/UserControllerTest.java` around lines 348 - 349, The new invalid-input test is asserting the old response field name, so it fails against the current API contract. Update the `UserControllerTest` case that checks the `INVALID_INPUT` response to use `$.exceptionType` instead of `$.exceptionName`, matching the other assertions in this test class and the actual serialized error payload.
This was referenced Jun 25, 2026
dstle
force-pushed
the
feature/15-update-profile
branch
from
7a0fd7a to
05fc5ab
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/com/codeit/team5/mopl/user/controller/UserControllerTest.java (1)
317-317: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
updateUser_success에서 서비스 호출 인자 검증이 너무 느슨합니다.현재
any()3개 검증은 경로userId/request.name/image매핑이 깨져도 통과할 수 있습니다. 최소eq(userId)와 요청값 캡처 검증으로 계약을 고정해 주세요.제안 수정안
- given(userService.update(any(), any(), any())).willReturn(response); + given(userService.update(eq(userId), any(UserUpdateRequest.class), any())).willReturn(response); ... - verify(userService).update(any(), any(), any()); + ArgumentCaptor<UserUpdateRequest> requestCaptor = ArgumentCaptor.forClass(UserUpdateRequest.class); + verify(userService).update(eq(userId), requestCaptor.capture(), any()); + assertThat(requestCaptor.getValue().name()).isEqualTo("새이름");Also applies to: 333-333
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/codeit/team5/mopl/user/controller/UserControllerTest.java` at line 317, The `updateUser_success` test is too loose because `userService.update` is stubbed with three `any()` matchers, so broken mapping from `userId`, `request.name`, or `image` could still pass. Tighten the test around `userService.update` by matching the path `userId` with `eq(userId)` and capturing the request/image arguments to assert they contain the expected values from the controller input. Apply the same stronger verification in the other affected `userService.update` test case so the controller-to-service contract stays fixed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/test/java/com/codeit/team5/mopl/user/controller/UserControllerTest.java`:
- Line 317: The `updateUser_success` test is too loose because
`userService.update` is stubbed with three `any()` matchers, so broken mapping
from `userId`, `request.name`, or `image` could still pass. Tighten the test
around `userService.update` by matching the path `userId` with `eq(userId)` and
capturing the request/image arguments to assert they contain the expected values
from the controller input. Apply the same stronger verification in the other
affected `userService.update` test case so the controller-to-service contract
stays fixed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4961c1a8-0fcc-4c72-81d2-498f66f9e175
📒 Files selected for processing (7)
src/main/java/com/codeit/team5/mopl/binarycontent/exception/InvalidImageExtensionException.javasrc/test/java/com/codeit/team5/mopl/auth/controller/AuthControllerTest.javasrc/test/java/com/codeit/team5/mopl/binarycontent/storage/s3/S3BinaryContentStorageTest.javasrc/test/java/com/codeit/team5/mopl/content/controller/ContentControllerTest.javasrc/test/java/com/codeit/team5/mopl/user/controller/UserControllerIntegrationTest.javasrc/test/java/com/codeit/team5/mopl/user/controller/UserControllerTest.javasrc/test/java/com/codeit/team5/mopl/user/service/UserServiceTest.java
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/test/java/com/codeit/team5/mopl/user/service/UserServiceTest.java (1)
220-221: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win업로드 이벤트 페이로드를
any()로 숨기지 말고 캡처하세요.
BinaryContentUploadEvent가 실제 업로드 경계라서, key나 bytes가 잘못 들어가도 현재 테스트는 통과합니다. 이벤트를 캡처해 저장 키와 원본 바이트를 확인하면 S3/로컬 업로드 회귀를 바로 잡을 수 있습니다.🧪 이벤트 페이로드 검증 예시
- verify(eventPublisher).publishEvent(any(BinaryContentUploadEvent.class)); + ArgumentCaptor<BinaryContentUploadEvent> eventCaptor = + ArgumentCaptor.forClass(BinaryContentUploadEvent.class); + verify(eventPublisher).publishEvent(eventCaptor.capture()); + assertThat(eventCaptor.getValue().key()).isEqualTo("profiles/key.jpg"); + assertThat(eventCaptor.getValue().bytes()).containsExactly(1, 2, 3);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/codeit/team5/mopl/user/service/UserServiceTest.java` around lines 220 - 221, The test currently verifies BinaryContentUploadEvent with any(), which hides the actual payload and can miss upload regressions. Update UserServiceTest to capture the published BinaryContentUploadEvent from eventPublisher.publishEvent in the upload flow and assert its key and bytes match the saved BinaryContent and original input, using the existing binaryContentRepository and eventPublisher verification points to locate the change.src/test/java/com/codeit/team5/mopl/user/controller/UserControllerTest.java (1)
326-333: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win서비스 호출 인자를 캡처해 multipart 바인딩 계약까지 검증하세요.
현재는
userService.update(any(), any(), any())로만 검증해서, 컨트롤러가userId,request.name, 이미지 파일명/바이트를 잘못 전달해도 mock 응답만 반환되면 테스트가 통과합니다.ContentControllerTest처럼 captor로 실제 변환 결과를 확인하면 API 경계 회귀를 더 잘 잡을 수 있습니다.🧪 검증 강화 예시
- verify(userService).update(any(), any(), any()); + ArgumentCaptor<UserUpdateRequest> requestCaptor = ArgumentCaptor.forClass(UserUpdateRequest.class); + ArgumentCaptor<FileRequest> imageCaptor = ArgumentCaptor.forClass(FileRequest.class); + verify(userService).update(eq(userId), requestCaptor.capture(), imageCaptor.capture()); + assertThat(requestCaptor.getValue().name()).isEqualTo("새이름"); + assertThat(imageCaptor.getValue().filename()).isEqualTo("profile.jpg"); + assertThat(imageCaptor.getValue().bytes()).containsExactly(1, 2, 3);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/codeit/team5/mopl/user/controller/UserControllerTest.java` around lines 326 - 333, The test currently only verifies that UserControllerTest calls userService.update with any arguments, so it can miss broken multipart binding. Update the test to capture the actual arguments passed from the controller and assert the mapped userId, request name, and uploaded image file details (such as original filename and content) instead of relying on broad any() matchers. Use the existing UserControllerTest and userService.update call site to locate the assertion, and follow the capture-and-verify style used in ContentControllerTest.
♻️ Duplicate comments (1)
src/main/java/com/codeit/team5/mopl/binarycontent/BinaryContentStorage.java (1)
10-21: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
generateKey와validateImageKey계약이 아직 서로 다릅니다.Line 10-15는 미허용 확장자나 무확장자 파일명을 받아도
.../{uuid}형태의 키를 그냥 만들어 주는데, Line 18-21과LocalBinaryContentStorage.store/S3BinaryContentStorage.store는 같은 키를validateImageKey(key)로 바로 거부합니다. 지금 구조에서는 키 생성과BinaryContent.pending(...)저장은 먼저 성공하고, 실제 업로드 단계에서만 실패해서 PENDING 레코드만 남는 불일치가 생길 수 있습니다.방향을 하나로 고정하는 편이 안전합니다.
- fail-fast로 통일:
generateKey에서InvalidImageExtensionException을 바로 던지기- 무확장자 허용으로 통일:
validateImageKey와 각 저장소store도 같은 규칙으로 맞추기예시: fail-fast로 맞추는 최소 수정안
default String generateKey(StorageDirectory directory, UUID ownerId, String originalFilename) { String extension = StringUtils.getFilenameExtension(originalFilename); - String normalizedExt = ImageExtension.from(extension) - .map(e -> "." + e.name().toLowerCase(Locale.ROOT)) - .orElse(""); + ImageExtension imageExtension = ImageExtension.from(extension) + .orElseThrow(() -> new InvalidImageExtensionException(extension)); + String normalizedExt = "." + imageExtension.name().toLowerCase(Locale.ROOT); return directory.value() + "/" + ownerId + "/" + UUID.randomUUID() + normalizedExt; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/codeit/team5/mopl/binarycontent/BinaryContentStorage.java` around lines 10 - 21, `generateKey`와 `validateImageKey`의 확장자 처리 규칙이 서로 달라 `BinaryContent.pending(...)`는 성공하지만 `LocalBinaryContentStorage.store`와 `S3BinaryContentStorage.store`에서 뒤늦게 실패하는 불일치가 생깁니다. `BinaryContentStorage.generateKey`에서 허용되지 않은 확장자나 무확장자 파일명을 바로 거부하도록 `validateImageKey`와 동일한 정책으로 맞추고, 필요하면 `LocalBinaryContentStorage.store`와 `S3BinaryContentStorage.store`도 같은 규칙을 공유하도록 정리하세요. 이렇게 해서 키 생성 시점과 저장 시점의 검증 결과가 항상 일치하게 만드세요.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/codeit/team5/mopl/config/StorageConfig.java`:
- Around line 30-38: The credential validation in StorageConfig should reject
partial AWS credentials in both directions, not just when accessKey exists
without secretKey. Update the logic around hasAccessKey, hasSecretKey, and the
AwsCredentialsProvider selection so that any case where exactly one of the two
values is set throws an IllegalStateException before falling back to
DefaultCredentialsProvider, while still allowing StaticCredentialsProvider only
when both keys are present.
In
`@src/test/java/com/codeit/team5/mopl/content/controller/ContentControllerTest.java`:
- Line 115: The ContentControllerTest verification is too weak because create()
is only checked with any(FileRequest.class), so it won’t catch incorrect
filename or byte conversion. Update the test around contentService.create and
FileRequest handling by capturing the FileRequest argument with an
ArgumentCaptor, then assert both the filename and bytes alongside the existing
requestCaptor checks. Keep the focus on the controller-to-service contract in
ContentControllerTest and the create() call.
---
Outside diff comments:
In `@src/test/java/com/codeit/team5/mopl/user/controller/UserControllerTest.java`:
- Around line 326-333: The test currently only verifies that UserControllerTest
calls userService.update with any arguments, so it can miss broken multipart
binding. Update the test to capture the actual arguments passed from the
controller and assert the mapped userId, request name, and uploaded image file
details (such as original filename and content) instead of relying on broad
any() matchers. Use the existing UserControllerTest and userService.update call
site to locate the assertion, and follow the capture-and-verify style used in
ContentControllerTest.
In `@src/test/java/com/codeit/team5/mopl/user/service/UserServiceTest.java`:
- Around line 220-221: The test currently verifies BinaryContentUploadEvent with
any(), which hides the actual payload and can miss upload regressions. Update
UserServiceTest to capture the published BinaryContentUploadEvent from
eventPublisher.publishEvent in the upload flow and assert its key and bytes
match the saved BinaryContent and original input, using the existing
binaryContentRepository and eventPublisher verification points to locate the
change.
---
Duplicate comments:
In `@src/main/java/com/codeit/team5/mopl/binarycontent/BinaryContentStorage.java`:
- Around line 10-21: `generateKey`와 `validateImageKey`의 확장자 처리 규칙이 서로 달라
`BinaryContent.pending(...)`는 성공하지만 `LocalBinaryContentStorage.store`와
`S3BinaryContentStorage.store`에서 뒤늦게 실패하는 불일치가 생깁니다.
`BinaryContentStorage.generateKey`에서 허용되지 않은 확장자나 무확장자 파일명을 바로 거부하도록
`validateImageKey`와 동일한 정책으로 맞추고, 필요하면 `LocalBinaryContentStorage.store`와
`S3BinaryContentStorage.store`도 같은 규칙을 공유하도록 정리하세요. 이렇게 해서 키 생성 시점과 저장 시점의 검증
결과가 항상 일치하게 만드세요.
🪄 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: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c798188f-4558-45a1-809d-1b51d610ce15
📒 Files selected for processing (13)
src/main/java/com/codeit/team5/mopl/binarycontent/BinaryContentStorage.javasrc/main/java/com/codeit/team5/mopl/binarycontent/StorageDirectory.javasrc/main/java/com/codeit/team5/mopl/binarycontent/support/MultipartFiles.javasrc/main/java/com/codeit/team5/mopl/config/StorageConfig.javasrc/main/java/com/codeit/team5/mopl/content/service/ContentService.javasrc/main/java/com/codeit/team5/mopl/global/dto/FileRequest.javasrc/main/java/com/codeit/team5/mopl/user/entity/User.javasrc/main/java/com/codeit/team5/mopl/user/service/UserService.javasrc/test/java/com/codeit/team5/mopl/binarycontent/storage/local/LocalBinaryContentStorageTest.javasrc/test/java/com/codeit/team5/mopl/content/controller/ContentControllerTest.javasrc/test/java/com/codeit/team5/mopl/content/service/ContentServiceTest.javasrc/test/java/com/codeit/team5/mopl/user/controller/UserControllerTest.javasrc/test/java/com/codeit/team5/mopl/user/service/UserServiceTest.java
# Conflicts: # src/main/java/com/codeit/team5/mopl/global/config/JwtPropertiesConfig.java # src/main/java/com/codeit/team5/mopl/global/config/PasswordEncoderConfig.java # src/main/java/com/codeit/team5/mopl/user/service/UserService.java # src/test/java/com/codeit/team5/mopl/content/controller/ContentControllerTest.java # src/test/java/com/codeit/team5/mopl/user/controller/UserControllerTest.java
dstle
force-pushed
the
feature/15-update-profile
branch
from
47d16a7 to
18a5f76
Compare
plzslp
approved these changes
Jun 25, 2026
plzslp
previously approved these changes
Jun 25, 2026
plzslp
approved these changes
Jun 26, 2026
This was referenced Jun 26, 2026
9 tasks
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.
관련 이슈
closes [Feat] 프로필 관리 - 프로필 사진 수정 #15
closes [Feat] S3 파일 업로드 구현 #145
개요
사용자 프로필 변경 API(이름/이미지)를 추가하고, 이미지의 실제 S3
저장을 구현했습니다.
함께 이미지 업로드 공통 구조(확장자 검증, 파일 변환, 폴더 prefix)를
정리했습니다.
주요 변경
프로필 변경 API
PATCH /api/users/{userId}(multipart/form-data)name(필수) +image(선택, 빈 값 허용)S3 이미지 저장 구현
S3Client빈(StorageConfig)accessKey 설정 시 Static
store에서putObject+ Content-Type 지정accessKey 설정 시 Static
store에서putObject+ Content-Type 지정mopl.storage.type=s3일 때만 활성(ConditionalOnProperty)이미지 공통 구조 정리
ImageExtensionenum: 허용 확장자 + Content-Type 일원화 (확장자추가가 한 곳에서)
MultipartFiles.toImageResource, fail-fast)validateImageKey)→ 컨트롤러를 거치지 않는 경로(배치 등)도 방어
FileResource+MultipartFiles: MultipartFile(web 타입)을서비스에서 분리 (Content도 동일 적용)
StoragePrefixenum: 폴더 prefix(thumbnails/profiles) 관리리팩토링 / 수정
config→global.config(컨벤션),MultipartFiles→
binarycontent.supportapplication-prod.yml의 s3bucket→bucket-name(바인딩누락 수정)
테스트
변경에 맞춰 갱신·통과 확인했습니다.
mopl프로파일로 실제 업로드 검증가능합니다.
후속 작업
@PreAuthorize등으로 적용
변경 사항
테스트 내용
체크리스트
리뷰 포인트
스크린샷 / 참고 자료
🔗 관련 이슈
#15#145📝 작업 내용
이 PR은 사용자 프로필 수정과 이미지 업로드 흐름을 하나로 묶어, 프로필 이름 변경과 프로필 이미지를 함께 처리할 수 있도록 개선합니다.
기존에는 업로드 파일이 컨트롤러/서비스에서 직접 다뤄져 책임이 분산되어 있었고, 저장소별 키 규칙과 확장자 검증도 각 구현체에 흩어져 있었습니다. 이번 변경으로 업로드 입력, 키 생성, 확장자 검증, 저장 위치(prefix) 관리가 공통 구조로 정리되었고, 로컬 저장과 S3 저장이 같은 규칙을 따르도록 맞췄습니다.
주요 변경사항
사용자 프로필 수정 API를 추가해
PATCH /api/users/{userId}에서 이름과 이미지 업로드를 함께 처리하도록 했습니다.multipart/form-data 기반으로 요청을 받고, 이미지가 없으면 기존 프로필 이미지를 유지하는 흐름으로 바뀌었습니다.
이미지 처리 공통 로직을 분리해 확장자 검증, 파일 변환, 저장 경로(prefix)를 재사용 가능하게 정리했습니다.
FileRequest,ImageExtension,StorageDirectory를 도입해 웹 계층의 multipart 파일과 저장 계층의 바이너리 처리 사이 경계를 명확히 했습니다.S3 저장을 실제 업로드 가능한 구현으로 전환했습니다.
AWS SDK v2 기반
S3Client를 도입하고, 운영 환경에서는 기본 자격 증명 체인, 로컬 환경에서는 설정된 정적 자격 증명을 사용할 수 있게 했습니다.S3 사용은
mopl.storage.type=s3일 때만 활성화됩니다.저장/업로드 예외 처리와 API 계약을 정비했습니다.
허용되지 않은 이미지 확장자는 명시적인 도메인 예외로 처리하고, S3 업로드 실패는 저장소 예외로 래핑해 실패 원인을 일관되게 다루도록 했습니다.
📊 변경 효과 요약 (가능한 경우에만)