[step2] 리팩터링 #5
[step2] 리팩터링 #5
Conversation
- /members/me 요청 추가 및 SecurityFilterChain에 해당 요청에 대한 필터들 추가 - 테스트 추가
- AuthorizeRequestMatcherRegistry에 request별 matcher 등록 - 각 인가 필터에서 AuthorizeRequestMatcherRegistry에 등록된 attribute 체크
| @@ -2,8 +2,9 @@ | |||
|
|
|||
| import nextstep.security.access.matcher.MvcRequestMatcher; | |||
| import nextstep.security.authentication.*; | |||
| import nextstep.security.authorization.LoginAuthorizationFilter; | |||
| import nextstep.security.authorization.SessionAuthorizationFilter; | |||
There was a problem hiding this comment.
진님 말씀처럼 요구사항에 용어 정리가 잘 안되어 있었어요 🥲
첫번째 요구사항은 인증이 완료된 사용자의 정보를 응답하는게 맞습니다.
미션 문서를 만들 때는 인증을 위해 인증 정보를 전달하는 방식 중 하나가 로그인이라고 생각했었어요. (엄밀하게 말해서는 Form 기반 로그인이겠네요 ㅎㅎ)
인증 처리는 요청으로 들어온 인증 정보에 있는 값이 유효한지(요청에서 주장하는 사용자가 맞는지)를 확인하는 과정이고, 인증이 완료되었다는 것은 인증 정보로 들어온 principal, credential에 해당하는 사용자가 있다는 뜻이죠. 우리가 만드는 프레임워크에서는 인증이 완료 되었을 때 인증 객체(Authentication)에 isAuthenticated()를 호출하면 true 값이 반환 되구요.
어제 강의에서 다뤘던 것처럼 Form 기반 로그인은 인증 요청과 리소스 접근(인가) 요청이 분리되어 있기 때문에 인증 객체(를 담고있는 SecurityContext)의 영속화가 필요하구요. (이게 말씀하신 session에 setAttribute)
정리해보자면 아래와 같습니다.
- /members: 인증 완료된 'ADMIN' 사용자만 접근 가능
- /memebers/me: 인증 완료된 모든 사용자가 접근 가능
- 인증 방식은 Basic 인증, Form 기반 로그인을 지원
- 인증 완료: Authentication.isAuthenticated() == true
- Basic 인증: 인증 완료된 인증 객체를 인가 처리시 바로 사용
- Form 기반 로그인: 인증 완료된 인증 객체를 저장해뒀다가, 다음 요청에서 인가 처리시 사용
There was a problem hiding this comment.
네네, 인증과 인가,,,, 과연 로그인은 그럼 어디에 속하는가가 조금 많이 헷갈렸는데요.
분명 인가에 대한 내용 같지만 과제 중간 제목에 로그인한 사용자에 대한 체크라는 부분이 있어서, session에 setAttribute한 부분까지 체크를 해야하는가? 하는 의문이 컸습니다. 그래서 실제로 MemberTest에보면 session set 까지 해주는 부분이 있고요^_ㅠ
말씀주신 내용 참고해서 다시 코드 개선해보겠습니다!
제가 요구사항을 완벽하게 숙지하지 못해서 더 헷갈린 것 같아요. 🙏
| filters.add(new LoginAuthorizationFilter(securityContextRepository())); | ||
| filters.add(new RoleAuthorizationFilter()); | ||
| filters.add(new SessionAuthorizationFilter(securityContextRepository(), requestMatcherRegistryMapping())); | ||
| filters.add(new RoleAuthorizationFilter(requestMatcherRegistryMapping())); | ||
| return new DefaultSecurityFilterChain(new MvcRequestMatcher(HttpMethod.GET, "/members"), filters); |
There was a problem hiding this comment.
요청에 따라 인가 방법을 결정할 수 있게 되면서, securityFilterChain도 도 하나로 통일 할 수 있습니다.
이 부분 한 번 수정해보세요!
(+) 왜 securityFilterChain을 하나로 관리하는 요구사항이 있을지도 고민해보셨으면 좋겠어요!
There was a problem hiding this comment.
네엥... SessionAuthorizationFilter이 위 계속 문의드린 로그인 관련된 기능이었는데 제외시키고 나머지도 하나로 통일 시켜보겠습네다. 👍
| @@ -25,4 +25,11 @@ public ResponseEntity<List<Member>> list() { | |||
| return ResponseEntity.ok(members); | |||
| } | |||
|
|
|||
| @GetMapping("/members/me") | |||
| public ResponseEntity<Member> me() { | |||
| @@ -17,22 +16,22 @@ | |||
| import java.io.IOException; | |||
|
|
|||
| public class RoleAuthorizationFilter extends GenericFilterBean { | |||
| private static final MvcRequestMatcher DEFAULT_REQUEST_MATCHER = new MvcRequestMatcher(HttpMethod.GET, | |||
| "/members"); | |||
|
|
|||
| private static final String ADMIN_ROLE = "ADMIN"; | |||
| String attribute = requestMatcherRegistry.getAttribute((HttpServletRequest) request); | ||
| Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); | ||
| boolean isAuthorized = requestMatcherRegistry.isAuthorized(attribute, authentication); | ||
|
|
||
| if (!authentication.getAuthorities().contains(ADMIN_ROLE)) { | ||
| if (!isAuthorized) { |
There was a problem hiding this comment.
이와같이 사용한다면 굳이 attribute를 registry에서 꺼내오지 않고 registry에 메시지를 보내도 괜찮지 않을까요?
boolean isAuthorized = requestMatcherRegistry.isAuthorized(authentication);
| if (attribute.contains(AuthorizedUrl.AUTHORITY)) { | ||
| Pattern pattern = Pattern.compile("\\((.*?)\\)"); | ||
| Matcher matcher = pattern.matcher(attribute); | ||
| if(matcher.find()) { | ||
| String role = matcher.group(1).trim(); | ||
| return authentication.getAuthorities().contains(role); | ||
| } | ||
| } else if (attribute.contains(AuthorizedUrl.AUTHENTICATED)) { | ||
| return authentication.isAuthenticated(); | ||
| } |
There was a problem hiding this comment.
인가 방법이 늘어날수록 if - else if - else if ... 구문이 추가되겠네요.
지금은 mappings가 String으로 되어있는데, 이 부분을 인가에 필요한 값을 쉽게 가져올 수 있는 구조로 변경해봐도 좋을 것 같아요.
There was a problem hiding this comment.
메소드 하나 내에서 난리친것을 인정합니다.. 반성합니다..
예쁘게 추상화 해보겠습네다 🙏
안녕하세요..! 리팩토링 구현 PR 요청드립니다!
커밋순으로 보면 좋을 것 같긴합니다. 커밋중에서 상단 3건은 1단계 코드리뷰에서 받은 부분을 수정하였고, 나머지가 2단계 구현입니다.
조금 헷갈리고 의문스러운 부분이 있는데요,, 제가 이해가 부족한 부분이 있는 것 같은데 잘못된 부분 피드백 부탁드립니다.!
이전 과제에서 /members도 로그인한 유저만 접근가능하다라는 전제조건이 있어서 조금 헷갈렸어요 ^_ㅠ