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 지하철 경로 조회 - 1,2단계] 현구막(최현구) 미션 제출합니다. #81

Merged
merged 14 commits into from
May 17, 2021

Conversation

Hyeon9mak
Copy link
Member

안녕하세요 재연링! 현구막이라고 합니다!

계정 인증 방법에 대해서 쿠키와 세션에 대해서만 얄팍하게 알고 있었는데, 이번 미션을 통해서 토큰에 대해 배우고 JWT 사용도 해보았습니다!
API가 굉장히 잘 만들어져있어서 무언갈 고민한다기보다... 시키는대로 하면 되는? 그런 미션이어서 페어 마크랑 함께 당황했네요 😅
크루 웨지가 '리모컨을 누르는거 같다' 라는 비유를 했다는데, 굉장히 공감하고 있습니다.

1. vue에서 별도로 auth accessToken을 관리하는 방법이 있을까요?

https://han-um.tistory.com/17
블로그에 나와있는 방법을 참고해서 localStorage 쪽에 accessToken을 보관했습니다.
그런데 vue 코드가 담긴 프로젝트를 더 살펴보니 auth accessToken을 관리하는 모듈이 있는 것 같더라구요...
vue는 다른 프레임워크들과 다르게 auth accessToken을 관리하는 방법을 제공하는것인가요??

2. @RestControllerAdvice vs @ResponseStatus

@RestControllerAdvice 어노테이션을 통해서 exception들을 핸들링 하곤 했는데, 대부분 ResponseEntity로 Http 상태코드만 반환하곤 했습니다. 그러다 오늘 @ResponseStatus 라는 어노테이션을 알게되었는데, exception 클래스 위에 붙여서 자동으로 Http 상태코드를 반환할 수 있는 것 같더라구요!

별도로 Advice 클래스를 만들지 않아도 자동으로 Http 상태코드 response를 반환해준다는게 굉장한 장점인 것 같은데, 현업에서는 어떤 방식이 더 선호되는지 궁금합니다!


리뷰 잘 부탁드리겠습니다!
감사합니다! 🙇‍♂️

Copy link

@jaeyeonling jaeyeonling left a comment

Choose a reason for hiding this comment

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

안녕하세요 현구막! 지하철 경로 조회 미션을 함께 할 재연링입니다 😄
요구사항에 맞게 잘 구현해주셨습니다 💯
소소한 피드백 남겨두었으니 확인 부탁드려요!

Comment on lines 104 to 106
email: email,
age: age,
password: password

Choose a reason for hiding this comment

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

Suggested change
email: email,
age: age,
password: password
email,
age,
password

변수의 이름과 값이 동일할 경우 위와 같이 적용 가능합니다 : )

Copy link
Member Author

Choose a reason for hiding this comment

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

와우!! 감사합니다!! 재연링은 JS도 빠삭하시군요..

// const member = wait fetch("/members/me")
// this.setMember(member);
const { email, password } = this.member;
const data = await fetch("http://localhost:8080/login", {

Choose a reason for hiding this comment

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

스키마, 도메인, 포트(http://localhost:8080)를 변경할 경우 모든 url을 수정해줘야 하는 문제가 있네요!
또한 API 요청 코드에 중복되는 정보가 많은 것 같은데, 해당 기능을 메서드로 분리해보면 좋을 것 같아요~

Copy link
Member Author

@Hyeon9mak Hyeon9mak May 16, 2021

Choose a reason for hiding this comment

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

스키마, 도메인, 포트(http://localhost:8080)를 변경할 경우 모든 url을 수정해줘야 하는 문제가 있네요!

https://velog.io/@skyepodium/vue-proxy-%EC%82%AC%EC%9A%A9%ED%95%98%EA%B8%B0
위 블로그를 참고해서 http://localhost:8080을 호출하는 개발서버 프록시를 vue.config.js 파일에 등록했습니다!

또한 API 요청 코드에 중복되는 정보가 많은 것 같은데, 해당 기능을 메서드로 분리해보면 좋을 것 같아요~

API를 요청하는 fetch 메서드를 분리하면 어느 쪽에 포함시킬지 고민이었는데, /frontend/src/utils 디렉토리를 발견해서 그 쪽으로 분리시켰습니다!

'email': email,
'password': password
})
}).then(response => response.json());

Choose a reason for hiding this comment

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

로그인 실패 시 예외처리는 필요 없을까요!?

