Skip to content
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

Make default expression handler in PrePostMethodSecurityConfiguration to use existing permission evaluator #11598

Open
Tracked by #11337
GFriedrich opened this issue Jul 19, 2022 · 3 comments
Assignees
Labels
in: config An issue in spring-security-config type: enhancement A general enhancement

Comments

@GFriedrich
Copy link

GFriedrich commented Jul 19, 2022

Expected Behavior
The expression handler that gets created per default in PrePostMethodSecurityConfiguration at

private final DefaultMethodSecurityExpressionHandler expressionHandler = new DefaultMethodSecurityExpressionHandler();
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 GFriedrich added status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels Jul 19, 2022
@jzheaux jzheaux self-assigned this Jul 20, 2022
@jzheaux
Copy link
Contributor

jzheaux commented Jul 20, 2022

@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 jzheaux added in: config An issue in spring-security-config and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 20, 2022
@jzheaux jzheaux added this to the General Backlog milestone Jul 20, 2022
@GFriedrich
Copy link
Author

@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. 🤝

@jzheaux
Copy link
Contributor

jzheaux commented Jul 21, 2022

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

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 type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

2 participants