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

Favor PathPatternParser Over HandlerMappingIntrospector #16408

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -43,17 +43,19 @@
import org.springframework.security.config.annotation.web.ServletRegistrationsSupport.RegistrationMapping;
import org.springframework.security.config.annotation.web.configurers.AbstractConfigAttributeRequestMatcherRegistry;
import org.springframework.security.web.servlet.util.matcher.MvcRequestMatcher;
import org.springframework.security.web.servlet.util.matcher.PathPatternRequestMatcher;
import org.springframework.security.web.util.matcher.AntPathRequestMatcher;
import org.springframework.security.web.util.matcher.AnyRequestMatcher;
import org.springframework.security.web.util.matcher.DispatcherTypeRequestMatcher;
import org.springframework.security.web.util.matcher.OrRequestMatcher;
import org.springframework.security.web.util.matcher.RegexRequestMatcher;
import org.springframework.security.web.util.matcher.RequestMatcher;
import org.springframework.security.web.util.matcher.RequestMatcherBuilder;
import org.springframework.util.Assert;
import org.springframework.util.ClassUtils;
import org.springframework.web.context.WebApplicationContext;
import org.springframework.web.servlet.DispatcherServlet;
import org.springframework.web.servlet.handler.HandlerMappingIntrospector;
import org.springframework.web.util.pattern.PathPatternParser;

