Skip to content

Commit

Permalink
Fix mvcMatchers overriding previous paths
Browse files Browse the repository at this point in the history 
Closes gh-10956
  • Loading branch information
@marcusdacoregio
marcusdacoregio committed May 12, 2022
1 parent 15b3744 commit 7983c69
Show file tree
Hide file tree
Showing 4 changed files with 302 additions and 11 deletions.
22 changes: 14 additions & 8 deletions ...c/main/java/org/springframework/security/config/annotation/web/builders/HttpSecurity.java
View file
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -3008,20 +3008,26 @@ private <C extends SecurityConfigurerAdapter<DefaultSecurityFilterChain, HttpSec
*/
public final class MvcMatchersRequestMatcherConfigurer extends RequestMatcherConfigurer {

private final List<MvcRequestMatcher> 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<MvcRequestMatcher> matchers) {
private MvcMatchersRequestMatcherConfigurer(ApplicationContext context, List<MvcRequestMatcher> mvcMatchers,
List<RequestMatcher> 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;
}
Expand All @@ -3046,7 +3052,7 @@ public class RequestMatcherConfigurer extends AbstractRequestMatcherRegistry<Req
public MvcMatchersRequestMatcherConfigurer mvcMatchers(HttpMethod method, String... mvcPatterns) {
List<MvcRequestMatcher> mvcMatchers = createMvcMatchers(method, mvcPatterns);
setMatchers(mvcMatchers);
return new MvcMatchersRequestMatcherConfigurer(getContext(), mvcMatchers);
return new MvcMatchersRequestMatcherConfigurer(getContext(), mvcMatchers, this.matchers);
}

@Override
Expand Down
80 changes: 79 additions & 1 deletion ...gframework/security/config/annotation/web/configurers/ChannelSecurityConfigurerTests.java
View file
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -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;
Expand Down Expand Up @@ -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 {

Expand Down Expand Up @@ -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();
}

}

}
133 changes: 132 additions & 1 deletion ...ramework/security/config/annotation/web/configurers/HttpSecurityRequestMatchersTests.java
View file
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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
Expand Down
Loading

0 comments on commit 7983c69

Please sign in to comment.