oscerd commented on code in PR #26747:
URL: https://github.com/apache/camel/pull/26747#discussion_r4122493821
##########
components/camel-keycloak/src/test/java/org/apache/camel/component/keycloak/security/cache/ConcurrentMapTokenCacheTest.java:
##########
@@ -172,4 +172,35 @@ void testConcurrentAccess() throws InterruptedException {
assertEquals(threadCount, cache.size());
}
+
+ @Test
+ void testExpiredResultNotServed() {
+ // A result whose token has already expired must not be served, even
while the configured TTL has not elapsed.
+ Map<String, Object> claims = new HashMap<>();
+ claims.put("active", true);
+ claims.put("sub", "test-user");
+ claims.put("exp", System.currentTimeMillis() / 1000 - 60); // expired
60 seconds ago
+ KeycloakTokenIntrospector.IntrospectionResult expired
+ = new KeycloakTokenIntrospector.IntrospectionResult(claims);
+
+ cache.put("expired-token", expired);
+
+ assertNull(cache.get("expired-token"));
Review Comment:
Addressed in e9580c7 and f7cff3f.
`testResultExpiringBeforeTtlNotServedAfterExp` (in both
`ConcurrentMapTokenCacheTest` and `CaffeineTokenCacheTest`) puts a token whose
`exp` is about 2 s away against a 300 s TTL, then waits with Awaitility until
it is no longer served. That exercises the shortened TTL rather than `put()`'s
early return. The already-expired test's comment now says it covers the early
return and points to that test.
_Claude Code on behalf of @oscerd_
##########
components/camel-keycloak/src/test/java/org/apache/camel/component/keycloak/security/KeycloakSecurityProcessorTest.java:
##########
@@ -518,4 +518,25 @@ public KeycloakTokenIntrospector getTokenIntrospector() {
assertFalse(routeReached.get(),
"Route body must not be reached when the token has the
required permission but the wrong authorized party");
}
+
+ @Test
+ void testActiveButExpiredIntrospectionResultRejected() throws Exception {
Review Comment:
Addressed in e9580c7.
`testActiveButExpiredIntrospectionResultRejectedOnRolesPath` and
`testActiveButExpiredIntrospectionResultRejectedOnPermissionsPath` cover
`validateRoles()` and `validatePermissions()`. They assert that the expiry
rejection fires before the role or permission check.
_Claude Code on behalf of @oscerd_
##########
components/camel-keycloak/src/main/java/org/apache/camel/component/keycloak/security/cache/ConcurrentMapTokenCache.java:
##########
@@ -64,11 +64,30 @@ public KeycloakTokenIntrospector.IntrospectionResult
get(String token) {
@Override
public void put(String token,
KeycloakTokenIntrospector.IntrospectionResult result) {
- cache.put(token, new CachedEntry(result, ttlMillis));
+ if (result.isExpired()) {
+ // Never cache a result whose token has already expired: it must
not be served on a later hit.
+ LOG.trace("Token already expired; skipping cache put");
+ return;
+ }
+ cache.put(token, new CachedEntry(result, effectiveTtlMillis(result)));
LOG.trace("Token introspection result cached");
cleanupExpiredEntries();
}
+ /**
+ * Computes the effective time-to-live for a result, bounding the
configured TTL by the token's own remaining
+ * validity so a cached result is never returned after the token's {@code
exp}. Results without an {@code exp} claim
+ * keep the configured TTL.
+ */
+ private long
effectiveTtlMillis(KeycloakTokenIntrospector.IntrospectionResult result) {
+ Long expSeconds = result.getExpiration();
+ if (expSeconds == null) {
+ return ttlMillis;
+ }
+ long remainingMillis = expSeconds * 1000L - System.currentTimeMillis();
+ return Math.min(ttlMillis, remainingMillis);
Review Comment:
Addressed in e9580c7. Both cache tests now have
`testResultExpiringBeforeTtlNotServedAfterExp` with `exp` inside the TTL window
(about 2 s against 300 s), so replacing the `min` with `return ttlMillis;` (or
`return maxTtlNanos;` in the Caffeine cache) now fails. It waits with
`await().atMost(10, SECONDS)` rather than a sleep.
_Claude Code on behalf of @oscerd_
##########
components/camel-keycloak/src/main/java/org/apache/camel/component/keycloak/security/cache/CaffeineTokenCache.java:
##########
@@ -126,4 +127,56 @@ public CacheStats getStats() {
public Cache<String, KeycloakTokenIntrospector.IntrospectionResult>
getCaffeineCache() {
return cache;
}
+
+ /**
+ * Caffeine expiry policy that bounds each entry's lifetime by the smaller
of the configured TTL and the token's own
+ * remaining validity ({@code exp}), so a cached introspection result is
never returned after the token has expired.
+ * Reads do not extend an entry's lifetime.
+ */
+ private static final class IntrospectionExpiry
+ implements Expiry<String,
KeycloakTokenIntrospector.IntrospectionResult> {
+
+ private final long maxTtlNanos;
+
+ IntrospectionExpiry(long maxTtlNanos) {
+ this.maxTtlNanos = maxTtlNanos;
+ }
+
+ @Override
+ public long expireAfterCreate(
+ String key, KeycloakTokenIntrospector.IntrospectionResult
value, long currentTime) {
+ return expiryNanos(value);
+ }
+
+ @Override
+ public long expireAfterUpdate(
+ String key, KeycloakTokenIntrospector.IntrospectionResult
value, long currentTime, long currentDuration) {
+ return expiryNanos(value);
+ }
+
+ @Override
+ public long expireAfterRead(
+ String key, KeycloakTokenIntrospector.IntrospectionResult
value, long currentTime, long currentDuration) {
+ // Reads must not extend the cached lifetime beyond the token's
expiry.
+ return currentDuration;
+ }
+
+ private long expiryNanos(KeycloakTokenIntrospector.IntrospectionResult
value) {
+ Long expSeconds = value.getExpiration();
+ if (expSeconds == null) {
+ return maxTtlNanos;
+ }
+ long remainingMillis = expSeconds * 1000L -
System.currentTimeMillis();
+ if (remainingMillis <= 0) {
+ // Already expired: expire immediately so the entry is not
served.
+ return 0L;
+ }
+ long remainingNanos =
TimeUnit.MILLISECONDS.toNanos(remainingMillis);
+ if (remainingNanos < 0) {
Review Comment:
Addressed in e9580c7. The unreachable guard is gone. The comment on the
`remainingMillis <= 0` branch now names the `expSeconds * 1000L` wrap as the
overflow that actually reaches it, which fails closed, and notes that
`toNanos()` saturates, so `min()` still yields the TTL for a far-future `exp`.
_Claude Code on behalf of @oscerd_
##########
components/camel-keycloak/src/test/java/org/apache/camel/component/keycloak/security/cache/ConcurrentMapTokenCacheTest.java:
##########
@@ -172,4 +174,54 @@ void testConcurrentAccess() throws InterruptedException {
assertEquals(threadCount, cache.size());
}
+
+ @Test
+ void testExpiredResultNotServed() {
+ // A result whose token has already expired must not be served, even
while the configured TTL has not elapsed.
+ Map<String, Object> claims = new HashMap<>();
Review Comment:
Addressed in f7cff3f. The comment now says the mechanism is `put()`'s
`isExpired()` early return, so no entry is ever inserted, and points to
`testResultExpiringBeforeTtlNotServedAfterExp` for the TTL bounding. The
Caffeine test's comment was corrected the same way: `expiryNanos` returns 0
there.
_Claude Code on behalf of @oscerd_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
