Skip to content

RoleHierarchy bean does not apply to AuthorityAuthorizationManager #13911

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
bwgjoseph opened this issue Sep 30, 2023 · 4 comments
Closed

RoleHierarchy bean does not apply to AuthorityAuthorizationManager #13911

bwgjoseph opened this issue Sep 30, 2023 · 4 comments
Assignees
Labels
status: invalid An issue that we don't feel is valid type: bug A general bug

Comments

@bwgjoseph
Copy link

Describe the bug

Declaring RoleHierarchy in a @Bean behave differently when declared manually to AuthorityAuthorizationManager.

I'm using spring-boot-3.1.4

To Reproduce

Please refer to my repo for full example.

I have the following security configuration

@EnableMethodSecurity
@EnableWebSecurity(debug = false)
@Configuration(proxyBeanMethods = false)
public class SecurityConfig {

    @Bean
    public SecurityFilterChain securityFilterChain(HttpSecurity http) throws Exception {
        return http
            .formLogin(AbstractHttpConfigurer::disable)
            .httpBasic(AbstractHttpConfigurer::disable)
            .anonymous(AbstractHttpConfigurer::disable)
            .csrf(AbstractHttpConfigurer::disable)
            .logout(AbstractHttpConfigurer::disable)
            .sessionManagement(session -> session.sessionCreationPolicy(SessionCreationPolicy.STATELESS))
            .authorizeHttpRequests(httpReq -> httpReq.anyRequest().access(haveReadPermission()))
            .build();
    }

    // see below for the configuration for RoleHierarchy and AuthorityAuthorizationManager
}

And the following test

@WebMvcTest(MeController.class)
@Import({ SecurityConfig.class})
class MeControllerTests {
    @Autowired
    private MockMvc mockMvc;

    @Test
    @WithMockUser(roles = { "ADMIN" }, authorities = { "ROLE_ADMIN" } )
    void test1() throws Exception {
        this.mockMvc
            .perform(MockMvcRequestBuilders
                .get("/me"))
            .andDo(MockMvcResultHandlers.print());
    }
}

Pass Scenario

Given the following configuration; manually passing in role hierarchy, the test will pass

public RoleHierarchy roleHierarchy() {
    RoleHierarchyImpl roleHierarchy = new RoleHierarchyImpl();

    String roleHierarchyFromMap = """
            ROLE_ADMIN > ROLE_STAFF
            ROLE_STAFF > ROLE_USER
            ROLE_STAFF > ROLE_GUEST
            """;

    roleHierarchy.setHierarchy(roleHierarchyFromMap);
    return roleHierarchy;
}

private AuthorizationManager<RequestAuthorizationContext> haveReadPermission() {
    AuthorityAuthorizationManager<RequestAuthorizationContext> authority = AuthorityAuthorizationManager.hasAnyAuthority("ROLE_USER");
    authority.setRoleHierarchy(this.roleHierarchy());
    return authority;
}
click to view test result
MockHttpServletRequest:
      HTTP Method = GET
      Request URI = /me
       Parameters = {}
          Headers = []
             Body = null
    Session Attrs = {}

Handler:
             Type = com.bwgjoseph.springsecurityrolehierarchybug.MeController
           Method = com.bwgjoseph.springsecurityrolehierarchybug.MeController#me(MyUserDetails)

Async:
    Async started = false
     Async result = null

Resolved Exception:
             Type = null

ModelAndView:
        View name = null
             View = null
            Model = null

FlashMap:
       Attributes = null

MockHttpServletResponse:
           Status = 200
    Error message = null
          Headers = [Content-Type:"application/json", X-Content-Type-Options:"nosniff", X-XSS-Protection:"0", Cache-Control:"no-cache, no-store, max-age=0, must-revalidate", Pragma:"no-cache", Expires:"0", X-Frame-Options:"DENY"]
     Content type = application/json
             Body = {"username":"rob","email":"[email protected]","roles":["ROLE_ADMIN"],"authorities":[{"authority":"ROLE_ADMIN"}],"enabled":true,"password":"N/A","credentialsNonExpired":true,"accountNonExpired":true,"accountNonLocked":true}
    Forwarded URL = null
   Redirected URL = null
          Cookies = []

Fail Scenario

With the following configuration; configure RoleHierarchy as @Bean and remove manually passing in role hierarchy. The test will fail.

@Bean // enable this
public RoleHierarchy roleHierarchy() {
    RoleHierarchyImpl roleHierarchy = new RoleHierarchyImpl();

    String roleHierarchyFromMap = """
            ROLE_ADMIN > ROLE_STAFF
            ROLE_STAFF > ROLE_USER
            ROLE_STAFF > ROLE_GUEST
            """;

    roleHierarchy.setHierarchy(roleHierarchyFromMap);
    return roleHierarchy;
}

private AuthorizationManager<RequestAuthorizationContext> haveReadPermission() {
    AuthorityAuthorizationManager<RequestAuthorizationContext> authority = AuthorityAuthorizationManager.hasAnyAuthority("ROLE_USER");
    // authority.setRoleHierarchy(this.roleHierarchy()); // remove this
    return authority;
}
click to view test result
MockHttpServletRequest:
      HTTP Method = GET
      Request URI = /me
       Parameters = {}
          Headers = []
             Body = null
    Session Attrs = {}

Handler:
             Type = null

Async:
    Async started = false
     Async result = null

