From 393081f2e6a590df71cdef94e66c5996bbcc44ed Mon Sep 17 00:00:00 2001 From: Madhura Bhave Date: Wed, 8 Sep 2021 15:12:01 -0700 Subject: [PATCH] Enable PathPattern based matching for MVC actuators Closes gh-24645 --- ...undryWebEndpointServletHandlerMapping.java | 4 +- ...ndpointManagementContextConfiguration.java | 3 +- .../WebMvcEndpointIntegrationTests.java | 12 ++++ .../AbstractWebMvcEndpointHandlerMapping.java | 56 ++++++++++++++----- .../servlet/WebMvcEndpointHandlerMapping.java | 22 ++++++++ .../MvcWebEndpointIntegrationTests.java | 54 ++++++++++++++++-- .../web/servlet/WebMvcAutoConfiguration.java | 7 ++- 7 files changed, 135 insertions(+), 23 deletions(-) diff --git a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/cloudfoundry/servlet/CloudFoundryWebEndpointServletHandlerMapping.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/cloudfoundry/servlet/CloudFoundryWebEndpointServletHandlerMapping.java index 472535cf6b..9801cd3157 100644 --- a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/cloudfoundry/servlet/CloudFoundryWebEndpointServletHandlerMapping.java +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/cloudfoundry/servlet/CloudFoundryWebEndpointServletHandlerMapping.java @@ -38,6 +38,7 @@ import org.springframework.boot.actuate.endpoint.web.ExposableWebEndpoint; import org.springframework.boot.actuate.endpoint.web.Link; import org.springframework.boot.actuate.endpoint.web.WebOperation; import org.springframework.boot.actuate.endpoint.web.servlet.AbstractWebMvcEndpointHandlerMapping; +import org.springframework.boot.autoconfigure.web.servlet.WebMvcAutoConfiguration; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; import org.springframework.web.bind.annotation.ResponseBody; @@ -64,7 +65,8 @@ class CloudFoundryWebEndpointServletHandlerMapping extends AbstractWebMvcEndpoin Collection endpoints, EndpointMediaTypes endpointMediaTypes, CorsConfiguration corsConfiguration, CloudFoundrySecurityInterceptor securityInterceptor, EndpointLinksResolver linksResolver) { - super(endpointMapping, endpoints, endpointMediaTypes, corsConfiguration, true); + super(endpointMapping, endpoints, endpointMediaTypes, corsConfiguration, true, + WebMvcAutoConfiguration.pathPatternParser); this.securityInterceptor = securityInterceptor; this.linksResolver = linksResolver; } diff --git a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/endpoint/web/servlet/WebMvcEndpointManagementContextConfiguration.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/endpoint/web/servlet/WebMvcEndpointManagementContextConfiguration.java index c0ee3e87a4..9985b6e6b1 100644 --- a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/endpoint/web/servlet/WebMvcEndpointManagementContextConfiguration.java +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/endpoint/web/servlet/WebMvcEndpointManagementContextConfiguration.java @@ -45,6 +45,7 @@ import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; import org.springframework.boot.autoconfigure.condition.ConditionalOnWebApplication; import org.springframework.boot.autoconfigure.condition.ConditionalOnWebApplication.Type; +import org.springframework.boot.autoconfigure.web.servlet.WebMvcAutoConfiguration; import org.springframework.boot.context.properties.EnableConfigurationProperties; import org.springframework.context.annotation.Bean; import org.springframework.core.env.Environment; @@ -82,7 +83,7 @@ public class WebMvcEndpointManagementContextConfiguration { boolean shouldRegisterLinksMapping = shouldRegisterLinksMapping(webEndpointProperties, environment, basePath); return new WebMvcEndpointHandlerMapping(endpointMapping, webEndpoints, endpointMediaTypes, corsProperties.toCorsConfiguration(), new EndpointLinksResolver(allEndpoints, basePath), - shouldRegisterLinksMapping); + shouldRegisterLinksMapping, WebMvcAutoConfiguration.pathPatternParser); } private boolean shouldRegisterLinksMapping(WebEndpointProperties webEndpointProperties, Environment environment, diff --git a/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/integrationtest/WebMvcEndpointIntegrationTests.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/integrationtest/WebMvcEndpointIntegrationTests.java index 384d4ef271..afa442cc4f 100644 --- a/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/integrationtest/WebMvcEndpointIntegrationTests.java +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/integrationtest/WebMvcEndpointIntegrationTests.java @@ -33,6 +33,7 @@ import org.springframework.boot.actuate.endpoint.web.EndpointServlet; import org.springframework.boot.actuate.endpoint.web.annotation.ControllerEndpoint; import org.springframework.boot.actuate.endpoint.web.annotation.RestControllerEndpoint; import org.springframework.boot.actuate.endpoint.web.annotation.ServletEndpoint; +import org.springframework.boot.actuate.endpoint.web.servlet.WebMvcEndpointHandlerMapping; import org.springframework.boot.autoconfigure.ImportAutoConfiguration; import org.springframework.boot.autoconfigure.context.PropertyPlaceholderAutoConfiguration; import org.springframework.boot.autoconfigure.data.rest.RepositoryRestMvcAutoConfiguration; @@ -56,6 +57,7 @@ import org.springframework.test.web.servlet.setup.DefaultMockMvcBuilder; import org.springframework.test.web.servlet.setup.MockMvcBuilders; import org.springframework.test.web.servlet.setup.MockMvcConfigurer; +import static org.assertj.core.api.Assertions.assertThat; import static org.hamcrest.Matchers.both; import static org.hamcrest.Matchers.hasKey; import static org.springframework.security.test.web.servlet.setup.SecurityMockMvcConfigurers.springSecurity; @@ -78,6 +80,16 @@ class WebMvcEndpointIntegrationTests { this.context.close(); } + @Test + void webMvcEndpointHandlerMappingIsConfiguredWithPathPatternParser() { + this.context = new AnnotationConfigServletWebApplicationContext(); + this.context.register(DefaultConfiguration.class); + this.context.setServletContext(new MockServletContext()); + this.context.refresh(); + WebMvcEndpointHandlerMapping handlerMapping = this.context.getBean(WebMvcEndpointHandlerMapping.class); + assertThat(handlerMapping.getPatternParser()).isEqualTo(WebMvcAutoConfiguration.pathPatternParser); + } + @Test void endpointsAreSecureByDefault() throws Exception { this.context = new AnnotationConfigServletWebApplicationContext(); diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/web/servlet/AbstractWebMvcEndpointHandlerMapping.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/web/servlet/AbstractWebMvcEndpointHandlerMapping.java index cfd6b139e7..cb984c0dd4 100644 --- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/web/servlet/AbstractWebMvcEndpointHandlerMapping.java +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/web/servlet/AbstractWebMvcEndpointHandlerMapping.java @@ -67,7 +67,7 @@ import org.springframework.web.servlet.handler.MatchableHandlerMapping; import org.springframework.web.servlet.handler.RequestMatchResult; import org.springframework.web.servlet.mvc.method.RequestMappingInfo; import org.springframework.web.servlet.mvc.method.RequestMappingInfoHandlerMapping; -import org.springframework.web.util.UrlPathHelper; +import org.springframework.web.util.pattern.PathPatternParser; /** * A custom {@link HandlerMapping} that makes {@link ExposableWebEndpoint web endpoints} @@ -95,7 +95,7 @@ public abstract class AbstractWebMvcEndpointHandlerMapping extends RequestMappin private final Method handleMethod = ReflectionUtils.findMethod(OperationHandler.class, "handle", HttpServletRequest.class, Map.class); - private static final RequestMappingInfo.BuilderConfiguration builderConfig = getBuilderConfig(); + private RequestMappingInfo.BuilderConfiguration builderConfig = new RequestMappingInfo.BuilderConfiguration(); /** * Creates a new {@code WebEndpointHandlerMapping} that provides mappings for the @@ -123,14 +123,48 @@ public abstract class AbstractWebMvcEndpointHandlerMapping extends RequestMappin public AbstractWebMvcEndpointHandlerMapping(EndpointMapping endpointMapping, Collection endpoints, EndpointMediaTypes endpointMediaTypes, CorsConfiguration corsConfiguration, boolean shouldRegisterLinksMapping) { + this(endpointMapping, endpoints, endpointMediaTypes, corsConfiguration, shouldRegisterLinksMapping, null); + } + + /** + * Creates a new {@code AbstractWebMvcEndpointHandlerMapping} that provides mappings + * for the operations of the given endpoints. + * @param endpointMapping the base mapping for all endpoints + * @param endpoints the web endpoints + * @param endpointMediaTypes media types consumed and produced by the endpoints + * @param corsConfiguration the CORS configuration for the endpoints or {@code null} + * @param shouldRegisterLinksMapping whether the links endpoint should be registered + * @param pathPatternParser the path pattern parser + */ + public AbstractWebMvcEndpointHandlerMapping(EndpointMapping endpointMapping, + Collection endpoints, EndpointMediaTypes endpointMediaTypes, + CorsConfiguration corsConfiguration, boolean shouldRegisterLinksMapping, + PathPatternParser pathPatternParser) { this.endpointMapping = endpointMapping; this.endpoints = endpoints; this.endpointMediaTypes = endpointMediaTypes; this.corsConfiguration = corsConfiguration; this.shouldRegisterLinksMapping = shouldRegisterLinksMapping; + setPatternParser(pathPatternParser); setOrder(-100); } + @Override + @SuppressWarnings("deprecation") + public void afterPropertiesSet() { + this.builderConfig = new RequestMappingInfo.BuilderConfiguration(); + if (getPatternParser() != null) { + this.builderConfig.setPatternParser(getPatternParser()); + } + else { + this.builderConfig.setPathMatcher(null); + this.builderConfig.setTrailingSlashMatch(true); + this.builderConfig.setSuffixPatternMatch(false); + + } + super.afterPropertiesSet(); + } + @Override protected void initHandlerMethods() { for (ExposableWebEndpoint endpoint : this.endpoints) { @@ -151,7 +185,8 @@ public abstract class AbstractWebMvcEndpointHandlerMapping extends RequestMappin @Override public RequestMatchResult match(HttpServletRequest request, String pattern) { - RequestMappingInfo info = RequestMappingInfo.paths(pattern).options(builderConfig).build(); + Assert.isNull(getPatternParser(), "This HandlerMapping uses PathPatterns."); + RequestMappingInfo info = RequestMappingInfo.paths(pattern).options(this.builderConfig).build(); RequestMappingInfo matchingInfo = info.getMatchingCondition(request); if (matchingInfo == null) { return null; @@ -161,15 +196,6 @@ public abstract class AbstractWebMvcEndpointHandlerMapping extends RequestMappin return new RequestMatchResult(patterns.iterator().next(), lookupPath, getPathMatcher()); } - @SuppressWarnings("deprecation") - private static RequestMappingInfo.BuilderConfiguration getBuilderConfig() { - RequestMappingInfo.BuilderConfiguration config = new RequestMappingInfo.BuilderConfiguration(); - config.setPathMatcher(null); - config.setSuffixPatternMatch(false); - config.setTrailingSlashMatch(true); - return config; - } - private void registerMappingForOperation(ExposableWebEndpoint endpoint, WebOperation operation) { WebOperationRequestPredicate predicate = operation.getRequestPredicate(); String path = predicate.getPath(); @@ -202,7 +228,7 @@ public abstract class AbstractWebMvcEndpointHandlerMapping extends RequestMappin } private RequestMappingInfo createRequestMappingInfo(WebOperationRequestPredicate predicate, String path) { - return RequestMappingInfo.paths(this.endpointMapping.createSubPath(path)) + return RequestMappingInfo.paths(this.endpointMapping.createSubPath(path)).options(this.builderConfig) .methods(RequestMethod.valueOf(predicate.getHttpMethod().name())) .consumes(predicate.getConsumes().toArray(new String[0])) .produces(predicate.getProduces().toArray(new String[0])).build(); @@ -211,7 +237,7 @@ public abstract class AbstractWebMvcEndpointHandlerMapping extends RequestMappin private void registerLinksMapping() { RequestMappingInfo mapping = RequestMappingInfo.paths(this.endpointMapping.createSubPath("")) .methods(RequestMethod.GET).produces(this.endpointMediaTypes.getProduced().toArray(new String[0])) - .options(builderConfig).build(); + .options(this.builderConfig).build(); LinksHandler linksHandler = getLinksHandler(); registerMapping(mapping, linksHandler, ReflectionUtils.findMethod(linksHandler.getClass(), "links", HttpServletRequest.class, HttpServletResponse.class)); @@ -335,7 +361,7 @@ public abstract class AbstractWebMvcEndpointHandlerMapping extends RequestMappin } private Object getRemainingPathSegments(HttpServletRequest request) { - String[] pathTokens = tokenize(request, UrlPathHelper.PATH_ATTRIBUTE, true); + String[] pathTokens = tokenize(request, HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE, true); String[] patternTokens = tokenize(request, HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE, false); int numberOfRemainingPathSegments = pathTokens.length - patternTokens.length + 1; Assert.state(numberOfRemainingPathSegments >= 0, "Unable to extract remaining path segments"); diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/web/servlet/WebMvcEndpointHandlerMapping.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/web/servlet/WebMvcEndpointHandlerMapping.java index 350c4a41e9..9834d07127 100644 --- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/web/servlet/WebMvcEndpointHandlerMapping.java +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/web/servlet/WebMvcEndpointHandlerMapping.java @@ -31,6 +31,7 @@ import org.springframework.boot.actuate.endpoint.web.Link; import org.springframework.web.bind.annotation.ResponseBody; import org.springframework.web.cors.CorsConfiguration; import org.springframework.web.servlet.HandlerMapping; +import org.springframework.web.util.pattern.PathPatternParser; /** * A custom {@link HandlerMapping} that makes web endpoints available over HTTP using @@ -62,6 +63,27 @@ public class WebMvcEndpointHandlerMapping extends AbstractWebMvcEndpointHandlerM setOrder(-100); } + /** + * Creates a new {@code WebMvcEndpointHandlerMapping} instance that provides mappings + * for the given endpoints. + * @param endpointMapping the base mapping for all endpoints + * @param endpoints the web endpoints + * @param endpointMediaTypes media types consumed and produced by the endpoints + * @param corsConfiguration the CORS configuration for the endpoints or {@code null} + * @param linksResolver resolver for determining links to available endpoints + * @param shouldRegisterLinksMapping whether the links endpoint should be registered + * @param pathPatternParser the path pattern parser + */ + public WebMvcEndpointHandlerMapping(EndpointMapping endpointMapping, Collection endpoints, + EndpointMediaTypes endpointMediaTypes, CorsConfiguration corsConfiguration, + EndpointLinksResolver linksResolver, boolean shouldRegisterLinksMapping, + PathPatternParser pathPatternParser) { + super(endpointMapping, endpoints, endpointMediaTypes, corsConfiguration, shouldRegisterLinksMapping, + pathPatternParser); + this.linksResolver = linksResolver; + setOrder(-100); + } + @Override protected LinksHandler getLinksHandler() { return new WebMvcLinksHandler(); diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/servlet/MvcWebEndpointIntegrationTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/servlet/MvcWebEndpointIntegrationTests.java index 1c4bda2425..4e74b4114b 100644 --- a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/servlet/MvcWebEndpointIntegrationTests.java +++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/servlet/MvcWebEndpointIntegrationTests.java @@ -56,8 +56,11 @@ import org.springframework.util.StringUtils; import org.springframework.web.cors.CorsConfiguration; import org.springframework.web.filter.OncePerRequestFilter; import org.springframework.web.servlet.handler.RequestMatchResult; +import org.springframework.web.util.ServletRequestPathUtils; +import org.springframework.web.util.pattern.PathPatternParser; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; /** * Integration tests for web endpoints exposed using Spring MVC. @@ -104,24 +107,37 @@ class MvcWebEndpointIntegrationTests }); } + @Test + void matchWhenPathPatternParserShouldThrowException() { + assertThatIllegalArgumentException().isThrownBy(() -> getMatchResult("/spring/", true)); + } + @Test void matchWhenRequestHasTrailingSlashShouldNotBeNull() { - assertThat(getMatchResult("/spring/")).isNotNull(); + assertThat(getMatchResult("/spring/", false)).isNotNull(); } @Test void matchWhenRequestHasSuffixShouldBeNull() { - assertThat(getMatchResult("/spring.do")).isNull(); + assertThat(getMatchResult("/spring.do", false)).isNull(); } - private RequestMatchResult getMatchResult(String servletPath) { + private RequestMatchResult getMatchResult(String servletPath, boolean isPatternParser) { MockHttpServletRequest request = new MockHttpServletRequest(); request.setServletPath(servletPath); - AnnotationConfigServletWebServerApplicationContext context = createApplicationContext(); + AnnotationConfigServletWebServerApplicationContext context = new AnnotationConfigServletWebServerApplicationContext(); + if (isPatternParser) { + context.register(WebMvcConfiguration.class); + } + else { + context.register(PathMatcherWebMvcConfiguration.class); + } context.register(TestEndpointConfiguration.class); context.refresh(); WebMvcEndpointHandlerMapping bean = context.getBean(WebMvcEndpointHandlerMapping.class); try { + // Setup request attributes + ServletRequestPathUtils.parseAndCache(request); // Trigger initLookupPath bean.getHandler(request); } @@ -156,7 +172,35 @@ class MvcWebEndpointIntegrationTests String endpointPath = environment.getProperty("endpointPath"); return new WebMvcEndpointHandlerMapping(new EndpointMapping(endpointPath), endpointDiscoverer.getEndpoints(), endpointMediaTypes, corsConfiguration, - new EndpointLinksResolver(endpointDiscoverer.getEndpoints()), StringUtils.hasText(endpointPath)); + new EndpointLinksResolver(endpointDiscoverer.getEndpoints()), StringUtils.hasText(endpointPath), + new PathPatternParser()); + } + + } + + @Configuration(proxyBeanMethods = false) + @ImportAutoConfiguration({ JacksonAutoConfiguration.class, HttpMessageConvertersAutoConfiguration.class, + ServletWebServerFactoryAutoConfiguration.class, WebMvcAutoConfiguration.class, + DispatcherServletAutoConfiguration.class, ErrorMvcAutoConfiguration.class }) + static class PathMatcherWebMvcConfiguration { + + @Bean + TomcatServletWebServerFactory tomcat() { + return new TomcatServletWebServerFactory(0); + } + + @Bean + WebMvcEndpointHandlerMapping webEndpointHandlerMapping(Environment environment, + WebEndpointDiscoverer endpointDiscoverer, EndpointMediaTypes endpointMediaTypes) { + CorsConfiguration corsConfiguration = new CorsConfiguration(); + corsConfiguration.setAllowedOrigins(Arrays.asList("https://example.com")); + corsConfiguration.setAllowedMethods(Arrays.asList("GET", "POST")); + String endpointPath = environment.getProperty("endpointPath"); + WebMvcEndpointHandlerMapping handlerMapping = new WebMvcEndpointHandlerMapping( + new EndpointMapping(endpointPath), endpointDiscoverer.getEndpoints(), endpointMediaTypes, + corsConfiguration, new EndpointLinksResolver(endpointDiscoverer.getEndpoints()), + StringUtils.hasText(endpointPath)); + return handlerMapping; } } diff --git a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/servlet/WebMvcAutoConfiguration.java b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/servlet/WebMvcAutoConfiguration.java index 7517e992b3..114204bf0c 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/servlet/WebMvcAutoConfiguration.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/servlet/WebMvcAutoConfiguration.java @@ -158,6 +158,11 @@ public class WebMvcAutoConfiguration { */ public static final String DEFAULT_SUFFIX = ""; + /** + * Instance of {@link PathPatternParser} shared across MVC and actuator configuration. + */ + public static final PathPatternParser pathPatternParser = new PathPatternParser(); + private static final String SERVLET_LOCATION = "/"; @Bean @@ -246,7 +251,7 @@ public class WebMvcAutoConfiguration { public void configurePathMatch(PathMatchConfigurer configurer) { if (this.mvcProperties.getPathmatch() .getMatchingStrategy() == WebMvcProperties.MatchingStrategy.PATH_PATTERN_PARSER) { - configurer.setPatternParser(new PathPatternParser()); + configurer.setPatternParser(pathPatternParser); } configurer.setUseSuffixPatternMatch(this.mvcProperties.getPathmatch().isUseSuffixPattern()); configurer.setUseRegisteredSuffixPatternMatch(