From 0c6bff4afbfbc362da619395df5c010e5826fe76 Mon Sep 17 00:00:00 2001 From: Rob Winch Date: Fri, 16 Aug 2019 06:43:39 -0500 Subject: [PATCH] SecurityMockMvcConfigurer Honors Filter Order Fixes gh-7265 --- test/spring-security-test.gradle | 1 + .../setup/SecurityMockMvcConfigurer.java | 95 +++++++++++++++++-- .../setup/SecurityMockMvcConfigurerTests.java | 28 ++++-- .../SecurityMockMvcConfigurersTests.java | 87 +++++++++++++++++ 4 files changed, 196 insertions(+), 15 deletions(-) create mode 100644 test/src/test/java/org/springframework/security/test/web/servlet/setup/SecurityMockMvcConfigurersTests.java diff --git a/test/spring-security-test.gradle b/test/spring-security-test.gradle index 5f77d4ab63..9fbf195f7d 100644 --- a/test/spring-security-test.gradle +++ b/test/spring-security-test.gradle @@ -14,6 +14,7 @@ dependencies { provided 'javax.servlet:javax.servlet-api' + testCompile project(path : ':spring-security-config', configuration : 'tests') testCompile 'com.fasterxml.jackson.core:jackson-databind' testCompile 'io.projectreactor:reactor-test' testCompile 'javax.xml.bind:jaxb-api' diff --git a/test/src/main/java/org/springframework/security/test/web/servlet/setup/SecurityMockMvcConfigurer.java b/test/src/main/java/org/springframework/security/test/web/servlet/setup/SecurityMockMvcConfigurer.java index 909a92fee9..f654fcd755 100644 --- a/test/src/main/java/org/springframework/security/test/web/servlet/setup/SecurityMockMvcConfigurer.java +++ b/test/src/main/java/org/springframework/security/test/web/servlet/setup/SecurityMockMvcConfigurer.java @@ -16,6 +16,11 @@ package org.springframework.security.test.web.servlet.setup; import javax.servlet.Filter; +import javax.servlet.FilterChain; +import javax.servlet.FilterConfig; +import javax.servlet.ServletException; +import javax.servlet.ServletRequest; +import javax.servlet.ServletResponse; import org.springframework.security.config.BeanIds; import org.springframework.test.web.servlet.request.RequestPostProcessor; @@ -23,6 +28,8 @@ import org.springframework.test.web.servlet.setup.ConfigurableMockMvcBuilder; import org.springframework.test.web.servlet.setup.MockMvcConfigurerAdapter; import org.springframework.web.context.WebApplicationContext; +import java.io.IOException; + import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.testSecurityContext; /** @@ -34,12 +41,13 @@ import static org.springframework.security.test.web.servlet.request.SecurityMock * @since 4.0 */ final class SecurityMockMvcConfigurer extends MockMvcConfigurerAdapter { - private Filter springSecurityFilterChain; + private final DelegateFilter delegateFilter; /** * Creates a new instance */ SecurityMockMvcConfigurer() { + this.delegateFilter = new DelegateFilter(); } /** @@ -47,30 +55,101 @@ final class SecurityMockMvcConfigurer extends MockMvcConfigurerAdapter { * @param springSecurityFilterChain the {@link javax.servlet.Filter} to use */ SecurityMockMvcConfigurer(Filter springSecurityFilterChain) { - this.springSecurityFilterChain = springSecurityFilterChain; + this.delegateFilter = new DelegateFilter(springSecurityFilterChain); + } + + @Override + public void afterConfigurerAdded(ConfigurableMockMvcBuilder builder) { + builder.addFilters(this.delegateFilter); } @Override public RequestPostProcessor beforeMockMvcCreated( ConfigurableMockMvcBuilder builder, WebApplicationContext context) { String securityBeanId = BeanIds.SPRING_SECURITY_FILTER_CHAIN; - if (this.springSecurityFilterChain == null + if (getSpringSecurityFilterChain() == null && context.containsBean(securityBeanId)) { - this.springSecurityFilterChain = context.getBean(securityBeanId, - Filter.class); + setSpringSecurityFitlerChain(context.getBean(securityBeanId, + Filter.class)); } - if (this.springSecurityFilterChain == null) { + if (getSpringSecurityFilterChain() == null) { throw new IllegalStateException( "springSecurityFilterChain cannot be null. Ensure a Bean with the name " + securityBeanId + " implementing Filter is present or inject the Filter to be used."); } - builder.addFilters(this.springSecurityFilterChain); + // This is used by other test support to obtain the FilterChainProxy context.getServletContext().setAttribute(BeanIds.SPRING_SECURITY_FILTER_CHAIN, - this.springSecurityFilterChain); + getSpringSecurityFilterChain()); return testSecurityContext(); } + + private void setSpringSecurityFitlerChain(Filter filter) { + this.delegateFilter.setDelegate(filter); + } + + private Filter getSpringSecurityFilterChain() { + return this.delegateFilter.delegate; + } + + /** + * Allows adding in {@link #afterConfigurerAdded(ConfigurableMockMvcBuilder)} to preserve Filter order and then + * lazily set the delegate in {@link #beforeMockMvcCreated(ConfigurableMockMvcBuilder, WebApplicationContext)}. + * + * {@link org.springframework.web.filter.DelegatingFilterProxy} is not used because it is not easy to lazily set + * the delegate or get the delegate which is necessary for the test infrastructure. + */ + static class DelegateFilter implements Filter { + + private Filter delegate; + + DelegateFilter() { + } + + DelegateFilter(Filter delegate) { + this.delegate = delegate; + } + + void setDelegate(Filter delegate) { + this.delegate = delegate; + } + + Filter getDelegate() { + return this.delegate; + } + + @Override + public void init(FilterConfig filterConfig) throws ServletException { + this.delegate.init(filterConfig); + } + + @Override + public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) + throws IOException, ServletException { + this.delegate.doFilter(request, response, chain); + } + + @Override + public void destroy() { + this.delegate.destroy(); + } + + @Override + public int hashCode() { + return this.delegate.hashCode(); + } + + @Override + public boolean equals(Object obj) { + return this.delegate.equals(obj); + } + + @Override + public String toString() { + return this.delegate.toString(); + } + } } diff --git a/test/src/test/java/org/springframework/security/test/web/servlet/setup/SecurityMockMvcConfigurerTests.java b/test/src/test/java/org/springframework/security/test/web/servlet/setup/SecurityMockMvcConfigurerTests.java index 65cae6355d..eb4c41567f 100644 --- a/test/src/test/java/org/springframework/security/test/web/servlet/setup/SecurityMockMvcConfigurerTests.java +++ b/test/src/test/java/org/springframework/security/test/web/servlet/setup/SecurityMockMvcConfigurerTests.java @@ -15,19 +15,22 @@ */ package org.springframework.security.test.web.servlet.setup; -import javax.servlet.Filter; -import javax.servlet.ServletContext; - import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.junit.MockitoJUnitRunner; - import org.springframework.security.config.BeanIds; import org.springframework.test.web.servlet.setup.ConfigurableMockMvcBuilder; import org.springframework.web.context.WebApplicationContext; +import javax.servlet.Filter; +import javax.servlet.ServletContext; +import javax.servlet.ServletException; +import java.io.IOException; + +import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.verify; @@ -56,9 +59,10 @@ public class SecurityMockMvcConfigurerTests { returnFilterBean(); SecurityMockMvcConfigurer configurer = new SecurityMockMvcConfigurer(this.filter); + configurer.afterConfigurerAdded(this.builder); configurer.beforeMockMvcCreated(this.builder, this.context); - verify(this.builder).addFilters(this.filter); + assertFilterAdded(this.filter); verify(this.servletContext).setAttribute(BeanIds.SPRING_SECURITY_FILTER_CHAIN, this.filter); } @@ -68,27 +72,37 @@ public class SecurityMockMvcConfigurerTests { returnFilterBean(); SecurityMockMvcConfigurer configurer = new SecurityMockMvcConfigurer(); + configurer.afterConfigurerAdded(this.builder); configurer.beforeMockMvcCreated(this.builder, this.context); - verify(this.builder).addFilters(this.beanFilter); + assertFilterAdded(this.beanFilter); } @Test public void beforeMockMvcCreatedNoBean() throws Exception { SecurityMockMvcConfigurer configurer = new SecurityMockMvcConfigurer(this.filter); + configurer.afterConfigurerAdded(this.builder); configurer.beforeMockMvcCreated(this.builder, this.context); - verify(this.builder).addFilters(this.filter); + assertFilterAdded(this.filter); } @Test(expected = IllegalStateException.class) public void beforeMockMvcCreatedNoFilter() throws Exception { SecurityMockMvcConfigurer configurer = new SecurityMockMvcConfigurer(); + configurer.afterConfigurerAdded(this.builder); configurer.beforeMockMvcCreated(this.builder, this.context); } + private void assertFilterAdded(Filter filter) throws IOException, ServletException { + ArgumentCaptor filterArg = ArgumentCaptor.forClass( + SecurityMockMvcConfigurer.DelegateFilter.class); + verify(this.builder).addFilters(filterArg.capture()); + assertThat(filterArg.getValue().getDelegate()).isEqualTo(filter); + } + private void returnFilterBean() { when(this.context.containsBean(anyString())).thenReturn(true); when(this.context.getBean(anyString(), eq(Filter.class))) diff --git a/test/src/test/java/org/springframework/security/test/web/servlet/setup/SecurityMockMvcConfigurersTests.java b/test/src/test/java/org/springframework/security/test/web/servlet/setup/SecurityMockMvcConfigurersTests.java new file mode 100644 index 0000000000..162114d6ee --- /dev/null +++ b/test/src/test/java/org/springframework/security/test/web/servlet/setup/SecurityMockMvcConfigurersTests.java @@ -0,0 +1,87 @@ +/* + * Copyright 2002-2019 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.security.test.web.servlet.setup; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.context.annotation.Configuration; +import org.springframework.context.annotation.Import; +import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity; +import org.springframework.security.config.users.AuthenticationTestConfiguration; +import org.springframework.test.context.junit4.SpringRunner; +import org.springframework.test.context.web.WebAppConfiguration; +import org.springframework.test.web.servlet.MockMvc; +import org.springframework.test.web.servlet.setup.MockMvcBuilders; +import org.springframework.web.context.WebApplicationContext; +import org.springframework.web.servlet.config.annotation.EnableWebMvc; + +import javax.servlet.Filter; + +import static org.mockito.Mockito.mock; +import static org.springframework.security.test.web.servlet.setup.SecurityMockMvcConfigurers.springSecurity; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +/** + * @author Rob Winch + */ +@RunWith(SpringRunner.class) +@WebAppConfiguration +public class SecurityMockMvcConfigurersTests { + @Autowired + WebApplicationContext wac; + + Filter noOpFilter = mock(Filter.class); + + /** + * Since noOpFilter is first does not continue the chain, security will not be invoked and the status should be OK + * + * @throws Exception + */ + @Test + public void applySpringSecurityWhenAddFilterFirstThenFilterFirst() throws Exception { + MockMvc mockMvc = MockMvcBuilders.webAppContextSetup(this.wac) + .addFilters(this.noOpFilter) + .apply(springSecurity()) + .build(); + + mockMvc.perform(get("/")) + .andExpect(status().isOk()); + } + + /** + * Since noOpFilter is second security will be invoked and the status will be not OK. We know this because if noOpFilter + * were first security would not be invoked sincet noOpFilter does not continue the FilterChain + * @throws Exception + */ + @Test + public void applySpringSecurityWhenAddFilterSecondThenSecurityFirst() throws Exception { + MockMvc mockMvc = MockMvcBuilders.webAppContextSetup(this.wac) + .apply(springSecurity()) + .addFilters(this.noOpFilter) + .build(); + + mockMvc.perform(get("/")) + .andExpect(status().is4xxClientError()); + } + + @Configuration + @EnableWebMvc + @EnableWebSecurity + @Import(AuthenticationTestConfiguration.class) + static class Config {} +}