Resolved Exception:
             Type = null

ModelAndView:
        View name = null
             View = null
            Model = null

FlashMap:
       Attributes = null

MockHttpServletResponse:
           Status = 403
    Error message = Forbidden
          Headers = [X-Content-Type-Options:"nosniff", X-XSS-Protection:"0", Cache-Control:"no-cache, no-store, max-age=0, must-revalidate", Pragma:"no-cache", Expires:"0", X-Frame-Options:"DENY"]
     Content type = null
             Body = 
    Forwarded URL = null
   Redirected URL = null
          Cookies = []

Expected behavior

I expect that AuthorityAuthorizationManager should have the same behavior whether RoleHierarchy is defined as a @Bean or manually through the setter.

This issue - #13188 - seem to suggest that this feature should be in working state after spring-boot 3.1.0

Possible related issue

Sample

Click the link to a GitHub repository with a minimal, reproducible sample.

@bwgjoseph bwgjoseph added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Sep 30, 2023
@marcusdacoregio marcusdacoregio self-assigned this Oct 4, 2023
@marcusdacoregio marcusdacoregio removed the status: waiting-for-triage An issue we've not yet triaged label Oct 4, 2023
@marcusdacoregio
Copy link
Contributor

Hi, @bwgjoseph, thanks for the report.

#13188 make it so when you use hasAuthority, hasRole, and its variants, inside authorizeHttpRequests, Spring Security should consider the RoleHierarchy bean. Since you are defining your own AuthorizationManager (not using authorizeHttpRequests) you must set the field using the provided setter.

You can make your RoleHierarchy a bean and inject it into the AuthorizationManager bean method definition if you want:

@Bean
public RoleHierarchy roleHierarchy() {
    RoleHierarchyImpl roleHierarchy = new RoleHierarchyImpl();

    String roleHierarchyFromMap = """
            ROLE_ADMIN > ROLE_STAFF
            ROLE_STAFF > ROLE_USER
            ROLE_STAFF > ROLE_GUEST
            """;

    roleHierarchy.setHierarchy(roleHierarchyFromMap);
    return roleHierarchy;
}

private AuthorizationManager<RequestAuthorizationContext> haveReadPermission(RoleHierarchy roleHierarchy) {
    AuthorityAuthorizationManager<RequestAuthorizationContext> authority = AuthorityAuthorizationManager.hasAnyAuthority("ROLE_USER");
    authority.setRoleHierarchy(roleHierarchy);
    return authority;
}

@marcusdacoregio marcusdacoregio added the status: invalid An issue that we don't feel is valid label Oct 4, 2023
@bwgjoseph
Copy link
Author

Hi @marcusdacoregio,

Thanks for the clarification!

In that case, then is it possible to consider having AuthorizationManager also infer directly from the bean than having to set it manually?

As I have a number AuthorizationManager methods (e.g haveReadPermission, haveWritePermission, ...), having to keep re-declaring it is a little bit tedious.

Thanks!

@marcusdacoregio
Copy link
Contributor

marcusdacoregio commented Oct 4, 2023

You can consider creating a factory to do that work for you, something like:

@Component
class AuthorizationManagerFactory {

    private final RoleHierarchy roleHierarchy;

    // constructor

    public AuthorizationManager<RequestAuthorizationContext> haveReadPermission() {
        AuthorityAuthorizationManager<RequestAuthorizationContext> authority = AuthorityAuthorizationManager.hasAnyAuthority("ROLE_USER");
        authority.setRoleHierarchy(this.roleHierarchy);
        return authority;
    }

    // ... other methods

}

Then, you can:

    @Bean
    public SecurityFilterChain securityFilterChain(HttpSecurity http, AuthorizationManagerFactory factory) throws Exception {
        return http
            // ...
            .authorizeHttpRequests(httpReq -> httpReq.anyRequest().access(factory.haveReadPermission()))
            .build();
    }

@bwgjoseph
Copy link
Author

@marcusdacoregio

Sorry, I meant the tedious is having to keep calling the setRoleHierarchy with each of the method. As I'm not familiar enough with this, I'm not sure if it's possible to have AuthorityAuthorizationManager inferred from the Bean like how authorizeHttpRequests is done.

public AuthorizationManager<RequestAuthorizationContext> haveReadPermission() {
	AuthorityAuthorizationManager<RequestAuthorizationContext> authority = AuthorityAuthorizationManager.hasAnyAuthority("ROLE_USER");
	// not required to set, inferred from the bean hierarchy
	// authority.setRoleHierarchy(this.roleHierarchy);
	return authority;
}

Although, it's not really a big issue to keep re-declaring per method. Just wanted to know if the experience can be better. So I can do something along this line

@Bean
public RoleHierarchy roleHierarchy() {
    RoleHierarchyImpl roleHierarchy = new RoleHierarchyImpl();

    // omitted
	
    return roleHierarchy;
}

public AuthorizationManager<RequestAuthorizationContext> haveReadPermission() {
	return AuthorityAuthorizationManager.hasAnyAuthority("ROLE_USER");
}

public AuthorizationManager<RequestAuthorizationContext> haveWritePermission() {
	return AuthorityAuthorizationManager.hasAnyAuthority("ROLE_MODERATOR");
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: invalid An issue that we don't feel is valid type: bug A general bug
Projects
None yet
Development

No branches or pull requests

2 participants