-
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] 기존 jwt 토큰 응답 방식을 쿠키로 변경하고 path 및 보안 설정 #131
Conversation
Test Results67 tests 67 ✅ 4s ⏱️ Results for commit 11d67fc. ♻️ This comment has been updated with latest results. |
abdcb23
to
d10d68b
Compare
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.
고생했어요 재즈 😄
@@ -15,12 +15,26 @@ | |||
@RequiredArgsConstructor | |||
public class AttendeeController { | |||
|
|||
private static final String ACCESS_TOKEN = "ACCESS_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.
궁금해서 SET-COOKIE 에 들어갈 key-value 이름 컨벤션도 찾아봤는데 역시나 이상없네요 👍
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.
불필요한 DTO 삭제 좋네요 👍
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.
HttpOnly, Secure 옵션으로 더 안전하게 인증할 수 있겠네요. path 옵션을 통해 다른 약속의 토큰이 전달되지 않도록 설정하여 그로인해 발생할 수 있는 문제도 해결되겠어요. 👍
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.
고생하셨습니다 재즈!
코멘트 몇 가지 남겨 드렸습니다^^
Postman으로 쿠키가 path 별로 다르게 전송됐을 때 사용자의 자동 로그인이 유지되는지 확인해 봤는데 잘 동작하는 것 같아요👍
Cookie cookie = new Cookie(ACCESS_TOKEN, value); | ||
cookie.setHttpOnly(true); | ||
cookie.setSecure(true); | ||
cookie.setPath(path); |
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.
path
설정 좋아요👍
Cookie[] cookies = request.getCookies(); | ||
String token = getCookieValue(cookies); | ||
|
||
if (token == null) { | ||
throw new MomoException(AuthErrorCode.NOT_FOUND_TOKEN); | ||
} | ||
|
||
return jwtManager.extract(token); | ||
} | ||
|
||
private String getToken(NativeWebRequest webRequest) { | ||
String header = webRequest.getHeader(AUTHORIZATION); | ||
if (header == null) { | ||
throw new MomoException(AuthErrorCode.NOT_FOUND_TOKEN); | ||
private String getCookieValue(Cookie[] cookies) { | ||
if (cookies == null) { | ||
return null; | ||
} | ||
return header.replaceFirst(BEARER, ""); | ||
|
||
return Arrays.stream(cookies) | ||
.filter(cookie -> ACCESS_TOKEN.equals(cookie.getName())) | ||
.map(Cookie::getValue) | ||
.findFirst() | ||
.orElse(null); | ||
} |
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.
명시적으로 null
을 다루기보다는 Optional
API를 활용해 보면 어떨까요?
Cookie[] cookies = request.getCookies(); | |
String token = getCookieValue(cookies); | |
if (token == null) { | |
throw new MomoException(AuthErrorCode.NOT_FOUND_TOKEN); | |
} | |
return jwtManager.extract(token); | |
} | |
private String getToken(NativeWebRequest webRequest) { | |
String header = webRequest.getHeader(AUTHORIZATION); | |
if (header == null) { | |
throw new MomoException(AuthErrorCode.NOT_FOUND_TOKEN); | |
private String getCookieValue(Cookie[] cookies) { | |
if (cookies == null) { | |
return null; | |
} | |
return header.replaceFirst(BEARER, ""); | |
return Arrays.stream(cookies) | |
.filter(cookie -> ACCESS_TOKEN.equals(cookie.getName())) | |
.map(Cookie::getValue) | |
.findFirst() | |
.orElse(null); | |
} | |
String token = getCookieValue(cookies).orElse(""); | |
return jwtManager.extract(token); | |
} | |
private Optional<String> getCookieValue(Cookie[] cookies) { | |
if (cookies == null) { | |
return Optional.empty(); | |
} | |
return Arrays.stream(cookies) | |
.filter(cookie -> ACCESS_TOKEN.equals(cookie.getName())) | |
.map(Cookie::getValue) | |
.findFirst(); | |
} |
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.
현재 로직에 따르면 cookies가 null일 때 예외가 발생해야합니다.
그런데 제안하신 로직에서는 cookies가 null일 때 예외가 발생하지 않고 ""이 반환되는 것 같아요..!
아니면 아래처럼 resolveArgument() 함수 내에서 token의 null 체크 부분을 삭제하고 getCookieValue()함수에서 optional을 사용해서 cookie가 null 값일 때 예외를 발생시키는 방법도 있을 것 같습니다.
@Override
public Long resolveArgument(
@NonNull MethodParameter parameter,
ModelAndViewContainer mavContainer,
@NonNull NativeWebRequest webRequest,
WebDataBinderFactory binderFactory
) {
HttpServletRequest request = (HttpServletRequest) webRequest.getNativeRequest();
Cookie[] cookies = request.getCookies();
String token = getCookieValue(cookies);
return jwtManager.extract(token);
}
private String getCookieValue(Cookie[] cookies) {
cookies = Optional.ofNullable(cookies)
.orElseThrow(() -> new MomoException(AuthErrorCode.NOT_FOUND_TOKEN));
return Arrays.stream(cookies)
.filter(cookie -> ACCESS_TOKEN.equals(cookie.getName()))
.map(Cookie::getValue)
.findFirst()
.orElseThrow(() -> new MomoException(AuthErrorCode.NOT_FOUND_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.
@ehBeak
페드로 제안의 의미는 쿠키의 토큰이 JwtManger의 verifyToken 메서드 한 곳에서 검증 관리되는 게 응집도 측면에서 자연스럽다는 것을 말하는 것 같습니다.
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.
해당 맥락에서 쿠키는 결국 토큰이므로 null
일 때의 예외 처리 역시 JwtManager
의 책임이라고 생각했어요🙂
|
||
RestAssured.given().log().all() | ||
.cookie("ACCESS_TOKEN", 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.
Fixture 에 상수로 추가해줘도 될 것 같아요!
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 void login( | ||
@PathVariable String uuid, @RequestBody @Valid AttendeeLoginRequest request, HttpServletResponse response |
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.
HttpServletResponse
는 헤더 설정을 위해 넣은 것으로 보여요.
저번에 헤더 값을 넣을 때 HttpServletResponse
대신, ResponseEntity
를 사용하기로 이야기 했던 것 같습니다. (이야기 나왔던 pr링크) ResponseEntity
를 헤더 값을 바꿔 보는 것은 어떨까요?
만약 ResponseEntity로 수정하게 된다면 아래와 같이 수정하게 될 것 같습니다..!
@PostMapping("/api/v1/meetings/{uuid}/login")
public ResponseEntity<Void> login(@PathVariable String uuid, @RequestBody @Valid AttendeeLoginRequest request) {
String token = attendeeService.login(uuid, request);
String path = String.format("/api/v1/meetings/%s/", uuid);
return ResponseEntity.ok()
.header(HttpHeaders.SET_COOKIE, createCookie(token, path).toString())
.build();
}
private ResponseCookie createCookie(String value, String path) {
return ResponseCookie.from(ACCESS_TOKEN, value)
.httpOnly(true)
.secure(true)
.path(path)
.build();
}
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.
오 좋은데요? 👍
Cookie cookie = createCookie(token, path); | ||
response.addCookie(cookie); |
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.
HttpServletResponse는 헤더 설정을 위해 넣은 것으로 보여요.
저번에 헤더 값을 넣을 때 HttpServletResponse 대신, ResponseEntity를 사용하기로 이야기 했던 것 같습니다. (#56) ResponseEntity를 헤더 값을 바꿔 보는 것은 어떨까요?
혹시 response.addCookie(cookie)에서 기존에 있던 쿠키를 유지하고 새로 만들어진 쿠키를 추가하는 것일까요? 만약 그 점을 의도하셨다면, 위에 말씀드린 ResponseEntity를 사용한 방법으로는 힘들 것 같네요. 방법을 찾아보거나 이대로 유지해야 겠네요..!
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.
Set-Cookie
가 설정된 경우 브라우저는 기존 쿠키들을 전부 지우고 해당 헤더에 명시된 쿠키만을 설정하는게 아니라, 헤더에 명시되어 있는 쿠키만 업데이트/추가 하는것으로 알고 있어요.
동일한 도메인과 경로에 동일한 이름을 가진 쿠키가 설정될 때만 기존 쿠키가 새로운 값으로 업데이트됩니다.
ResponseEntity
를 사용한 방법으로는 힘들 것 같다는게 어떤 말씀이신지 좀 더 자세히 설명해 주실 수 있으신가요?
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 new MomoApiResponse<>(attendeeService.login(uuid, request)); | ||
String token = attendeeService.login(uuid, request); | ||
String path = String.format("/api/v1/meetings/%s/", uuid); |
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 path = String.format("/api/v1/meetings/%s/", uuid); | |
String path = String.format("/api/v1/meetings/%s", uuid); |
@@ -158,12 +158,12 @@ void lock() { | |||
.when().post("/api/v1/meetings/{uuid}/login", meeting.getUuid()) | |||
.then().log().all() | |||
.statusCode(HttpStatus.OK.value()) | |||
.extract().jsonPath().getString("data.token"); | |||
.extract().cookie("ACCESS_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.
바로 추출할 수 있네요. 좋은데요? 😊
bdf1760
to
58b9ab0
Compare
관련 이슈
작업 내용
HttpOnly
,Secure
옵션을 활성화 하였습니다.HttpOnly
Secure
특이 사항
리뷰 요구사항 (선택)