[김병희] 선택 - RoleHierarchy 리뷰 요청 드립니다.

README.md

@@ -33,5 +33,20 @@ RequestMatcherDelegatingAuthorizationManager객체의 mappings 정보는 Authori
 - /members는 "ADMIN" 사용자만에게만 권한을 부여하기 위해 HasAuthorityAuthorizationManager로 처리  
 
 
+## 추가 RoleHierarchy
+
+RoleHierarchy 리팩터링
+RoleHierarchy는 기본적으로 NullRoleHierarchy가 설정된 다음 계층 구조의 권한 설정이 생길 경우 RoleHierarchyImpl가 동작하도록 되어있다. 실제 시큐리티의 구조를 참고해서 아래와 같이 설정하도록 수정한다.
+
+```java
+@Bean
+public RoleHierarchy roleHierarchy() {
+    return RoleHierarchyImpl.with()
+            .role("ADMIN").implies("USER")
+            .build();
+}
+```
+
+
 
 

src/main/java/nextstep/app/SecurityConfig.java

@@ -50,9 +50,18 @@ public SecuredMethodInterceptor securedMethodInterceptor() {
         return new SecuredMethodInterceptor();
     }
 
+//    @Bean
+//    public SecuredAspect securedAspect() {
+//        return new SecuredAspect();
+//    }
+
+
     @Bean
