spring-security icon indicating copy to clipboard operation
spring-security copied to clipboard

Make default expression handler in PrePostMethodSecurityConfiguration to use existing permission evaluator

Open GFriedrich opened this issue 2 years ago • 3 comments

Expected Behavior The expression handler that gets created per default in PrePostMethodSecurityConfiguration at https://github.com/spring-projects/spring-security/blob/74d646f569ad5df4bd1055ab016444cea9af6fbd/config/src/main/java/org/springframework/security/config/annotation/method/configuration/PrePostMethodSecurityConfiguration.java#L63 doesn't use the existing permission evaluator from the Spring context but keeps the default DenyAllPermissionEvaluator.

Current Behavior The expression handler should be setup in the PrePostMethodSecurityConfiguration with the existing permission evaluator like

	@Autowired(required = false)
	void setPermissionEvaluator(PermissionEvaluator permissionEvaluator) {
		this.expressionHandler.setPermissionEvaluator(permissionEvaluator);
	}

Context I've switched from the @EnableGlobalMethodSecurity annotation to the @EnableMethodSecurity annotation and this caused the existing permission evaluator not to be applied to @PreAuthorize annotations for methods.

There is of course a way to fix that easily by defining a custom expression handler that utilizes the permission evaluator, but I would've expected that the old way of the @EnableGlobalMethodSecurity using the existing permission evaluator should also work with the new annotation without defining additional beans.

But maybe this was a conscious decision or I'm simply missing something. Thanks in advance for taking a look.

GFriedrich avatar Jul 19 '22 18:07 GFriedrich

@GFriedrich, glad to hear you are using the new @EnableMethodSecurity.

It is intentional to try and keep things as simple as possible for the initial releases. Since this feature's absence doesn't create a security issue in the worst case (all evaluations are denied by default) and because it's quite simple to make the adjustment, it's a lower priority feature to add. Also, I don't want to add a feature just for the sake of making it easier to migrate.

That said, I think that it's reasonable to leave this ticket open and see if people vote for it as a desired feature.

In the meantime, I agree that the simplest way to use your PermissionEvaluator is to publish a @Bean like so:

@Bean 
MethodSecurityExpressionHandler methodSecurityExpressionHandler() {
    DefaultMethodSecurityExpressionHandler expressionHandler = new DefaultMethodSecurityExpressionHandler();
    expressionHandler.setPermissionEvaluator(...);
    return expressionHandler;
}

Or, if you don't want to affect the expression handler, you can always exercise your PermissionEvaluator bean directly in the annotations like this:

@PreAuthorize("@permissionEvaluator.hasPermission(...)")

adding the @permissionEvaluator. string to each expression.

jzheaux avatar Jul 20 '22 22:07 jzheaux

@jzheaux: Thanks for checking and good to hear that I didn't miss anything. Maybe it is worth to mention it somewhere that there is at least this kind of difference when switching the annotations. Simply to make a migration path clear to everybody and not falling for the same issue over and over again. Thanks for your time nevertheless. 🤝

GFriedrich avatar Jul 20 '22 22:07 GFriedrich

Agreed, @GFriedrich. I've linked this ticket to the migration guide ticket so that it gets included.

jzheaux avatar Jul 21 '22 16:07 jzheaux