[JWT 인증 서버] 최정훈 제출합니다.#49
Conversation
|
안녕하세요 정훈님! 과제 수행하시느라 고생 많으셨습니다. 이번 리뷰에서는 JWT 토큰의 생성·검증 로직이 안전하게 구현되었는지, 예외 처리는 꼼꼼하게 되었는지를 중심으로 살펴보려고 합니다. 정답을 정해두고 하는 리뷰라기보다는, 더 좋은 코드를 함께 고민하는 자리이니 편하게 이야기 나눴으면 좋겠습니다. |
| .csrf(csrf -> csrf.disable()) | ||
|
|
||
| // JWT를 사용하므로 세션을 생성하지 않음 | ||
| .sessionManagement(session -> | ||
| session.sessionCreationPolicy(SessionCreationPolicy.STATELESS)) | ||
|
|
||
| // 기본 로그인 화면 비활성화 | ||
| .formLogin(form -> form.disable()) | ||
|
|
||
| // HTTP Basic 인증 비활성화 | ||
| .httpBasic(basic -> basic.disable()) |
There was a problem hiding this comment.
csrf, session, formLogin, httpBasic을 비활성화 해두셨는데 비활성화를 사용하시는 이유를 설명해주시면 좋을 것 같습니다!
There was a problem hiding this comment.
정훈님, User 엔티티를 보면 id, username, password, role 같은 기본적인 컬럼들을 깔끔하게 잘 구성해 주셨습니다!
한 가지 논의해보고 싶은 점은, 현재 **Refresh Token(RT)**을 저장하는 구조가 빠져 있다는 점입니다. Access Token은 무상태(Stateless)로 검증되므로 DB 저장 컬럼이 필수가 아니지만, Refresh Token은 토큰 갱신 및 보안 제어를 위해 서버 측 저장소(테이블이나 Redis 등)가 꼭 필요하다고 생각합니다.
만약 Access Token이 만료되었을 때, 프론트엔드가 보낸 Refresh Token이 서버에 저장된 값과 일치하는지 확인하고 검증하는 과정이 있어야 안전하게 Access Token을 재발급해줄 수 있거든요. (서버에 저장해두지 않으면 토큰이 탈취당했을 때 강제 로그아웃 등의 제어가 불가능해지는 문제도 있습니다.)
혹시 정훈님이 이번 로컬 로그인을 구현하시면서 구상하셨던 Access Token과 Refresh Token의 구체적인 관리 전략이나 각각의 역할에 대해 피드백으로 공유해 주시면 감사하겠습니다!
| // Authorization 헤더가 없거나 | ||
| // Bearer 형식이 아니면 인증 정보를 설정하지 않고 다음 필터 실행 | ||
| if (authorizationHeader == null || !authorizationHeader.startsWith("Bearer ")) { | ||
| filterChain.doFilter(request, response); | ||
| return; | ||
| } |
There was a problem hiding this comment.
이 부분을 보면 Authorization헤더가 아니거나 토큰 타입이 Bearer가 아니면 다음 필터를 실행시키는 로직인 것 같습니다!
토큰이 없는데 에러메세지를 출력하지않고 다음 필터로 넘기셨는지 의도가 궁금합니다!
Donghwan814
left a comment
Donghwan814
left a comment
There was a problem hiding this comment.
안녕하세요 정훈님! 시험은 잘 보셨을까요? 혹시 월요일까지 시험이 있으시다면 남은 시험도 파이팅입니다!
시험 기간이 겹치다 보니 코드 리뷰가 조금 늦어진 점 양해 부탁드립니다 🥲
이번 코드 리뷰는 마지막 과제인 인증/인가와 시큐리티에서 중요하게 다루는 JWT 관련 내용입니다. 보안과 직접적으로 연결되는 부분이다 보니 신경 써야 할 내용도 많고, 헷갈리는 개념도 많았을 것이라고 생각합니다. 처음부터 완벽하게 구현하기에는 분명 어려움이 있었을 것이라 예상됩니다.
그래서 이번 리뷰에서는 정훈님께서 코드를 이렇게 작성하신 의도를 먼저 확인하고, 수정해야 하는 부분들을 추가적으로 피드백하는 시간을 가져보려고 합니다!
과제 제출하시느라 정말 고생 많으셨습니다!
There was a problem hiding this comment.
trace는 지금 서버 내에서 뜨는 로그 내용을 출력해주는거 같은데 저 내용을 클라이언트 입장에서 봤을 때 서버가 어떤 구조로 동작하고 어떻게 처리하는지 파악해 보안 위험이 있습니다. 그래서 클라이언트한테는 서버 로그를 보여주는 방식이 아닌 간단한 에러 메시지만 반환하고 서버로그는 서버 내부에서만 확인 가능하도록 처리하는 방식이 좋습니다.
혹시 trace 즉 서버 로그를 body값에 넘겨주는 특별한 의도가 있는지 정훈님의 의견을 들어보고 싶습니다!
|
|
||
| // 회원가입 | ||
| @PostMapping("/register") | ||
| public ResponseEntity<Map<String, String>> register(@RequestBody RegisterRequest request) { |
There was a problem hiding this comment.
회원가입 응답에서 Map<String, String> 방식을 사용해 메시지를 반환해주신 부분 확인했습니다!
로그인 응답에서는 TokenResponse라는 DTO를 사용하고 있는데, 회원가입 응답에서는 DTO가 아닌 Map을 사용하신 이유가 궁금했습니다.
개인적으로는 로그인 응답처럼 회원가입 응답도 MessageResponse나 RegisterResponse 같은 DTO로 분리하면 응답 구조가 더 명확해지고, 전체 코드 스타일도 일관성 있게 가져갈 수 있을 것 같다고 생각했습니다.
정훈님께서는 이 부분을 어떤 의도로 구현하셨는지 의견을 들어보고 싶습니다!
| @GetMapping | ||
| public ResponseEntity<List<Map<String, Object>>> getPosts(Authentication authentication) { | ||
| return ResponseEntity.ok( | ||
| List.of( | ||
| Map.of( | ||
| "id", 1, | ||
| "title", "JWT 테스트 게시글의 제목", | ||
| "content", "JWT 테스트 게시글의 내용", | ||
| "username", authentication.getName() | ||
| ) | ||
| ) | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
현재는 인증 테스트용으로 하드코딩을 진행해서 작성해 주신 것 같습니다! 이후 실제 데이터 테스트 단계에서는 하드코딩된 값이 아닌 DB 조회 기반으로 변경하는 과정이 필요해 보입니다. 이 부분을 추후에 어떻게 구현하실 계획인지 정훈님의 의견을 들어보고 싶습니다!
| } | ||
|
|
||
| // 로그인 | ||
| @Transactional(readOnly = true) |
There was a problem hiding this comment.
로그인 메서드에만 @Transactional(readOnly = true)를 사용해주신 부분 확인했습니다!
제가 이해하기로 로그인 로직에서는 사용자의 이메일이나 아이디를 기준으로 회원 정보를 조회한 뒤, 비밀번호 검증과 JWT 발급을 진행하기 때문에 DB 데이터를 수정하기보다는 조회 위주의 작업이 이루어지는 것으로 알고 있습니다. 그래서 readOnly = true를 적용해주신 것 같다고 생각했습니다.
다만 회원가입 메서드에는 트랜잭션이 따로 적용되어 있지 않고, 로그인 메서드에만 @Transactional(readOnly = true)가 적용되어 있어서 이 부분을 어떤 기준으로 구분하셨는지 궁금했습니다.
3 participants
과제명
JWT 인증 서버
💡 작업 내용
🔗 참고 링크
🤔 느낀 점 / 어려웠던 점