Spring-security: SecurityMockMvcConfigurer regression in Spring Boot 2.2.1

[JohnWu-Pro]

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());
    }

}

[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?

[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.

[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);

[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();
        }
    }

[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.

[Daniel-Jacob]

can I pick this up?

[JohnWu-Pro]

Sure. Feel free submit your PR.

[jzheaux]

@Daniel-Jacob thanks for your interest! It's yours.

[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.

[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.

[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());

[jzheaux]

@Daniel-Jacob is this still something that you'd like to complete?

[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).

[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.

[rwinch]

Thanks @dadikovi I think it is still needed since we the NullPointerException can still occur (as demonstrated above)

[rwinch]

Closing in favor of gh-8451