From ad5cb8a3cdb1aa7b27ca54fed3101d313518d7eb Mon Sep 17 00:00:00 2001 From: Madhura Bhave Date: Wed, 1 Mar 2017 14:41:40 -0800 Subject: [PATCH] Skip MvcSecurityInterceptor if Spring Security present If Spring Security is on the classpath, the role check can be done as part of the ManagementWebSecurityConfigurerAdapter. Fixes gh-8255 --- ...tWebMvcManagementContextConfiguration.java | 22 +++- ...anagementWebSecurityAutoConfiguration.java | 4 +- .../mvc/MvcEndpointSecurityInterceptor.java | 7 +- ...vcManagementContextConfigurationTests.java | 21 +--- .../JolokiaAutoConfigurationTests.java | 4 - ...mentWebSecurityAutoConfigurationTests.java | 52 +++++++++ ...vcManagementContextConfigurationTests.java | 104 ++++++++++++++++++ .../MvcEndpointSecurityInterceptorTests.java | 9 +- 8 files changed, 184 insertions(+), 39 deletions(-) create mode 100644 spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/NoSpringSecurityEndpointWebMvcManagementContextConfigurationTests.java diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/EndpointWebMvcManagementContextConfiguration.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/EndpointWebMvcManagementContextConfiguration.java index 0bc891edf75..19aace81891 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/EndpointWebMvcManagementContextConfiguration.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/EndpointWebMvcManagementContextConfiguration.java @@ -54,6 +54,7 @@ import org.springframework.context.annotation.ConditionContext; import org.springframework.context.annotation.Conditional; import org.springframework.core.env.Environment; import org.springframework.core.type.AnnotatedTypeMetadata; +import org.springframework.util.ClassUtils; import org.springframework.util.CollectionUtils; import org.springframework.util.StringUtils; import org.springframework.web.cors.CorsConfiguration; @@ -102,16 +103,29 @@ public class EndpointWebMvcManagementContextConfiguration { EndpointHandlerMapping mapping = new EndpointHandlerMapping(endpoints, corsConfiguration); mapping.setPrefix(this.managementServerProperties.getContextPath()); - MvcEndpointSecurityInterceptor securityInterceptor = new MvcEndpointSecurityInterceptor( - this.managementServerProperties.getSecurity().isEnabled(), - this.managementServerProperties.getSecurity().getRoles()); - mapping.setSecurityInterceptor(securityInterceptor); + if (isSecurityInterceptorRequired()) { + MvcEndpointSecurityInterceptor securityInterceptor = new MvcEndpointSecurityInterceptor( + this.managementServerProperties.getSecurity().getRoles()); + mapping.setSecurityInterceptor(securityInterceptor); + } + for (EndpointHandlerMappingCustomizer customizer : this.mappingCustomizers) { customizer.customize(mapping); } return mapping; } + private boolean isSecurityInterceptorRequired() { + return this.managementServerProperties.getSecurity().isEnabled() + && !isSpringSecurityAvailable(); + } + + private boolean isSpringSecurityAvailable() { + return ClassUtils.isPresent( + "org.springframework.security.config.annotation.web.WebSecurityConfigurer", + getClass().getClassLoader()); + } + private CorsConfiguration getCorsConfiguration(EndpointCorsProperties properties) { if (CollectionUtils.isEmpty(properties.getAllowedOrigins())) { return null; diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/ManagementWebSecurityAutoConfiguration.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/ManagementWebSecurityAutoConfiguration.java index 7eac112ff73..3e33e8c5bcb 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/ManagementWebSecurityAutoConfiguration.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/ManagementWebSecurityAutoConfiguration.java @@ -257,8 +257,10 @@ public class ManagementWebSecurityAutoConfiguration { private void configurePermittedRequests( ExpressionUrlAuthorizationConfigurer.ExpressionInterceptUrlRegistry requests) { + List roles = this.management.getSecurity().getRoles(); requests.requestMatchers(new LazyEndpointPathRequestMatcher( - this.contextResolver, EndpointPaths.SENSITIVE)).authenticated(); + this.contextResolver, EndpointPaths.SENSITIVE)) + .hasAnyRole(roles.toArray(new String[roles.size()])); // Permit access to the non-sensitive endpoints requests.requestMatchers(new LazyEndpointPathRequestMatcher( this.contextResolver, EndpointPaths.NON_SENSITIVE)).permitAll(); diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/MvcEndpointSecurityInterceptor.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/MvcEndpointSecurityInterceptor.java index b043def19ec..22271dea873 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/MvcEndpointSecurityInterceptor.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/MvcEndpointSecurityInterceptor.java @@ -43,21 +43,18 @@ public class MvcEndpointSecurityInterceptor extends HandlerInterceptorAdapter { private static final Log logger = LogFactory .getLog(MvcEndpointSecurityInterceptor.class); - private final boolean secure; - private final List roles; private AtomicBoolean loggedUnauthorizedAttempt = new AtomicBoolean(); - public MvcEndpointSecurityInterceptor(boolean secure, List roles) { - this.secure = secure; + public MvcEndpointSecurityInterceptor(List roles) { this.roles = roles; } @Override public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler) throws Exception { - if (CorsUtils.isPreFlightRequest(request) || !this.secure) { + if (CorsUtils.isPreFlightRequest(request)) { return true; } HandlerMethod handlerMethod = (HandlerMethod) handler; diff --git a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/EndpointWebMvcManagementContextConfigurationTests.java b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/EndpointWebMvcManagementContextConfigurationTests.java index ae96f997c3d..ed5e0e0b26b 100644 --- a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/EndpointWebMvcManagementContextConfigurationTests.java +++ b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/EndpointWebMvcManagementContextConfigurationTests.java @@ -16,8 +16,6 @@ package org.springframework.boot.actuate.autoconfigure; -import java.util.List; - import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -30,7 +28,6 @@ import org.springframework.boot.autoconfigure.security.SecurityAutoConfiguration import org.springframework.boot.autoconfigure.web.HttpMessageConvertersAutoConfiguration; import org.springframework.boot.autoconfigure.web.WebClientAutoConfiguration; import org.springframework.boot.autoconfigure.web.WebMvcAutoConfiguration; -import org.springframework.boot.test.util.EnvironmentTestUtils; import org.springframework.mock.web.MockServletContext; import org.springframework.test.util.ReflectionTestUtils; import org.springframework.web.context.support.AnnotationConfigWebApplicationContext; @@ -58,6 +55,8 @@ public class EndpointWebMvcManagementContextConfigurationTests { PropertyPlaceholderAutoConfiguration.class, WebClientAutoConfiguration.class, EndpointWebMvcManagementContextConfiguration.class); + this.context.refresh(); + } @After @@ -68,25 +67,13 @@ public class EndpointWebMvcManagementContextConfigurationTests { } @Test - public void endpointHandlerMapping() throws Exception { - EnvironmentTestUtils.addEnvironment(this.context, - "management.security.enabled=false", - "management.security.roles=my-role,your-role"); - this.context.refresh(); + public void endpointHandlerMappingShouldNotHaveSecurityInterceptor() throws Exception { EndpointHandlerMapping mapping = this.context.getBean("endpointHandlerMapping", EndpointHandlerMapping.class); assertThat(mapping.getPrefix()).isEmpty(); MvcEndpointSecurityInterceptor securityInterceptor = (MvcEndpointSecurityInterceptor) ReflectionTestUtils .getField(mapping, "securityInterceptor"); - Object secure = ReflectionTestUtils.getField(securityInterceptor, "secure"); - List roles = getRoles(securityInterceptor); - assertThat(secure).isEqualTo(false); - assertThat(roles).containsExactly("my-role", "your-role"); - } - - @SuppressWarnings("unchecked") - private List getRoles(MvcEndpointSecurityInterceptor securityInterceptor) { - return (List) ReflectionTestUtils.getField(securityInterceptor, "roles"); + assertThat(securityInterceptor).isNull(); } } diff --git a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/JolokiaAutoConfigurationTests.java b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/JolokiaAutoConfigurationTests.java index be4d595fc80..6638c06b4c5 100644 --- a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/JolokiaAutoConfigurationTests.java +++ b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/JolokiaAutoConfigurationTests.java @@ -17,7 +17,6 @@ package org.springframework.boot.actuate.autoconfigure; import java.util.Collection; -import java.util.Collections; import org.hamcrest.Matchers; import org.junit.After; @@ -26,7 +25,6 @@ import org.junit.Test; import org.springframework.boot.actuate.endpoint.mvc.EndpointHandlerMapping; import org.springframework.boot.actuate.endpoint.mvc.JolokiaMvcEndpoint; import org.springframework.boot.actuate.endpoint.mvc.MvcEndpoint; -import org.springframework.boot.actuate.endpoint.mvc.MvcEndpointSecurityInterceptor; import org.springframework.boot.autoconfigure.context.PropertyPlaceholderAutoConfiguration; import org.springframework.boot.autoconfigure.web.HttpMessageConvertersAutoConfiguration; import org.springframework.boot.autoconfigure.web.WebMvcAutoConfiguration; @@ -144,8 +142,6 @@ public class JolokiaAutoConfigurationTests { public EndpointHandlerMapping endpointHandlerMapping( Collection endpoints) { EndpointHandlerMapping mapping = new EndpointHandlerMapping(endpoints); - mapping.setSecurityInterceptor(new MvcEndpointSecurityInterceptor(false, - Collections.emptyList())); return mapping; } diff --git a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/ManagementWebSecurityAutoConfigurationTests.java b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/ManagementWebSecurityAutoConfigurationTests.java index 5e8f8e14223..205a6b40d55 100644 --- a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/ManagementWebSecurityAutoConfigurationTests.java +++ b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/ManagementWebSecurityAutoConfigurationTests.java @@ -55,6 +55,7 @@ import org.springframework.test.web.servlet.ResultMatcher; import org.springframework.test.web.servlet.request.MockMvcRequestBuilders; import org.springframework.test.web.servlet.result.MockMvcResultMatchers; import org.springframework.test.web.servlet.setup.MockMvcBuilders; +import org.springframework.util.Base64Utils; import org.springframework.util.StringUtils; import org.springframework.web.context.support.AnnotationConfigWebApplicationContext; @@ -254,6 +255,57 @@ public class ManagementWebSecurityAutoConfigurationTests { .andExpect(status().isUnauthorized()); } + @Test + public void sensitiveEndpointWithRightRoleShouldReturnOk() throws Exception { + this.context = new AnnotationConfigWebApplicationContext(); + this.context.setServletContext(new MockServletContext()); + this.context.register(AuthenticationConfig.class, WebConfiguration.class); + EnvironmentTestUtils.addEnvironment(this.context, "management.security.roles:USER"); + this.context.refresh(); + + MockMvc mockMvc = MockMvcBuilders.webAppContextSetup(this.context) + .apply(springSecurity()) + .build(); + + String authorizationHeader = Base64Utils.encodeToString("user:password".getBytes()); + mockMvc // + .perform(get("/env").header("authorization", "Basic " + authorizationHeader)) + .andExpect(status().isOk()); + } + + @Test + public void sensitiveEndpointWithRightRoleShouldReturnForbidden() throws Exception { + this.context = new AnnotationConfigWebApplicationContext(); + this.context.setServletContext(new MockServletContext()); + this.context.register(AuthenticationConfig.class, WebConfiguration.class); + this.context.refresh(); + + MockMvc mockMvc = MockMvcBuilders.webAppContextSetup(this.context) + .apply(springSecurity()) + .build(); + + String authorizationHeader = Base64Utils.encodeToString("user:password".getBytes()); + mockMvc // + .perform(get("/env").header("authorization", "Basic " + authorizationHeader)) + .andExpect(status().isForbidden()); + } + + @Test + public void nonSensitiveEndpointShouldAlwaysReturnOk() throws Exception { + this.context = new AnnotationConfigWebApplicationContext(); + this.context.setServletContext(new MockServletContext()); + this.context.register(WebConfiguration.class); + this.context.refresh(); + + MockMvc mockMvc = MockMvcBuilders.webAppContextSetup(this.context) + .apply(springSecurity()) + .build(); + + mockMvc // + .perform(get("/health")) + .andExpect(status().isOk()); + } + private ResultMatcher springAuthenticateRealmHeader() { return MockMvcResultMatchers.header().string("www-authenticate", Matchers.containsString("realm=\"Spring\"")); diff --git a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/NoSpringSecurityEndpointWebMvcManagementContextConfigurationTests.java b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/NoSpringSecurityEndpointWebMvcManagementContextConfigurationTests.java new file mode 100644 index 00000000000..e96997d1ea2 --- /dev/null +++ b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/NoSpringSecurityEndpointWebMvcManagementContextConfigurationTests.java @@ -0,0 +1,104 @@ +/* + * Copyright 2012-2017 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 + * + * http://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.boot.actuate.autoconfigure; + +import java.util.List; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; + +import org.springframework.boot.actuate.endpoint.mvc.EndpointHandlerMapping; +import org.springframework.boot.actuate.endpoint.mvc.MvcEndpointSecurityInterceptor; +import org.springframework.boot.autoconfigure.context.PropertyPlaceholderAutoConfiguration; +import org.springframework.boot.autoconfigure.jackson.JacksonAutoConfiguration; +import org.springframework.boot.autoconfigure.web.HttpMessageConvertersAutoConfiguration; +import org.springframework.boot.autoconfigure.web.WebClientAutoConfiguration; +import org.springframework.boot.autoconfigure.web.WebMvcAutoConfiguration; +import org.springframework.boot.junit.runner.classpath.ClassPathExclusions; +import org.springframework.boot.junit.runner.classpath.ModifiedClassPathRunner; +import org.springframework.boot.test.util.EnvironmentTestUtils; +import org.springframework.context.annotation.AnnotationConfigApplicationContext; +import org.springframework.test.util.ReflectionTestUtils; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Tests for {@link EndpointWebMvcManagementContextConfiguration} when Spring Security is not available. + * + * @author Madhura Bhave + */ +@RunWith(ModifiedClassPathRunner.class) +@ClassPathExclusions("spring-security-*.jar") +public class NoSpringSecurityEndpointWebMvcManagementContextConfigurationTests { + + private AnnotationConfigApplicationContext context; + + @Before + public void setup() { + this.context = new AnnotationConfigApplicationContext(); + this.context.register(WebMvcAutoConfiguration.class, JacksonAutoConfiguration.class, + HttpMessageConvertersAutoConfiguration.class, + EndpointAutoConfiguration.class, EndpointWebMvcAutoConfiguration.class, + ManagementServerPropertiesAutoConfiguration.class, + PropertyPlaceholderAutoConfiguration.class, + WebClientAutoConfiguration.class, + EndpointWebMvcManagementContextConfiguration.class); + } + + @After + public void close() { + if (this.context != null) { + this.context.close(); + } + } + + @Test + public void endpointHandlerMappingWhenSecurityEnabledShouldHaveSecurityInterceptor() throws Exception { + EnvironmentTestUtils.addEnvironment(this.context, + "management.security.enabled=true", + "management.security.roles=my-role,your-role"); + this.context.refresh(); + EndpointHandlerMapping mapping = this.context.getBean("endpointHandlerMapping", + EndpointHandlerMapping.class); + MvcEndpointSecurityInterceptor securityInterceptor = (MvcEndpointSecurityInterceptor) ReflectionTestUtils + .getField(mapping, "securityInterceptor"); + List roles = getRoles(securityInterceptor); + assertThat(roles).containsExactly("my-role", "your-role"); + } + + @Test + public void endpointHandlerMappingWhenSecurityDisabledShouldHaveSecurityInterceptor() throws Exception { + EnvironmentTestUtils.addEnvironment(this.context, + "management.security.enabled=false", + "management.security.roles=my-role,your-role"); + this.context.refresh(); + EndpointHandlerMapping mapping = this.context.getBean("endpointHandlerMapping", + EndpointHandlerMapping.class); + MvcEndpointSecurityInterceptor securityInterceptor = (MvcEndpointSecurityInterceptor) ReflectionTestUtils + .getField(mapping, "securityInterceptor"); + assertThat(securityInterceptor).isNull(); + } + + @SuppressWarnings("unchecked") + private List getRoles(MvcEndpointSecurityInterceptor securityInterceptor) { + return (List) ReflectionTestUtils.getField(securityInterceptor, "roles"); + } + +} + diff --git a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/mvc/MvcEndpointSecurityInterceptorTests.java b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/mvc/MvcEndpointSecurityInterceptorTests.java index cbfa6981332..0413a7c2875 100644 --- a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/mvc/MvcEndpointSecurityInterceptorTests.java +++ b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/mvc/MvcEndpointSecurityInterceptorTests.java @@ -66,7 +66,7 @@ public class MvcEndpointSecurityInterceptorTests { @Before public void setup() throws Exception { this.roles = Arrays.asList("SUPER_HERO"); - this.securityInterceptor = new MvcEndpointSecurityInterceptor(true, this.roles); + this.securityInterceptor = new MvcEndpointSecurityInterceptor(this.roles); this.endpoint = new TestEndpoint("a"); this.mvcEndpoint = new TestMvcEndpoint(this.endpoint); this.handlerMethod = new HandlerMethod(this.mvcEndpoint, "invoke"); @@ -75,13 +75,6 @@ public class MvcEndpointSecurityInterceptorTests { this.response = mock(HttpServletResponse.class); } - @Test - public void securityDisabledShouldAllowAccess() throws Exception { - this.securityInterceptor = new MvcEndpointSecurityInterceptor(false, this.roles); - assertThat(this.securityInterceptor.preHandle(this.request, this.response, - this.handlerMethod)).isTrue(); - } - @Test public void endpointNotSensitiveShouldAllowAccess() throws Exception { this.endpoint.setSensitive(false);