Skip to content

fix: RequiredArgsConstructor 동작하도록 임시 수정 #410

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Gyuhyeok99 merged 2 commits into from
Jul 29, 2025
Merged

fix: RequiredArgsConstructor 동작하도록 임시 수정 #410

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
50b9e51
fix: RequiredArgsConstructor 동작하도록 임시 수정
Gyuhyeok99 Jul 27, 2025
e2785f3
fix: ci 작동되게 수정
Gyuhyeok99 Jul 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 41 additions & 11 deletions src/main/java/com/example/solidconnection/security/aspect/RoleAuthorizationAspect.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,41 +1,71 @@
package com.example.solidconnection.security.aspect;

import static com.example.solidconnection.common.exception.ErrorCode.ACCESS_DENIED;
import static com.example.solidconnection.common.exception.ErrorCode.USER_NOT_FOUND;

import com.example.solidconnection.common.exception.CustomException;
import com.example.solidconnection.common.resolver.AuthorizedUser;
import com.example.solidconnection.security.annotation.RequireRoleAccess;
import com.example.solidconnection.siteuser.domain.Role;
import com.example.solidconnection.siteuser.domain.SiteUser;
import com.example.solidconnection.siteuser.repository.SiteUserRepository;
import java.lang.reflect.Parameter;
import java.util.Arrays;
import lombok.RequiredArgsConstructor;
import org.aspectj.lang.ProceedingJoinPoint;
import org.aspectj.lang.annotation.Around;
import org.aspectj.lang.annotation.Aspect;
import org.aspectj.lang.reflect.MethodSignature;
import org.springframework.stereotype.Component;

@Aspect
@Component
@RequiredArgsConstructor
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();
}
Comment on lines 24 to +44
Copy link

@coderabbitai coderabbitai bot Jul 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

아키텍처 및 보안 고려사항

현재 구현의 주요 변경사항과 고려사항:

  1. 보안 측면

    • 사용자 ID 기반 인증으로 변경되어 ID 조작 가능성 검토 필요
    • @AuthorizedUser 어노테이션이 붙은 파라미터가 실제로 인증된 사용자의 ID인지 검증 필요

  2. 성능 측면

    • 리플렉션 사용으로 인한 오버헤드
    • 매 요청마다 DB 조회 발생

  3. 개선 제안

    • Spring Security의 @AuthenticationPrincipal과 유사한 방식으로 HandlerMethodArgumentResolver 활용
    • 인증된 사용자 정보를 SecurityContext에서 직접 가져오는 방식 고려

이러한 점들이 PR 설명에서 언급된 "임시 수정"의 이유인 것으로 보입니다.

🏁 Script executed:

#!/bin/bash
# @AuthorizedUser 어노테이션의 실제 구현과 사용처 확인
echo "=== @AuthorizedUser 어노테이션 구현 확인 ==="
fd -e java "AuthorizedUser.java" --exec cat {}

echo -e "\n=== @AuthorizedUser 사용처 확인 ==="
rg "@AuthorizedUser" -A 3 -B 1

Length 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.java

Length of output: 694

아키텍처 및 보안 고려사항

  1. 보안 측면

    • 사용자 ID 기반 인증 방식으로 변경되어 ID 변조 공격 가능성이 존재합니다.
    • extractAuthorizedUserId가 ProceedingJoinPoint.getArgs()로 인자를 읽어오기 때문에 무결성 검사를 반드시 강화해야 합니다.

  2. 성능 측면

    • Aspect 실행 시마다 SiteUserRepository.findById로 DB 조회가 이루어져 응답 지연이 발생할 수 있습니다.
    • 리플렉션 기반 인자 추출 로직이 반복 호출될 때 오버헤드가 큽니다.

  3. 아키텍처 측면

    • 인가 로직이 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.

Copy link
Collaborator

@nayonsoso nayonsoso Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aspect 실행 시마다 SiteUserRepository.findById로 DB 조회가 이루어져 응답 지연이 발생할 수 있습니다.

흠~ 이 말은 일리가 있긴 하네요.
그런데 이 PR은 수습이 목표이고, 디스코드에 논의한대로 PreAuthorize 어노테이션을 사용하면
레포지토리가 아니라 security context 에서 사용자 정보를 가져오니 DB 조회가 없을것입니다~

Copy link

@coderabbitai coderabbitai bot Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nayonsoso 맞습니다! 임시 수습 목적이라면 현재 접근법이 합리적이네요.

