rootvector2 commented on code in PR #3625:
URL: https://github.com/apache/cxf/pull/3625#discussion_r4371731739
##########
rt/rs/security/sso/saml/src/main/java/org/apache/cxf/rs/security/saml/sso/TokenReplayCache.java:
##########
@@ -31,5 +31,26 @@ public interface TokenReplayCache<T> extends Closeable {
void putId(T id, Instant expiry);
+ /**
+ * Add the given identifier to the cache if, and only if, it is not
already contained in it, as a
+ * single atomic operation, so that concurrent requests carrying the same
identifier cannot all
+ * pass a replay check before any of them has cached it. This default
implementation only
+ * serializes calls within a single JVM; an implementation backed by a
distributed cache should
+ * override it with the atomic put-if-absent operation of the underlying
cache.
+ * @param id The identifier to be added
+ * @param expiry A custom expiry time for the identifier. Can be null in
which case, the default
+ * expiry is used.
+ * @return true if the identifier was added, false if it was already
contained in the cache
+ */
+ default boolean putIdIfAbsent(T id, Instant expiry) {
+ synchronized (this) {
Review Comment:
Good catch. The default now returns `false` for `id == null ||
"".equals(id)`, same as `EHCacheTokenReplayCache` and `JCacheTokenReplayCache`.
##########
rt/rs/security/sso/saml/src/test/java/org/apache/cxf/rs/security/saml/sso/TokenReplayCacheTest.java:
##########
@@ -103,6 +111,73 @@ public void testTokenReplayCacheHugeExpiry() throws
Exception {
replayCache.close();
}
+ @Test
+ public void testTokenReplayCachePutIdIfAbsent() throws Exception {
+ TokenReplayCache<String> replayCache = createCache();
+ try {
+ String id = UUID.randomUUID().toString();
+ assertTrue(replayCache.putIdIfAbsent(id, null));
+ assertTrue(replayCache.contains(id));
+ assertFalse(replayCache.putIdIfAbsent(id, null));
+ assertFalse(replayCache.putIdIfAbsent(id,
Instant.now().plusSeconds(100L)));
+
+ // An identifier added with putId() is also seen
+ id = UUID.randomUUID().toString();
+ replayCache.putId(id);
+ assertFalse(replayCache.putIdIfAbsent(id, null));
+
+ // An empty identifier cannot be cached, so is never reported as
added
+ assertFalse(replayCache.putIdIfAbsent(null, null));
+ assertFalse(replayCache.putIdIfAbsent("", null));
+ assertFalse(replayCache.contains(""));
+ } finally {
+ replayCache.close();
+ }
+ }
+
+ // Concurrent requests carrying the same identifier must not all be
accepted: exactly one
+ // putIdIfAbsent call per identifier may succeed.
+ @Test
+ public void testTokenReplayCacheConcurrentPutIdIfAbsent() throws Exception
{
Review Comment:
Fixed. `barrier.await` and `future.get` now use 10s/30s timeouts like the
validator test, and the executor is shut down with `shutdownNow()`.
--
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]
