SecurityMockMvcConfigurer Honors Filter Order

Fixes gh-7265

Author: rwinch

test/spring-security-test.gradle

@@ -12,6 +12,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'

test/src/main/java/org/springframework/security/test/web/servlet/setup/SecurityMockMvcConfigurer.java

@@ -16,13 +16,20 @@
 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;
 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,43 +41,115 @@
  * @since 4.0
  */
 final class SecurityMockMvcConfigurer extends MockMvcConfigurerAdapter {
-	private Filter springSecurityFilterChain;
+	private final DelegateFilter delegateFilter;
 
 	/**
 	 * Creates a new instance
 	 */
 	SecurityMockMvcConfigurer() {
+		this.delegateFilter = new DelegateFilter();
 	}
 
 	/**
 	 * Creates a new instance with the provided {@link javax.servlet.Filter}
 	 * @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();
+		}
+	}
 }

test/src/test/java/org/springframework/security/test/web/servlet/setup/SecurityMockMvcConfigurerTests.java

@@ -15,21 +15,24 @@
  */
 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 static org.mockito.Matchers.anyString;
-import static org.mockito.Matchers.eq;
+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;
 import static org.mockito.Mockito.when;
 
@@ -56,9 +59,10 @@ public void beforeMockMvcCreatedOverrideBean() throws Exception {
 		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 void beforeMockMvcCreatedBean() throws Exception {
 		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<SecurityMockMvcConfigurer.DelegateFilter> 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)))

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 {}
+}