Remove Principal handler logic from security

Update ManagementSecurityAutoConfiguration so that MVC Endpoints that
have Principal arguments are not treated in any special way. This
restores Spring Boot 1.1.x behavior where the 'sensitive' flag is used
to determine access rules.

The HealthMvcEndpoint still uses the Principal (when available) to
determine if full status information can be displayed. It now also
explicitly checks the environment for `endpoints.health.sensitive`
to determine if the user has opted-out and requires complete health
details.

The health MVC endpoint should now work as follows:

* Default configuration - No login is required, full information is only
  displayed if a Principal is available.
* endpoints.health.sensitive=true - Login is required, full information
  is displayed.
* endpoints.health.sensitive=false - Login is not required, full
  information is displayed.

Fixes gh-2211

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

@@ -78,9 +78,6 @@ public class EndpointAutoConfiguration {
 	@Autowired
 	private InfoPropertiesConfiguration properties;
 
-	@Autowired(required = false)
-	private ManagementServerProperties management;
-
 	@Autowired(required = false)
 	private HealthAggregator healthAggregator = new OrderedHealthAggregator();
 
@@ -105,20 +102,7 @@ public class EndpointAutoConfiguration {
 	@Bean
 	@ConditionalOnMissingBean
 	public HealthEndpoint healthEndpoint() {
-		HealthEndpoint endpoint = new HealthEndpoint(this.healthAggregator,
-				this.healthIndicators);
-		endpoint.setSensitive(isHealthEndpointSensitive());
-		return endpoint;
-	}
-
-	/**
-	 * The default health endpoint sensitivity depends on whether all the endpoints by
-	 * default are secure or not. User can always override with
-	 * {@literal endpoints.health.sensitive}.
-	 */
-	private boolean isHealthEndpointSensitive() {
-		return (this.management != null) && (this.management.getSecurity() != null)
-				&& this.management.getSecurity().isEnabled();
+		return new HealthEndpoint(this.healthAggregator, this.healthIndicators);
 	}
 
 	@Bean

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

@@ -33,6 +33,7 @@ import org.springframework.beans.factory.BeanFactory;
 import org.springframework.beans.factory.NoSuchBeanDefinitionException;
 import org.springframework.beans.factory.SmartInitializingSingleton;
 import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.boot.actuate.autoconfigure.ManagementServerProperties.Security;
 import org.springframework.boot.actuate.endpoint.Endpoint;
 import org.springframework.boot.actuate.endpoint.EnvironmentEndpoint;
 import org.springframework.boot.actuate.endpoint.HealthEndpoint;
@@ -163,7 +164,9 @@ public class EndpointWebMvcAutoConfiguration implements ApplicationContextAware,
 	@ConditionalOnBean(HealthEndpoint.class)
 	@ConditionalOnProperty(prefix = "endpoints.health", name = "enabled", matchIfMissing = true)
 	public HealthMvcEndpoint healthMvcEndpoint(HealthEndpoint delegate) {
-		HealthMvcEndpoint healthMvcEndpoint = new HealthMvcEndpoint(delegate);
+		Security security = this.managementServerProperties.getSecurity();
+		boolean secure = (security == null || security.isEnabled());
+		HealthMvcEndpoint healthMvcEndpoint = new HealthMvcEndpoint(delegate, secure);
 		if (this.healthMvcEndpointProperties.getMapping() != null) {
 			healthMvcEndpoint.addStatusMapping(this.healthMvcEndpointProperties
 					.getMapping());

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

@@ -22,7 +22,6 @@ import java.util.List;
 import java.util.Set;
 
 import javax.annotation.PostConstruct;
-import javax.servlet.http.HttpServletRequest;
 
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.boot.actuate.endpoint.Endpoint;
@@ -62,7 +61,6 @@ import org.springframework.security.config.annotation.web.configuration.WebSecur
 import org.springframework.security.config.annotation.web.configurers.ExpressionUrlAuthorizationConfigurer;
 import org.springframework.security.web.AuthenticationEntryPoint;
 import org.springframework.security.web.authentication.www.BasicAuthenticationEntryPoint;
-import org.springframework.security.web.util.matcher.RequestMatcher;
 import org.springframework.util.StringUtils;
 
 /**
@@ -256,23 +254,9 @@ public class ManagementSecurityAutoConfiguration {
 				String[] endpointPaths,
 				ExpressionUrlAuthorizationConfigurer<HttpSecurity>.ExpressionInterceptUrlRegistry requests) {
 			requests.antMatchers(endpointPaths).permitAll();
-			if (this.endpointHandlerMapping != null) {
-				requests.requestMatchers(new PrincipalHandlerRequestMatcher())
-						.permitAll();
-			}
 			requests.anyRequest().hasRole(this.management.getSecurity().getRole());
 		}
 
-		private final class PrincipalHandlerRequestMatcher implements RequestMatcher {
-
-			@Override
-			public boolean matches(HttpServletRequest request) {
-				return ManagementWebSecurityConfigurerAdapter.this.endpointHandlerMapping
-						.isPrincipalHandler(request);
-			}
-
-		}
-
 	}
 
 	private static String[] getEndpointPaths(EndpointHandlerMapping endpointHandlerMapping) {

spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/EndpointHandlerMapping.java

@@ -17,22 +17,17 @@
 package org.springframework.boot.actuate.endpoint.mvc;
 
 import java.lang.reflect.Method;
-import java.security.Principal;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
 
-import javax.servlet.http.HttpServletRequest;
-
 import org.springframework.boot.actuate.endpoint.Endpoint;
 import org.springframework.context.ApplicationContext;
 import org.springframework.context.ApplicationContextAware;
 import org.springframework.util.Assert;
 import org.springframework.util.StringUtils;
-import org.springframework.web.method.HandlerMethod;
-import org.springframework.web.servlet.HandlerExecutionChain;
 import org.springframework.web.servlet.HandlerMapping;
 import org.springframework.web.servlet.mvc.condition.PatternsRequestCondition;
 import org.springframework.web.servlet.mvc.method.RequestMappingInfo;
@@ -46,8 +41,8 @@ import org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandl
  * <p>
  * One of the aims of the mapping is to support endpoints that work as HTTP endpoints but
  * can still provide useful service interfaces when there is no HTTP server (and no Spring
- * MVC on the classpath). Note that any endpoints having method signaturess will break in
- * a non-servlet environment.
+ * MVC on the classpath). Note that any endpoints having method signatures will break in a
+ * non-servlet environment.
  *
  * @author Phillip Webb
  * @author Christian Dupuis
@@ -62,8 +57,6 @@ public class EndpointHandlerMapping extends RequestMappingHandlerMapping impleme
 
 	private boolean disabled = false;
 
-	private Set<HandlerMethod> principalHandlers = new HashSet<HandlerMethod>();
-
 	/**
 	 * Create a new {@link EndpointHandlerMapping} instance. All {@link Endpoint}s will be
 	 * detected from the {@link ApplicationContext}.
@@ -102,9 +95,6 @@ public class EndpointHandlerMapping extends RequestMappingHandlerMapping impleme
 			return;
 		}
 		String[] patterns = getPatterns(handler, mapping);
-		if (handlesPrincipal(method)) {
-			this.principalHandlers.add(new HandlerMethod(handler, method));
-		}
 		super.registerHandlerMethod(handler, method, withNewPatterns(mapping, patterns));
 	}
 
@@ -132,15 +122,6 @@ public class EndpointHandlerMapping extends RequestMappingHandlerMapping impleme
 		return "";
 	}
 
-	private boolean handlesPrincipal(Method method) {
-		for (Class<?> type : method.getParameterTypes()) {
-			if (Principal.class.equals(type)) {
-				return true;
-			}
-		}
-		return false;
-	}
-
 	private RequestMappingInfo withNewPatterns(RequestMappingInfo mapping,
 			String[] patternStrings) {
 		PatternsRequestCondition patterns = new PatternsRequestCondition(patternStrings);
@@ -150,23 +131,6 @@ public class EndpointHandlerMapping extends RequestMappingHandlerMapping impleme
 				mapping.getCustomCondition());
 	}
 
-	/**
-	 * Returns {@code true} if the given request is mapped to an endpoint and to a method
-	 * that includes a {@link Principal} argument.
-	 * @param request the http request
-	 * @return {@code true} if the request is
-	 */
-	public boolean isPrincipalHandler(HttpServletRequest request) {
-		try {
-			HandlerExecutionChain handlerChain = getHandler(request);
-			Object handler = (handlerChain == null ? null : handlerChain.getHandler());
-			return (handler != null && this.principalHandlers.contains(handler));
-		}
-		catch (Exception ex) {
-			return false;
-		}
-	}
-
 	/**
 	 * @param prefix the prefix to set
 	 */

spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/HealthMvcEndpoint.java

@@ -25,6 +25,9 @@ import org.springframework.boot.actuate.endpoint.Endpoint;
 import org.springframework.boot.actuate.endpoint.HealthEndpoint;
 import org.springframework.boot.actuate.health.Health;
 import org.springframework.boot.actuate.health.Status;
+import org.springframework.boot.bind.RelaxedPropertyResolver;
+import org.springframework.context.EnvironmentAware;
+import org.springframework.core.env.Environment;
 import org.springframework.http.HttpStatus;
 import org.springframework.http.ResponseEntity;
 import org.springframework.util.Assert;
@@ -37,20 +40,31 @@ import org.springframework.web.bind.annotation.ResponseBody;
  * @author Christian Dupuis
  * @author Dave Syer
  * @author Andy Wilkinson
+ * @author Phillip Webb
  * @since 1.1.0
  */
-public class HealthMvcEndpoint implements MvcEndpoint {
+public class HealthMvcEndpoint implements MvcEndpoint, EnvironmentAware {
+
+	private final HealthEndpoint delegate;
+
+	private final boolean secure;
 
 	private Map<String, HttpStatus> statusMapping = new HashMap<String, HttpStatus>();
 
-	private HealthEndpoint delegate;
+	private RelaxedPropertyResolver propertyResolver;
 
 	private long lastAccess = 0;
 
 	private Health cached;
 
 	public HealthMvcEndpoint(HealthEndpoint delegate) {
+		this(delegate, true);
+	}
+
+	public HealthMvcEndpoint(HealthEndpoint delegate, boolean secure) {
+		Assert.notNull(delegate, "Delegate must not be null");
 		this.delegate = delegate;
+		this.secure = secure;
 		setupDefaultStatusMapping();
 	}
 
@@ -59,6 +73,12 @@ public class HealthMvcEndpoint implements MvcEndpoint {
 		addStatusMapping(Status.OUT_OF_SERVICE, HttpStatus.SERVICE_UNAVAILABLE);
 	}
 
+	@Override
+	public void setEnvironment(Environment environment) {
+		this.propertyResolver = new RelaxedPropertyResolver(environment,
+				"endpoints.health.");
+	}
+
 	/**
 	 * Set specific status mappings.
 	 * @param statusMapping a map of status code to {@link HttpStatus}
@@ -108,44 +128,40 @@ public class HealthMvcEndpoint implements MvcEndpoint {
 					"message", "This endpoint is disabled"), HttpStatus.NOT_FOUND);
 		}
 		Health health = getHealth(principal);
-		Status status = health.getStatus();
-		if (this.statusMapping.containsKey(status.getCode())) {
-			return new ResponseEntity<Health>(health, this.statusMapping.get(status
-					.getCode()));
+		HttpStatus status = this.statusMapping.get(health.getStatus().getCode());
+		if (status != null) {
+			return new ResponseEntity<Health>(health, status);
 		}
 		return health;
 	}
 
 	private Health getHealth(Principal principal) {
-		Health health = (useCachedValue(principal) ? this.cached : (Health) this.delegate
-				.invoke());
-		// Not too worried about concurrent access here, the worst that can happen is the
-		// odd extra call to delegate.invoke()
-		this.cached = health;
-		if (!secure(principal) && this.delegate.isSensitive()) {
-			// If not secure we only expose the status
-			health = Health.status(health.getStatus()).build();
+		long accessTime = System.currentTimeMillis();
+		if (isCacheStale(accessTime) || isSecure(principal) || isUnrestricted()) {
+			this.lastAccess = accessTime;
+			this.cached = this.delegate.invoke();
 		}
-		return health;
+		if (isSecure(principal) || isUnrestricted()) {
+			return this.cached;
 		}
-
-	private boolean secure(Principal principal) {
-		return (principal != null && !principal.getClass().getName()
-				.contains("Anonymous"));
+		return Health.status(this.cached.getStatus()).build();
 	}
 
-	private boolean useCachedValue(Principal principal) {
-		long accessTime = System.currentTimeMillis();
-		if (cacheIsStale(accessTime) || secure(principal) || !this.delegate.isSensitive()) {
-			this.lastAccess = accessTime;
-			return false;
+	private boolean isCacheStale(long accessTime) {
+		if (this.cached == null) {
+			return true;
 		}
-		return this.cached != null;
+		return (accessTime - this.lastAccess) > this.delegate.getTimeToLive();
 	}
 
-	private boolean cacheIsStale(long accessTime) {
-		return this.cached == null
-				|| (accessTime - this.lastAccess) > this.delegate.getTimeToLive();
+	private boolean isUnrestricted() {
+		Boolean sensitive = this.propertyResolver.getProperty("sensitive", Boolean.class);
+		return !this.secure || Boolean.FALSE.equals(sensitive);
+	}
+
+	private boolean isSecure(Principal principal) {
+		return (principal != null && !principal.getClass().getName()
+				.contains("Anonymous"));
 	}
 
 	@Override

spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/mvc/HealthMvcEndpointTests.java

@@ -23,8 +23,11 @@ import org.junit.Test;
 import org.springframework.boot.actuate.endpoint.HealthEndpoint;
 import org.springframework.boot.actuate.health.Health;
 import org.springframework.boot.actuate.health.Status;
+import org.springframework.core.env.MapPropertySource;
+import org.springframework.core.env.PropertySource;
 import org.springframework.http.HttpStatus;
 import org.springframework.http.ResponseEntity;
+import org.springframework.mock.env.MockEnvironment;
 import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;
 import org.springframework.security.core.authority.AuthorityUtils;
 
@@ -44,10 +47,16 @@ import static org.mockito.Mockito.mock;
  */
 public class HealthMvcEndpointTests {
 
+	private static final PropertySource<?> NON_SENSITIVE = new MapPropertySource("test",
+			Collections.<String, Object> singletonMap("endpoints.health.sensitive",
+					"false"));;
+
 	private HealthEndpoint endpoint = null;
 
 	private HealthMvcEndpoint mvc = null;
 
+	private MockEnvironment environment;
+
 	private UsernamePasswordAuthenticationToken user = new UsernamePasswordAuthenticationToken(
 			"user", "password",
 			AuthorityUtils.commaSeparatedStringToAuthorityList("ROLE_USER"));
@@ -57,6 +66,8 @@ public class HealthMvcEndpointTests {
 		this.endpoint = mock(HealthEndpoint.class);
 		given(this.endpoint.isEnabled()).willReturn(true);
 		this.mvc = new HealthMvcEndpoint(this.endpoint);
+		this.environment = new MockEnvironment();
+		this.mvc.setEnvironment(this.environment);
 	}
 
 	@Test
@@ -143,9 +154,9 @@ public class HealthMvcEndpointTests {
 
 	@Test
 	public void unsecureAnonymousAccessUnrestricted() {
+		this.environment.getPropertySources().addLast(NON_SENSITIVE);
 		given(this.endpoint.invoke()).willReturn(
 				new Health.Builder().up().withDetail("foo", "bar").build());
-		given(this.endpoint.isSensitive()).willReturn(false);
 		Object result = this.mvc.invoke(null);
 		assertTrue(result instanceof Health);
 		assertTrue(((Health) result).getStatus() == Status.UP);
@@ -154,8 +165,8 @@ public class HealthMvcEndpointTests {
 
 	@Test
 	public void unsecureIsNotCachedWhenAnonymousAccessIsUnrestricted() {
+		this.environment.getPropertySources().addLast(NON_SENSITIVE);
 		given(this.endpoint.getTimeToLive()).willReturn(10000L);
-		given(this.endpoint.isSensitive()).willReturn(false);
 		given(this.endpoint.invoke()).willReturn(
 				new Health.Builder().up().withDetail("foo", "bar").build());
 		Object result = this.mvc.invoke(null);

spring-boot-samples/spring-boot-sample-actuator/src/test/java/sample/actuator/EndpointsPropertiesSampleActuatorApplicationTests.java

@@ -20,7 +20,9 @@ import java.util.Map;
 
 import org.junit.Test;
 import org.junit.runner.RunWith;
+import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.beans.factory.annotation.Value;
+import org.springframework.boot.autoconfigure.security.SecurityProperties;
 import org.springframework.boot.test.IntegrationTest;
 import org.springframework.boot.test.SpringApplicationConfiguration;
 import org.springframework.boot.test.TestRestTemplate;
@@ -47,6 +49,9 @@ import static org.junit.Assert.assertTrue;
 @ActiveProfiles("endpoints")
 public class EndpointsPropertiesSampleActuatorApplicationTests {
 
+	@Autowired
+	private SecurityProperties security;
+
 	@Value("${local.server.port}")
 	private int port;
 
@@ -64,8 +69,9 @@ public class EndpointsPropertiesSampleActuatorApplicationTests {
 
 	@Test
 	public void testCustomContextPath() throws Exception {
-		ResponseEntity<String> entity = new TestRestTemplate().getForEntity(
-				"http://localhost:" + this.port + "/admin/health", String.class);
+		ResponseEntity<String> entity = new TestRestTemplate("user", getPassword())
+				.getForEntity("http://localhost:" + this.port + "/admin/health",
+						String.class);
 		assertEquals(HttpStatus.OK, entity.getStatusCode());
 		assertTrue("Wrong body: " + entity.getBody(),
 				entity.getBody().contains("\"status\":\"UP\""));
@@ -73,4 +79,9 @@ public class EndpointsPropertiesSampleActuatorApplicationTests {
 		assertTrue("Wrong body: " + entity.getBody(),
 				entity.getBody().contains("\"hello\":\"world\""));
 	}
+
+	private String getPassword() {
+		return this.security.getUser().getPassword();
+	}
+
 }

spring-boot-samples/spring-boot-sample-actuator/src/test/java/sample/actuator/NonSensitiveHealthTests.java

@@ -0,0 +1,58 @@
+/*
+ * Copyright 2012-2014 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 sample.actuator;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.springframework.beans.factory.annotation.Value;
+import org.springframework.boot.test.IntegrationTest;
+import org.springframework.boot.test.SpringApplicationConfiguration;
+import org.springframework.boot.test.TestRestTemplate;
+import org.springframework.http.HttpStatus;
+import org.springframework.http.ResponseEntity;
+import org.springframework.test.annotation.DirtiesContext;
+import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
+import org.springframework.test.context.web.WebAppConfiguration;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+/**
+ * Tests for /health with {@code endpoints.health.sensitive=false}.
+ *
+ * @author Phillip Webb
+ */
+@RunWith(SpringJUnit4ClassRunner.class)
+@SpringApplicationConfiguration(classes = SampleActuatorApplication.class)
+@WebAppConfiguration
+@IntegrationTest({ "server.port=0", "endpoints.health.sensitive=false" })
+@DirtiesContext
+public class NonSensitiveHealthTests {
+
+	@Value("${local.server.port}")
+	private int port;
+
+	@Test
+	public void testSecureHealth() throws Exception {
+		ResponseEntity<String> entity = new TestRestTemplate().getForEntity(
+				"http://localhost:" + this.port + "/health", String.class);
+		assertEquals(HttpStatus.OK, entity.getStatusCode());
+		assertTrue("Wrong body: " + entity.getBody(),
+				entity.getBody().contains("\"hello\":1"));
+	}
+
+}