-    public SecuredAspect securedAspect() {
-        return new SecuredAspect();
+    public RoleHierarchy roleHierarchy() {
+       return RoleHierarchyImpl.with()
+               .role("ADMIN")
+               .implies("USER","GUEST")
+               .build();
     }
 
     @Bean
@@ -62,6 +71,7 @@ public RequestMatcherDelegatingAuthorizationManager requestAuthorizationManager(
         mappings.add(new RequestMatcherEntry<>(new MvcRequestMatcher(HttpMethod.GET, "/members"), new HasAuthorityAuthorizationManager("ADMIN")));
         mappings.add(new RequestMatcherEntry<>(new MvcRequestMatcher(HttpMethod.GET, "/search"), new PermitAllAuthorizationManager()));
         mappings.add(new RequestMatcherEntry<>(new MvcRequestMatcher(HttpMethod.POST, "/private"), new DenyAllAuthorizationManager()));
+        mappings.add(new RequestMatcherEntry<>(new MvcRequestMatcher(HttpMethod.GET, "/hierarchy"), new HasAuthorityAuthorizationManager("USER").withRoleHierarchy(roleHierarchy())));
         mappings.add(new RequestMatcherEntry<>(new AnyRequestMatcher(), new PermitAllAuthorizationManager()));
         return new RequestMatcherDelegatingAuthorizationManager(mappings);
     }

src/main/java/nextstep/app/ui/MemberController.java

@@ -32,6 +32,13 @@ public ResponseEntity<List<Member>> search() {
         return ResponseEntity.ok(members);
     }
 
+
+    @GetMapping("/hierarchy")
+    public ResponseEntity<Void> hierarchy() {
+        return ResponseEntity.ok().build();
+    }
+
+
     @GetMapping("/members/me")
     public Member findMe() {
         final String email = (String) SecurityContextHolder.getContext().getAuthentication().getPrincipal();

src/main/java/nextstep/security/authorization/CheckAuthenticationFilter.java

@@ -2,33 +2,45 @@
 
 import jakarta.servlet.FilterChain;
 import jakarta.servlet.ServletException;
+import jakarta.servlet.ServletRequest;
+import jakarta.servlet.ServletResponse;
 import jakarta.servlet.http.HttpServletRequest;
 import jakarta.servlet.http.HttpServletResponse;
 import nextstep.security.authentication.Authentication;
 import nextstep.security.context.SecurityContextHolder;
-import org.springframework.web.filter.OncePerRequestFilter;
+import org.springframework.web.filter.GenericFilterBean;
 
 import java.io.IOException;
 
-public class CheckAuthenticationFilter extends OncePerRequestFilter {
+public class CheckAuthenticationFilter extends GenericFilterBean {
 
-    private final RequestMatcherDelegatingAuthorizationManager requestMatcherDelegatingAuthorizationManager;
+    private final AuthorizationManager<HttpServletRequest> authorizationManager;
 
-    public CheckAuthenticationFilter(RequestMatcherDelegatingAuthorizationManager requestMatcherDelegatingAuthorizationManager) {
-        this.requestMatcherDelegatingAuthorizationManager = requestMatcherDelegatingAuthorizationManager;
+    public CheckAuthenticationFilter(AuthorizationManager<HttpServletRequest> authorizationManager) {
+        this.authorizationManager = authorizationManager;
     }
 
     @Override
-    protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) throws ServletException, IOException {
+    public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException {
         Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
 
-        AuthorizationDecision authorizationDecision = requestMatcherDelegatingAuthorizationManager.check(authentication, request);
-
-        if (authorizationDecision.isDeny()) {
-            response.setStatus(HttpServletResponse.SC_FORBIDDEN);
+        try {
+            AuthorizationDecision authorizationDecision = authorizationManager.check(authentication, (HttpServletRequest) request);
+            authorizationCheck(authorizationDecision);
+        } catch (ForbiddenException e) {
+            ((HttpServletResponse) response).setStatus(HttpServletResponse.SC_FORBIDDEN);
             return;
         }
 
-        filterChain.doFilter(request, response);
+        chain.doFilter(request, response);
     }
+
+
+    private static void authorizationCheck(AuthorizationDecision authorizationDecision) {
+        if (authorizationDecision.isDeny()) {
+            throw new ForbiddenException();
+        }
+    }
+
+
 }

src/main/java/nextstep/security/authorization/HasAuthorityAuthorizationManager.java

@@ -2,8 +2,11 @@
 
 import nextstep.security.authentication.Authentication;
 
+import java.util.Collection;
+
 public class HasAuthorityAuthorizationManager implements AuthorizationManager {
     private final String allowRole;
+    private RoleHierarchy roleHierarchy = new NullRoleHierarchy();
 
     public HasAuthorityAuthorizationManager(String allowRole) {
         this.allowRole = allowRole;
@@ -15,10 +18,21 @@ public AuthorizationDecision check(Authentication authentication, Object object)
             return AuthorizationDecision.deny();
         }
 
-        if (authentication.getAuthorities().contains(allowRole)) {
+        if (isHasAccess(authentication)) {
             return AuthorizationDecision.granted();
         }
 
         return AuthorizationDecision.deny();
     }
+
+    private boolean isHasAccess(Authentication authentication) {
+        Collection<String> reachableGrantedAuthorities = roleHierarchy.getReachableGrantedAuthorities(authentication.getAuthorities());
+
+        return reachableGrantedAuthorities.contains(allowRole);
+    }
+
+    public HasAuthorityAuthorizationManager withRoleHierarchy(RoleHierarchy roleHierarchy) {
+        this.roleHierarchy = roleHierarchy;
+        return this;
+    }
 }

src/main/java/nextstep/security/authorization/NullRoleHierarchy.java

@@ -0,0 +1,11 @@
+package nextstep.security.authorization;
+
+import java.util.Collection;
+
+public class NullRoleHierarchy implements RoleHierarchy {
+
+    @Override
+    public Collection<String> getReachableGrantedAuthorities(Collection<String> authorities) {
+        return authorities;
+    }
+}

src/main/java/nextstep/security/authorization/RoleHierarchy.java

@@ -0,0 +1,8 @@
+package nextstep.security.authorization;
+
+import java.util.Collection;
+
+public interface RoleHierarchy {
+    Collection<String> getReachableGrantedAuthorities(
+            Collection<String> authorities);
+}

src/main/java/nextstep/security/authorization/RoleHierarchyImpl.java

@@ -0,0 +1,57 @@
+package nextstep.security.authorization;
+
+import java.util.*;
+
+public class RoleHierarchyImpl implements RoleHierarchy {
+    private final Map<String, Set<String>> hierarchyPath;
+
+    private RoleHierarchyImpl(Map<String, Set<String>> hierarchyPath) {
+        this.hierarchyPath = hierarchyPath;
+    }
+
+    @Override
+    public Collection<String> getReachableGrantedAuthorities(Collection<String> authorities) {
+        Set<String> grantedAuthorities = new HashSet<>();
+
+        for (String authority : authorities) {
+            if (hierarchyPath.get(authority) != null) {
+                grantedAuthorities.addAll(hierarchyPath.get(authority));
+            }
+        }
+
+        return grantedAuthorities;
+    }
+
+    public static RoleBuilder with() {
+        return new RoleBuilder();
+    }
+
+    public static class RoleBuilder {
+        private static final Map<String, Set<String>> hierarchy = new HashMap<>();
+        private String role;
+
+        public RoleBuilder role(String role) {
+            this.role = role;
+            hierarchy.put(role, new HashSet<>(Set.of(role)));
+            return this;
+        }
+
+        public ImpliesBuilder implies(String... implies) {
+            hierarchy.get(role).addAll(Set.of(implies));
+
+            for (int i = 0; i < implies.length; i++) {
+                Set<String> paths = hierarchy.getOrDefault(implies[i], new HashSet<>());
+                paths.addAll(List.of(implies).subList(i, implies.length));
+                hierarchy.put(implies[i], paths);
+            }
+
+            return new ImpliesBuilder();
+        }
+
+        public static class ImpliesBuilder {
+            public RoleHierarchyImpl build() {
+                return new RoleHierarchyImpl(hierarchy);
+            }
+        }
+    }
+}

src/test/java/nextstep/app/FormLoginTest.java

@@ -125,14 +125,7 @@ void user_login_after_members() throws Exception {
     @Test
     @DisplayName("인증된 사용자는 자신의 정보를 조회할 수 있다.")
     void request_success_members_me() throws Exception {
-        MockHttpSession session = new MockHttpSession();
-
-        mockMvc.perform(post("/login")
-                .param("username", TEST_USER_MEMBER.getEmail())
-                .param("password", TEST_USER_MEMBER.getPassword())
-                .session(session)
-                .contentType(MediaType.APPLICATION_FORM_URLENCODED_VALUE)
-        ).andExpect(status().isOk());
+        MockHttpSession session = login(TEST_USER_MEMBER);
 
         mockMvc.perform(get("/members/me")
                         .session(session)
@@ -157,14 +150,7 @@ void request_fail_members_me() throws Exception {
     @Test
     @DisplayName("ADMIN 권한을 가진 사용자가 요청할 경우 모든 회원 정보를 조회할 수 있다.")
     void request_search_success_with_admin_user() throws Exception {
-        MockHttpSession session = new MockHttpSession();
-
-        mockMvc.perform(post("/login")
-                .param("username", TEST_ADMIN_MEMBER.getEmail())
-                .param("password", TEST_ADMIN_MEMBER.getPassword())
-                .session(session)
-                .contentType(MediaType.APPLICATION_FORM_URLENCODED_VALUE)
-        ).andExpect(status().isOk());
+        MockHttpSession session = login(TEST_ADMIN_MEMBER);
 
         mockMvc.perform(get("/members")
                         .session(session)
@@ -177,14 +163,7 @@ void request_search_success_with_admin_user() throws Exception {
     @Test
     @DisplayName("제한된 URI은 ADMIN 이여도 접근할수 없다")
     void request_private_with_admin_user() throws Exception {
-        MockHttpSession session = new MockHttpSession();
-
-        mockMvc.perform(post("/login")
-                .param("username", TEST_ADMIN_MEMBER.getEmail())
-                .param("password", TEST_ADMIN_MEMBER.getPassword())
-                .session(session)
-                .contentType(MediaType.APPLICATION_FORM_URLENCODED_VALUE)
-        ).andExpect(status().isOk());
+        MockHttpSession session = login(TEST_ADMIN_MEMBER);
 
         mockMvc.perform(post("/private")
                         .session(session)
@@ -196,19 +175,45 @@ void request_private_with_admin_user() throws Exception {
     @Test
     @DisplayName("제한된 URI은 접근할수 없다")
     void request_private_with_user_user() throws Exception {
-        MockHttpSession session = new MockHttpSession();
-
-        mockMvc.perform(post("/login")
-                .param("username", TEST_USER_MEMBER.getEmail())
-                .param("password", TEST_USER_MEMBER.getPassword())
-                .session(session)
-                .contentType(MediaType.APPLICATION_FORM_URLENCODED_VALUE)
-        ).andExpect(status().isOk());
+        MockHttpSession session = login(TEST_USER_MEMBER);
 
         mockMvc.perform(post("/private")
                         .session(session)
                         .contentType(MediaType.APPLICATION_FORM_URLENCODED_VALUE)
                 )
                 .andExpect(status().isForbidden());
     }
+
+
+    @Test
+    @DisplayName("USER 권한 이상을 가진 사용자는 hierarchy url에 접근이 가능하다.")
+    void request_success_hierarchy() throws Exception {
+        MockHttpSession adminSession = login(TEST_ADMIN_MEMBER);
+        MockHttpSession userSession = login(TEST_USER_MEMBER);
+
+        mockMvc.perform(get("/hierarchy")
+                        .session(adminSession)
+                        .contentType(MediaType.APPLICATION_FORM_URLENCODED_VALUE)
+                )
+                .andExpect(status().isOk());
+
+
+        mockMvc.perform(get("/hierarchy")
+                        .session(userSession)
+                        .contentType(MediaType.APPLICATION_FORM_URLENCODED_VALUE)
+                )
+                .andExpect(status().isOk());
+    }
+
+    private MockHttpSession login(Member member) throws Exception {
+        MockHttpSession session = new MockHttpSession();
+
+        mockMvc.perform(post("/login")
+                .param("username", member.getEmail())
+                .param("password", member.getPassword())
+                .session(session)
+                .contentType(MediaType.APPLICATION_FORM_URLENCODED_VALUE)
+        ).andExpect(status().isOk());
+        return session;
+    }
 }

src/test/java/nextstep/security/authorization/HasAuthorityAuthorizationManagerTest.java

@@ -53,4 +53,50 @@ void admin_authenticate() {
 
         assertThat(check.isDeny()).isFalse();
     }
+
+    @DisplayName("roleHierarchy 를 설정하면 상위 권한에서 인가를 허용한다")
+    @Test
+    void roleHierarchy_authenticate() {
+        // given
+        final String allowedRole = "ADMIN";
+        RoleHierarchyImpl roleHierarchy = RoleHierarchyImpl.with()
+                .role("ADMIN")
+                .implies("USER")
+                .build();
+
+        HasAuthorityAuthorizationManager authenticatedAuthorizationManager = new HasAuthorityAuthorizationManager(allowedRole);
+        authenticatedAuthorizationManager.withRoleHierarchy(roleHierarchy);
+
+        Authentication authentication = TestAuthentication.admin();
+
+
+        // when
+        AuthorizationDecision check = authenticatedAuthorizationManager.check(authentication, new MockHttpServletRequest());
+
+        // then
+        assertThat(check.isDeny()).isFalse();
+    }
+
+    @DisplayName("roleHierarchy 를 설정하면 같은 권한에 인가를 허용한다")
+    @Test
+    void roleHierarchy_equal_authenticate() {
+        // given
+        final String allowedRole = "USER";
+        RoleHierarchyImpl roleHierarchy = RoleHierarchyImpl.with()
+                .role("ADMIN")
+                .implies("USER")
+                .build();
+
+        HasAuthorityAuthorizationManager authenticatedAuthorizationManager = new HasAuthorityAuthorizationManager(allowedRole);
+        authenticatedAuthorizationManager.withRoleHierarchy(roleHierarchy);
+
+        Authentication authentication = TestAuthentication.user();
+
+
+        // when
+        AuthorizationDecision check = authenticatedAuthorizationManager.check(authentication, new MockHttpServletRequest());
+
+        // then
+        assertThat(check.isDeny()).isFalse();
+    }
 }