Deprecate Authorization Logic that uses `FilterInvocation`

[therepanic]

Closes: gh-17781

[therepanic]

However, I believe that we can also replace SecurityExpressionHandler<FilterInvocation> in favor of SecurityExpressionHandler<RequestAuthorizationContext> in the AbstractAuthorizeTag and JspAuthorizeTag classes.

However, there is a nuance: in FilterInvocation, if we pass request parameters, a DummyRequest is created that inherits from HttpServletRequestWrapper. We could introduce the same class and pass it to RequestAuthorizationContext. What do you think about this? Where should we place such a class? I think in our case it would be enough to copy it from FilterInvocation directly into AbstractAuthorizeTag.

[jzheaux]

Hi, @therepanic, thanks for the PR. Can you clarify where it is that the dummy request is created, as far as JSP Tag usage is concerned? I imagine that an evaluating tag would have access to the original HttpServletRequest and not need a wrapper dummy request.

I agree that this is used in the default WebInvocationPrivilegeEvaluator; however, I believe that is the extent of it.

[therepanic]

Thanks for your reply, @jzheaux. Yes, I was probably wrong.

I've thought about it now, and I think we can't replace SecurityExpressionHandler<FilterInvocation> in favor of SecurityExpressionHandler<RequestAuthorizationContext> in the AbstractAuthorizeTag and JspAuthorizeTag.

If we do that, it will be a breaking change. We can see a tests like this:

    @Test
    @SuppressWarnings(“rawtypes”)
    public void expressionFromChildContext() throws IOException {
        SecurityContextHolder.getContext().setAuthentication(new TestingAuthenticationToken(“user”, ‘pass’, “USER”));
		DefaultWebSecurityExpressionHandler expected = new DefaultWebSecurityExpressionHandler();
...

In order for this to work, I believe we need to replace DefaultWebSecurityExpressionHandler in the test, but this would again be a breaking change. If we cannot replace them, do we need to deprecate JspAuthorizeTag and AbstractAuthorizeTag?

WDYT?

[jzheaux]

@therepanic, good question. This issue is also addressed in WebSecurity. You can see the SecurityExpressionHandler adapter there.

Let's consider having it continue to look for SecurityExpressionHandler<FilterInvocation> and also falling back to DefaultHttpSecurityExpressionHandler, adapting it to SecurityExpressionHandler<FilterInvocation>. This will match the behavior in WebSecurity.

In a separate ticket, we can look at WebSecurity, AbstractAuthorizeTag, and other places that look for a SecurityExpressionHandler<FilterInvocation> and enhancing them to also look for a SecurityExpressionHandler<RequestAuthorizationContext>.

How do you feel about that plan?

[therepanic]

Hi, @jzheaux. I think that's a great plan and will be better. Avoiding the breaking change while enabling the deprecation path is the right approach.

I suppose we need to create an adapter something like this? As I understand it, you mean searching for the RequestAuthorizationContext and adapting it in FilterInvocation? For example, in the AbstractAuthorizeTag#getExpressionHandler method.

class RequestAuthorizationContextToFilterInvocationExpressionHandlerAdapter
implements SecurityExpressionHandler<FilterInvocation> {