-
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
[FEAT] 토큰 재발급 api #156
[FEAT] 토큰 재발급 api #156
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.
수고하셨습니다~
제가 이해하고 있는 토큰 재발급 기능 내용이 조금 달라서 논의가 필요할 듯 하여 우선 코멘트로 답니다!
@PostMapping("/token") | ||
public ResponseEntity<Response> recreateToken(Principal principal) { | ||
val memberId = Long.parseLong(principal.getName()); | ||
val response = authService.recreateToken(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.
토큰 이름 따라서 메소드명도 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.
뭔가 이름이 겹치는 거 같아서 혼란스러울 거 같은데 다른 이름을 추천해주실 수 있나요?
Token token = generateToken(new UserAuthentication(memberId, null, null)); | ||
Member member = findMember(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.
val 부탁드려요 :)
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 TokenResponse recreateToken(long memberId) { | ||
Token token = generateToken(new UserAuthentication(memberId, null, null)); | ||
Member member = findMember(memberId); | ||
member.updateRefreshToken(token.getRefreshToken()); |
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.
리프레시 토큰도 새로 만들어주는 이유가 있을까요?? 제가 이해하기로는 리프레시 토큰을 이용해서 액세스 토큰만을 새로 발급해주는 것으로 알고 있었어서요! 그래서 리프레시 토큰이 만료되면 로그인으로 밖에 새로 발급할 수 없는 것으로 이해했습니다! (대부분의 서비스들이 평균 1~2주 후 자동로그인이 해제되는 이유도 이 때문이구요!)
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.
전면 수정 후 다시 push햇으니 리뷰해주시면 감사하겠습니다!
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.
너무 고생하셨습니다!! 그런데 왜 201로 하셨는 지 이유를 설명해주실 수 있을까요?? 재발급이라는 명칭자체가 200에 더 가깝다고 느껴지는데 궁금합니다!!
새롭게 리팩토링하고 push햇으니 확인 부탁드립니다~
|
또한 리소스를 제공해준다는 점에서 201이 맞다 생각했는데 |
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.
수고 많으셨어요!
Entity를 직접 외부로 드러내는 메서드만 숨겨주시면 좋을 것 같아요!
@@ -26,6 +29,12 @@ public ResponseEntity<Response> signIn(@RequestHeader("Authorization") String so | |||
return ResponseEntity.ok(success(SUCCESS_SIGN_IN.getMessage(), response)); | |||
} | |||
|
|||
@PostMapping("/token") | |||
public ResponseEntity<Response> reissueToken(@RequestHeader("Authorization") String refreshToken) { | |||
val response = authService.reissueToken(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.
reissue도 좋네요~
Member findBySocialTypeAndSocialId(SocialType socialType, String socialId); | ||
Member findMemberById(long memberId); | ||
CottonCountResponse giveCotton(long memberId, CottonType cottonType); | ||
MemberHomeInfoResponse getMemberHomeInfo(long memberId); | ||
Member findMemberByRefreshToken(String 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.
이전에 한번 논의했던 사항인 것 같긴 한데,
Member처럼 직접 Entity를 외부로 반환하는 것은 매우 위험하다고 생각해요. (위험성 중 하나로 Controller에서도 Entity에 접근할 수 있는 경우) 기존대로 직접 Entity를 호출해야하는 경우는 각 Service 내에서 Repository를 통해 가져오는 것이 안전하다고 생각해요.
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.
수고하셨습니다!
✨ Related Issue
📝 기능 구현 명세
🐥 추가적인 언급 사항