From e71606aa60dbcfe7f70e89c5d9f4dca89a8f4c57 Mon Sep 17 00:00:00 2001 From: Oh Myung Woon Date: Thu, 9 Apr 2020 22:25:55 +0900 Subject: [PATCH 1/2] Add constructors receiving AuthenticationManager [gh-8309] --- ...bstractAuthenticationProcessingFilter.java | 38 +++++++++++++++++++ .../UsernamePasswordAuthenticationFilter.java | 5 +++ ...namePasswordAuthenticationFilterTests.java | 17 +++++++++ 3 files changed, 60 insertions(+) diff --git a/web/src/main/java/org/springframework/security/web/authentication/AbstractAuthenticationProcessingFilter.java b/web/src/main/java/org/springframework/security/web/authentication/AbstractAuthenticationProcessingFilter.java index 5f7cf4b8395..1331aaa895b 100644 --- a/web/src/main/java/org/springframework/security/web/authentication/AbstractAuthenticationProcessingFilter.java +++ b/web/src/main/java/org/springframework/security/web/authentication/AbstractAuthenticationProcessingFilter.java @@ -156,6 +156,43 @@ protected AbstractAuthenticationProcessingFilter( this.requiresAuthenticationRequestMatcher = requiresAuthenticationRequestMatcher; } + /** + * Creates a new instance with an {@link AuthenticationManager} + * + * @param authenticationManager the {@link AuthenticationManager} used to authenticate an {@link Authentication} object. + * Cannot be null. + */ + protected AbstractAuthenticationProcessingFilter(AuthenticationManager authenticationManager) { + setAuthenticationManager(authenticationManager); + } + + /** + * Creates a new instance with a default filterProcessesUrl and an {@link AuthenticationManager} + * + * @param defaultFilterProcessesUrl the default value for filterProcessesUrl. + * @param authenticationManager the {@link AuthenticationManager} used to authenticate an {@link Authentication} object. + * Cannot be null. + */ + protected AbstractAuthenticationProcessingFilter(String defaultFilterProcessesUrl, + AuthenticationManager authenticationManager) { + setFilterProcessesUrl(defaultFilterProcessesUrl); + setAuthenticationManager(authenticationManager); + } + + /** + * Creates a new instance with a {@link RequestMatcher} and an {@link AuthenticationManager} + * + * @param requiresAuthenticationRequestMatcher the {@link RequestMatcher} used to determine + * if authentication is required. Cannot be null. + * @param authenticationManager the {@link AuthenticationManager} used to authenticate an {@link Authentication} object. + * Cannot be null. + */ + protected AbstractAuthenticationProcessingFilter(RequestMatcher requiresAuthenticationRequestMatcher, + AuthenticationManager authenticationManager) { + setRequiresAuthenticationRequestMatcher(requiresAuthenticationRequestMatcher); + setAuthenticationManager(authenticationManager); + } + // ~ Methods // ======================================================================================================== @@ -357,6 +394,7 @@ protected AuthenticationManager getAuthenticationManager() { } public void setAuthenticationManager(AuthenticationManager authenticationManager) { + Assert.notNull(authenticationManager, "authenticationManager cannot be null"); this.authenticationManager = authenticationManager; } diff --git a/web/src/main/java/org/springframework/security/web/authentication/UsernamePasswordAuthenticationFilter.java b/web/src/main/java/org/springframework/security/web/authentication/UsernamePasswordAuthenticationFilter.java index 895d0503a91..8af9503c941 100644 --- a/web/src/main/java/org/springframework/security/web/authentication/UsernamePasswordAuthenticationFilter.java +++ b/web/src/main/java/org/springframework/security/web/authentication/UsernamePasswordAuthenticationFilter.java @@ -17,6 +17,7 @@ package org.springframework.security.web.authentication; import org.springframework.lang.Nullable; +import org.springframework.security.authentication.AuthenticationManager; import org.springframework.security.authentication.AuthenticationServiceException; import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; import org.springframework.security.core.Authentication; @@ -63,6 +64,10 @@ public UsernamePasswordAuthenticationFilter() { super(new AntPathRequestMatcher("/login", "POST")); } + public UsernamePasswordAuthenticationFilter(AuthenticationManager authenticationManager) { + super(new AntPathRequestMatcher("/login", "POST"), authenticationManager); + } + // ~ Methods // ======================================================================================================== diff --git a/web/src/test/java/org/springframework/security/web/authentication/UsernamePasswordAuthenticationFilterTests.java b/web/src/test/java/org/springframework/security/web/authentication/UsernamePasswordAuthenticationFilterTests.java index 8fa56640860..6dc762e57d5 100644 --- a/web/src/test/java/org/springframework/security/web/authentication/UsernamePasswordAuthenticationFilterTests.java +++ b/web/src/test/java/org/springframework/security/web/authentication/UsernamePasswordAuthenticationFilterTests.java @@ -57,6 +57,23 @@ public void testNormalOperation() { assertThat(((WebAuthenticationDetails) result.getDetails()).getRemoteAddress()).isEqualTo("127.0.0.1"); } + @Test + public void testConstructorInjectionOfAuthenticationManager() { + MockHttpServletRequest request = new MockHttpServletRequest("POST", "/"); + request.addParameter( + UsernamePasswordAuthenticationFilter.SPRING_SECURITY_FORM_USERNAME_KEY, + "rod"); + request.addParameter( + UsernamePasswordAuthenticationFilter.SPRING_SECURITY_FORM_PASSWORD_KEY, + "dokdo"); + + UsernamePasswordAuthenticationFilter filter = + new UsernamePasswordAuthenticationFilter(createAuthenticationManager()); + + Authentication result = filter.attemptAuthentication(request, new MockHttpServletResponse()); + assertThat(result).isNotNull(); + } + @Test public void testNullPasswordHandledGracefully() { MockHttpServletRequest request = new MockHttpServletRequest("POST", "/"); From 6503898643396c471f4202ae557a70a493414dcc Mon Sep 17 00:00:00 2001 From: Myungwoon Oh Date: Fri, 10 Apr 2020 00:24:21 +0900 Subject: [PATCH 2/2] Add tests for newly added constructors Fixes gh-8309 --- ...bstractAuthenticationProcessingFilter.java | 11 -- .../UsernamePasswordAuthenticationFilter.java | 6 +- ...ctAuthenticationProcessingFilterTests.java | 102 +++++++++++++++++- 3 files changed, 102 insertions(+), 17 deletions(-) diff --git a/web/src/main/java/org/springframework/security/web/authentication/AbstractAuthenticationProcessingFilter.java b/web/src/main/java/org/springframework/security/web/authentication/AbstractAuthenticationProcessingFilter.java index 1331aaa895b..d5910b564fc 100644 --- a/web/src/main/java/org/springframework/security/web/authentication/AbstractAuthenticationProcessingFilter.java +++ b/web/src/main/java/org/springframework/security/web/authentication/AbstractAuthenticationProcessingFilter.java @@ -156,16 +156,6 @@ protected AbstractAuthenticationProcessingFilter( this.requiresAuthenticationRequestMatcher = requiresAuthenticationRequestMatcher; } - /** - * Creates a new instance with an {@link AuthenticationManager} - * - * @param authenticationManager the {@link AuthenticationManager} used to authenticate an {@link Authentication} object. - * Cannot be null. - */ - protected AbstractAuthenticationProcessingFilter(AuthenticationManager authenticationManager) { - setAuthenticationManager(authenticationManager); - } - /** * Creates a new instance with a default filterProcessesUrl and an {@link AuthenticationManager} * @@ -394,7 +384,6 @@ protected AuthenticationManager getAuthenticationManager() { } public void setAuthenticationManager(AuthenticationManager authenticationManager) { - Assert.notNull(authenticationManager, "authenticationManager cannot be null"); this.authenticationManager = authenticationManager; } diff --git a/web/src/main/java/org/springframework/security/web/authentication/UsernamePasswordAuthenticationFilter.java b/web/src/main/java/org/springframework/security/web/authentication/UsernamePasswordAuthenticationFilter.java index 8af9503c941..6a6d89d41ca 100644 --- a/web/src/main/java/org/springframework/security/web/authentication/UsernamePasswordAuthenticationFilter.java +++ b/web/src/main/java/org/springframework/security/web/authentication/UsernamePasswordAuthenticationFilter.java @@ -52,6 +52,8 @@ public class UsernamePasswordAuthenticationFilter extends public static final String SPRING_SECURITY_FORM_USERNAME_KEY = "username"; public static final String SPRING_SECURITY_FORM_PASSWORD_KEY = "password"; + private static final AntPathRequestMatcher DEFAULT_ANT_PATH_REQUEST_MATCHER = + new AntPathRequestMatcher("/login", "POST"); private String usernameParameter = SPRING_SECURITY_FORM_USERNAME_KEY; private String passwordParameter = SPRING_SECURITY_FORM_PASSWORD_KEY; @@ -61,11 +63,11 @@ public class UsernamePasswordAuthenticationFilter extends // =================================================================================================== public UsernamePasswordAuthenticationFilter() { - super(new AntPathRequestMatcher("/login", "POST")); + super(DEFAULT_ANT_PATH_REQUEST_MATCHER); } public UsernamePasswordAuthenticationFilter(AuthenticationManager authenticationManager) { - super(new AntPathRequestMatcher("/login", "POST"), authenticationManager); + super(DEFAULT_ANT_PATH_REQUEST_MATCHER, authenticationManager); } // ~ Methods diff --git a/web/src/test/java/org/springframework/security/web/authentication/AbstractAuthenticationProcessingFilterTests.java b/web/src/test/java/org/springframework/security/web/authentication/AbstractAuthenticationProcessingFilterTests.java index 15623dc27cb..48f821834b8 100644 --- a/web/src/test/java/org/springframework/security/web/authentication/AbstractAuthenticationProcessingFilterTests.java +++ b/web/src/test/java/org/springframework/security/web/authentication/AbstractAuthenticationProcessingFilterTests.java @@ -50,6 +50,8 @@ import org.springframework.security.web.authentication.rememberme.TokenBasedRememberMeServices; import org.springframework.security.web.authentication.session.SessionAuthenticationStrategy; import org.springframework.security.web.firewall.DefaultHttpFirewall; +import org.springframework.security.web.util.matcher.AntPathRequestMatcher; +import org.springframework.security.web.util.matcher.RequestMatcher; import org.springframework.test.util.ReflectionTestUtils; /** @@ -212,6 +214,78 @@ public void testNormalOperationWithDefaultFilterProcessesUrl() throws Exception assertThat(request.getSession()).isEqualTo(sessionPreAuth); } + @Test + public void testNormalOperationWithDefaultFilterProcessesUrlAndAuthenticationManager() throws Exception { + // Setup our HTTP request + MockHttpServletRequest request = createMockAuthenticationRequest(); + HttpSession sessionPreAuth = request.getSession(); + + // Setup our filter configuration + MockFilterConfig config = new MockFilterConfig(null, null); + + // Setup our expectation that the filter chain will not be invoked, as we redirect + // to defaultTargetUrl + MockFilterChain chain = new MockFilterChain(false); + MockHttpServletResponse response = new MockHttpServletResponse(); + + // Setup our test object, to grant access + MockAuthenticationFilter filter = new MockAuthenticationFilter( + "/j_mock_post", mock(AuthenticationManager.class)); + + filter.setSessionAuthenticationStrategy( + mock(SessionAuthenticationStrategy.class)); + filter.setAuthenticationSuccessHandler(successHandler); + filter.setAuthenticationFailureHandler(failureHandler); + filter.afterPropertiesSet(); + + // Test + filter.doFilter(request, response, chain); + assertThat(response.getRedirectedUrl()).isEqualTo("/mycontext/logged_in.jsp"); + assertThat(SecurityContextHolder.getContext().getAuthentication()).isNotNull(); + assertThat( + SecurityContextHolder.getContext().getAuthentication().getPrincipal().toString()).isEqualTo( + "test"); + // Should still have the same session + assertThat(request.getSession()).isEqualTo(sessionPreAuth); + } + + @Test + public void testNormalOperationWithRequestMatcherAndAuthenticationManager() throws Exception { + // Setup our HTTP request + MockHttpServletRequest request = createMockAuthenticationRequest(); + request.setServletPath("/j_eradicate_corona_virus"); + request.setRequestURI("/mycontext/j_eradicate_corona_virus"); + HttpSession sessionPreAuth = request.getSession(); + + // Setup our filter configuration + MockFilterConfig config = new MockFilterConfig(null, null); + + // Setup our expectation that the filter chain will not be invoked, as we redirect + // to defaultTargetUrl + MockFilterChain chain = new MockFilterChain(false); + MockHttpServletResponse response = new MockHttpServletResponse(); + + // Setup our test object, to grant access + MockAuthenticationFilter filter = new MockAuthenticationFilter( + new AntPathRequestMatcher("/j_eradicate_corona_virus"), mock(AuthenticationManager.class)); + + filter.setSessionAuthenticationStrategy( + mock(SessionAuthenticationStrategy.class)); + filter.setAuthenticationSuccessHandler(successHandler); + filter.setAuthenticationFailureHandler(failureHandler); + filter.afterPropertiesSet(); + + // Test + filter.doFilter(request, response, chain); + assertThat(response.getRedirectedUrl()).isEqualTo("/mycontext/logged_in.jsp"); + assertThat(SecurityContextHolder.getContext().getAuthentication()).isNotNull(); + assertThat( + SecurityContextHolder.getContext().getAuthentication().getPrincipal().toString()).isEqualTo( + "test"); + // Should still have the same session + assertThat(request.getSession()).isEqualTo(sessionPreAuth); + } + @Test public void testStartupDetectsInvalidAuthenticationManager() { AbstractAuthenticationProcessingFilter filter = new MockAuthenticationFilter(); @@ -430,20 +504,33 @@ public void setRememberMeServicesShouldntAllowNulls() { private class MockAuthenticationFilter extends AbstractAuthenticationProcessingFilter { + private static final String DEFAULT_FILTER_PROCESSING_URL = "/j_mock_post"; + private AuthenticationException exceptionToThrow; private boolean grantAccess; MockAuthenticationFilter(boolean grantAccess) { this(); - setRememberMeServices(new NullRememberMeServices()); + setupRememberMeServicesAndAuthenticationException(); this.grantAccess = grantAccess; - this.exceptionToThrow = new BadCredentialsException( - "Mock requested to do so"); } private MockAuthenticationFilter() { - super("/j_mock_post"); + super(DEFAULT_FILTER_PROCESSING_URL); + } + + private MockAuthenticationFilter(String defaultFilterProcessingUrl, AuthenticationManager authenticationManager) { + super(defaultFilterProcessingUrl, authenticationManager); + setupRememberMeServicesAndAuthenticationException(); + this.grantAccess = true; + } + + private MockAuthenticationFilter(RequestMatcher requiresAuthenticationRequestMatcher, + AuthenticationManager authenticationManager) { + super(requiresAuthenticationRequestMatcher, authenticationManager); + setupRememberMeServicesAndAuthenticationException(); + this.grantAccess = true; } public Authentication attemptAuthentication(HttpServletRequest request, @@ -456,6 +543,13 @@ public Authentication attemptAuthentication(HttpServletRequest request, throw exceptionToThrow; } } + + private void setupRememberMeServicesAndAuthenticationException() { + setRememberMeServices(new NullRememberMeServices()); + this.exceptionToThrow = new BadCredentialsException( + "Mock requested to do so"); + } + } private class MockFilterChain implements FilterChain {