Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Spring MVC(인증)] 정상희 미션 제출합니다. #103

Open
wants to merge 13 commits into
base: sangheejeong
Choose a base branch
from

Conversation

SANGHEEJEONG
Copy link

@SANGHEEJEONG SANGHEEJEONG commented Dec 26, 2024

안녕하세요 루카님 새해 복 많이 받으세요!!

nextStep의 강의(?) 또는 한 챕터가 바뀔 때마다 새롭고 낯선 느낌이 드네요 :)
지난번 미션을 하면서는 레이어드 아키텍처에 따른 계층형 구조로 미션을 진행했는데 이번 미션은 주어진 견본 코드가 도메인형 구조라 조금 새로웠던 것 같습니다.

이번 미션을 하면서 DTO 클래스명을 정하는 것이 좀 고민이었는데요,
미션을 다 끝내고 보니 일관성이 하나도 없는 것 같아서 리팩토링을 좀 했습니다. 괜찮은지 봐주시면 감사하겠습니다.

또 이번 미션이 유독 낯설게 느껴졌는데 아마 HttpServletRequest, Interceptor 같은 처음 보는 단어들이 많이 나와서 그런 것 같습니다. 미션을 진행하면서 스터디를 할 때 약간 그때그때 나오는 새로운 개념들을 찾는 식으로 공부를 했는데 뭔가 미션을 위한 공부를 한 느낌?이 들더라구요,, 약간 어디서부터 어디까지 알아야 하는지 모르겠어서 막막했습니다. 리뷰어님은 처음 스프링 MVC를 스터디할 때 어떤 방식으로 하셨었는지 궁금합니다.

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. AuthClaims 객체의 역할
  2. 쿠키에 대한 학습
  3. JWT에 담을 내용
  4. 인증 인가 관련 예외 상황 핸들링

🖍️ 학습에 관하여

정말 막막하다고 생각해요.
Spring Framework은 정말 큰 기술이에요.

사실 우리는 @PathVariable이 어떻게 내부적으로 동작하게 되는지 모르고 사용했었죠.
이제서야 ArgumentResolver라는 개념을 만나고 약간은 좀 알 것 같은 느낌이 들게 됐네요.
이렇게 Top-Down으로 깍아나가면 될 것 같습니다.

프레임워크 동작원리에 대해서 아는 것은 디버깅을 하거나, 그를 활용할 때 많이 도움이 되겠죠.
하지만, 저희의 리소스는 한정적이죠.
(단언)모든 것을 다 알 수 없습니다. 모름에 익숙해져야 합니다.

그런 프레임워크의 개념 하나하나는 너무 쉽게 얻어지는 정보입니다. Spring 같이 레퍼런스가 많은 프레임워크를 사용할 때 오는 장점 중 하나죠.

1️⃣ 인증 과정 생각하기

더 중요한 것은, 인증-인가를 할 때 어떤 과정이 있어야하는지 입니다.

  1. 유저가 아이디와 비밀번호으로 회원가입을 해야하고
  2. 유저가 1번에서 입력한 아이디와 비밀번호로 로그인을 해야하고
  3. HTTP는 비상태성, 비연결성이니 새로온 요청의 유저가 로그인한 유저인지 알려면 쿠키같은 수단을 써야해!
  4. 쿠키에 담았을 때 보안상 문제점들을 고려해서 어떻게 쿠키를 만들어야겠다!
  5. 이제 로그인을 하면 클라이언트가 갖고 있을 수 있도록 쿠키를 보내주고
  6. 인가처리 시 요청에 있는 쿠키에서 인증정보를 확인하여 통과시키자!
  7. 쿠키에 있는 인증정보로 회원 정보로 파싱해서 컨트롤러에 넘겨주면 더 매끄럽겠네!

