[DDING-000] 이메일 전송 현황 조회 구현 #361
Open
Conversation
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
Gopistol
requested review from
5uhwann,
KoSeonJe,
Seooooo24 and
wonjunYou
as code owners
Walkthrough이 PR은 특정 폼의 이메일 전송 상태를 조회하는 새로운 GET 엔드포인트를 추가하고, 관련 DTO/쿼리, 서비스·저장소 메서드 및 이메일 히스토리 집계 로직을 도입합니다. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Controller as CentralFormController
participant Facade as FacadeCentralFormService
participant FormEmailService as FormEmailSendHistoryService
participant EmailService as EmailSendHistoryService
participant RepoEmail as EmailSendHistoryRepository
participant Entity as EmailSendHistories
Client->>Controller: GET /server/central/my/forms/{formId}/emails/status
Controller->>Facade: getEmailSendStatusByFormId(formId)
Facade->>FormEmailService: getAllByFormId(formId)
FormEmailService-->>Facade: List<FormEmailSendHistory>
Facade->>EmailService: getAllByFormEmailSendHistoryIds(ids)
EmailService->>RepoEmail: findAllByFormEmailSendHistoryIdIn(ids)
RepoEmail-->>EmailService: List<EmailSendHistory>
EmailService-->>Facade: EmailSendHistories
Facade->>Entity: EmailSendHistories.getLatestByFormApplication()
Entity-->>Facade: EmailSendHistories (latest)
Facade-->>Controller: EmailSendStatusQuery
Controller->>Controller: EmailSendStatusResponse.from(query)
Controller-->>Client: 200 OK + EmailSendStatusResponse
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/main/java/ddingdong/ddingdongBE/domain/form/api/CentralFormApi.java`:
- Around line 157-165: The API getEmailSendStatus currently lacks owner
verification causing an IDOR; update the controller method signature
getEmailSendStatus to accept the authenticated principal (e.g.,
`@AuthenticationPrincipal` PrincipalDetails principal) and pass principal.getId()
(or equivalent) into the service, then implement ownership validation in the
service (create/modify getEmailSendStatusByFormId(Long formId, Long userId)) by
calling clubService.getByUserId(userId), formService.getById(formId) and
validateEqualsClub(club, form) before returning the EmailSendStatusResponse.
In
`@src/main/java/ddingdong/ddingdongBE/domain/form/service/dto/query/EmailSendStatusQuery.java`:
- Around line 30-38: The EntityGraph for
findAllFetchedByFormEmailSendHistoryIdIn() only fetches formApplication, so
EmailSendStatusInfoQuery.from() will trigger a LazyInitializationException when
accessing emailSendHistory.getFormEmailSendHistory().getFormApplicationStatus()
outside a transaction; update the repository annotation to include
formEmailSendHistory (e.g., `@EntityGraph`(attributePaths = {"formApplication",
"formEmailSendHistory"})) or replace the method with an explicit `@Query` that
uses a fetch join to eagerly load formEmailSendHistory along with
formApplication so EmailSendStatusInfoQuery.from() can safely read both
relations.
In
`@src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java`:
- Around line 249-261: The method getEmailSendStatusByFormId is missing
ownership validation; update it to accept the calling club (or current club) and
perform validateEqualsClub(club, form) before fetching histories: retrieve the
Form entity (e.g., via formService.getById(formId)), call
validateEqualsClub(club, form), then proceed with the existing logic
(formEmailSendHistoryService.getAllByFormId, etc.). Also update the
FacadeCentralFormService interface and the Controller method signature to
include the club parameter (or otherwise provide the caller's Club) so the
ownership check can be enforced.
In
`@src/main/java/ddingdong/ddingdongBE/email/repository/EmailSendHistoryRepository.java`:
- Around line 17-18: Rename or replace the invalid Spring Data JPA-derived
method findAllFetchedByFormEmailSendHistoryIdIn in EmailSendHistoryRepository:
either rename it to findAllByFormEmailSendHistoryIdIn to follow JPA naming
conventions, or keep the current name but annotate the method with an explicit
`@Query` that defines the intended JPQL/SQL; also update any callers (e.g.,
EmailSendHistoryService using the method) to match the new method name if you
choose renaming.
♻️ Duplicate comments (1)
src/main/java/ddingdong/ddingdongBE/domain/form/controller/CentralFormController.java (1)
130-134: 폼 소유자 검증 필요.
formId만 전달되므로 서비스에서 소유권 검증이 없다면 IDOR 위험이 있습니다.
🧹 Nitpick comments (6)
src/main/java/ddingdong/ddingdongBE/email/repository/EmailSendHistoryRepository.java (1)
3-9: 사용되지 않는 importLine 9의
@Queryimport가 현재 코드에서 사용되지 않습니다. 위의 메서드 명명 문제를@Query로 해결하지 않는 경우 제거를 고려하세요.src/main/java/ddingdong/ddingdongBE/domain/form/repository/FormEmailSendHistoryRepository.java (2)
4-7: 사용되지 않는 import 제거 권장
FormApplication(Line 4)과Optional(Line 7) import가 현재 인터페이스에서 사용되지 않습니다.♻️ 수정 제안
package ddingdong.ddingdongBE.domain.form.repository; import ddingdong.ddingdongBE.domain.form.entity.FormEmailSendHistory; -import ddingdong.ddingdongBE.domain.formapplication.entity.FormApplication; import ddingdong.ddingdongBE.domain.formapplication.entity.FormApplicationStatus; import java.util.List; -import java.util.Optional; import org.springframework.data.jpa.repository.JpaRepository;
12-13: Spring Data JPA 명명 규칙 검토
getAllByFormId는 Spring Data JPA 파생 쿼리 메서드로 정상 작동합니다. 다만, 더 관례적인 방식은findAllByFormId또는findByFormId입니다. 현재 코드베이스에서 여러 리포지토리(FormApplicationRepository.getAllByFormIdAndStatus,FormAnswerRepository.getAllFileByForm)가getAll접두사를 일관되게 사용하고 있으므로, 프로젝트 전체의 명명 규칙을 통일하려면 이를 고려하여 리팩토링하는 것이 좋습니다.src/main/java/ddingdong/ddingdongBE/email/service/EmailSendHistoryService.java (1)
75-79: 빈 리스트 입력 시 동작 확인 필요
formEmailSendHistoryIds가 빈 리스트인 경우, repository 쿼리가IN ()절로 변환되어 SQL 오류가 발생할 수 있습니다. 빈 리스트에 대한 early return 처리를 고려해 주세요.♻️ 빈 리스트 처리 제안
public EmailSendHistories getAllByFormEmailSendHistoryIds(List<Long> formEmailSendHistoryIds) { + if (formEmailSendHistoryIds.isEmpty()) { + return new EmailSendHistories(List.of()); + } List<EmailSendHistory> emailSendHistories = emailSendHistoryRepository.findAllFetchedByFormEmailSendHistoryIdIn( formEmailSendHistoryIds); return new EmailSendHistories(emailSendHistories); }src/main/java/ddingdong/ddingdongBE/domain/form/controller/dto/response/EmailSendStatusResponse.java (2)
24-35: 중첩 record의 접근 제한자 확인
EmailSendStatusInfoResponse가 package-private으로 선언되어 있습니다. API 응답으로 직렬화될 때 문제가 없는지 확인이 필요합니다. 다른 response DTO들과의 일관성을 위해public접근 제한자 추가를 고려해 주세요.♻️ public 접근 제한자 추가 제안
- record EmailSendStatusInfoResponse( + public record EmailSendStatusInfoResponse(
29-30: TODO 주석 처리 필요재전송 시
sendAt업데이트 관련 TODO가 남아있습니다. 이 기능이 이번 PR 범위에 포함되는지, 혹은 별도 이슈로 트래킹할지 확인해 주세요.이 TODO를 트래킹하기 위한 이슈를 생성해 드릴까요?
Comment on lines
+157
to
+165
| @Operation(summary = "이메일 전송 현황 조회 API") | ||
| @ApiResponse(responseCode = "200", description = "이메일 전송 현황 조회 성공", | ||
| content = @Content(schema = @Schema(implementation = EmailSendStatusResponse.class))) | ||
| @ResponseStatus(HttpStatus.OK) | ||
| @SecurityRequirement(name = "AccessToken") | ||
| @GetMapping("/my/forms/{formId}/emails/status") | ||
| EmailSendStatusResponse getEmailSendStatus( | ||
| @PathVariable("formId") Long formId | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the CentralFormApi file and check its location
find . -name "CentralFormApi.java" -type fRepository: COW-dev/ddingdong-be
Length of output: 137
🏁 Script executed:
# Search for getEmailSendStatus implementation and ownership validation
rg -n "getEmailSendStatus" -A 10 -B 2Repository: COW-dev/ddingdong-be
Length of output: 4047
🏁 Script executed:
# Search for form ownership/authorization validation patterns
rg -n "owner|userId|principalId|authorize|permission" --type java -C 3 | head -100Repository: COW-dev/ddingdong-be
Length of output: 10990
🏁 Script executed:
# Look for service implementation related to form
rg -n "FormService|FormServiceImpl" --type java | head -20Repository: COW-dev/ddingdong-be
Length of output: 3363
🏁 Script executed:
# Read the full implementation of getEmailSendStatusByFormId
sed -n '250,265p' src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.javaRepository: COW-dev/ddingdong-be
Length of output: 912
🏁 Script executed:
# Check the controller implementation to see if it receives userId/principal
cat -n src/main/java/ddingdong/ddingdongBE/domain/form/controller/CentralFormController.java | grep -A 10 "getEmailSendStatus"Repository: COW-dev/ddingdong-be
Length of output: 325
🏁 Script executed:
# Search for getAllByFormId to understand what it does
rg -n "getAllByFormId" -B 2 -A 8Repository: COW-dev/ddingdong-be
Length of output: 7661
🏁 Script executed:
# Search for Form ownership validation patterns
rg -n "getByFormIdAndUserId|validateOwnership|checkOwner|verifyOwner" --type javaRepository: COW-dev/ddingdong-be
Length of output: 46
🏁 Script executed:
# Check Form entity to see if it has userId/clubId field
fd "Form.java" --type f | xargs cat -n | head -50Repository: COW-dev/ddingdong-be
Length of output: 1928
🏁 Script executed:
# Check other /my/ endpoints to see if they validate ownership
rg -n "@GetMapping.*my/forms" -A 5 -B 2 src/main/java/ddingdong/ddingdongBE/domain/form/Repository: COW-dev/ddingdong-be
Length of output: 6785
🏁 Script executed:
# Check sendApplicationResultEmail which uses validateEqualsClub
sed -n '195,215p' src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.javaRepository: COW-dev/ddingdong-be
Length of output: 1209
🏁 Script executed:
# Look at Form entity club relationship and see if there's ownership validation elsewhere
rg -n "form.isNotEqualClubId|validateEqualsClub" -B 3 -A 3 src/main/java/ddingdong/ddingdongBE/domain/form/service/Repository: COW-dev/ddingdong-be
Length of output: 3292
폼 소유자 검증 누락으로 인한 IDOR 취약점 존재
경로가 /my/forms/{formId}인데 메서드 시그니처에 인증 사용자 정보가 없어 소유권 검증이 불가능합니다. 서비스 레이어에서도 ownership check가 없어서 다른 사용자가 formId만으로 임의의 폼의 이메일 전송 현황을 조회할 수 있습니다.
같은 패턴의 getFormStatistics() (라인 95-99)는 @AuthenticationPrincipal PrincipalDetails를 받아 userId를 기반으로 validateEqualsClub()을 통해 소유권을 검증합니다. 이 엔드포인트도 동일한 방식으로 수정해야 합니다.
✅ 수정 방안
`@GetMapping`("/my/forms/{formId}/emails/status")
EmailSendStatusResponse getEmailSendStatus(
- `@PathVariable`("formId") Long formId
+ `@PathVariable`("formId") Long formId,
+ `@AuthenticationPrincipal` PrincipalDetails principalDetails
);그 후 서비스 레이어에서:
public EmailSendStatusQuery getEmailSendStatusByFormId(Long formId, Long userId) {
Club club = clubService.getByUserId(userId);
Form form = formService.getById(formId);
validateEqualsClub(club, form); // 소유권 검증
// ... 기존 로직
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Operation(summary = "이메일 전송 현황 조회 API") | |
| @ApiResponse(responseCode = "200", description = "이메일 전송 현황 조회 성공", | |
| content = @Content(schema = @Schema(implementation = EmailSendStatusResponse.class))) | |
| @ResponseStatus(HttpStatus.OK) | |
| @SecurityRequirement(name = "AccessToken") | |
| @GetMapping("/my/forms/{formId}/emails/status") | |
| EmailSendStatusResponse getEmailSendStatus( | |
| @PathVariable("formId") Long formId | |
| ); | |
| `@Operation`(summary = "이메일 전송 현황 조회 API") | |
| `@ApiResponse`(responseCode = "200", description = "이메일 전송 현황 조회 성공", | |
| content = `@Content`(schema = `@Schema`(implementation = EmailSendStatusResponse.class))) | |
| `@ResponseStatus`(HttpStatus.OK) | |
| `@SecurityRequirement`(name = "AccessToken") | |
| `@GetMapping`("/my/forms/{formId}/emails/status") | |
| EmailSendStatusResponse getEmailSendStatus( | |
| `@PathVariable`("formId") Long formId, | |
| `@AuthenticationPrincipal` PrincipalDetails principalDetails | |
| ); |
🤖 Prompt for AI Agents
In `@src/main/java/ddingdong/ddingdongBE/domain/form/api/CentralFormApi.java`
around lines 157 - 165, The API getEmailSendStatus currently lacks owner
verification causing an IDOR; update the controller method signature
getEmailSendStatus to accept the authenticated principal (e.g.,
`@AuthenticationPrincipal` PrincipalDetails principal) and pass principal.getId()
(or equivalent) into the service, then implement ownership validation in the
service (create/modify getEmailSendStatusByFormId(Long formId, Long userId)) by
calling clubService.getByUserId(userId), formService.getById(formId) and
validateEqualsClub(club, form) before returning the EmailSendStatusResponse.
src/main/java/ddingdong/ddingdongBE/domain/form/service/dto/query/EmailSendStatusQuery.java
Show resolved
Hide resolved
Comment on lines
+249
to
+261
| @Override | ||
| public EmailSendStatusQuery getEmailSendStatusByFormId(Long formId) { | ||
| List<FormEmailSendHistory> formEmailSendHistories = formEmailSendHistoryService.getAllByFormId( | ||
| formId); | ||
| List<Long> formEmailSendHistoryIds = formEmailSendHistories.stream() | ||
| .map(FormEmailSendHistory::getId) | ||
| .toList(); | ||
| EmailSendHistories emailSendHistories = emailSendHistoryService.getAllByFormEmailSendHistoryIds( | ||
| formEmailSendHistoryIds); | ||
| EmailSendHistories latestEmailSendHistories = emailSendHistories.getLatestByFormApplication(); | ||
|
|
||
| return EmailSendStatusQuery.from(latestEmailSendHistories.getAll()); | ||
| } |
There was a problem hiding this comment.
Form 소유권 검증 누락 확인 필요
다른 메서드들(getStatisticsByForm, deleteForm 등)에서는 validateEqualsClub(club, form)을 통해 현재 사용자가 해당 Form의 소유자인지 검증하고 있습니다. 이 메서드에서는 해당 검증이 누락되어 있어, 다른 동아리의 Form에 대한 이메일 전송 현황을 조회할 수 있는 보안 문제가 발생할 수 있습니다.
🔒 소유권 검증 추가 제안
`@Override`
-public EmailSendStatusQuery getEmailSendStatusByFormId(Long formId) {
+public EmailSendStatusQuery getEmailSendStatusByFormId(Long formId, User user) {
+ Club club = clubService.getByUserId(user.getId());
+ Form form = formService.getById(formId);
+ validateEqualsClub(club, form);
+
List<FormEmailSendHistory> formEmailSendHistories = formEmailSendHistoryService.getAllByFormId(
formId);참고: 이 변경을 적용할 경우, FacadeCentralFormService 인터페이스와 Controller의 메서드 시그니처도 함께 수정해야 합니다.
🤖 Prompt for AI Agents
In
`@src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java`
around lines 249 - 261, The method getEmailSendStatusByFormId is missing
ownership validation; update it to accept the calling club (or current club) and
perform validateEqualsClub(club, form) before fetching histories: retrieve the
Form entity (e.g., via formService.getById(formId)), call
validateEqualsClub(club, form), then proceed with the existing logic
(formEmailSendHistoryService.getAllByFormId, etc.). Also update the
FacadeCentralFormService interface and the Controller method signature to
include the club parameter (or otherwise provide the caller's Club) so the
ownership check can be enforced.
src/main/java/ddingdong/ddingdongBE/email/repository/EmailSendHistoryRepository.java
Outdated
Show resolved
Hide resolved
KoSeonJe
approved these changes
Jan 19, 2026
Collaborator
KoSeonJe
left a comment
KoSeonJe
left a comment
There was a problem hiding this comment.
고생하셨습니다~ 페어프로그래밍으로 리뷰는 생략할게요
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
🚀 작업 내용
이메일 전송 현황 페이지에서, 하나의 Form에 지원서를 제출한 지원자들에게 보낸 이메일의 전송 상태를 반환하는 API를 추가했습니다.
🤔 고민했던 내용
💬 리뷰 중점사항
Summary by CodeRabbit
새로운 기능
개선사항
테스트
CI
✏️ Tip: You can customize this high-level summary in your review settings.