Copy link
Member Author

@Hyeon9mak Hyeon9mak May 16, 2021

Choose a reason for hiding this comment

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

성공하는 것만 고려한 코드였네요... response의 상태코드가 200(ok)인지 확인하는 것으로 예외처리를 진행했습니다!

'password': password
})
}).then(response => response.json());
localStorage.setItem("Authorization", data.accessToken);

Choose a reason for hiding this comment

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

Authorization 이라는 Key 값을 상수로 분리한다면, setItem과 getItem에서 오타로 인해 실수가 발생하는걸 방지할 수 있을 것 같습니다 :)

Copy link
Member Author

Choose a reason for hiding this comment

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

기존 존재하던 /frontent/src/utils/constants.js 파일에 상수화 시켰습니다!

'Authorization': `Bearer ${localStorage.getItem('Authorization')}`
}
})
localStorage.removeItem("Authorization");

Choose a reason for hiding this comment

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

스토리지 정리까지 👍

@RestControllerAdvice
public class GlobalExceptionHandler {

@ExceptionHandler(UnauthorizedException.class)

Choose a reason for hiding this comment

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

Q. @RestControllerAdvice 어노테이션을 통해서 exception들을 핸들링 하곤 했는데, 대부분 ResponseEntity로 Http 상태코드만 반환하곤 했습니다. 그러다 오늘 @ResponseStatus 라는 어노테이션을 알게되었는데, exception 클래스 위에 붙여서 자동으로 Http 상태코드를 반환할 수 있는 것 같더라구요!

별도로 Advice 클래스를 만들지 않아도 자동으로 Http 상태코드 response를 반환해준다는게 굉장한 장점인 것 같은데, 현업에서는 어떤 방식이 더 선호되는지 궁금합니다!

A. 당연한 얘기일 수 있지만 프로젝트 성격과 프로젝트 참여하는 사람 취향에 따라 달라질 것 같아요! 섞어서 쓰는 경우도 많을 것 같고요!
예시를 전달하기 위해 저희 서비스를 말씀드리면 예외에 따라 Body와 Header가 크게 변경되는 부분이 있고, 예외 상황에 따라 별도의 비즈니스 로직을 처리하는 경우가 있어 대부분은 ExceptionHandler를 사용하는 것 같습니다!

답변드리며 든 생각인데..! ExceptionHandler와 ResponseStatus를 섞어쓰는 경우는 있었어도, ResponseStatus 만으로 서비스를 했던 기억은 없네요 ㅎㅎ

Copy link
Member Author

Choose a reason for hiding this comment

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

감사합니다! 앞으로 미션에서 2가지 모두 섞어서 사용해보겠습니다 👍 👍 👍

return new TokenResponse(jws);
}

