선택 - RoleHierarchy 리뷰 요청 드립니다. #16
Conversation
| // -- for test | ||
| mappings.add(new RequestMatcherEntry<>( | ||
| new MvcRequestMatcher(HttpMethod.GET, "/user-granted"), | ||
| new HasAuthorityAuthorizationManager<>(roleHierarchy(), "USER")) | ||
| ); | ||
|
|
||
| mappings.add(new RequestMatcherEntry<>( | ||
| new MvcRequestMatcher(HttpMethod.GET, "/admin-granted"), | ||
| new HasAuthorityAuthorizationManager<>(roleHierarchy(), "ADMIN")) | ||
| ); | ||
| // |
There was a problem hiding this comment.
테스트를 위한 코드가 프로덕션 코드에 추가되어 있네요! 테스트 환경에서만 영향을 받도록 구조를 개선해보면 어떨까요?
| private final RoleHierarchy roleHierarchy; | ||
|
|
||
| public AuthoritiesAuthorizationManager() { | ||
| this.roleHierarchy = new NullRoleHierarchy(); |
There was a problem hiding this comment.
| this.roleHierarchy = new NullRoleHierarchy(); | |
| this(new NullRoleHierarchy()); |
소소하지만.. 주 생성자를 정해 하나의 생성자에서만 초기화를 담당하면 유지보수성을 높일 수 있습니다!
| import java.util.List; | ||
| import java.util.Set; | ||
|
|
||
| public class RoleHierarchyTest { |
There was a problem hiding this comment.
테스트 너무 좋네요 👍
추가로 순환 참조가 발생했을 때와 같은 예외 상황에 대한 테스트도 추가해보면 어떨까요?
There was a problem hiding this comment.
생성시에 탐지하는 것이 좋을 것 같아 생성자쪽에 추가해보았습니다!
| processed.add(current); | ||
| result.add(current); | ||
|
|
||
| reachableAuthoritiesMap.getOrDefault(current.getAuthority(), Collections.emptySet()) |
There was a problem hiding this comment.
지금은 stream 기능을 잘 활용해서 구현해 주셨는데요! 스프링과 같은 프레임워크의 경우 가독성보다 성능을 우선하기 때문에 stream 보단 for문을 선호하고 있습니다.
기존 애플리케이션의 방향성인 가독성보다 성능을 우선으로 구현해보는 것도 하나의 재미가 될 수 있을 것 같습니다 :)
There was a problem hiding this comment.
오... 그렇군요 !! 생각해보니 스프링이나 시큐리티 코드를 보면서 stream을 사용한 부분이 거의 안보인다 싶긴 했는데 성능적인 부분을 챙겨가는 것이었군요. 이부분은 for loop으로 변경해서 적용해보겠습니다. !!
|
|
||
| Set<GrantedAuthority> result = new HashSet<>(); | ||
| Set<GrantedAuthority> processed = new HashSet<>(); | ||
| Queue<GrantedAuthority> queue = new LinkedList<>(authorities); |
There was a problem hiding this comment.
| Queue<GrantedAuthority> queue = new LinkedList<>(authorities); | |
| Queue<GrantedAuthority> queue = new ArrayDeque<>(authorities); |
BFS 알고리즘을 활용하여 잘 구현해 주셨네요 👍
알고리즘에 맞는 자료구조로 바꾼다면 성능이 더 올라갈 것 같습니다!
|
안녕하세요 재연님 !! 리뷰 주신것 확인하였습니다. |
| } | ||
|
|
||
| @Bean | ||
| @ConditionalOnMissingBean(RequestMatcherDelegatingAuthorizationManager.class) |
| if (visiting.contains(role)) { | ||
| throw new CycleInRoleHierarchyException(); | ||
| } |
| @Bean | ||
| public RequestMatcherDelegatingAuthorizationManager requestAuthorizationManager() { | ||
| List<RequestMatcherEntry<AuthorizationManager<HttpServletRequest>>> mappings = new ArrayList<>(); | ||
|
|
||
| mappings.add(new RequestMatcherEntry<>( | ||
| new MvcRequestMatcher(HttpMethod.GET, "/members/me"), | ||
| new AuthenticatedAuthorizationManager<>()) | ||
| ); | ||
| mappings.add(new RequestMatcherEntry<>( | ||
| new MvcRequestMatcher(HttpMethod.GET, "/members"), | ||
| new HasAuthorityAuthorizationManager<>(roleHierarchy(), "ADMIN")) | ||
| ); | ||
| mappings.add(new RequestMatcherEntry<>( | ||
| new MvcRequestMatcher(HttpMethod.GET, "/search"), | ||
| new PermitAllAuthorizationManager<>()) | ||
| ); | ||
|
|
||
| mappings.add(new RequestMatcherEntry<>( | ||
| new MvcRequestMatcher(HttpMethod.GET, "/user-granted"), | ||
| new HasAuthorityAuthorizationManager<>(roleHierarchy(), "USER")) | ||
| ); | ||
|
|
||
| mappings.add(new RequestMatcherEntry<>( | ||
| new MvcRequestMatcher(HttpMethod.GET, "/admin-granted"), | ||
| new HasAuthorityAuthorizationManager<>(roleHierarchy(), "ADMIN")) | ||
| ); | ||
|
|
||
| mappings.add(new RequestMatcherEntry<>( | ||
| new AnyRequestMatcher(), | ||
| new DenyAllAuthorizationManager<>()) | ||
| ); | ||
|
|
||
| return new RequestMatcherDelegatingAuthorizationManager(mappings); | ||
| } |
There was a problem hiding this comment.
현재 프로덕션 코드와 테스트 코드에서 동일한 설정이 중복되고 있는데, Spring에서 활용하는 Configurer 패턴(설정자 패턴)을 활용하면 확장성이 늘어나 중복을 없애볼 수 있겠네요~
시간이 남고 심심하실까봐 학습하면 좋을 내용 하나 던지고 갑니다 ㅋㅋ
| processed.add(current); | ||
| result.add(current); | ||
|
|
||
| Set<GrantedAuthority> next = reachableAuthoritiesMap.getOrDefault(current.getAuthority(), Collections.emptySet()); |
There was a problem hiding this comment.
반복문으로 깔끔하게 변경해 주셨네요 👍
미션의 다른 부분에는 아직 스트림 코드가 남아 있는데, 이번 변경이 해당 부분만 수정하는 것이 맞을까요?
다른 코드에 코멘트를 드리려 했는데, 혹시 의도하신 것인지 확인을 위해 리뷰 남깁니다.
There was a problem hiding this comment.
아 제가 다른곳에서도 사용중이었네요 🥲
이부분은 확인을 못했습니다 ㅎㅎㅎ
반영했습니다. 꼼꼼한 리뷰 감사합니다 !
안녕하세요 재연님! 😁
Role Hierarchy를 추가해보았습니다. 리뷰 잘 부탁드립니다 !