1주차 인증 리뷰 부탁드립니다. :) #38
Conversation
| import java.io.IOException; | ||
| import java.util.List; | ||
|
|
||
| public class FilterChainProxy extends GenericFilterBean { |
There was a problem hiding this comment.
궁금한점은 왜 VirtualFilterChain 에 FilterChainDecorator를 입혀 spring security에서는 사용하고 있는지 입니다.
VirtualFilterChain의 역할과 FilterChainDecorator의 역할의 역할이 달라서 그런데요.
VirtualFilterChain는 특정 조건을 기반으로 필터들을 실행하는 역할을 하고, FilterChainDecorator는 기존 FilterChain에 추가적인 로직을 삽입하거나 수정할 수 있도록 도와주는 역할을 합니다.
예시를 들어보자면 /a 요청과 /b 요청에 다른 보안 필터를 적용한다고 생각했을 때 VirtualFilterChain가 각 필터를 실행하는 역할을 담당하게 되고,
이미 구현된 필터를 감싸서 가능을 확장할 때 사용하는 것이 FilterChainDecorator의 역할이 됩니다.
즉 필터 체인을 동적으로 조작한다는 점에선 동일하지만, 목적과 사용 방식이 다를 것 같아요 ㅎㅎ
There was a problem hiding this comment.
FilterChainDecorator 디자인 패턴의 데코레이터 패턴과는 다른 그냥 래퍼역할을 하는 인터페이스인것 같은데 이름의 연관성이 있는지 궁금합니다.
정석적인 데코레이터 패턴은 아니지만 FilterChain을 감싸서 기존 필터를 수정하지 않고 추가적인 필터를 실행한다는 관점에선 데코레이터의 역할을 하고 있다고 보여집니다.
따라서 데코레이터 디자인 패턴의 완벽한 구현체이다!는 아니지만, 데코레이터 패턴에서 얘기하는 핵심 개념인 원본을 유지한 상태로 확장이 가능하다. 라는 점에서는 데코레이터라는 이름이 어색하진 않은 것 같다는 의견 드립니다 :)
There was a problem hiding this comment.
security 패키지안에 securityconfig라는 클래스에 delegatingfilterproxy, filterchianproxy 빈들을 설정하고 userService 빈만 app 패키지 안에서 설정했습니다. 그런데 필터 등록이 안되더라고요.(filterchainproxy)
이것에 대해 명확하게 왜 그런건지 이유를 모르겠습니다.
정확한 것은 해당 코드를 봐야겠지만.. 가볍게 추측해 보자면
- SecurityAuthenticationApplication에서 componentScan을 변경하지 않았다.
- 설정한 Config에
@Configuration을 붙이지 않았다.
둘 중 하나가 아닐까 감히 예측해봅니다.. ㅋㅋ
There was a problem hiding this comment.
security 패키지안에 securityconfig라는 클래스에 delegatingfilterproxy, filterchianproxy 빈들을 설정하고 userService 빈만 app 패키지 안에서 설정했습니다. 그런데 필터 등록이 안되더라고요.(filterchainproxy)
이것에 대해 명확하게 왜 그런건지 이유를 모르겠습니다.정확한 것은 해당 코드를 봐야겠지만.. 가볍게 추측해 보자면
- SecurityAuthenticationApplication에서 componentScan을 변경하지 않았다.
- 설정한 Config에
@Configuration을 붙이지 않았다.둘 중 하나가 아닐까 감히 예측해봅니다.. ㅋㅋ
앗 당연히 @SpringBootApplication 붙은 application 클래스하위만 componentscan 대상이 되니.. 이제 보이는것 같네요. 감사합니다..ㅎ
| if (this.getPrincipal() instanceof UserDetails userDetails) { | ||
| return userDetails.getUsername(); | ||
| } | ||
| if (this.getPrincipal() instanceof Principal principal) { | ||
| return principal.getName(); | ||
| } | ||
| return (this.getPrincipal() == null) ? "" : this.getPrincipal().toString(); |
There was a problem hiding this comment.
principal이 추가될 때 마다 해당 클래스를 수정해야 하는 것은 문제가 있어 보이는데요.
생성 시점에 principal에서 필요한 값만 추출하여 넘겨 받는다면 해당 문제를 해결할 수 있을 것 같아요!
| import java.io.Serializable; | ||
| import java.security.Principal; | ||
|
|
||
| /** |
| public class DaoAuthenticationProvider implements AuthenticationProvider{ | ||
|
|
||
| private final UserDetailService userDetailService; | ||
| private final PasswordEncoder passwordEncoder; |
There was a problem hiding this comment.
네 passwordencoder는 추후 미션 진행하면서 조금 더 뜯어볼 예정입니다!
| if (result == null) { | ||
| throw new ProviderNotFoundException("지원하는 provider가 없습니다!"); | ||
| } |
There was a problem hiding this comment.
지원하는 provider를 찾아도 authenticate 시 null이 응답된다면 해당 예외 메시지가 출력되겠네요.
의도한 내용이 맞을까요?
There was a problem hiding this comment.
지원하는 provider를 찾아도 authenticate 시 null이 응답된다면 해당 예외 메시지가 출력되겠네요. 의도한 내용이 맞을까요?
네 맞습니다!
AuthenticationManager에서 인증을 support하는 AuthenticationProvider가 없다면 해당 예외를 던지게 만들어 놓았습니다!
ProviderManager 코드를 참고했는데요, ProviderManager에서는 parent AuthenticationManager가 존재한다면 부모 AuthenticationManager가 authenticate() 를 호출하는데, 결국은 그 부모도 support하는 AuthenticationProvider가 없다면 ProviderNotFoundException을 던지길래 참고했습니다.!
ProviderManager의 authenticate() 일부
if (result == null && this.parent != null) {
// Allow the parent to try.
try {
parentResult = this.parent.authenticate(authentication);
result = parentResult;
}
catch (ProviderNotFoundException ex) {
// ignore as we will throw below if no other exception occurred prior to
// calling parent and the parent
// may throw ProviderNotFound even though a provider in the child already
// handled the request
}
catch (AuthenticationException ex) {
parentException = ex;
lastException = ex;
}
....
// Parent was null, or didn't authenticate (or throw an exception).
if (lastException == null) {
lastException = new ProviderNotFoundException(this.messages.getMessage("ProviderManager.providerNotFound",
new Object[] { toTest.getName() }, "No AuthenticationProvider found for {0}"));
}
....
}There was a problem hiding this comment.
그런데 궁금한점은 ProviderManager 는 내부에 parent 프로퍼티로 AuthenticationManager 가져서 AuthenticationManager 를 계속해서 이을수 있다는것을 확인했는데요,
왜 이렇게 만들었을지 재연님은 어떻게 생각하시나요?
왜 parent를 프로퍼티로 두었을까요?
여러 AuthenticationManager 간의 인증 절차를 유연하게 설정하기 위해 관계 설정을 했던것일것 같긴한데 궁금합니다 :)
There was a problem hiding this comment.
provider를 찾지 못한 케이스와 provider를 찾았지만, 인증을 하지 못한 경우의 케이스는 예외 메시지를 다르게 가는게 좋을 것 같은데 어떻게 생각하시나요?
There was a problem hiding this comment.
parent를 가지고 있는 이유는 현재 ProviderManager에서 처리할 수 없을 경우, parent에 위임하는 계층형 구조를 지원하기 위해 저러한 설계가 된 것 같아요 ㅎㅎ
| } | ||
|
|
||
| // 인증된 토큰에 detail 값이 없다면 detail 넣어주기 | ||
| private void copyDetails(Authentication source, Authentication dest) { |
There was a problem hiding this comment.
ProviderManager는 여러 개의 AuthenticationProvider를 관리하고, 올바른 Provider를 선택하여 인증을 수행하는 역할을 담당하게 되는데요.
이 역할은 ProviderManager이 아닌 AuthenticationProvider의 역할에 더 적절해 보이는데 어떻게 생각하시나요?
There was a problem hiding this comment.
여기서 ProviderManager 는 지원하는 AuthenticationProvider를 순회하는 역할을 한다고 생각했습니다.
인증 로직 자체는 각 AuthenticationProvider에서 진행을 한다고 생각했어요. (마치 FilerChainProxy와 SecurityFilterChain 과 비슷한 관계같다고도 생각했습니다.)
AuthenticationProvider 는 인증된다면 Authentication 이라는 토큰객체에 사용자 정보를 담아 넘겨주는 역할을 하고요.
그런데 여기서는 result라는 Authentication 변수를 ProviderManager에서 선언하고, null인지 아닌지에 따라 로직을 다르게 태우기 때문에 ProviderManager에 있는게 더 적절해 보인다고 판단했습니다.
혹시 재연님은 다르게 생각하신다면 이유가 궁금합니다!
There was a problem hiding this comment.
copyDetails 내부에서 AbstractAuthenticationToken 타입을 체크하는 것은 객체지향 원칙(SOLID) 중 개방폐쇄 원칙와 리스코프 치환 원칙에 위배될 가능성이 있어 남긴 코멘트였는데요.
ProviderManager는 인증 프로세스를 관리하는 역할이라면, 특정 구현체에 의존하지 않는 것이 더 좋은 설계일 것 같은데 어떻게 생각하시나요?
내부에서 타입을 체크하지 않고 인증 프로세스를 관리하는 역할까지 가져갈 수 있도록 해결할 방법이 있을지 고민해보면 좋겠습니다!
| if (isHttpServlet) { | ||
| HttpServletRequest request = (HttpServletRequest) servletRequest; | ||
| HttpServletResponse response = (HttpServletResponse) servletResponse; | ||
|
|
||
| try { | ||
| boolean notTarget = isNotTarget(request); | ||
| if (notTarget) { | ||
| filterChain.doFilter(request, response); | ||
| return; | ||
| } | ||
|
|
||
| checkAuthentication(request); | ||
| filterChain.doFilter(request, response); | ||
| } catch (Exception e) { | ||
| response.setStatus(HttpServletResponse.SC_UNAUTHORIZED); | ||
| } | ||
| return; | ||
| } | ||
| throw new ServletException("BasicAuthFilter only supports HTTP requests"); |
There was a problem hiding this comment.
코드의 depth가 길어지면 코드의 가독성이 떨어지게 되는데요.
그런 관점에서 early return하는 코드를 작성한다면 가독성이 올라갈 것 같습니다!
| if (isHttpServlet) { | |
| HttpServletRequest request = (HttpServletRequest) servletRequest; | |
| HttpServletResponse response = (HttpServletResponse) servletResponse; | |
| try { | |
| boolean notTarget = isNotTarget(request); | |
| if (notTarget) { | |
| filterChain.doFilter(request, response); | |
| return; | |
| } | |
| checkAuthentication(request); | |
| filterChain.doFilter(request, response); | |
| } catch (Exception e) { | |
| response.setStatus(HttpServletResponse.SC_UNAUTHORIZED); | |
| } | |
| return; | |
| } | |
| throw new ServletException("BasicAuthFilter only supports HTTP requests"); | |
| if (!isHttpServlet) { | |
| throw new ServletException("BasicAuthFilter only supports HTTP requests"); | |
| } | |
| HttpServletRequest request = (HttpServletRequest) servletRequest; | |
| HttpServletResponse response = (HttpServletResponse) servletResponse; | |
| try { | |
| boolean notTarget = isNotTarget(request); | |
| if (notTarget) { | |
| filterChain.doFilter(request, response); | |
| return; | |
| } | |
| checkAuthentication(request); | |
| filterChain.doFilter(request, response); | |
| } catch (Exception e) { | |
| response.setStatus(HttpServletResponse.SC_UNAUTHORIZED); | |
| } | |
There was a problem hiding this comment.
피드백 반영하겠습니다! 감사합니다 :)
이번 미션에서는 기본적으로 security의 동작 원리 중심으로 보고있어 신경을 쓰지 못했지만 가독성도 고려해서 작성해보겠습니다! 👍
| import nextstep.security.util.Base64Convertor; | ||
| import org.springframework.web.servlet.HandlerInterceptor; | ||
|
|
||
| public class BasicAuthInterceptor implements HandlerInterceptor { |
There was a problem hiding this comment.
BasicAuthInterceptor와 FormLoginInterceptor는 사용되지 않는 것 같은데, 어떤 이유로 추가하셨을까요?
There was a problem hiding this comment.
이건 미션진행하다 남긴 유물들인데 지우지않고 학습용으로 남겨놓았습니다!
처음에 interceptor의 로직 > filter로 변환 과정에서 생겼던 것들입니다:)
…nSecurityContextRepository 구현
|
재연님 4단계까지 미션 완료했습니다! 후기를 말씀드리자면, 스프링 시큐리티 코드를 많이 뜯어봤는데, 프레임워크 코드를 이렇게 뜯어본 적이 처음이었습니다. 궁금한점은
boolean supports(Class<?> authentication);instanceof로 타입을 체크해줄수도 있었을건데 왜 이렇게 한건지... 재연님 생각은 어떠신가요?
|
| if (result == null) { | ||
| throw new ProviderNotFoundException("지원하는 provider가 없습니다!"); | ||
| } |
There was a problem hiding this comment.
provider를 찾지 못한 케이스와 provider를 찾았지만, 인증을 하지 못한 경우의 케이스는 예외 메시지를 다르게 가는게 좋을 것 같은데 어떻게 생각하시나요?
| if (result == null) { | ||
| throw new ProviderNotFoundException("지원하는 provider가 없습니다!"); | ||
| } |
There was a problem hiding this comment.
parent를 가지고 있는 이유는 현재 ProviderManager에서 처리할 수 없을 경우, parent에 위임하는 계층형 구조를 지원하기 위해 저러한 설계가 된 것 같아요 ㅎㅎ
| } | ||
|
|
||
| // 인증된 토큰에 detail 값이 없다면 detail 넣어주기 | ||
| private void copyDetails(Authentication source, Authentication dest) { |
There was a problem hiding this comment.
copyDetails 내부에서 AbstractAuthenticationToken 타입을 체크하는 것은 객체지향 원칙(SOLID) 중 개방폐쇄 원칙와 리스코프 치환 원칙에 위배될 가능성이 있어 남긴 코멘트였는데요.
ProviderManager는 인증 프로세스를 관리하는 역할이라면, 특정 구현체에 의존하지 않는 것이 더 좋은 설계일 것 같은데 어떻게 생각하시나요?
내부에서 타입을 체크하지 않고 인증 프로세스를 관리하는 역할까지 가져갈 수 있도록 해결할 방법이 있을지 고민해보면 좋겠습니다!
|
|
||
| import static org.springframework.http.HttpHeaders.AUTHORIZATION; | ||
|
|
||
| public class BasicAuthFilter implements Filter { |
There was a problem hiding this comment.
한가지 이해가 안되는게 있는데, 마지막에 BasicAuthentication과 UsernamePasswordAuthentication이 있을때, 세션이 있다면 BasicAuthentication에서 인증할 필요가 없도록 구조를 바꿔야하는게 맞을까요?
스프링 시큐리티를 구현하며 학습하고 있기 때문에 스프링 시큐리티 구조가 기준점이 되긴 하는데요.
스프링 시큐리티가 정답이라는 틀에서 벗어나 직접 설계를 고민해보면 더 의미 있는 학습이 될 것 같아요!
세션이 있을 때 인증을 한다면 어떠한 장점이 있고, 항상 수행하면 어떤 장점이 있을지 고민 후 준호님의 설계에 맞춰 트레이드 오프를 선택해보면 좋을 것 같은데요.
준호님이 생각하시는 장단점은 어떤 것이 있을까요?
그리고 지금 설계에서 어떤 부분을 더 중요하게 고려하고 싶은지도 궁금합니다 ㅎㅎ
| * @param authentication | ||
| * @return <code>true</code> if the implementation can more closely evaluate the Authentication class presented | ||
| */ | ||
| boolean supports(Class<?> authentication); |
There was a problem hiding this comment.
provider manager에서 왜 support 할때 리플렉션을 통한 클래스 정보를 넘길까요?
instanceof를 사용할 경우 상속 관계에서 확인이 어렵고, 타입을 체크하기 위해선 무조건 객체가 생성되어야 하기 때문에 클래스 정보로 확인하는 것이 더 유연한 설계로 보여지네요!
| } | ||
|
|
||
| @Bean | ||
| public DelegatingFilterProxy delegatingFilterProxy() { |
There was a problem hiding this comment.
Spring에서 관리하는 필터 (Security filter)의 경우 GenericFilterBean이나 OncePerRequestFilter같은걸 사용하던데 이건 스프링 의존성을 가지고 있어 Filter를 implement하는것보다 bean에 대한 추가적인 정보 등을 가져올 수 있더라고요.
궁금한게, delegatingFilterproxy는 tomcat에 등록되는 filter라 GenericFilterBean를 사용하는게 의미가 없어 보이는데,
filterchainproxy의 virtualfilterchain을 타고 어떻게 스프링과 이어지는 그 경계가 궁금합니다. (delegatingfilterproxy 필터 뒤에 톰켓을통해 등록하는 필터들이 돌기전에 security filter로 등록된 스프링관련 동작이 작동하는건가요?)
서블릿과 스프링 시큐리티의 경계 지점이 DelegatingFilterProxy이라고 볼 수 있습니다.
먼저 요청의 흐름을 보면
- 클라이언트 요청 (Tomcat)
- Tomcat 필터 체인 실행
- DelegatingFilterProxy (Spring Security)
- FilterChainProxy
- SecurityFilterChain
- Spring Security 필터 끝
- Tomact 필터 실행
순서로 진행된다고 볼 수 있습니다.
즉, 질문주신 대로 톰캣을 통해 등록된 필터들이 동작하기 전에 스프링 시큐리티로 등록된 스프링 관련 동작이 먼저 수행되게 됩니다!
| @@ -0,0 +1,32 @@ | |||
| package nextstep.security.config; | |||
There was a problem hiding this comment.
지금은 config 패키지에 너무 많은 정보가 모여있는데, 의미있는 컨텍스트를 묶어보면 어떨까요?
| * SecurityContextHolder.getContext()를 가져와 SecurityContextRepository.saveContext() 호출. | ||
| * SecurityContextHolder.clearContext()로 정리. | ||
| */ | ||
| public class SecurityContextHolderFilter extends GenericFilterBean { |
There was a problem hiding this comment.
OncePerRequestFilter가 아닌 GenericFilterBean로 구현해주신 이유가 있을까요?
| */ | ||
| public class SecurityContextHolderFilter extends GenericFilterBean { | ||
|
|
||
| private final SecurityContextRepository securityContextRepository = new HttpSessionSecurityContextRepository(); |
There was a problem hiding this comment.
지금은 SecurityContextRepository가 인터페이스로 분리되어 있지만, 내부에서 초기화하고 있기 때문에 확장성이 떨어지는 구조네요.
어떻게 확장이 가능한 구조로 만들 수 있을까요?
| private static final List<String> targetURIList = List.of("/login"); | ||
|
|
||
| private final AuthenticationManager authenticationManager; | ||
| private final SecurityContextRepository securityContextRepository; |
There was a problem hiding this comment.
해당 필터에서 SecurityContextRepository에 대한 의존성을 가지고 있는데요.
SecurityContextRepository와 관련된 역할은 한 Filter로 모아서 처리할 수 있으면 코드의 응집도가 높아질 것 같습니다!
안녕하세요!
시큐리티가 아직 익숙하지 않아 공식문서들을 계속 읽어보고 설정 및 깨진 테스트 코드를 수정하느라 아직 1단계 까지 밖에 진행하지 못한상황인데 금일까지 진행 완료하겠습니다! (일단 한데까지라도 올려봐요)
저는 spring의 동작원리도 책으로보고 코드도 겉핥기로 알고 있어 이렇게 직접 코드를 뜯어본 적은 처음인데요, 감안하고 봐주시면 감사하겠습니다 😢
제가 진행하면서 느낀 궁금한 점은 다음과 같아요.
Filterchain proxy에서 securityfilter.getfilter를 통해가져온 filter를 돌면서 dofilter를 VirtualFilterChain을 통해 실행시키는것을 확인했습니다.
궁금한점은 왜
VirtualFilterChain에FilterChainDecorator를 입혀 spring security에서는 사용하고 있는지 입니다.위에서 첫번째 this.filterChainDecorator.decorate(chain)는 결국 타고들어가보면 결국 chain의 dofilter를 호출하는데 왜 이렇게 복잡하게 추상화했을까? 란 생각이 들었습니다.
그리고
FilterChainDecorator디자인 패턴의 데코레이터 패턴과는 다른 그냥 래퍼역할을 하는 인터페이스인것 같은데 이름의 연관성이 있는지 궁금합니다.셋째로, 제가 가장 많은 시간을 들인 부분인데, 처음에 제가 security 패키지안에 securityconfig라는 클래스에 delegatingfilterproxy, filterchianproxy 빈들을 설정하고 userService 빈만 app 패키지 안에서 설정했습니다. 그런데 필터 등록이 안되더라고요.(filterchainproxy)
이것에 대해 명확하게 왜 그런건지 이유를 모르겠습니다.
더 진행하면서 궁금한점은 댓글로 남기겠습니다!
재연님 잘부탁드립니다! 감사합니다☺️