private void validatePassword(String password, String savedPassword) {

Choose a reason for hiding this comment

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

추후 암호화 대비하여 쿼리가 아닌 로직으로 패스워드 검증 💯
해당 로직을 Service가 아닌 Domain(Member)로 옮긴다면 어떠한 장점이 있을지 고민해보아요

Copy link
Member Author

Choose a reason for hiding this comment

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

AuthService도 결국 서비스 계층 중 하나니까, 서비스가 간결해지고 도메인이 풍부해질거같아요.
거기다 현재 validatePassword 메서드는 이메일로 찾아낸 Member 객체에서 Password를 getter로 뽑아와서 검증하는 형태를 가지고 있습니다. 객체에 메세지를 보내지 못하고 있는거 같아요.


validatePassword(password, member.getPassword());

String jws = jwtTokenProvider.createToken(String.valueOf(member.getId()));

Choose a reason for hiding this comment

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

Suggested change
String jws = jwtTokenProvider.createToken(String.valueOf(member.getId()));
String jwt = jwtTokenProvider.createToken(String.valueOf(member.getId()));

JWT는 JWS 이외에 다른 형태로 존재할 수 있어 위와 같은 네이밍으로 작성했다고 생각이 듭니다
지금 JWT는 JWS의 구조를 가지고 있는 것은 맞지만, 이는 JWS Compact Serialization의 형태를 띄는 것일 뿐 인증 토큰이라는 의미는 변하지 않습니다
JWS는 암호화를 통해 토큰을 보호하는 JOSE 스펙 중 하나로 추후 JWS 구조가 아닌 JWE 구조로 변경됐을 때 해당 네이밍이 jwe로 바뀌는 것 또한 맞는가? 라는 의문이 있습니다
즉, JWT는 인증 토큰의 형식을 나타내는 의미에 가깝지만 JWS는 인증의 형태와 방식을 나타내는 의미에 가깝기 때문에 jwt라는 네이밍이 더욱 적절하지 않을까 싶은데 현구막의 생각은 어떠신가요 ㅎㅎ

해당 리뷰는 사소한 내용으로 넘어갈 수 있지만 공부할 키워드를 전달하기 위해 길게 남겨보았습니다~!

Copy link
Member Author

Choose a reason for hiding this comment

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

말씀해주신 부분을 보고 곰곰히 생각해보니 자바에서 ArrayList를 선언할 때가 떠오르네요.

ArrayList<Integer> numbers = new ArrayList<>();
List<Integer> numbers = new ArrayList<>();
List<Integer> numbers = new LinkedList<>();

게다가 보안 때문에 사용되는 토큰에서 JWS의 이름으로 인증 형태와 방식을 나타낸다는 것은
인증 형태와 방식을 노출 한다는 것과 같은 의미인거 같아요!

method: 'GET',
headers: {
'Content-Type': 'application/json',
'Authorization': `Bearer ${localStorage.getItem("Authorization")}`

Choose a reason for hiding this comment

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

Q. vue에서 별도로 auth accessToken을 관리하는 방법이 있을까요?

https://han-um.tistory.com/17

블로그에 나와있는 방법을 참고해서 localStorage 쪽에 accessToken을 보관했습니다.
그런데 vue 코드가 담긴 프로젝트를 더 살펴보니 auth accessToken을 관리하는 모듈이 있는 것 같더라구요...
vue는 다른 프레임워크들과 다르게 auth accessToken을 관리하는 방법을 제공하는것인가요??

A. vue이기 떄문에 존재한다기 보다는, 프레임워크나 라이브러리에 따라 해당 기능을 지원해줄 수 있을 것 같습니다
vue 이외에 react, angular 등도 동일한 기능을 제공할 수 있을 것 같고, 프레임워크에서 지원하지 않는다면 라이브러리를 통해 지원을 받을 수도 있을 것 같아요 ㅎㅎ
예를 들어 Http Client인 axios 라이브러리를 사용할 경우 동일한 기능을 제공받을 수 있겠네요!

하지만 이러한 기능을 어렵게 생각할 필요 없이, 현구막이 http 요청을 하는 메서드를 분리하여 localStorage에 토큰이 존재할 시 header에 토큰을 포함해주는 기능을 만든다면 동일한 결과를 볼 수 있지 않을까 싶습니다 :)

Copy link
Member Author

Choose a reason for hiding this comment

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

하지만 이러한 기능을 어렵게 생각할 필요 없이, 현구막이 http 요청을 하는 메서드를 분리하여 localStorage에 토큰이 존재할 시 header에 토큰을 포함해주는 기능을 만든다면 동일한 결과를 볼 수 있지 않을까 싶습니다 :)

감사합니다! 유틸 클래스 파일에 별도로 분리해보았습니다!

@Hyeon9mak
Copy link
Member Author

Hyeon9mak commented May 16, 2021

안녕하세요 재연링!! 피드백 주신 부분들을 모두 적용해보았습니다!
피드백을 적용하는 동안 궁금한 점이 몇 가지 생겼어요!!

1. 로그인 실패시 예외처리

기존 코드의 try-catch 키워드에서 사용자에게 요청이 실패했음을 알려주고 있었어요!

  try {
    ...
    this.showSnackbar(SNACKBAR_MESSAGES.LOGIN.SUCCESS);
  } catch (e) {
    this.showSnackbar(SNACKBAR_MESSAGES.LOGIN.FAIL);
    throw new Error(e);
  }

거기다 자바스크립트에 익숙하지 않다보니, 추가 예외처리는 무엇을 위함인지 잘 모르겠습니다.. 😰
로그인 실패시 넘어오는 상태코드를 잡아내도록 유도하신게 맞을까요...?!
아니면 제가 try-catch 부분 코드를 잘못 이해하고 있는걸까요???

2. 인터셉터?

