[Spring MVC (인증)] 천희정 미션 제출합니다. #193
Conversation
There was a problem hiding this comment.
👍 늦게라도 다시 미션을 시작해주셔서 좋네요. 상세한 고민을 하지 못하고 작성했다고 하셨지만, 본인이 직접 먼저 개선 포인트를 찾고 앞으로의 리팩터링 계획을 세워주셔서 좋은 자세입니다. 우선은 PR 본문에서 말씀해주신 사항들 제외하고 리뷰를 남기려고 해봤고요, 희정님께서 작성해주신 리팩터링 계획에 맞춰서 수정된 코드를 보면서 2차 리뷰를 해보면 좋을 것 같아요.
JWT 토큰을 그대로 읽어서 나온 값으로 권한을 줄 것인가, 아니면 DB를 조회해서 찾을 것인가는 정답은 없는 문제이긴 합니다만, 개인의 취향은 있을 것 같아요. 간단한 프로그램에서는 문제가 없지만 만약 보안 및 인증이 중요한 실제 서비스라면 저는 DB 저장을 선택할 것 같아요. 일단 JWT 토큰 자체가 서버에서 직접 만료시킬 수 없다 라는 점이 큽니다. 따라서 실제 유저의 권한값과 토큰에 저장되어 있는 권한값이 다를 수 있기 때문에, 단순히 토큰에 들어있는 정보만 가지고 권한을 사용하는 것은 문제가 될 소지가 있기 때문이죠.
| @Value("${roomescape.auth.jwt.secret}") | ||
| private String SECRET_KEY; |
There was a problem hiding this comment.
여기를 생성자 주입을 통해 불변으로 만들겠다고 하신거죠? 고민 좋네요 👍
| @Autowired | ||
| public WebConfig(AdminInterceptor adminInterceptor) { | ||
| this.adminInterceptor = adminInterceptor; | ||
| } |
There was a problem hiding this comment.
스프링 4.3 버전 이후부터는 단일 생성자에 대해서는 @Autowired를 명시하지 않더라도 자동으로 의존성 주입이 이루어집니다!
| if (request.getCookies() == null) return null; | ||
|
|
||
| for (var cookie : request.getCookies()) { |
There was a problem hiding this comment.
사소하지만 request.getCookies() 를 미리 변수에 할당해두면 두 번 호출을 피할 수 있겠군요
| public Member findByToken(String token) { | ||
| String email = jwtProvider.getEmailFromToken(token); | ||
| if (email == null) { | ||
| return null; | ||
| } | ||
| return memberDao.findByEmail(email).orElse(null); | ||
| } |
There was a problem hiding this comment.
Member 라는 도메인만 보면 토큰이라는 정보를 모르는데요! Member가 토큰을 받아서 그 토큰 안에 이메일이라는 것이 있다는 것까지 알고 이메일을 파싱해서 조회해도 괜찮은걸까요? 회원이라는 도메인이 어디까지 책임을 가져야 하는가에 대해서 고민해보면 좋을 것 같아요!
| !gradle/wrapper/gradle-wrapper.jar | ||
| !**/src/main/**/build/ | ||
| !**/src/test/**/build/ | ||
| .DS_Store |
| @RestController | ||
| public class LoginController { | ||
|
|
||
| private final MemberDao memberDao; |
There was a problem hiding this comment.
생각해보기) 우리가 지금 계층형 구조(흔히 레이어드 아키텍쳐라고 하죠)를 채택하고 사용하고 있는데, 표현 계층인 컨트롤러가 서비스를 건너뛰고 바로 DAO를 사용하는 것에는 어떤 문제가 있을까요?
물론 이 문제에 대해서는 의존 방향이 아래로 향하기만 하면 문제가 없다는 의견도 있기 때문에 반드시 DAO를 여기서 쓰는 것이 틀렸다고 리뷰드리는 것은 아닙니다!
| public boolean validateToken(String token) { | ||
| try { | ||
| Jwts.parser() | ||
| .setSigningKey(SECRET_KEY) | ||
| .parseClaimsJws(token); | ||
| return true; | ||
| } catch (Exception e) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
일반적으로 validate~ 라는 네이밍을 가진 메서드의 경우 특정 조건이 유효함을 검증한다 라고 쓰이기 때문에 boolean 반환을 기대하는 것은 어색해보여요. 예외를 그대로 던지는 형태로 사용하거나, isValid 정도로 네이밍해보는 것은 어떨까요?
|
|
||
| return ResponseEntity.ok().build(); | ||
| } catch (Exception e) { | ||
| return ResponseEntity.status(401).build(); |
There was a problem hiding this comment.
401 Unauthorized 는 요청한 리소스에 대해 권한이 없을 때 를 나타내는 HTTP 코드인데요, 개인적으로는 로그인 실패에 이 상태코드가 맞는지는 조금 의문이 듭니다! 로그인 자체는 권한이 필요한게 아니라 권한이 얻기 위해 아이디와 비밀번호를 검증하는 과정이기 때문에 그렇습니다. 어떠한 이유로 401이라는 상태코드를 사용하셨나요?
2 participants
안녕하세요, 스터디 미션 제출이 많이 늦어져서 정말 죄송합니다.
개인적인 사정으로 제출이 많이 늦어졌지만, 이제라도 미션을 공유드립니다.
이번 미션을 진행하며 크게 아래와 같은 부분에서의 고민, 시행착오가 있었고, 수정해서 제출하도록 하겠습니다.
처음에는 큰 고민 없이 DB에서 role 값을 읽으려고 했습니다.
하지만 현재 미션에서는,
단순한 ADMIN/USER 구분만 필요
권한이 자주 변경되지 않는 시스템
매번 DB 조회하는 것보다 JWT에서 바로 읽는 것이 성능상 유리
따라서 JWT 토큰 내에서 role 정보를 읽어서 처리하는 방향으로 변경했는데,
이 과정에서 기존 코드를 수정하느라 시간이 많이 걸렸습니다.
추가로, DB에서 role값을 읽는 것이 보안적으로는 더 안전한 방법이라는 생각이 듭니다.
JWT 시크릿 키 불변성 문제
현재 이렇게 JWT 시크릿 키를 사용하고 있는데, final 키워드가 없어서 런타임 중에 변경될 위험이 있습니다.
다음과 같이 생성자 주입과 final을 사용해서 불변 값으로 개선하겠습니다
리뷰를 작성하면서 코드를 다시 확인해보니 발견한 부분입니다.
앞으로는 신경쓰도록 하겠습니다.
이번 3단계 미션을 진행하면서 1번과 연관되어 많은 시행착오를 겪었습니다.
이 부분은 DB 조회용과 신규 생성용으로 구분하려다 보니 생긴 문제인데, 오히려 코드가 복잡하다는 생각이 들었습니다. DTO 분리로 책임을 명확히 하는 방향으로 리팩토링을 진행하겠습니다.
JWT 검증 실패에는 토큰 만료, 형식 오류, 서명 검증 실패 등 다양한 원인이 있는데 모든 예외를 똑같이 처리해서 디버깅이 어려웠습니다. 이 부분도 수정하겠습니다.
반복되는 쿠키 추출 로직을 따로 분리하려고 합니다. 현재는, AdminInterceptor에서만 사용하고 있지만, 나중에 다른 곳에서도 필요할 수 있어서, 분리를 고민하게 되었습니다.
코드를 작성하면서 "일단 돌아가니까 됐다"는 마음으로 넘어가는 순간이 많았던 것 같아서 많이 반성합니다.
앞으로는 DTO 분리, 구체적인 예외 처리, 공통 유틸리티 분리 등의 리팩토링을 통해 더 나은 코드를 작성하도록 하겠습니다.
늦은 제출에도 불구하고 리뷰해주셔서 감사합니다 🙇🏻♂️