-
Notifications
You must be signed in to change notification settings - Fork 6
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-be: refresh token 구현 #816
Conversation
1729092158.789629 |
1729092165.639789 |
1729092166.118939 |
1729092168.664619 |
📌 Test Coverage Report
|
러기랑 의논 과정에서 굳이 |
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.
초코칩 수고하셨습니다~ 간단한 코멘트 남겨두었습니다.
@@ -208,11 +212,13 @@ security: | |||
jwt: | |||
token: | |||
secret-key: ${JWT_TOKEN_SECRET_KEY} | |||
expire-length: ${JWT_TOKEN_EXPIRE_CYCLE} | |||
access-expire-length: ${ACCESS_TOKEN_EXPIRE_CYCLE} |
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.
아직 github-secret에 값을 추가하지 않아서, 팀에서 의논 후에 값을 추가하면 작성하려 했습니다!
private boolean isRefreshTokenCookie(Cookie cookie) { | ||
return cookieProperties.refreshTokenKey().equals(cookie.getName()); | ||
} | ||
|
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.
개행 하나 더 들어갔어요~
.body(new LoginResponse(clubId)); | ||
} | ||
|
||
@PostMapping("/logout") | ||
public ResponseEntity<Void> logout() { | ||
ResponseCookie cookie = cookieManager.clearTokenCookie(); | ||
ResponseCookie cookie = cookieManager.clearAccessTokenCookie(); |
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.
고생하셨습니다~ 간단한 코멘트 남겨놓았어요.
@@ -42,7 +42,8 @@ public void addInterceptors(InterceptorRegistry registry) { | |||
.excludePathPatterns("/**/signup") | |||
.excludePathPatterns("/**/login") | |||
.excludePathPatterns("/**/applyform/*/submit") | |||
.excludePathPatterns("/"); | |||
.excludePathPatterns("/") | |||
.excludePathPatterns("/**/auth/refresh"); |
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 boolean isValidTokenExpired(HttpServletRequest request) { | ||
try { | ||
String token = cookieManager.extractAccessToken(request); | ||
return authService.isTokenValid(token) && !authService.isTokenNotExpired(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.
이럴 바엔 notExpired 메서드를 지우고 isExpired로 사용하시는건 어떤가용
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.
근데 지금 코드에서 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.
accessToken과 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.
어색한 것보단 추상화된 메서드가 getToken()밖에 없어서 현재 코드에선 이점이 없다고 생각했어요.
각 클래스마다 @Getter
붙여주는 것만으로도 충분한 것 같아서요.
인터페이스를 구현해도 어차피 @getter는 붙여줘야 하니깐요..
혹시 제 논리가 틀렸다면 바로 짚어주세요. 🥺
} | ||
|
||
@Override | ||
public boolean isTokenExpired(String token) throws IllegalTokenException { |
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.
초코칩뿐만 아니라 다른 팀원분들께도 질문합니다.
얘랑 isValid랑 뭐가 다른가요...? isValid 하나로 퉁쳐서 사용할 수 없나요?
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.
isValid의 메서드명이 모호한 것 같아서, 메서드명을 구체화했습니다.
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.
겹치지 않습니다. 만료 여부와 시그니처 검증 여부는 다릅니다.
String token = cookieManager.extractToken(request); | ||
return authService.isTokenValid(token); | ||
String token = cookieManager.extractAccessToken(request); | ||
return authService.isTokenValid(token) && authService.isTokenNotExpired(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.
지금 조건문이 (만료되었는지 + a) && (만료되었는지)
이렇게 되어 있는 것 같은데
제가 잘못 파악했나요 ? ? ? ?
@cutehumanS2 @xogns1514 @HyungHoKim00 이해의 도움을 드리기 위해 간단한 다이어그램 작성했습니다. 먼저 accessToken의 검증 과정입니다. 아래는 RTR 진행 과정입니다. 보시면서 궁금한 점이 있다면, DM이나 커멘트 남겨주세요! |
Original issue description
목적
refresh token을 RTR 방식으로 구현합니다.
RTR 방식은 Refresh Token를 탈취 당했을 때, 피해 시간을 최소화하기 위한 방식입니다. 기존에는 refresh를 통해 Access Token만 재발급했다면, RTR은 Refresh Token과 Access Token 모두 재발급하는 것입니다.
작업 세부사항
REFRESH_TOKEN_01
closes #815