Spring Security Optimize the addition of OAuth2AuthorizationRequestRedirectFilter

Description When both oauth2Login and oauth2Client are configured, the OAuth2AuthorizationRequestRedirectFilter should only be added once to the DefaultSecurityFilterChain.

Below is a proof: * OAuth2ClientConfigurer.java

public void configure(B builder) {
        this.authorizationCodeGrantConfigurer.configure(builder);
    }

and AuthorizationCodeGrantConfigurer.java

private void configure(B builder) {
            OAuth2AuthorizationRequestRedirectFilter authorizationRequestRedirectFilter = createAuthorizationRequestRedirectFilter(
                    builder);
            builder.addFilter(postProcess(authorizationRequestRedirectFilter));
            OAuth2AuthorizationCodeGrantFilter authorizationCodeGrantFilter = createAuthorizationCodeGrantFilter(
                    builder);
            builder.addFilter(postProcess(authorizationCodeGrantFilter));
        }

• OAuth2LoginConfigurer.java

public void configure(B http) throws Exception {
        OAuth2AuthorizationRequestRedirectFilter authorizationRequestFilter;
        if (this.authorizationEndpointConfig.authorizationRequestResolver != null) {
            authorizationRequestFilter = new OAuth2AuthorizationRequestRedirectFilter(
                    this.authorizationEndpointConfig.authorizationRequestResolver);
        }
        else {
            String authorizationRequestBaseUri = this.authorizationEndpointConfig.authorizationRequestBaseUri;
            if (authorizationRequestBaseUri == null) {
                authorizationRequestBaseUri = OAuth2AuthorizationRequestRedirectFilter.DEFAULT_AUTHORIZATION_REQUEST_BASE_URI;
            }
            authorizationRequestFilter = new OAuth2AuthorizationRequestRedirectFilter(
                    OAuth2ClientConfigurerUtils.getClientRegistrationRepository(this.getBuilder()),
                    authorizationRequestBaseUri);
        }
        if (this.authorizationEndpointConfig.authorizationRequestRepository != null) {
            authorizationRequestFilter
                .setAuthorizationRequestRepository(this.authorizationEndpointConfig.authorizationRequestRepository);
        }
        if (this.authorizationEndpointConfig.authorizationRedirectStrategy != null) {
            authorizationRequestFilter
                .setAuthorizationRedirectStrategy(this.authorizationEndpointConfig.authorizationRedirectStrategy);
        }
        RequestCache requestCache = http.getSharedObject(RequestCache.class);
        if (requestCache != null) {
            authorizationRequestFilter.setRequestCache(requestCache);
        }
        http.addFilter(this.postProcess(authorizationRequestFilter));
        OAuth2LoginAuthenticationFilter authenticationFilter = this.getAuthenticationFilter();
        if (this.redirectionEndpointConfig.authorizationResponseBaseUri != null) {
            authenticationFilter.setFilterProcessesUrl(this.redirectionEndpointConfig.authorizationResponseBaseUri);
        }
        if (this.authorizationEndpointConfig.authorizationRequestRepository != null) {
            authenticationFilter
                .setAuthorizationRequestRepository(this.authorizationEndpointConfig.authorizationRequestRepository);
        }
        configureOidcSessionRegistry(http);
        super.configure(http);
    }

OAuth2ClientConfigurer.java and OAuth2LoginConfigurer.java both added an OAuth2AuthorizationRequestRedirectFilter filter, although this doesn't really affect the DefaultSecurityFilterChain itself. However, as someone who strives for perfection, having two such Filters is unnecessary. I believe that according to the principle of responsibility, adding the OAuth2AuthorizationRequestRedirectFilter filter should only be reserved for OAuth2LoginConfigurer, as it aligns more closely with the semantics of OAuth login.

Comment From: jzheaux

Thanks for the recommendation, @ilxqx, and for your dedication to clean code.

While it may seem like duplication, the two filter instances are separate in their purpose and can reasonably be configured with separate endpoints and separate components.

Given that it's been this way for some time now, some applications likely rely on this ability for the client filter to differ in configuration from the login filter. As such, we shouldn't remove that support lightly.

I'm going to close this for the time being. Please continue to reach out on this ticket with any additional information.

Comment From: ilxqx

Given that it's been this way for some time now, some applications likely rely on this ability for the client filter to differ in configuration from the login filter. As such, we shouldn't remove that support lightly.

@jzheaux Thank you for your reply.

I have a plan where we don't need to remove any filter from OAuth2ClientConfigurer.java and OAuth2LoginConfigurer.java. We can use getSharedObject/setSharedObject to share the same instance of OAuth2AuthorizationRequestRedirectFilter.

In this way, even if an application has both oauthLogin and oauthClient functions at the same time, only one OAuth2AuthorizationRequestRedirectFilter is added in the DefaultSecurityFilterChain.

Assuming OAuth2ClientConfigurer.java and OAuth2LoginConfigurer.java create an instance of OAuth2AuthorizationRequestRedirectFilter pseudocode:

var filter = http.getSharedObject(OAuth2AuthorizationRequestRedirectFilter.class);
if (filter == null) {
     filter = new OAuth2AuthorizationRequestRedirectFilter();
     http.setSharedObject(OAuth2AuthorizationRequestRedirectFilter.class, filter);
}
// Set additional properties therein as needed
filter.setXxx()
// ...

Can this approach solve the problem?