Add Authorization Denied Handlers for Method Security

[marcusdacoregio]

For this feature, we have at least 2 options:

1. Create a DeniedHandlerMethodInterceptor that intercepts AccessDeniedException thrown from methods annotated with @DeniedHandler:

• The AuthorizationManagerAfterMethodInterceptor would have to throw a more contextual AccessDeniedException, something like MethodInvocationResultAccessDeniedException so we can have access to the result object that resulted in an exception to pass it to the MethodAccessDeniedHandler.
• When the @Pre/PostFilter filters non-collection types, it would have to throw an AccessDeniedException for not authorized objects instead of just returning null so DeniedHandlerMethodInterceptor can catch it.

2. Add the logic inside AuthorizationManagerAfterMethodInterceptor, AuthorizationManagerBeforeMethodInterceptor and DefaultMethodSecurityExpressionHandler:

• The MethodSecurityEvaluationContext would have to expose the Method in order to access it from DefaultMethodSecurityExpressionHandler
• Don't need to throw an exception if the @DeniedHandler is present
• Don't need to create contextual AccessDeniedException, those classes have access to both MethodInvocation and the result.

[jzheaux]

Thanks for putting this together, @marcusdacoregio! I've left some feedback inline.

[marcusdacoregio]

This class has been copied from spring-security-web. We would probably want to deprecate it in spring-security-web and recommend usages to use the class from spring-security-core

[marcusdacoregio]

@rwinch @jzheaux I've pushed a new commit that uses a HandleAccessDeniedMethodInterceptor to handle denied exceptions thrown from the method security annotations. This won't work with @PreFilter and @PostFilter because they do not throw such exceptions and I don't know if changing its semantics right now would be the best choice.

One concern is that the MethodAccessDeniedHandler is using the Object type for deniedObject, this will make users have to be aware of the possible types and it forces the use of the instanceof operator. To aliviate the problem, I provided AbstractMethodInvocationAccessDeniedHandler with two type-safe methods that can be overriden. Any suggestions how can this be improved?

Another concern is that exceptions can be expensive and it can have a significant impact on application performance (although I haven't done any benchmarking yet). Consider the following class:

class User {

  // ...

  @PreAuthorize("hasRole('ADMIN')")
  @HandleAccessDenied(FirstNameFallbackHandler.class)
  public String getName() {
    return this.name;
  }

  @PreAuthorize("hasRole('ADMIN')")
  @HandleAccessDenied
  public String getAddress() {
    return this.address;
  }

  @PreAuthorize("hasRole('ADMIN')")
  @HandleAccessDenied
  public String getPhoneNumber() {
    return this.phoneNumber;
  }
 
}

If we were to use those getters, 3 access denied exceptions would have to be constructed (if it is not an admin) just to be handled by HandleAccessDeniedMethodInterceptor. For me, it sounds better not to create an exception and handle the access denied at the moment we validate the security expression. What do you think?

[marcusdacoregio]

After talking to @rwinch and @jzheaux we decided to support filtering non-collection types only in @PreAuthorize and @PostAuthorize for now, this have some advatanges:

• Allow us to know exactly if the access denied happened on the annotation expression itself
• An exception is not instantiated if there is a handler (an exception can still be thrown by the handler)
• Better use of generics by accepting MethodAccessDeniedHandler<MethodInvocation> for @PreAuthorize and MethodAccessDeniedHandler<MethodInvocationResult> for @PostAuthorize
• Since Method Security now supports Meta Annotations (Add Meta-annotation Parameter Support #14480), users can create their own annotation and do not depend on a Spring Security on their business code.

[rwinch]

Thanks @marcusdacoregio This is really taking shape! I've provided some comments below for your consideration

[rwinch]

Consider naming this AuthorizationDeniedException to align with the AuthorizationDecision contained within it

[jzheaux]

Or AuthorizationDecisionException. AuthenticationException implies that authentication failed, e.g. AuthenticationFailedException is a little redundant.

[rwinch]

Should probably verify decision.isGranted() == false

[rwinch]

We could change the MethodAccessDeniedHandlerResolver to always return non-null result. The default implementation would be throw new AccessDeniedException("Access Denied");

[rwinch]

This could be somewhat error prone if we later add a setter method for the deniedHandlerResolver as it might override an explicitly set denied handler. We should probably only set it here if the handler is the default by assigning the default to a static instance and then checking if it is ==).

private static final MethodAccessDeniedHandlerResolver DEFAULT_HANDLER_RESOLVER = ...

private MethodAccessDeniedHandlerResolver deniedHandlerResolver = DEFAULT_HANDLER_RESOLVER;

public void setApplicationContext(ApplicationContext applicationContext) {
     // ...
    if (this.deniedHandlerResolver == DEFAULT_HANDLER_RESOLVER) {
         this.deniedHandlerResolver = new DefaultMethodAccessDeniedHandlerResolver(applicationContext);
    }
}

[rwinch]

In isolation, postAuthorize might return null, but in the context we are using this method, this should never happen because this method is never invoked unless a PostAuthorize was found for the authorization expression to be evaluated. The validation is necessary with this design since this can be invoked in isolation, but it isn't ideal that we know there should be a guarantee to have a non-null PostAuthorize for our use cases, but we must account for it because the APIs are separate.

PreAuthorize is in a very similar situation.

I believe this along with the performance / consistency considerations is a reason to change the design

[rwinch]

The way that this is currently implemented the PreAuthorize and PostAuthorize annotations are looked up twice. The first is in PreAuthorizeAuthorizationManager and PostAuthorizeAuthorizationManager using PreAuthorizeExpressionAttributeRegistry and PostAuthorizeExpressionAttributeRegistry respectively. Using additional reflection to lookup methods is unnecessary overhead for something that is likely going to be invoked a lot.

The other concern is that the way in which the annotation is resolved for the MethodAccessDeniedHandler and the expression is different. I haven't looked through the code, but it is possible that the resolution is inconsistent. If it isn't already inconsistent, it is possible they will become inconsistent.

[rwinch]

I think that we should remove this interface in favor of encapsulating the logic of looking up the user provided MethodAccessDeniedHandler in a framework provided MethodAccessDeniedHandler. The framework provided MethodAccessDeniedHandler would receive as an argument the information from PreAuthorize or the PostAuthorize annotation that was used to create the expression. This means that the annotation only needs to be resolved once (improves performance) and we can guarantee that there is consistency in resolving the expression and the MethodAccessDeniedHandler to delegate to.

[rwinch]

I do not think that this is being used anymore and I think that it can be removed.

[rwinch]

Consider the name MethodAuthorizationDecisionHandler to better align with the argument that is being passed in.

[rwinch]

Consider a method name that makes it more clear that it can replace the deniedObject. I wonder if this is more of a post processor of sorts in which the class name and the method name could be updated to include postProcess. We'd want the interface documented that it is allowed to throw an AccessDeniedException (likely though it would prefer a DecisionAwareAccessDeniedException since there is an AuthorizationDecision available)

[marcusdacoregio]

@rwinch @jzheaux the PR has been updated based on our last discussion.

• Rob mentioned that it would be nice if we could create a new interface as a replacement for AuthorizationDecision so we can have all the benefits of using an interface instead of a class in the APIs. I create the AuthorizationResult interface and made AuthorizationDecision implement it.
• MethodAccessDeniedHandler has been renamed to AuthorizationDeniedPostProcessor.
• PreAuthorizeExpressionAttributeRegistry and PostAuthorizeExpressionAttributeRegistry create an ExpressionAttribute containing the postProcessorClass instead of resolving it in the method interceptors.
• The PostProcessableAuthorizationDecision implements ResolvableTypeProvider that allow us to verify its generics at runtime, so we can know whether the post processor class can post process the type that we expect. See the logic here. I could use some ideas on how to improve that.

[rwinch]

I've provided feedback below

[rwinch]

Instead of naming it Default, perhaps consider naming it something around how it behaves.

[rwinch]

Consider naming this AuthorizationDeniedException to align with AccessDeniedException and include the information that it used when Authorization was denied.

[rwinch]

If the class were generic, then DefaultPreInvocationAuthorizationDeniedPostProcessor could be removed. Consider having a static method similar to Collections.empty() which allows type safe way to provide an implementation of AuthorizationDeniedPostProcessor

[marcusdacoregio]

I don’t think the class can be generic because it is used inside the PreAuthorize and PostAuthorize and annotation’s attributes must be constants. We need an actual implementation of the concrete type to use

[rwinch]

I do not think that this is being used anymore and I think that it can be removed.

[jzheaux]

This looks awesome, @marcusdacoregio! I've left some minor feedback inline.