크루들과 이야기를 나누다보니 인터셉터를 적용한 크루들이 많더라구요.
"인터셉터는 인가를 결정하는 단계에서 유용하게 사용된다." 라고 얼핏 이야기를 들은거 같은데,
정확하게 와닿는 사용하는 이유가 와닿지 않습니다..
혹시 참고할만한 문서나 자료 추천 부탁드려도 괜찮을까요...?!

3. 호옥시... 3단계에서는..

프론트는 너무 방대해서 어지럽고, 백엔드는 제 코드가 아니고 제공된 코드라 눈에 잘 안익고... 😭
여러가지로 아쉬움이 많은 미션인데..
혹시 3단계에서는 atdd-subway-map(현재 미션은 atdd-subway-path)에서 구현한 백엔드 코드로 리뷰를 부탁드려도 괜찮을까요?!


이번 피드백도 잘 부탁드리겠습니다!! 🙇‍♂️ 🙇‍♂️ 🙇‍♂️

Copy link

@jaeyeonling jaeyeonling left a comment

Choose a reason for hiding this comment

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

코멘트를 잘 반영해주셨습니다 👍
질문의 답변 남겨두었으니 확인 부탁드리고 다음 단계 진행해주세요!

const response = await fetchJsonWithBody('/api/members', FETCH_METHODS.POST, this.member);
if (!response.ok) {
throw new Error(`${response.status}`);
}
this.showSnackbar(SNACKBAR_MESSAGES.COMMON.SUCCESS);
await this.$router.replace(`/login`);
} catch (e) {

Choose a reason for hiding this comment

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

Q. 로그인 실패시 예외처리
기존 코드의 try-catch 키워드에서 사용자에게 요청이 실패했음을 알려주고 있었어요!

try {
...
this.showSnackbar(SNACKBAR_MESSAGES.LOGIN.SUCCESS);
} catch (e) {
this.showSnackbar(SNACKBAR_MESSAGES.LOGIN.FAIL);
throw new Error(e);
}
거기다 자바스크립트에 익숙하지 않다보니, 추가 예외처리는 무엇을 위함인지 잘 모르겠습니다.. 😰
로그인 실패시 넘어오는 상태코드를 잡아내도록 유도하신게 맞을까요...?!
아니면 제가 try-catch 부분 코드를 잘못 이해하고 있는걸까요???

A. 지금은 서버에서 전달하는 예외 메시지가 아닌 하드코딩 된 정보를 사용하고 있는데요!
클라이언트에 잘못이 아닌 서버 내부의 문제일수도 있고, 서버에서 유용한 정보를 전달해줄 수 있는데 서버에서 전달한 메시지를 쓸 필요가 없다는 의미였습니다 ㅎㅎ
해당 부분은 중요도가 높지 않아 넘어가셔도 될 것 같아요 :)

@@ -20,10 +26,11 @@ public boolean supportsParameter(MethodParameter parameter) {
return parameter.hasParameterAnnotation(AuthenticationPrincipal.class);
}

