A bug in org.acegisecurity.vote.UnanimousBased

[iversion]

In org.acegisecurity.vote.UnanimousBased, the original decide method is as follows:

public class UnanimousBased extends AbstractAccessDecisionManager {
    public void decide(Authentication authentication, Object object, ConfigAttributeDefinition config)
        throws AccessDeniedException {
        int grant = 0;
        int abstain = 0;

        Iterator configIter = config.getConfigAttributes();

        while (configIter.hasNext()) {
            ConfigAttributeDefinition thisDef = new ConfigAttributeDefinition();
            thisDef.addConfigAttribute((ConfigAttribute) configIter.next());

            Iterator voters = this.getDecisionVoters().iterator();

            while (voters.hasNext()) {
                AccessDecisionVoter voter = (AccessDecisionVoter) voters.next();
                int result = voter.vote(authentication, object, thisDef);

                switch (result) {
                case AccessDecisionVoter.ACCESS_GRANTED:
                    grant++;

                    break;

                case AccessDecisionVoter.ACCESS_DENIED:
                    throw new AccessDeniedException(messages.getMessage("AbstractAccessDecisionManager.accessDenied",
                            "Access is denied"));

                default:
                    abstain++;

                    break;
                }
            }
        }

        // To get this far, there were no deny votes
        if (grant > 0) {
            return;
        }

        // To get this far, every AccessDecisionVoter abstained
        checkAllowIfAllAbstainDecisions();
    }
}

In theory, the decide() method should firstly get a ConfigAttributeDefinition list responding to the resource, the pass the list to the voter which will judge the login user whether has the authorization to access the resource. But in the code above, the list only has been been added one attribute in the loop of "configIter.hasNext()". So the voter judge the authorization every time based on the only one attribute instead of a list. If using the original UnanimousBased and the resource support more than one role, there will be an exception that access is denied thrown.

In my opinion, the code should be corrected like follows:

public void decide(Authentication authentication, Object object, ConfigAttributeDefinition config)
        throws AccessDeniedException {
        int grant = 0;
        int abstain = 0;

        Iterator configIter = config.getConfigAttributes();

        ConfigAttributeDefinition thisDef = new ConfigAttributeDefinition();
        while (configIter.hasNext()) {
            ConfigAttribute configAttr = (ConfigAttribute) configIter.next();
            thisDef.addConfigAttribute(configAttr);
        }

        Iterator voters = this.getDecisionVoters().iterator();
        
        while (voters.hasNext()) {
            AccessDecisionVoter voter = (AccessDecisionVoter) voters.next();
            int result = voter.vote(authentication, object, thisDef);

            switch (result) {
            case AccessDecisionVoter.ACCESS_GRANTED:
                grant++;
                break;

            case AccessDecisionVoter.ACCESS_DENIED:
                throw new AccessDeniedException(messages.getMessage("AbstractAccessDecisionManager.accessDenied",
                        "Access is denied"));

            default:
                abstain++;
                break;
            }
        }

        // To get this far, there were no deny votes
        if (grant > 0) {
            return;
        }

        // To get this far, every AccessDecisionVoter abstained
        checkAllowIfAllAbstainDecisions();
    }

Does every one agree with me? Thank you ahead for any advice

[Luke Taylor]

In theory, the decide() method should firstly get a ConfigAttributeDefinition list responding to the resource, the pass the list to the voter which will judge the login user whether has the authorization to access the resource. But in the code above, the list only has been been added one attribute in the loop of "configIter.hasNext()". So the voter judge the authorization every time based on the only one attribute instead of a list.

From the Javadoc for this class:

This concrete implementation polls all configured AccessDecisionVoters for each ConfigAttribute and grants access if only grant votes were received.

Other voting implementations usually pass the entire list of ConfigAttributeDefinitions to the AccessDecisionVoter. This implementation differs in that each AccessDecisionVoter knows only about a single ConfigAttribute at a time.

So it's doing what it claims to do...

[Luke Taylor]

There is also some related discussion on UnanimousBased in this issue:

http://opensource.atlassian.com/proj...browse/SEC-379