Conversation
|
리뷰 참고 사항 중 1번 질문은 변경된 닉네임으로 저장소에서 refreshToken을 지우는 방향으로 구현해보는 것은 어떨까요? |
|
|
||
| public ReissueAccessTokenResponse reissueAccessTokenByRefreshToken(final String refreshToken, | ||
| final String accessToken) { | ||
| final String accessToken) { |
There was a problem hiding this comment.
뭐죠 이 애매한 개행..? w(゚Д゚)ww(゚Д゚)w
| final LoginCheckerInterceptor loginCheckerInterceptor, | ||
| final TokenInterceptor tokenInterceptor |
There was a problem hiding this comment.
(╯‵□′)╯︵┻━┻ (╯°□°)╯︵ ┻━┻
| NOT_FOUND_OAUTH(1010, "현재 지원하지 않는 OAuth 요청입니다."), | ||
| INVALID_REFRESH_TOKEN(1011, "존재하지 않는 refreshToken 입니다."), | ||
| TOKEN_PAIR_NOT_MATCHING_EXCEPTION(1012, "올바르지 않은 TokenPair 입니다."), | ||
| ALREADY_EXIST_NICKNAME(1013, "존재하는 닉네임입니다."), |
There was a problem hiding this comment.
스플릿: NICKNAME_ALREADY_EXIST
아코: DUPLICATE_NICKNAME
바론: 영문명은 상관없지만 "중복되는 닉네임입니다." 어떠신가요?
|
|
||
| private void validateMemberAuthentication(final Member requestMember, | ||
| final Member targetMember) { | ||
| private void validateMemberAuthentication(final Member requestMember, final Member targetMember) { |
There was a problem hiding this comment.
메서드 순서 변경이 필요해 보입니다! 이 메서드를 위로 올리는 것은 어떨까요?
| if (isSameNickname(member, nickname)) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
기존에 사용했던 닉네임으로 재변경 할 때
- 예외를 던진다
- 무시한다. (토큰 재발급을 막거나, 기존에 있던 값을 다시 반환한다.)
중에 결정해야 할 것 같습니다. (프론트와 논의 필요)
| } | ||
|
|
||
| @Transactional | ||
| public TokenPair updateNickname(final Long memberId, final MemberInfo memberInfo, |
There was a problem hiding this comment.
닉네임 변경으로 인해 토큰 재발급이 일어나고, 응답이 반환되는 보안 문제가 걱정된다면 닉네임 같은 개인정보를 바꾼 후에 재로그인을 요청하는 것은 어떨까요?
닉네임이 변경되면 서버 내부적으로 TOKEN을 삭제 -> 로그인이 만료된 것 처럼 취급이 되어서 프론트엔드에서 자연스럽게 재로그인을 요청하도록 하는 것은 어떤가요?
| inMemoryTokenPairRepository.addOrUpdateTokenPair(reissuedRefreshToken, reissuedAccessToken); | ||
| return new TokenPair(reissuedAccessToken, reissuedRefreshToken); | ||
| } | ||
|
|
| member.updateNickname(nickname.getValue()); | ||
| memberRepository.save(member); | ||
|
|
||
| return reissueTokenPair(member.getId(), member.getNickname()); |
There was a problem hiding this comment.
여기 return 문 위에 개행이 필요합니다. ~ (아코: 왜 추가 안하셨죠?)
| } | ||
|
|
||
|
|
||
| private boolean isSameNickname(final Member member, final Nickname nickname) { |
There was a problem hiding this comment.
Member한테 물어보는 방향은 어떠신가요?
There was a problem hiding this comment.
Member 에게 동일한 닉네임을 가지고 있는지 물어보도록 메서드 추가했습니다.
|
|
||
| public void updateNickname(final Nickname newNickname) { | ||
| this.nickname = newNickname; | ||
| } |
There was a problem hiding this comment.
의견)
isSameNickname의 책임을 Member한테 넘기면서, 내부적으로new Nickname()을 통해 닉네임 형식 검증 + 중복 검증을 해보면 어떨까요?- 그렇게 되면 이
Nickname객체를 받는 메서드는 사라질 것 같습니다.
There was a problem hiding this comment.
isSameNickname 의 검증 책임을 멤버에게 넘기려고 했는데, memberRepository 에서 중복을 확인할 때 Nickname 객체가 필요하더라고요. 그래서 해당 메서드는 그대로 두고, Nickname 객체는 외부에서 생성하여 검증하는 걸로 했습니다!
| @Autowired | ||
| private KillingPartLikeRepository likeRepository; | ||
|
|
||
| private TokenProvider tokenProvider; |
There was a problem hiding this comment.
@Autowired 로 변경할 수 있을 것 같아요
There was a problem hiding this comment.
아쉽게도 UsingJpaTest 라서 @Autowired 로는 변경할 수 없었습니다 😭
| saveAndClearEntityManager(); | ||
| // when | ||
| memberService.deleteById(targetId, new MemberInfo(targetId, Authority.MEMBER)); | ||
| memberService.deleteById(targetId, new MemberInfo(targetId, MEMBER)); |
There was a problem hiding this comment.
static import 제거완입니다 👍
| // when, then | ||
| assertThatThrownBy(() -> | ||
| memberService.deleteById(targetId, new MemberInfo(unsavedMemberId, Authority.MEMBER)) | ||
| memberService.deleteById(targetId, new MemberInfo(unsavedMemberId, MEMBER)) |
| memberService.deleteById(targetMember.getId(), | ||
| new MemberInfo(requestMember.getId(), Authority.MEMBER)) | ||
| memberService.deleteById(targetMember.getId(), | ||
| new MemberInfo(requestMember.getId(), MEMBER)) |
|
|
||
| // when | ||
| // then | ||
| assertThat(memberService.updateNickname(savedMember.getId(), new MemberInfo(savedMember.getId(), MEMBER), |
There was a problem hiding this comment.
기왕 개행된거, , 로 된 거 전부 개행하는 것은 어떨까요?
| void fail_updateNickname_duplicate_nickname() { | ||
| // given | ||
| final Member newMember = memberRepository.save(new Member("temp@email", "shook2")); | ||
| final String newNickname = "shook"; // 중복된 닉네임 |
There was a problem hiding this comment.
변수명을 duplicateNickname 으로 하는건 어떠신가요?
There was a problem hiding this comment.
더 좋은 것 같네요 바로 변경했습니다 👍🏻
| } | ||
|
|
||
| @DisplayName("닉네임이 빈 문자열이면 예외를 던진다.") | ||
| @ValueSource(strings = {"", " ", " ", "\r", "\n", "\t"}) |
There was a problem hiding this comment.
ヾ(⌐■_■)ノ♪
아코: Empty, Blank 어노테이션이 있다고 하네요.
| void create_fail_lengthOver100() { | ||
| void create_fail_lengthOver20() { | ||
| //given | ||
| final String nickname = ".".repeat(101); |
| .contentType(ContentType.JSON) | ||
| .body(request) | ||
| .when().log().all() | ||
| .header(HttpHeaders.AUTHORIZATION, "Bearer " + accessToken) |
There was a problem hiding this comment.
token이 담긴 header는 given 으로 둬요 ^__^
There was a problem hiding this comment.
모든 header 는 given 으로 이동하는 것으로 변경되었습니다 👍🏻
| .contentType(ContentType.JSON) | ||
| .body(request) | ||
| .when().log().all() | ||
| .header(HttpHeaders.AUTHORIZATION, "Bearer " + accessToken) |
There was a problem hiding this comment.
ditto
중복된 닉네임 수정 시 예외 처리 여부, 상태 코드 반환 여부, 재발급 여부는 추후 논의가 필요합니다.
| .contentType(ContentType.JSON) | ||
| .body(request) | ||
| .when().log().all() | ||
| .header(HttpHeaders.AUTHORIZATION, "Bearer " + accessToken) |
somsom13
left a comment
There was a problem hiding this comment.
백엔드 4인이 모인 몹 코드리뷰 결과입니다.
고생하셨어요 베로 ✍(◔◡◔)
- token service 로 책임 분리
- 리프레시 토큰이 다시 닉네임을 가지도록 수정
somsom13
left a comment
There was a problem hiding this comment.
베로~~ 너무 고생 많으셨어요!!
닉네임 변경이라는게 생각보다 고민해야 할 요소들이 정말 많은 기능이었네요... 🥲 😓
하지만 역시 베로답게 야무지게 구현, 수정해주셨군요 👍
로직 상에 문제가 생기지 않을까~? 싶은 부분들이 조금 있어서 코멘트 남겼습니다! 베로의 의견도 알려주시고 같이 이야기 해보면 좋을 것 같아요. (੭ˊᵕˋ)੭*ଘ
| @PathVariable(name = "member_id") final Long memberId, | ||
| @Authenticated final MemberInfo memberInfo, | ||
| @RequestBody final NicknameUpdateRequest request, | ||
| @Valid @RequestBody final NicknameUpdateRequest request, |
There was a problem hiding this comment.
저도 매의 눈을 뜨기 위해 노력할게요 🦅 👀
| tokenService.validateRefreshToken(refreshToken); | ||
| final String accessToken = tokenService.extractAccessToken(authorization); | ||
| final ReissueAccessTokenResponse response = tokenService.reissueAccessTokenByRefreshToken(refreshToken, | ||
| accessToken); |
There was a problem hiding this comment.
TokenService 의 validateRefreshToken, extractAccessToken 모두 토큰 재발급 용도로 이 controller 메서드 내에서만 사용되는 것 같아요.
reissue 메서드 내에서 validate, extract 를 함께 하도록 하지 않고 validate와 extract를 public으로 두고 controller 에서 따로 호출하도록 하신 이유가 있을까요?
There was a problem hiding this comment.
지금은 사라졌지만 닉네임 변경 시 리프레시 토큰을 받도록 하면 다른 곳에서도 검증이 필요해서 public 메서드로 만들어두었습니다!
그런데 생각해보면 현재 리프레시 토큰은 path 가 /reissue 로 고정되어 있기 때문에 private 메서드로 둬도 될 것 같네요 좋은 리뷰 감사합니다 👍🏻
| } | ||
|
|
||
| public String extractAccessToken(final String authorization) { | ||
| return authorization.substring(TOKEN_PREFIX.length()); |
There was a problem hiding this comment.
Bearer {accessToken} 구조의 문자열에서 엑세스 토큰만 추출하는 로직인 것 같은데, 엑세스 토큰이 Bearer로 시작하지 않는 상황에 대해서는 따로 검증하지 않아도 괜찮을까요?
There was a problem hiding this comment.
그리고 만약 authorization 문자열이 Bearer 보다 짧은, 잘못된 값이 들어오면 substring 에서 IndexOutOfBoundException이 발생할 수도 있을 것 같아요!
| final Claims claims = tokenProvider.parseClaims(refreshToken); | ||
| final Long memberId = claims.get("memberId", Long.class); | ||
| final String nickname = claims.get("nickname", String.class); | ||
|
|
||
| inMemoryTokenPairRepository.validateTokenPair(refreshToken, accessToken); |
There was a problem hiding this comment.
tokenProvider.parseClaims 보다 InMemoryTokenPairRepository.validateTokenPair 가 선행되어야 하지 않을까요?
현재 TokenScheduler에 의해 1초마다 스케줄러를 돌려서 만료된 refreshToken을 삭제하고, Repository에 없는 refreshToken으로 재발급 요청이 오면 RefreshTokenNotFoundException 을 발생시키는 것으로 알고있어요.
만약 refreshToken이 만료되었는데, 이를 캐치하지 못하고 먼저 tokenProvider.parseClaims로 refreshToken을 추출한다면 아래와 같은 문제가 생길 것 같아요.
parseClaims가 파라미터로 받은 token의 유효기간이 만료되었다면 TokenException.ExpiredTokenException 을 발생시키고 프론트엔드에게 EXPIRED_TOKEN(1001, "유효기간이 만료된 토큰입니다.") 에러 메세지가 반환됩니다.
그러면 프론트엔드에서는 accessToken이 만료되었으니, refreshToken을 가지고 reissue 요청을 보내야 하는 상황 으로 인식이 될 것 같아요. (저희가 만료된 토큰이 accessToken인지, refreshToken인지에 대해 식별을 안하고 있는 걸로 알고 있습니다)
그런데 만료된 것이 refreshToken이라면 어떻게 될까요? 이 상황에서도 동일하게 유효기간이 만료된 토큰 이라는 에러 메세지가 프론트엔드에게 전달될 것 같아요.
그리고 다음 단계인 InMemoryTokenPairRepository.validateTokenPair까지 흐름이 넘어가지 않고 예외가 발생한 상태로 끝나버리겠죠?
이렇게 되면 결과적으로 "accessToken이 만료되어서 재발급 받으려 하는데, refreshToken도 이미 만료가 된 상황"에 대한 처리가 제대로 이루어지지 않을 것 같다는 생각이 들어요.
베로는 어떻게 생각하시나요?
There was a problem hiding this comment.
날카로운 지적입니다. validateTokenPair 가 먼저 실행되어야 한다는 것에 100% 동의합니다
추가로 parseClaim 을 할 때, 어떤 토큰이 만료되었는지 로깅하는 건 어떤가요?
| private static final String TOKEN_PREFIX = "Bearer "; | ||
|
|
||
| private final AuthService authService; | ||
| private final TokenService tokenService; |
| public TokenPair updateNickname(final Long memberId, final MemberInfo memberInfo, | ||
| final NicknameUpdateRequest request) { | ||
| final Member member = getMemberIfValidRequest(memberId, memberInfo); | ||
| public boolean updateNickname(final Long memberId, final Long requestMemberId, |
There was a problem hiding this comment.
💬 위의 deleteById 는 memberInfo 를 그대로 파라미터로 받아오고 있는데, 두 메서드가 파라미터를 받는 방식을 통일해보면 어떨까요?
There was a problem hiding this comment.
현재 memberInfo 의 id 만 쓰이고 있는 상태라 id 만 넘겨주는 걸로 통일했습니다 😄
| public TokenPair updateNickname(final Long memberId, final MemberInfo memberInfo, | ||
| final NicknameUpdateRequest request) { | ||
| final Member member = getMemberIfValidRequest(memberId, memberInfo); | ||
| public boolean updateNickname(final Long memberId, final Long requestMemberId, |
There was a problem hiding this comment.
MemberService에서 Token을 재발급 해주는 로직이 사라지니 확실히 더 책임 분리가 잘 된 느낌이 드네용 ㅎㅎ 👍
There was a problem hiding this comment.
매의 눈... 이거 왜 남아 있었을까요 🤦🏻♀️
| @Parameter( | ||
| name = "memberInfo", | ||
| hidden = true | ||
| ), |
| @SpringBootTest | ||
| class TokenServiceTest { | ||
|
|
||
|
|
seokhwan-an
left a comment
There was a problem hiding this comment.
배로! 역시 코드를 잘짜시네요!
리뷰할 때마다 배워나가는 것 같아요! 👍👍
리뷰 남긴 것 확인 부탁드립니다!
| final Member requestMember = findById(requestMemberId); | ||
| final Member targetMember = findById(memberId); |
There was a problem hiding this comment.
이 부분에서 현재 request의 Path로 넘어온 id와 arguemntResolver로 넘어온 id로 member를 DB에서 불러온 뒤에 같은지 비교를 하고 있는데 현재 member의 경우 equal&hash가 id로만 재정의 되어 있습니다. 따라서 db에서 member를 불러오기에 앞서서 memberId와 requestMemberId를 비교하고 그 다음에 실제 존재하는 member인지 검증하기 위해 DB로 요청을 보내면 db로 보내는 요청횟수를 줄일 수 있을 것 같습니다.
There was a problem hiding this comment.
오 굉장히 합리적인 방법이네요
아코 말씀대로 id가 같은지 먼저 비교하고, member 를 찾아오는 로직으로 변경했습니다! 로직이 정말 간결해졌네요 👍🏻👍🏻 👍🏻 👍🏻 👍🏻 👍🏻
| .includePathPattern("/songs/*/parts/*/comments", PathMethod.POST) | ||
| .includePathPattern("/members/*", PathMethod.DELETE); | ||
| .includePathPattern("/members/*", PathMethod.DELETE) | ||
| .includePathPattern("/members/*/nickname", PathMethod.PATCH); |
| member.updateNickname(nickname.getValue()); | ||
| memberRepository.save(member); | ||
|
|
There was a problem hiding this comment.
이 부분 jpa 더티채킹을 통해서 값이 수정되어서 memberRespository.save()를 하지 않아도 될 것 같습니다!
| public class TokenService { | ||
|
|
||
| public static final String EMPTY_REFRESH_TOKEN = "none"; | ||
| public static final String TOKEN_PREFIX = "Bearer "; | ||
|
|
||
| private final TokenProvider tokenProvider; | ||
| private final InMemoryTokenPairRepository inMemoryTokenPairRepository; | ||
|
|
There was a problem hiding this comment.
현재 TokenService 내부에 TokenProvider와 InMemoryTokenPairRepository를 가지고 구성이 되어 있는데 MemberService와 AuthService는 TokenService를 의존하는 것이 아닌 TokenProvider와 InMemoryTokenPairRepository의존하고 있는데 이 부분이 전에 말했던 순환참조 때문이었나요? 그렇지 않으면 TokenService를 의존하는 것이 좋을 것 같은데 어떻게 생각하시나요?
| @ParameterizedTest | ||
| void updateNickname_badRequest(final String invalidNickname) { | ||
| // given | ||
| final Member member = memberRepository.save(new Member("hi@naver.com", "hi")); |
There was a problem hiding this comment.
현재 이 부분은 이용되지 않는 것 같습니다!
📝작업 내용
닉네임 변경 API를 구현한다.
💬리뷰 참고사항
2차 리뷰 참고 사항
#️⃣연관된 이슈
closes #462