From 70dfb3d391210eb5e86f8285990df90a524159ca Mon Sep 17 00:00:00 2001 From: Rob Winch Date: Thu, 16 Nov 2023 13:14:04 -0600 Subject: [PATCH] Add HandlerMappingIntrospector Caching Closes gh-14128 --- .../annotation/web/builders/WebSecurity.java | 9 + .../WebMvcSecurityConfiguration.java | 162 ++++++++++++++ .../config/http/HttpConfigurationBuilder.java | 47 +++- .../web/builders/WebSecurityTests.java | 4 +- ...vocationPrivilegeEvaluatorConfigTests.java | 105 +++++++++ ...ingIntrospectorCacheFilterConfigTests.java | 127 +++++++++++ ...anagerWebInvocationPrivilegeEvaluator.java | 36 ++- ...MappingIntrospectorRequestTransformer.java | 96 ++++++++ ...rWebInvocationPrivilegeEvaluatorTests.java | 22 ++ ...ngIntrospectorRequestTransformerTests.java | 206 ++++++++++++++++++ 10 files changed, 807 insertions(+), 7 deletions(-) create mode 100644 config/src/test/java/org/springframework/security/config/annotation/web/configuration/AuthorizationManagerWebInvocationPrivilegeEvaluatorConfigTests.java create mode 100644 config/src/test/java/org/springframework/security/config/annotation/web/configuration/HandlerMappingIntrospectorCacheFilterConfigTests.java create mode 100644 web/src/main/java/org/springframework/security/web/access/HandlerMappingIntrospectorRequestTransformer.java create mode 100644 web/src/test/java/org/springframework/security/web/access/HandlerMappingIntrospectorRequestTransformerTests.java diff --git a/config/src/main/java/org/springframework/security/config/annotation/web/builders/WebSecurity.java b/config/src/main/java/org/springframework/security/config/annotation/web/builders/WebSecurity.java index 6f3e966ef0..504322f5a2 100644 --- a/config/src/main/java/org/springframework/security/config/annotation/web/builders/WebSecurity.java +++ b/config/src/main/java/org/springframework/security/config/annotation/web/builders/WebSecurity.java @@ -49,6 +49,7 @@ import org.springframework.security.web.FilterInvocation; import org.springframework.security.web.ObservationFilterChainDecorator; import org.springframework.security.web.SecurityFilterChain; import org.springframework.security.web.access.AuthorizationManagerWebInvocationPrivilegeEvaluator; +import org.springframework.security.web.access.AuthorizationManagerWebInvocationPrivilegeEvaluator.HttpServletRequestTransformer; import org.springframework.security.web.access.DefaultWebInvocationPrivilegeEvaluator; import org.springframework.security.web.access.RequestMatcherDelegatingWebInvocationPrivilegeEvaluator; import org.springframework.security.web.access.WebInvocationPrivilegeEvaluator; @@ -108,6 +109,8 @@ public final class WebSecurity extends AbstractConfiguredSecurityBuilder expressionHandler = this.defaultWebSecurityExpressionHandler; @@ -350,6 +353,9 @@ public final class WebSecurity extends AbstractConfiguredSecurityBuilder requestTransformerClass = HttpServletRequestTransformer.class; + this.privilegeEvaluatorRequestTransformer = applicationContext.getBeanProvider(requestTransformerClass) + .getIfUnique(); } @Override diff --git a/config/src/main/java/org/springframework/security/config/annotation/web/configuration/WebMvcSecurityConfiguration.java b/config/src/main/java/org/springframework/security/config/annotation/web/configuration/WebMvcSecurityConfiguration.java index 0423767e8b..9a2fd125c8 100644 --- a/config/src/main/java/org/springframework/security/config/annotation/web/configuration/WebMvcSecurityConfiguration.java +++ b/config/src/main/java/org/springframework/security/config/annotation/web/configuration/WebMvcSecurityConfiguration.java @@ -18,7 +18,19 @@ package org.springframework.security.config.annotation.web.configuration; import java.util.List; +import jakarta.servlet.Filter; +import jakarta.servlet.http.HttpServletRequest; + +import org.springframework.beans.BeanMetadataElement; import org.springframework.beans.BeansException; +import org.springframework.beans.factory.FactoryBean; +import org.springframework.beans.factory.config.BeanDefinition; +import org.springframework.beans.factory.config.ConfigurableListableBeanFactory; +import org.springframework.beans.factory.config.RuntimeBeanReference; +import org.springframework.beans.factory.support.BeanDefinitionBuilder; +import org.springframework.beans.factory.support.BeanDefinitionRegistry; +import org.springframework.beans.factory.support.BeanDefinitionRegistryPostProcessor; +import org.springframework.beans.factory.support.ManagedList; import org.springframework.context.ApplicationContext; import org.springframework.context.ApplicationContextAware; import org.springframework.context.annotation.Bean; @@ -26,13 +38,19 @@ import org.springframework.context.expression.BeanFactoryResolver; import org.springframework.expression.BeanResolver; import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.security.core.context.SecurityContextHolderStrategy; +import org.springframework.security.web.FilterChainProxy; +import org.springframework.security.web.SecurityFilterChain; +import org.springframework.security.web.access.HandlerMappingIntrospectorRequestTransformer; +import org.springframework.security.web.context.AbstractSecurityWebApplicationInitializer; import org.springframework.security.web.method.annotation.AuthenticationPrincipalArgumentResolver; import org.springframework.security.web.method.annotation.CsrfTokenArgumentResolver; import org.springframework.security.web.method.annotation.CurrentSecurityContextArgumentResolver; import org.springframework.security.web.servlet.support.csrf.CsrfRequestDataValueProcessor; +import org.springframework.web.filter.CompositeFilter; import org.springframework.web.method.support.HandlerMethodArgumentResolver; import org.springframework.web.servlet.config.annotation.EnableWebMvc; import org.springframework.web.servlet.config.annotation.WebMvcConfigurer; +import org.springframework.web.servlet.handler.HandlerMappingIntrospector; import org.springframework.web.servlet.support.RequestDataValueProcessor; /** @@ -50,6 +68,8 @@ import org.springframework.web.servlet.support.RequestDataValueProcessor; */ class WebMvcSecurityConfiguration implements WebMvcConfigurer, ApplicationContextAware { + private static final String HANDLER_MAPPING_INTROSPECTOR_BEAN_NAME = "mvcHandlerMappingIntrospector"; + private BeanResolver beanResolver; private SecurityContextHolderStrategy securityContextHolderStrategy = SecurityContextHolder @@ -84,4 +104,146 @@ class WebMvcSecurityConfiguration implements WebMvcConfigurer, ApplicationContex } } + /** + * Used to ensure Spring MVC request matching is cached. + * + * Creates a {@link BeanDefinitionRegistryPostProcessor} that detects if a bean named + * HANDLER_MAPPING_INTROSPECTOR_BEAN_NAME is defined. If so, it moves the + * AbstractSecurityWebApplicationInitializer.DEFAULT_FILTER_NAME to another bean name + * and then adds a {@link CompositeFilter} that contains + * {@link HandlerMappingIntrospector#createCacheFilter()} and the original + * FilterChainProxy under the original Bean name. + * @return + */ + @Bean + static BeanDefinitionRegistryPostProcessor springSecurityHandlerMappingIntrospectorBeanDefinitionRegistryPostProcessor() { + return new BeanDefinitionRegistryPostProcessor() { + @Override + public void postProcessBeanFactory(ConfigurableListableBeanFactory beanFactory) throws BeansException { + } + + @Override + public void postProcessBeanDefinitionRegistry(BeanDefinitionRegistry registry) throws BeansException { + if (!registry.containsBeanDefinition(HANDLER_MAPPING_INTROSPECTOR_BEAN_NAME)) { + return; + } + + BeanDefinition hmiRequestTransformer = BeanDefinitionBuilder + .rootBeanDefinition(HandlerMappingIntrospectorRequestTransformer.class) + .addConstructorArgReference(HANDLER_MAPPING_INTROSPECTOR_BEAN_NAME) + .getBeanDefinition(); + registry.registerBeanDefinition(HANDLER_MAPPING_INTROSPECTOR_BEAN_NAME + "RequestTransformer", + hmiRequestTransformer); + + String filterChainProxyBeanName = AbstractSecurityWebApplicationInitializer.DEFAULT_FILTER_NAME + + "Proxy"; + BeanDefinition filterChainProxy = registry + .getBeanDefinition(AbstractSecurityWebApplicationInitializer.DEFAULT_FILTER_NAME); + registry.registerBeanDefinition(filterChainProxyBeanName, filterChainProxy); + + BeanDefinitionBuilder hmiCacheFilterBldr = BeanDefinitionBuilder + .rootBeanDefinition(HandlerMappingIntrospectorCachFilterFactoryBean.class) + .setRole(BeanDefinition.ROLE_INFRASTRUCTURE); + + ManagedList filters = new ManagedList<>(); + filters.add(hmiCacheFilterBldr.getBeanDefinition()); + filters.add(new RuntimeBeanReference(filterChainProxyBeanName)); + BeanDefinitionBuilder compositeSpringSecurityFilterChainBldr = BeanDefinitionBuilder + .rootBeanDefinition(SpringSecurityFilterCompositeFilter.class) + .addConstructorArgValue(filters); + + registry.removeBeanDefinition(AbstractSecurityWebApplicationInitializer.DEFAULT_FILTER_NAME); + registry.registerBeanDefinition(AbstractSecurityWebApplicationInitializer.DEFAULT_FILTER_NAME, + compositeSpringSecurityFilterChainBldr.getBeanDefinition()); + } + }; + } + + /** + * {@link FactoryBean} to defer creation of + * {@link HandlerMappingIntrospector#createCacheFilter()} + */ + static class HandlerMappingIntrospectorCachFilterFactoryBean + implements ApplicationContextAware, FactoryBean { + + private ApplicationContext applicationContext; + + @Override + public void setApplicationContext(ApplicationContext applicationContext) { + this.applicationContext = applicationContext; + } + + @Override + public Filter getObject() throws Exception { + HandlerMappingIntrospector handlerMappingIntrospector = this.applicationContext + .getBean(HANDLER_MAPPING_INTROSPECTOR_BEAN_NAME, HandlerMappingIntrospector.class); + return handlerMappingIntrospector.createCacheFilter(); + } + + @Override + public Class getObjectType() { + return Filter.class; + } + + } + + /** + * Extension to {@link CompositeFilter} to expose private methods used by Spring + * Security's test support + */ + static class SpringSecurityFilterCompositeFilter extends CompositeFilter { + + private FilterChainProxy springSecurityFilterChain; + + SpringSecurityFilterCompositeFilter(List filters) { + setFilters(filters); // for the parent + } + + @Override + public void setFilters(List filters) { + super.setFilters(filters); + this.springSecurityFilterChain = findFilterChainProxy(filters); + } + + /** + * Used through reflection by Spring Security's Test support to lookup the + * FilterChainProxy Filters for a specific HttpServletRequest. + * @param request + * @return + */ + private List getFilters(HttpServletRequest request) { + List filterChains = getFilterChainProxy().getFilterChains(); + for (SecurityFilterChain chain : filterChains) { + if (chain.matches(request)) { + return chain.getFilters(); + } + } + return null; + } + + /** + * Used by Spring Security's Test support to find the FilterChainProxy + * @return + */ + private FilterChainProxy getFilterChainProxy() { + return this.springSecurityFilterChain; + } + + /** + * Find the FilterChainProxy in a List of Filter + * @param filters + * @return non-null FilterChainProxy + * @throws IllegalStateException if the FilterChainProxy cannot be found + */ + private static FilterChainProxy findFilterChainProxy(List filters) { + for (Filter filter : filters) { + if (filter instanceof FilterChainProxy fcp) { + return fcp; + } + } + throw new IllegalStateException("Couldn't find FilterChainProxy in " + filters); + } + + } + } diff --git a/config/src/main/java/org/springframework/security/config/http/HttpConfigurationBuilder.java b/config/src/main/java/org/springframework/security/config/http/HttpConfigurationBuilder.java index 7de81bde31..e8526a3540 100644 --- a/config/src/main/java/org/springframework/security/config/http/HttpConfigurationBuilder.java +++ b/config/src/main/java/org/springframework/security/config/http/HttpConfigurationBuilder.java @@ -24,6 +24,7 @@ import jakarta.servlet.ServletRequest; import org.w3c.dom.Element; import org.springframework.beans.BeanMetadataElement; +import org.springframework.beans.BeansException; import org.springframework.beans.factory.FactoryBean; import org.springframework.beans.factory.config.BeanDefinition; import org.springframework.beans.factory.config.BeanReference; @@ -36,6 +37,8 @@ import org.springframework.beans.factory.support.ManagedList; import org.springframework.beans.factory.support.ManagedMap; import org.springframework.beans.factory.support.RootBeanDefinition; import org.springframework.beans.factory.xml.ParserContext; +import org.springframework.context.ApplicationContext; +import org.springframework.context.ApplicationContextAware; import org.springframework.security.access.vote.AffirmativeBased; import org.springframework.security.access.vote.AuthenticatedVoter; import org.springframework.security.access.vote.RoleVoter; @@ -46,6 +49,7 @@ import org.springframework.security.core.context.SecurityContextHolderStrategy; import org.springframework.security.core.session.SessionRegistryImpl; import org.springframework.security.web.access.AuthorizationManagerWebInvocationPrivilegeEvaluator; import org.springframework.security.web.access.DefaultWebInvocationPrivilegeEvaluator; +import org.springframework.security.web.access.HandlerMappingIntrospectorRequestTransformer; import org.springframework.security.web.access.channel.ChannelDecisionManagerImpl; import org.springframework.security.web.access.channel.ChannelProcessingFilter; import org.springframework.security.web.access.channel.InsecureChannelProcessor; @@ -82,6 +86,7 @@ import org.springframework.util.Assert; import org.springframework.util.ClassUtils; import org.springframework.util.StringUtils; import org.springframework.util.xml.DomUtils; +import org.springframework.web.servlet.handler.HandlerMappingIntrospector; /** * Stateful class which helps HttpSecurityBDP to create the configuration for the @@ -93,6 +98,11 @@ import org.springframework.util.xml.DomUtils; */ class HttpConfigurationBuilder { + private static final String HANDLER_MAPPING_INTROSPECTOR = "org.springframework.web.servlet.handler.HandlerMappingIntrospector"; + + private static final boolean mvcPresent = ClassUtils.isPresent(HANDLER_MAPPING_INTROSPECTOR, + HttpConfigurationBuilder.class.getClassLoader()); + private static final String ATT_CREATE_SESSION = "create-session"; private static final String ATT_SESSION_FIXATION_PROTECTION = "session-fixation-protection"; @@ -744,10 +754,14 @@ class HttpConfigurationBuilder { // Create and register a AuthorizationManagerWebInvocationPrivilegeEvaluator for // use with // taglibs etc. - BeanDefinition wipe = BeanDefinitionBuilder + BeanDefinitionBuilder wipeBldr = BeanDefinitionBuilder .rootBeanDefinition(AuthorizationManagerWebInvocationPrivilegeEvaluator.class) - .addConstructorArgReference(authorizationFilterParser.getAuthorizationManagerRef()) - .getBeanDefinition(); + .addConstructorArgReference(authorizationFilterParser.getAuthorizationManagerRef()); + if (mvcPresent) { + wipeBldr.addPropertyValue("requestTransformer", + new RootBeanDefinition(HandlerMappingIntrospectorRequestTransformerFactoryBean.class)); + } + BeanDefinition wipe = wipeBldr.getBeanDefinition(); this.pc.registerBeanComponent( new BeanComponentDefinition(wipe, this.pc.getReaderContext().generateBeanName(wipe))); this.fsi = new RuntimeBeanReference(fsiId); @@ -913,6 +927,33 @@ class HttpConfigurationBuilder { return BeanDefinitionBuilder.rootBeanDefinition(ObservationRegistryFactory.class).getBeanDefinition(); } + static class HandlerMappingIntrospectorRequestTransformerFactoryBean + implements FactoryBean, + ApplicationContextAware { + + private ApplicationContext applicationContext; + + @Override + public AuthorizationManagerWebInvocationPrivilegeEvaluator.HttpServletRequestTransformer getObject() + throws Exception { + HandlerMappingIntrospector hmi = this.applicationContext.getBeanProvider(HandlerMappingIntrospector.class) + .getIfAvailable(); + return (hmi != null) ? new HandlerMappingIntrospectorRequestTransformer(hmi) + : AuthorizationManagerWebInvocationPrivilegeEvaluator.HttpServletRequestTransformer.IDENTITY; + } + + @Override + public Class getObjectType() { + return AuthorizationManagerWebInvocationPrivilegeEvaluator.HttpServletRequestTransformer.class; + } + + @Override + public void setApplicationContext(ApplicationContext applicationContext) throws BeansException { + this.applicationContext = applicationContext; + } + + } + static class RoleVoterBeanFactory extends AbstractGrantedAuthorityDefaultsBeanFactory { private RoleVoter voter = new RoleVoter(); diff --git a/config/src/test/java/org/springframework/security/config/annotation/web/builders/WebSecurityTests.java b/config/src/test/java/org/springframework/security/config/annotation/web/builders/WebSecurityTests.java index 46c837312b..3219676502 100644 --- a/config/src/test/java/org/springframework/security/config/annotation/web/builders/WebSecurityTests.java +++ b/config/src/test/java/org/springframework/security/config/annotation/web/builders/WebSecurityTests.java @@ -20,6 +20,7 @@ import java.io.IOException; import io.micrometer.observation.ObservationRegistry; import io.micrometer.observation.ObservationTextPublisher; +import jakarta.servlet.Filter; import jakarta.servlet.ServletException; import jakarta.servlet.http.HttpServletResponse; import org.junit.jupiter.api.AfterEach; @@ -39,7 +40,6 @@ import org.springframework.security.config.annotation.web.configuration.WebSecur import org.springframework.security.core.userdetails.PasswordEncodedUser; 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.security.web.firewall.HttpStatusRequestRejectedHandler; import org.springframework.security.web.servlet.util.matcher.MvcRequestMatcher; @@ -67,7 +67,7 @@ public class WebSecurityTests { MockFilterChain chain; @Autowired - FilterChainProxy springSecurityFilterChain; + Filter springSecurityFilterChain; @BeforeEach public void setup() { diff --git a/config/src/test/java/org/springframework/security/config/annotation/web/configuration/AuthorizationManagerWebInvocationPrivilegeEvaluatorConfigTests.java b/config/src/test/java/org/springframework/security/config/annotation/web/configuration/AuthorizationManagerWebInvocationPrivilegeEvaluatorConfigTests.java new file mode 100644 index 0000000000..90297c75ef --- /dev/null +++ b/config/src/test/java/org/springframework/security/config/annotation/web/configuration/AuthorizationManagerWebInvocationPrivilegeEvaluatorConfigTests.java @@ -0,0 +1,105 @@ +/* + * Copyright 2002-2023 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. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.security.config.annotation.web.configuration; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; + +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.security.authentication.TestAuthentication; +import org.springframework.security.config.test.SpringTestContext; +import org.springframework.security.test.context.annotation.SecurityTestExecutionListeners; +import org.springframework.security.web.access.AuthorizationManagerWebInvocationPrivilegeEvaluator.HttpServletRequestTransformer; +import org.springframework.security.web.access.HandlerMappingIntrospectorRequestTransformer; +import org.springframework.security.web.access.WebInvocationPrivilegeEvaluator; +import org.springframework.test.context.ContextConfiguration; +import org.springframework.test.context.junit.jupiter.SpringExtension; +import org.springframework.test.context.web.WebAppConfiguration; +import org.springframework.web.servlet.config.annotation.EnableWebMvc; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; + +/** + * Checks that HandlerMappingIntrospectorRequestTransformer is autowired into + * {@link org.springframework.security.web.access.AuthorizationManagerWebInvocationPrivilegeEvaluator}. + * + * @author Rob Winch + */ +@ContextConfiguration +@WebAppConfiguration +@ExtendWith({ SpringExtension.class }) +@SecurityTestExecutionListeners +public class AuthorizationManagerWebInvocationPrivilegeEvaluatorConfigTests { + + public final SpringTestContext spring = new SpringTestContext(this); + + @Autowired(required = false) + HttpServletRequestTransformer requestTransformer; + + @Autowired + WebInvocationPrivilegeEvaluator wipe; + + @Test + void mvcEnabledConfigThenHandlerMappingIntrospectorRequestTransformerBeanExists() { + this.spring.register(MvcEnabledConfig.class).autowire(); + assertThat(this.requestTransformer).isInstanceOf(HandlerMappingIntrospectorRequestTransformer.class); + } + + @Test + void mvcNotEnabledThenNoRequestTransformerBeanExists() { + this.spring.register(MvcNotEnabledConfig.class).autowire(); + assertThat(this.requestTransformer).isNull(); + } + + @Test + void mvcNotEnabledAndTransformerThenWIPEDelegatesToTransformer() { + this.spring.register(MvcNotEnabledConfig.class, TransformerConfig.class).autowire(); + + this.wipe.isAllowed("/uri", TestAuthentication.authenticatedUser()); + + verify(this.requestTransformer).transform(any()); + } + + @Configuration + static class TransformerConfig { + + @Bean + HttpServletRequestTransformer httpServletRequestTransformer() { + return mock(HttpServletRequestTransformer.class); + } + + } + + @Configuration + @EnableWebMvc + @EnableWebSecurity + static class MvcEnabledConfig { + + } + + @Configuration + @EnableWebSecurity + static class MvcNotEnabledConfig { + + } + +} diff --git a/config/src/test/java/org/springframework/security/config/annotation/web/configuration/HandlerMappingIntrospectorCacheFilterConfigTests.java b/config/src/test/java/org/springframework/security/config/annotation/web/configuration/HandlerMappingIntrospectorCacheFilterConfigTests.java new file mode 100644 index 0000000000..865b99ee21 --- /dev/null +++ b/config/src/test/java/org/springframework/security/config/annotation/web/configuration/HandlerMappingIntrospectorCacheFilterConfigTests.java @@ -0,0 +1,127 @@ +/* + * Copyright 2002-2023 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. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.security.config.annotation.web.configuration; + +import java.io.IOException; + +import jakarta.servlet.Filter; +import jakarta.servlet.FilterChain; +import jakarta.servlet.ServletException; +import jakarta.servlet.ServletRequest; +import jakarta.servlet.ServletResponse; +import jakarta.servlet.http.HttpServletRequest; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; + +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.context.annotation.Configuration; +import org.springframework.security.config.test.SpringTestContext; +import org.springframework.security.test.context.annotation.SecurityTestExecutionListeners; +import org.springframework.security.test.context.support.WithMockUser; +import org.springframework.stereotype.Component; +import org.springframework.test.context.ContextConfiguration; +import org.springframework.test.context.junit.jupiter.SpringExtension; +import org.springframework.test.context.web.WebAppConfiguration; +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.config.annotation.EnableWebMvc; +import org.springframework.web.servlet.handler.HandlerMappingIntrospector; +import org.springframework.web.servlet.handler.HandlerMappingIntrospector.CachedResult; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.springframework.security.test.web.servlet.setup.SecurityMockMvcConfigurers.springSecurity; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; + +/** + * @author Rob Winch + */ +@ContextConfiguration +@WebAppConfiguration +@ExtendWith({ SpringExtension.class }) +@SecurityTestExecutionListeners +class HandlerMappingIntrospectorCacheFilterConfigTests { + + @Autowired + WebApplicationContext context; + + MockMvc mockMvc; + + public final SpringTestContext spring = new SpringTestContext(this); + + @Autowired(required = false) + MvcEnabledConfig.CaptureHandlerMappingIntrospectorCache captureCacheFilter; + + @Autowired(required = false) + HandlerMappingIntrospector hmi; + + @Test + @WithMockUser + void hmiIsCached() throws Exception { + this.spring.register(MvcEnabledConfig.class).autowire(); + this.mockMvc = MockMvcBuilders.webAppContextSetup(this.context) + .apply(springSecurity()) + .addFilter(this.captureCacheFilter) + .build(); + this.mockMvc.perform(get("/")); + assertThat(this.captureCacheFilter.cachedResult).isNotNull(); + } + + @Test + @WithMockUser + void configurationLoadsIfNoHMI() { + // no BeanCreationException due to missing HandlerMappingIntrospector + this.spring.register(MvcNotEnabledConfig.class).autowire(); + // ensure assumption of HandlerMappingIntrospector is null is true + assertThat(this.hmi).isNull(); + } + + @Configuration + @EnableWebMvc + @EnableWebSecurity + static class MvcEnabledConfig { + + @Component + static class CaptureHandlerMappingIntrospectorCache implements Filter { + + final HandlerMappingIntrospector hmi; + + private CachedResult cachedResult; + + CaptureHandlerMappingIntrospectorCache(HandlerMappingIntrospector hmi) { + this.hmi = hmi; + } + + @Override + public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) + throws IOException, ServletException { + // capture the old cached value to check that caching has already occurred + this.cachedResult = this.hmi.setCache((HttpServletRequest) request); + chain.doFilter(request, response); + } + + } + + } + + @Configuration + @EnableWebSecurity + static class MvcNotEnabledConfig { + + } + +} diff --git a/web/src/main/java/org/springframework/security/web/access/AuthorizationManagerWebInvocationPrivilegeEvaluator.java b/web/src/main/java/org/springframework/security/web/access/AuthorizationManagerWebInvocationPrivilegeEvaluator.java index 4dbc5ef546..e36a54fda6 100644 --- a/web/src/main/java/org/springframework/security/web/access/AuthorizationManagerWebInvocationPrivilegeEvaluator.java +++ b/web/src/main/java/org/springframework/security/web/access/AuthorizationManagerWebInvocationPrivilegeEvaluator.java @@ -40,6 +40,8 @@ public final class AuthorizationManagerWebInvocationPrivilegeEvaluator private ServletContext servletContext; + private HttpServletRequestTransformer requestTransformer = HttpServletRequestTransformer.IDENTITY; + public AuthorizationManagerWebInvocationPrivilegeEvaluator( AuthorizationManager authorizationManager) { Assert.notNull(authorizationManager, "authorizationManager cannot be null"); @@ -54,8 +56,8 @@ public final class AuthorizationManagerWebInvocationPrivilegeEvaluator @Override public boolean isAllowed(String contextPath, String uri, String method, Authentication authentication) { FilterInvocation filterInvocation = new FilterInvocation(contextPath, uri, method, this.servletContext); - AuthorizationDecision decision = this.authorizationManager.check(() -> authentication, - filterInvocation.getHttpRequest()); + HttpServletRequest httpRequest = this.requestTransformer.transform(filterInvocation.getHttpRequest()); + AuthorizationDecision decision = this.authorizationManager.check(() -> authentication, httpRequest); return decision == null || decision.isGranted(); } @@ -64,4 +66,34 @@ public final class AuthorizationManagerWebInvocationPrivilegeEvaluator this.servletContext = servletContext; } + /** + * Set a {@link HttpServletRequestTransformer} to be used prior to passing to the + * {@link AuthorizationManager}. + * @param requestTransformer the {@link HttpServletRequestTransformer} to use. + */ + public void setRequestTransformer(HttpServletRequestTransformer requestTransformer) { + Assert.notNull(requestTransformer, "requestTransformer cannot be null"); + this.requestTransformer = requestTransformer; + } + + /** + * Used to transform the {@link HttpServletRequest} prior to passing it into the + * {@link AuthorizationManager}. + */ + public interface HttpServletRequestTransformer { + + HttpServletRequestTransformer IDENTITY = (request) -> request; + + /** + * Return the {@link HttpServletRequest} that is passed into the + * {@link AuthorizationManager} + * @param request the {@link HttpServletRequest} created by the + * {@link WebInvocationPrivilegeEvaluator} + * @return the {@link HttpServletRequest} that is passed into the + * {@link AuthorizationManager} + */ + HttpServletRequest transform(HttpServletRequest request); + + } + } diff --git a/web/src/main/java/org/springframework/security/web/access/HandlerMappingIntrospectorRequestTransformer.java b/web/src/main/java/org/springframework/security/web/access/HandlerMappingIntrospectorRequestTransformer.java new file mode 100644 index 0000000000..42cf51c3c3 --- /dev/null +++ b/web/src/main/java/org/springframework/security/web/access/HandlerMappingIntrospectorRequestTransformer.java @@ -0,0 +1,96 @@ +/* + * Copyright 2002-2023 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. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.security.web.access; + +import java.util.Collections; +import java.util.Enumeration; +import java.util.HashMap; +import java.util.Map; + +import jakarta.servlet.DispatcherType; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletRequestWrapper; + +import org.springframework.util.Assert; +import org.springframework.web.servlet.handler.HandlerMappingIntrospector; + +/** + * Transforms by passing it into + * {@link HandlerMappingIntrospector#setCache(HttpServletRequest)}. Before, it wraps the + * {@link HttpServletRequest} to ensure that the methods needed work since some methods by + * default throw {@link UnsupportedOperationException}. + * + * @author Rob Winch + */ +public class HandlerMappingIntrospectorRequestTransformer + implements AuthorizationManagerWebInvocationPrivilegeEvaluator.HttpServletRequestTransformer { + + private final HandlerMappingIntrospector introspector; + + public HandlerMappingIntrospectorRequestTransformer(HandlerMappingIntrospector introspector) { + Assert.notNull(introspector, "introspector canot be null"); + this.introspector = introspector; + } + + @Override + public HttpServletRequest transform(HttpServletRequest request) { + CacheableRequestWrapper cacheableRequest = new CacheableRequestWrapper(request); + this.introspector.setCache(cacheableRequest); + return cacheableRequest; + } + + static final class CacheableRequestWrapper extends HttpServletRequestWrapper { + + private final Map attributes = new HashMap<>(); + + /** + * Constructs a request object wrapping the given request. + * @param request the {@link HttpServletRequest} to be wrapped. + * @throws IllegalArgumentException if the request is null + */ + CacheableRequestWrapper(HttpServletRequest request) { + super(request); + } + + @Override + public DispatcherType getDispatcherType() { + return DispatcherType.REQUEST; + } + + @Override + public Enumeration getAttributeNames() { + return Collections.enumeration(this.attributes.keySet()); + } + + @Override + public Object getAttribute(String name) { + return this.attributes.get(name); + } + + @Override + public void setAttribute(String name, Object o) { + this.attributes.put(name, o); + } + + @Override + public void removeAttribute(String name) { + this.attributes.remove(name); + } + + } + +} diff --git a/web/src/test/java/org/springframework/security/web/access/AuthorizationManagerWebInvocationPrivilegeEvaluatorTests.java b/web/src/test/java/org/springframework/security/web/access/AuthorizationManagerWebInvocationPrivilegeEvaluatorTests.java index a2b48d357e..c5cb7669c1 100644 --- a/web/src/test/java/org/springframework/security/web/access/AuthorizationManagerWebInvocationPrivilegeEvaluatorTests.java +++ b/web/src/test/java/org/springframework/security/web/access/AuthorizationManagerWebInvocationPrivilegeEvaluatorTests.java @@ -25,14 +25,17 @@ import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockServletContext; import org.springframework.security.authentication.TestAuthentication; import org.springframework.security.authorization.AuthorizationDecision; import org.springframework.security.authorization.AuthorizationManager; +import org.springframework.security.web.access.AuthorizationManagerWebInvocationPrivilegeEvaluator.HttpServletRequestTransformer; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.verify; @@ -45,6 +48,9 @@ class AuthorizationManagerWebInvocationPrivilegeEvaluatorTests { @Mock private AuthorizationManager authorizationManager; + @Mock + private HttpServletRequestTransformer requestTransformer; + @Test void constructorWhenAuthorizationManagerNullThenIllegalArgument() { assertThatIllegalArgumentException() @@ -84,4 +90,20 @@ class AuthorizationManagerWebInvocationPrivilegeEvaluatorTests { assertThat(captor.getValue().getServletContext()).isSameAs(servletContext); } + @Test + void setRequestTransformerWhenNullThenIllegalArgumentException() { + assertThatIllegalArgumentException().isThrownBy(() -> this.privilegeEvaluator.setRequestTransformer(null)); + } + + @Test + void isAllowedWhenRequestTransformerThenUsesRequestTransformerResult() { + HttpServletRequest request = new MockHttpServletRequest(); + given(this.requestTransformer.transform(any())).willReturn(request); + this.privilegeEvaluator.setRequestTransformer(this.requestTransformer); + + this.privilegeEvaluator.isAllowed("/test", TestAuthentication.authenticatedUser()); + + verify(this.authorizationManager).check(any(), eq(request)); + } + } diff --git a/web/src/test/java/org/springframework/security/web/access/HandlerMappingIntrospectorRequestTransformerTests.java b/web/src/test/java/org/springframework/security/web/access/HandlerMappingIntrospectorRequestTransformerTests.java new file mode 100644 index 0000000000..96bf8e9581 --- /dev/null +++ b/web/src/test/java/org/springframework/security/web/access/HandlerMappingIntrospectorRequestTransformerTests.java @@ -0,0 +1,206 @@ +/* + * Copyright 2002-2023 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. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.security.web.access; + +import java.util.Collections; + +import jakarta.servlet.DispatcherType; +import jakarta.servlet.http.HttpServletRequest; +import org.assertj.core.api.AssertionsForClassTypes; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.web.servlet.handler.HandlerMappingIntrospector; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; + +/** + * @author Rob Winch + */ +@ExtendWith(MockitoExtension.class) +class HandlerMappingIntrospectorRequestTransformerTests { + + @Mock + HandlerMappingIntrospector hmi; + + HandlerMappingIntrospectorRequestTransformer transformer; + + @BeforeEach + void setup() { + this.transformer = new HandlerMappingIntrospectorRequestTransformer(this.hmi); + } + + @Test + void constructorWhenHmiIsNullThenIllegalArgumentException() { + AssertionsForClassTypes.assertThatExceptionOfType(IllegalArgumentException.class) + .isThrownBy(() -> new HandlerMappingIntrospectorRequestTransformer(null)); + } + + @Test + void transformThenNewRequestPassedToSetCache() { + MockHttpServletRequest request = new MockHttpServletRequest(); + + HttpServletRequest transformedRequest = this.transformer.transform(request); + + ArgumentCaptor requestArg = ArgumentCaptor.forClass(HttpServletRequest.class); + verify(this.hmi).setCache(requestArg.capture()); + assertThat(transformedRequest).isNotEqualTo(request); + } + + @Test + void transformThenResultPassedToSetCache() { + MockHttpServletRequest request = new MockHttpServletRequest(); + + HttpServletRequest transformedRequest = this.transformer.transform(request); + + ArgumentCaptor requestArg = ArgumentCaptor.forClass(HttpServletRequest.class); + verify(this.hmi).setCache(requestArg.capture()); + assertThat(requestArg.getValue()).isEqualTo(transformedRequest); + } + + /** + * The request passed into the transformer does not allow interactions on certain + * methods, we need to ensure that the methods used by + * {@link HandlerMappingIntrospector#setCache(HttpServletRequest)} are overridden. + */ + @Test + void transformThenResultDoesNotDelegateToSetAttribute() { + HttpServletRequest request = mock(HttpServletRequest.class); + + this.transformer.transform(request); + + ArgumentCaptor requestArg = ArgumentCaptor.forClass(HttpServletRequest.class); + verify(this.hmi).setCache(requestArg.capture()); + HttpServletRequest transformedRequest = requestArg.getValue(); + String attrName = "any"; + String attrValue = "value"; + transformedRequest.setAttribute(attrName, attrValue); + verifyNoInteractions(request); + assertThat(transformedRequest.getAttribute(attrName)).isEqualTo(attrValue); + } + + @Test + void transformThenSetAttributeWorks() { + HttpServletRequest request = mock(HttpServletRequest.class); + + this.transformer.transform(request); + + ArgumentCaptor requestArg = ArgumentCaptor.forClass(HttpServletRequest.class); + verify(this.hmi).setCache(requestArg.capture()); + HttpServletRequest transformedRequest = requestArg.getValue(); + String attrName = "any"; + String attrValue = "value"; + transformedRequest.setAttribute(attrName, attrValue); + assertThat(transformedRequest.getAttribute(attrName)).isEqualTo(attrValue); + } + + /** + * The request passed into the transformer does not allow interactions on certain + * methods, we need to ensure that the methods used by + * {@link HandlerMappingIntrospector#setCache(HttpServletRequest)} are overridden. + */ + @Test + void transformThenResultDoesNotDelegateToGetAttribute() { + HttpServletRequest request = mock(HttpServletRequest.class); + + this.transformer.transform(request); + + ArgumentCaptor requestArg = ArgumentCaptor.forClass(HttpServletRequest.class); + verify(this.hmi).setCache(requestArg.capture()); + HttpServletRequest transformedRequest = requestArg.getValue(); + transformedRequest.getAttribute("any"); + verifyNoInteractions(request); + } + + /** + * The request passed into the transformer does not allow interactions on certain + * methods, we need to ensure that the methods used by + * {@link HandlerMappingIntrospector#setCache(HttpServletRequest)} are overridden. + */ + @Test + void transformThenResultDoesNotDelegateToGetAttributeNames() { + HttpServletRequest request = mock(HttpServletRequest.class); + + this.transformer.transform(request); + + ArgumentCaptor requestArg = ArgumentCaptor.forClass(HttpServletRequest.class); + verify(this.hmi).setCache(requestArg.capture()); + HttpServletRequest transformedRequest = requestArg.getValue(); + transformedRequest.getAttributeNames(); + verifyNoInteractions(request); + } + + @Test + void transformThenGetAttributeNamesWorks() { + HttpServletRequest request = mock(HttpServletRequest.class); + + this.transformer.transform(request); + + ArgumentCaptor requestArg = ArgumentCaptor.forClass(HttpServletRequest.class); + verify(this.hmi).setCache(requestArg.capture()); + HttpServletRequest transformedRequest = requestArg.getValue(); + String attrName = "any"; + String attrValue = "value"; + transformedRequest.setAttribute(attrName, attrValue); + assertThat(Collections.list(transformedRequest.getAttributeNames())).containsExactly(attrName); + } + + /** + * The request passed into the transformer does not allow interactions on certain + * methods, we need to ensure that the methods used by + * {@link HandlerMappingIntrospector#setCache(HttpServletRequest)} are overridden. + */ + @Test + void transformThenResultDoesNotDelegateToRemoveAttribute() { + HttpServletRequest request = mock(HttpServletRequest.class); + + this.transformer.transform(request); + + ArgumentCaptor requestArg = ArgumentCaptor.forClass(HttpServletRequest.class); + verify(this.hmi).setCache(requestArg.capture()); + HttpServletRequest transformedRequest = requestArg.getValue(); + transformedRequest.removeAttribute("any"); + verifyNoInteractions(request); + } + + /** + * The request passed into the transformer does not allow interactions on certain + * methods, we need to ensure that the methods used by + * {@link HandlerMappingIntrospector#setCache(HttpServletRequest)} are overridden. + */ + @Test + void transformThenResultDoesNotDelegateToGetDispatcherType() { + HttpServletRequest request = mock(HttpServletRequest.class); + + this.transformer.transform(request); + + ArgumentCaptor requestArg = ArgumentCaptor.forClass(HttpServletRequest.class); + verify(this.hmi).setCache(requestArg.capture()); + HttpServletRequest transformedRequest = requestArg.getValue(); + assertThat(transformedRequest.getDispatcherType()).isEqualTo(DispatcherType.REQUEST); + verifyNoInteractions(request); + } + +}