2️⃣ 스프링으로 인증 도입해보기

  • 6번과 HTTP Request의 헤더와 바디를 직접 보기 위해서는 **HttpRequest(HttpServletRequest)**를 사용할 수 있구나,
  • 5번과 같이 응답에 특정 쿠키나 헤더를 설정하기 위해서는 **HttpResponse(HttpServletResponse)**를 사용할 수 있구나
  • 6번과 같이 많은 컨트롤러에 진입하기 전� 인증 여부를 판단하는 역할을 부여하기 위해 적절한 수단을 찾으면 그것이 Interceptor 일 것이고,
  • 7번과 같이 토큰 정보를 어플리케이션 로직이 더 다루기 쉽도록 변화하는 역할을 하기 위해서 ArgumentResolver라는 개념을 사용할 수 있겠죠.

제 생각엔 1️⃣번이 생각이 된다면, 2️⃣번을 찾아나가는 것은 어려운 일은 아닐 것이라고 생각해요.
적절히 프레임워크에서 제공하는 방식을 따라가는 것도 좋은 타협이지만, 스스로 생각한 방식을 어떻게하면 프레임워크의 도움을 받아서 할 수 있을까 Top-Down으로 학습해 나가시면 좋겠습니다.

뭔가 원리를 모르는데서 오는 답답함이 정말 공감이 되는데요.
(저는 개발자가 되기 전엔 물리학과 출신이라 평생 공부를 Bottom-Up으로만 해봤어요..ㅋ)
우리는 프레임워크를 잘다루는 사람이 되기보단 좋은 프로덕트를 만드는 사람들이라고 생각하고 학습 임하시면 더 동기부여가 되실거라고 생각합니다.


상희님 이번에 리뷰를 하루나 밀려버렸네요.

일단 기한도 많이 있고, 깊게 얘기 나누기에 너무 좋은 주제라서 RC 드리겠습니다.
이번 미션이 조금 학습적인 성향이 강해서 모두가 비슷하게 코드를 작성하였지만, 조금 자유도 있게 리팩토링 해나가면서 의문 많이 던져주시면 좋겠어요.

Comment on lines +1 to +37
# 🌀 Spring MVC (인증)

# 1단계
___
## 로그인
#### 로그인 페이지
+ 이메일, 비밀번호 입력
#### 로그인 요청
+ 이메일, 비밀번호 -> 멤버 조회
+ 조회한 멤버로 토큰 발급
+ Cookie를 만들어 응답
#### 인증 정보 조회
+ Cookie -> 토큰 정보 추출
+ 멤버를 찾아서 응답

로그인 관련 API
+ GET/login : 로그인 페이지 호출
+ POST/login : 로그인 요청
+ GET/login/check : 인증 정보 조회

# 2단계
___
## 로그인 리팩터링
#### HandlerMethodArgumentResolver
+ 컨트롤러 메서드 파라미터로 자동 주입

## 예약 생성 기능 변경
+ 예약 : ReservationRequest(요청 DTO)
-> name이 있으면 name으로 Member 찾기
-> name이 없으면 Cookie에 담긴 정보 활용

# 3단계
___
## 관리자 기능
+ admin 페이지 진입 (HandlerInterceptor 이용)
-> 관리자 : 진입 가능
-> 관리자 X : 401 코드 응답

Choose a reason for hiding this comment

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

[Approve]

깔끔한 요구사항 정리 감사합니다.

Comment on lines +1 to +7
package roomescape.auth;

public record AuthClaims(
String name,
String role
) {
}

Choose a reason for hiding this comment

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

[Comment]

🤔 AuthClaims는 어떤 역할을 하는 객체인가요?

이름으로 유추해봤을 땐, token payload에 들어갈 인증 정보로 판단이 되는데요.

지금 해당 객체가 사용되는 목적은 3가지 정도로 보여집니다.

  1. GET /login/check 로그인 정보 조회 API 응답하는 객체
  2. POST /reservations 쿠키의 토큰에 있는 회원 정보를 담는 객체
  3. class JwtUtils 토큰에서 꺼낸 내용을 래핑하는 객체

각 역할이 충돌하는 바가 있다고 봐요.

2, 3번은 서버에서 회원정보를 파악하기 위해 만든 것입니다.
1번은 요구사항에도 있듯이 회원정보를 띄워주기 위해 존재하는 조금 더 클라이언트가 원하는 정보입니다.

저는 1, 2, 3이 각각 다른 목적으로 사용된다고 보여져요. (최소한 1번과 3번은 달라야한다고 생각합니다.)

