fix: RequiredArgsConstructor 동작하도록 임시 수정 #410
Merged
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
Gyuhyeok99
requested review from
lsy1307,
nayonsoso,
whqtker and
wibaek
as code owners
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20분
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (4)
src/main/java/com/example/solidconnection/security/aspect/RoleAuthorizationAspect.java (4)
28-28: TODO 주석 관련 제안"추후 개선 필요"라는 TODO가 있는데, 구체적으로 어떤 부분의 개선이 필요한지 명시하면 좋겠습니다.
예시:
- // todo: 추후 개선 필요 + // TODO: 리플렉션 사용 대신 Spring의 HandlerMethodArgumentResolver 활용하여 성능 개선 필요
38-39: 성능 최적화 고려사항매 요청마다 DB 조회가 발생하는 구조입니다. 다음과 같은 최적화를 고려해보세요:
- 캐싱 도입: 자주 접근하는 사용자 정보를 캐싱
- 세션 활용: 이미 인증된 사용자 정보를 세션에서 가져오기
@Cacheable(value = "users", key = "#siteUserId") public SiteUser findByIdWithCache(Long siteUserId) { return siteUserRepository.findById(siteUserId) .orElseThrow(() -> new CustomException(USER_NOT_FOUND)); }
54-58: 타입 체크 로직 개선 제안현재
Long객체와 primitivelong타입을 별도로 체크하고 있는데, 실제로는 auto-boxing으로 인해 중복된 체크입니다:- if (arg instanceof Long) { - return (Long) arg; - } else if (parameters[i].getType() == long.class) { - return (Long) arg; - } + if (arg instanceof Long || parameters[i].getType() == long.class) { + return (Long) arg; + }
65-65: 컬렉션 변환 최적화
Arrays.asList()는 매번 새로운 List를 생성합니다. Stream API나 Set을 사용하면 더 효율적입니다:- boolean hasAccess = Arrays.asList(allowedRoles).contains(siteUser.getRole()); + boolean hasAccess = Arrays.stream(allowedRoles).anyMatch(role -> role == siteUser.getRole());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/example/solidconnection/security/aspect/RoleAuthorizationAspect.java(1 hunks)src/test/java/com/example/solidconnection/security/aspect/RoleAuthorizationAspectTest.java(5 hunks)
🔇 Additional comments (2)
src/test/java/com/example/solidconnection/security/aspect/RoleAuthorizationAspectTest.java (2)
8-8: 테스트에 필요한 import 추가 확인
@AuthorizedUser어노테이션 사용을 위한 import가 적절히 추가되었습니다.
88-99: TestService 메소드 시그니처 변경 확인변경 사항:
- 파라미터 타입 변경:
SiteUser→@AuthorizedUser long siteUserId- 일관된 네이밍: 모든 메소드에서
siteUserId로 파라미터명 통일테스트 서비스 메소드들이 실제 구현과 동일한 방식으로 수정되어 적절합니다.
Comment on lines
24
to
+44
| public class RoleAuthorizationAspect { | ||
|
|
||
| // todo: 추후 siteUserId로 파라미터 변경 시 수정 필요 | ||
| private final SiteUserRepository siteUserRepository; | ||
|
|
||
| // todo: 추후 개선 필요 | ||
| @Around("@annotation(requireRoleAccess)") | ||
| public Object checkRoleAccess(ProceedingJoinPoint joinPoint, RequireRoleAccess requireRoleAccess) throws Throwable { | ||
| SiteUser siteUser = null; | ||
| for (Object arg : joinPoint.getArgs()) { | ||
| if (arg instanceof SiteUser) { | ||
| siteUser = (SiteUser) arg; | ||
| break; | ||
| } | ||
| } | ||
| if (siteUser == null) { | ||
|
|
||
| Long siteUserId = extractAuthorizedUserId(joinPoint); | ||
|
|
||
| if (siteUserId == null) { | ||
| throw new CustomException(ACCESS_DENIED); | ||
| } | ||
| Role[] allowedRoles = requireRoleAccess.roles(); | ||
|
|
||
| SiteUser siteUser = siteUserRepository.findById(siteUserId) | ||
| .orElseThrow(() -> new CustomException(USER_NOT_FOUND)); | ||
|
|
||
| validateUserRole(siteUser, requireRoleAccess.roles()); | ||
|
|
||
| return joinPoint.proceed(); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
아키텍처 및 보안 고려사항
현재 구현의 주요 변경사항과 고려사항:
-
보안 측면
- 사용자 ID 기반 인증으로 변경되어 ID 조작 가능성 검토 필요
@AuthorizedUser어노테이션이 붙은 파라미터가 실제로 인증된 사용자의 ID인지 검증 필요
-
성능 측면
- 리플렉션 사용으로 인한 오버헤드
- 매 요청마다 DB 조회 발생
-
개선 제안
- Spring Security의
@AuthenticationPrincipal과 유사한 방식으로HandlerMethodArgumentResolver활용 - 인증된 사용자 정보를 SecurityContext에서 직접 가져오는 방식 고려
- Spring Security의
이러한 점들이 PR 설명에서 언급된 "임시 수정"의 이유인 것으로 보입니다.
🏁 Script executed:
#!/bin/bash
# @AuthorizedUser 어노테이션의 실제 구현과 사용처 확인
echo "=== @AuthorizedUser 어노테이션 구현 확인 ==="
fd -e java "AuthorizedUser.java" --exec cat {}
echo -e "\n=== @AuthorizedUser 사용처 확인 ==="
rg "@AuthorizedUser" -A 3 -B 1Length of output: 33412
🏁 Script executed:
#!/bin/bash
# Extract implementation of extractAuthorizedUserId
echo "=== extractAuthorizedUserId 정의 위치 ==="
rg -n "extractAuthorizedUserId" -n src/main/java/com/example/solidconnection/security/aspect/RoleAuthorizationAspect.java
echo -e "\n=== extractAuthorizedUserId 메소드 전체 내용 ==="
ast-grep --pattern $'public class RoleAuthorizationAspect {
$$$
?checkRoleAccess($$$)
$$$
private Long extractAuthorizedUserId($_) {
$$$
}
}' -r src/main/java/com/example/solidconnection/security/aspect/RoleAuthorizationAspect.javaLength of output: 694
아키텍처 및 보안 고려사항
-
보안 측면
- 사용자 ID 기반 인증 방식으로 변경되어 ID 변조 공격 가능성이 존재합니다.
extractAuthorizedUserId가ProceedingJoinPoint.getArgs()로 인자를 읽어오기 때문에 무결성 검사를 반드시 강화해야 합니다.
-
성능 측면
- Aspect 실행 시마다
SiteUserRepository.findById로 DB 조회가 이루어져 응답 지연이 발생할 수 있습니다. - 리플렉션 기반 인자 추출 로직이 반복 호출될 때 오버헤드가 큽니다.
- Aspect 실행 시마다
-
아키텍처 측면
- 인가 로직이 Aspect와 데이터 계층까지 결합되어 결합도가 높습니다.
HandlerMethodArgumentResolver와SecurityContextHolder기반의 표준 Spring Security 패턴을 적용해 책임 분리를 검토하세요.
장기적으로는 위 세 가지를 개선하여 안정성·성능·가독성을 모두 높일 수 있습니다.
🤖 Prompt for AI Agents
In
src/main/java/com/example/solidconnection/security/aspect/RoleAuthorizationAspect.java
around lines 24 to 44, the current approach extracts user ID from method
arguments and queries the database on each aspect invocation, causing security
risks, performance overhead, and tight coupling. To fix this, refactor the
authorization logic to use Spring Security's SecurityContextHolder to obtain the
authenticated user securely, avoid direct database calls in the aspect by
delegating role validation to a dedicated service or component, and consider
using HandlerMethodArgumentResolver for argument resolution. This will improve
security by preventing ID tampering, reduce DB query overhead, and separate
concerns for better maintainability.
Collaborator
There was a problem hiding this comment.
Aspect 실행 시마다 SiteUserRepository.findById로 DB 조회가 이루어져 응답 지연이 발생할 수 있습니다.
흠~ 이 말은 일리가 있긴 하네요.
그런데 이 PR은 수습이 목표이고, 디스코드에 논의한대로 PreAuthorize 어노테이션을 사용하면
레포지토리가 아니라 security context 에서 사용자 정보를 가져오니 DB 조회가 없을것입니다~
There was a problem hiding this comment.
@nayonsoso 맞습니다! 임시 수습 목적이라면 현재 접근법이 합리적이네요.
말씀하신 대로 @PreAuthorize를 사용하면 SecurityContext에서 인증된 사용자 정보를 직접 가져와서 DB 조회 없이 권한 검사가 가능합니다. 이는 제가 앞서 제안한 SecurityContextHolder 활용 방안과 정확히 일치하는 방향입니다.
현재 구현 → @PreAuthorize 마이그레이션은 아키텍처적으로 올바른 선택입니다:
- 성능: DB 조회 제거
- 보안: Spring Security 표준 방식으로 ID 변조 위험 해결
- 유지보수성: 프레임워크 표준 패턴 활용
임시 수습으로서는 충분히 동작할 것 같습니다! 👍
🧠 Learnings used
Learnt from: whqtker
PR: solid-connection/solid-connect-server#408
File: src/main/java/com/example/solidconnection/chat/service/ChatService.java:46-54
Timestamp: 2025-07-28T03:14:33.849Z
Learning: 페치 조인을 사용한 N+1 문제 해결에 대해 사용자 whqtker는 JPQL 쿼리 접근법을 선호하며, SELECT 쿼리의 경우 영속성 컨텍스트 간 불일치 문제를 고려하지 않아도 된다고 인식하고 있습니다.
Merged
lsy1307
approved these changes
Jul 27, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
관련 이슈
작업 내용
채팅 기능 테스트를 위해 멘토링 api를 bruno에서 호출하는데 계속 접근권한이 없다고 호출이 안되고 있었습니다...
뭔가 하고 봤더니 SiteUser에서 long으로 받도록 수정이 되어서 RequiredArgsConstructor가 동작하지 않고있었습니다!
우선 급하게 동작하도록 수정해놨고 시간나면 다시 손좀 보겠습니다 ㅎㅎ..
특이 사항
리뷰 요구사항 (선택)