-
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
[FIX/#41] JWT 필터 내 whilelist 처리 오류 해결 #42
Conversation
… 헤더 검증에 대해 자동으로 false 처리
|
@hyunw9 @yummygyudon hotfix라 머지하지만 리뷰는 꼭 남겨주세요!! |
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.
몇 가지 중요한 내용 리뷰 남겨보았어요..!!
긴급했던 만큼 리뷰를 진행한 후에 반영이 되지 못했지만 아마 예상하기로는 완전히 문제가 해결되지는 않았을 것 같다는 생각입니다...!!
if (shouldNotFilter(request)) { | ||
filterChain.doFilter(request, response); | ||
return; | ||
} | ||
|
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.
P1
해당 로직은 필요없습니다!
OncePerRequestFilter 소스 코드를 확인하면 이해하기 좋은데요!!
내부적으로 shouldNotFilter
메서드를 통과해야지만 Filter의 로직이 수행되도록 구현되어있어요!!
// org.springframework.web.filter.OncePerRequestFilter 클래스 소스코드 발췌
public abstract class OncePerRequestFilter extends GenericFilterBean {
...
public final void doFilter(ServletRequest request, ServletResponse response, FilterChain filterChain) throws
ServletException, IOException {
if (request instanceof HttpServletRequest httpRequest) {
if (response instanceof HttpServletResponse httpResponse) {
String alreadyFilteredAttributeName = this.getAlreadyFilteredAttributeName();
boolean hasAlreadyFilteredAttribute = request.getAttribute(alreadyFilteredAttributeName) != null;
if (!this.skipDispatch(httpRequest) && !this.shouldNotFilter(httpRequest)) {
if (hasAlreadyFilteredAttribute) {
if (DispatcherType.ERROR.equals(request.getDispatcherType())) {
this.doFilterNestedErrorDispatch(httpRequest, httpResponse, filterChain);
return;
}
filterChain.doFilter(request, response);
} else {
request.setAttribute(alreadyFilteredAttributeName, Boolean.TRUE);
try {
// 여기에서 호출되는 것을 알 수 있습니다.
this.doFilterInternal(httpRequest, httpResponse, filterChain);
} finally {
request.removeAttribute(alreadyFilteredAttributeName);
}
}
} else {
filterChain.doFilter(request, response);
}
return;
}
}
throw new ServletException("OncePerRequestFilter only supports HTTP requests");
}
...
protected boolean shouldNotFilter(HttpServletRequest request) throws ServletException {
return false;
}
protected abstract void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) throws ServletException, IOException;
...
}
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.
오호 그렇군요 지금보니 override하는 메서드였네요 어제 문제 확인할때는 해당 내용을 몰랐어서 일단 임의로 수정하긴했는데, 제가 postman으로 요청보냈을 때에는 유빈이가 슬랙에서 요청준 문제는 해결되어서 추가로 요청 들어오면 긴급하게 수정하겠지만 일정상 다음주쯤에 다시 반영 가능할 것 같습니다!
@ExceptionHandler(RuntimeException.class) | ||
ResponseEntity<BaseResponse<?>> handleInternalException(final RuntimeException e) { | ||
log.error(e.getMessage()); | ||
return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR) | ||
.body(BaseResponse.ofFailure(INTERNAL_SERVER_ERROR)); | ||
} |
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.
P1
음 우선 filter의 경우 Dispatcher Servlet 도달 이전에 수행되는 계층이라 Dispatcher Servlet 도달 이후 역할을 수행하는 ExceptionHandler(@ControllerAdvice)에 Filter 단에서 발생하는 Custom Exception은 처리를 못합니다.
그렇기 때문에 직접적인 HttpServletResponse의 Body를 직접 조작해줘야 하는 것으로 알고 있어요!
(Body를 조작하는 메서드를 정의해서 적용하거나 별도 Exception에 따른 조작을 수행하는 Filter를 마지막 순서에 등록하는 방법이 있습니다.)
지금 확인해보니 본 프로젝트에서는 Error 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.
아 필터 단에서의 에러는 아직 반영되지 않은게 맞습니다! 제 의도는 RuntimeException보다 AuthException만 해당 핸들러에 반영되어 있어서 BaseException으로 바꾸는 겸 internalException도 추가한거였어요! 말씀해주신 부분이랑 shouldNotFilter 적용해서 다음주중으로 반영해두겠습니다!
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.
중요한 내용은 이미 동규형이 말씀을 해주셨네요 !
ShouldNotFilter는 오버라이드 해서 기존 로직 그대로 첨부하면 될 것 같습니다.
고생하셨습니다 !
import sopt.makers.authentication.support.code.base.*; | ||
|
||
import org.springframework.http.*; | ||
|
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.
P5
애스터리스크 제거해주시면 좋을것 같습니다~
|
||
import org.springframework.http.*; | ||
|
||
import lombok.*; |
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.
P5
요기도욥
Related Issue 🚀
Work Description ✏️
PR Point 📸