From dced154f71d9624d47fbaed6108679d1c6dc1947 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Edd=C3=BA=20Mel=C3=A9ndez?= Date: Mon, 1 Aug 2016 09:01:08 -0700 Subject: [PATCH 1/2] Fix health endpoint security Commit b02aba4 has renamed `management.security.role` to `management.security.roles`. Unfortunately, the `HealthMvcEndpoint` was still looking at the old property. This commit makes sure that the proper key is used and any custom role is applied rather than an unconditional `ADMIN` role. See gh-6540 --- .../endpoint/mvc/HealthMvcEndpoint.java | 17 ++++++++++++++--- .../endpoint/mvc/HealthMvcEndpointTests.java | 18 ++++++++++++++++++ 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/HealthMvcEndpoint.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/HealthMvcEndpoint.java index 5167f6f798..adb34e8c34 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/HealthMvcEndpoint.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/HealthMvcEndpoint.java @@ -17,7 +17,10 @@ package org.springframework.boot.actuate.endpoint.mvc; import java.security.Principal; +import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; +import java.util.List; import java.util.Map; import org.springframework.boot.actuate.endpoint.HealthEndpoint; @@ -35,6 +38,7 @@ import org.springframework.security.core.Authentication; import org.springframework.security.core.GrantedAuthority; import org.springframework.util.Assert; import org.springframework.util.ClassUtils; +import org.springframework.util.StringUtils; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.ResponseBody; @@ -45,6 +49,7 @@ import org.springframework.web.bind.annotation.ResponseBody; * @author Dave Syer * @author Andy Wilkinson * @author Phillip Webb + * @author Eddú Meléndez * @since 1.1.0 */ @ConfigurationProperties(prefix = "endpoints.health") @@ -184,11 +189,17 @@ public class HealthMvcEndpoint extends AbstractEndpointMvcAdapter roles = Arrays.asList(StringUtils.trimArrayElements(StringUtils + .commaDelimitedListToStringArray(this.roleResolver.getProperty("roles")))); + if (roles.isEmpty()) { + roles = Collections.singletonList("ROLE_ADMIN"); + } for (GrantedAuthority authority : authentication.getAuthorities()) { String name = authority.getAuthority(); - if (role.equals(name) || ("ROLE_" + role).equals(name)) { - return true; + for (String role : roles) { + if (role.equals(name) || ("ROLE_" + role).equals(name)) { + return true; + } } } } diff --git a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/mvc/HealthMvcEndpointTests.java b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/mvc/HealthMvcEndpointTests.java index 2bee0f8b5d..e403892366 100644 --- a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/mvc/HealthMvcEndpointTests.java +++ b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/mvc/HealthMvcEndpointTests.java @@ -42,6 +42,7 @@ import static org.mockito.Mockito.mock; * @author Christian Dupuis * @author Dave Syer * @author Andy Wilkinson + * @author Eddú Meléndez */ public class HealthMvcEndpointTests { @@ -49,6 +50,10 @@ public class HealthMvcEndpointTests { Collections.singletonMap("endpoints.health.sensitive", "false")); + private static final PropertySource SECURITY_ROLES = new MapPropertySource("test", + Collections.singletonMap("management.security.roles", + "HERO, USER")); + private HealthEndpoint endpoint = null; private HealthMvcEndpoint mvc = null; @@ -140,6 +145,19 @@ public class HealthMvcEndpointTests { assertThat(((Health) result).getDetails().get("foo")).isNull(); } + @Test + public void secureCustomRole() { + this.mvc = new HealthMvcEndpoint(this.endpoint, false); + this.mvc.setEnvironment(this.environment); + this.environment.getPropertySources().addLast(SECURITY_ROLES); + given(this.endpoint.invoke()) + .willReturn(new Health.Builder().up().withDetail("foo", "bar").build()); + Object result = this.mvc.invoke(this.user); + assertThat(result instanceof Health).isTrue(); + assertThat(((Health) result).getStatus() == Status.UP).isTrue(); + assertThat(((Health) result).getDetails().get("foo")).isEqualTo("bar"); + } + @Test public void healthIsCached() { given(this.endpoint.getTimeToLive()).willReturn(10000L); From 4882544c843a0a04764b05747b627c063614c18f Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Sat, 13 Aug 2016 08:02:04 +0200 Subject: [PATCH 2/2] Polish contribution Closes gh-6540 --- .../endpoint/mvc/HealthMvcEndpoint.java | 6 +--- .../endpoint/mvc/HealthMvcEndpointTests.java | 31 +++++++++++++------ 2 files changed, 23 insertions(+), 14 deletions(-) diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/HealthMvcEndpoint.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/HealthMvcEndpoint.java index adb34e8c34..2e65671ed9 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/HealthMvcEndpoint.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/HealthMvcEndpoint.java @@ -18,7 +18,6 @@ package org.springframework.boot.actuate.endpoint.mvc; import java.security.Principal; import java.util.Arrays; -import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -190,10 +189,7 @@ public class HealthMvcEndpoint extends AbstractEndpointMvcAdapter roles = Arrays.asList(StringUtils.trimArrayElements(StringUtils - .commaDelimitedListToStringArray(this.roleResolver.getProperty("roles")))); - if (roles.isEmpty()) { - roles = Collections.singletonList("ROLE_ADMIN"); - } + .commaDelimitedListToStringArray(this.roleResolver.getProperty("roles", "ROLE_ADMIN")))); for (GrantedAuthority authority : authentication.getAuthorities()) { String name = authority.getAuthority(); for (String role : roles) { diff --git a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/mvc/HealthMvcEndpointTests.java b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/mvc/HealthMvcEndpointTests.java index e403892366..e03ab21ac5 100644 --- a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/mvc/HealthMvcEndpointTests.java +++ b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/mvc/HealthMvcEndpointTests.java @@ -60,13 +60,17 @@ public class HealthMvcEndpointTests { private MockEnvironment environment; - private UsernamePasswordAuthenticationToken user = new UsernamePasswordAuthenticationToken( - "user", "password", - AuthorityUtils.commaSeparatedStringToAuthorityList("ROLE_USER")); + private UsernamePasswordAuthenticationToken user = createAuthenticationToken("ROLE_USER"); - private UsernamePasswordAuthenticationToken admin = new UsernamePasswordAuthenticationToken( - "user", "password", - AuthorityUtils.commaSeparatedStringToAuthorityList("ROLE_ADMIN")); + private UsernamePasswordAuthenticationToken admin = createAuthenticationToken("ROLE_ADMIN"); + + private UsernamePasswordAuthenticationToken hero = createAuthenticationToken("ROLE_HERO"); + + private UsernamePasswordAuthenticationToken createAuthenticationToken(String authority) { + return new UsernamePasswordAuthenticationToken( + "user", "password", + AuthorityUtils.commaSeparatedStringToAuthorityList(authority)); + } @Before public void init() { @@ -147,17 +151,26 @@ public class HealthMvcEndpointTests { @Test public void secureCustomRole() { - this.mvc = new HealthMvcEndpoint(this.endpoint, false); - this.mvc.setEnvironment(this.environment); this.environment.getPropertySources().addLast(SECURITY_ROLES); given(this.endpoint.invoke()) .willReturn(new Health.Builder().up().withDetail("foo", "bar").build()); - Object result = this.mvc.invoke(this.user); + Object result = this.mvc.invoke(this.hero); assertThat(result instanceof Health).isTrue(); assertThat(((Health) result).getStatus() == Status.UP).isTrue(); assertThat(((Health) result).getDetails().get("foo")).isEqualTo("bar"); } + @Test + public void secureCustomRoleNoAccess() { + this.environment.getPropertySources().addLast(SECURITY_ROLES); + given(this.endpoint.invoke()) + .willReturn(new Health.Builder().up().withDetail("foo", "bar").build()); + Object result = this.mvc.invoke(this.admin); + assertThat(result instanceof Health).isTrue(); + assertThat(((Health) result).getStatus() == Status.UP).isTrue(); + assertThat(((Health) result).getDetails().get("foo")).isNull(); + } + @Test public void healthIsCached() { given(this.endpoint.getTimeToLive()).willReturn(10000L);