From 7983c695e23027cfdb7689b348640367256738fe 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 | 22 +-- .../ChannelSecurityConfigurerTests.java | 80 ++++++++++- .../HttpSecurityRequestMatchersTests.java | 133 +++++++++++++++++- .../UrlAuthorizationConfigurerTests.java | 78 +++++++++- 4 files changed, 302 insertions(+), 11 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 ab44c8b5882..11406a85500 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 @@ -1,5 +1,5 @@ /* - * Copyright 2002-2020 the original author or authors. + * Copyright 2002-2022 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -3008,20 +3008,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; } @@ -3046,7 +3052,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 491c1960ead..59e1b725b8d 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 @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2022 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -21,16 +21,21 @@ 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; import org.springframework.security.config.annotation.web.configuration.WebSecurityConfigurerAdapter; import org.springframework.security.config.test.SpringTestRule; +import org.springframework.security.web.PortMapperImpl; +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; @@ -92,6 +97,24 @@ public void requestWhenRequiresChannelConfiguredInLambdaThenRedirectsToHttps() t this.mvc.perform(get("/")).andExpect(redirectedUrl("https://localhost/")); } + // 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 { @@ -154,4 +177,59 @@ protected void configure(HttpSecurity http) throws Exception { } + @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 ca4ae5ff5bd..48b914ef83d 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 @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2022 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -23,7 +23,10 @@ import org.junit.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 34dcb0f52da..32c5e02096f 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 @@ -1,5 +1,5 @@ /* - * Copyright 2002-2015 the original author or authors. + * Copyright 2002-2022 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -16,6 +16,8 @@ package org.springframework.security.config.annotation.web.configurers; +import java.util.Base64; + import javax.servlet.http.HttpServletResponse; import org.junit.After; @@ -23,16 +25,24 @@ import org.junit.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"; + } + + } + + } + }