Merge pull request #7634 from mbhave/gh-7605

* pr/7634:
  Refine error message from Endpoint MVC security
  Send error with message from Endpoint MVC security

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

@@ -17,11 +17,16 @@
 package org.springframework.boot.actuate.endpoint.mvc;
 
 import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
 
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+
 import org.springframework.http.HttpStatus;
+import org.springframework.util.StringUtils;
 import org.springframework.web.cors.CorsUtils;
 import org.springframework.web.method.HandlerMethod;
 import org.springframework.web.servlet.handler.HandlerInterceptorAdapter;
@@ -34,10 +39,15 @@ import org.springframework.web.servlet.handler.HandlerInterceptorAdapter;
  */
 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(boolean secure, List<String> roles) {
 		this.secure = secure;
 		this.roles = roles;
@@ -59,17 +69,30 @@ public class MvcEndpointSecurityInterceptor extends HandlerInterceptorAdapter {
 				return true;
 			}
 		}
-		setFailureResponseStatus(request, response);
+		sendFailureResponse(request, response);
 		return false;
 	}
 
-	private void setFailureResponseStatus(HttpServletRequest request,
-			HttpServletResponse response) {
+	private void sendFailureResponse(HttpServletRequest request,
+			HttpServletResponse response) throws Exception {
 		if (request.getUserPrincipal() != null) {
-			response.setStatus(HttpStatus.FORBIDDEN.value());
+			String roles = StringUtils.collectionToDelimitedString(this.roles, " ");
+			response.sendError(HttpStatus.FORBIDDEN.value(),
+					"Access is denied. User must have one of the these roles: " + roles);
 		}
 		else {
-			response.setStatus(HttpStatus.UNAUTHORIZED.value());
+			logUnauthorizedAttempt();
+			response.sendError(HttpStatus.UNAUTHORIZED.value(),
+					"Full authentication is required to access this resource.");
+		}
+	}
+
+	private void logUnauthorizedAttempt() {
+		if (this.loggedUnauthorizedAttempt.compareAndSet(false, true)
+				&& logger.isInfoEnabled()) {
+			logger.info("Full authentication is required to access "
+					+ "actuator endpoints. Consider adding Spring Security "
+					+ "or set 'management.security.enabled' to false.");
 		}
 	}
 

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

@@ -16,19 +16,26 @@
 
 package org.springframework.boot.actuate.endpoint.mvc;
 
+import java.security.Principal;
 import java.util.Arrays;
 import java.util.List;
 
+import javax.servlet.http.HttpServletResponse;
+
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
 
 import org.springframework.boot.actuate.endpoint.AbstractEndpoint;
+import org.springframework.boot.test.rule.OutputCapture;
+import org.springframework.http.HttpStatus;
 import org.springframework.mock.web.MockHttpServletRequest;
-import org.springframework.mock.web.MockHttpServletResponse;
 import org.springframework.mock.web.MockServletContext;
 import org.springframework.web.method.HandlerMethod;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
 
 /**
  * Tests for {@link MvcEndpointSecurityInterceptor}.
@@ -37,6 +44,9 @@ import static org.assertj.core.api.Assertions.assertThat;
  */
 public class MvcEndpointSecurityInterceptorTests {
 
+	@Rule
+	public OutputCapture output = new OutputCapture();
+
 	private MvcEndpointSecurityInterceptor securityInterceptor;
 
 	private TestMvcEndpoint mvcEndpoint;
@@ -47,7 +57,7 @@ public class MvcEndpointSecurityInterceptorTests {
 
 	private MockHttpServletRequest request;
 
-	private MockHttpServletResponse response;
+	private HttpServletResponse response;
 
 	private MockServletContext servletContext;
 
@@ -62,7 +72,7 @@ public class MvcEndpointSecurityInterceptorTests {
 		this.handlerMethod = new HandlerMethod(this.mvcEndpoint, "invoke");
 		this.servletContext = new MockServletContext();
 		this.request = new MockHttpServletRequest(this.servletContext);
-		this.response = new MockHttpServletResponse();
+		this.response = mock(HttpServletResponse.class);
 	}
 
 	@Test
@@ -87,11 +97,30 @@ public class MvcEndpointSecurityInterceptorTests {
 	}
 
 	@Test
-	public void sensitiveEndpointIfRoleIsNotPresentShouldNotAllowAccess()
+	public void sensitiveEndpointIfNotAuthenticatedShouldNotAllowAccess()
 			throws Exception {
+		assertThat(this.securityInterceptor.preHandle(this.request, this.response,
+				this.handlerMethod)).isFalse();
+		verify(this.response).sendError(HttpStatus.UNAUTHORIZED.value(),
+				"Full authentication is required to access this resource.");
+		assertThat(this.securityInterceptor.preHandle(this.request, this.response,
+				this.handlerMethod)).isFalse();
+		assertThat(this.output.toString())
+				.containsOnlyOnce("Full authentication is required to access actuator "
+						+ "endpoints. Consider adding Spring Security or set "
+						+ "'management.security.enabled' to false");
+	}
+
+	@Test
+	public void sensitiveEndpointIfRoleIsNotCorrectShouldNotAllowAccess()
+			throws Exception {
+		Principal principal = mock(Principal.class);
+		this.request.setUserPrincipal(principal);
 		this.servletContext.declareRoles("HERO");
 		assertThat(this.securityInterceptor.preHandle(this.request, this.response,
 				this.handlerMethod)).isFalse();
+		verify(this.response).sendError(HttpStatus.FORBIDDEN.value(),
+				"Access is denied. User must have one of the these roles: SUPER_HERO");
 	}
 
 	private static class TestEndpoint extends AbstractEndpoint<Object> {