이 서로 다른 상황에서 같은 객체로 해결하려 했을 때 어떤 점이 문제가 생길지 고려해보면 좋겠습니다.

중복을 줄이는 것은 개발자라면 누구나 느끼는 마음일텐데요.
SOLID 원칙의 SRP 같은 내용이나, 클린 아키텍처의 독립성(우발적 중복)같은 키워드를 살펴보시면서, 필드가 유사한 객체를 어디까지 분리할지 고려해보면 좋겠어요.

Copy link
Author

Choose a reason for hiding this comment

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

미션을 진행할 때 1번에서 사용할 때 name만 반환하면 되는데 AuthClaims를 모두 반환하는 것이 맞는지에 대해 살짝 고민을 하긴 했었는데요 ㅎㅎ.. 이렇게 사용되는 목적을 직접 보니까 확실히 사용되는 부분이 다른 것이 느껴졌습니다.

사실 지금 JPA 미션 도중에 이 리뷰를 보러 왔는데 JPA 미션이 MVC 미션에서 변경을 하면서 진행을 하는 과정이다보니까 이전 미션의 코드에 영향을 받을 수 밖에 없더라구요..
요구사항이 변경되었을 때 AuthClaims의 경우 다양한 목적을 가진 DTO라서 하나를 변경하면 여러 곳에서 수정을 해야하는 상황이 발생한다는 것을 깨닫고 루카님의 리뷰가 이 지점에서 있었던 것 같아서 다시 읽으러 왔습니다,, ㅎ
중복을 피하려고만 하다가 미션 도중에 직접 문제를 겪어보니 확실히 알겠네요 !

빠른 시일 내에 수정해서 올리겠습니다.

Comment on lines 30 to 37
HttpServletRequest request = ((ServletWebRequest) webRequest).getRequest();
String token = Arrays.stream(request.getCookies())
.filter(cookie -> "token".equals(cookie.getName()))
.findFirst()
.map(Cookie::getValue)
.orElseThrow(() -> new IllegalArgumentException("토큰을 찾을 수 없습니다."));

return memberService.checkLogin(new AuthToken(token));

Choose a reason for hiding this comment

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

[Comment]

HttpRequest -> Cookie -> Jwt -> 인증정보 이렇게 꺼내는 동작이 AutInterceptor에도 동일하게 존재하는데요.
이런 중복은 한번 줄여볼 수 있지 않을까요?

