[URECA-75] Feat: 북마크 토글 API 구현 - #49
Conversation
📝 WalkthroughWalkthrough컨트롤러에 Swagger Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/ureca/unity/domain/summary/controller/SummaryController.java (1)
24-45:⚠️ 인가(Authorization) 검증 누락현재 구현에서 보안 취약점이 있습니다:
getMySummaries와getBookmarkedSummaries는@RequestParam으로userId를 받아 다른 사용자의 요약 목록을 조회할 수 있습니다.getSummaryDetail과toggleBookmark는summaryId만으로 접근하여, 다른 사용자의 요약을 조회/수정할 수 있습니다.
UserController.withdrawal(관련 코드 스니펫 참조)처럼Authentication에서 인증된 사용자 정보를 가져오는 것이 안전합니다.🔒 인가 검증 추가 예시
+import org.springframework.security.core.Authentication; // 전체 요약 목록 `@GetMapping` -public List<SummaryListResponse> getMySummaries(`@RequestParam` Long userId) { +public List<SummaryListResponse> getMySummaries(Authentication authentication) { + Long userId = (Long) authentication.getPrincipal(); return summaryService.getMySummaries(userId); } // 북마크 토글 (프론트가 PATCH로 호출 중) `@PatchMapping`("/{summaryId}/bookmark") -public void toggleBookmark(`@PathVariable` Long summaryId) { +public void toggleBookmark(`@PathVariable` Long summaryId, Authentication authentication) { + Long userId = (Long) authentication.getPrincipal(); - summaryService.toggleBookmark(summaryId); + summaryService.toggleBookmark(summaryId, userId); // Service에서 소유권 검증 필요 }Service 레이어에서도 해당 summary가 요청한 사용자의 것인지 검증하는 로직이 필요합니다.
🤖 Fix all issues with AI agents
In `@src/main/java/com/ureca/unity/domain/summary/service/SummaryService.java`:
- Around line 60-61: The line creating a new SummaryResponse without assigning
or returning it in SummaryService is dead code; remove the unused instantiation
"new SummaryResponse(gemini.getTitle(), gemini.getSubject(), keywords, points);"
(or if the intention was to use it, assign it to a variable or return it from
the method) so either drop the creation entirely or update the method to
return/consume the SummaryResponse instance.
🧹 Nitpick comments (3)
src/main/java/com/ureca/unity/domain/summary/service/SummaryService.java (2)
114-121: 예외 처리 일관성 개선 권장
toggleBookmark에서IllegalArgumentException을 사용하고 있지만, 같은 클래스의createSummary(line 31)에서는CustomException을 사용합니다. 프로젝트 전체에서 예외 처리 방식을 통일하면 클라이언트가 일관된 에러 응답을 받을 수 있습니다.또한
getSummaryDetail(line 86)은 summary가 없을 때null을 반환하는 반면,toggleBookmark은 예외를 던집니다. 동일한 "not found" 상황에 대해 일관된 처리 방식을 권장합니다.♻️ CustomException 사용 예시
`@Transactional` public void toggleBookmark(Long summaryId) { Boolean isBookmarked = summaryMapper.findBookmarkStatus(summaryId); if (isBookmarked == null) { - throw new IllegalArgumentException("summary not found"); + throw new CustomException(ErrorCode.SUMMARY_NOT_FOUND); // ErrorCode에 추가 필요 } summaryMapper.updateBookmark(summaryId, !isBookmarked); }
84-86: null 반환 대신 예외 처리 고려
getSummaryDetail에서 summary가 없을 때null을 반환하면, 컨트롤러에서 HTTP 200 OK와 함께 빈 응답이 전달됩니다. RESTful API 설계상 존재하지 않는 리소스에 대해서는 404 응답이 더 적절합니다.♻️ 404 응답 처리 예시
`@Transactional`(readOnly = true) public SummaryDetailResponse getSummaryDetail(Long summaryId) { SummaryModel summary = summaryMapper.findById(summaryId); - if (summary == null) return null; + if (summary == null) { + throw new CustomException(ErrorCode.SUMMARY_NOT_FOUND); + }src/main/java/com/ureca/unity/domain/summary/controller/SummaryController.java (1)
41-45: HTTP 응답 상태 코드 명시 권장
toggleBookmark은 응답 본문 없이 상태만 변경합니다.UserController.withdrawal처럼@ResponseStatus(HttpStatus.NO_CONTENT)를 명시하면 API 의도가 더 명확해집니다.♻️ 응답 상태 명시
+import org.springframework.http.HttpStatus; // 북마크 토글 (프론트가 PATCH로 호출 중) `@PatchMapping`("/{summaryId}/bookmark") +@ResponseStatus(HttpStatus.NO_CONTENT) public void toggleBookmark(`@PathVariable` Long summaryId) { summaryService.toggleBookmark(summaryId); }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/java/com/ureca/unity/global/util/CookieUtils.java`:
- Around line 38-45: The deleteRefreshTokenCookie method should defensively
validate and normalize its path and sameSite inputs: in Cookie
deleteRefreshTokenCookie(boolean secure, String path, String sameSite) ensure
path is non-null and non-empty (default to "/") and normalize/whitelist sameSite
to allowed values ("Lax","Strict","None") by trimming and case-normalizing; if
sameSite is invalid, fall back to "Lax" (or a chosen safe default), and when
sameSite equals "None" enforce/verify secure==true (or override sameSite to
"Lax" if secure is false) so browsers won’t ignore the cookie; also avoid
calling setAttribute with a null value.
🧹 Nitpick comments (2)
src/main/java/com/ureca/unity/domain/auth/controller/LogoutController.java (1)
34-41: 레거시 쿠키 삭제 로직은 반복을 줄일 수 있어요.
동일 패턴이 다른 컨트롤러에도 있으니, SameSite 목록을 루프 처리하거나 공통 헬퍼로 추출하면 유지보수가 쉬워집니다.♻️ 예시 리팩터
-// 2. 레거시 쿠키(Path=/api/auth)도 삭제 (과거 잔재 청소) -response.addCookie(CookieUtils.deleteRefreshTokenCookie(cookieSecure, "/api/auth", "Lax")); -response.addCookie(CookieUtils.deleteRefreshTokenCookie(cookieSecure, "/api/auth", "Strict")); +// 2. 레거시 쿠키(Path=/api/auth)도 삭제 (과거 잔재 청소) +for (String sameSite : new String[] {"Lax", "Strict"}) { + response.addCookie( + CookieUtils.deleteRefreshTokenCookie(cookieSecure, "/api/auth", sameSite) + ); +}src/main/java/com/ureca/unity/domain/auth/controller/OAuthController.java (1)
42-45: 레거시 쿠키 삭제 로직을 공통화해 중복을 줄이는 게 좋아요.
LogoutController와 동일 패턴이므로 헬퍼(예:CookieUtils.deleteLegacyRefreshTokenCookies)나 루프 처리로 통일하면 향후 변경에 강해집니다.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/ureca/unity/global/util/CookieUtils.java (1)
11-35: CSRF 비활성화 상태에서 SameSite=Lax로 변경한 보안 영향을 재검토하세요.
SecurityConfig에서 CSRF 보호가 명시적으로 비활성화(csrf.disable())되어 있어, SameSite가 CSRF 방어의 유일한 방어선입니다. POST/api/auth/refresh엔드포인트는 상태 변경 작업이므로, SameSite=Lax 설정은 크로스사이트 POST 요청에서도 쿠키가 전송되게 하여 CSRF 위험을 증가시킵니다.추가로 다음을 권장합니다:
- 일관성 부족: 새로운 오버로드 메서드
deleteRefreshTokenCookie(boolean secure, String path, String sameSite)는 유연하지만,UserController.withdraw()등 일부 곳에서는 여전히 기본 메서드(SameSite 하드코딩)를 사용합니다. 모든 쿠키 삭제 지점에서 새 오버로드를 사용하도록 통일하세요.- 위협 모델 문서화: Lax로 변경한 이유(OAuth 요구사항 등)를 코드 주석이나 아키텍처 문서에 명시하세요. 현재는 레거시 정리 주석에만 언급되어 있습니다.
- Strict 검토: OAuth 흐름이 Lax를 반드시 요구하지 않는다면, 기본값을 Strict로 유지하거나 환경 변수로 설정 가능하게 하세요.
3 participants
Key Changes
작업 내역
💬 공유사항 to 리뷰어
비고
Summary by CodeRabbit
New Features
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.