DefaultMethodSecurityExpressionHandler createSecurityExpressionRoot Should Have Protected Access Instead Of Private

[adase11]

Describe the bug
DefaultMethodSecurityExpressionHandler for v5.8.0 adds a new signature for createSecurityExpressionRoot as createSecurityExpressionRoot(Supplier<Authentication> authentication, MethodInvocation invocation) in addition to the existing
createSecurityExpressionRoot(Authentication authentication, MethodInvocation invocation) . However, the new signature is private while the existing one is protected. This causes an issue for any usage that extends the DefaultMethodSecurityExpressionHandler and overrides the protected createSecurityExpressionRoot because the createEvaluationContext method always calls the private createSecurityExpressionRoot, leaving any extension of DefaultMethodSecurityExpressionHandler unable to override this behavior. A work around could be to also override createEvaluationContext however that method uses MethodSecurityEvaluationContext which is package private and therefore cannot be used when overriding createEvaluationContext.

Proposed Fix
Make MethodSecurityExpressionOperations createSecurityExpressionRoot(Supplier<Authentication> authentication, MethodInvocation invocation) protected instead of private

Sample
See - DefaultMethodSecurityExpressionHandler for the code in question

[jzheaux]

@adase11, thanks for the report.

These days, Spring Security tends to prefer encouraging composition, though changing a method to be protected is not out of the question. Can you please describe in more detail what you are trying to do and the result of trying the following to approaches:

1. Instead of extending DefaultMethodSecurityExpressionHandler, implement MethodSecurityExpressionHandler and call DefaultMethodSecurityExpressionHandler as a member variable.
2. Instead of customizing the expression handler, change your authorization expressions to refer to a bean that contains any specialized logic, for example @PreAuthorize("@myBean.authz(#root)")

[adase11]

Thanks @jzheaux - I'm migrating to Spring Security v5.8 from a Spring Boot 2.7.6 project in preparation for Spring Boot 3.0 & Spring Security 6. In the current app I overrode the DefaultMethodSecurityExpressionHandler:createSecurityExpressionRoot method in order to use a custom SecurityExpressionRoot without having to rewrite most of the DefaultMethodSecurityExpressionHandler logic. Like so:

public class CustomMethodSecurityExpressionHandler extends DefaultMethodSecurityExpressionHandler {
	@Override
	protected MethodSecurityExpressionOperations createSecurityExpressionRoot(Authentication authentication, MethodInvocation invocation) {
		final CustomMethodSecurityExpressionRoot adminMethodSecurityExpressionRoot = new CustomMethodSecurityExpressionRoot(authentication,
				invocation);
		adminMethodSecurityExpressionRoot.setPermissionEvaluator(getPermissionEvaluator());
		adminMethodSecurityExpressionRoot.setTrustResolver(new AuthenticationTrustResolverImpl());
		adminMethodSecurityExpressionRoot.setRoleHierarchy(getRoleHierarchy());
		return adminMethodSecurityExpressionRoot;
	}
}

Now, because createEvaluationContext calls directly the new private overloaded version of createSecurityExpressionRoot that's been added this ^ will no longer work.

Your first suggestion, implementing MethodSecurityExpressionHandler and calling DefaultMethodSecurityExpressionHandler as a member variable, is the direction I was working with in the meantime. However, I thought it could be desirable treat both implementations with the same level of visibility especially now because createEvaluationContext calls the private method directly rather than the protected one and as a result, the strategy I mentioned above will no longer work to register a custom SecurityExpressionRoot and will show up as a runtime rather than compile time error.

[adase11]

Just to clarify I think that both of your suggestions would work. I appreciate the recommendations!

[paveljandejsek]

@adase11 Can you please share how did your solution with DefaultMethodSecurityExpressionHandler member variable works? I am facing the same problem, but cannot seem to find a working solution yet.

[adase11]

@paveljandejsek - I believe that the 2 options that @jzheaux mentioned are the best way to handle this. I chose to go with changing the authorization expressions to refer to bean that contains specialized logic which worked well for my use case.

If you require extending DefaultMethodSecurityExpressionHandler for some other reason though you could go with implementing MethodSecurityExpressionHandler and call DefaultMethodSecurityExpressionHandler as a member variable.

Or you could override both createEvaluationContext & createSecurityExpressionRoot and implement your own version of MethodSecurityEvaluationContext (extending MethodBasedEvaluationContext) with similar logic to the package private MethodSecurityEvaluationContext to like so:

@Override
public EvaluationContext createEvaluationContext(Supplier<Authentication> authentication, MethodInvocation invocation) {
	final MethodSecurityExpressionOperations root = createSecurityExpressionRootOverride(authentication);
	final MethodSecurityEvaluationContext ctx = new MethodSecurityEvaluationContext(root, invocation, getParameterNameDiscoverer());
	ctx.setBeanResolver(getBeanResolver());
	return ctx;
}