Comment on lines 27 to 30
if (!userClaims.role().equals("ADMIN")) {
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.

[Request Change]

Oh NO...
만약 토큰을 생성할 때, 실수로 "admin"이라고 소문자로 작성하면 인증이 실패하겠어요...
과연 role이 string 타입이여도 괜찮을까요...

Copy link
Author

Choose a reason for hiding this comment

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

오 찾아보니까 말씀해주신 문제(ex : 역할 변경 request에서 잘못된 역할값이 들어왔을 때)를 방지하기 위해서 enum 타입을 활용하는 방법이 있네요

Comment on lines 21 to 38
public AuthToken getToken(LoginRequest loginRequest) {
try {
Member member = memberDao.findByEmailAndPassword(loginRequest.email(), loginRequest.password());
return jwtUtils.createToken(member);
} catch (EmptyResultDataAccessException e) {
throw new IllegalArgumentException("로그인 정보가 잘못되었습니다.");
}
}

public AuthClaims checkLogin(AuthToken userToken) {
try {
AuthClaims userClaims = jwtUtils.getClaimsFromToken(userToken.token());
memberDao.findByName(userClaims.name());
return userClaims;
} catch (EmptyResultDataAccessException exception) {
throw new IllegalArgumentException("존재하지 않는 회원입니다.");
}
}

Choose a reason for hiding this comment

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

[Request Change]

이 두 로직에서 핸들링 되는 예외 상황은 아래와 같습니다.

  1. 로그인 실패 아이디나 비밀번호로 회원 조회가 안될 때
  2. 없는 회원의 인증정보 (jwt에서 내용이 잘 나왔다는 가정하에) 토큰에 있는 인증 정보로 회원 조회가 안될 때

IllegalArgumentException을 뱉어내고, 400 BAD REQUEST로 응답이 갈 것으로 보여집니다.


두가지 고민 포인트가 있겠어요.

  1. 저는 이 값이 처리할 수 없는 형식이라기보단, 부적절한 접근 시도, 혹은 로그인 실패 같은 인증과 관련된 예외상황이라고 생각해요. 그렇다면 어떤 응답이 적절할까요? 그리고 이런 인증관련된 예외는 이곳에서 말고 여러영역(예를 들면 인터셉터, 아규먼트리졸버, ...)에서 발생할 수 있는데요. 이런 아이들을 어떻게 하면 효율적으로 예외 핸들링 할 수 있을까요?
  2. 위의 상황 2번에서 토큰에서 내용이 잘 나왔다고 가정했지만, 서명정보가 불일치하거나 만료되거나 하였으면 예외를 발생할 수 있겠어요. 그 예외에 대해서는 지금 핸들링되고 있지 않아요.

Copy link
Author

Choose a reason for hiding this comment

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

  1. exceptionController를 활용하면 핸들링할 수 있을 것 같습니다.
    인터셉터, 아규먼트 리졸버에서 발생한 예외 -> 컨트롤러 전달 -> @ControllerAdvice로 처리

  2. 넵 처리 완료했습니다!

Comment on lines 23 to 26
@Override
public boolean supportsParameter(MethodParameter parameter) {
return parameter.getParameterType().equals(AuthClaims.class);
}

Choose a reason for hiding this comment

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

[Approve]

Spring을 학습하며 미션에 적용하는게 쉽지 않으실텐데 이렇게 특정 타입의 Argument를 처리할 수 있도록 등록하시다니 대단합니다.

이 방법도 좋은 방법이라고 생각하는데요. 보편적인 방법?전략?을 설명 드리면 ArgumentResolver를 등록할 때 커스텀 어노테이션을 정의하고 해당 어노테이션이 있는지 없는지 체크하는 경우가 많아요.

제 생각엔 특정 객체로 일치를 시키면 해당 객체는 컨트롤러에 아규먼트로 오게되면 그 리졸버를 무조건 타게 되겠지만,
근데 어노테이션으로 목적을 더 명식적으로 표현해서 의도에 더 맞게 사용할 수 있는 것이 장점 같아요.

저희가 애용하는 @PathVariable, @RequestBody 같은 아이들도 스프링에서 기본적으로 등록한 ArgumentResolver에 의해서 매개변수로 들어올 수 있답니다.🥰

Suggested change
@Override
public boolean supportsParameter(MethodParameter parameter) {
return parameter.getParameterType().equals(AuthClaims.class);
}
@Override
public boolean supportsParameter(MethodParameter parameter) {
return parameter.hasParameterAnnotation(�AuthCustomAnnotation.class) &&
parameter.getParameterType().equals(AuthClaims.class);
}

Comment on lines +17 to +22
String accessToken = Jwts.builder()
.setSubject(member.getId().toString())
.claim("name", member.getName())
.claim("role", member.getRole())
.signWith(Keys.hmacShaKeyFor(secretKey.getBytes()))
.compact();

Choose a reason for hiding this comment

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

[Comment]

JWT 토큰을 인코딩할 때 설정하는 정보들은 보안에 매우 중요하다고 볼 수 있겠죠.
조금 더 고민해보면 좋을 것 같아요.

  1. claims 인증 정보의 어떤것을 넣어야할까? (해당 정보를 활용하는 주체가 누군지를 파악하고 고민해보면 좋겠어요.)
  2. token 만료 시간은 얼마나 설정해야할까? (지금 이 토큰은 만료시간이 어떻게 되지? 만약 그렇다면 어떤 위험성이 있지?)
  3. 만료된 토큰으로 인가 요청이 오면 어떻게 해야될까? (만료된 토큰은 비정상 접근일까? 이고민은 코드 반영은 말고 생각만 해보죠)
  4. subject에는 어떤 내용이 들어가야 자연스러운지? (중요하지 않음)

Copy link
Author

Choose a reason for hiding this comment

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

  1. 현재 claims는 서버가 인증 및 권한 검증을 위해 사용하고 있습니다. 인증을 위해서는 토큰 발급 시간과 만료 시간이 추가되면 좋을 것 같네요! 그리고 claims는 최소한의 정보만 넣는 것이 중요하므로 i사용자의 id만 포함하는 것이 보안상 맞는 것 같습니다.
    (사실 저는 미션에서 제공한 견본 코드대로 작성을 했는데요, 미션을 진행하다보니 name으로 찾으라는 요구사항이 많아서 견본 코드를 저렇게 제공해준 것 같아요. 약간 미션에서는 name이 id 같은 역할을 하는 것처럼 요구사항들이 주어진 것 같습니다.)

  2. 현재는 만료 시간이 설정되지 않았는데 이 경우 만약 토큰이 탈취가 되어도 평생 사용된다는 문제점이 있습니다.
    찾아보니까 Access 랑 Refresh 두 가지 토큰을 사용하여 이 문제를 해결할 수 있는 것 같습니다!

보통의 경우
Access Token 만료기간 : 30분 ~ 1시간
Refresh Token 만료기간 : 3일 ~ 1달으로 설정하는 듯 하다.

  1. 401 Unauthorized 응답을 보내는 것이 맞는 것 같습니다.
    또한 만료된 토큰은 세션 만료 상태에서 요청된 상황 (정상), 탈취된 상황 (비정상)
    두 경우 모두 가능하므로 모두 고려하여 대응해야 합니다.

  2. subject는 토큰의 주체를 나타내므로 사용자의 고유 식별자( id, email ) 등을 넣으면 좋을 것 같습니다.

Comment on lines +30 to +34
String token = memberService.getToken(loginRequest).token();
Cookie cookie = new Cookie("token", token);
cookie.setHttpOnly(true);
cookie.setPath("/");
response.addCookie(cookie);

Choose a reason for hiding this comment

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

[Approve]

이번 미션에서 아주 중요한 내용이라고 할 수 있겠죠.

쿠키가 무엇인지,
각각 옵션들이 갖는 의미가 무엇인지 한번 살펴보면 좋겠어요.

쿠키에 대해서 학습하시고, 각 옵션을 설정한 이유에 대해서 저한테 한번 설명해주시면 좋을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

쿠키 :
사용자의 브라우저(로컬 환경)에 저장되는 데이터
ex) 로그인 상태, 사용자 설정, 장바구니 등을 저장하여 다음 요청에서도 이를 활용 가능

