gnodet-bot commented on code in PR #26747:
URL: https://github.com/apache/camel/pull/26747#discussion_r4071739097
##########
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:
⚠️ **Trivially-passing test** — this assertion succeeds for the wrong reason.
The new `put()` has a guard at the top: `if (result.isExpired()) { return;
}`. Since the token in this test has `exp = now - 60s`, `isExpired()` returns
`true` and the entry is **never inserted**. `cache.get("expired-token")`
returns `null` simply because the key is absent, not because the cache's TTL
bounding evicted it.
The test as written does not exercise `effectiveTtlMillis()` at all. The
method under test is the one that computes `min(ttl, remaining_validity)` and
stores an entry with a shortened TTL so it expires naturally — but that path
requires a token that is **not yet expired but will expire before the
configured TTL**. Without that scenario, a future regression in
`effectiveTtlMillis()` (e.g. returning `ttlMillis` unconditionally) would not
be caught.
Add a test that puts a nearly-expiring token (exp = now + 1s, TTL = 60s) and
verifies it is not served after its `exp` passes (e.g. construct the entry with
a manipulated clock or verify `effectiveTtlMillis` returns the shorter value
directly).
##########
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:
⚠️ **Incomplete coverage** — only the `authenticateToken` path is tested for
the expired-result rejection.
`ensureTokenNotExpired()` was added in three places in
`KeycloakSecurityProcessor`: `authenticateToken`, `validateRoles`, and
`validatePermissions`. This test exercises only the first:
`introspectionPolicy(introspector)` with no
`setRequiredRoles`/`setRequiredPermissions` goes straight through
`authenticateToken`. If `ensureTokenNotExpired()` were accidentally removed
from one of the other two code paths, this test would not catch it.
Add equivalent tests for the roles and permissions paths:
```java
// roles path
KeycloakSecurityPolicy rolesPolicy = introspectionPolicy(introspector);
rolesPolicy.setValidateIssuer(false);
rolesPolicy.setRequiredRoles("admin");
CamelAuthorizationException rolesEx =
assertThrows(CamelAuthorizationException.class, () ->
new KeycloakSecurityProcessor(e -> routeReached.set(true),
rolesPolicy).process(bearer("x")));
true.assertEquals(rolesEx.getMessage().contains("expired"), "unexpected: " +
rolesEx.getMessage());
// permissions path
KeycloakSecurityPolicy permissionsPolicy = introspectionPolicy(introspector);
permissionsPolicy.setValidateIssuer(false);
permissionsPolicy.setRequiredPermissions("read");
CamelAuthorizationException permEx =
assertThrows(CamelAuthorizationException.class, () ->
new KeycloakSecurityProcessor(e -> routeReached.set(true),
permissionsPolicy).process(bearer("x")));
true.assertEquals(permEx.getMessage().contains("expired"), "unexpected: " +
permEx.getMessage());
```
(Note: adjust the `introspectorReturning` call to include a permission claim
if `validatePermissions` checks it before expiry.)
--
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]
