[Spring MVC(인증)] 신혜빈 미션 제출합니다. #101
Conversation
| @Value("${jwt.secret}") | ||
| private String secretKey; |
There was a problem hiding this comment.
@Value 어노테이션을 활용해서 값을 주입 받는군요~!
덕분에 보안에 민감한 정보가 코드 레벨에 노출되지 않겠어요 👍
| @PostConstruct | ||
| public void init() { | ||
| if (secretKey == null) { | ||
| throw new IllegalStateException("SECRET_KEY가 초기화되지 않았습니다."); | ||
| } | ||
| key = new SecretKeySpec(secretKey.getBytes(), SignatureAlgorithm.HS256.getJcaName()); | ||
| } |
There was a problem hiding this comment.
시크릿 키 설정이 되지 않았을 경우를 대비해서 예외 처리를 해주셨군요! 꼼꼼한 처리를 하려고 노력하신 모습이 보여요.
다만 이곳에는 세 가지 정도의 리뷰 포인트가 존재할 것 같아요.
@PostConstruct어노테이션은 Bean 객체가 생성된 이후에 작동하는 것으로 알고 있어요. 하지만 secretKey가 존재하지 않아서 예외가 발생하는 타이밍은 Bean 객체가 생성될 시점이기 때문에, 혜빈님이 의도한 예외 처리 플로우는 작동되지 않을 거예요.- secretKey가 없어서 터지는 상황에 대한 예외 처리가 필요한지 고민해보면 좋을 것 같아요. 혜빈님은 예외 처리가 언제 필요한 것 같으신가요?
제 개인적으로는, 예외 처리가 효과적인 상황은 런타임 시점, 즉 스프링 애플리케이션이 돌아가고 있을 때일텐데요.@Value어노테이션으로 주입할 값이 없을 때 발생하는 예외는 스프링 자체적으로 애플리케이션이 정상적으로 뜨기 전에 발생하게 돼요. 그래서 주입할 값이 없다면 스프링 애플리케이션이 애초에 실행되지 않아요.
그런 이유에서, 저는 이 케이스에 대한 예외 핸들링은 필요하지 않을 것 같다는 생각입니다. 혜빈님은 어떻게 생각하시나요? - key 라는 값이 객체 필드여야 하는지에 대한 고민을 해보면 좋겠어요. 객체 필드로 두신 이유가 무엇일까요?
| public Long getUserIdFromToken(String token) { | ||
| try { | ||
| String subject = Jwts.parserBuilder() | ||
| .setSigningKey(key) | ||
| .build() | ||
| .parseClaimsJws(token) | ||
| .getBody() | ||
| .getSubject(); | ||
| return Long.valueOf(subject); | ||
| } catch (Exception e) { | ||
| throw new IllegalArgumentException("유효하지 않은 토큰입니다.", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
예외를 Stack Trace가 가능하도록 잘 처리해주셨어요~!
한 가지 리뷰 드리고 싶은 점은, JwtUtil이 Token으로부터 너무 구체적인 값을 꺼내는 것이 아닐까 하는 문제예요.
JwtUtil의 책임은 어디까지일까요? 제가 생각하는 JwtUtil의 책임은, 주어진 Token으로부터 적절히 Claims를 꺼내오는 것 인 것 같아요. UserId는 우리가 만들고 있는 서비스 사정이고, JwtUtil은 이 사정을 몰라도 된다고 생각합니다.
| @Autowired | ||
| private JwtUtil jwtUtil; | ||
| @Autowired | ||
| private MemberDao memberDao; |
There was a problem hiding this comment.
전반적으로 의존성 주입을 생성자 주입이 아닌 @Autowired 명시를 통한 필드 주입으로 해주고 계시네요. 저는 가능하다면 필드 주입보다 생성자 주입을 권해드리고 싶어요.
그 이유는 코드 절약(?) 및 테스트와 관련이 있을 것 같아요. 생성자 주입을 사용하게 되면, 경우에 따라서 @Autowired 어노테이션을 생략할 수 있으며, 단위 테스트가 쉬워진다는 매우 큰 장점이 있는 것 같네요.
| Long userId = jwtUtil.getUserIdFromToken(token); | ||
| Member member = memberDao.findById(userId); | ||
|
|
||
| if (member == null || !"ADMIN".equals(member.getRole())) { |
There was a problem hiding this comment.
문자열 비교를 할 때 상수 값을 기준으로 비교해서 NPE를 방지하는 디테일! 아주 굿~
| if (token == null) { | ||
| response.setStatus(401); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
예외 상황일 경우에 예외를 던지는 것과, false 값을 반환하는 것의 주요 차이는 무엇일까요?
요거에 대해 한번 고민해보시고, token이 제대로 담겨오지 않았을 경우에는 어떤 선택지를 고르는 게 좋을지 고민해봐요.
| if (cookies != null) { | ||
| for (Cookie cookie : cookies) { | ||
| if ("token".equals(cookie.getName())) { | ||
| return cookie.getValue(); | ||
| } | ||
| } | ||
| } | ||
| return null; |
There was a problem hiding this comment.
stream을 사용해서 indent depth를 줄여볼 수 있을 것 같아요~! 한번 시도해보시죠!
| try { | ||
| String token = loginService.authenticate(loginRequestDto.getEmail(), loginRequestDto.getPassword()); | ||
|
|
||
| Cookie cookie = loginService.createAuthCookie(token); | ||
| response.addCookie(cookie); | ||
| Map<String, Object> responseBody = loginService.createLoginResponse(token); | ||
|
|
||
| return ResponseEntity.ok(responseBody); | ||
| } catch (IllegalArgumentException e) { | ||
| Map<String, Object> errorResponse = new HashMap<>(); | ||
| errorResponse.put("message", e.getMessage()); | ||
| return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body(errorResponse); | ||
| } |
There was a problem hiding this comment.
Controller에서의 try-catch 보다, ExceptionHandler 와 같은 기능을 이용하는 것은 어떨까요?
| if (reservationRequest.getName() == null) { | ||
| Long userId = jwtUtil.getUserIdFromToken(token); | ||
| Member member = memberDao.findById(userId); | ||
| if (member == null) { | ||
| return ResponseEntity.status(HttpStatus.UNAUTHORIZED).build(); | ||
| } | ||
| reservationRequest.setName(member.getName()); | ||
| } | ||
|
|
||
| ReservationResponse reservation = reservationService.save(reservationRequest); |
There was a problem hiding this comment.
어노테이션 활용 부족
이번 코드를 작성하면서 어노테이션을 잘 활용하지 못한 것 같아 조금 아쉽습니다.
제가 작성한 코드에서 어노테이션을 적절히 활용할 수 있었던 부분이 있다면 피드백 부탁드립니다.
어노테이션 활용을 연습할 때 유용한 팁이나 자주 사용하는 어노테이션에 대한 설명도 해주시면 정말 감사하겠습니다.
말씀 주신 활용 부분은 이 부분이 될 것 같아요. 예약을 생성하기 위해서는 token으로부터 Member를 찾아내어야 하는데요. 이 로직들은 예약 생성에 관한 내용과 직접적인 연관은 없지만, 반드시 거쳐야 하는 필수 로직이죠.
반드시 거쳐야 하지만 주 관심사가 아닌 로직들을 횡단 관심사 라고 부릅니다. 이런 횡단 관심사 로직들을 공통적으로 처리하기 위한 방안으로, 스프링에서는 Interceptor/Argument Resolver라는 기술을 제공하고 있어요.
지금과 같이 Member 라는 객체가 필요한 경우에는, 횡단 관심사 로직을 처리하면서 객체를 원하는대로 조립할 수 있는 Argument Resolver를 사용하면 좋을 것 같아요. 이때 커스텀 어노테이션을 활용해볼 수 있어요.
해결 방안에 대해서는 혜빈님이 스스로 고민해보시면 좋을 것 같아요. 이전에 태연님과 제가 작성한 코드 링크를 첨부할테니, 참고해서 공부하는 것도 좋겠습니다!
| Long userId = jwtUtil.getUserIdFromToken(token); | ||
| Member member = memberDao.findById(userId); | ||
|
|
||
| if (member == null) { | ||
| return ResponseEntity.status(HttpStatus.UNAUTHORIZED).build(); | ||
| } |
There was a problem hiding this comment.
계층 구조에 대한 점검
이번 미션동안 login 폴더에 있는 클래스들을 계층별로 잘 분리하려고 노력하였습니다.
제 눈엔 코드가 잘 분리된 거 같은 데 아직 계층 분리에 익숙하진 않아서 확신이 서진 않네요 ㅜㅠ
혹시 계층 구조가 부자연스럽거나 개선할 만한 부분이 있다면 짚어주실 수 있을까요?
계층 구조는 전반적으로 잘 나누어주신 것 같아요. 다만 이 코드 영역을 Service 계층의 책임으로 옮겨보면 더 좋을 것 같습니다.
Member를 찾는 행위를 Service 계층에서 하고, 만약 적절한 Member를 찾지 못했다면 예외를 던지도록 로직을 구성해볼 수 있을 것 같아요.
이렇게 되면, LoginController에서 MemberDao와 JwtUtil에 대한 의존성도 제거할 수 있을 것 같아요!
안녕하세요!! 승용 리뷰어님!! 이렇게 또 만나뵙게 되어 반갑네요 ㅎㅎ 잘 부탁드립니다!! :)
<참고사항>
login폴더 안의 모든 클래스와 ReservationController 클래스 빼고는 모두 기본 제공 클래스들입니다!!
<구현과정>
1단계 - 로그인 기능 구현
2단계 - 로그인 기능 리팩터링
3단계 - 관리자 기능 구현
<궁금한 점>
계층 구조에 대한 점검
이번 미션동안 login 폴더에 있는 클래스들을 계층별로 잘 분리하려고 노력하였습니다.
제 눈엔 코드가 잘 분리된 거 같은 데 아직 계층 분리에 익숙하진 않아서 확신이 서진 않네요 ㅜㅠ
혹시 계층 구조가 부자연스럽거나 개선할 만한 부분이 있다면 짚어주실 수 있을까요?
어노테이션 활용 부족
이번 코드를 작성하면서 어노테이션을 잘 활용하지 못한 것 같아 조금 아쉽습니다.
제가 작성한 코드에서 어노테이션을 적절히 활용할 수 있었던 부분이 있다면 피드백 부탁드립니다.
어노테이션 활용을 연습할 때 유용한 팁이나 자주 사용하는 어노테이션에 대한 설명도 해주시면 정말 감사하겠습니다.