사용 이유 :
HTTP의 속성 중 비연결성, stateless를 해결하기 위해서 사용
-> HTTP 통신 간에 유지하려는 정보가 있는 경우 사용

31줄 : 쿠키 생성 key, value값 -> "token", 생성한 token값
32줄 : HTTP만 -> 브라우저(javascript) 접근을 차단함으로써 쿠키에 접근할 수 없도록 제한하는 역할
33줄 : 경로설정을 통해서 설정된 경로와 하위 경로에 대해서만 쿠키에 접근할 수 있게 함
34줄 : HttpServletResponse 객체인 response 즉, 응답에 쿠키를 넣어서 전송

오 사실 견본 코드의 내용이라서 한 번 검색만 해보고 넘겼었는데 이렇게 다시 정리하고 쿠키에 대해서 좀 더 자세히 찾아보니까 사용 목적에 대해 명확하게 알게 된 것 같아서 좋네요!

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.

상희, 안녕하세요.

이번 미션은 충분히 학습할 내용은 하신 것 같아서 이만 Approve 하겠습니다.

늦게 응답드려 죄송해요 ㅠㅠㅠㅠㅠㅠㅠㅠㅠㅠㅠㅠ

Comment on lines 19 to 23
@Override
public boolean supportsParameter(MethodParameter parameter) {
return parameter.getParameterType().equals(AuthClaims.class);
return parameter.hasParameterAnnotation(AuthCustomAnnotation.class) &&
parameter.getParameterType().equals(AuthClaims.class);
}

Choose a reason for hiding this comment

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

👍

Comment on lines +3 to +7
public enum Role {
ADMIN,
USER

}

Choose a reason for hiding this comment

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

👍

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