coheigea commented on code in PR #3625:
URL: https://github.com/apache/cxf/pull/3625#discussion_r4370594894
##########
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:
The cache concurrency test can hang. In
testTokenReplayCacheConcurrentPutIdIfAbsent, barrier.await() and future.get()
have no timeouts. If one thread throws before reaching the barrier, the build
hangs instead of failing. The validator test does use timeouts.
##########
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:
The default method doesn’t reject null or empty IDs the way both built-in
caches do. A custom cache whose putId("") silently does nothing, as the
built-in ones do, would return true every time, so an assertion with an empty
ID could be replayed without limit. Adding the same id == null || "".equals(id)
guard at the top of the default would make them consistent. The parameter is a
generic T, so the empty-string check would need to be "".equals(id).
--
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]
