Description
Nick Williams (Migrated from SEC-2334) said:
Currently, UrlAuthorizationConfigurer, ExpressionUrlAuthorizationConfigurer, and GlobalMethodSecurityConfiguration provide ways for configuring a custom AccessDecisionManager (either by calling a method, as in the first two cases, or overriding a method, as in the latter case). For all of the standard AccessDecisionManager implementations, you want to construct them with a List of AccessDecisionVoter implementations. Then, some AccessDecisionVoter implementations require either a SecurityExpressionHandler or PreInvocationAuthorizationAdvice to function properly.
So, taking the ExpressionUrlAuthorizationConfigurer case as an example (the UrlAuthorizationConfigurer case is extremely similar), let's take a look at what's involved to simply switch from the AffirmativeBased decision manager to the ConsensusBased decision manager:
@Override
protected void configure(HttpSecurity security) throws Exception {
List<AccessDecisionVoter> decisionVoters = new ArrayList<AccessDecisionVoter>();
WebExpressionVoter expressionVoter = new WebExpressionVoter();
expressionVoter.setExpressionHandler(expressionHandler);
decisionVoters.add(new DefaultWebSecurityExpressionHandler());
security
.authorizeRequests()
.accessDecisionManager(new ConsensusBased(decisionVoters))
...
}The obvious problem here is that the first four lines of code in this method are duplicated and unnecessary. They are identical to code found in ExpressionUrlAuthorizationConfigurer. Now AbstractInterceptUrlConfigurer defines a package-private, abstract method getDecisionVoters that UrlAuthorizationConfigurer and ExpressionUrlAuthorizationConfigurer implement. How does this configuration method change if we make getDecisionVoters public and take advantage of it?
@Override
protected void configure(HttpSecurity security) throws Exception
{
security
.authorizeRequests()
.accessDecisionManager(new ConsensusBased(
security.authorizeRequests().getDecisionVoters()
))
...
}This code is much cleaner and takes advantage of the default, standard expression handler and voters while using a different decision manager. For clarity, it might make sense to rename getDecisionVoters to something like createDefaultDecisionVoters, but you get the idea.
Next let's take a look at GlobalMethodSecurityConfiguration. Assume you want to do the same thing with it: just change from the AffirmativeBased to the ConsensusBased decision manager. As of now:
@Configuration
@EnableGlobalMethodSecurity(...)
public static class MethodSecurityConfiguration extends GlobalMethodSecurityConfiguration {
@Override
protected AccessDecisionManager accessDecisionManager() {
List<AccessDecisionVoter> decisionVoters = new ArrayList<>();
ExpressionBasedPreInvocationAdvice expressionAdvice = new ExpressionBasedPreInvocationAdvice();
expressionAdvice.setExpressionHandler(getExpressionHandler());
if(prePostEnabled()) {
decisionVoters.add(new PreInvocationAuthorizationAdviceVoter(
expressionAdvice));
}
if(jsr250Enabled()) {
decisionVoters.add(new Jsr250Voter());
}
decisionVoters.add(new RoleVoter());
decisionVoters.add(new AuthenticatedVoter());
return new ConsensusBased(decisionVoters);
}
}Once again, we've duplicated a lot of code unnecessarily. Additionally, prePostEnabled() and jsr250Enabled() have private access and can't be called from here. Arguably you could eliminate this last problem by only adding the voters that your @EnableGlobalMethodSecurity config requires, but still. It's definitely not clean, and the only way to get it "right" is to have some knowledge of the Spring Security source code. Now let's look at this same change using the (currently non-existent) createDefaultDecisionVoters method:
@Configuration
@EnableGlobalMethodSecurity(...)
public static class MethodSecurityConfiguration extends GlobalMethodSecurityConfiguration {
@Override
protected AccessDecisionManager accessDecisionManager() {
return new ConsensusBased(createDefaultDecisionVoters());
}
}WOW. Much cleaner. Adding the createDefaultDecisionVoters() here also provides us with another capability. If you want, you can create the default voters and add just your one custom voter to them, still creating the default decision manager (or a custom decision manager--it's up to you):
@Configuration
@EnableGlobalMethodSecurity(...)
public static class MethodSecurityConfiguration extends GlobalMethodSecurityConfiguration {
@Override
protected List<AccessDecisionVoter> createDefaultDecisionVoters() {
List<AccessDecisionVoter> list = super.createDefaultDecisionVoters();
list.add(new MyCustomDecisionVoter());
return list;
}
}Finally, in both of these cases, exposing a getDecisionVoters/createDefaultDecisionVoters method ensures that if any bug fixes are made to the default implementations, or if any new default voters are added, the user gets this update on the next upgrade.
Given these use cases and compelling arguments :-), I propose the following:
Rename AbstractInterceptUrlConfigurer#getDecisionVoters to createDefaultDecisionVoters and make it public.
Create a GlobalMethodSecurityConfiguration#getDecisionVoters method, move the voter creation to it, and make it protected.
It's possible I missed other configuration classes with similar design, in which case I implicitly make analogous proposals for those classes as well.