feat: 사용자 인증 구현

[Choi-JJunho]

🔥 연관 이슈

• close GET /user/student/me #22

🚀 작업 내용

1. UserArgumentResolver를 구현했습니다.

• Controller에서 @StudentAuth 어노테이션과 Studnet 객체를 파라미터로 선언하면 인증 절차를 거쳐 인증된 Student 객체가 나옵니다.

2. 예외처리

• 인증 과정에서 발생하는 예외 상황에서 서버의 에러메시지는 너무 상세하다는 생각이 들었습니다. 이에 CustomException을 구현하여 log로 남길 메시지와 클라이언트에 보여줘야할 예외 메시지를 구분했습니다.
  • 클라이언트에게 보여줘야할 메시지 형태에 대한 ErrorResponse를 생성했습니다.

- code는 현재 활용중이지 않는 것으로 인지하고있어 code는 적용하지 않았습니다. 자세한 내용은 아래 리뷰 중점사항 확인 부탁드립니다.

💬 리뷰 중점사항

CustomException

exception을 구현하는 과정에서 로그에 세부 사항을 남기고자 CustomException 클래스에 withDetail()이라는 static 메소드를 구현했습니다.
이에 GlobalExceptionHandler에서는 각 예외별로 static한 메시지를 관리하도록 구성해봤습니다.
해당 구조에 대한 각자의 의견 부탁드립니다.

ErrorResponse와 error code

에러코드를 현재 사용중인가?, 에러 코드를 정의해야하는 이유에 대한 납득이 잘 안되었습니다. 만약 비즈니스 로직 내에서 모든 상황에 대한 에러코드를 정의해줘야한다면 모든 비즈니스 로직은 예외 결과를 반환하는 View를 알게되는데 과연 올바른 방향인지 고민해볼 필요가 있어보입니다.

[github-actions]

Unit Test Results

11 tests   11 ✔️  5s ⏱️
  6 suites    0 💤
  6 files      0 ❌

Results for commit be0b750.

♻️ This comment has been updated with latest results.

[Invidam]

CustomException

코드 구조는 마음에 듭니다!
여러 종류의 익셉션이 있고 종류에 따라 다른 핸들러 메서드가 처리한다고 이해했는데

(비슷한 로직임에도 모든 핸들러 메서드에서 처리하지 않고) 각각 다른 메서드에서 처리하도록 한 이유가 궁금합니다.. 메서드 마다 다른 처리 로직이 생길 수 있기 때문인가요?

에러 코드

저는 시간이 안될 것 같아서 얘기하기 어려울 것 같아요!

[Invidam]

A

        if (nativeRequest == null) {
          //예외처리
        }
        String request = nativeRequest.getHeader("Authorization");

        if (request == null) {
          //예외처리
        }
        Long userId = jwtProvider.getUserId(request);

        return studentRepository.findById(userId)
            .orElseThrow(() -> UserNotFoundException.witDetail("request: " + request));

같은 흐름은 어떨까요?

[Choi-JJunho]

예측가능한 예외상황을 명시하는게 읽기 더 수월하군요 👍
반영하겠습니다.

[Invidam]

C

