Summary
SecurityMockMvcConfigurer$DelegateFilter is not null-safe, if no springSecurityFilterChain is available.
Actual Behavior
java.lang.NullPointerException
at org.springframework.security.test.web.servlet.setup.SecurityMockMvcConfigurer$DelegateFilter.doFilter(SecurityMockMvcConfigurer.java:132)
at org.springframework.mock.web.MockFilterChain.doFilter(MockFilterChain.java:134)
at org.springframework.test.web.servlet.MockMvc.perform(MockMvc.java:183)
at io.restassured.module.mockmvc.internal.MockMvcRequestSenderImpl.performRequest(MockMvcRequestSenderImpl.java:219)
at io.restassured.module.mockmvc.internal.MockMvcRequestSenderImpl.sendRequest(MockMvcRequestSenderImpl.java:448)
at io.restassured.module.mockmvc.internal.MockMvcRequestSenderImpl.put(MockMvcRequestSenderImpl.java:505)
at io.restassured.module.mockmvc.internal.MockMvcRequestSenderImpl.put(MockMvcRequestSenderImpl.java:101)
at com.example.demo.rest.DemoResourceTest.testEcho(DemoResourceTest.java:36)
Expected Behavior
No exception should be thrown even when no springSecurityFilterChain is available.
Configuration
Our application uses spring security, and org.springframework.security.test.web.servlet.setup.SecurityMockMvcConfigurers is also in the test classpath. In a REST endpoint test (Mockito+RestAssured based, not full integration test, and security is not of interest), springSecurityFilterChain is not available.
Version
The issue exists in spring-security-test 2.2.x. In spring-security-test 2.0.x, it works fine.
Related to issue https://github.com/spring-projects/spring-security/issues/7688
Sample
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>
<parent>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-parent</artifactId>
<version>2.2.2.RELEASE</version>
<relativePath/> <!-- lookup parent from repository -->
</parent>
<groupId>com.example</groupId>
<artifactId>security-mock-mvc-config-regression</artifactId>
<version>0.0.1-SNAPSHOT</version>
<name>Sample for SecurityMockMvcConfigurer regression</name>
<properties>
<java.version>1.8</java.version>
</properties>
<dependencies>
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-security</artifactId>
</dependency>
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-web</artifactId>
</dependency>
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-test</artifactId>
<scope>test</scope>
<exclusions>
<exclusion>
<groupId>org.junit.vintage</groupId>
<artifactId>junit-vintage-engine</artifactId>
</exclusion>
</exclusions>
</dependency>
<dependency>
<groupId>org.springframework.security</groupId>
<artifactId>spring-security-test</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>io.rest-assured</groupId>
<artifactId>spring-mock-mvc</artifactId>
<scope>test</scope>
</dependency>
</dependencies>
<build>
<plugins>
<plugin>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-maven-plugin</artifactId>
</plugin>
</plugins>
</build>
</project>
package com.example.demo.rest;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.PutMapping;
import org.springframework.web.bind.annotation.RequestBody;
import org.springframework.web.bind.annotation.RestController;
@RestController
public class DemoResource {
@PutMapping(value = "/echo")
public ResponseEntity<String> echo(@RequestBody String message) {
return ResponseEntity.ok(message.toLowerCase());
}
}
package com.example.demo.rest;
import static io.restassured.module.mockmvc.RestAssuredMockMvc.given;
import static org.assertj.core.api.Assertions.assertThat;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.mockito.InjectMocks;
import org.mockito.MockitoAnnotations;
import org.springframework.http.HttpStatus;
import io.restassured.module.mockmvc.RestAssuredMockMvc;
public class DemoResourceTest {
@InjectMocks
private DemoResource target;
@BeforeEach
public void beforeEach() {
MockitoAnnotations.initMocks(this);
RestAssuredMockMvc.standaloneSetup(target);
}
@Test
public void testEcho() {
String input = "TEST";
String message = given()
.body(input)
.when()
.put("/echo")
.then()
.statusCode(HttpStatus.OK.value())
.extract()
.response().asString();
assertThat(message).isEqualTo(input.toLowerCase());
}
}
Comment From: jzheaux
Have you already tried the RestAssured support for turning off security?
For example. you could do something like:
RestAssuredMockMvcConfig noSecurity() {
return config().mockMvcConfig(mockMvcConfig()
.dontAutomaticallyApplySpringSecurityMockMvcConfigurer());
}
And then in your test doing:
given().config(noSecurity())
.body(...
The RestAssured folks may have a different opinion on the preferred way to shut off security for a request.
I prefer the above solution since Spring Security's test support probably shouldn't be added at all if Spring Security isn't configured. The SecurityMockMvcConfigurer#beforeMockMvcCreated implementation is written with this in mind.
However, you could also consider doing:
RestAssuredMockMvc.standaloneSetup(target,
springSecurity((request, response, chain) -> chain.doFilter(request, response)));
Which would replace the spring security filter chain for all tests in the class.
Would one of these address your concern?
Comment From: JohnWu-Pro
I worked out a workaround while I was filing the issue. And, both of your suggestions work as well.
On the other side, SecurityMockMvcConfigurer is still not null-safe, from the coding style point of view.
Comment From: jzheaux
The way null-safety is achieved in Spring Security is typically to throw an exception when a null is not permissible. So, while I agree SecurityMockMvcConfigurer#DelegateFilter could be polished, your particular outcome would be the same.
I agree that it would be better for DelegateFilter to be null-safe. Would you be interested in submitting a PR that did something like the following:
+ Assert.state(this.delegate != null,
+ "delegate cannot be null. Ensure a Bean with the name "
+ + BeanIds.SPRING_SECURITY_FILTER_CHAIN
+ + " implementing Filter is present or inject the Filter to be used.");
this.delegate.doFilter(request, response, chain);
Comment From: JohnWu-Pro
I tried to submit a PR but somehow I couldn't push my changes to github and I don't have time to figure out why at this moment :-P.
So, here is the updated org.springframework.security.test.web.servlet.setup.SecurityMockMvcConfigurer.DelegateFilter, to make it more permissible.
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 {
if (this.delegate != null) {
this.delegate.init(filterConfig);
}
}
@Override
public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain)
throws IOException, ServletException {
if (this.delegate != null) {
this.delegate.doFilter(request, response, chain);
} else {
chain.doFilter(request, response);
}
}
@Override
public void destroy() {
if (this.delegate != null) {
this.delegate.destroy();
}
}
@Override
public int hashCode() {
return this.delegate == null ? super.hashCode() : this.delegate.hashCode();
}
@Override
public boolean equals(Object obj) {
return this.delegate == null ? super.equals(obj) : this.delegate.equals(obj);
}
@Override
public String toString() {
return this.delegate == null ? super.toString() : this.delegate.toString();
}
}
Comment From: jzheaux
@JohnWu-Pro, sorry to hear you are having trouble with GitHub, and thank you for the code snippet.
I don't think this is the route that we want to go, though.
Basically instead of this:
if (delegate != null) {
doFilter();
}
We should do:
if (delegate == null) {
throw new IllegalStateException("delegate cannot be null - please make sure springSecurityFilterChain is wired")
}
doFilter();
This lines up more with Spring Security's fundamental design principle of erroring when something is misconfigured (e.g. trying to use Spring Security's test support without setting up Spring Security is a misconfiguration).
For the time being, I'll open this one up for contribution. If you are still interested in doing a PR let me know; otherwise, we'll await the interest of another community member.
Comment From: Daniel-Jacob
can I pick this up?
Comment From: JohnWu-Pro
Sure. Feel free submit your PR.
Comment From: jzheaux
@Daniel-Jacob thanks for your interest! It's yours.
Comment From: wabrit
I'm having trouble using the following suggestion (I can't use the dontAutomaticallyApplySpringSecurityMockMvcConfigurer suggestion because this is in the context of Spring Cloud CDC and the actual test code is generated from groovy contracts):
RestAssuredMockMvc.standaloneSetup(target,
springSecurity((request, response, chain) -> chain.doFilter(request, response)));
As I then get an error to the effect that it can't find a method getFilters() on my lambda expression (which is being called by reflection from WebTestUtils.findFilter()).
If I define that method, I'm not sure what filters to actually return from it; if I return an empty list, my controllers that I passed to RestAssuredMockMvc.standaloneSetup are never called.
Comment From: jzheaux
@wabrit thanks for the additional detail. It's probably best to reach out to RestAssured to find how to turn the feature off instead of turning the Spring Security filter chain into a pass-through. My earlier suggestions above are just my informal observations and may not work in all cases.
That said, based on the error in WebTestUtils, I think the following would be a bit more robust:
SecurityFilterChain security = new DefaultSecurityFilterChain(
AnyRequestMatcher.INSTANCE,
(request, response, chain) -> chain.doFilter(request, response));
RestAssuredMockMvc.standaloneSetup(target,
springSecurity(new FilterChainProxy(security)));
DefaultSecurityFilterChain#getFilters returns the filters specified in the constructor. The one specified here in the constructor simply passes the request to the rest of the servlet filter chain.
Comment From: wabrit
Thanks @jzheaux - I figured out eventually the way to set up a global config which worked for me in the CDC case:
RestAssuredMockMvc.config = new RestAssuredMockMvcConfig().mockMvcConfig(
mockMvcConfig().dontAutomaticallyApplySpringSecurityMockMvcConfigurer());
Comment From: jzheaux
@Daniel-Jacob is this still something that you'd like to complete?
Comment From: rwinch
I agree with @jzheaux comment that it should be something like https://github.com/spring-projects/spring-security/issues/7745#issuecomment-568045369 For the error message we need to remember that the springSecurityFilter can also be injected (it might not be a bean lookup).
Comment From: dadikovi
Hello guys! I created a PR for the original problem (DelegateFilter is not null-safe). However, I found out that in org.springframework.security.test.web.servlet.setup.SecurityMockMvcConfigurer#beforeMockMvcCreated a very similiar IllegalStateException is already thrown if the delegate could not be set (because there is no security filter chain). If you think this PR is still needed, let me know if I have still something to do with it.
Comment From: rwinch
Thanks @dadikovi I think it is still needed since we the NullPointerException can still occur (as demonstrated above)
Comment From: rwinch
Closing in favor of gh-8451