Cache endpoint responses on a per-principal basis

Previously, any HTTP request to an endpoint that included a principal
would bypass the cache. This prevented authenticated requests from
making use of the cache and its configurable time-to-live.

This commit updates the caching operation invoker to include the
principal, if any, in its cache key. As a result, requests that
include a principal will make use of the cache, potentially returning
the result of a previous invocation of the same endpoint by the same
principal.

Closes gh-19538

Author: wilkinsona

spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/invoker/cache/CachingOperationInvoker.java

@@ -16,6 +16,7 @@
 
 package org.springframework.boot.actuate.endpoint.invoker.cache;
 
+import java.security.Principal;
 import java.time.Duration;
 import java.util.Map;
 import java.util.Objects;
@@ -48,7 +49,7 @@ public class CachingOperationInvoker implements OperationInvoker {
 
 	private final long timeToLive;
 
-	private final Map<ApiVersion, CachedResponse> cachedResponses;
+	private final Map<CacheKey, CachedResponse> cachedResponses;
 
 	/**
 	 * Create a new instance with the target {@link OperationInvoker} to use to compute
@@ -78,19 +79,17 @@ public Object invoke(InvocationContext context) {
 		}
 		long accessTime = System.currentTimeMillis();
 		ApiVersion contextApiVersion = context.getApiVersion();
-		CachedResponse cached = this.cachedResponses.get(contextApiVersion);
+		CacheKey cacheKey = new CacheKey(contextApiVersion, context.getSecurityContext().getPrincipal());
+		CachedResponse cached = this.cachedResponses.get(cacheKey);
 		if (cached == null || cached.isStale(accessTime, this.timeToLive)) {
 			Object response = this.invoker.invoke(context);
 			cached = createCachedResponse(response, accessTime);
-			this.cachedResponses.put(contextApiVersion, cached);
+			this.cachedResponses.put(cacheKey, cached);
 		}
 		return cached.getResponse();
 	}
 
 	private boolean hasInput(InvocationContext context) {
-		if (context.getSecurityContext().getPrincipal() != null) {
-			return true;
-		}
 		Map<String, Object> arguments = context.getArguments();
 		if (!ObjectUtils.isEmpty(arguments)) {
 			return arguments.values().stream().anyMatch(Objects::nonNull);
@@ -167,4 +166,52 @@ private static Object applyCaching(Object response, long timeToLive) {
 
 	}
 
+	private static final class CacheKey {
+
+		private final ApiVersion apiVersion;
+
+		private final Principal principal;
+
+		private CacheKey(ApiVersion apiVersion, Principal principal) {
+			this.principal = principal;
+			this.apiVersion = apiVersion;
+		}
+
+		@Override
+		public int hashCode() {
+			final int prime = 31;
+			int result = 1;
+			result = prime * result + this.apiVersion.hashCode();
+			result = prime * result + ((this.principal == null) ? 0 : this.principal.hashCode());
+			return result;
+		}
+
+		@Override
+		public boolean equals(Object obj) {
+			if (this == obj) {
+				return true;
+			}
+			if (obj == null) {
+				return false;
+			}
+			if (getClass() != obj.getClass()) {
+				return false;
+			}
+			CacheKey other = (CacheKey) obj;
+			if (this.apiVersion != other.apiVersion) {
+				return false;
+			}
+			if (this.principal == null) {
+				if (other.principal != null) {
+					return false;
+				}
+			}
+			else if (!this.principal.equals(other.principal)) {
+				return false;
+			}
+			return true;
+		}
+
+	}
+
 }

spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/invoker/cache/CachingOperationInvokerTests.java

@@ -64,6 +64,11 @@ void cacheInTtlRangeWithNoParameter() {
 		assertCacheIsUsed(Collections.emptyMap());
 	}
 
+	@Test
+	void cacheInTtlWithPrincipal() {
+		assertCacheIsUsed(Collections.emptyMap(), mock(Principal.class));
+	}
+
 	@Test
 	void cacheInTtlWithNullParameters() {
 		Map<String, Object> parameters = new HashMap<>();
@@ -97,9 +102,17 @@ void cacheInTtlWithFluxResponse() {
 	}
 
 	private void assertCacheIsUsed(Map<String, Object> parameters) {
+		assertCacheIsUsed(parameters, null);
+	}
+
+	private void assertCacheIsUsed(Map<String, Object> parameters, Principal principal) {
 		OperationInvoker target = mock(OperationInvoker.class);
 		Object expected = new Object();
-		InvocationContext context = new InvocationContext(mock(SecurityContext.class), parameters);
+		SecurityContext securityContext = mock(SecurityContext.class);
+		if (principal != null) {
+			given(securityContext.getPrincipal()).willReturn(principal);
+		}
+		InvocationContext context = new InvocationContext(securityContext, parameters);
 		given(target.invoke(context)).willReturn(expected);
 		CachingOperationInvoker invoker = new CachingOperationInvoker(target, CACHE_TTL);
 		Object response = invoker.invoke(context);
@@ -126,20 +139,46 @@ void targetAlwaysInvokedWithParameters() {
 	}
 
 	@Test
-	void targetAlwaysInvokedWithPrincipal() {
+	void targetAlwaysInvokedWithDifferentPrincipals() {
 		OperationInvoker target = mock(OperationInvoker.class);
 		Map<String, Object> parameters = new HashMap<>();
 		SecurityContext securityContext = mock(SecurityContext.class);
-		given(securityContext.getPrincipal()).willReturn(mock(Principal.class));
+		given(securityContext.getPrincipal()).willReturn(mock(Principal.class), mock(Principal.class),
+				mock(Principal.class));
 		InvocationContext context = new InvocationContext(securityContext, parameters);
-		given(target.invoke(context)).willReturn(new Object());
+		Object result1 = new Object();
+		Object result2 = new Object();
+		Object result3 = new Object();
+		given(target.invoke(context)).willReturn(result1, result2, result3);
 		CachingOperationInvoker invoker = new CachingOperationInvoker(target, CACHE_TTL);
-		invoker.invoke(context);
-		invoker.invoke(context);
-		invoker.invoke(context);
+		assertThat(invoker.invoke(context)).isEqualTo(result1);
+		assertThat(invoker.invoke(context)).isEqualTo(result2);
+		assertThat(invoker.invoke(context)).isEqualTo(result3);
 		verify(target, times(3)).invoke(context);
 	}
 
+	@Test
+	void targetInvokedWhenCalledWithAndWithoutPrincipal() {
+		OperationInvoker target = mock(OperationInvoker.class);
+		Map<String, Object> parameters = new HashMap<>();
+		SecurityContext anonymous = mock(SecurityContext.class);
+		SecurityContext authenticated = mock(SecurityContext.class);
+		given(authenticated.getPrincipal()).willReturn(mock(Principal.class));
+		InvocationContext anonymousContext = new InvocationContext(anonymous, parameters);
+		Object anonymousResult = new Object();
+		given(target.invoke(anonymousContext)).willReturn(anonymousResult);
+		InvocationContext authenticatedContext = new InvocationContext(authenticated, parameters);
+		Object authenticatedResult = new Object();
+		given(target.invoke(authenticatedContext)).willReturn(authenticatedResult);
+		CachingOperationInvoker invoker = new CachingOperationInvoker(target, CACHE_TTL);
+		assertThat(invoker.invoke(anonymousContext)).isEqualTo(anonymousResult);
+		assertThat(invoker.invoke(authenticatedContext)).isEqualTo(authenticatedResult);
+		assertThat(invoker.invoke(anonymousContext)).isEqualTo(anonymousResult);
+		assertThat(invoker.invoke(authenticatedContext)).isEqualTo(authenticatedResult);
+		verify(target, times(1)).invoke(anonymousContext);
+		verify(target, times(1)).invoke(authenticatedContext);
+	}
+
 	@Test
 	void targetInvokedWhenCacheExpires() throws InterruptedException {
 		OperationInvoker target = mock(OperationInvoker.class);

spring-boot-project/spring-boot-docs/src/docs/asciidoc/production-ready-features.adoc

@@ -406,8 +406,6 @@ The following example sets the time-to-live of the `beans` endpoint's cache to 1
 
 NOTE: The prefix `management.endpoint.<name>` is used to uniquely identify the endpoint that is being configured.
 
-NOTE: When making an authenticated HTTP request, the `Principal` is considered as input to the endpoint and, therefore, the response will not be cached.
-
 
 
 [[production-ready-endpoints-hypermedia]]