@Override
protected MethodSecurityExpressionOperations createSecurityExpressionRoot(Authentication authentication, MethodInvocation invocation) {
	return createSecurityExpressionRootOverride(() -> authentication);
}

private MethodSecurityExpressionOperations createSecurityExpressionRootOverride(Supplier<Authentication> authentication) {
	final CustomMethodSecurityExpressionRoot methodSecurityExpressionRoot = new MethodSecurityExpressionRoot(authentication);
	methodSecurityExpressionRoot.setPermissionEvaluator(getPermissionEvaluator());
	methodSecurityExpressionRoot.setTrustResolver(authenticationTrustResolver);
	methodSecurityExpressionRoot.setRoleHierarchy(getRoleHierarchy());
	return methodSecurityExpressionRoot;
}

In my opinion - these three options are listed from most to least desirable (where changing the authorization expressions to refer to bean that contains specialized logic is the most desirable). Hope that helps!

---

To extrapolate a bit further on my original proposal - This last solution would be cleaned up significantly if both signatures for createSecurityExpressionRoot were protected (because the currently protected one just calls the private one). If that was done then the code that I had posted above that originally overrode createSecurityExpressionRoot could be changed to override the new signature for the method - like so (By changing Authentication authentication to Supplier<Authentication> authentication:

public class CustomMethodSecurityExpressionHandler extends DefaultMethodSecurityExpressionHandler {
	@Override
	protected MethodSecurityExpressionOperations createSecurityExpressionRoot(Supplier<Authentication>  authentication, MethodInvocation invocation) {
		final CustomMethodSecurityExpressionRoot adminMethodSecurityExpressionRoot = new CustomMethodSecurityExpressionRoot(authentication,
				invocation);
		adminMethodSecurityExpressionRoot.setPermissionEvaluator(getPermissionEvaluator());
		adminMethodSecurityExpressionRoot.setTrustResolver(new AuthenticationTrustResolverImpl());
		adminMethodSecurityExpressionRoot.setRoleHierarchy(getRoleHierarchy());
		return adminMethodSecurityExpressionRoot;
	}
}

[adase11]

The member variable solution I was working on would look something like:

public class CustomMethodSecurityExpressionHandler extends AbstractSecurityExpressionHandler<MethodInvocation>
		implements MethodSecurityExpressionHandler {
	private final DefaultMethodSecurityExpressionHandler delegate = new DefaultMethodSecurityExpressionHandler();

	@Override
	public Object filter(Object filterTarget, Expression filterExpression, EvaluationContext ctx) {
		return delegate.filter(filterTarget, filterExpression, ctx);
	}
	
	...
}

I had not yet tested to ensure feature parity

[paveljandejsek]

@adase11 Thank you very much for elaborating!

I still hope that private->protected change might happen, since right now it seems it would be the simplest option for migration, but thanks for those suggestions!

I might try looking into calling the bean itself as well (altough I want to access some info from the MethodInvocation, but maybe I could live without it).

The member variable solution on the other hand seems to lead me to some casting/type checking; while the extension of the createEvaluationContext(Supplier<Authentication> authentication, MethodInvocation invocation) on the other hand then leads to package protected class MethodSecurityEvaluationContext and while both are definitely doable they also seem to me like a bit "ugly" solutions so I hope to avoid them 🙂

Anyway thanks again!

[adase11]

No problem @paveljandejsek, I agree.

[jzheaux]

Another option I believe is to extend the createEvaluationContext method itself, like so:

@Component
public class MyExpressionHandler extends DefaultMethodSecurityExpressionHandler {
    @Override 
    public EvaluationContext createEvaluationContext(Supplier<Authentication> authentication, MethodInvocation mi) {
        CustomMethodSecurityExpressionRoot adminMethodSecurityExpressionRoot = 
                new CustomMethodSecurityExpressionRoot(authentication,	invocation);
	adminMethodSecurityExpressionRoot.setPermissionEvaluator(getPermissionEvaluator());
	adminMethodSecurityExpressionRoot.setTrustResolver(new AuthenticationTrustResolverImpl());
	adminMethodSecurityExpressionRoot.setRoleHierarchy(getRoleHierarchy());
        StandardEvaluationContext context = (StandardEvaluationContext) super.createEvaluationContext(authentication, mi);
        context.setRootObject(adminMethodSecurityExpressionRoot);
        return context;
    }
}

As @adase11 has already found, using a custom bean in the annotation expression is often preferrable.

[jzheaux]

I've added #12356 to provide more detail on the migration steps for DefaultMethodSecurityExpressionHandler usage.

Given that, I'll close this ticket. @paveljandejsek, please try the migration guide once that ticket is complete. If the migration steps don't work for you, we can come back here and revisit.

[xak2000]

@jzheaux I agree that usage of custom bean is a more simple solution, but also it makes security expressions less concise.

Compare
@PreAuthorize("hasUserId(#userId) && hasRole('USER')")
vs
@PreAuthorize("@authz.hasUserId(#userId) && hasRole('USER')").

And, if there are more custom methods needs to be used in expressions (combined with &&, || etc), the verbosity will only grow.

The workaround with overriding createEvaluationContext and calling super works, but doesn't look decent.

It will create one root object using DefaultMethodSecurityExpressionHandler.createSecurityExpressionRoot() just to throw it away right after creation by calling setRootObject(..) method.

But this is OK. What is worse that we still have protected createSecurityExpressionRoot(Authentication, MethodInvocation) method in addition to new private createSecurityExpressionRoot(Supplier<Authentication>, MethodInvocation). The old protected method is still called from createEvaluationContext(Authentication, T), that is called from bunch of other places. So, it's unclear how to properly extend DefaultMethodSecurityExpressionHandler to make sure SecurityExpressionRoot will always be correct.

I ended up with this implementation:

public class CustomMethodSecurityExpressionHandler extends DefaultMethodSecurityExpressionHandler {

	private final CustomService customService;

	public CustomMethodSecurityExpressionHandler(CustomService customService) {
		this.customService = customService;
	}

	@Override
	public EvaluationContext createEvaluationContext(Supplier<Authentication> authentication, MethodInvocation mi) {
		StandardEvaluationContext evaluationContext =
				(StandardEvaluationContext) super.createEvaluationContext(authentication, mi);
		evaluationContext.setRootObject(createSecurityExpressionRoot(authentication, mi));
		return evaluationContext;
	}

	@Override
	protected MethodSecurityExpressionOperations createSecurityExpressionRoot(Authentication authentication,
			MethodInvocation mi) {
		return createSecurityExpressionRoot(() -> authentication, mi);
	}

	private MethodSecurityExpressionOperations createSecurityExpressionRoot(Supplier<Authentication> authentication,
			MethodInvocation mi) {

		CustomMethodSecurityExpressionRoot root =
				new CustomMethodSecurityExpressionRoot(authentication, customService);

		root.setThis(mi.getThis());
		root.setPermissionEvaluator(getPermissionEvaluator());
		root.setTrustResolver(getTrustResolver());
		root.setRoleHierarchy(getRoleHierarchy());
		root.setDefaultRolePrefix(getDefaultRolePrefix());

		return root;
	}

}

As you can see, I forced to override both new createEvaluationContext and old createSecurityExpressionRoot because this old method is still called from some parts of the framework (I'm not sure if it is relevant or not, but still). Moreover, my implementation of the old createSecurityExpressionRoot is exactly copy-paste of the implementation from DefaultMethodSecurityExpressionHandler - it just delegates to new version of the method. The only reason why I need to do this is because the new version of the method is private, so the old version will always call its own private method. The interface of DefaultMethodSecurityExpressionHandler is really confusing at this point. It's really unclear how to properly extend it. Maybe it's because it was never intended to be extended. :)

But what is the canonical way to create your own authorization expression DSL then?

Probably, the better way will be to make new createSecurityExpressionRoot method protected and add javadocs to both old and new versions of this method describing that you need to override only the new version of this method (the old version already delegates to the new in the implementation and if new will be protected, then will delegate to an overriden implementation) to make it work, while overriding old version alone will not work. Of course this is not the solution of the real problem (it's too hard to extend authorization DSL), but at least it will be much clearer to extend it this way instead of how I shown in my example.

Another option will be to remove the old version of createSecurityExpressionRoot method (the version without Supplier). Then, new version (if will be promoted to be protected) will be the source of truth, or at least it will be clear that the only point of extension is createEvaluationContext method (if the new version of createSecurityExpressionRoot method will be kept private). Maybe it's even the preferred way because this change is already breaking, but it is rogue-breaking, because nothing is broken until some method with @PreAuthorize("someCustomMethod()") is called. Only then you will notice that your custom SecurityExpressionRoot is actually not applied anymore. So, there is no reason to have this protected method if its overriding doesn't work anymore after update. In this case I would prefer compile-time breaking.

I understand your opinion about composition over inheritance and I generally agree with it, but in this case composition doesn't help. I tried, but there are too many internal (not public) components involved, so, I can't just use DefaultMethodSecurityExpressionHandler as a delegate. Even creation of custom SecurityExpressionRoot alone is tricky in this case because standard implementation uses root.setPermissionEvaluator(getPermissionEvaluator()) etc. methods, and most of these getters are not public (so you can't call delegate.getPermissionEvaluator()).

I think the ability to extend SecurityExpressionRoot to provide your own authorization DSL is a good and useful feature. It would be useful to make this more easy, not more hard. :)

Of course, it is alway possible to implement MethodSecurityExpressionHandler from scratch, but it will be too hard and will require to copy-paste half of Spring Security (because many components are not public). And also it is really hard and error prone to do it properly, especially when all you want to do is to extend the authorization DSL. :)