// parameter에 @AuthenticationPrincipal이 붙어있는 경우 동작
@Override
public Object resolveArgument(MethodParameter parameter, ModelAndViewContainer mavContainer, NativeWebRequest webRequest, WebDataBinderFactory binderFactory) {

Choose a reason for hiding this comment

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

Q. 인터셉터?
크루들과 이야기를 나누다보니 인터셉터를 적용한 크루들이 많더라구요.
"인터셉터는 인가를 결정하는 단계에서 유용하게 사용된다." 라고 얼핏 이야기를 들은거 같은데,
정확하게 와닿는 사용하는 이유가 와닿지 않습니다..
혹시 참고할만한 문서나 자료 추천 부탁드려도 괜찮을까요...?!

A. ArgumentResolver는 말 그대로 매개변수의 리졸빙을 담당하는 역할을 해야 합니다
즉, 인증에 대한 부분은 다른 객체에게(ex. AuthInterceptor) 넘겨줄 수 있을 것 같은데요!
그 이유에 대해 생각해본다면

  1. 인증이 필요한 기능이지만 LoginMember(AuthenticationPrincipal)가 필요 없을 수 있다.
  2. 말씀하신 것 처럼 인가에 대한 처리가 힘들다.

라고 볼 수 있을 것 같은데요

2번의 예시를 이해하는 것이 어려우신 것 같아 스프링 시큐리티에서 검증할 수 있는 예시 코드를 추가드립니다~

@GetMapping("/any")
void any() { }

@Secured("ROLE_USER")
@GetMapping("/user")
void user() { }

@Secured("ROLE_ADMIN")
@GetMapping("/admin")
void admin() { }

설명이 필요하거나 추가적으로 궁금하신 부분이 있다면 DM 주세요 😄

import wooteco.subway.auth.exception.UnauthorizedException;
import wooteco.subway.auth.infrastructure.JwtTokenProvider;
import wooteco.subway.member.dao.MemberDao;
import wooteco.subway.member.domain.Member;

@Service
public class AuthService {

Choose a reason for hiding this comment

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

Q. 호옥시... 3단계에서는..
프론트는 너무 방대해서 어지럽고, 백엔드는 제 코드가 아니고 제공된 코드라 눈에 잘 안익고... 😭
여러가지로 아쉬움이 많은 미션인데..
혹시 3단계에서는 atdd-subway-map(현재 미션은 atdd-subway-path)에서 구현한 백엔드 코드로 리뷰를 부탁드려도 괜찮을까요?!

A. 물론입니다 👍

@jaeyeonling jaeyeonling merged commit 7d41d53 into woowacourse:hyeon9mak May 17, 2021
@Hyeon9mak
Copy link
Member Author

Hyeon9mak commented May 19, 2021

학습로그

@Valid 어노테이션과 사용 가능한 종류

Anotation 제약조건
@NotNull Null 불가
@null Null만 입력 가능
@notempty Null, 빈 문자열 불가
@notblank Null, 빈 문자열, 스페이스만 있는 문자열 불가
@SiZe(min=,max=) 문자열, 배열등의 크기가 만족하는가?
@pattern(regex=) 정규식을 만족하는가?
@max(숫자) 지정 값 이하인가?
@min(숫자) 지정 값 이상인가
@future 현재 보다 미래인가?
@past 현재 보다 과거인가?
@positive 양수만 가능
@PositiveOrZero 양수와 0만 가능
@Negative 음수만 가능
@NegativeOrZero 음수와 0만 가능
@Email 이메일 형식만 가능
@digits(integer=, fraction = ) 대상 수가 지정된 정수와 소수 자리 수 보다 작은가?
@DecimalMax(value=) 지정된 값(실수) 이하인가?
@DecimalMin(value=) 지정된 값(실수) 이상인가?
@assertfalse false 인가?
@AssertTrue true 인가?

태그


JS에서 JSON의 키와 값이 일치하는 경우 생략 가능

JSON 자료구조에 사용되는 Key와 Value가 완전히 동일한 경우 아래와 같이 생략이 가능하다.

JSON.stringify({
    email: email,
    age: age,
    password: password
})
JSON.stringify({
    email,
    age,
    password
})

태그

  • JS, Javascript, 자바스크립트, JSON, JSON stringfy

Exception Handling

Q. @RestControllerAdvice 어노테이션을 통해서 exception들을 핸들링 하곤 했는데, 대부분 ResponseEntity로 Http 상태코드만 반환하곤 했습니다. 그러다 오늘 @ResponseStatus 라는 어노테이션을 알게되었는데, exception 클래스 위에 붙여서 자동으로 Http 상태코드를 반환할 수 있는 것 같더라구요!
별도로 Advice 클래스를 만들지 않아도 자동으로 Http 상태코드 response를 반환해준다는게 굉장한 장점인 것 같은데, 현업에서는 어떤 방식이 더 선호되는지 궁금합니다!

A. 당연한 얘기일 수 있지만 프로젝트 성격과 프로젝트 참여하는 사람 취향에 따라 달라질 것 같아요! 섞어서 쓰는 경우도 많을 것 같고요!
예시를 전달하기 위해 저희 서비스를 말씀드리면 예외에 따라 Body와 Header가 크게 변경되는 부분이 있고, 예외 상황에 따라 별도의 비즈니스 로직을 처리하는 경우가 있어 대부분은 ExceptionHandler를 사용하는 것 같습니다!
답변드리며 든 생각인데..! ExceptionHandler와 ResponseStatus를 섞어쓰는 경우는 있었어도, ResponseStatus 만으로 서비스를 했던 기억은 없네요 ㅎㅎ

@ControllerAdvice만 쓰거나 @ResponseStatus를 섞어쓰는 경우는 있어도, @ResponseStatus만 사용하는 경우는 없다.

태그

  • @ControllerAdvice, @RestControllerAdvice, @ResponseStatus, Exception Handle, Http Status

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