Revert "Skip MvcSecurityInterceptor if Spring Security present"

Instead of entirely skipping the interceptor, we will be additionally
checking for authorities.

Author: mbhave

spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/EndpointWebMvcManagementContextConfiguration.java

@@ -54,7 +54,6 @@
 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 EndpointHandlerMapping endpointHandlerMapping() {
 		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;

spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/ManagementWebSecurityAutoConfiguration.java

@@ -257,10 +257,8 @@ private AuthenticationEntryPoint entryPoint() {
 
 		private void configurePermittedRequests(
 				ExpressionUrlAuthorizationConfigurer<HttpSecurity>.ExpressionInterceptUrlRegistry requests) {
-			List<String> 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();

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<String> roles;
 
 	private AtomicBoolean loggedUnauthorizedAttempt = new AtomicBoolean();
 
-	public MvcEndpointSecurityInterceptor(List<String> roles) {
+	public MvcEndpointSecurityInterceptor(boolean secure, List<String> 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;

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.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 void setup() {
 				PropertyPlaceholderAutoConfiguration.class,
 				WebClientAutoConfiguration.class,
 				EndpointWebMvcManagementContextConfiguration.class);
-		this.context.refresh();
-
 	}
 
 	@After
@@ -67,13 +68,25 @@ public void close() {
 	}
 
 	@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<String> roles = getRoles(securityInterceptor);
+		assertThat(secure).isEqualTo(false);
+		assertThat(roles).containsExactly("my-role", "your-role");
+	}
+
+	@SuppressWarnings("unchecked")
+	private List<String> getRoles(MvcEndpointSecurityInterceptor securityInterceptor) {
+		return (List<String>) ReflectionTestUtils.getField(securityInterceptor, "roles");
 	}
 
 }

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.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 @@ protected static class EndpointsConfig extends Config {
 		public EndpointHandlerMapping endpointHandlerMapping(
 				Collection<? extends MvcEndpoint> endpoints) {
 			EndpointHandlerMapping mapping = new EndpointHandlerMapping(endpoints);
+			mapping.setSecurityInterceptor(new MvcEndpointSecurityInterceptor(false,
+					Collections.<String>emptyList()));
 			return mapping;
 		}
 

spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/ManagementWebSecurityAutoConfigurationTests.java

@@ -55,7 +55,6 @@
 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 void testMarkAllEndpointsSensitive() throws Exception {
 				.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\""));

spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/NoSpringSecurityEndpointWebMvcManagementContextConfigurationTests.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 void setup() throws Exception {
 		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);