Skip to content

Conversation

@kimsky247-coder
Copy link

안녕하세요 경환님! 지난 미션에 이어 또 만나 뵙게 되어 반갑습니다. 이번 미션도 잘 부탁드립니다🙇

프로젝트 구조

auth/
- AdminInterceptor - 관리자 권한을 검증

member/
- Member - 회원 도메인 객체
- MemberController - 회원 관련 API 엔드포인트 정의
- MemberService - 회원 비즈니스 로직 처리
- MemberDao - 회원 데이터베이스 접근 로직
- LoginMember - 로그인된 회원 정보를 담는 객체
- LoginMemberArgumentResolver - 토큰을 통해 LoginMember 객체를 주입하는 리졸버
- LoginRequest - 로그인 요청 DTO
- MemberRequest - 회원 생성/수정 요청 DTO
- MemberResponse - 회원 정보 응답 DTO

reservation/
- Reservation - 예약 도메인 객체
- ReservationController - 예약 관련 API 엔드포인트 정의
- ReservationService - 예약 비즈니스 로직 처리
- ReservationDao - 예약 데이터베이스 접근 로직
- ReservationRequest - 예약 생성 요청 DTO
- ReservationResponse - 예약 정보 응답 DTO

theme/ (변경 없음)
time/ (변경 없음)

util/
- JwtUtil - JWT 토큰 생성 및 파싱을 담당하는 유틸리티 클래스

root/
- RoomescapeApplication
- WebMvcConfiguration - Interceptor및 ArgumentResolver 등록 설정
- PageController - 화면 이동을 담당하는 컨트롤러
- ExceptionController - 전역 예외 처리

궁금한 점

  1. 패키지 구조를 제공된 코드의 구조를 따라 Controller, Service 패키지로 나누는 방식 대신 도메인별로 관련 클래스들을 모아두는 방식을 유지했습니다. 기존에 사용하던 계층형 구조에 비해 관련 파일을 찾기는 편했지만, 도메인 간의 의존성이 복잡해질 경우에도 이 구조가 유리한지, 현업에서는 어떤 방식을 사용하는지 궁금합니다!

  2. 현재는 /admin 경로만 인터셉터에 등록되어 있지만, 추후 기능이 추가되면

 public void addInterceptors(InterceptorRegistry registry) {
    registry.addInterceptor(adminInterceptor)
            .addPathPatterns("/admin")
            .addPathPatterns("/themes")
            .addPathPatterns("/times");
            (...)
}

위와 같이 코드가 길어질 것 같습니다. 경로를 효율적으로 관리할 수 있는 방법이 있는지 궁금합니다

Copy link

@dooboocookie dooboocookie left a comment

Choose a reason for hiding this comment

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

안녕하세요, 하늘.
리뷰어 루카입니다.

이번에는 인증 인가 하는 부분을 위주로 리뷰드렸습니다.

그리고 질문주신 내용에 대해서는 시원하게 답변을 드리기 어려울것 같아요.

  1. 절대적은 규칙은 없습니다. 제가 봤던 프로젝트에서는 1개만 채택한 경우는 못본 것 같아요. 패키지든 모듈이든 어떤 수단을 통해서 레이어와 도메인을 구분하고 구조화했습니다.

  2. uri path 를 계층화하고 상수화해서 참조하는 방식으로 쓰면되지 않을까 싶습니다 어떤 부분이 우려되시나요?


리뷰를 늦게드려서 촉박하실 수 있지만 반영부탁드립니다!!


Member member = memberService.findById(memberId);

if (!"ADMIN".equals(member.getRole())) {

Choose a reason for hiding this comment

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

🔥 Request Change

role은 member의 중요한 필드네요.
String보다는 조금 더 엄격한 타입이 필요해보입니다.

Copy link
Author

Choose a reason for hiding this comment

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

Role은 로직의 핵심 필드이기 때문에 String으로 관리할 경우 오타나 유효하지 않은 값이 들어갈 위험이 있기 떄문에 더 엄격한 관리가 필요한 것 같습니다. Role Enum을 사용하여 허용된 값만 사용할 수 있도록 타입을 제한했습니다.

Comment on lines 25 to 48
public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler) throws Exception {
String token = extractToken(request.getCookies());

if (token == null) {
response.setStatus(401);
return false;
}

try {
Long memberId = JwtUtil.getMemberIdFromToken(token);

Member member = memberService.findById(memberId);

if (!"ADMIN".equals(member.getRole())) {
response.setStatus(401);
return false;
}
return true;

} catch (Exception e) {
response.setStatus(401);
return false;
}
}

Choose a reason for hiding this comment

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

👀 Comment

throws Exception을 하는 메서드인데,
코드상에서 던져지고 있는 Exception은 없어보이는데요.
44-47라인의 catch 블럭은 어떤 의도를 갖고 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

