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

SEC-2332: GlobalMethodSecurityConfiguration does not configure the proper voters #2535

Closed
spring-issuemaster opened this issue Sep 17, 2013 · 4 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link

commented Sep 17, 2013

Nick Williams (Migrated from SEC-2332) said:

I could be missing something obvious here, and if so, please forgive me and skip to the bottom of this description. GlobalMethodSecurityConfiguration#accessDecisionManager() doesn't seem quite right to me:

First, it always configures a PreInvocationAuthorizationAdviceVoter, but if pre-post annotations aren't enabled this voter will never be used, so it seems like omitting it in this case would be preferable.

Second, it never adds a Jsr250Voter, even if JSR 250 annotations are enabled. How can JSR 250 annotations work without the proper voter?

It also never adds a WebExpressionVoter, AclEntryVoter, or RoleHierarchyVoter for those cases where the configuration supports it.

I believe it should be adding other voters (but, again, only when necessary) to fully complete the voting system and make it match the configuration.

Either Way
Whether the configured voters need to change or not, it seems to me that accessDecisionManager() isn't very friendly to being overridden. If someone ONLY wants to change the decision manager implementation but NOT which voters are used, he still has to re-implement/duplicate the 6 (or more) lines of code that constructs the voters. I think there should be a protected List<AccessDecisionVoter> accessDecisionVoters() method that handles this process and is called from accessDecisionManager() when constructing the manager.

@spring-issuemaster

This comment has been minimized.

Copy link
Author

commented Sep 18, 2013

Rob Winch said:

First, it always configures a PreInvocationAuthorizationAdviceVoter, but if pre-post annotations aren't enabled this voter will never be used

You are correct. This has been fixed.

Second, it never adds a Jsr250Voter, even if JSR 250 annotations are enabled.

You are correct. This got past the tests because there was only a test for verifying if @DenyAll worked (it did because all the voters abstained). I have updated the code to fix this and added an assertion that @PermitAll works properly.

It also never adds a WebExpressionVoter

The HttpServletRequest is not available on method invocations so this does not make sense.

..., AclEntryVoter

AclEntryVoter would be used by your PermissionEvaluator (i.e. AclPermissionEvaluator) so it doesn't make sense to include this as a voter.

, or RoleHierarchyVoter

RoleHierarchyVoter is not added with the namespace either.

Whether the configured voters need to change or not, it seems to me that accessDecisionManager() isn't very friendly to being overridden

I'm not very keen on adding another method as this starts to make GlobalMethodSecurityConfiguration too granular. There is a very easy way to do this by overriding the accessDecisionManager method already. Yes it does involve some code, but at this time this sort of customizations are relatively rare. I'd prefer not to make GlobalMethodSecurityConfiguration too complex. If you really disagree, feel free to create an enhancement request and we will see how much traction it gets. At that point I can re-evaluate the request.

@spring-issuemaster

This comment has been minimized.

Copy link
Author

commented Sep 18, 2013

Nick Williams said:

The HttpServletRequest is not available on method invocations so this does not make sense.

Duh. What was I thinking? :-)

AclEntryVoter would be used by your PermissionEvaluator (i.e. AclPermissionEvaluator) so it doesn't make sense to include this as a voter.
...
RoleHierarchyVoter is not added with the namespace either.

I'm a little confused about these two. First, AclPermissionEvaluator does not have any way to configure a voter, so AclEntryVoter could not be "used by" it. In the Javadoc for AclPermissionEvaluator, the only thing I see referencing voters is "Similar in behaviour to AclEntryVoter." So which is used when ACL is enabled? AclPermissionEvaluator or AclEntryVoter?

So if the namespace configures neither of these voters, then how can they ever get configured? Once an AccessDecisionManager is configured you can't change its voters (unless you use a deprecated method, of course). Is overriding the accessDecisionManager method the only way to configure the AclEntryVoter and RoleHierarchyVoter?


I'll consider filing an enhancement request and submitting a pull request. Don't know yet how much it's worth to me. :-)

@spring-issuemaster

This comment has been minimized.

Copy link
Author

commented Sep 19, 2013

Rob Winch said:

I'm a little confused about these two. First, AclPermissionEvaluator does not have any way to configure a voter, so AclEntryVoter could not be "used by" it.

You are correct. What I meant to type was that most people (or least should) tend to use the AclPermissionEvaluator. So if they need Acl support they would configure this and not need the AclEntryVoter.

So if the namespace configures neither of these voters, then how can they ever get configured? Once an AccessDecisionManager is configured you can't change its voters (unless you use a deprecated method, of course). Is overriding the accessDecisionManager method the only way to configure the AclEntryVoter and RoleHierarchyVoter?

Yes. Keep in mind adding every bell and whistle to Java Configuration support will make Java Configuration support just as difficult as using standard @bean so we must consider this carefully before adding it.

@spring-issuemaster

This comment has been minimized.

Copy link
Author

commented Sep 19, 2013

Nick Williams said:

Okay. Makes sense. Thanks for clearing that up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.