Announcement Announcement Module
Collapse
No announcement yet.
A bug in org.acegisecurity.vote.UnanimousBased Page Title Module
Move Remove Collapse
This topic is closed
X
X
Conversation Detail Module
Collapse
  • Filter
  • Time
  • Show
Clear All
new posts

  • A bug in org.acegisecurity.vote.UnanimousBased

    In org.acegisecurity.vote.UnanimousBased, the original decide method is as follows:
    Code:
    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:
    Code:
    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

  • #2
    Originally posted by iversion View Post
    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...

    Comment


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

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

      Comment

      Working...
      X