1단계 - 인증(Authentication) 리뷰 요청 드립니다. #41
Conversation
…_SECURITY_CONTEXT_KEY private 접근제어자로 변경
| private final String principal; | ||
| private final String credentials; | ||
| private boolean authenticated = false; | ||
| private UserDetails userDetails; |
There was a problem hiding this comment.
AuthenticationToken은 말그대로 인증된 토큰이기 때문에 UserDetails는 필요로 하지 않습니다.
UserDetails의 사용처를 생각해보면, 최초 인입된 유저가 인증이 가능한 유저인지 확인하기 위해서 사용될 뿐이지 이미 인증된 토큰에는 별도로 UserDetails를 사용하지 않습니다. 현재 만들어주신 코드에도 사용처가 없기도 하구요.
| public UsernamePasswordAuthenticationToken(String principal, String credentials) { | ||
| this.principal = principal; | ||
| this.credentials = credentials; | ||
| } |
There was a problem hiding this comment.
사용되지 않는 메소드들은 모두 제거해주시면 좋을 것 같아요 :)
| @Override | ||
| public Authentication authenticate(Authentication authentication) throws AuthenticationException { | ||
|
|
||
| Class<? extends Authentication> toTest = authentication.getClass(); |
| for (AuthenticationProvider provider : this.providers) { | ||
| if (provider.supports(toTest)) { | ||
|
|
||
| try { | ||
| result = provider.authenticate(authentication); | ||
| if (result != null) { | ||
| break; | ||
| } | ||
| } catch (AuthenticationException ex) { | ||
| lastException = ex; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (result != null) { | ||
| return result; | ||
| } | ||
| if (lastException == null) { | ||
| lastException = new ProviderNotFoundException(String.format("No AuthenticationProvider found for %s", toTest.getName())); | ||
| } | ||
|
|
||
| throw lastException; |
There was a problem hiding this comment.
| for (AuthenticationProvider provider : this.providers) { | |
| if (provider.supports(toTest)) { | |
| try { | |
| result = provider.authenticate(authentication); | |
| if (result != null) { | |
| break; | |
| } | |
| } catch (AuthenticationException ex) { | |
| lastException = ex; | |
| } | |
| } | |
| } | |
| if (result != null) { | |
| return result; | |
| } | |
| if (lastException == null) { | |
| lastException = new ProviderNotFoundException(String.format("No AuthenticationProvider found for %s", toTest.getName())); | |
| } | |
| throw lastException; | |
| return providers.stream() | |
| .filter(provider -> provider.supports(toTest)) | |
| .map(provider -> provider.authenticate(authentication)) | |
| .filter(Objects::nonNull) | |
| .findAny() | |
| .orElseThrow(); |
로 표현될 수 있을 것 같네요 :)
| @@ -0,0 +1,40 @@ | |||
| package nextstep.authentication.filter; | |||
There was a problem hiding this comment.
SecurityContextHolder를 관리하는 필터이기 때문에 context 패키지에 이동하는 것이 적절해보이네요~
| filterChain.doFilter(servletRequest, servletResponse); | ||
| } finally { | ||
| securityContextRepository.saveContext(SecurityContextHolder.getContext(), request, response); | ||
| SecurityContextHolder.clearContext(); |
| if (authenticationResult == null) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
인증이 null일 때 바로 return하면 필터 동작이 어디로 향할까요?
There was a problem hiding this comment.
인증이 null일 때 다음 필터로 이동할 수 있도록 filterChain.doFilter(servletRequest, servletResponse); 코드를 추가했습니다!
| filterChain.doFilter(servletRequest, servletResponse); | ||
| } | ||
|
|
||
| private Authentication attemptAuthentication(HttpServletRequest request, HttpServletResponse response) throws AuthenticationException { |
There was a problem hiding this comment.
attempAuthentication하는 동작은 request에서 헤더를 보고 토큰을 읽어 authenticationManager.authenticate를 실행하는 과정인데요. 이 과정에서 SecurityContext는 사실 불필요한 동작입니다.
현재 필터에서의 동작은
- 헤더에 원하는 값을 뽑아내어 토큰을 생성한다.
- 토큰을 가지고
AuthenticationManager.authenticate를 실행한다. - 성공하면 SecurityContext에 인증 값을 넣는다.
- 실패하면 초기화하고 401을 반환한다.
입니다. 즉, SecurityContext를 조작하는 행위는 모든 인증 과정이 끝나고 성공과 실패에 따른 행위이지 그 중간에 SecurityContext를 건들진 않아요.
There was a problem hiding this comment.
SecurityContext를 사용하지 않으면 FormLoginTest.login_after_members() 테스트가 통과하지 않는데 테스트를 수정하는게 좋을까요?
There was a problem hiding this comment.
SecurityContext를 사용하지 않으려고 아래처럼 수정을 시도했었습니다.
private Authentication attemptAuthentication(HttpServletRequest request) throws AuthenticationException {
String authorization = request.getHeader("Authorization");
String credentials = authorization.split(" ")[1];
String decodedString = Base64Convertor.decode(credentials);
String[] usernameAndPassword = decodedString.split(":");
String username = usernameAndPassword[0];
String password = usernameAndPassword[1];
UsernamePasswordAuthenticationToken authRequest = UsernamePasswordAuthenticationToken.unauthenticated(username, password);
return authenticationManager.authenticate(authRequest);
}| HttpSession httpSession = request.getSession(false); | ||
| SecurityContext context = this.readSecurityContextFromSession(httpSession); | ||
| if (context == null) { | ||
| return SecurityContextHolder.createEmptyContext(); | ||
| } | ||
|
|
||
| return context; |
There was a problem hiding this comment.
로직이 복잡할 필요 없이 loadContext는 단순히
- request.getSession의 값이 null이면 null을 반환한다.
- �httpSession.getAttribute(SPRING_SECURITY_CONTEXT_KEY)를 반환한다.
면 됩니다. readSecurityContextFromSession의 분기는 위 과정으로 이미 동일하게 로직이 구성될 수 있어 별도로 필요 없어보여요.
안녕하세요 진영님
1단계 인증 step2~4에 대한 리뷰 요청드립니다.
FilterChainProxy가 여러 개의 SecurityFilterChain을 갖고 있는 형태로 변경하는 부분은 시간 상 구현하지 못했습니다.
적절한 패키지로 나누는 것도 한번 고민해보겠습니다.
감사합니다.