-
Notifications
You must be signed in to change notification settings - Fork 8
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
[BE] 리프래쉬 토큰 저장소 쿠키로 수정(#807) #810
base: develop
Are you sure you want to change the base?
Conversation
Test Results 70 files 70 suites 9s ⏱️ For more details on these failures, see this check. Results for commit b615361. ♻️ This comment has been updated with latest results. |
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.
무빈~ 오랜만이니까 RC 줄게요 😈
@@ -52,127 +52,127 @@ public void run(ApplicationArguments args) { | |||
new Room("방 제목 1", "방 설명 1", 3, | |||
null, null, List.of("TDD", "클린코드"), | |||
1, 1, 20, member1, | |||
LocalDateTime.of(2024, 12, 20, 12, 0), | |||
LocalDateTime.of(2024, 12, 26, 12, 0), | |||
LocalDateTime.of(2025, 12, 20, 12, 0), |
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.
ㅋㄱㅋㄱㅋㄱㅋㄱㄲ 😭
우리의 2024년은 어디로
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.
ㅋㅋㅋㅋㅋㅋㅋ 다 터지는 테스트...
import java.util.Optional; | ||
|
||
@Service | ||
public class CookieService { |
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.
해당 로직이 Service 의 느낌은 아닌거 같아요.
애플리케이션의 핵심 로직을 처리하는게 @Service
의 느낌인데
해당 로직은 Service 를 도와주는 Helper 성 클래스의 느낌입니다.
@Component
와 다른 이름은 어떠신가용?🙂
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.
원래 구상은 infrastructure 의 TokenProvider 과 비슷한 역할이라고 생각했었는데, LoginService 에선 TokenService 를 통해 토큰을 다루는 작업을 한 번 더 감싸 사용하는 것과 결을 맞추려고 서비스로 분류했었습니다.
물론 내용을 까보면 말한것처럼 service 의 내용은 아니라고 생각해 cookieProvider 로 수정하겠습니다~
다만, 계층 분리와 관련해서 고민하던 점이 있는데 조이썬 @youngsu5582 의견이 듣고싶어요.
LoginService 에서 accessToken, refreshToken 을 tokenService 를 통해 가져와 사용하고 있는데, 이 중 refreshToken 을 Cookie 로 감싸서 보내는게 이번 PR의 목적입니다.
그렇다면 어느 계층에서 refreshToken 을 cookie 로 감싸는게 맞다고 생각하는지 궁금해요!
컨트롤러에선 당연히 Set-cookie
를 통해 쿠키를 담아야 하기에 쿠기가 사용된다는 사실을 알 수 밖에 없는데, 서비스 레벨에서 refreshToken 을 쿠키로 감싸서 보내는게 맞을지, 아니면 TokenService 에서 쿠키로 감싼 후 내보내는게 맞을지가 고민입니다.
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.
사실, 저도 충분히 Service 로 사용될 수 있겠다는 생각은 듭니다.
하지만, 엄밀히 Service 라는 개념
비즈니스 로직과 트랜잭션 관리 및 캐시 같은 요구사항을 처리하기 위해 사용한다.
접근하면, gRPC 나, HTTP 를 사용하지 않는 다른 프로토콜일때 이 Service 를 가져다가 편하게 사용할 수 있을까?
라고 생각하면 아닐꺼 같아요.
저는 이 관점에서 HTTP 요청을 담당하는 Controller 가 담당하는게 타당하다고 생각해요.
( Service 는 단순히 String Token 그 자체만 반환 )
이게 계속 반복되고 불필요하다면 제 위 의견처럼 Helper 성 클래스를 만드는 게 맞다고 생각하고용.
사실, 관점의 차이라고 생각합니다.
무빈의 의견 더 들려줘도 좋아요~
return ResponseCookie.from(name, value) | ||
.httpOnly(true) | ||
.secure(true) | ||
.sameSite("Lax") |
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.
쿠키에서 시행할 수 있는 보안적인 옵션들입니다!
위부터 각각
- 웹 브라우저를 통한 통신에서만 쿠키가 사용되게 하기(js, python 등 다른 매체를 통한 요청에선 쿠키 사용 불가능),
- HTTPS 프로토콜을 사용할 때에만 쿠키가 사용되게 하기
- Cross-site 에 대한 쿠키 전달 옵션("Strict": same-site 에서만 쿠키 전달 가능, "lax": 몇 가지 요청에 한해 서드 파티 쿠키 전송 가능, "None": cross-site 에도 제한없이 쿠키 전달 가능)
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.
그러면 Strict 가 아니라, Lax 가 필요한 이유에는 뭐가 있나요??
@@ -39,11 +41,12 @@ public ResponseEntity<LoginResponse> login(@RequestBody LoginRequest loginReques | |||
|
|||
return ResponseEntity.ok() | |||
.header(AUTHORIZATION_HEADER, tokenInfo.accessToken()) | |||
.body(new LoginResponse(tokenInfo.refreshToken(), userInfo, memberRoleResponse.role())); | |||
.header(SET_COOKIE, tokenInfo.refreshToken().toString()) |
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.
ResponseCookie 의 toString 이 들어가는거 같은데 toString
을 하면 뭐가 들어가나요?
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.
ResponseEntity 에서 헤더를 설정할 때엔 항상 key, value 가 String 의 형태여야 합니다.
이를 위해 ResponseCookie에 toString() 이 재정의 되어있으며, 이 값은 쿠키의 key, value, 설정 등을 웹에서 쿠키로 인식하는 문자열 형태로 만들어줍니다!
ResponseCookie 클래스를 들어가보면 StringBuilder 에 차곡차곡 쌓아서 리턴해주는걸 볼 수 있어요 ㅎㅎ
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.
아하! 클린 코드 관점이라면 TokenInfo 가 메소드로 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.
무빈~ 의견 남겼으니 이에 대해 답변만 해주구 머지해두 될듯용 🙂
📌 관련 이슈
✨ PR 세부 내용
리프래시 토큰을 로컬 스토리지 저장에서 쿠키로 저장하도록 바꿨습니다.
사유는 텐텐의 요구였고, 근거는 여기에 정리되어 있습니다.