From e5e1f24d1f9d8d1bee11a997f23e689faee7aa51 Mon Sep 17 00:00:00 2001 From: Madhura Bhave Date: Wed, 1 Mar 2017 16:31:36 -0800 Subject: [PATCH] Revert "Skip MvcSecurityInterceptor if Spring Security present" Instead of entirely skipping the interceptor, we will be additionally checking for authorities. --- ...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, 39 insertions(+), 184 deletions(-) delete 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 19aace81891..0bc891edf75 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,7 +54,6 @@ 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; @@ -103,29 +102,16 @@ public class EndpointWebMvcManagementContextConfiguration { EndpointHandlerMapping mapping = new EndpointHandlerMapping(endpoints, corsConfiguration); mapping.setPrefix(this.managementServerProperties.getContextPath()); - if (isSecurityInterceptorRequired()) { - MvcEndpointSecurityInterceptor securityInterceptor = new MvcEndpointSecurityInterceptor( - this.managementServerProperties.getSecurity().getRoles()); - mapping.setSecurityInterceptor(securityInterceptor); - } - + MvcEndpointSecurityInterceptor securityInterceptor = new MvcEndpointSecurityInterceptor( + this.managementServerProperties.getSecurity().isEnabled(), + 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 3e33e8c5bcb..7eac112ff73 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,10 +257,8 @@ public class ManagementWebSecurityAutoConfiguration { private void configurePermittedRequests( ExpressionUrlAuthorizationConfigurer.ExpressionInterceptUrlRegistry requests) { - List roles = this.management.getSecurity().getRoles(); requests.requestMatchers(new LazyEndpointPathRequestMatcher( - this.contextResolver, EndpointPaths.SENSITIVE)) - .hasAnyRole(roles.toArray(new String[roles.size()])); + this.contextResolver, EndpointPaths.SENSITIVE)).authenticated(); // 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 22271dea873..b043def19ec 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,18 +43,21 @@ 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(List roles) { + public MvcEndpointSecurityInterceptor(boolean secure, List roles) { + this.secure = secure; this.roles = roles; } @Override public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler) throws Exception { - if (CorsUtils.isPreFlightRequest(request)) { + if (CorsUtils.isPreFlightRequest(request) || !this.secure) { 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 ed5e0e0b26b..ae96f997c3d 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,6 +16,8 @@ package org.springframework.boot.actuate.autoconfigure; +import java.util.List; + import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -28,6 +30,7 @@ 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; @@ -55,8 +58,6 @@ public class EndpointWebMvcManagementContextConfigurationTests { PropertyPlaceholderAutoConfiguration.class, WebClientAutoConfiguration.class, EndpointWebMvcManagementContextConfiguration.class); - this.context.refresh(); - } @After @@ -67,13 +68,25 @@ public class EndpointWebMvcManagementContextConfigurationTests { } @Test - public void endpointHandlerMappingShouldNotHaveSecurityInterceptor() throws Exception { + public void endpointHandlerMapping() 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); assertThat(mapping.getPrefix()).isEmpty(); MvcEndpointSecurityInterceptor securityInterceptor = (MvcEndpointSecurityInterceptor) ReflectionTestUtils .getField(mapping, "securityInterceptor"); - assertThat(securityInterceptor).isNull(); + 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"); } } 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 6638c06b4c5..be4d595fc80 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,6 +17,7 @@ package org.springframework.boot.actuate.autoconfigure; import java.util.Collection; +import java.util.Collections; import org.hamcrest.Matchers; import org.junit.After; @@ -25,6 +26,7 @@ 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; @@ -142,6 +144,8 @@ 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 205a6b40d55..5e8f8e14223 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,7 +55,6 @@ 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; @@ -255,57 +254,6 @@ 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 deleted file mode 100644 index e96997d1ea2..00000000000 --- a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/NoSpringSecurityEndpointWebMvcManagementContextConfigurationTests.java +++ /dev/null @@ -1,104 +0,0 @@ -/* - * 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 0413a7c2875..cbfa6981332 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(this.roles); + this.securityInterceptor = new MvcEndpointSecurityInterceptor(true, this.roles); this.endpoint = new TestEndpoint("a"); this.mvcEndpoint = new TestMvcEndpoint(this.endpoint); this.handlerMethod = new HandlerMethod(this.mvcEndpoint, "invoke"); @@ -75,6 +75,13 @@ 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);