Skip to content

Commit d3a451f

Browse files
marcusdacoregiomarcusdacoregio
committed
Fix mvcMatchers overriding previous paths
Closes gh-10956
1 parent 9d37810 commit d3a451f

File tree

4 files changed

+300
-8
lines changed

4 files changed

+300
-8
lines changed

‎config/src/main/java/org/springframework/security/config/annotation/web/builders/HttpSecurity.java‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3283,20 +3283,26 @@ private <C extends SecurityConfigurerAdapter<DefaultSecurityFilterChain, HttpSec
32833283
*/
32843284
public final class MvcMatchersRequestMatcherConfigurer extends RequestMatcherConfigurer {
32853285

3286+
private final List<MvcRequestMatcher> mvcMatchers;
3287+
32863288
/**
32873289
* Creates a new instance
32883290
* @param context the {@link ApplicationContext} to use
3289-
* @param matchers the {@link MvcRequestMatcher} instances to set the servlet path
3290-
* on if {@link #servletPath(String)} is set.
3291+
* @param mvcMatchers the {@link MvcRequestMatcher} instances to set the servlet
3292+
* path on if {@link #servletPath(String)} is set.
3293+
* @param allMatchers the {@link RequestMatcher} instances to continue the
3294+
* configuration
32913295
*/
3292-
private MvcMatchersRequestMatcherConfigurer(ApplicationContext context, List<MvcRequestMatcher> matchers) {
3296+
private MvcMatchersRequestMatcherConfigurer(ApplicationContext context, List<MvcRequestMatcher> mvcMatchers,
3297+
List<RequestMatcher> allMatchers) {
32933298
super(context);
3294-
this.matchers = new ArrayList<>(matchers);
3299+
this.mvcMatchers = new ArrayList<>(mvcMatchers);
3300+
this.matchers = allMatchers;
32953301
}
32963302

32973303
public RequestMatcherConfigurer servletPath(String servletPath) {
3298-
for (RequestMatcher matcher : this.matchers) {
3299-
((MvcRequestMatcher) matcher).setServletPath(servletPath);
3304+
for (MvcRequestMatcher matcher : this.mvcMatchers) {
3305+
matcher.setServletPath(servletPath);
33003306
}
33013307
return this;
33023308
}
@@ -3321,7 +3327,7 @@ public class RequestMatcherConfigurer extends AbstractRequestMatcherRegistry<Req
33213327
public MvcMatchersRequestMatcherConfigurer mvcMatchers(HttpMethod method, String... mvcPatterns) {
33223328
List<MvcRequestMatcher> mvcMatchers = createMvcMatchers(method, mvcPatterns);
33233329
setMatchers(mvcMatchers);
3324-
return new MvcMatchersRequestMatcherConfigurer(getContext(), mvcMatchers);
3330+
return new MvcMatchersRequestMatcherConfigurer(getContext(), mvcMatchers, this.matchers);
33253331
}
33263332

33273333
@Override

‎config/src/test/java/org/springframework/security/config/annotation/web/configurers/ChannelSecurityConfigurerTests.java‎

Lines changed: 80 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2019 the original author or authors.
2+
* Copyright 2002-2021 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -21,17 +21,22 @@
2121

2222
import org.springframework.beans.factory.annotation.Autowired;
2323
import org.springframework.context.annotation.Bean;
24+
import org.springframework.core.Ordered;
25+
import org.springframework.core.annotation.Order;
2426
import org.springframework.security.config.annotation.ObjectPostProcessor;
2527
import org.springframework.security.config.annotation.web.builders.HttpSecurity;
2628
import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity;
2729
import org.springframework.security.config.annotation.web.configuration.WebSecurityConfigurerAdapter;
2830
import org.springframework.security.config.test.SpringTestContext;
2931
import org.springframework.security.config.test.SpringTestContextExtension;
32+
import org.springframework.security.web.PortMapperImpl;
33+
import org.springframework.security.web.SecurityFilterChain;
3034
import org.springframework.security.web.access.channel.ChannelDecisionManagerImpl;
3135
import org.springframework.security.web.access.channel.ChannelProcessingFilter;
3236
import org.springframework.security.web.access.channel.InsecureChannelProcessor;
3337
import org.springframework.security.web.access.channel.SecureChannelProcessor;
3438
import org.springframework.test.web.servlet.MockMvc;
39+
import org.springframework.web.servlet.config.annotation.EnableWebMvc;
3540

3641
import static org.mockito.ArgumentMatchers.any;
3742
import static org.mockito.Mockito.spy;
@@ -44,6 +49,7 @@
4449
*
4550
* @author Rob Winch
4651
* @author Eleftheria Stein
52+
* @author Onur Kagan Ozcan
4753
*/
4854
@ExtendWith(SpringTestContextExtension.class)
4955
public class ChannelSecurityConfigurerTests {
@@ -93,6 +99,24 @@ public void requestWhenRequiresChannelConfiguredInLambdaThenRedirectsToHttps() t
9399
this.mvc.perform(get("/")).andExpect(redirectedUrl("https://localhost/"));
94100
}
95101

102+
// gh-10956
103+
@Test
104+
public void requestWhenRequiresChannelWithMultiMvcMatchersThenRedirectsToHttps() throws Exception {
105+
this.spring.register(RequiresChannelMultiMvcMatchersConfig.class).autowire();
106+
this.mvc.perform(get("/test-1")).andExpect(redirectedUrl("https://localhost/test-1"));
107+
this.mvc.perform(get("/test-2")).andExpect(redirectedUrl("https://localhost/test-2"));
108+
this.mvc.perform(get("/test-3")).andExpect(redirectedUrl("https://localhost/test-3"));
109+
}
110+
111+
// gh-10956
112+
@Test
113+
public void requestWhenRequiresChannelWithMultiMvcMatchersInLambdaThenRedirectsToHttps() throws Exception {
114+
this.spring.register(RequiresChannelMultiMvcMatchersInLambdaConfig.class).autowire();
115+
this.mvc.perform(get("/test-1")).andExpect(redirectedUrl("https://localhost/test-1"));
116+
this.mvc.perform(get("/test-2")).andExpect(redirectedUrl("https://localhost/test-2"));
117+
this.mvc.perform(get("/test-3")).andExpect(redirectedUrl("https://localhost/test-3"));
118+
}
119+
96120
@EnableWebSecurity
97121
static class ObjectPostProcessorConfig extends WebSecurityConfigurerAdapter {
98122

@@ -155,4 +179,59 @@ protected void configure(HttpSecurity http) throws Exception {
155179

156180
}
157181

182+
@EnableWebSecurity
183+
@EnableWebMvc
184+
static class RequiresChannelMultiMvcMatchersConfig {
185+
186+
@Bean
187+
@Order(Ordered.HIGHEST_PRECEDENCE)
188+
SecurityFilterChain filterChain(HttpSecurity http) throws Exception {
189+
// @formatter:off
190+
http
191+
.portMapper()
192+
.portMapper(new PortMapperImpl())
193+
.and()
194+
.requiresChannel()
195+
.mvcMatchers("/test-1")
196+
.requiresSecure()
197+
.mvcMatchers("/test-2")
198+
.requiresSecure()
199+
.mvcMatchers("/test-3")
200+
.requiresSecure()
201+
.anyRequest()
202+
.requiresInsecure();
203+
// @formatter:on
204+
return http.build();
205+
}
206+
207+
}
208+
209+
@EnableWebSecurity
210+
@EnableWebMvc
211+
static class RequiresChannelMultiMvcMatchersInLambdaConfig {
212+
213+
@Bean
214+
@Order(Ordered.HIGHEST_PRECEDENCE)
215+
SecurityFilterChain filterChain(HttpSecurity http) throws Exception {
216+
// @formatter:off
217+
http
218+
.portMapper((port) -> port
219+
.portMapper(new PortMapperImpl())
220+
)
221+
.requiresChannel((channel) -> channel
222+
.mvcMatchers("/test-1")
223+
.requiresSecure()
224+
.mvcMatchers("/test-2")
225+
.requiresSecure()
226+
.mvcMatchers("/test-3")
227+
.requiresSecure()
228+
.anyRequest()
229+
.requiresInsecure()
230+
);
231+
// @formatter:on
232+
return http.build();
233+
}
234+
235+
}
236+
158237
}

‎config/src/test/java/org/springframework/security/config/annotation/web/configurers/HttpSecurityRequestMatchersTests.java‎

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,10 @@
2323
import org.junit.jupiter.api.Test;
2424

2525
import org.springframework.beans.factory.annotation.Autowired;
26+
import org.springframework.context.annotation.Bean;
2627
import org.springframework.context.annotation.Configuration;
28+
import org.springframework.core.Ordered;
29+
import org.springframework.core.annotation.Order;
2730
import org.springframework.mock.web.MockFilterChain;
2831
import org.springframework.mock.web.MockHttpServletRequest;
2932
import org.springframework.mock.web.MockHttpServletResponse;
@@ -33,6 +36,7 @@
3336
import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity;
3437
import org.springframework.security.config.annotation.web.configuration.WebSecurityConfigurerAdapter;
3538
import org.springframework.security.web.FilterChainProxy;
39+
import org.springframework.security.web.SecurityFilterChain;
3640
import org.springframework.web.bind.annotation.RequestMapping;
3741
import org.springframework.web.bind.annotation.RestController;
3842
import org.springframework.web.context.support.AnnotationConfigWebApplicationContext;
@@ -167,6 +171,38 @@ public void requestMatcherWhensMvcMatcherServletPathInLambdaThenPathIsSecured()
167171
assertThat(this.response.getStatus()).isEqualTo(HttpServletResponse.SC_OK);
168172
}
169173

174+
@Test
175+
public void requestMatcherWhenMultiMvcMatcherInLambdaThenAllPathsAreDenied() throws Exception {
176+
loadConfig(MultiMvcMatcherInLambdaConfig.class);
177+
this.request.setRequestURI("/test-1");
178+
this.springSecurityFilterChain.doFilter(this.request, this.response, this.chain);
179+
assertThat(this.response.getStatus()).isEqualTo(HttpServletResponse.SC_UNAUTHORIZED);
180+
setup();
181+
this.request.setRequestURI("/test-2");
182+
this.springSecurityFilterChain.doFilter(this.request, this.response, this.chain);
183+
assertThat(this.response.getStatus()).isEqualTo(HttpServletResponse.SC_UNAUTHORIZED);
184+
setup();
185+
this.request.setRequestURI("/test-3");
186+
this.springSecurityFilterChain.doFilter(this.request, this.response, this.chain);
187+
assertThat(this.response.getStatus()).isEqualTo(HttpServletResponse.SC_UNAUTHORIZED);
188+
}
189+
190+
@Test
191+
public void requestMatcherWhenMultiMvcMatcherThenAllPathsAreDenied() throws Exception {
192+
loadConfig(MultiMvcMatcherConfig.class);
193+
this.request.setRequestURI("/test-1");
194+
this.springSecurityFilterChain.doFilter(this.request, this.response, this.chain);
195+
assertThat(this.response.getStatus()).isEqualTo(HttpServletResponse.SC_UNAUTHORIZED);
196+
setup();
197+
this.request.setRequestURI("/test-2");
198+
this.springSecurityFilterChain.doFilter(this.request, this.response, this.chain);
199+
assertThat(this.response.getStatus()).isEqualTo(HttpServletResponse.SC_UNAUTHORIZED);
200+
setup();
201+
this.request.setRequestURI("/test-3");
202+
this.springSecurityFilterChain.doFilter(this.request, this.response, this.chain);
203+
assertThat(this.response.getStatus()).isEqualTo(HttpServletResponse.SC_UNAUTHORIZED);
204+
}
205+
170206
public void loadConfig(Class<?>... configs) {
171207
this.context = new AnnotationConfigWebApplicationContext();
172208
this.context.register(configs);
@@ -175,6 +211,101 @@ public void loadConfig(Class<?>... configs) {
175211
this.context.getAutowireCapableBeanFactory().autowireBean(this);
176212
}
177213

214+
@EnableWebSecurity
215+
@Configuration
216+
@EnableWebMvc
217+
static class MultiMvcMatcherInLambdaConfig {
218+
219+
@Bean
220+
@Order(Ordered.HIGHEST_PRECEDENCE)
221+
SecurityFilterChain first(HttpSecurity http) throws Exception {
222+
// @formatter:off
223+
http
224+
.requestMatchers((requests) -> requests
225+
.mvcMatchers("/test-1")
226+
.mvcMatchers("/test-2")
227+
.mvcMatchers("/test-3")
228+
)
229+
.authorizeRequests((authorize) -> authorize.anyRequest().denyAll())
230+
.httpBasic(withDefaults());
231+
// @formatter:on
232+
return http.build();
233+
}
234+
235+
@Bean
236+
SecurityFilterChain second(HttpSecurity http) throws Exception {
237+
// @formatter:off
238+
http
239+
.requestMatchers((requests) -> requests
240+
.mvcMatchers("/test-1")
241+
)
242+
.authorizeRequests((authorize) -> authorize
243+
.anyRequest().permitAll()
244+
);
245+
// @formatter:on
246+
return http.build();
247+
}
248+
249+
@RestController
250+
static class PathController {
251+
252+
@RequestMapping({ "/test-1", "/test-2", "/test-3" })
253+
String path() {
254+
return "path";
255+
}
256+
257+
}
258+
259+
}
260+
261+
@EnableWebSecurity
262+
@Configuration
263+
@EnableWebMvc
264+
static class MultiMvcMatcherConfig {
265+
266+
@Bean
267+
@Order(Ordered.HIGHEST_PRECEDENCE)
268+
SecurityFilterChain first(HttpSecurity http) throws Exception {
269+
// @formatter:off
270+
http
271+
.requestMatchers()
272+
.mvcMatchers("/test-1")
273+
.mvcMatchers("/test-2")
274+
.mvcMatchers("/test-3")
275+
.and()
276+
.authorizeRequests()
277+
.anyRequest().denyAll()
278+
.and()
279+
.httpBasic(withDefaults());
280+
// @formatter:on
281+
return http.build();
282+
}
283+
284+
@Bean
285+
SecurityFilterChain second(HttpSecurity http) throws Exception {
286+
// @formatter:off
287+
http
288+
.requestMatchers()
289+
.mvcMatchers("/test-1")
290+
.and()
291+
.authorizeRequests()
292+
.anyRequest().permitAll();
293+
// @formatter:on
294+
return http.build();
295+
}
296+
297+
@RestController
298+
static class PathController {
299+
300+
@RequestMapping({ "/test-1", "/test-2", "/test-3" })
301+
String path() {
302+
return "path";
303+
}
304+
305+
}
306+
307+
}
308+
178309
@EnableWebSecurity
179310
@Configuration
180311
@EnableWebMvc

0 commit comments

Comments
 (0)