formLogin() does not work with REST Docs

[ValtteriL]

Summary

I was asked to open this issue here after Spring REST Docs team investigated my issue why REST Docs did not get configured with formLogin().

SecurityMockMvcRequestBuilders$FormLoginRequestBuilder does not implement Mergeable so the default configuration that is set up on the MockMvc instance is not applied. Crucially in the case of Spring REST Docs, this means that the ConfigurerApplyingRequestPostProcessor that applies the REST Docs context to the request is lost.

The original issue can be found here: spring-projects/spring-restdocs#655

The following code causes java.lang.IllegalStateException: REST Docs configuration not found. Did you forget to apply a MockMvcRestDocumentationConfigurer when building the MockMvc instance?

@RunWith(SpringRunner.class)
@SpringBootTest
@AutoConfigureMockMvc
@AutoConfigureRestDocs
public class LoginLogoutTest {

    @Autowired
    private MockMvc mockMvc;

    @Test
    public void adminCanLoginLogout() throws Exception {
        mockMvc.perform(formLogin().user(TestConfig.ADMIN_USERNAME).password(TestConfig.PASSWORD))
            .andExpect(status().isOk())
            .andExpect(authenticated().withUsername(TestConfig.ADMIN_USERNAME))
            .andDo(document("login"));

        mockMvc.perform(logout())
            .andExpect(status().isOk())
            .andExpect(unauthenticated())
            .andDo(document("logout"));
    }
}

Actual Behavior

On mvn test the code causes java.lang.IllegalStateException: REST Docs configuration not found. Did you forget to apply a MockMvcRestDocumentationConfigurer when building the MockMvc instance? on the line with document("login").

Expected Behavior

No errors. The REST Docs should be correctly configured.

Version

All versions

Sample

restdocs-formlogin.zip
This sample causes the same error on mvn test.

[rhamedy]

Out of curiosity and the following comment

SecurityMockMvcRequestBuilders$FormLoginRequestBuilder does not implement Mergeable so the default configuration that is set up on the MockMvc instance is not applied. Crucially in the case of Spring REST Docs, this means that the ConfigurerApplyingRequestPostProcessor that applies the REST Docs context to the request is lost.

I decided to give this a try. I updated the SecurityMockMvcRequestBuilders$FormLoginRequestBuilder to implement the Mergeable interface. I wrote a test in SecurityMockMvcRequestBuildersFormLoginTests and the test seems to work as expected and perform the merge ✅

@Test
public void mergeWorksAsExpected() throws Exception {
	String username = "userA";
	String password = "passwordA";
	String loginUrl = "/signin";

	SecurityMockMvcRequestBuilders.FormLoginRequestBuilder builder = formLogin()
			.user(" ")
			.password(" ")
			.loginProcessingUrl(" ");

	SecurityMockMvcRequestBuilders.FormLoginRequestBuilder defaultBuilder = formLogin()
			.loginProcessingUrl(loginUrl)
			.user(username)
			.password(password);

	builder.merge(defaultBuilder);

	MockHttpServletRequest request = builder.buildRequest(servletContext);

	assertThat(request.getParameter("username")).isEqualTo(username);
	assertThat(request.getParameter("password")).isEqualTo(password);
	assertThat(request.getPathInfo()).isEqualTo(loginUrl);

}

I then exported the spring-security-test project as jar from eclipse and installed it in the provided sample project by ValtteriL using a local maven-repository. I run the tests but, still throwing the same exception

java.lang.IllegalArgumentException: Cannot merge with [org.springframework.test.web.servlet.request.MockHttpServletRequestBuilder]
	at com.service.backend.login.LoginLogoutTest.adminCanLoginLogout(LoginLogoutTest.java:33)

Curious why the merge method is not invoked 🤔 Either more changes in addition to merge and is merge enabled is required or I did not correctly configure the sample project to use the updated spring-security-test jar.

[sandmannn]

Looks like there are two additional components to this change, one in spring-framework and another in spring-security.

