AuthorizationManager should add support check like AuthenticationProvider

[abccbaandy]

Expected Behavior
Can be safely cast the Authentication principal to to myPrincipal in the check(Supplier<Authentication> authentication, T object) method.

Current Behavior
Have no idea if the MyAuthorizationManager can handle the Authentication.

Context
Need extra check if Authentication is instanceof MyAuthentication.
If have a method like

boolean supports(Class<?> authentication)

Then we can put the check code in the method, which make the authorization check code more clean.

[jzheaux]

Instead of a supports method, if an AuthenticationManager doesn't support a specific Authentication, then AuthenticationManager#authenticate should return null.

Have you tried that approach and if so where is it causing trouble for you?

[abccbaandy]

Not sure if you misunderstand my idea here. Let me explain with some example code.

Current:

MyAuthorizationManager implement AuthorizationManager{
  check(Supplier<Authentication> authentication, T object) {
    // Can not safe cast the authentication here, need another support method for class check 
  }
}

What I want:

MyAuthorizationManager implement AuthorizationManager{
  check(Supplier<Authentication> authentication, T object) {
    // Can safe cast the authentication here
  }
  boolean supports(Class<?> authentication) {
    return MyAuthentication.class.isAssignableFrom(authentication);
  }
}

And the supports method should called by the framework.

Hope these code explain more clear what I want.

[jzheaux]

I see your meaning, you want to be able to safely cast, and supports gives you the confidence that you can.

That said, you can safely cast in the following way as well:

MyAuthorizationManager implement AuthorizationManager{
  check(Supplier<Authentication> authentication, T object) {
    if (!(authentication.get() instanceof MyAuthentication myAuthentication)) {
         return null;
    }
    // ...
  }
}

I realize that it is a little different than what you are used to in AuthenticationProvider. Other than the unfamiliarity, does this approach give you any trouble?

[abccbaandy]

Of course I can check the type by myself in check method.
But I think it's not just unfamiliarity, it make the code more elegant.

Or is there any reason that AuthenticationProvider can have supports method but AuthorizationManager need check by itself?

[jzheaux]

Good question. The main reason is that it seems unnecessary, and we like to keep APIs as lean as possible. Introducing this method would require that the rest of Spring Security now call supports in advance of calling check; but, if an application just needs to call check, then everyone gets the same outcome without the extra method.

Also, considering your interest in this, please consider following #11428 which details future efforts to deprecate AuthenticationProvider. When you take a look at AuthenticationManager, ReactiveAuthenticationManager, AuthorizationManager, and ReactiveAuthorizationManager, you'll see that AuthenticationProvider, while perhaps the more familiar API for folks, is the outlier as far as API defintiion.

[jzheaux]

I'm going to close this as declined at this time, though I appreciate you raising the question and discussing it with me. Please feel free to continue the conversation here (and I'll continue responding) or contribute to the conversation at #11428.

[abccbaandy]

Then why the AuthenticationProvider have support method?

In fact, I really like the support idea, I even use this design in my own project, do check/valid in the core logic seems very bloated.
Just like we put @ValidSomeing in the parameter instead valid them in the code, our code can be more focus on what it should focus on.

By the way, I didn't see much relate information in that post, the post seems mainly talk about naming instead support method?

[jzheaux]

Then why the AuthenticationProvider have support method?

I think it's just another way to conceptualize the solution. That said, the interface was introduced before my time so I don't know.

These days, we suggest that folks have just one AuthenticationProvider per auth mechanism or use AuthenticationManagerResolver otherwise, so the supports method is largely immaterial at that point.

Just like we put @ValidSomeing in the parameter instead valid them in the code

I'm not sure I follow the analogy; I think you are talking about isolating concerns, which I agree is an important design principle.

My point is that this is an intrinsic part of the authorization concern. Consider that AuthorizationManager#check returns null to mean "I abstain", which is a reasonable AuthorizationDecision. So even if you did have a supports method, you'd still have to check for null in the return type to see if that AuthorizationManager abstained.

If you have to check for null anyway, then it seems there is no gain from an additional method call.

(Additionally, there is the issue of the Authentication being deferred (through a Supplier), so you'd need to load it once for supports and again for check.)

Here is the difference:

if (manager.supports(authentication.get())) {
    AuthorizationDecision decision = manager.check(authentication, object);
    if (decision == null) {
        // do abstain logic
    } else if (decision.isGranted()) {
        // do granted logic
    } else {
        // do denied logic
    }
} else {
    // do abstain logic
}

vs

AuthorizationDecision decision = manager.check(authentication, object);
if (decision == null) {
    // do abstain logic
} else if (decision.isGranted()) {
    // do granted logic
} else {
    // do denied logic
}

This has the benefit that the manager returning null means "I can't decide", which is a nice conflation of the two notions: "I don't support" and "I choose not to say whether this object is authorized". It also has the benefit that Supplier<Authentication>#get is called only once.

By the way, I didn't see much relate information in that post

What I was saying with the link is that AuthenticationProvider is under consideration for deprecation. In that case, that would mean that none of the prevailing authentication and authorization components have a supports method. I was just appealing to consistency, but I believe the above argument is likely stronger.