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

AffirmativeBased vs. AuthorizationManagers.anyOf(...) documentation #13069

Closed
KaVeKa opened this issue Apr 19, 2023 · 1 comment
Closed

AffirmativeBased vs. AuthorizationManagers.anyOf(...) documentation #13069

KaVeKa opened this issue Apr 19, 2023 · 1 comment
Assignees
Labels
in: core An issue in spring-security-core type: bug A general bug
Milestone

Comments

@KaVeKa
Copy link

KaVeKa commented Apr 19, 2023

I did not know whether to log this as a bug or an enhancement, but it feels more like a bug to me. Either in the migration documentation or in code. :-)

Describe the bug
Given 2 AuthorizationManagers/DecisionVoters:

  • AffirmativeBased way of working denies access when the first DecisionVoterabstains and the second denies.
  • AuthorizationManagers.anyOf(...) allows access when the first AuthorizationManagerabstains.

To Reproduce
I believe there is a small difference in the implementation between AffirmativeBased and AuthorizationManagers.anyOf().
More precisely in handling AccessDecisionVoter.ABSTAIN vs a null instance of AuthorizationDecision.

AffirmativeBased.java

public void decide(Authentication authentication, Object object, Collection<ConfigAttribute> configAttributes)
			throws AccessDeniedException {
    int deny = 0;
    for (AccessDecisionVoter voter : getDecisionVoters()) {
	    int result = voter.vote(authentication, object, configAttributes);
	    switch (result) {
	    case AccessDecisionVoter.ACCESS_GRANTED:
		    return;
	    case AccessDecisionVoter.ACCESS_DENIED:
		    deny++;
		    break;
	    default:
		    break; // --> Continues the loop and checks all DecisionVoters.
	    }
    }
    if (deny > 0) {
	    throw new AccessDeniedException(
			    this.messages.getMessage("AbstractAccessDecisionManager.accessDenied", "Access is denied"));
    }
    // To get this far, every AccessDecisionVoter abstained
    checkAllowIfAllAbstainDecisions();
}

AuthorizationManagers.java

public static <T> AuthorizationManager<T> anyOf(AuthorizationManager<T>... managers) {
    return (authentication, object) -> {
	    List<AuthorizationDecision> decisions = new ArrayList<>();
	    for (AuthorizationManager<T> manager : managers) {
		    AuthorizationDecision decision = manager.check(authentication, object);
		    if (decision == null || decision.isGranted()) {
			    return decision; // --> Shortcuts the whole decision making process. Basically says ABSTAIN == GRANTED
		    }
		    decisions.add(decision);
	    }
	    return new CompositeAuthorizationDecision(false, decisions);
    };
}

Expected behavior
The behavior should be the same, or the migration docs should state that the implementations are not equivalent.

@KaVeKa KaVeKa added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Apr 19, 2023
@jzheaux
Copy link
Contributor

jzheaux commented Apr 24, 2023

Hi, @KaVeKal. I agree that this is a bug in AuthorizationManagers#anyOf, but I also see a bug in the docs related to allOf. anyOf should return true only if any of the non-abstaining delegates return true. allOf should return true only if all of the non-abstaining delegates return true.

I'll update the code as well as the docs accordingly.

Also, I'm thinking that it would be good to introduce a configuration option to clarify what should be done when all delegates abstain. I've created #13085 to address that.

@jzheaux jzheaux added this to the 5.8.4 milestone Apr 24, 2023
@jzheaux jzheaux added in: core An issue in spring-security-core and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core An issue in spring-security-core type: bug A general bug
Projects
None yet
Development

No branches or pull requests

2 participants