44-47라인의 catch 블럭은 토큰 파싱이나 유효성 검증 과정에서 발생하는 예외를 일괄적으로 인증 실패로 처리하려는 의도였습니다. 하지만 Exception을 광범위하게 잡을 경우, DB 연결 오류 같은 예기치 못한 서버 에러까지 401로 처리되어 디버깅이 어려워지는 문제가 있음을 인지했습니다. 그래서 토큰 관련 예외만 명시적으로 catch하여 처리하도록 범위를 좁히고, 불필요해진 throws Exception 선언은 제거했습니다.

Comment on lines 32 to 44
@PostMapping("/login")
public ResponseEntity<Void> login(@RequestBody LoginRequest request, HttpServletResponse response) {
Member member = memberDao.findByEmailAndPassword(request.getEmail(), request.getPassword());

String accessToken = JwtUtil.createToken(member);

Cookie cookie = new Cookie("token", accessToken);
cookie.setHttpOnly(true);
cookie.setPath("/");
response.addCookie(cookie);

return ResponseEntity.ok().build();
}

Choose a reason for hiding this comment

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

👀 Comment

로그인 처리를 하는 컨트롤러군요.

이 메서드만 보고 2가지를 인지할 수 있었어요.

  1. Member는 이메일과 비밀번호가 있어서 그걸 통해서 로그인하는구나
  2. JWT를 응답 쿠키에 담아주는구나.

제가 봤을 땐 굳이 컨트롤러에 두지 않아도 되는 부분이 있어보여요.
httpAPI가 아니게되거나,
Spring application이 아니게되면,
가장 먼저 바뀌게 될테니까요. 서비스의 핵심적인 부분과 HTTP요청응답을 위해 컨트롤러에 존재해야되는 부분을 잘 구분해보시죠.

Copy link
Author

Choose a reason for hiding this comment

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

컨트롤러가 회원 조회나 토큰 생성 같은 비즈니스 로직까지 직접 처리하고 있어, 서비스의 핵심 로직이 HTTP 웹 계층에 강하게 결합되어 있는 문제가 있었습니다. 추후 HTTP API가 아닌 다른 환경에서 로그인을 구현할 때 해당 로직을 재사용하기 어려울 것 같습니다.
회원 검증과 JWT 토큰을 생성하는 핵심 로직은 MemberService가 전담하도록 수정하고, 컨트롤러는 오직 생성된 토큰을 받아 HTTP 쿠키를 설정하고 응답하는 역할 만 하도록 수정했습니다


@PostMapping("/login")
public ResponseEntity<Void> login(@RequestBody LoginRequest request, HttpServletResponse response) {
Member member = memberDao.findByEmailAndPassword(request.getEmail(), request.getPassword());

Choose a reason for hiding this comment

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

🔥 Request Change

  • 가입되어있는 이메일이 없다.
  • 비밀번호가 틀렸따.

위 두가지 상황이 있을 수 있는데요.

application 관점에서는 어떤 분기를 타야할까요? 혹은 어떤 예외가 발생해야할까요?
api 관점에서는 어떤 응답이 내려가야될까요?

Copy link
Author

Choose a reason for hiding this comment

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

application 관점에서는 로그인 실패 원인을 명확히 구분하는 것이 중요하다고 생각했습니다. 이에 따라 로그인 검증 로직을 MemberService로 위임하고, 가입되지 않은 이메일인 경우와 비밀번호가 일치하지 않는 경우 각각 예외를 발생시키도록 구현했습니다.
api 관점에서는 두 경우 모두 인증에 실패한 상황이므로 전역 예외 처리기가 이를 잡아 응답하도록 수정했습니다

}

@Override
public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler) throws Exception {

Choose a reason for hiding this comment

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

👍 Approve

interceptor로 인가처리를 잘 구현하셨나요!!
좋습니다.

추가적인 질문이 있는데요.
LoginMemberArgumentResolver또한 존재합니다.
위 리졸버 또한 토큰의 값을 이용해서 회원 조회를 하는데요.
리졸버에서도 인가처리가 되고있다고 보여집니다.
인터셉터와 리졸버를 둘 다 구현하신 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

두 기능 모두 토큰을 다룬다는 점에서 역할이 겹쳐 보일 수 있지만 검증과 값 변환이라는 책임이 다르기 때문에 둘 다 사용해야한다고 생각했습니다.
인터셉터는 컨트롤러가 실행되기 전에 토큰이 유효한지 확인하고 잘못된 요청을 차단하는 역할을 담당하고, 아규먼트 리졸버는 검증이 끝난 토큰에서 사용자 정보를 꺼내 컨트롤러 파라미터로 넘겨주는 역할을 합니다. 따라서 인터셉터에서는 인가 로직을 전담하고 아규먼트 리졸버를 통해서는 컨트롤러의 중복 코드를 줄이는 방식으로 각자의 역할을 분리했습니다.

@dooboocookie
Copy link

직접 고려해봤으면 하는 사항들여서 리뷰를 조금 간접적으로 드렸는데 다 잘 생각해보신 것 같아서 이만 approve 하겠습니다!

@boorownie boorownie merged commit 408d7a3 into next-step:kimsky247-coder Jan 5, 2026
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.

3 participants