Suggested change

	String request = nativeRequest.getHeader("Authorization");
	if (request != null) {
	Long userId = jwtProvider.getUserId(request);
	return studentRepository.findById(userId)
	.orElseThrow(() -> UserNotFoundException.witDetail("request: " + request));
	String authorizationHeader = nativeRequest.getHeader("Authorization");
	if (authorizationHeader != null) {
	Long userId = jwtProvider.getUserId(authorizationHeader);
	return studentRepository.findById(userId)
	.orElseThrow(() -> UserNotFoundException.witDetail("Authorization Header: " + authorizationHeader));

은 어떨까요? getHeader()을 통해 request를 가져오는 게 어색하게 느껴집니다.

[Choi-JJunho]

좋습니다! 추가로 "Authorization" 또한 상수로 추출하여 관리하겠습니다.

[Invidam]

A

사용하지 않는데, 존재하는 이유가 있을까요?

[Choi-JJunho]

오! 자동생성하면서 미처 신경쓰지 못한 부분이였네요!
추후 사용될 여지가 있을 것 같다는 생각도 들지만 필요할때 만들어서 사용하는게 더 나아보이네요

제거하겠습니다~

[Invidam]

C

실 서버 DB에는 외래키가 걸려있지 않은데, JoinColumn을 사용해도 되나요? 저는 외래키가 걸려있는 경우에만 사용한다고 알고 있습니다..

[Choi-JJunho]

와.. FK가 아니였네요 diagram만 보고 속았습니다...
데이터 정합성은 어디로... 배신감드네요

날카로운 지적 감사합니다

[Invidam]

A

기본키가 다른 테이블의 외래키인 경우 JoinColumn 대신해서 PrimaryKeyJoinColumn을 사용할 수도 있는 것 같네요

(큰 이점이 있는 진 모르겠어요)

참고: https://codingexplore.tistory.com/68

[Choi-JJunho]

흠... 큰 효용성은 잘 모르겠어서 일단 JoinColumn만 제거하는 방향으로 진행하겠습니다.

[Invidam]

C

사용한 이유가 있나요?? 사용처가 없다면 제거해도 될 것 같아요~

Suggested change

@Slf4j

[Choi-JJunho]

로그찍어본다고 써뒀던거같은데 깜빡했군요
감사합니다!

[Invidam]

A

메서드들 형태가 비슷한데, 하나의 메서드에서 처리하는 건 어떻게 생각하세요?

(이렇게 해야한다는 의견은 아닌데, 코인에서는 하나의 핸들러메서드에서 처리했어서 의견이 궁금합니다. 현재 코드 생김새는 비슷하지만, 항상 비슷한 건 아니라고 봐야할까요..?)

[Choi-JJunho]

예외별로 처리하는 HttpStatus가 다르다는 이유가 가장 컸습니다.

이러한 구조에는 다음과 같은 생각의 흐름이 있었습니다.

1. Exception 내부에 HttpStatus를 선언한다면 비즈니스 로직 상에서 클라이언트에게 반환해야하는 코드까지 알고 신경써야하기 때문에 좋지 않은 선택이다.
2. 비즈니스 로직 상에서는 Exception만 적절히 선택하고 예외메시지를 구체적으로 작성하여 디버깅에 도움을 준다.
3. 해당 예외가 어떤 상태코드를 던져야하는지는 Handler에서 알아서 정한다.

이에 예외별로 Http Status Code를 다르게 던진다는 생각으로 메소드로 분리해서 관리하도록 구성했습니다.
추후 각각의 예외상황에 클라이언트의 어떤 추가요청이 있을지 모르겠다는 생각 또한 메소드 분리를 선택하게 된 이유였습니다.

[Invidam]

이해했습니다~ 서비스에서 http 상태코드를 몰라도된다는 점도 있었군요~

[songsunkook]

A

개선된 switch문의 패턴 매칭 기능을 사용하면 하나의 메서드 안에 간결하게 작성할 수 있지 않을까 했는데 자바 21버전부터 도입되는 기능이었네요.. ㅠㅠ

호불호 갈리는 switch문이지만 저는 이렇게 짜보면 좋을 것 같다고 생각했었습니다.

    @ExceptionHandler
    public ResponseEntity<ErrorResponse> exceptioinHandler(Exception exception) {
        log.warn(exception.getMessage());
        return switch (exception) {
            case MethodArgumentNotValidException e -> new ResponseEntity<>(ErrorResponse.from(e.getMessage()), HttpStatus.BAD_REQUEST);
            case IllegalArgumentException e -> new ResponseEntity<>(ErrorResponse.from("잘못된 요청입니다."), HttpStatus.BAD_REQUEST);
            case UserNotFoundException e -> new ResponseEntity<>(ErrorResponse.from(e.getMessage()), HttpStatus.NOT_FOUND);
            case AuthException e -> new ResponseEntity<>(ErrorResponse.from(e.getMessage()), HttpStatus.UNAUTHORIZED);
        };
    }

참고: https://belief-driven-design.com/looking-at-java-21-switch-pattern-matching-14648/

[Invidam]

A

널 검사 하는 이유가 있나요? 저는 한 번도 안했는데..

[Choi-JJunho]

accessToken은 별도로 저장을 안하기 때문에 token값을 정확하게 체크하기가 어렵다고 판단했습니다.
이에 null이 아니다 == 토큰이 정상적으로 생성되었다 라고 생각하고 위와같은 테스트를 짰습니다.

[Invidam]

액세스 토큰을 검사하는 이유는 이해했습니다! 리프레시토큰에 널검사를 한는 이유는 인메모리 db에 키-값 형태로 저장되었기 때문인가요??

[Choi-JJunho]

리프레시에대한 설명이 부족했군요!! 😅

바로 아래줄의

softly.assertThat(response.jsonPath().getString("refresh_token")).isEqualTo(token.getRefreshToken());

구문에서 인메모리 DB에 저장된 토큰값을 가지고 비교하고있긴 합니다.
(UserToken token = tokenRepository.findById(userResult.getId()).get(); 로 가져온 값)

모종의 이유로 인메모리 DB에 RefreshToken이 Null로 저장될 경우를 생각했습니다.
이 경우 Null check를 하지 않으면 refreshToken이 null로 반환되어도 정상적인 시나리오로 판별되기 때문에 .isNotNull() 조건을 추가했습니다.

[Invidam]

👍

[Invidam]

인증은 중요한 절차이니 여러 실패케이스에 대해서도 테스트하는 건 어떨까요?

• Payload를 수정한 경우
• 유효하지 않은 키로 생성한 토큰인 경우
  등등..

[Invidam]

에러 코드 관련해서는

현재 클라이언트 측에서 크게 활용하지 않는다고 알고 있고, (관련해서 언급된 적이 한 번도 없어요)
별도 엑셀문서까지 작성해야해서 번거로움이 있었던 경험이 있습니다.

[songsunkook]

인증 부분은 구현하기 힘들었을 것 같은데 고생하셨습니다!
메서드 네이밍에 오타가 있는 것 같아서 그 부분만 수정 부탁드립니다!

[songsunkook]

R

Suggested change

	public static AuthException witDetail(String detail) {
	public static AuthException withDetail(String detail) {

네이밍에 오타가 난 것 같습니다!

[songsunkook]

R

여기도 네이밍 오타가 있는 것 같습니다!

[songsunkook]

A

현재까지는 code를 수정하는 곳이 없어서 에러 발생 시 항상 code 0을 반환하는 것으로 보이는데, 혹시 해당 변수의 의미가 무엇인지 알 수 있을까요??

작업 내용에 써있었네요 죄송합니다!

[songsunkook]

A

개선된 switch문의 패턴 매칭 기능을 사용하면 하나의 메서드 안에 간결하게 작성할 수 있지 않을까 했는데 자바 21버전부터 도입되는 기능이었네요.. ㅠㅠ

호불호 갈리는 switch문이지만 저는 이렇게 짜보면 좋을 것 같다고 생각했었습니다.

    @ExceptionHandler
    public ResponseEntity<ErrorResponse> exceptioinHandler(Exception exception) {
        log.warn(exception.getMessage());
        return switch (exception) {
            case MethodArgumentNotValidException e -> new ResponseEntity<>(ErrorResponse.from(e.getMessage()), HttpStatus.BAD_REQUEST);
            case IllegalArgumentException e -> new ResponseEntity<>(ErrorResponse.from("잘못된 요청입니다."), HttpStatus.BAD_REQUEST);
            case UserNotFoundException e -> new ResponseEntity<>(ErrorResponse.from(e.getMessage()), HttpStatus.NOT_FOUND);
            case AuthException e -> new ResponseEntity<>(ErrorResponse.from(e.getMessage()), HttpStatus.UNAUTHORIZED);
        };
    }

참고: https://belief-driven-design.com/looking-at-java-21-switch-pattern-matching-14648/

[songsunkook]

CustomException

exception을 구현하는 과정에서 로그에 세부 사항을 남기고자 CustomException 클래스에 withDetail()이라는 static 메소드를 구현했습니다.
이에 GlobalExceptionHandler에서는 각 예외별로 static한 메시지를 관리하도록 구성해봤습니다.
해당 구조에 대한 각자의 의견 부탁드립니다.

실제로 실행해보니 디버깅용 메시지와 응답 메시지를 분리한 것이 실용적으로 보였습니다. 좋은 아이디어인 것 같아요!

ErrorResponse와 error code

에러코드를 현재 사용중인가?, 에러 코드를 정의해야하는 이유에 대한 납득이 잘 안되었습니다. 만약 비즈니스 로직 내에서 모든 상황에 대한 에러코드를 정의해줘야한다면 모든 비즈니스 로직은 예외 결과를 반환하는 View를 알게되는데 과연 올바른 방향인지 고민해볼 필요가 있어보입니다.

말씀하신 내용을 들어보니 확실히 비즈니스 로직 내에서 에러 코드를 정의하는 부분은 적절하지 않아 보입니다. 익셉션 핸들러에서 익셉션의 종류에 따라 에러 코드를 정리해주면 좋겠다고 생각했는데 이미 그렇게 구현하셨더라구요..!! 저는 이대로 진행하는 것이 좋아 보입니다.

[daheeParkk]

수고하셨습니다!!

exception을 구현하는 과정에서 로그에 세부 사항을 남기고자 CustomException 클래스에 withDetail()이라는 static 메소드를 구현했습니다.
이에 GlobalExceptionHandler에서는 각 예외별로 static한 메시지를 관리하도록 구성해봤습니다.
해당 구조에 대한 각자의 의견 부탁드립니다.

에러의 자세한 부분은 로그로만 남겨서 클라이언트 쪽에 과도한 정보가 넘어가지 않아 좋은 것 같습니다!

에러코드를 현재 사용중인가?, 에러 코드를 정의해야하는 이유에 대한 납득이 잘 안되었습니다. 만약 비즈니스 로직 내에서 모든 상황에 대한 에러코드를 정의해줘야한다면 모든 비즈니스 로직은 예외 결과를 반환하는 View를 알게되는데 과연 올바른 방향인지 고민해볼 필요가 있어보입니다.

에러 코드는 클라이언트 쪽에서 사용할 것 같지도 않고, 알 필요도 없어보입니다.
프론트엔드에서 사용하지 않는다면 반환하지 않아도 될 것 같습니다!

[daheeParkk]

A

"잘못된 로그인 정보입니다" -> "비밀번호가 틀립니다" 로 하는건 어떤가요?
많아질 수도 있고 로그니까 자세히 적어야 안 헷갈리고 좋을 것 같아서요..!

[Choi-JJunho]

클라이언트에게 주는 정보가 아닌 디버깅용이라고 생각했을때 충분이 타당한 의견이라고 생각되네요~!
반영하겠습니다

[daheeParkk]

C

thows Exception 제거해줘도 괜찮을 것 같아요!

[Choi-JJunho]

override하면서 자동으로 생겼는데 사용되지 않는군요!
제거하겠습니다~

[daheeParkk]

C

두 메서드는 사용하고 있지 않은데, 있는 이유가 있는 건가요??

[Choi-JJunho]

오? 테스트 끄적이다 만들었던것 같은데 안쓰는 상태로 남았군요..
제거하겠습니다 감사합니다!