From 1959c25a03553a28cb196578f87d1adabb9d2e0a Mon Sep 17 00:00:00 2001 From: Marcus Da Coregio Date: Tue, 22 Mar 2022 12:37:51 -0300 Subject: [PATCH] Fix mvcMatchers overriding previous paths Closes gh-10956 --- .../annotation/web/builders/HttpSecurity.java | 20 ++- .../ChannelSecurityConfigurerTests.java | 77 ++++++++++ .../HttpSecurityRequestMatchersTests.java | 131 ++++++++++++++++++ .../UrlAuthorizationConfigurerTests.java | 76 ++++++++++ 4 files changed, 297 insertions(+), 7 deletions(-) diff --git a/config/src/main/java/org/springframework/security/config/annotation/web/builders/HttpSecurity.java b/config/src/main/java/org/springframework/security/config/annotation/web/builders/HttpSecurity.java index 3ed743869bb..9840448af31 100644 --- a/config/src/main/java/org/springframework/security/config/annotation/web/builders/HttpSecurity.java +++ b/config/src/main/java/org/springframework/security/config/annotation/web/builders/HttpSecurity.java @@ -3290,20 +3290,26 @@ private mvcMatchers; + /** * Creates a new instance * @param context the {@link ApplicationContext} to use - * @param matchers the {@link MvcRequestMatcher} instances to set the servlet path - * on if {@link #servletPath(String)} is set. + * @param mvcMatchers the {@link MvcRequestMatcher} instances to set the servlet + * path on if {@link #servletPath(String)} is set. + * @param allMatchers the {@link RequestMatcher} instances to continue the + * configuration */ - private MvcMatchersRequestMatcherConfigurer(ApplicationContext context, List matchers) { + private MvcMatchersRequestMatcherConfigurer(ApplicationContext context, List mvcMatchers, + List allMatchers) { super(context); - this.matchers = new ArrayList<>(matchers); + this.mvcMatchers = new ArrayList<>(mvcMatchers); + this.matchers = allMatchers; } public RequestMatcherConfigurer servletPath(String servletPath) { - for (RequestMatcher matcher : this.matchers) { - ((MvcRequestMatcher) matcher).setServletPath(servletPath); + for (MvcRequestMatcher matcher : this.mvcMatchers) { + matcher.setServletPath(servletPath); } return this; } @@ -3328,7 +3334,7 @@ public class RequestMatcherConfigurer extends AbstractRequestMatcherRegistry mvcMatchers = createMvcMatchers(method, mvcPatterns); setMatchers(mvcMatchers); - return new MvcMatchersRequestMatcherConfigurer(getContext(), mvcMatchers); + return new MvcMatchersRequestMatcherConfigurer(getContext(), mvcMatchers, this.matchers); } @Override diff --git a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/ChannelSecurityConfigurerTests.java b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/ChannelSecurityConfigurerTests.java index 83b2045a842..50b89966151 100644 --- a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/ChannelSecurityConfigurerTests.java +++ b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/ChannelSecurityConfigurerTests.java @@ -26,6 +26,8 @@ import org.springframework.beans.factory.annotation.Autowired; import org.springframework.context.annotation.Bean; +import org.springframework.core.Ordered; +import org.springframework.core.annotation.Order; import org.springframework.security.config.annotation.ObjectPostProcessor; import org.springframework.security.config.annotation.web.builders.HttpSecurity; import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity; @@ -34,11 +36,13 @@ import org.springframework.security.config.test.SpringTestContextExtension; import org.springframework.security.web.PortMapperImpl; import org.springframework.security.web.RedirectStrategy; +import org.springframework.security.web.SecurityFilterChain; import org.springframework.security.web.access.channel.ChannelDecisionManagerImpl; import org.springframework.security.web.access.channel.ChannelProcessingFilter; import org.springframework.security.web.access.channel.InsecureChannelProcessor; import org.springframework.security.web.access.channel.SecureChannelProcessor; import org.springframework.test.web.servlet.MockMvc; +import org.springframework.web.servlet.config.annotation.EnableWebMvc; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.spy; @@ -107,6 +111,24 @@ public void requestWhenRequiresChannelConfiguredWithUrlRedirectThenRedirectsToUr this.mvc.perform(get("/")).andExpect(redirectedUrl("https://localhost/test")); } + // gh-10956 + @Test + public void requestWhenRequiresChannelWithMultiMvcMatchersThenRedirectsToHttps() throws Exception { + this.spring.register(RequiresChannelMultiMvcMatchersConfig.class).autowire(); + this.mvc.perform(get("/test-1")).andExpect(redirectedUrl("https://localhost/test-1")); + this.mvc.perform(get("/test-2")).andExpect(redirectedUrl("https://localhost/test-2")); + this.mvc.perform(get("/test-3")).andExpect(redirectedUrl("https://localhost/test-3")); + } + + // gh-10956 + @Test + public void requestWhenRequiresChannelWithMultiMvcMatchersInLambdaThenRedirectsToHttps() throws Exception { + this.spring.register(RequiresChannelMultiMvcMatchersInLambdaConfig.class).autowire(); + this.mvc.perform(get("/test-1")).andExpect(redirectedUrl("https://localhost/test-1")); + this.mvc.perform(get("/test-2")).andExpect(redirectedUrl("https://localhost/test-2")); + this.mvc.perform(get("/test-3")).andExpect(redirectedUrl("https://localhost/test-3")); + } + @EnableWebSecurity static class ObjectPostProcessorConfig extends WebSecurityConfigurerAdapter { @@ -200,4 +222,59 @@ public void sendRedirect(HttpServletRequest request, HttpServletResponse respons } + @EnableWebSecurity + @EnableWebMvc + static class RequiresChannelMultiMvcMatchersConfig { + + @Bean + @Order(Ordered.HIGHEST_PRECEDENCE) + SecurityFilterChain filterChain(HttpSecurity http) throws Exception { + // @formatter:off + http + .portMapper() + .portMapper(new PortMapperImpl()) + .and() + .requiresChannel() + .mvcMatchers("/test-1") + .requiresSecure() + .mvcMatchers("/test-2") + .requiresSecure() + .mvcMatchers("/test-3") + .requiresSecure() + .anyRequest() + .requiresInsecure(); + // @formatter:on + return http.build(); + } + + } + + @EnableWebSecurity + @EnableWebMvc + static class RequiresChannelMultiMvcMatchersInLambdaConfig { + + @Bean + @Order(Ordered.HIGHEST_PRECEDENCE) + SecurityFilterChain filterChain(HttpSecurity http) throws Exception { + // @formatter:off + http + .portMapper((port) -> port + .portMapper(new PortMapperImpl()) + ) + .requiresChannel((channel) -> channel + .mvcMatchers("/test-1") + .requiresSecure() + .mvcMatchers("/test-2") + .requiresSecure() + .mvcMatchers("/test-3") + .requiresSecure() + .anyRequest() + .requiresInsecure() + ); + // @formatter:on + return http.build(); + } + + } + } diff --git a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/HttpSecurityRequestMatchersTests.java b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/HttpSecurityRequestMatchersTests.java index 9e60e93994f..a5b40a3554a 100644 --- a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/HttpSecurityRequestMatchersTests.java +++ b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/HttpSecurityRequestMatchersTests.java @@ -23,7 +23,10 @@ import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; +import org.springframework.core.Ordered; +import org.springframework.core.annotation.Order; import org.springframework.mock.web.MockFilterChain; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; @@ -33,6 +36,7 @@ import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity; import org.springframework.security.config.annotation.web.configuration.WebSecurityConfigurerAdapter; import org.springframework.security.web.FilterChainProxy; +import org.springframework.security.web.SecurityFilterChain; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RestController; import org.springframework.web.context.support.AnnotationConfigWebApplicationContext; @@ -167,6 +171,38 @@ public void requestMatcherWhensMvcMatcherServletPathInLambdaThenPathIsSecured() assertThat(this.response.getStatus()).isEqualTo(HttpServletResponse.SC_OK); } + @Test + public void requestMatcherWhenMultiMvcMatcherInLambdaThenAllPathsAreDenied() throws Exception { + loadConfig(MultiMvcMatcherInLambdaConfig.class); + this.request.setRequestURI("/test-1"); + this.springSecurityFilterChain.doFilter(this.request, this.response, this.chain); + assertThat(this.response.getStatus()).isEqualTo(HttpServletResponse.SC_UNAUTHORIZED); + setup(); + this.request.setRequestURI("/test-2"); + this.springSecurityFilterChain.doFilter(this.request, this.response, this.chain); + assertThat(this.response.getStatus()).isEqualTo(HttpServletResponse.SC_UNAUTHORIZED); + setup(); + this.request.setRequestURI("/test-3"); + this.springSecurityFilterChain.doFilter(this.request, this.response, this.chain); + assertThat(this.response.getStatus()).isEqualTo(HttpServletResponse.SC_UNAUTHORIZED); + } + + @Test + public void requestMatcherWhenMultiMvcMatcherThenAllPathsAreDenied() throws Exception { + loadConfig(MultiMvcMatcherConfig.class); + this.request.setRequestURI("/test-1"); + this.springSecurityFilterChain.doFilter(this.request, this.response, this.chain); + assertThat(this.response.getStatus()).isEqualTo(HttpServletResponse.SC_UNAUTHORIZED); + setup(); + this.request.setRequestURI("/test-2"); + this.springSecurityFilterChain.doFilter(this.request, this.response, this.chain); + assertThat(this.response.getStatus()).isEqualTo(HttpServletResponse.SC_UNAUTHORIZED); + setup(); + this.request.setRequestURI("/test-3"); + this.springSecurityFilterChain.doFilter(this.request, this.response, this.chain); + assertThat(this.response.getStatus()).isEqualTo(HttpServletResponse.SC_UNAUTHORIZED); + } + public void loadConfig(Class... configs) { this.context = new AnnotationConfigWebApplicationContext(); this.context.register(configs); @@ -175,6 +211,101 @@ public void loadConfig(Class... configs) { this.context.getAutowireCapableBeanFactory().autowireBean(this); } + @EnableWebSecurity + @Configuration + @EnableWebMvc + static class MultiMvcMatcherInLambdaConfig { + + @Bean + @Order(Ordered.HIGHEST_PRECEDENCE) + SecurityFilterChain first(HttpSecurity http) throws Exception { + // @formatter:off + http + .requestMatchers((requests) -> requests + .mvcMatchers("/test-1") + .mvcMatchers("/test-2") + .mvcMatchers("/test-3") + ) + .authorizeRequests((authorize) -> authorize.anyRequest().denyAll()) + .httpBasic(withDefaults()); + // @formatter:on + return http.build(); + } + + @Bean + SecurityFilterChain second(HttpSecurity http) throws Exception { + // @formatter:off + http + .requestMatchers((requests) -> requests + .mvcMatchers("/test-1") + ) + .authorizeRequests((authorize) -> authorize + .anyRequest().permitAll() + ); + // @formatter:on + return http.build(); + } + + @RestController + static class PathController { + + @RequestMapping({ "/test-1", "/test-2", "/test-3" }) + String path() { + return "path"; + } + + } + + } + + @EnableWebSecurity + @Configuration + @EnableWebMvc + static class MultiMvcMatcherConfig { + + @Bean + @Order(Ordered.HIGHEST_PRECEDENCE) + SecurityFilterChain first(HttpSecurity http) throws Exception { + // @formatter:off + http + .requestMatchers() + .mvcMatchers("/test-1") + .mvcMatchers("/test-2") + .mvcMatchers("/test-3") + .and() + .authorizeRequests() + .anyRequest().denyAll() + .and() + .httpBasic(withDefaults()); + // @formatter:on + return http.build(); + } + + @Bean + SecurityFilterChain second(HttpSecurity http) throws Exception { + // @formatter:off + http + .requestMatchers() + .mvcMatchers("/test-1") + .and() + .authorizeRequests() + .anyRequest().permitAll(); + // @formatter:on + return http.build(); + } + + @RestController + static class PathController { + + @RequestMapping({ "/test-1", "/test-2", "/test-3" }) + String path() { + return "path"; + } + + } + + } + @EnableWebSecurity @Configuration @EnableWebMvc diff --git a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/UrlAuthorizationConfigurerTests.java b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/UrlAuthorizationConfigurerTests.java index 914ea135ea9..87396467ded 100644 --- a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/UrlAuthorizationConfigurerTests.java +++ b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/UrlAuthorizationConfigurerTests.java @@ -16,6 +16,8 @@ package org.springframework.security.config.annotation.web.configurers; +import java.util.Base64; + import javax.servlet.http.HttpServletResponse; import org.junit.jupiter.api.AfterEach; @@ -23,16 +25,24 @@ import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.context.ApplicationContext; +import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.mock.web.MockFilterChain; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.mock.web.MockServletContext; +import org.springframework.security.config.Customizer; import org.springframework.security.config.annotation.authentication.builders.AuthenticationManagerBuilder; import org.springframework.security.config.annotation.web.builders.HttpSecurity; import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity; import org.springframework.security.config.annotation.web.configuration.WebSecurityConfigurerAdapter; +import org.springframework.security.core.userdetails.User; +import org.springframework.security.core.userdetails.UserDetails; +import org.springframework.security.core.userdetails.UserDetailsService; +import org.springframework.security.provisioning.InMemoryUserDetailsManager; import org.springframework.security.web.FilterChainProxy; +import org.springframework.security.web.SecurityFilterChain; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RestController; import org.springframework.web.context.support.AnnotationConfigWebApplicationContext; @@ -125,6 +135,35 @@ public void anonymousUrlAuthorization() { loadConfig(AnonymousUrlAuthorizationConfig.class); } + // gh-10956 + @Test + public void multiMvcMatchersConfig() throws Exception { + loadConfig(MultiMvcMatcherConfig.class); + this.request.addHeader("Authorization", + "Basic " + new String(Base64.getEncoder().encode("user:password".getBytes()))); + this.request.setRequestURI("/test-1"); + this.springSecurityFilterChain.doFilter(this.request, this.response, this.chain); + assertThat(this.response.getStatus()).isEqualTo(HttpServletResponse.SC_FORBIDDEN); + setup(); + this.request.addHeader("Authorization", + "Basic " + new String(Base64.getEncoder().encode("user:password".getBytes()))); + this.request.setRequestURI("/test-2"); + this.springSecurityFilterChain.doFilter(this.request, this.response, this.chain); + assertThat(this.response.getStatus()).isEqualTo(HttpServletResponse.SC_FORBIDDEN); + setup(); + this.request.addHeader("Authorization", + "Basic " + new String(Base64.getEncoder().encode("user:password".getBytes()))); + this.request.setRequestURI("/test-3"); + this.springSecurityFilterChain.doFilter(this.request, this.response, this.chain); + assertThat(this.response.getStatus()).isEqualTo(HttpServletResponse.SC_FORBIDDEN); + setup(); + this.request.addHeader("Authorization", + "Basic " + new String(Base64.getEncoder().encode("user:password".getBytes()))); + this.request.setRequestURI("/test-x"); + this.springSecurityFilterChain.doFilter(this.request, this.response, this.chain); + assertThat(this.response.getStatus()).isEqualTo(HttpServletResponse.SC_OK); + } + public void loadConfig(Class... configs) { this.context = new AnnotationConfigWebApplicationContext(); this.context.register(configs); @@ -228,4 +267,41 @@ public void configurePathMatch(PathMatchConfigurer configurer) { } + @EnableWebSecurity + @EnableWebMvc + static class MultiMvcMatcherConfig { + + @Bean + SecurityFilterChain security(HttpSecurity http, ApplicationContext context) throws Exception { + // @formatter:off + http + .httpBasic(Customizer.withDefaults()) + .apply(new UrlAuthorizationConfigurer<>(context)).getRegistry() + .mvcMatchers("/test-1").hasRole("ADMIN") + .mvcMatchers("/test-2").hasRole("ADMIN") + .mvcMatchers("/test-3").hasRole("ADMIN") + .anyRequest().hasRole("USER"); + // @formatter:on + return http.build(); + } + + @Bean + UserDetailsService userDetailsService() { + UserDetails user = User.withDefaultPasswordEncoder().username("user").password("password").roles("USER") + .build(); + return new InMemoryUserDetailsManager(user); + } + + @RestController + static class PathController { + + @RequestMapping({ "/test-1", "/test-2", "/test-3", "/test-x" }) + String path() { + return "path"; + } + + } + + } + }