/**
* A base class for registering {@link RequestMatcher}'s. For example, it might allow for
Expand All @@ -74,6 +76,8 @@ public abstract class AbstractRequestMatcherRegistry<C> {

private static final RequestMatcher ANY_REQUEST = AnyRequestMatcher.INSTANCE;

private final RequestMatcherBuilder requestMatcherBuilder = new DefaultRequestMatcherBuilder();

private ApplicationContext context;

private boolean anyRequestConfigured = false;
Expand Down Expand Up @@ -217,13 +221,9 @@ public C requestMatchers(HttpMethod method, String... patterns) {
if (servletContext == null) {
return requestMatchers(RequestMatchers.antMatchersAsArray(method, patterns));
}
List<RequestMatcher> matchers = new ArrayList<>();
for (String pattern : patterns) {
AntPathRequestMatcher ant = new AntPathRequestMatcher(pattern, (method != null) ? method.name() : null);
MvcRequestMatcher mvc = createMvcMatchers(method, pattern).get(0);
matchers.add(new DeferredRequestMatcher((c) -> resolve(ant, mvc, c), mvc, ant));
}
return requestMatchers(matchers.toArray(new RequestMatcher[0]));
RequestMatcherBuilder builder = context.getBeanProvider(RequestMatcherBuilder.class)
.getIfUnique(() -> this.requestMatcherBuilder);
return requestMatchers(builder.pattern(method, patterns));
}

private boolean anyPathsDontStartWithLeadingSlash(String... patterns) {
Expand All @@ -235,7 +235,7 @@ private boolean anyPathsDontStartWithLeadingSlash(String... patterns) {
return false;
}

private RequestMatcher resolve(AntPathRequestMatcher ant, MvcRequestMatcher mvc, ServletContext servletContext) {
private RequestMatcher resolve(AntPathRequestMatcher ant, RequestMatcher mvc, ServletContext servletContext) {
ServletRegistrationsSupport registrations = new ServletRegistrationsSupport(servletContext);
Collection<RegistrationMapping> mappings = registrations.mappings();
if (mappings.isEmpty()) {
Expand Down Expand Up @@ -264,11 +264,14 @@ private RequestMatcher resolve(AntPathRequestMatcher ant, MvcRequestMatcher mvc,
}

private static String computeErrorMessage(Collection<? extends ServletRegistration> registrations) {
String template = "This method cannot decide whether these patterns are Spring MVC patterns or not. "
+ "If this endpoint is a Spring MVC endpoint, please use requestMatchers(MvcRequestMatcher); "
+ "otherwise, please use requestMatchers(AntPathRequestMatcher).\n\n"
+ "This is because there is more than one mappable servlet in your servlet context: %s.\n\n"
+ "For each MvcRequestMatcher, call MvcRequestMatcher#setServletPath to indicate the servlet path.";
String template = """
This method cannot decide whether these patterns are Spring MVC patterns or not. \
This is because there is more than one mappable servlet in your servlet context: %s.

To address this, please create one ServletRequestMatcherBuilder#servletPath for each servlet that has \
authorized endpoints and use them to construct request matchers manually. \
If all your URIs are unambiguous, then you can simply publish one ServletRequestMatcherBuilders#servletPath as \
a @Bean and Spring Security will use it for all URIs""";
Map<String, Collection<String>> mappings = new LinkedHashMap<>();
for (ServletRegistration registration : registrations) {
mappings.put(registration.getClassName(), registration.getMappings());
Expand All @@ -278,10 +281,10 @@ private static String computeErrorMessage(Collection<? extends ServletRegistrati

/**
* <p>
* If the {@link HandlerMappingIntrospector} is available in the classpath, maps to an
* {@link MvcRequestMatcher} that does not care which {@link HttpMethod} is used. This
* matcher will use the same rules that Spring MVC uses for matching. For example,
* often times a mapping of the path "/path" will match on "/path", "/path/",
* If the {@link HandlerMappingIntrospector} is available in the classpath, maps to a
* {@link PathPatternRequestMatcher} that does not care which {@link HttpMethod} is
* used. This matcher will use the same rules that Spring MVC uses for matching. For
* example, often times a mapping of the path "/path" will match on "/path", "/path/",
* "/path.html", etc. If the {@link HandlerMappingIntrospector} is not available, maps
* to an {@link AntPathRequestMatcher}.
* </p>
Expand Down Expand Up @@ -402,6 +405,35 @@ static List<RequestMatcher> regexMatchers(String... regexPatterns) {

}

class DefaultRequestMatcherBuilder implements RequestMatcherBuilder {

@Override
public RequestMatcher pattern(HttpMethod method, String pattern) {
Assert.state(!AbstractRequestMatcherRegistry.this.anyRequestConfigured,
"Can't configure mvcMatchers after anyRequest");
AntPathRequestMatcher ant = new AntPathRequestMatcher(pattern, (method != null) ? method.name() : null);
RequestMatcher mvc;
if (!AbstractRequestMatcherRegistry.this.context.containsBean(HANDLER_MAPPING_INTROSPECTOR_BEAN_NAME)) {
throw new NoSuchBeanDefinitionException("A Bean named " + HANDLER_MAPPING_INTROSPECTOR_BEAN_NAME
+ " of type " + HandlerMappingIntrospector.class.getName()
+ " is required to use MvcRequestMatcher. Please ensure Spring Security & Spring MVC are configured in a shared ApplicationContext.");
}
HandlerMappingIntrospector introspector = AbstractRequestMatcherRegistry.this.context
.getBean(HANDLER_MAPPING_INTROSPECTOR_BEAN_NAME, HandlerMappingIntrospector.class);
if (introspector.allHandlerMappingsUsePathPatternParser()) {
PathPatternParser pathPatternParser = AbstractRequestMatcherRegistry.this.context
.getBeanProvider(PathPatternParser.class)
.getIfUnique(() -> PathPatternParser.defaultInstance);
mvc = PathPatternRequestMatcher.withPathPatternParser(pathPatternParser).pattern(method, pattern);
}
else {
mvc = createMvcMatchers(method, pattern).get(0);
}
return new DeferredRequestMatcher((c) -> resolve(ant, mvc, c), mvc, ant);
}

}

static class DeferredRequestMatcher implements RequestMatcher {

final Function<ServletContext, RequestMatcher> requestMatcherFactory;
Expand Down Expand Up @@ -453,13 +485,8 @@ public boolean matches(HttpServletRequest request) {
ServletRegistration registration = request.getServletContext().getServletRegistration(name);
Assert.notNull(registration,
() -> computeErrorMessage(request.getServletContext().getServletRegistrations().values()));
try {
Class<?> clazz = Class.forName(registration.getClassName());
return DispatcherServlet.class.isAssignableFrom(clazz);
}
catch (ClassNotFoundException ex) {
return false;
}
return new RegistrationMapping(registration, request.getHttpServletMapping().getPattern())
.isDispatcherServlet();
}

}
Expand All @@ -468,15 +495,15 @@ static class DispatcherServletDelegatingRequestMatcher implements RequestMatcher

private final AntPathRequestMatcher ant;

private final MvcRequestMatcher mvc;
private final RequestMatcher mvc;

private final RequestMatcher dispatcherServlet;

DispatcherServletDelegatingRequestMatcher(AntPathRequestMatcher ant, MvcRequestMatcher mvc) {
DispatcherServletDelegatingRequestMatcher(AntPathRequestMatcher ant, RequestMatcher mvc) {
this(ant, mvc, new OrRequestMatcher(new MockMvcRequestMatcher(), new DispatcherServletRequestMatcher()));
}

DispatcherServletDelegatingRequestMatcher(AntPathRequestMatcher ant, MvcRequestMatcher mvc,
DispatcherServletDelegatingRequestMatcher(AntPathRequestMatcher ant, RequestMatcher mvc,
RequestMatcher dispatcherServlet) {
this.ant = ant;
this.mvc = mvc;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,20 +16,25 @@

package org.springframework.security.config.http;

import jakarta.servlet.http.HttpServletRequest;
import org.w3c.dom.Element;

import org.springframework.beans.factory.FactoryBean;
import org.springframework.beans.factory.config.BeanDefinition;
import org.springframework.beans.factory.support.BeanDefinitionBuilder;
import org.springframework.beans.factory.support.RootBeanDefinition;
import org.springframework.beans.factory.xml.ParserContext;
import org.springframework.http.HttpMethod;
import org.springframework.security.web.servlet.util.matcher.MvcRequestMatcher;
import org.springframework.security.web.servlet.util.matcher.PathPatternRequestMatcher;
import org.springframework.security.web.util.matcher.AntPathRequestMatcher;
import org.springframework.security.web.util.matcher.AnyRequestMatcher;
import org.springframework.security.web.util.matcher.RegexRequestMatcher;
import org.springframework.security.web.util.matcher.RequestMatcher;
import org.springframework.util.ClassUtils;
import org.springframework.util.StringUtils;
import org.springframework.web.servlet.handler.HandlerMappingIntrospector;
import org.springframework.web.util.pattern.PathPatternParser;

/**
* Defines the {@link RequestMatcher} types supported by the namespace.
Expand All @@ -40,7 +45,7 @@
public enum MatcherType {

ant(AntPathRequestMatcher.class), regex(RegexRequestMatcher.class), ciRegex(RegexRequestMatcher.class),
mvc(MvcRequestMatcher.class);
mvc(MvcRequestMatcherFactoryBean.class);

private static final String HANDLER_MAPPING_INTROSPECTOR = "org.springframework.web.servlet.handler.HandlerMappingIntrospector";

Expand Down Expand Up @@ -100,4 +105,56 @@ static MatcherType fromElementOrMvc(Element elt) {
return MatcherType.fromElement(elt);
}

private static class MvcRequestMatcherFactoryBean implements FactoryBean<RequestMatcher>, RequestMatcher {

private final HandlerMappingIntrospector introspector;

private final String pattern;

private PathPatternParser pathPatternParser = PathPatternParser.defaultInstance;

private String servletPath;

private HttpMethod method;

public MvcRequestMatcherFactoryBean(HandlerMappingIntrospector introspector, String pattern) {
this.introspector = introspector;
this.pattern = pattern;
}

@Override
public RequestMatcher getObject() {
if (this.introspector.allHandlerMappingsUsePathPatternParser()) {
return PathPatternRequestMatcher.withPathPatternParser(this.pathPatternParser)
.servletPath(this.servletPath)
.pattern(this.method, this.pattern);
}
return new MvcRequestMatcher.Builder(this.introspector).servletPath(this.servletPath)
.pattern(this.method, this.pattern);
}

@Override
public Class<?> getObjectType() {
return RequestMatcher.class;
}

@Override
public boolean matches(HttpServletRequest request) {
return getObject().matches(request);
}

public void setPathPatternParser(PathPatternParser pathPatternParser) {
this.pathPatternParser = pathPatternParser;
}

public void setServletPath(String servletPath) {
this.servletPath = servletPath;
}

public void setMethod(HttpMethod method) {
this.method = method;
}

}

}
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,12 @@ import org.springframework.security.web.access.IpAddressAuthorizationManager
import org.springframework.security.web.access.intercept.AuthorizationFilter
import org.springframework.security.web.access.intercept.RequestAuthorizationContext
import org.springframework.security.web.servlet.util.matcher.MvcRequestMatcher
import org.springframework.security.web.servlet.util.matcher.PathPatternRequestMatcher
import org.springframework.security.web.util.matcher.AnyRequestMatcher
import org.springframework.security.web.util.matcher.RequestMatcher
import org.springframework.util.ClassUtils
import org.springframework.web.servlet.handler.HandlerMappingIntrospector
import org.springframework.web.util.pattern.PathPatternParser
import java.util.function.Supplier

/**
Expand Down Expand Up @@ -292,11 +294,20 @@ class AuthorizeHttpRequestsDsl : AbstractRequestMatcherDsl {
PatternType.ANT -> requests.requestMatchers(rule.httpMethod, rule.pattern).access(rule.rule)
PatternType.MVC -> {
val introspector = requests.applicationContext.getBean(HANDLER_MAPPING_INTROSPECTOR_BEAN_NAME, HandlerMappingIntrospector::class.java)
val mvcMatcher = MvcRequestMatcher.Builder(introspector)
.servletPath(rule.servletPath)
.pattern(rule.pattern)
mvcMatcher.setMethod(rule.httpMethod)
requests.requestMatchers(mvcMatcher).access(rule.rule)
if (introspector.allHandlerMappingsUsePathPatternParser()) {
val pathPatternParser: PathPatternParser = requests.applicationContext.getBeanProvider(PathPatternParser::class.java)
.getIfUnique({PathPatternParser.defaultInstance})
val mvcMatcher = PathPatternRequestMatcher.withPathPatternParser(pathPatternParser)
.servletPath(rule.servletPath)
.pattern(rule.httpMethod, rule.pattern)
requests.requestMatchers(mvcMatcher).access(rule.rule)
} else {
val mvcMatcher = MvcRequestMatcher.Builder(introspector)
.servletPath(rule.servletPath)
.pattern(rule.pattern)
mvcMatcher.setMethod(rule.httpMethod)
requests.requestMatchers(mvcMatcher).access(rule.rule)
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import org.springframework.security.web.util.matcher.AnyRequestMatcher;
import org.springframework.security.web.util.matcher.RequestMatcher;
import org.springframework.test.util.ReflectionTestUtils;
import org.springframework.web.util.pattern.PathPattern;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.ArgumentMatchers.any;
Expand Down Expand Up @@ -120,7 +121,8 @@ public void mixingPatternsAndPlaceholdersDoesntCauseOrderingIssues() {

private String getPattern(SecurityFilterChain chain) {
RequestMatcher requestMatcher = ((DefaultSecurityFilterChain) chain).getRequestMatcher();
return (String) ReflectionTestUtils.getField(requestMatcher, "pattern");
Object pattern = ReflectionTestUtils.getField(requestMatcher, "pattern");
return (pattern instanceof PathPattern) ? pattern.toString() : (String) pattern;
}

private void checkPathAndFilterOrder(FilterChainProxy filterChainProxy) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,11 @@

import jakarta.servlet.DispatcherType;
import jakarta.servlet.Servlet;
import org.jetbrains.annotations.NotNull;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import org.springframework.beans.BeansException;
import org.springframework.beans.factory.NoSuchBeanDefinitionException;
import org.springframework.beans.factory.ObjectProvider;
import org.springframework.context.ApplicationContext;
Expand All @@ -38,19 +40,24 @@
import org.springframework.security.web.servlet.MockServletContext;
import org.springframework.security.web.servlet.TestMockHttpServletMappings;
import org.springframework.security.web.servlet.util.matcher.MvcRequestMatcher;
import org.springframework.security.web.servlet.util.matcher.PathPatternRequestMatcher;
import org.springframework.security.web.util.matcher.AntPathRequestMatcher;
import org.springframework.security.web.util.matcher.DispatcherTypeRequestMatcher;
import org.springframework.security.web.util.matcher.RegexRequestMatcher;
import org.springframework.security.web.util.matcher.RequestMatcher;
import org.springframework.security.web.util.matcher.RequestMatcherBuilder;
import org.springframework.test.web.servlet.MockMvc;
import org.springframework.test.web.servlet.setup.MockMvcBuilders;
import org.springframework.web.context.WebApplicationContext;
import org.springframework.web.servlet.DispatcherServlet;
import org.springframework.web.servlet.config.annotation.EnableWebMvc;
import org.springframework.web.servlet.handler.HandlerMappingIntrospector;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
import static org.assertj.core.api.InstanceOfAssertFactories.type;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.BDDMockito.given;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
Expand Down Expand Up @@ -87,6 +94,15 @@ public void setUp() {
given(given).willReturn(postProcessors);
given(postProcessors.getObject()).willReturn(NO_OP_OBJECT_POST_PROCESSOR);
given(this.context.getServletContext()).willReturn(MockServletContext.mvc());
ObjectProvider<RequestMatcherBuilder> requestMatcherFactories = new ObjectProvider<>() {
@Override
public @NotNull RequestMatcherBuilder getObject() throws BeansException {
return AbstractRequestMatcherRegistryTests.this.matcherRegistry.new DefaultRequestMatcherBuilder();
}
};
given(this.context.getBeanProvider(RequestMatcherBuilder.class)).willReturn(requestMatcherFactories);
HandlerMappingIntrospector introspector = mock(HandlerMappingIntrospector.class);
given(this.context.getBean(any(), eq(HandlerMappingIntrospector.class))).willReturn(introspector);
this.matcherRegistry.setApplicationContext(this.context);
mockMvcIntrospector(true);
}
Expand Down Expand Up @@ -222,23 +238,23 @@ public void requestMatchersWhenNoDispatcherServletMockMvcThenMvcRequestMatcherTy
assertThat(requestMatchers).hasSize(1);
assertThat(requestMatchers.get(0)).asInstanceOf(type(DispatcherServletDelegatingRequestMatcher.class))
.extracting((matcher) -> matcher.requestMatcher(request))
.isInstanceOf(MvcRequestMatcher.class);
.isInstanceOf(PathPatternRequestMatcher.class);
servletContext.addServlet("servletOne", Servlet.class).addMapping("/one");
servletContext.addServlet("servletTwo", Servlet.class).addMapping("/two");
requestMatchers = this.matcherRegistry.requestMatchers("/**");
assertThat(requestMatchers).isNotEmpty();
assertThat(requestMatchers).hasSize(1);
assertThat(requestMatchers.get(0)).asInstanceOf(type(DispatcherServletDelegatingRequestMatcher.class))
.extracting((matcher) -> matcher.requestMatcher(request))
.isInstanceOf(MvcRequestMatcher.class);
.isInstanceOf(PathPatternRequestMatcher.class);
servletContext.addServlet("servletOne", Servlet.class);
servletContext.addServlet("servletTwo", Servlet.class);
requestMatchers = this.matcherRegistry.requestMatchers("/**");
assertThat(requestMatchers).isNotEmpty();
assertThat(requestMatchers).hasSize(1);
assertThat(requestMatchers.get(0)).asInstanceOf(type(DispatcherServletDelegatingRequestMatcher.class))
.extracting((matcher) -> matcher.requestMatcher(request))
.isInstanceOf(MvcRequestMatcher.class);
.isInstanceOf(PathPatternRequestMatcher.class);
}
}

Expand Down
Loading
Loading