From c76bd2d81e633292e43b3f583f75e435375593ab Mon Sep 17 00:00:00 2001
From: Phillip Webb <pwebb@pivotal.io>
Date: Tue, 20 Dec 2016 18:47:39 -0800
Subject: [PATCH] Refine error message from Endpoint MVC security

Update the error message to return less information to the client.
Details of how to disable security are now written to the log instead.

See gh-7605
See gh-7634
---
 .../mvc/MvcEndpointSecurityInterceptor.java   | 31 ++++++++++++++-----
 .../MvcEndpointSecurityInterceptorTests.java  | 14 +++++++--
 2 files changed, 35 insertions(+), 10 deletions(-)

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 7800dd5e0c..e9c6d3b785 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
@@ -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;
@@ -66,18 +76,23 @@ public class MvcEndpointSecurityInterceptor extends HandlerInterceptorAdapter {
 	private void sendFailureResponse(HttpServletRequest request,
 			HttpServletResponse response) throws Exception {
 		if (request.getUserPrincipal() != null) {
-			StringBuilder message = new StringBuilder();
-			for (String role : this.roles) {
-				message.append(role).append(" ");
-			}
+			String roles = StringUtils.collectionToDelimitedString(this.roles, " ");
 			response.sendError(HttpStatus.FORBIDDEN.value(),
-					"Access is denied. User must have one of the these roles: "
-							+ message.toString().trim());
+					"Access is denied. User must have one of the these roles: " + roles);
 		}
 		else {
+			logUnauthorizedAttempt();
 			response.sendError(HttpStatus.UNAUTHORIZED.value(),
-					"Full authentication is required to access this resource. "
-							+ "Consider adding Spring Security or set management.security.enabled to false.");
+					"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.");
 		}
 	}
 
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 bc8f347763..cbfa698133 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
@@ -23,9 +23,11 @@ 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.MockServletContext;
@@ -42,6 +44,9 @@ import static org.mockito.Mockito.verify;
  */
 public class MvcEndpointSecurityInterceptorTests {
 
+	@Rule
+	public OutputCapture output = new OutputCapture();
+
 	private MvcEndpointSecurityInterceptor securityInterceptor;
 
 	private TestMvcEndpoint mvcEndpoint;
@@ -97,8 +102,13 @@ public class MvcEndpointSecurityInterceptorTests {
 		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. "
-						+ "Consider adding Spring Security or set management.security.enabled to false.");
+				"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