Allow filtering non-collection types
For this feature, we have at least 2 options:
- Create a
DeniedHandlerMethodInterceptorthat interceptsAccessDeniedExceptionthrown from methods annotated with@DeniedHandler:
- The
AuthorizationManagerAfterMethodInterceptorwould have to throw a more contextualAccessDeniedException, something likeMethodInvocationResultAccessDeniedExceptionso we can have access to the result object that resulted in an exception to pass it to theMethodAccessDeniedHandler. - When the
@Pre/PostFilterfilters non-collection types, it would have to throw anAccessDeniedExceptionfor not authorized objects instead of just returningnullsoDeniedHandlerMethodInterceptorcan catch it.
- Add the logic inside
AuthorizationManagerAfterMethodInterceptor,AuthorizationManagerBeforeMethodInterceptorandDefaultMethodSecurityExpressionHandler:
- The
MethodSecurityEvaluationContextwould have to expose theMethodin order to access it fromDefaultMethodSecurityExpressionHandler - Don't need to throw an exception if the
@DeniedHandleris present - Don't need to create contextual
AccessDeniedException, those classes have access to bothMethodInvocationand theresult.
@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?
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@PreAuthorizeandMethodAccessDeniedHandler<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.
@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
AuthorizationDecisionso we can have all the benefits of using an interface instead of a class in the APIs. I create theAuthorizationResultinterface and madeAuthorizationDecisionimplement it. -
MethodAccessDeniedHandlerhas been renamed toAuthorizationDeniedPostProcessor. -
PreAuthorizeExpressionAttributeRegistryandPostAuthorizeExpressionAttributeRegistrycreate anExpressionAttributecontaining thepostProcessorClassinstead of resolving it in the method interceptors. - The
PostProcessableAuthorizationDecisionimplementsResolvableTypeProviderthat 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.