CSRF Filter, Refactoring 리뷰 요청드립니다.#3
Conversation
jaeyeonling
left a comment
jaeyeonling
left a comment
There was a problem hiding this comment.
안녕하세요! 마지막 미션을 함께 할 김재연입니다. 잘 부탁드립니다 🙇
전체적으로 깔끔하고 스프링 시큐리티 철학에 맞게 잘 구현해 주셨네요 👍
코멘트 남겨두었으니 확인 부탁드릴게요 ㅎㅎ
| return new FilterChainProxy(securityFilterChains); | ||
| public SecurityFilterChain securityFilterChain(HttpSecurity http) { | ||
| return http | ||
| .csrf(c -> c.ignoringRequestMatchers("/login", "/logout")) |
|
|
||
| public void addFilter(Filter filter) { | ||
| Integer order = this.filterOrders.getOrder(filter.getClass()); | ||
| if (order == null) { |
There was a problem hiding this comment.
지금은 Order가 없는 Filter의 경우 예외를 발생시키고 있는데요! 프레임워크에서 제공하는 Filter가 아닌 경우 Order가 존재하지 않는 것은 허용하는 설계가 더 좋을 것 같은데 어떻게 생각하시나요?
| import java.io.IOException; | ||
| import java.util.*; | ||
|
|
||
| public class HttpSecurity { |
| @Autowired | ||
| private ApplicationContext context; | ||
|
|
||
| @Bean(HTTP_SECURITY_BEAN_NAME) |
There was a problem hiding this comment.
빈 이름을 BEAN_NAME_PREFIX로 지정하신 이유가 있을까요?
| @Bean(HTTP_SECURITY_BEAN_NAME) | ||
| @Scope("prototype") | ||
| HttpSecurity httpSecurity() { | ||
| AuthenticationManagerBuilder authenticationBuilder = new AuthenticationManagerBuilder(context); |
There was a problem hiding this comment.
스프링 시큐리티 5.7 버전부터 AuthenticationManagerBuilder 대신 AuthenticationManager를 빈으로 직접 등록하는 방식을 권장하고 있는데요.
반영해달라는 내용은 아니고, 미션과 별개로 왜 그런 변화를 주었는지 고민해보면 재미있는 포인트가 될 것 같습니다 ㅎㅎ
| try { | ||
| validateToken(request, actualToken); | ||
| } catch (AccessDeniedException e) { | ||
| accessDeniedHandler.onAccessFailure(response, e); |
There was a problem hiding this comment.
accessDeniedHandler 호출 이후 종료하지 않고 다음 filterChain으로 전달하는 것이 의도가 맞으실까요?
| throws IOException, ServletException { | ||
| for (RequestMatcher ignoringRequestMatcher : ignoringRequestMatchers) { | ||
| if (ignoringRequestMatcher.matches(request)) { | ||
| filterChain.doFilter(request, response); |
There was a problem hiding this comment.
isIgnoreRequest라는 메서드 이름으로 봤을 때 내부적으로 filterChain을 호출한다는 생각을 못했는데요.
해당 메서드는 무시하는 요청인지 확인하는 역할만 담당하고, 필터에 대한 처리는 해당 메서드 응답에 따라 처리하도록 설계하는 방법은 어떨까요?
| request.setAttribute(actualToken.parameterName(), actualToken); | ||
| request.setAttribute(actualToken.headerName(), actualToken); |
There was a problem hiding this comment.
토큰을 저장하는 역할을 Repository가 아닌 Filter에서 처리하신 이유가 있을까요??
| import java.util.HashSet; | ||
| import java.util.Set; | ||
|
|
||
| public class CsrfConfigurer implements SecurityConfigurer { |
There was a problem hiding this comment.
필요 시 비활성화 할 수 있도록 csrf.disable()도 같이 구현해보면 어떨까요?
|
|
||
| private OAuth2UserService getOAuth2UserService(HttpSecurity http) { | ||
| ApplicationContext context = http.getSharedObject(ApplicationContext.class); | ||
| OAuth2UserService bean = context.getBean(OAuth2UserService.class); |
There was a problem hiding this comment.
전체적인 설계에서 확장할 수 있는 부분과 확장할 수 없는 부분을 명확히 구분하고,
확장 가능한 부분은 Configurer 형태로 제공하면 더 간결하고 사용성이 좋아질 것 같습니다~
예를 들어, Spring Security에서는
http.oauth2Login().userService(customUserService);
와 같이 OAuth2 설정을 Configurer를 통해 확장할 수 있도록 제공하고 있습니다.
2 participants
안녕하세요.
이번 미션은 저번 과정을 온라인으로 들어서인지, 조금 더 어려웠습니다.
또한 리팩터링을 진행하면서 왜 이렇게 해야할까 에 대한 생각으로 오래 걸린 미션인 것 같네요.
많은 피드백 부탁드립니다!