Firstly, we need to read the post processors from the parent during the merge() call and save them in the instance of the FormLoginRequestBuilder. We need to create an additional interface that has the capability to provide its internally stored postProcessorts via getPostProcessors() method. I am not sure what a good name can be here, e.g. MergeableParentSmartRequestBuilder. There is already an interface ConfigurableSmartRequestBuilder in org.springframework.test.web.servlet.request that allows adding to a class an additional post processor using the with() method; we want to have an additional interface to get all the stored post processors.
MockHttpServletRequestBuilder should implement this interface. In the newly created merge() call of the FormLoginRequestBuilder we need to check if the parent argument of the merge() call belongs to this new interface, retreive the postProcessors using the getter function from the parent (MockHttpServletRequestBuilder will be the most important implementor of the interface) and save them in the FormLoginRequestBuilder.

Secondly, we need to implement the SmartRequestBuilder in the FormLoginRequestBuilder. This allows executing the postProcessRequest() method which is the way for the required documentation-related settings to be passed along.

Thus it looks like there are prerequisites that must be applied in the spring-framework repository first https://github.com/spring-projects/spring-framework/blob/master/spring-test/src/main/java/org/springframework/test/web/servlet/request/MockHttpServletRequestBuilder.java before proceeding with this issue (both the new interface and implementation of this interface in the MockHttpServletRequestBuilder. Adding the contributors to relevant classes @rstoyanchev and @rwinch , please comment if we should proceed in this way.

[dadikovi]

Hi! Is there any update about this issue? Is there a corresponding issue in the spring-framework repository as well? Is this issue still considered ideal for contribution, because if yes I would like to take it :)

[rwinch]

@dadikovi The issue is yours.

NOTE: I'm not aware of any corresponding issues logged in Spring Framework.

[dadikovi]

@rwinch but the solution defined in #7572 (comment) seems legit?

[rwinch]

@dadikovi I haven't looked into the proposed solution. What might be a good place to start is producing a unit test that reproduces the issue so I can easily look at it in isolation. You could do this with a draft PR. What do you think?

[dadikovi]

@rwinch Thank you for your answer, I will do that! :)

[dadikovi]

@rwinch I could manage to understand the problem here, @sandmannn 's comment #7572 (comment) seems 100% accurate for me, but since the solution would need contribution in another spring project (which I would do happily) and I'm new here, I would appreciate your double-check.
I will provide further details in commit under the PR.

[rwinch]

@dadikovi I don't think anything needs to be done outside of Spring Security. I believe if these changes are made to Spring Security's FormLoginRequestBuilder everything work work correctly:

private Mergeable mergeable;

@Override
public MockHttpServletRequest buildRequest(ServletContext servletContext) {
	RequestBuilder builder = post(this.loginProcessingUrl)
			.accept(this.acceptMediaType).param(this.usernameParam, this.username)
			.param(this.passwordParam, this.password);
	if (this.mergeable != null) {
		builder = (RequestBuilder) this.mergeable.merge(builder);
	}
	MockHttpServletRequest request = builder
			.buildRequest(servletContext);
	return this.postProcessor.postProcessRequest(request);
}

@Override
public Object merge(Object o) {
	this.mergeable = (Mergeable) o;
	return this.mergeable;
}

Similar changes would need to be applied to LogoutRequestBuilder. Would you be interested in trying to verify this would work and if so putting a PR together based on this work?

[dadikovi]

@rwinch wow, it works! Thanks!
My suspicion is, that this expects that the merge method of parent will copy all of the headers, params and the URL from the FormLoginRequestBuilder instance.
Probably I'm overthinking it, but I found some places (eg. in MockMultipartHttpServletRequestBuilder) a check if the parent is instance of MockHttpServletRequestBuilder (this implementation does the necessary copies), and an IllegalArgumentException throw otherwise. Should we do this?
Anyway, I will send these changes to the PR, also, I added the same changes to logout as well.
If anything more (or less) needed for this PR, please let me know :)