Conversation
|
|
||
| if (!diary.getAuthor().getId().equals(user.getId())) { | ||
| throw new org.springframework.security.access.AccessDeniedException("본인이 작성한 일기만 삭제할 수 있습니다."); | ||
| } |
There was a problem hiding this comment.
앞에 패키지명을 제거하고 import하면 좋을 거 같아요!
| log.info("Group created with id: {}", group.getId()); | ||
| return new GroupResponseDto(group.getId(), group.getName(), code, MemberStatus.ACCEPTED); | ||
| return new GroupResponseDto(group.getId(), group.getName(), code, MemberStatus.ACCEPTED, 1, null); | ||
| } |
There was a problem hiding this comment.
GroupResponseDto 내에 신규 그룹에 의한 DTO를 반환하는 정적 팩토리 메서드를 만들어도 좋을 것 같아요!
ex. GroupResponseDto.fromNewGroup()
Summary of ChangesHello, 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! This pull request introduces significant enhancements to diary and group management. It provides users with the ability to delete their own diary entries, enriches the information displayed for individual diaries and groups, and refines the sorting of group lists for a more intuitive user experience. The changes also include underlying repository and service layer updates to support these new features and improve data handling. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe 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 by creating a comment using either
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 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces functionality to delete diaries and enriches group information with member count and last diary timestamp. A security audit identified two medium-severity broken access control issues in the diary deletion logic, specifically the lack of a group membership check and the failure to validate the group context provided in the URL. Additionally, the review noted an unused path variable, a significant N+1 performance issue when fetching group information, and a suggestion to simplify complex sorting logic. The changes otherwise look good, including the addition of S3 deletion and ordering of notifications.
| List<GroupResponseDto> dtos = groupMembers.stream() | ||
| .map(groupMember -> { | ||
| Group group = groupMember.getGroup(); | ||
| return new GroupResponseDto(group.getId(), group.getName(), group.getCode(), groupMember.getStatus()); | ||
| int memberCount = (int) group.getGroupMembers().stream() | ||
| .filter(gm -> gm.getStatus() == MemberStatus.ACCEPTED) | ||
| .count(); | ||
| LocalDateTime lastDiaryAt = diaryRepository.findTop1ByGroupOrderByCreatedAtDesc(group) | ||
| .map(Diary::getCreatedAt) | ||
| .orElse(null); | ||
| return new GroupResponseDto(group.getId(), group.getName(), group.getCode(), groupMember.getStatus(), memberCount, lastDiaryAt); | ||
| }) | ||
| .toList(); |
There was a problem hiding this comment.
This block of code introduces a significant performance issue known as the N+1 query problem. For each group the user is a member of, two additional database queries are executed inside the map operation:
group.getGroupMembers().stream()...count(): This triggers a lazy-loading query to fetch all members of a group just to count them.diaryRepository.findTop1ByGroupOrderByCreatedAtDesc(group): This executes another query to find the latest diary for the group.
If a user is in N groups, this will result in 2*N extra queries, which can severely degrade performance.
To optimize this, you should fetch the required information in batches. Here's a suggested approach:
- Collect all
groupIds from the initialgroupMemberslist. - Execute a single query to get the count of accepted members for all those groups at once. You can use a
GROUP BYclause. - Execute another single query to get the latest diary's creation date for all those groups, again using
GROUP BY. - Store these results in
Maps (e.g.,Map<Long, Integer>for member counts,Map<Long, LocalDateTime>for last diary dates). - Use these maps within your
mapstream to construct theGroupResponseDtowithout hitting the database in a loop.
| @PathVariable Long groupId, | ||
| @PathVariable Long diaryId, | ||
| @AuthenticationPrincipal CustomUserDetails userDetails | ||
| ) { | ||
| log.info("[DIARY-DELETE] Request user: {}, diary: {}", userDetails.getUsername(), diaryId); | ||
| diaryService.deleteDiary(diaryId, userDetails.getUsername()); |
There was a problem hiding this comment.
The deleteDiary endpoint is mapped under a path that includes {groupId}, but the implementation ignores this parameter and does not validate that the diary being deleted actually belongs to the specified group. This creates a broken access control vulnerability, allowing a user to delete their diary by providing an arbitrary or incorrect group ID in the URL, bypassing intended hierarchical access control. For data integrity and security, it's crucial to pass the groupId to the service method and add the corresponding validation logic there.
diaryService.deleteDiary(groupId, diaryId, userDetails.getUsername());| if (!diary.getAuthor().getId().equals(user.getId())) { | ||
| throw new org.springframework.security.access.AccessDeniedException("본인이 작성한 일기만 삭제할 수 있습니다."); | ||
| } |
There was a problem hiding this comment.
The deleteDiary method only verifies that the requesting user is the author of the diary. It fails to check if the user is still an active member of the group to which the diary belongs. This is inconsistent with the getDiaryDetail method, which correctly validates group membership. As a result, a user who has been removed from a group can still delete their previous diaries within that group, even though they can no longer view them.
| if (!diary.getAuthor().getId().equals(user.getId())) { | |
| throw new org.springframework.security.access.AccessDeniedException("본인이 작성한 일기만 삭제할 수 있습니다."); | |
| } | |
| Diary diary = diaryRepository.getOrThrow(diaryId); | |
| User user = userRepository.getByEmailOrThrow(email); | |
| groupMemberService.validateAcceptedMember(user, diary.getGroup()); | |
| if (!diary.getAuthor().getId().equals(user.getId())) { | |
| throw new org.springframework.security.access.AccessDeniedException("본인이 작성한 일기만 삭제할 수 있습니다."); | |
| } |
| .thenComparing(dto -> { | ||
| if (dto.status() == MemberStatus.ACCEPTED) { | ||
| LocalDateTime lastDiaryAt = dto.lastDiaryAt(); | ||
| return lastDiaryAt != null ? lastDiaryAt : LocalDateTime.MIN; | ||
| } | ||
| return joinedAtByGroupId.getOrDefault(dto.id(), LocalDateTime.MIN); | ||
| }, Comparator.reverseOrder())) |
There was a problem hiding this comment.
The lambda function used for the secondary sorting criterion is quite complex, with conditional logic that makes it hard to read. To improve readability and maintainability, this can be simplified.
Instead of manually handling nulls by returning LocalDateTime.MIN, you can use Comparator.nullsLast(Comparator.reverseOrder()) as the key comparator.
The key extractor can then be simplified to: dto -> dto.status() == MemberStatus.ACCEPTED ? dto.lastDiaryAt() : joinedAtByGroupId.get(dto.id()).
This makes the intent of sorting null lastDiaryAt values last more explicit and the code more concise.
2 participants
No description provided.