Some enhancement suggestions for new Authorization architecture classes for customizing the authorization logic

[fatihdogmus]

Hello,

I was trying to extend the current pre authorization behavior to suit our needs. Basically I wanted to look at the request headers, if some conditions are met, execute a custom authorization code and grant access from that logic, otherwise delegate to default pre authorize authorization manager.

But this was very hard to understand and do.

In the documents, for customizing the @PreAuthorize authorization logic, it is said to be done like this:

But this only shows how to replicate the default behavior, not how to customize it.

As I set to do customize my logic and add a custom AuthorizationManager, I was faced with these challanges:

1. AuthorizationManagerBeforeMethodInterceptor doesn't have a factory function or constructor that just accepts an AuthorizationManager and can only be instantiated with a default authorization managers(Yes it has one that accepts an AuthorizationManager, but it also needs a pointcut, which I will come back later)
2. Since I saw that, I wanted to create a custom class that extended PreAuthorizeAuthorizationManager. In here, I wanted to execute some logic or delegate to super in other cases. But I saw that PreAuthorizeAuthorizationManager, or any other concrete implementation of AuthorizationManager, are marked as final and can't be extended to customize the logic and be provided to AuthorizationManagerBeforeMethodInterceptor.
3. Event if you write a custom AuthorizationManager that executes your logic or delegates to constructed PreAuthorizeAuthorizationManager in other cases, you need to provide a Pointcut to pass to interceptor. Unfortunately, the utility class used to get the Pointcuts for @PreAuthorize annotation(AuthorizationMethodPointcuts) is package private and you can't easily create the pointcut that you need.

Because of that, I had to create a default AuthorizationManagerBeforeMethodInterceptor, get its Pointcut, than create my own custom interceptor with this pointcut and my custom delegating AuthorizationManger.

Here is the code that I had to write:

And this is the custom AuthorizationManager:

The code is very small, but in order to get there, I had to fumble through a lot of source code.

There were a lot of pain points while trying to figure out how to do some basic thing.

Here our my suggestions:

1. Make the concrete implementations of AuthorizationManager non-final, so that we can extend them and customize it with our logic, or make them more composeable.
2. Add a builder or constructor to AuthorizationManagerBeforeMethodInterceptor to only get an AuthorizationManager to be used inside, and not bother with pointcuts and use the default one, as it is the case with factory function that takes a PreAuthorizeAuthorizationManager.
3. Add a section on how to create your own AuthorizationManager and register it as default for one or more of the interceptors that come with @EnableMethodSecurity, ideally showing how to delegate to default managers.

I don't know if I over-complicated things with my limited knowledge of the new authorization architecture, but in other parts of the spring security, by just reading the source code and understanding the default behavior, I could extend or compose the existing classes with my own to suit my needs. But this was not that easy to do with the current authorization classes.

Thank you.

[jzheaux]

Thanks for the analysis and suggestions, @fatihdogmus, I'm glad that you are trying out the new AuthorizationManager support.

Given that the support is quite new, we anticipate enhancements being needed. That said, I'm a little confused about your usage of method security support, and I want to make sure I'm clear before we try and figure out how to change things.

First, why are you trying to look at request headers during method invocation instead of in the filter chain? Second, do these headers affect both authentication and authorization? Maybe a sample request and its interpretation would help me understand better.

[jzheaux]

In the meantime, I think I can simplify the code you have written. Have you already seen this sample in the documentation (slightly altered for your situation):

@Bean 
@Role(BeanDefinition.ROLE_INFRASTRUCTURE)
public Advisor preAuthorize(InternalPreAuthorizeAuthorizationManager rules) {
    AnnotationMethodMatcher pattern = new AnnotationMethodMatcher(PreAuthorize.class);
    AuthorizationManagerBeforeMethodInterceptor interceptor = 
        new AuthorizationManagerBeforeMethodInterceptor(pattern, rules);
    interceptor.setOrder(AuthorizationInterceptorsOrder.PRE_AUTHORIZE.getOrder());
    return interceptor;
}

You can also take a look at AnnotationMatchingPointcut if you need more sophisticated annotation searching behavior like class-level annotations or annotation inheritance.

[fatihdogmus]

Hello,

Thanks for taking your time for answering, appreciate all the work you guys are doing on spring security side.

Sorry that I couldn't write earlier, I was on site mission and couldn't access my PC.

First, why are you trying to look at request headers during method invocation instead of in the filter chain

We mostly use @PreAuthorize to secure our endpoints. AFAIU, authorization checks for annotations don't happen in filters and are executed via the mentioned interceptor, via AOP. That is why I tried to override the base interceptor. I don't know if that is completely true though, since I'm not very familiar with the new architecture.

Second, do these headers affect both authentication and authorization.

Yes they do. Basically, we send a secret header that if present, should bypass all authentication and authorization checks. We have a filter for authenticating the user as an internal user if the header is present. Here is the code sample for what we use for authentication, key parts omitted and written as psuedocode.

@Component
public class InternalRequestAuthenticationFilter extends OncePerRequestFilter {

    @Override
    protected void doFilterInternal(final HttpServletRequest request, final HttpServletResponse response, final FilterChain filterChain) throws ServletException, IOException {
        String header = request.getHeader("someHeader");
        if (headerIsInternalUser(header)) {
            SecurityContextHolder.clearContext();
            SecurityContext securityContext = SecurityContextHolder.createEmptyContext();
            securityContext.setAuthentication(new AnAuthenticationObjectWithInternalUser());
            SecurityContextHolder.setContext(securityContext);
        }

        filterChain.doFilter(request, response);
    }
}

This filter is injected in SecurityFilterChain and executed before our actual authentication checks are performed, so that the filters down the line will see that user is already authenticated.

In the meantime, I think I can simplify the code you have written.

Thank you for your suggestion, I didn't see that part. But when I tried it with my code, it doesn't compile since AnnotationMethodMatcher doesn't extend pointcut and the constructor expects a pointcut to be provided. I tried the given example in docs too and it doesn't work neither. I thought it is a new addition, we are on spring boot 2.6.4, but when I updated to 2.6.6 it still doesn't work. Am I doing something wrong?

Error attached below

When I tried the same code with AnnotationMatchingPointcut, it works like a charm though. I didn't know that class existed and thought these sorts of getting a pointcut based on an annotation can only be done with the package private class. This definitely simplifies the code, thank you.

[jzheaux]

Thanks, @fatihdogmus, I see my mistake. Yes, please use AnnotationMatchingPointcut instead. I've opened #11095 to correct the documentation.

[jzheaux]

Okay, gotcha.

Given that the components that exist are already simple to use, I'd like to leave the API as-is to keep it simple. The constructor already takes an AuthorizationManager interface and the Pointcut, as you demonstrated, is straightforward. As such I'm going to close this issue.

I'm open to taking another look in the future if such a need arises.