Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default RequestPostProcessor should be executed first [SPR-12945] #17538

Closed
spring-issuemaster opened this issue Apr 22, 2015 · 4 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

commented Apr 22, 2015

Rob Winch opened SPR-12945 and commented

If a RequestPostProcessor is used with the default request, it is placed after additional RequestPostProcessors. For example, the following test fails:

@Before
public void setup() {
	MockHttpServletRequestBuilder defaultRequest = get("/")
			.with(requestAttr("ATTR").value("default"));
	mvc = MockMvcBuilders.webAppContextSetup(context)
		.defaultRequest(defaultRequest)
		.build();
}

@Test
public void defaultRequestPostProcessorsShouldExecuteBeforeAdditionalRequestPostProcessors() throws Exception {
	MockHttpServletRequestBuilder request = get("/abc")
			.with(requestAttr("ATTR").value("override"));

	mvc.perform(request)
		.andExpect(request().attribute("ATTR", "override"));
}

private static RequestAttributePostProcessor requestAttr(String name) {
	return new RequestAttributePostProcessor().attr(name);
}

public class RequestAttributePostProcessor implements RequestPostProcessor {

	String attr;

	String value;

	public RequestAttributePostProcessor attr(String attr) {
		this.attr = attr;
		return this;
	}

	public RequestAttributePostProcessor value(String value) {
		this.value = value;
		return this;
	}

	public MockHttpServletRequest postProcessRequest(MockHttpServletRequest request) {
		request.setAttribute(attr, value);
		return request;
	}

}

A default RequestPostProcessor should happen first so that additional RequestPostProcessors override additional RequestPostProcessors.

The problem appears to be in MockHttpServletRequestBuilder.merge which adds the parent to the end instead of the front of the postProcessors.


Affects: 4.1.6

Issue Links:

  • SEC-2941 Default RequestPostProcessor overrides additional DefaultRequestPostProcessor ("is depended on by")

Referenced from: pull request #782, and commits spring-projects/spring-security@dd09243, spring-projects/spring-security@269127c

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 22, 2015

Rob Winch commented

I have put together a PR at #782

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 24, 2015

Rossen Stoyanchev commented

Thanks for the PR. Looking at MockHttpServletRequestBuilder.merge the idea is that it

Merges the properties of the "parent" RequestBuilder accepting values only if not already set in "this" instance.

So in that same vein, a RequestPostProcessor could b expected to check if an attribute is already present. I do see however that when simply looking at it from the perspective of using a default RequestBuilder and then an actual one, it's more intuitive to think in terms of overriding and not merge semantics. So I'll accept the PR, hopefully it won't cause regressions.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 24, 2015

Rob Winch commented

Thanks Rossen Stoyanchev

One thing to point out is that merging is difficult from the perspective of not knowing what context the RequestPostProcessor is being used. For example:

@Test
public void multiplePostProcessorOverride() throws Exception {
	MockHttpServletRequestBuilder request = get("/abc")
                        .with(requestAttr("ATTR").value("first"))
			.with(requestAttr("ATTR").value("override"));

	mvc.perform(request)
		.andExpect(request().attribute("ATTR", "override"));
}

If the RequestPostProcessor was modified to "merge" behavior to fix the default RequestPostProcessor instance as shown in the original description (only set the attribute if it wasn't already set), it would break the scenario above.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 24, 2015

Rossen Stoyanchev commented

Good point. There is the merging of two RequestBuilder's but each has a list of RequestPostProcessor's already that need to be applied in the specified order. So it's really the RequestPostProcessor contract that matters and the merge then becomes applying the default ones first and then overlaying the actual ones. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.