-
Notifications
You must be signed in to change notification settings - Fork 0
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
[#8] 카카오 로그인 OIDC -> 자체 JWT 발급 방식 변경 #11
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[PR 질문에 대한 답변]
1 ~ 3: 코멘트 달아두었습니다.
- 지금 AuthContoller의 메서드들은 단순히 AuthService의 메서드를 호출하는 로직밖에 없는데 컨트롤러와 서비스에서 테스트를 어떻게 다르게 하는지 모르겠어요
Controller와 Service가 하는 역할의 차이는 무엇일까요?
ControllerTest는 어떤 것을 검증하기 위한 것인지 조사해 보시고 저에게 답변 부탁드립니다.
- KakaoOAuthClient도 테스트 코드 작성하나요??
테스트 코드가 필요할지 한 번 판단해보시고, 필요가 없다면 그 이유를 반대로 필요가 있어도 그 이유를 저에게 설명해 주세요.
String refreshToken = jwtTokenProvider.generateRefreshToken(member.getId()); | ||
memberService.updateRefreshToken(member.getId(), refreshToken); | ||
|
||
AuthTokens authTokens = new AuthTokens(accessToken, jwtTokenProvider.getAccessTokenExpireTime(), refreshToken, jwtTokenProvider.getRefreshTokenExpireTime()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
클라이언트 입장에서는 액세스 토큰 유효 시간이 몇분, 몇시간인지 알면 좋겠지만 그것보다는 "언제 토큰이 만료되는지 구체적인 시각"을 알고 싶어하지 않을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
만료일시를 반환하도록 변경했습니다~!
} | ||
} | ||
|
||
public String getMemberIdFromToken(String token) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jwt 토큰의 payload가 항상 memberId는 아닐텐데 좀더 범용적인 메서드명으로 지어보면 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getSubjectFromToken
로 변경해보았습니다.
} catch (Exception e) { | ||
throw InvalidTokenException.EXCEPTION; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이렇게 되면 모든 예외가 InvalidTokenException으로 처리될텐데, 구체적으로 어떤 에러인지 스택 트레이스를 추적할 수 없을 것 같아요..! (나중에 로깅하신다 생각했을 때 어떤 문제가 있을지 고민해보세요.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
구체적인 예외를 로깅에서 확인할 수 있도록 수정했습니다.
private static final long ACCESS_TOKEN_EXPIRE_TIME = 1_000L * 60 * 60; | ||
private static final long REFRESH_TOKEN_EXPIRE_TIME = 1_000L * 60 * 60 * 24 * 14; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
보통 설정 파일로 빼는 편이에요.
해커가 유효 시간을 우리 소스 코드를 보고 악용할 여지가 있을 거예요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 설정 파일로 분리했습니다.
@@ -0,0 +1,4 @@ | |||
package com.hyun.udong.auth.presentation.dto; | |||
|
|||
public record AuthTokens(String accessToken, long accessTokenAge, String refreshToken, long refreshTokenAge) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
age보다는 만료 시각을 주면 좋겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
만료일시를 반환하도록 변경했습니다~!
} | ||
|
||
return MemberResponse.from(memberRepository.save(member)); | ||
public Member save(Member member) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
save인데 없으면 기존 회원을 반환하지말고 에러를 던지면 되지 않을까요?
있으면 update 없으면 create 이 과정을 같이 하고 싶으면 upsert 표현을 많이 씁니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AuthService.kakaoLogin()에서 기존회원일 시 member의 id를 받아와야 해서요..! (토큰 발급 시 필요함)
Member member = memberService.findBySocialIdAndSocialType(profile.getId(), SocialType.KAKAO)
.orElseGet(() -> memberService.save(profile.toMember()));
로 바꿔봤는데 괜찮을까요?
public Member updateRefreshToken(Long id, String refreshToken) { | ||
Member member = findById(id); | ||
member.updateRefreshToken(refreshToken); | ||
return member; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아마 이런 코드때문에 authService, memberService의 경계를 모호하게 느끼시는 것 같습니다.
authService가 memberRepository를 의존하면 어떨까요?
꼭 AService는 무조건 ARepository만 의존해야한다는 법칙은 없습니다 ㅎㅎ
오히려 MemberService는 refreshToken 관련 정보는 모르는 것이 더 자연스럽다고 생각해요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
와 그렇네요..! authService로 로직 이동했습니다!
수정한 코드는 jpa의 변경감지를 포기하게되는데 정상인거겠죠...?
필요 없다고 생각한 이유는 카카오가 준 code와 accessToken이 유효해야 한다가 전제인데 외부 api 호출은 모킹으로 테스트하기 때문에 가짜 값으로 api 테스트하는 게 의미가 있나?라고 생각했기 때문입니다.
|
넵! 답변 잘해주셨네요.
넵. 맞아요. |
#️⃣ Issue
🧑🏻🏫 메인 리뷰어 지정
📝 요약
기존에 세션 대신 idToken을 사용하려 했으나 자체 토큰 발급으로 변경
🛠 작업 내용
References
참고 블로그
참고 repo
🤔 리뷰 시 참고 사항
MemberService.save()
메서드의 동작과 네이밍이 애매한 것 같기도 합니다..(save인데 기존회원이면 반환?)AuthService
와MemberService
의 역할 분배가 잘 된건지 모르겠습니다. (테스트할 때 애매했어요,,)AuthContoller
의 메서드들은 단순히AuthService
의 메서드를 호출하는 로직밖에 없는데 컨트롤러와 서비스에서 테스트를 어떻게 다르게 하는지 모르겠어요KakaoOAuthClient
도 테스트 코드 작성하나요??✅ 체크리스트