refactor: 문의 관련 첨부파일 public 으로 변경 #337
Conversation
WalkthroughReplaced pre-signed S3 URL usage with public URL retrieval across inquiry-related services. Updated Folder enum and FileRequest mappings to mark inquiry and inquiry-answer attachments as PUBLIC. Adjusted S3Service API from getPreSignedUrl to getPublicUrl. No other control flow or public APIs changed in services. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant InquiryService
participant S3Service
Client->>InquiryService: Request attachment/detail
InquiryService->>S3Service: getPublicUrl(s3Key)
S3Service-->>InquiryService: publicUrl
InquiryService-->>Client: Response with publicUrl
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @chominju02, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
이 PR은 문의 및 문의 답변 관련 첨부파일의 접근 방식을 변경하여, 기존의 임시 서명된 URL 대신 직접적인 공개 URL을 통해 접근할 수 있도록 합니다. 이는 첨부파일의 가시성을 PRIVATE에서 PUBLIC으로 전환하고, 관련 서비스 로직을 업데이트하는 것을 포함합니다.
Highlights
- 첨부파일 URL 생성 방식 변경: 문의 및 문의 답변 첨부파일에 대해 S3 preSignedUrl 대신 publicUrl을 사용하도록 변경되었습니다.
- 폴더 가시성 업데이트: Folder enum에서 INQUIRY 및 INQUIRY_ANSWER 폴더의 가시성이 PRIVATE에서 PUBLIC으로 변경되었습니다.
- 새 첨부파일 기본 가시성 설정: FileRequest에서 새로운 문의 및 문의 답변 첨부파일 생성 시 기본 가시성을 Visibility.PUBLIC으로 설정하도록 수정되었습니다.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
이번 PR은 문의 관련 첨부파일의 접근 권한을 private에서 public으로 변경하는 리팩토링을 수행했습니다. S3 URL 생성 로직을 pre-signed URL에서 public URL로 변경하고, 관련 enum과 엔티티의 가시성 설정을 업데이트한 점이 확인됩니다. 전반적으로 변경 사항은 명확하며 PR의 목적에 부합합니다. 다만, 코드의 유지보수성을 높이기 위해 S3Service의 getUrl 메서드를 활용하는 방안을 제안합니다. 자세한 내용은 각 파일의 주석을 참고해주세요.
| String publicUrl = s3Service.getPublicUrl(attachment.getS3Key()); | ||
|
|
||
| return new InquiryDetailResponse.AttachmentResponse( | ||
| attachment.getFileName(), | ||
| preSignedUrl | ||
| publicUrl | ||
| ); |
There was a problem hiding this comment.
S3Service에 파일의 공개 여부에 따라 적절한 URL(public 또는 pre-signed)을 반환하는 getUrl(File file) 메서드가 있습니다. getPublicUrl을 직접 호출하는 대신 s3Service.getUrl(attachment)를 사용하면 향후 가시성 정책이 변경되더라도 이 코드를 수정할 필요가 없어 유지보수성이 향상됩니다.
| String publicUrl = s3Service.getPublicUrl(attachment.getS3Key()); | |
| return new InquiryDetailResponse.AttachmentResponse( | |
| attachment.getFileName(), | |
| preSignedUrl | |
| publicUrl | |
| ); | |
| String url = s3Service.getUrl(attachment); | |
| return new InquiryDetailResponse.AttachmentResponse( | |
| attachment.getFileName(), | |
| url | |
| ); |
| String publicUrl = s3Service.getPublicUrl(attachment.getS3Key()); | ||
|
|
||
| return new InquiryDetailResponse.AttachmentDetailResponse( | ||
| attachment.getFileName(), | ||
| preSignedUrl, | ||
| publicUrl, | ||
| attachment.getS3Key() | ||
| ); |
There was a problem hiding this comment.
createAttachResponse 메서드와 마찬가지로, s3Service.getUrl(attachment)를 사용하면 코드의 유연성과 유지보수성을 높일 수 있습니다. 파일의 visibility 속성에 따라 URL 생성을 S3Service에 위임하는 것이 좋습니다.
| String publicUrl = s3Service.getPublicUrl(attachment.getS3Key()); | |
| return new InquiryDetailResponse.AttachmentDetailResponse( | |
| attachment.getFileName(), | |
| preSignedUrl, | |
| publicUrl, | |
| attachment.getS3Key() | |
| ); | |
| String url = s3Service.getUrl(attachment); | |
| return new InquiryDetailResponse.AttachmentDetailResponse( | |
| attachment.getFileName(), | |
| url, | |
| attachment.getS3Key() | |
| ); |
| String publicUrl = s3Service.getPublicUrl(attachment.getS3Key()); | ||
|
|
||
| return new InquiryDetailResponse.AttachmentDetailResponse( | ||
| attachment.getFileName(), | ||
| preSignedUrl, | ||
| publicUrl, | ||
| attachment.getS3Key() | ||
| ); |
There was a problem hiding this comment.
S3Service에 파일의 공개 여부에 따라 적절한 URL(public 또는 pre-signed)을 반환하는 getUrl(File file) 메서드가 있습니다. getPublicUrl을 직접 호출하는 대신 s3Service.getUrl(attachment)를 사용하면 향후 가시성 정책이 변경되더라도 이 코드를 수정할 필요가 없어 유지보수성이 향상됩니다.
| String publicUrl = s3Service.getPublicUrl(attachment.getS3Key()); | |
| return new InquiryDetailResponse.AttachmentDetailResponse( | |
| attachment.getFileName(), | |
| preSignedUrl, | |
| publicUrl, | |
| attachment.getS3Key() | |
| ); | |
| String url = s3Service.getUrl(attachment); | |
| return new InquiryDetailResponse.AttachmentDetailResponse( | |
| attachment.getFileName(), | |
| url, | |
| attachment.getS3Key() | |
| ); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/main/java/life/mosu/mosuserver/presentation/common/FileRequest.java (1)
22-29: Enforce s3Key validation in FileUploadHelper.saveAttachmentsFileUploadHelper currently accepts any client-provided s3Key, which allows referencing or tagging unintended S3 objects. Add a validation step before tagging to prevent IDOR and path traversal.
• Location to update:
• src/main/java/life/mosu/mosuserver/infra/persistence/s3/FileUploadHelper.java – insidesaveAttachments(...)beforeupdateTag(s3Key)• Example diff:
public <R, E> void saveAttachments( List<R> requests, Long parentId, JpaRepository<E, Long> repository, BiFunction<R, Long, E> toEntityMapper, Function<R, String> getKey ) { if (requests == null || requests.isEmpty()) { return; } List<E> entitiesToSave = new ArrayList<>(requests.size()); R req = null; try { for (R r : requests) { req = r; String s3Key = getKey.apply(req); + validateS3Key(s3Key, parentId); updateTag(s3Key); E entity = toEntityMapper.apply(req, parentId); entitiesToSave.add(entity); } } catch (Exception e) { log.error("saving attachments 에러 발생{}: {}", req, e.getMessage()); } repository.saveAll(entitiesToSave); } +/** Ensure the key is scoped to the expected folder and contains no traversal sequences */ +private void validateS3Key(String s3Key, Long parentId) { + String expectedPrefix = String.format("inquiries/%d/attachments/", parentId); + if (!s3Key.startsWith(expectedPrefix) || s3Key.contains("..")) { + throw new IllegalArgumentException("Invalid s3Key: " + s3Key); + } +}This enforces:
- s3Key belongs to the
inquiries/{id}/attachments/prefix- no
..or other traversal patterns
🧹 Nitpick comments (5)
src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAttachmentService.java (2)
65-69: Reconsider exposing s3Key in public responsesIf attachments are now PUBLIC, returning both publicUrl and s3Key may be redundant and leaks internal storage layout. Unless the client explicitly needs s3Key, prefer removing it from externally exposed DTOs and keep it server-internal.
25-31: Use provided lambda arg instead of capturing inquiryEntityMinor nit: use the id argument from FileUploadHelper.saveAttachments for clarity and to avoid capturing the outer entity unnecessarily.
- (req, id) -> req.toInquiryAttachmentEntity(inquiryEntity.getId()), + (req, id) -> req.toInquiryAttachmentEntity(id),src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerAttachmentService.java (1)
26-32: Use provided lambda arg instead of capturing answerEntityUse the id argument for builder creation to avoid capturing the outer entity.
- (req, id) -> req.toInquiryAnswerAttachmentEntity(answerEntity.getId()), + (req, id) -> req.toInquiryAnswerAttachmentEntity(id),src/main/java/life/mosu/mosuserver/domain/file/Folder.java (1)
18-20: Security impact: making INQUIRY/INQUIRY_ANSWER PUBLIC requires bucket/policy alignmentSwitching to PUBLIC at the domain level is fine if the S3 layer and CDN/bucket policies are aligned. Please verify:
- S3 Block Public Access isn’t preventing these reads (or you’re serving via CloudFront with Origin Access/Origin Access Control and object ACLs set appropriately).
- getPublicUrl returns a CDN URL (preferred) rather than direct S3 URL to leverage caching and control headers.
- Object keys are sufficiently unguessable (UUID-like) to reduce enumeration risk.
- A migration plan exists for historical PRIVATE objects (see service comments).
If any of the above are not yet in place, consider introducing a transitional “visibility-aware URL” method and/or running a backfill.
src/main/java/life/mosu/mosuserver/presentation/common/FileRequest.java (1)
17-18: Update API docs to reflect public usage of s3KeyThe schema description still mentions “비공개 이미지를 처리하기 위한 키”, which is misleading now that these are public. Suggest updating to a neutral description.
- @Schema(description = "S3 키", example = "비공개 이미지를 처리하기 위한 키") + @Schema(description = "S3 키", example = "업로드된 파일을 식별하기 위한 키")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerAttachmentService.java(1 hunks)src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAttachmentService.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/file/Folder.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/common/FileRequest.java(2 hunks)
🔇 Additional comments (2)
src/main/java/life/mosu/mosuserver/presentation/common/FileRequest.java (2)
26-27: LGTM: inquiry attachments now PUBLICThe visibility change aligns with the Folder enum and service layer URL changes.
53-54: LGTM: inquiry-answer attachments now PUBLICConsistent with the rest of the refactor.
| String publicUrl = s3Service.getPublicUrl(attachment.getS3Key()); | ||
|
|
||
| return new InquiryDetailResponse.AttachmentResponse( | ||
| attachment.getFileName(), | ||
| preSignedUrl | ||
| publicUrl | ||
| ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Public URL switch: ensure legacy attachments won’t break
Same concern as for inquiry attachments. Existing inquiry-answer attachments stored as PRIVATE will fail if we now return public URLs. Plan for either (a) data migration to PUBLIC or (b) a temporary conditional URL strategy based on entity visibility.
To reduce risk of dead code and ensure consistency, please also verify whether createAttachResponse(...) is used anywhere:
I can provide a visibility-aware S3Service.getUrl(visibility, key) shim if you want a transitional solution.
Also applies to: 77-83
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check for lingering pre-signed URL usage"
rg -n "getPreSignedUrl\\("
echo
echo "Is createAttachResponse referenced?"
rg -n "createAttachResponse\\("Length of output: 986
Handle PRIVATE inquiry-answer attachments to prevent broken URLs
Verified that the private createAttachResponse method in InquiryAnswerAttachmentService is only called within that class (no dead-code risk). You still need to handle legacy PRIVATE attachments or migrate them to PUBLIC to avoid broken links.
• Files to update:
– src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerAttachmentService.java (around lines 67–72 & 77–83)
– src/main/java/life/mosu/mosuserver/infra/persistence/s3/S3Service.java
• Suggested changes:
- Add a visibility-aware helper in your S3Service:
public String getUrl(Visibility visibility, String s3Key) {
return visibility == Visibility.PUBLIC
? getPublicUrl(s3Key)
: getPreSignedUrl(s3Key);
}- In InquiryAnswerAttachmentService#createAttachResponse, switch to the new helper (or inline conditional):
- String publicUrl = s3Service.getPublicUrl(attachment.getS3Key());
+ String url = s3Service.getUrl(attachment.getVisibility(), attachment.getS3Key());
return new InquiryDetailResponse.AttachmentResponse(
attachment.getFileName(),
- publicUrl
+ url
);Alternatively, if you prefer not to introduce a shim, inline the conditional:
String url = attachment.getVisibility() == Visibility.PUBLIC
? s3Service.getPublicUrl(key)
: s3Service.getPreSignedUrl(key);This ensures PRIVATE attachments continue working until you migrate them to PUBLIC.
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerAttachmentService.java
around lines 67–72 and 77–83 and in
src/main/java/life/mosu/mosuserver/infra/persistence/s3/S3Service.java, handle
PRIVATE attachments so URLs aren’t broken: add a visibility-aware helper in
S3Service (getUrl or similar) that returns a public URL for PUBLIC visibility
and a presigned URL for PRIVATE, then update
InquiryAnswerAttachmentService#createAttachResponse to call that helper (or
inline a conditional) using attachment.getVisibility() to choose between
getPublicUrl and getPreSignedUrl so legacy PRIVATE items continue to work until
migrated.
| String publicUrl = s3Service.getPublicUrl(attachment.getS3Key()); | ||
|
|
||
| return new InquiryDetailResponse.AttachmentDetailResponse( | ||
| attachment.getFileName(), | ||
| preSignedUrl, | ||
| publicUrl, | ||
| attachment.getS3Key() | ||
| ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Switch to public URL: confirm migration/backward-compat for existing private objects
Changing from pre-signed to public URLs works for newly created PUBLIC objects, but existing inquiry attachments previously stored as PRIVATE will 403 if not migrated. Please either:
- run a data migration to update object ACL/metadata and DB visibility to PUBLIC for legacy inquiry attachments, or
- temporarily serve URLs conditionally by the attachment’s stored visibility (PUBLIC → public URL, PRIVATE → pre-signed), until migration completes.
You can quickly check whether the attachment entities carry a visibility field we can branch on, and whether any lingering usages of getPreSignedUrl() remain:
If helpful, I can draft a one-off migration plan (DB update + S3 ACL/object metadata update via prefix) and a temporary dual-path resolver in S3Service.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find visibility on inquiry attachment entities"
rg -n -A 5 -B 2 $'class InquiryAttachmentJpaEntity|visibility|getVisibility\\('
echo
echo "Find any remaining usages of pre-signed URL API"
rg -n "getPreSignedUrl\\("Length of output: 27648
Handle private inquiry attachments by branching on visibility or migrating
The InquiryAttachmentService currently always calls s3Service.getPublicUrl(...), which will 403 for any legacy attachments stored as PRIVATE. Since the domain model indeed persists a visibility field (e.g. InquiryAttachmentJpaEntity extends File with visibility and isPublic()), you should either:
- Branch within
InquiryAttachmentService
Updatesrc/main/java/life/mosu/mosuserver/application/inquiry/InquiryAttachmentService.java(around lines 63–69) to:boolean isPublic = attachment.isPublic(); String url = isPublic ? s3Service.getPublicUrl(attachment.getS3Key()) : s3Service.getPreSignedUrl(attachment.getS3Key()); return new InquiryDetailResponse.AttachmentDetailResponse( attachment.getFileName(), url, attachment.getS3Key() );
- Or perform a one-off migration
- Run a DB script to set
visibility = 'PUBLIC'for all existinginquiry_attachmentrows. - Update S3 object ACLs (and metadata, if needed) under the
inquiry/prefix to be public.
- Run a DB script to set
Either approach prevents 403s for legacy private files.
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAttachmentService.java
around lines 63–69, the service always calls s3Service.getPublicUrl(...) which
will 403 for attachments persisted as PRIVATE; fix by branching on the
attachment visibility: call attachment.isPublic() (or check the visibility
field) and when true use s3Service.getPublicUrl(attachment.getS3Key()),
otherwise use s3Service.getPreSignedUrl(attachment.getS3Key()), then return the
AttachmentDetailResponse with the chosen URL and existing fileName and s3Key;
alternatively, if you prefer a migration approach, run a one‑time DB script to
set visibility='PUBLIC' for existing inquiry_attachment rows and update S3
object ACLs under the inquiry/ prefix so existing getPublicUrl calls continue to
work.
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit