spring-security icon indicating copy to clipboard operation
spring-security copied to clipboard

Allow filtering non-collection types

Open marcusdacoregio opened this issue 1 year ago • 3 comments

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.
  1. 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.

marcusdacoregio avatar Mar 08 '24 14:03 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 avatar Mar 13 '24 12:03 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 (#14480), users can create their own annotation and do not depend on a Spring Security on their business code.

marcusdacoregio avatar Mar 15 '24 12:03 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.

marcusdacoregio avatar Mar 20 '24 12:03 marcusdacoregio