Spring Security AbstractAuthenticationProcessingFilter produces error when registering a custom UsernamePasswordAuthenticationFilter Bean

Summary

Registering a custom UsernamePasswordAuthenticationFilter Bean produces java.lang.IllegalArgumentException: authenticationManager must be specified error.

Actual Behavior

When I add a custom UsernamePasswordAuthenticationFilter in the @Configuration class that extends WebSecurityConfigurerAdapter, Spring application fails to run.

2020-04-02 22:53:09.405 ERROR 58116 --- [  restartedMain] o.s.boot.SpringApplication               : Application run failed

org.springframework.context.ApplicationContextException: Unable to start web server; nested exception is org.springframework.boot.web.server.WebServerException: Unable to start embedded Tomcat
    at org.springframework.boot.web.servlet.context.ServletWebServerApplicationContext.onRefresh(ServletWebServerApplicationContext.java:156) ~[spring-boot-2.2.6.RELEASE.jar:2.2.6.RELEASE]
...
Caused by: java.lang.IllegalArgumentException: authenticationManager must be specified
    at org.springframework.util.Assert.notNull(Assert.java:198) ~[spring-core-5.2.5.RELEASE.jar:5.2.5.RELEASE]
    at org.springframework.security.web.authentication.AbstractAuthenticationProcessingFilter.afterPropertiesSet(AbstractAuthenticationProcessingFilter.java:164) ~[spring-security-web-5.2.2.RELEASE.jar:5.2.2.RELEASE]
    at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.invokeInitMethods(AbstractAutowireCapableBeanFactory.java:1855) ~[spring-beans-5.2.5.RELEASE.jar:5.2.5.RELEASE]
    at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.initializeBean(AbstractAutowireCapableBeanFactory.java:1792) ~[spring-beans-5.2.5.RELEASE.jar:5.2.5.RELEASE]
    ... 56 common frames omitted

Expected Behavior

Spring application should run successfully without error even after the custom UsernamePasswordAuthenticationFilter filter Bean has been added.

Configuration

@EnableWebSecurity
public class WebSecurityConfig extends WebSecurityConfigurerAdapter {

    @Override
    protected void configure(HttpSecurity http) throws Exception {
        http
                .authorizeRequests()
                    .anyRequest().authenticated()
                .and()
                .formLogin()
                .and()
                .httpBasic()
                .and()
                .addFilterBefore(getSimpleAuthFilter(), UsernamePasswordAuthenticationFilter.class);
    }

    @Bean
    public SimpleUsernamePasswordAuthFilter getSimpleAuthFilter() throws Exception {
        return new SimpleUsernamePasswordAuthFilter(authenticationManager());
    }
}

Version

spring-boot-2.2.6.RELEASE spring-security-web:5.2.2.RELEASE

Sample

https://github.com/HomoEfficio/spring-security-practice

Comment From: rwinch

Thanks for the report and the simple example to recreate your issue. This is not a bug. The issue was with SimpleUsernamePasswordAuthFilter which had a separate reference to AuthenticationManager. This means there was an unused (null) variable in UsernamePasswordAuthenticationFilter. It also meant that SimpleUsernamePasswordAuthFilter.setAuthenticationManager was setting an AuthenticationManager that would never be used.

I sent a PR to your sample repository https://github.com/HomoEfficio/spring-security-practice/pull/1

Comment From: HomoEfficio

@rwinch Thanks for the kind and warm answer, and PR.

Considering the responsibilities and usages of the AbstractAuthenticationProcessingFilter, authenticationManager property seems to be required, not optional.
So it is better to use constructor injection rather than to use setter injection, especially for the required properties.

But for now, the required property authenticationManager can only be set by setter method, which does not look so great in my opinion, and it leads to this uncomfortable issue.

What about adding another constructor that receives an AuthenticationManager to the AbstractAuthenticationProcessingFilter so we can seamlessly use constructor injection?
If you think this is reasonable, I will make another PR, including some follow-up changes for the UsernamePasswordAuthenticationFilter.

Comment From: rwinch

Thanks for your offer to help! I am ok with adding a constructor that accepts AuthenticationManager, but we cannot remove the old constructor for passivity reasons. If you are alright with submitting a PR, I can review it once you submit it.