-
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 추가 #40
Conversation
- AdminTokenProvider.
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.
수고하셨습니다! 몇 가지만 확인해주시면 될 것 같습니다 :)
@@ -24,9 +26,12 @@ | |||
|
|||
@NoArgsConstructor | |||
@Component | |||
public class AdminJwtTokenProvider implements AdminTokenProvider { | |||
public class AdminJwtAccessTokenProvider implements AdminAccessTokenProvider, |
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.
개인적인 생각으로는 해당 클래스에서 AT와 RT를 모두 만들고 있기 때문에 기존 이름 (AdminJwtTokenProvider)이 더 적합할 것 같은데, 클래스 이름을 변경하신 이유가 있으실까요?
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.
JwtTokenProvider 구조를 변경하면서 이름이 자동으로 변경되었던 것 같습니다 수정하도록 하겠습니다!
@@ -56,6 +57,14 @@ public ResponseEntity<AdminAccessTokenResponse> reGenerateAccessToken( | |||
return ResponseEntity.ok(adminAuthService.reGenerateAccessToken(refreshToken)); | |||
} | |||
|
|||
@DeleteMapping("/logout") | |||
public ResponseEntity<Void> login(@AdminRefreshToken final 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.
로그아웃을 하는 메서드인만큼 메서드명을 logout으로 하는 게 더 좋을 것 같습니다!
@Autowired | ||
private AdminRedisRefreshTokenRepository adminRedisRefreshTokenRepository; | ||
|
||
|
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 final RedisTemplate<String, Long> redisTemplate; | ||
|
||
|
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.
공백 있습니다 :)
|
||
@Builder | ||
public record AdminRefreshToken( | ||
@Id 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.
@Id
가 들어간 이유가 궁금한데, 혹시 고유 식별자 역할로 쓰이게 하시기 위함일까요?
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.
확인해보니 레디스로 관리할 경우에는 말씀하신 것 처럼 @Id
(springframework.data의 어노테이션)가 필요한 것으로 나오네요..! 그대로 남겨두시는게 맞지 않을까 생각됩니다. 참고하실만한 글 공유드리겠습니다!
@@ -0,0 +1,6 @@ | |||
package com.atwoz.admin.domain.admin.service; |
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.
AT 발급 인터페이스는 application/auth
, RT 발급 인터페이스는 domain/admin/service
에 작성하신 이유가 궁금합니다. 궁극적으로 모두 토큰을 발급하기도 하고, 구현체 또한 AdminJwtAccessTokenProvider
에서 모두 구현한만큼 이를 추상화한 인터페이스들의 위치도 통일시키는 게 좋을 것 같은데 어떻게 생각하시나요?
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.
저희가 RT만 레디스에 저장하기 때문에 RT 도메인 객체는 도메인에 위치해야합니다. 하지만 RT의 암호화된 토큰은 JwtTokenProvider에서 만들어집니다. 만약 JwtTokenProvider 통해 서비스 레이어에서 생성해준다면 RT 도메인 객체는 응용 계층에서 만든 암호화된 토큰을 받게 됩니다. 이로 인한 응용 계층의 의존이 생기기 때문에 RTProvider를 팩토리 메서드 인자로 넘겨줘서 내부에서 암호화된 토큰이 만들어지도록 구현해야한다고 생각했습니다. RTProvider는 도메인 서비스이기에 도메인 레이어에 위치해야합니다. 하지만 ATProvider는 도메인에 어떠한 영향도 미치지 않기 때문에 응용 계층에서 생성해서 응답 바디에 넘겨주면 됩니다. 제가 AT, RT 생성 기능을 모두 추상화한 인터페이스로 둔 이유가 바로 이 이유 때문이었습니다. 추가적인 의견 및 제가 잘못 이해해서 설계한 부분이 있다면 댓글로 의견 남겨주시면 감사하겠습니다!
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.
아 이해했습니다. AT 발급 (ATProvider)은 도메인에 영향을 미치지 않고 응용 계층에서만 이용되기 때문에 응용 계층에 인터페이스를 위치시키고, RT 발급 (RTPRovider)을 AT 발급처럼 응용 계층에 둔다면 도메인 계층에 있는 AdminRefreshToken에서 응용 계층에 대한 의존이 발생되기 때문에 도메인 패키지 하위에 서비스 패키지를 별도로 만들어주신거군요.
네 저도 이 방식이 더 나은 방식 같습니다! 태인님의 의견을 들으면서 인터페이스를 둘 위치를 더 깊게 고민할 수 있어서 좋았습니다 :)
@@ -33,37 +36,46 @@ public AdminTokenResponse signUp(final AdminSignUpRequest adminSignUpRequest) { | |||
adminProfileSignUpRequest.phoneNumber() | |||
); | |||
Admin savedAdmin = adminRepository.save(admin); | |||
AdminRefreshToken adminRefreshToken = createAdminRefreshToken(savedAdmin); | |||
adminRefreshTokenRepository.save(adminRefreshToken); | |||
String accessToken = adminAccessTokenProvider.createAccessToken(savedAdmin.getId()); |
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.
AT를 만들기까지의 과정과 RT를 만들기까지의 과정을 메서드 분리하는 것은 어떨지 여쭤보고 싶습니다..! (RT의 경우, 리포지터리에 save하는 것도 createAdminRefreshToken 메서드에 포함시키면 될 것 같습니다.)
또는, Admin을 save하는 것을 메서드 분리해도 좋을 듯 합니다.
public AdminTokenResponse signUp(final AdminSignUpRequest adminSignUpRequest) {
Admin savedAdmin = saveAdmin(adminSignUpRequest);
String accessToken = adminAccessTokenProvider.createAccessToken(savedAdmin.getId());
AdminRefreshToken adminRefreshToken = createAdminRefreshToken(savedAdmin);
return new AdminTokenResponse(accessToken, adminRefreshToken.refreshToken());
}
private Admin saveAdmin(final AdminSignUpRequest request) {
AdminProfileSignUpRequest adminProfileSignUpRequest = request.adminProfileSignUpRequest();
Admin admin = Admin.createWith(
request.email(),
request.password(),
request.confirmPassword(),
adminProfileSignUpRequest.name(),
adminProfileSignUpRequest.phoneNumber()
);
return adminRepository.save(admin);
}
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.
메서드 분리를 굳이 할 필요가 없어보여 하지 않았는데 다른 곳에서 재사용을 하지 않아도 작성하신 코드를 보니 분리하는게 나을 것 같다는 생각이 드네요. 메서드 분리하도록 하겠습니다!
login -> logout
AdminJwtAccessTokenProvider -> AdminJwtTokenProvider
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.
수고하셨습니다 👍 계층 구조 잘 나눠주신 것 같아서 코드리뷰 하기도 너무 편했습니다!
타임아웃도 잘 설정해주시고 딱히 변경해야할 부분이 보이진 않아서 Approve 합니다!
public void logout(final String refreshToken) { | ||
adminRefreshTokenRepository.delete(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.
프론트엔드에서 헤더에서 토큰을 제거해주고 동시에 로그아웃 api를 요청해서 리프레쉬 토큰까지 제거해서 로그아웃을 시켜준다라는 의미로 작성하신 코드가 맞을까요?!
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.
네 맞습니다!
프론트에서 로그아웃 처리시 로그아웃 api를 요청해서 서버에 있는 로그인 했던 유저의 RT를 삭제합니다!
📄 Summary
close #38