From 38eeae216604cca7bca730847a8bf45fe85392e8 Mon Sep 17 00:00:00 2001 From: Madhura Bhave Date: Tue, 13 Dec 2016 00:27:40 -0800 Subject: [PATCH 1/2] Send error with message from Endpoint MVC security Update `MvcEndpointSecurityInterceptor` to that it sends an error in the same way as Spring Security. Prior to this commit the `ErrorController` would not handle endpoint security errors. Fixes gh-7605 Closes gh-7634 --- .../mvc/MvcEndpointSecurityInterceptor.java | 18 +++++++++---- .../MvcEndpointSecurityInterceptorTests.java | 27 ++++++++++++++++--- 2 files changed, 36 insertions(+), 9 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 08d4dba5c0..7800dd5e0c 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 @@ -59,17 +59,25 @@ 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()); + StringBuilder message = new StringBuilder(); + for (String role : this.roles) { + message.append(role).append(" "); + } + response.sendError(HttpStatus.FORBIDDEN.value(), + "Access is denied. User must have one of the these roles: " + + message.toString().trim()); } else { - response.setStatus(HttpStatus.UNAUTHORIZED.value()); + response.sendError(HttpStatus.UNAUTHORIZED.value(), + "Full authentication is required to access this resource. " + + "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 8586bdd7f7..bc8f347763 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 @@ -16,19 +16,24 @@ 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.Test; import org.springframework.boot.actuate.endpoint.AbstractEndpoint; +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}. @@ -47,7 +52,7 @@ public class MvcEndpointSecurityInterceptorTests { private MockHttpServletRequest request; - private MockHttpServletResponse response; + private HttpServletResponse response; private MockServletContext servletContext; @@ -62,7 +67,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 +92,25 @@ 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. " + + "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 { From c76bd2d81e633292e43b3f583f75e435375593ab Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Tue, 20 Dec 2016 18:47:39 -0800 Subject: [PATCH 2/2] 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 roles; + private AtomicBoolean loggedUnauthorizedAttempt = new AtomicBoolean(); + public MvcEndpointSecurityInterceptor(boolean secure, List 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