Skip to content

RoleHierarchy is not used by AbstractAuthorizeTag #7059

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
larsgrefer opened this issue Jun 29, 2019 · 6 comments · Fixed by #8652
Closed

RoleHierarchy is not used by AbstractAuthorizeTag #7059

larsgrefer opened this issue Jun 29, 2019 · 6 comments · Fixed by #8652
Assignees
Labels
in: config An issue in spring-security-config status: backported An issue that has been backported to maintenance branches status: ideal-for-contribution An issue that we actively are looking for someone to help us with type: bug A general bug
Milestone

Comments

@larsgrefer
Copy link
Contributor

Summary

I've noticed a strange behaviour when setting up a RoleHierarchy in a simple Spring Boot application, when trying to use it with org.springframework.security.taglibs.authz.AbstractAuthorizeTag

Actual Behavior

Effektively two DefaultWebSecurityExpressionHandler get created:

  • private DefaultWebSecurityExpressionHandler defaultWebSecurityExpressionHandler = new DefaultWebSecurityExpressionHandler();
    private SecurityExpressionHandler<FilterInvocation> expressionHandler = defaultWebSecurityExpressionHandler;
  • DefaultWebSecurityExpressionHandler defaultHandler = new DefaultWebSecurityExpressionHandler();
    AuthenticationTrustResolver trustResolver = http
    .getSharedObject(AuthenticationTrustResolver.class);
    if (trustResolver != null) {
    defaultHandler.setTrustResolver(trustResolver);
    }
    ApplicationContext context = http.getSharedObject(ApplicationContext.class);
    if (context != null) {
    String[] roleHiearchyBeanNames = context.getBeanNamesForType(RoleHierarchy.class);
    if (roleHiearchyBeanNames.length == 1) {
    defaultHandler.setRoleHierarchy(context.getBean(roleHiearchyBeanNames[0], RoleHierarchy.class));
    }
    String[] grantedAuthorityDefaultsBeanNames = context.getBeanNamesForType(GrantedAuthorityDefaults.class);
    if (grantedAuthorityDefaultsBeanNames.length == 1) {
    GrantedAuthorityDefaults grantedAuthorityDefaults = context.getBean(grantedAuthorityDefaultsBeanNames[0], GrantedAuthorityDefaults.class);
    defaultHandler.setDefaultRolePrefix(grantedAuthorityDefaults.getRolePrefix());
    }
    String[] permissionEvaluatorBeanNames = context.getBeanNamesForType(PermissionEvaluator.class);
    if (permissionEvaluatorBeanNames.length == 1) {
    PermissionEvaluator permissionEvaluator = context.getBean(permissionEvaluatorBeanNames[0], PermissionEvaluator.class);
    defaultHandler.setPermissionEvaluator(permissionEvaluator);
    }
    }

The second one picks up my RoleHierarchy bean, but the first doesnt.
org.springframework.security.taglibs.authz.AbstractAuthorizeTag#getExpressionHandler resolves the first handler, therefore the RoleHierarchy is ignored.

Expected Behavior

I'd expect AbstractAuthorizeTag to use my RoleHierarchy when resolving hasRole() expressions.

Configuration

@Configuration
public class SecurityConfiguration extends WebSecurityConfigurerAdapter {

	@Override
	protected void configure(AuthenticationManagerBuilder auth) throws Exception {
		auth.inMemoryAuthentication()
			.withUser("admin").password("{noop}admin").roles("admin").and()
			.withUser("user").password("{noop}user").roles("user");
	}

	@Bean
	public RoleHierarchy roleHierarchy() {
		RoleHierarchyImpl roleHierarchy = new RoleHierarchyImpl();
		roleHierarchy.setHierarchy("ROLE_admin > ROLE_user");
		return roleHierarchy;
	}
}

Version

Spring Security 5.2.0.M3

Sample

see #2997
see #4115
see 8a66d0c#diff-23827daef0917bb5218098c8108b9125

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 29, 2019
@larsgrefer
Copy link
Contributor Author

After doing some more research, I think this issue is related to #5272

@rwinch
Copy link
Member

rwinch commented Jul 3, 2019

Thanks for the report @larsgrefer! Would you be willing to submit a PR for this?

@rwinch rwinch added in: config An issue in spring-security-config type: bug A general bug status: ideal-for-contribution An issue that we actively are looking for someone to help us with and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 3, 2019
@larsgrefer
Copy link
Contributor Author

I'm not sure how exactly this PR should look like.

Ideally there would be only one DefaultWebSecurityExpressionHandler instance which is used everywhere by default (instead of multiple instances whose default configuration needs to be kept in sync), but where should this instance be created and where should it be configured?

@rwinch
Copy link
Member

rwinch commented Jul 10, 2019

@larsgrefer I think we should start by fixing the immediate problem and updating the configuration to be kept in sync. This will be faster and less risky (which is desirable for a bug fix). We can explore using the same instance in a separate PR.

@evgeniycheban
Copy link
Contributor

@rwinch I can take this task.

@rwinch
Copy link
Member

rwinch commented Jun 1, 2020

Thank you @evgeniycheban The issue is yours

evgeniycheban added a commit to evgeniycheban/spring-security that referenced this issue Jun 4, 2020
@eleftherias eleftherias added this to the 5.4.0-M2 milestone Jun 10, 2020
@spring-projects-issues spring-projects-issues added the status: backported An issue that has been backported to maintenance branches label Jun 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: config An issue in spring-security-config status: backported An issue that has been backported to maintenance branches status: ideal-for-contribution An issue that we actively are looking for someone to help us with type: bug A general bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants