6주차 과제 - 로그인 만들기 #81
Merged
6주차 과제 - 로그인 만들기 #81
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hannut91
reviewed
Jul 15, 2023
Comment on lines
+15
to
+16
| private static final String VALID_TOKEN = "eyJhbGciOiJIUzI1NiJ9.eyJ1c2VySWQiOjF9.neCsyNLzy3lQ4o2yliotWT06FwSGZagaHpKdAkjnGGw"; | ||
| private static final String INVALID_TOKEN = "eyJhbGciOiJIUzI1NiJ9.eyJ1c2VySWQiOjF9.neCsyNLzy3lQ4o2yliotWT06FwSGZagaHpKdAkjnGGK"; |
There was a problem hiding this comment.
Suggested change
| private static final String VALID_TOKEN = "eyJhbGciOiJIUzI1NiJ9.eyJ1c2VySWQiOjF9.neCsyNLzy3lQ4o2yliotWT06FwSGZagaHpKdAkjnGGw"; | |
| private static final String INVALID_TOKEN = "eyJhbGciOiJIUzI1NiJ9.eyJ1c2VySWQiOjF9.neCsyNLzy3lQ4o2yliotWT06FwSGZagaHpKdAkjnGGK"; | |
| private static final String VALID_TOKEN = "eyJhbGciOiJIUzI1NiJ9.eyJ1c2VySWQiOjF9.neCsyNLzy3lQ4o2yliotWT06FwSGZagaHpKdAkjnGGw"; | |
| private static final String INVALID_TOKEN = VALID_TOKEN + "WRONG"; |
이렇게 표현할 수도 있을 것 같아요.
혹은 이런 토큰들은 값만 봐서는 검증이 어렵기 때문에 테스트에서는 그냥 믿고 사용하는 데이터 입니다. 그래서 fixture로 분리해도 좋을 것 같아요
Comment on lines
76
to
84
| @Test | ||
| void parseTokenWithInvalidToken() { | ||
| assertThatThrownBy(() -> authenticationService.parseToken("")) | ||
| .isInstanceOf(InvalidAccessTokenException.class); | ||
|
|
||
| assertThatThrownBy(() -> authenticationService.parseToken(INVALID_TOKEN)) | ||
| .isInstanceOf(InvalidAccessTokenException.class); | ||
|
|
||
| } |
There was a problem hiding this comment.
잘못된 토큰의 종류를 여기서 보여줄 수 있겠네요. Parameterized 테스트를 사용해보면 좋을 것 같아요
https://junit.org/junit5/docs/current/user-guide/#writing-tests-parameterized-tests
Comment on lines
24
to
45
| public String login(UserLoginData userLoginData){ | ||
| User user = userRepository.findByEmail(userLoginData.getEmail()).orElseThrow(() -> new LoginFailException()); | ||
|
|
||
| if(!user.getPassword().equals(userLoginData.getPassword())){ | ||
| throw new LoginFailException(); | ||
| } | ||
|
|
||
| return jwtUtil.encode(user.getId()); | ||
| } | ||
|
|
||
| public Long parseToken(String accessToken) { | ||
| if(accessToken == null || accessToken.isBlank()){ | ||
| throw new InvalidAccessTokenException(accessToken); | ||
| } | ||
|
|
||
| try{ | ||
| Claims claims = jwtUtil.decode(accessToken); | ||
| return claims.get("userId", Long.class); | ||
| }catch (SignatureException e){ | ||
| throw new InvalidAccessTokenException(accessToken); | ||
| } | ||
| } |
There was a problem hiding this comment.
사용자를 조회해서 인증을 하는 부분과 토큰을 처리하는 두 가지 일을 하는 것처럼 보이네요. 뭔가 AuthenticationService가 두 가지 일을 하는 것 같네요.
�만약에 claim에 userId뿐만 아니라 다른 데이터를 추가하기로 했을 때 여기도 변경될 가능성이 높아보이고, login도 비밀번호를 직접 비교하는 것이 아니라 난수화된 상태로 비교하게 된다면 변경될 가능성이 있어보여요
hannut91
reviewed
Jul 16, 2023
Comment on lines
+25
to
+46
| public String login(UserLoginData userLoginData){ | ||
| User user = userRepository.findByEmail(userLoginData.getEmail()).orElseThrow(() -> new UserNotFoundException()); | ||
|
|
||
| if(!user.getPassword().equals(userLoginData.getPassword())){ | ||
| throw new LoginFailException(); | ||
| } | ||
|
|
||
| return jwtUtil.encode(user.getId()); | ||
| } | ||
|
|
||
| public Long parseToken(String accessToken) { | ||
| if(accessToken == null || accessToken.isBlank()){ | ||
| throw new InvalidAccessTokenException(); | ||
| } | ||
|
|
||
| try{ | ||
| Claims claims = jwtUtil.decode(accessToken); | ||
| return claims.get("userId", Long.class); | ||
| }catch (SignatureException e){ | ||
| throw new InvalidAccessTokenException(); | ||
| } | ||
| } |
There was a problem hiding this comment.
login이라는 메서드에서는 이 클래스의 속성인 jwtUtil과 userRepository 둘 다 사용하고 있습니다.
parseToken에서는 jwtUtil만 사용하고 있습니다.
메서드가 속성을 얼마나 활용하고 있냐로 이 클래스의 응집성을 파악해 볼 수 있습니다. parseToken은 jwtUtil에만 의존하고 있는 메서드 이므로 이 클래스는 응집도가 떨어진다는 것을 의미합니다.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
안녕하세요 : )
6주차 과제 리뷰 요청드립니다!
이번주는 개인적인 일이 있어서 아직 구현하지 못한 부분이 많습니다ㅠㅠ
우선 AuthenticationService와 jwtutil을 구현했고 로그인 메서드를 호출할 경우 토큰을 생성해서 돌려주는 부분까지 테스트했습니다.
나머지 기능들도 구현하는대로 커밋하겠습니다!!
좋은하루보내세요 !!><