말씀하신 대로 @PreAuthorize를 사용하면 SecurityContext에서 인증된 사용자 정보를 직접 가져와서 DB 조회 없이 권한 검사가 가능합니다. 이는 제가 앞서 제안한 SecurityContextHolder 활용 방안과 정확히 일치하는 방향입니다.

현재 구현 → @PreAuthorize 마이그레이션은 아키텍처적으로 올바른 선택입니다:

  1. 성능: DB 조회 제거
  2. 보안: Spring Security 표준 방식으로 ID 변조 위험 해결
  3. 유지보수성: 프레임워크 표준 패턴 활용

임시 수습으로서는 충분히 동작할 것 같습니다! 👍

🧠 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 쿼리의 경우 영속성 컨텍스트 간 불일치 문제를 고려하지 않아도 된다고 인식하고 있습니다.


private Long extractAuthorizedUserId(ProceedingJoinPoint joinPoint) {
MethodSignature signature = (MethodSignature) joinPoint.getSignature();
Parameter[] parameters = signature.getMethod().getParameters();
Object[] args = joinPoint.getArgs();

for (int i = 0; i < parameters.length; i++) {
if (parameters[i].isAnnotationPresent(AuthorizedUser.class)) {
Object arg = args[i];
if (arg instanceof Long) {
return (Long) arg;
} else if (parameters[i].getType() == long.class) {
return (Long) arg;
}
}
}
return null;
}

private void validateUserRole(SiteUser siteUser, Role[] allowedRoles) {
boolean hasAccess = Arrays.asList(allowedRoles).contains(siteUser.getRole());

if (!hasAccess) {
throw new CustomException(ACCESS_DENIED);
}
return joinPoint.proceed();
}
}
19 changes: 10 additions & 9 deletions src/test/java/com/example/solidconnection/security/aspect/RoleAuthorizationAspectTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import static org.junit.jupiter.api.Assertions.assertAll;

import com.example.solidconnection.common.exception.CustomException;
import com.example.solidconnection.common.resolver.AuthorizedUser;
import com.example.solidconnection.security.annotation.RequireRoleAccess;
import com.example.solidconnection.siteuser.domain.Role;
import com.example.solidconnection.siteuser.domain.SiteUser;
Expand Down Expand Up @@ -35,9 +36,9 @@ class RoleAuthorizationAspectTest {

// when & then
assertAll(
() -> assertThatCode(() -> testService.adminOnlyMethod(admin))
() -> assertThatCode(() -> testService.adminOnlyMethod(admin.getId()))
.doesNotThrowAnyException(),
() -> assertThatCode(() -> testService.mentorOrAdminMethod(mentor))
() -> assertThatCode(() -> testService.mentorOrAdminMethod(mentor.getId()))
.doesNotThrowAnyException()
);
}
Expand All @@ -48,7 +49,7 @@ class RoleAuthorizationAspectTest {
SiteUser user = siteUserFixture.사용자();

// when & then
assertThatCode(() -> testService.mentorOrAdminMethod(user))
assertThatCode(() -> testService.mentorOrAdminMethod(user.getId()))
.isInstanceOf(CustomException.class)
.hasMessage(ACCESS_DENIED.getMessage());
}
Expand All @@ -62,11 +63,11 @@ class RoleAuthorizationAspectTest {

// when & then
assertAll(
() -> assertThatCode(() -> testService.publicMethod(admin))
() -> assertThatCode(() -> testService.publicMethod(admin.getId()))
.doesNotThrowAnyException(),
() -> assertThatCode(() -> testService.publicMethod(mentor))
() -> assertThatCode(() -> testService.publicMethod(mentor.getId()))
.doesNotThrowAnyException(),
() -> assertThatCode(() -> testService.publicMethod(user))
() -> assertThatCode(() -> testService.publicMethod(user.getId()))
.doesNotThrowAnyException()
);
}
Expand All @@ -84,16 +85,16 @@ public TestService testService() {
static class TestService {

@RequireRoleAccess(roles = {Role.ADMIN})
public boolean adminOnlyMethod(SiteUser siteUser) {
public boolean adminOnlyMethod(@AuthorizedUser long siteUserId) {
return true;
}

@RequireRoleAccess(roles = {Role.ADMIN, Role.MENTOR})
public boolean mentorOrAdminMethod(SiteUser siteUser) {
public boolean mentorOrAdminMethod(@AuthorizedUser long siteUserId) {
return true;
}

public boolean publicMethod(SiteUser siteUser) {
public boolean publicMethod(@AuthorizedUser long siteUserId) {
return true;
}
}
Expand Down
Loading