Skip to content

Conversation

@nonactress
Copy link

@nonactress nonactress commented Dec 26, 2025

준수님 리뷰어로 다시 만나게 되어 반갑습니다!! 👍
이번 크리스마스는 잘 보내셨을까요??? 특히 지금은 연말 일정도 바쁘실 텐데 리뷰해주셔서 감사합니다!

미션 해결 방식

1단계 미션 : 로그인 기능 + Cookie 기반 사용자 조회 API

  1. /login API에서 이메일·비밀번호 인증 후 JWT 토큰 생성

  2. 이후 요청에서는 Cookie에 담긴 token을 기반으로 사용자 인증 수행

  3. /login/check API에서 Cookie → 토큰 → Member 조회 방식으로 로그인 사용자 정보 반환

2단계 미션 : 로그인/예약 생성 로직 리팩터링

2-1.로그인 리팩터링

  1. @AuthMember 커스텀 애노테이션 생성

  2. AuthenticationPrincipalArgumentResolver 구현

  3. 요청의 Cookie에서 token 추출

  4. JWT 검증 후 Member 조회

  5. 컨트롤러에서는 인증 로직 제거하고 Member 객체를 바로 주입받도록 개선

2-2 예약 생성 기능 변경

  1. 예약 생성 로직에서 분기 처리

    2-A. ReservationRequest.name != null → name으로 Member 조회

    2-B. ReservationRequest.name == null→ @AuthMember로 주입된 로그인 Member 사용

  2. 예약 생성 책임을 Service 레이어로 이동하여 중복 로직 제거

