Skip to content

Commit d4b52a3

Browse files
mbhavembhave
committed
Expose Health details if user has authority
If the Princial is a Spring Security Authentication object and the request doesn't have the right roles, check the authorities. Fixes gh-8471
1 parent caefb23 commit d4b52a3

File tree

6 files changed

+102
-36
lines changed

6 files changed

+102
-36
lines changed

‎spring-boot-actuator/src/main/java/org/springframework/boot/actuate/cloudfoundry/CloudFoundryHealthMvcEndpoint.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616

1717
package org.springframework.boot.actuate.cloudfoundry;
1818

19+
import java.security.Principal;
20+
1921
import javax.servlet.http.HttpServletRequest;
2022

2123
import org.springframework.boot.actuate.endpoint.HealthEndpoint;
@@ -36,7 +38,7 @@ class CloudFoundryHealthMvcEndpoint extends HealthMvcEndpoint {
3638
}
3739

3840
@Override
39-
protected boolean exposeHealthDetails(HttpServletRequest request) {
41+
protected boolean exposeHealthDetails(HttpServletRequest request, Principal principal) {
4042
return true;
4143
}
4244

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

Lines changed: 26 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
package org.springframework.boot.actuate.endpoint.mvc;
1818

19+
import java.security.Principal;
1920
import java.util.Arrays;
2021
import java.util.HashMap;
2122
import java.util.List;
@@ -33,7 +34,10 @@
3334
import org.springframework.core.env.Environment;
3435
import org.springframework.http.HttpStatus;
3536
import org.springframework.http.ResponseEntity;
37+
import org.springframework.security.core.Authentication;
38+
import org.springframework.security.core.GrantedAuthority;
3639
import org.springframework.util.Assert;
40+
import org.springframework.util.ClassUtils;
3741
import org.springframework.util.StringUtils;
3842
import org.springframework.web.bind.annotation.ResponseBody;
3943

@@ -133,12 +137,12 @@ public void addStatusMapping(String statusCode, HttpStatus httpStatus) {
133137

134138
@ActuatorGetMapping
135139
@ResponseBody
136-
public Object invoke(HttpServletRequest request) {
140+
public Object invoke(HttpServletRequest request, Principal principal) {
137141
if (!getDelegate().isEnabled()) {
138142
// Shouldn't happen because the request mapping should not be registered
139143
return getDisabledResponse();
140144
}
141-
Health health = getHealth(request);
145+
Health health = getHealth(request, principal);
142146
HttpStatus status = getStatus(health);
143147
if (status != null) {
144148
return new ResponseEntity<Health>(health, status);
@@ -160,13 +164,13 @@ private HttpStatus getStatus(Health health) {
160164
return null;
161165
}
162166

163-
private Health getHealth(HttpServletRequest request) {
167+
private Health getHealth(HttpServletRequest request, Principal principal) {
164168
long accessTime = System.currentTimeMillis();
165169
if (isCacheStale(accessTime)) {
166170
this.lastAccess = accessTime;
167171
this.cached = getDelegate().invoke();
168172
}
169-
if (exposeHealthDetails(request)) {
173+
if (exposeHealthDetails(request, principal)) {
170174
return this.cached;
171175
}
172176
return Health.status(this.cached.getStatus()).build();
@@ -179,14 +183,24 @@ private boolean isCacheStale(long accessTime) {
179183
return (accessTime - this.lastAccess) >= getDelegate().getTimeToLive();
180184
}
181185

182-
protected boolean exposeHealthDetails(HttpServletRequest request) {
186+
protected boolean exposeHealthDetails(HttpServletRequest request, Principal principal) {
183187
if (!this.secure) {
184188
return true;
185189
}
186-
for (String role : getRoles()) {
187-
if (request.isUserInRole(role) || request.isUserInRole("ROLE_" + role)) {
190+
List<String> roles = getRoles();
191+
for (String role : roles) {
192+
if (request.isUserInRole(role)) {
188193
return true;
189194
}
195+
if (isSpringSecurityAuthentication(principal)) {
196+
Authentication authentication = (Authentication) principal;
197+
for (GrantedAuthority authority : authentication.getAuthorities()) {
198+
String name = authority.getAuthority();
199+
if (role.equals(name)) {
200+
return true;
201+
}
202+
}
203+
}
190204
}
191205
return false;
192206
}
@@ -201,4 +215,9 @@ private List<String> getRoles() {
201215
return Arrays.asList(roles);
202216
}
203217

218+
private boolean isSpringSecurityAuthentication(Principal principal) {
219+
return ClassUtils.isPresent("org.springframework.security.core.Authentication",
220+
null) && (principal instanceof Authentication);
221+
}
222+
204223
}

‎spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/HealthMvcEndpointAutoConfigurationTests.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ public void testSecureByDefault() throws Exception {
6666
this.context.refresh();
6767
MockHttpServletRequest request = new MockHttpServletRequest();
6868
Health health = (Health) this.context.getBean(HealthMvcEndpoint.class)
69-
.invoke(request);
69+
.invoke(request, null);
7070
assertThat(health.getStatus()).isEqualTo(Status.UP);
7171
assertThat(health.getDetails().get("foo")).isNull();
7272
}
@@ -80,7 +80,7 @@ public void testNotSecured() throws Exception {
8080
"management.security.enabled=false");
8181
this.context.refresh();
8282
Health health = (Health) this.context.getBean(HealthMvcEndpoint.class)
83-
.invoke(null);
83+
.invoke(null, null);
8484
assertThat(health.getStatus()).isEqualTo(Status.UP);
8585
Health map = (Health) health.getDetails().get("test");
8686
assertThat(map.getDetails().get("foo")).isEqualTo("bar");

‎spring-boot-actuator/src/test/java/org/springframework/boot/actuate/cloudfoundry/CloudFoundryHealthMvcEndpointTests.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ public void cloudFoundryHealthEndpointShouldAlwaysReturnAllHealthDetails()
4242
given(endpoint.invoke())
4343
.willReturn(new Health.Builder().up().withDetail("foo", "bar").build());
4444
given(endpoint.isSensitive()).willReturn(false);
45-
Object result = mvc.invoke(null);
45+
Object result = mvc.invoke(null, null);
4646
assertThat(result instanceof Health).isTrue();
4747
assertThat(((Health) result).getStatus() == Status.UP).isTrue();
4848
assertThat(((Health) result).getDetails().get("foo")).isEqualTo("bar");

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

Lines changed: 52 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
import java.util.Arrays;
2020
import java.util.Collections;
21+
import java.util.Set;
2122

2223
import javax.servlet.http.HttpServletRequest;
2324

@@ -34,9 +35,12 @@
3435
import org.springframework.mock.env.MockEnvironment;
3536
import org.springframework.mock.web.MockHttpServletRequest;
3637
import org.springframework.mock.web.MockServletContext;
38+
import org.springframework.security.core.Authentication;
39+
import org.springframework.security.core.authority.SimpleGrantedAuthority;
3740

3841
import static org.assertj.core.api.Assertions.assertThat;
3942
import static org.mockito.BDDMockito.given;
43+
import static org.mockito.Mockito.doReturn;
4044
import static org.mockito.Mockito.mock;
4145

4246
/**
@@ -52,7 +56,7 @@ public class HealthMvcEndpointTests {
5256

5357
private static final PropertySource<?> SECURITY_ROLES = new MapPropertySource("test",
5458
Collections.<String, Object>singletonMap("management.security.roles",
55-
"HERO, USER"));
59+
"HERO"));
5660

5761
private HttpServletRequest request = new MockHttpServletRequest();
5862

@@ -62,13 +66,11 @@ public class HealthMvcEndpointTests {
6266

6367
private MockEnvironment environment;
6468

65-
private HttpServletRequest user = createAuthenticationToken("ROLE_USER");
69+
private HttpServletRequest defaultUser = createAuthenticationRequest("ROLE_ACTUATOR");
6670

67-
private HttpServletRequest actuator = createAuthenticationToken("ROLE_ACTUATOR");
71+
private HttpServletRequest hero = createAuthenticationRequest("HERO");
6872

69-
private HttpServletRequest hero = createAuthenticationToken("ROLE_HERO");
70-
71-
private HttpServletRequest createAuthenticationToken(String role) {
73+
private HttpServletRequest createAuthenticationRequest(String role) {
7274
MockServletContext servletContext = new MockServletContext();
7375
servletContext.declareRoles(role);
7476
return new MockHttpServletRequest(servletContext);
@@ -86,7 +88,7 @@ public void init() {
8688
@Test
8789
public void up() {
8890
given(this.endpoint.invoke()).willReturn(new Health.Builder().up().build());
89-
Object result = this.mvc.invoke(this.request);
91+
Object result = this.mvc.invoke(this.request, null);
9092
assertThat(result instanceof Health).isTrue();
9193
assertThat(((Health) result).getStatus() == Status.UP).isTrue();
9294
}
@@ -95,7 +97,7 @@ public void up() {
9597
@Test
9698
public void down() {
9799
given(this.endpoint.invoke()).willReturn(new Health.Builder().down().build());
98-
Object result = this.mvc.invoke(this.request);
100+
Object result = this.mvc.invoke(this.request, null);
99101
assertThat(result instanceof ResponseEntity).isTrue();
100102
ResponseEntity<Health> response = (ResponseEntity<Health>) result;
101103
assertThat(response.getBody().getStatus() == Status.DOWN).isTrue();
@@ -109,7 +111,7 @@ public void customMapping() {
109111
.willReturn(new Health.Builder().status("OK").build());
110112
this.mvc.setStatusMapping(
111113
Collections.singletonMap("OK", HttpStatus.INTERNAL_SERVER_ERROR));
112-
Object result = this.mvc.invoke(this.request);
114+
Object result = this.mvc.invoke(this.request, null);
113115
assertThat(result instanceof ResponseEntity).isTrue();
114116
ResponseEntity<Health> response = (ResponseEntity<Health>) result;
115117
assertThat(response.getBody().getStatus().equals(new Status("OK"))).isTrue();
@@ -123,7 +125,7 @@ public void customMappingWithRelaxedName() {
123125
.willReturn(new Health.Builder().outOfService().build());
124126
this.mvc.setStatusMapping(Collections.singletonMap("out-of-service",
125127
HttpStatus.INTERNAL_SERVER_ERROR));
126-
Object result = this.mvc.invoke(this.request);
128+
Object result = this.mvc.invoke(this.request, null);
127129
assertThat(result instanceof ResponseEntity).isTrue();
128130
ResponseEntity<Health> response = (ResponseEntity<Health>) result;
129131
assertThat(response.getBody().getStatus().equals(Status.OUT_OF_SERVICE)).isTrue();
@@ -134,7 +136,7 @@ public void customMappingWithRelaxedName() {
134136
public void presenceOfRightRoleShouldExposeDetails() {
135137
given(this.endpoint.invoke())
136138
.willReturn(new Health.Builder().up().withDetail("foo", "bar").build());
137-
Object result = this.mvc.invoke(this.actuator);
139+
Object result = this.mvc.invoke(this.defaultUser, null);
138140
assertThat(result instanceof Health).isTrue();
139141
assertThat(((Health) result).getStatus() == Status.UP).isTrue();
140142
assertThat(((Health) result).getDetails().get("foo")).isEqualTo("bar");
@@ -145,7 +147,7 @@ public void managementSecurityDisabledShouldExposeDetails() throws Exception {
145147
this.mvc = new HealthMvcEndpoint(this.endpoint, false);
146148
given(this.endpoint.invoke())
147149
.willReturn(new Health.Builder().up().withDetail("foo", "bar").build());
148-
Object result = this.mvc.invoke(this.user);
150+
Object result = this.mvc.invoke(this.defaultUser, null);
149151
assertThat(result instanceof Health).isTrue();
150152
assertThat(((Health) result).getStatus() == Status.UP).isTrue();
151153
assertThat(((Health) result).getDetails().get("foo")).isEqualTo("bar");
@@ -155,18 +157,32 @@ public void managementSecurityDisabledShouldExposeDetails() throws Exception {
155157
public void rightRoleNotPresentShouldNotExposeDetails() {
156158
given(this.endpoint.invoke())
157159
.willReturn(new Health.Builder().up().withDetail("foo", "bar").build());
158-
Object result = this.mvc.invoke(this.user);
160+
Object result = this.mvc.invoke(this.hero, null);
159161
assertThat(result instanceof Health).isTrue();
160162
assertThat(((Health) result).getStatus() == Status.UP).isTrue();
161163
assertThat(((Health) result).getDetails().get("foo")).isNull();
162164
}
163165

166+
@Test
167+
public void rightAuthorityPresentShouldExposeDetails() throws Exception {
168+
this.environment.getPropertySources().addLast(SECURITY_ROLES);
169+
Authentication principal = mock(Authentication.class);
170+
Set<SimpleGrantedAuthority> authorities = Collections.singleton(new SimpleGrantedAuthority("HERO"));
171+
doReturn(authorities).when(principal).getAuthorities();
172+
given(this.endpoint.invoke())
173+
.willReturn(new Health.Builder().up().withDetail("foo", "bar").build());
174+
Object result = this.mvc.invoke(this.defaultUser, principal);
175+
assertThat(result instanceof Health).isTrue();
176+
assertThat(((Health) result).getStatus() == Status.UP).isTrue();
177+
assertThat(((Health) result).getDetails().get("foo")).isEqualTo("bar");
178+
}
179+
164180
@Test
165181
public void customRolePresentShouldExposeDetails() {
166182
this.environment.getPropertySources().addLast(SECURITY_ROLES);
167183
given(this.endpoint.invoke())
168184
.willReturn(new Health.Builder().up().withDetail("foo", "bar").build());
169-
Object result = this.mvc.invoke(this.hero);
185+
Object result = this.mvc.invoke(this.hero, null);
170186
assertThat(result instanceof Health).isTrue();
171187
assertThat(((Health) result).getStatus() == Status.UP).isTrue();
172188
assertThat(((Health) result).getDetails().get("foo")).isEqualTo("bar");
@@ -177,38 +193,51 @@ public void customRoleShouldNotExposeDetailsForDefaultRole() {
177193
this.environment.getPropertySources().addLast(SECURITY_ROLES);
178194
given(this.endpoint.invoke())
179195
.willReturn(new Health.Builder().up().withDetail("foo", "bar").build());
180-
Object result = this.mvc.invoke(this.actuator);
196+
Object result = this.mvc.invoke(this.defaultUser, null);
181197
assertThat(result instanceof Health).isTrue();
182198
assertThat(((Health) result).getStatus() == Status.UP).isTrue();
183199
assertThat(((Health) result).getDetails().get("foo")).isNull();
184200
}
185201

186202
@Test
187-
public void customRoleFromListShouldNotExposeDetailsForDefaultRole() {
203+
public void customRoleFromListShouldExposeDetails() {
188204
// gh-8314
189205
this.mvc = new HealthMvcEndpoint(this.endpoint, true,
190206
Arrays.asList("HERO", "USER"));
191207
given(this.endpoint.invoke())
192208
.willReturn(new Health.Builder().up().withDetail("foo", "bar").build());
193-
Object result = this.mvc.invoke(this.hero);
209+
Object result = this.mvc.invoke(this.hero, null);
194210
assertThat(result instanceof Health).isTrue();
195211
assertThat(((Health) result).getStatus() == Status.UP).isTrue();
196212
assertThat(((Health) result).getDetails().get("foo")).isEqualTo("bar");
197213
}
198214

215+
@Test
216+
public void customRoleFromListShouldNotExposeDetailsForDefaultRole() {
217+
// gh-8314
218+
this.mvc = new HealthMvcEndpoint(this.endpoint, true,
219+
Arrays.asList("HERO", "USER"));
220+
given(this.endpoint.invoke())
221+
.willReturn(new Health.Builder().up().withDetail("foo", "bar").build());
222+
Object result = this.mvc.invoke(this.defaultUser, null);
223+
assertThat(result instanceof Health).isTrue();
224+
assertThat(((Health) result).getStatus() == Status.UP).isTrue();
225+
assertThat(((Health) result).getDetails().get("foo")).isNull();
226+
}
227+
199228
@Test
200229
public void healthIsCached() {
201230
given(this.endpoint.getTimeToLive()).willReturn(10000L);
202231
given(this.endpoint.invoke())
203232
.willReturn(new Health.Builder().up().withDetail("foo", "bar").build());
204-
Object result = this.mvc.invoke(this.actuator);
233+
Object result = this.mvc.invoke(this.defaultUser, null);
205234
assertThat(result instanceof Health).isTrue();
206235
Health health = (Health) result;
207236
assertThat(health.getStatus() == Status.UP).isTrue();
208237
assertThat(health.getDetails()).hasSize(1);
209238
assertThat(health.getDetails().get("foo")).isEqualTo("bar");
210239
given(this.endpoint.invoke()).willReturn(new Health.Builder().down().build());
211-
result = this.mvc.invoke(this.request); // insecure now
240+
result = this.mvc.invoke(this.request, null); // insecure now
212241
assertThat(result instanceof Health).isTrue();
213242
health = (Health) result;
214243
// so the result is cached
@@ -222,11 +251,11 @@ public void noCachingWhenTimeToLiveIsZero() {
222251
given(this.endpoint.getTimeToLive()).willReturn(0L);
223252
given(this.endpoint.invoke())
224253
.willReturn(new Health.Builder().up().withDetail("foo", "bar").build());
225-
Object result = this.mvc.invoke(this.request);
254+
Object result = this.mvc.invoke(this.request, null);
226255
assertThat(result instanceof Health).isTrue();
227256
assertThat(((Health) result).getStatus() == Status.UP).isTrue();
228257
given(this.endpoint.invoke()).willReturn(new Health.Builder().down().build());
229-
result = this.mvc.invoke(this.request);
258+
result = this.mvc.invoke(this.request, null);
230259
@SuppressWarnings("unchecked")
231260
Health health = ((ResponseEntity<Health>) result).getBody();
232261
assertThat(health.getStatus() == Status.DOWN).isTrue();
@@ -237,12 +266,12 @@ public void newValueIsReturnedOnceTtlExpires() throws InterruptedException {
237266
given(this.endpoint.getTimeToLive()).willReturn(50L);
238267
given(this.endpoint.invoke())
239268
.willReturn(new Health.Builder().up().withDetail("foo", "bar").build());
240-
Object result = this.mvc.invoke(this.request);
269+
Object result = this.mvc.invoke(this.request, null);
241270
assertThat(result instanceof Health).isTrue();
242271
assertThat(((Health) result).getStatus() == Status.UP).isTrue();
243272
Thread.sleep(100);
244273
given(this.endpoint.invoke()).willReturn(new Health.Builder().down().build());
245-
result = this.mvc.invoke(this.request);
274+
result = this.mvc.invoke(this.request, null);
246275
@SuppressWarnings("unchecked")
247276
Health health = ((ResponseEntity<Health>) result).getBody();
248277
assertThat(health.getStatus() == Status.DOWN).isTrue();

0 commit comments

Comments
 (0)