davsclaus commented on code in PR #26747:
URL: https://github.com/apache/camel/pull/26747#discussion_r4072422239
##########
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:
🟡 **This guard can't fire, and the comment describes protection it doesn't
give.**
`TimeUnit.MILLISECONDS.toNanos()` saturates rather than wrapping — it clamps
to `Long.MAX_VALUE` on overflow and never returns a negative for a positive
input. Since `remainingMillis > 0` is already established by the check above,
`remainingNanos` is always positive here.
The overflow that can actually happen is one line earlier, in `expSeconds *
1000L`: an absurd `exp` wraps to a large negative, `remainingMillis` goes
deeply negative, and the `<= 0` branch returns `0L`. That fails closed, which
is the right outcome — so the behaviour is fine either way, and
`ConcurrentMapTokenCache` reaches the same place via `isExpired()`.
So this is just tidying: either move the guard up to the multiplication
where an overflow is real, or drop it and correct the comment. As written it
reads as a deliberate safety net, and the next person to touch this will trust
it.
##########
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:
🟠**This `Math.min` — the headline change — would survive being deleted.**
Both `ConcurrentMapTokenCacheTest` and `CaffeineTokenCacheTest` test only
the two extremes:
* `exp` 60 seconds in the past → not served (that's the `isExpired()` early
return in `put`, not this line);
* `exp` 300 seconds in the future, against a TTL that is longer → served
(that's `ttlMillis` winning, not `remainingMillis`).
Neither case has `exp` land *inside* the TTL window, which is the only
situation where `remainingMillis` is the smaller value. Replacing this line
with `return ttlMillis;` leaves every test in the PR green, even though that is
precisely the bug being fixed.
A case with the TTL at, say, 300s and `exp` at `now + 2s`, asserting the
entry is no longer served shortly afterwards, would pin it down. Per
`CLAUDE.md` that wants Awaitility rather than `Thread.sleep`:
```java
await().atMost(10, TimeUnit.SECONDS).until(() ->
cache.get("short-lived-token") == null);
```
The same gap applies to `CaffeineTokenCacheTest` and its `expiryNanos`.
--
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]