3단계 미션 : 관리자(admin) 페이지 접근 제한

  1. AdminInterceptor 구현 (Cookie에서 token 추출, JWT 유효성 검증, email로 Member 조회, role이 ADMIN인지 확인)

  2. /admin/** 경로에 인터셉터 적용

  3. 권한 없을 경우 401 Unauthorized 응답

궁금한점

  1. 인증 vs 인가
    AdminInterceptor에서 JWT 검증과 role 체크를 같이 하는데 이 둘을 분리할 필요는 없을까요?

  2. JWT 검증
    현재 JWT 검증 로직이 Interceptor와 ArgumentResolver 양쪽에서 사용되고 있는데,
    인증 흐름을 하나로 모으는 것이 더 나은지, 아니면 지금처럼 역할별로 나누는 것이 괜찮은 것인지 궁금합니다.

@nonactress nonactress changed the base branch from main to nonactress December 26, 2025 16:31
Copy link

@gogo1414 gogo1414 left a comment

Choose a reason for hiding this comment

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

코드 리뷰 피드백

안녕하세요! 현진님

벌써 2025년이 끝나가고 새해가 찾아왔네요. 새해 복 많이 받으시고 새로운 해에는 되시는 일 모두 잘되시길 빌게요!


폴더 구조

우선, 인증 인가에 대한 내용 전에 폴더 구조에 대해서 한번 짚고 가려고 합니다!

미션을 진행하시다 보면서 어떤 구조로 진행했을 경우 조금 더 가독성이 좋고 관리하기 쉬운지 느끼셨을 것 같아요.

폴더를 정리하는 구조에는 크게 계층형 분리 방식과 도메인 기준 분리 방식 2가지가 있습니다.

# 계층형 (Layer-based)
├── controller/
│   ├── MemberController
│   └── ReservationController
├── service/
│   ├── MemberService
│   └── ReservationService
├── repository/

# 도메인형 (Domain-based)
├── member/
│   ├── MemberController
│   ├── MemberService
│   └── MemberRepository
├── reservation/
│   ├── ReservationController
│   ├── ReservationService
│   └── ReservationRepository

두 방식 모두 팀 혹은 개인의 코드 취향에 맞게 내부에 request, response, entity 등의 폴더를 추가로 만들어 관리할 수 있어요.

현진님은 도메인을 기준으로 폴더를 만드셨고, 내부에 따로 폴더를 만들어서 관리하고 계시진 않아요. 지금처럼 파일 수가 많지 않고 도메인 수도 적은 프로젝트라면 쉽게 찾을 수 있지만, 규모가 조금만 더 커지면 원하는 파일을 찾는 게 어려워지실 거예요.

지금과 같이 도메인 형태의 폴더 구조를 유지하면서, 내부에 각 목적에 맞는 폴더를 만들어 구조를 체계화해 보시는 건 어떨까요?

제가 모든 걸 알려드리기보다는, 같은 미션을 진행하는 다른 팀원분들의 코드를 확인하고 비교해보면서 본인에게 맞는 스타일을 찾아보셨으면 좋겠어요!


궁금한 점에 대한 답변

1. 인증 vs 인가

AdminInterceptor라는 네이밍만 보자면 지금처럼 JWT 검증과 동시에 role 체크를 진행하는 게 맞다고 생각해요.

다만, 회원가입을 한 유저에 대한 인증과 role을 체크하는 인가는 분리하면 좋습니다. 만약 회원 중 어드민, 일반 회원 외에도 여러 개의 권한 분류 체계가 생기게 되면 분리가 필요한 시점이 올 거예요.

저도 최근 회사에서 권한에 따라 사용 가능한 기능을 따로 적용해주기 위해 인증과 인가를 분리해서 진행했어요!

2. JWT 검증 로직 중복

우선 Interceptor와 ArgumentResolver의 동작 흐름 차이를 알고 계시나요?

HandlerInterceptor

  • 목적: 요청 처리 전/후에 공통 로직 실행
  • 흐름: HTTP 요청 → [preHandle()] → Controller → [postHandle()] → HTTP 응답
  • 특징:
    • 요청이 컨트롤러에 도달하기 전에 실행
    • true 반환 → 진행, false 반환 → 차단
    • URL 패턴 기반으로 적용 (예: /admin/**)
  • 주 용도: 인증/인가, 로깅, 공통 검증

HandlerMethodArgumentResolver

  • 목적: 컨트롤러 메서드의 파라미터에 값을 주입
  • 특징:
    • 컨트롤러 실행 직전에 파라미터별로 동작
    • 어노테이션/타입 기반으로 적용 (예: @AuthMember)
  • 주 용도: 로그인 사용자 정보 주입
@GetMapping("/reservations")
public List<Reservation> getMyReservations(@AuthMember Member member) {
    // ArgumentResolver가 Member 객체를 만들어서 주입
}

사실 둘은 역할이 다르기 때문에 분리하는 게 맞아요.

  • Interceptor → "이 경로에 접근할 수 있는가?" (접근 제어)
  • ArgumentResolver → "인증된 사용자 정보를 컨트롤러에 전달" (데이터 주입)

하지만 현재 코드를 보면 둘 다 JWT 토큰 추출과 검증을 각각 수행하고 있어요. 이 공통 로직은 별도로 분리하는 게 좋습니다.

예를 들어, JWT 검증과 Member 조회를 별도 서비스로 분리하면:

@Service
public class AuthService {
    public Member getMemberFromRequest(HttpServletRequest request) {
        String token = extractToken(request);
        validateToken(token);
        return findMemberByToken(token);
    }
}

이렇게 하면:

  • Interceptor: authService.getMemberFromRequest() 호출 → role 체크만 담당
  • ArgumentResolver: authService.getMemberFromRequest() 호출 → 주입만 담당

DRY 원칙도 지키고, 각자의 역할도 명확해져요!

고생 많으셨고, 다시 한번 새해 복 많이 받으세요!! 😄

.findFirst()
.orElse(null);
}
} No newline at end of file

Choose a reason for hiding this comment

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

여기 EOL이 생겼네요! 확인 부탁드려요

Copy link
Author

@nonactress nonactress Jan 2, 2026

Choose a reason for hiding this comment

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

넵 수정하겠습니다.

String email = jwtTokenProvider.getPayload(token);
Member member = memberDao.findByEmail(email);

if (member == null || !"ADMIN".equals(member.getRole())) {

Choose a reason for hiding this comment

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

"ADMIN" 값을 하드코딩해서 비교하는 것보다 다른 좋은 방법이 있지 않을까요?

예를 들어, role을 Enum으로 관리한다면 해당 Enum을 이용해 비교하는 방법이 존재할 것 같은데 어떻게 생각하시나요?

Copy link
Author

@nonactress nonactress Jan 2, 2026

Choose a reason for hiding this comment

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

와우 Enum을 적용해 보는 것이 훨씬 좋은 방법인 것 같습니다!!
바로 적용해보겠습니다!

적용 커밋 : ff9cf27

registry.addInterceptor(adminInterceptor)
.addPathPatterns("/admin/**");
}
}

Choose a reason for hiding this comment

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

여기도 EOL이 발생했네요!

Copy link
Author

Choose a reason for hiding this comment

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

수정하겠습니다!

반영 커밋 : a7e53ff

public class JwtTokenProvider {
@Value("${security.jwt.token.secret-key:this-is-a-sample-secret-key-at-least-32-bytes-long}")
private String secretKey;
@Value("${security.jwt.token.expire-length:3600000}")

Choose a reason for hiding this comment

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

3600000이 명시되어 있는데, 뭔가 숨긴 의미가 없어진건 아닐까요..? ㅎㅎㅎㅎㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

그러네여ㅎㅎㅎ 다시 숨겨보도록 하겠습니다!

반영 커밋 : 4035c63

Comment on lines 114 to 119






Choose a reason for hiding this comment

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

여기는 일부러 남기신걸까요??

Copy link
Author

@nonactress nonactress Jan 2, 2026

Choose a reason for hiding this comment

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

다음 단계 테스트코드를 작성하려고 여백을 남겨 놓은 것 같습니다. 개행 정리 해보겠습니다!!

반영 커밋 : 9900da9

Copy link

@gogo1414 gogo1414 Dec 31, 2025

Choose a reason for hiding this comment

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

application.propertiesJWT secret key가 하드코딩되어 있는데, 이 파일이 저장소에 그대로 올라가고 있어요.

민감한 정보(secret key, DB 비밀번호 등)는 환경변수나 별도 설정 파일로 분리하고, .gitignore에 등록해서 저장소에 올라가지 않도록 하는 것이 좋습니다.

한번 커밋되면 히스토리에 남아서 제거하기 어려우니, 초기 설정 시 주의해주세요!

Copy link
Author

@nonactress nonactress Jan 2, 2026

Choose a reason for hiding this comment

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

비밀번호를 다 알려주고 있었네요.... 😮
gitignore를 사용해서 secretKey를 다시 생성해보았습니다!!

반영 커밋 : 94f81b1

Copy link

Choose a reason for hiding this comment

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

아직 Github 내에서 해당 파일 확인이 가능해요. 다시 한번 확인 부탁드려요!

Copy link
Author

Choose a reason for hiding this comment

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

아 반영했습니다!
반영 커밋 : d3209bc

Choose a reason for hiding this comment

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

같은 파일 내에서 상수(SC_UNAUTHORIZED)와 숫자(401)를 혼용해서 사용하셨는데, 어떤 방식이 더 좋을까요?

Copy link
Author

Choose a reason for hiding this comment

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

상수로 하는 것이 가독성이 더 좋은 것 같아 보입니다. 상수로 통일 해보겠습니다!!!

String token = extractToken(request);

if (token == null || !jwtTokenProvider.validateToken(token)) {
response.setStatus(HttpServletResponse.SC_UNAUTHORIZED);

Choose a reason for hiding this comment

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

인증 실패와 권한 없음 모두 401로 응답하고 있는데, 클라이언트 입장에서 구분이 가능할까요? 401과 403의 차이를 알고 계신가요??

Copy link
Author

Choose a reason for hiding this comment

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

인증 실패 => 401
권한 없음 => 403

401과 403의 차이

  • 401(HttpServletResponse.SC_UNAUTHORIZED)은 인증 실패 시에 사용하는 http 상태 코드입니다.
  • 403(HttpServletResponse.SC_FORBIDDEN)은 인증은 되었지만 인가되지 않은 행동을 수행하려 할때 사용하는 http 상태 코드 입니다.

Copy link
Author

@nonactress nonactress Jan 2, 2026

Choose a reason for hiding this comment

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

위 내용을 조사하고 보니 밑 if구문은 403을 넘겨주는 것으로 수정했습니다!
수정 커밋 : f0a314c

}

if (!jwtTokenProvider.validateToken(token)) {
throw new RuntimeException("인증되지 않은 사용자입니다.");

Choose a reason for hiding this comment

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

RuntimeException을 직접 던지고 있는데, 이렇게 하면 예외 처리 시 어떤 불편함이 있을 수 있을까요?
클라이언트에게 어떤 응답이 가는지 의도한 대로 동작하는지 확인해보셨는지 궁금하네요

Copy link
Author

Choose a reason for hiding this comment

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

준수님께서 말씀해주신 부분을 확인해보니 리졸버에서 던진 예외를 처리하는 곳이 없어 500번대 http 상태코드를 날리고 있습니다!! 이렇게 되면 500번대 코드는 서버오류로 생각하게 되어 잘못된 로그인 정보라고 생각하지 않고 서버가 이상한가라고 생각할 것 같습니다.
image

(위 상황은 로그인 토큰이 없어졌다는 상황을 가정하기 위해 임의로 토큰을 지웠습니다.)
이처럼 토큰이 만료되었거나 이상하다면 에러가 나가게 되는데 이를 globalExceptionHandler로 처리해서 401에러코드로 바꿔야 할 것 같습니다!!!

반영 커밋 :

image 이제 `globalExceptionHandler`가 401오류로 잘 처리해서 보내는 것을 확인할 수 있습니다!!

Copy link
Author

Choose a reason for hiding this comment

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

반영 커밋 : f5678ef

Comment on lines +26 to +30
@Override
public boolean supportsParameter(MethodParameter parameter) {
return parameter.hasParameterAnnotation(AuthMember.class)
&& Member.class.isAssignableFrom(parameter.getParameterType());
}

Choose a reason for hiding this comment

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

supportsParameter 메서드는 언제 호출되고, 어떤 역할을 하는지 설명해주실 수 있나요?

Copy link
Author

Choose a reason for hiding this comment

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

supportRarameter 는 리졸버가 어떤 상황에서 사용되어야 하는지 조건을 확인하는 부분이라고 생각합니다. (마치 리졸버if이라고 생각하고 있습니다)
위 부분에선 @AuthMember가 붙고 member에 집어 넣을 수 있다면 리졸버가 실행됩니다!!

Copy link

Choose a reason for hiding this comment

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

네, 조건을 확인하는 역할이라는 점은 잘 이해하고 있네요!

조금 더 보충해서 이 supportsParameter 메서드가 언제 호출되는지도 설명해줄 수 있을까요?

힌트를 드리자면, HTTP 요청이 들어와서 컨트롤러 메서드가 실행되기까지의 흐름을 생각해보세요.
Spring이 컨트롤러 메서드를 호출하려면 파라미터에 값을 넣어줘야 하는데, 그 과정에서 HandlerMethodArgumentResolver가 어느 시점에 관여하는지 정리해보면 좋을 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

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

supportsParameterHandlerAdapter 가 실제 컨트롤러 메서드를 호출하기 직전에 호출됩니다. DispatcherServlet 이 컨트롤러를 결정하고 인터셉터의 preHandle 을 통과한 뒤, HandlerAdapter 가 메서드 실행에 필요한 파라미터 값들을 준비하는 과정에서 호출됩니다.

이 시점에 등록된 리졸버들을 순회하며 supportsParameter 로 확인하고, true라면 가능하다면 resolveArgument 를 통해 실제 값을 생성하여 컨트롤러에 전달하게 됩니다.

@nonactress
Copy link
Author

폴더 구조

반영 커밋 : d55ef7c저는 요번 미션에선 도메인을 기준으로 폴더를 나누는 것이 편하다고 생각하였고 다른 팀원들의 PR을 보니 계층형으로 나눈 분들도 계셨습니다!! 다만 아직은 하나의 도메인에 대해 다루는 서비스나 컨트롤러들이 2개 이상을 다루지 않아 패키지로 묶기엔 이르지 않나 해서 만들지 않았었는데 수정하고 global이라는 패키지를 만들어 다양한 도메인에서 사용하는 패키지를 묶어서 관리 해보겠습니다!

DRY 원칙

dry 원칙이란 donot repeat yourself의 줄임말로 다시 코드를 재사용 할때 복붙을 지양하라고 하는 원칙입니다! 따라서 준수님께서 제안해주신 방법인 authservice를 통해 적용 시켜 보았습니다!!

새해 병오년, 복 많이 받으세여~~~ 😄 항상 감사합니다!

Copy link

@gogo1414 gogo1414 left a comment

Choose a reason for hiding this comment

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

현진님 피드백 반영하시느라 고생 많으셨습니다!

이전과 달리 폴더 기준으로 나눠서 확실히 파악하는데는 용이했어요! 다만, Enum 클래스가 util에 있는게 맞는지는 한번 고민해보시면 좋을 것 같습니다!

추가로, advice라고 하는 폴더 네이밍이 직관적으로 와닿지는 않는데 해당 부분도 의도가 따로 있으셔서 그렇게 하신걸까요?

몇가지 코멘트와 리뷰에 대한 답변 해주시면 감사하겠습니다!

현진님도 새해 복 많이 받으시고, 코멘트 몇개 달았는데 해당 부분 수정 진행해주시고 다시 멘션 부탁드릴게요!

Comment on lines +9 to +14
public static Role find(String roleName) {
return Arrays.stream(values())
.filter(role -> role.name().equalsIgnoreCase(roleName))
.findFirst()
.orElseThrow(() -> new IllegalArgumentException("존재하지 않는 권한입니다: " + roleName));
}
Copy link

Choose a reason for hiding this comment

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

해당 find 메서드를 Enum에서 관리하시려고 한 이유가 궁금하네요!

Copy link
Author

Choose a reason for hiding this comment

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

서비스 레이어나 ArgumentResolver 에서 문자열을 Enum 으로 변환하는 로직을 직접 구현하게 되면 코드 중복이 발생할 것 같아 Role 안에 메소드를 만들었습니다!!

@Value("${security.jwt.token.secret-key:this-is-a-sample-secret-key-at-least-32-bytes-long}")
private String secretKey;
@Value("${security.jwt.token.expire-length:3600000}")
@Value("${security.jwt.token.secret-key}")
Copy link

Choose a reason for hiding this comment

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

만료 시간이 드러나진 않아서 좋은데, secret-key라고 지정했을 때는 만료 시간인지 실제 시크릿 키 값인지 이해하기 어렵지 않을까요??

Copy link
Author

Choose a reason for hiding this comment

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

수정하겠습니다!
반영 커밋 : 39ca5c7

Copy link

Choose a reason for hiding this comment

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

아직 Github 내에서 해당 파일 확인이 가능해요. 다시 한번 확인 부탁드려요!

HttpServletRequest request = webRequest.getNativeRequest(HttpServletRequest.class);
return authService.extractMember(request);
}
} No newline at end of file
Copy link

Choose a reason for hiding this comment

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

EOL 신경써주세요!

Copy link
Author

Choose a reason for hiding this comment

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

넵!!! intellij 설정도 하겠습니다!

반영 커밋 : 81cb206

Comment on lines +26 to +30
@Override
public boolean supportsParameter(MethodParameter parameter) {
return parameter.hasParameterAnnotation(AuthMember.class)
&& Member.class.isAssignableFrom(parameter.getParameterType());
}
Copy link

Choose a reason for hiding this comment

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

네, 조건을 확인하는 역할이라는 점은 잘 이해하고 있네요!

조금 더 보충해서 이 supportsParameter 메서드가 언제 호출되는지도 설명해줄 수 있을까요?

힌트를 드리자면, HTTP 요청이 들어와서 컨트롤러 메서드가 실행되기까지의 흐름을 생각해보세요.
Spring이 컨트롤러 메서드를 호출하려면 파라미터에 값을 넣어줘야 하는데, 그 과정에서 HandlerMethodArgumentResolver가 어느 시점에 관여하는지 정리해보면 좋을 것 같아요.

@nonactress
Copy link
Author

Role 위치

리뷰어님 말씀대로 Role 의 위치에 대해 다시 고민해 보았습니다!

기존에는 여러 곳에서 공통으로 사용될 수 있다는 생각에 util 패키지에 두었습니다. 하지만 Role 은 단순한 도구 기능이 아니라, '회원(Member)'이라는 도메인 상태를 정의하는 객체라는 점이라고 생각되어 entity 패키지 안에서 같이 관리하는 것이 좋다고 생각합니다!!!

반영 커밋 : 92c730f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants