Merge pull request #4942 from mstruk/KEYCLOAK-5807

KEYCLOAK-5807 Intermittent failures in UserStorageTest

server-spi/src/main/java/org/keycloak/storage/CacheableStorageProviderModel.java

@@ -197,11 +197,8 @@ public class CacheableStorageProviderModel extends PrioritizedComponentModel {
                         invalidate = true;
                     }
                 } else if (policy == CacheableStorageProviderModel.CachePolicy.EVICT_DAILY) {
-                    long dailyTimeout = dailyTimeout(getEvictionHour(), getEvictionMinute());
+                    long dailyBoundary = dailyEvictionBoundary(getEvictionHour(), getEvictionMinute());
-                    dailyTimeout = dailyTimeout - (24 * 60 * 60 * 1000);
+                    if (cached.getCacheTimestamp() <= dailyBoundary) {
-                    //String timeout = DateFormat.getDateTimeInstance(DateFormat.FULL, DateFormat.FULL).format(new Date(dailyTimeout));
-                    //String stamp = DateFormat.getDateTimeInstance(DateFormat.FULL, DateFormat.FULL).format(new Date(cached.getCacheTimestamp()));
-                    if (cached.getCacheTimestamp() <= dailyTimeout) {
                         invalidate = true;
                     }
                 } else if (policy == CacheableStorageProviderModel.CachePolicy.EVICT_WEEKLY) {
@@ -231,7 +228,20 @@ public class CacheableStorageProviderModel extends PrioritizedComponentModel {
             int add = (24 * 60 * 60 * 1000);
             cal.add(Calendar.MILLISECOND, add);
         } else {
-            cal.add(Calendar.MILLISECOND, (int)(cal2.getTimeInMillis() - cal.getTimeInMillis()));
+            cal = cal2;
+        }
+        return cal.getTimeInMillis();
+    }
+
+    public static long dailyEvictionBoundary(int hour, int minute) {
+        Calendar cal = Calendar.getInstance();
+        cal.setTimeInMillis(Time.currentTimeMillis());
+        cal.set(Calendar.HOUR_OF_DAY, hour);
+        cal.set(Calendar.MINUTE, minute);
+        if (cal.getTimeInMillis() > Time.currentTimeMillis()) {
+            // if daily evict for today hasn't happened yet set boundary
+            // to yesterday's time of eviction
+            cal.add(Calendar.DAY_OF_YEAR, -1);
         }
         return cal.getTimeInMillis();
     }

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/AbstractKeycloakTest.java

@@ -29,6 +29,7 @@ import java.net.URI;
 import java.net.URISyntaxException;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Calendar;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
@@ -386,6 +387,35 @@ public abstract class AbstractKeycloakTest {
         userResource.update(userRepresentation);
     }
 
+    /**
+     * Sets time of day by calculating time offset and using setTimeOffset() to set it.
+     *
+     * @param hour hour of day
+     * @param minute minute
+     * @param second second
+     */
+    public void setTimeOfDay(int hour, int minute, int second) {
+        setTimeOfDay(hour, minute, second, 0);
+    }
+
+    /**
+     * Sets time of day by calculating time offset and using setTimeOffset() to set it.
+     *
+     * @param hour hour of day
+     * @param minute minute
+     * @param second second
+     * @param addSeconds additional seconds to add to offset time
+     */
+    public void setTimeOfDay(int hour, int minute, int second, int addSeconds) {
+        Calendar now = Calendar.getInstance();
+        now.set(Calendar.HOUR_OF_DAY, hour);
+        now.set(Calendar.MINUTE, minute);
+        now.set(Calendar.SECOND, second);
+        int offset = (int) ((now.getTime().getTime() - System.currentTimeMillis()) / 1000);
+
+        setTimeOffset(offset + addSeconds);
+    }
+
     /**
      * Sets time offset in seconds that will be added to Time.currentTime() and Time.currentTimeMillis() both for client and server.
      *

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/UserStorageTest.java

@@ -371,42 +371,160 @@ public class UserStorageTest extends AbstractAuthTest {
                 .addPackages(true, "org.keycloak.testsuite");
     }
 
-    @Test
+    private void setDailyEvictionTime(int hour, int minutes) {
-    public void testDailyEviction() {
+        if (hour < 0 || hour > 23) {
-        ApiUtil.findUserByUsername(testRealmResource(), "thor");
+            throw new IllegalArgumentException("hour == " + hour);
-
+        }
-        // set eviction to 1 hour from now
+        if (minutes < 0 || minutes > 59) {
-        Calendar eviction = Calendar.getInstance();
+            throw new IllegalArgumentException("minutes == " + minutes);
-        eviction.add(Calendar.HOUR, 1);
+        }
         ComponentRepresentation propProviderRW = testRealmResource().components().component(propProviderRWId).toRepresentation();
         propProviderRW.getConfig().putSingle(CACHE_POLICY, CachePolicy.EVICT_DAILY.name());
-        propProviderRW.getConfig().putSingle(EVICTION_HOUR, Integer.toString(eviction.get(HOUR_OF_DAY)));
+        propProviderRW.getConfig().putSingle(EVICTION_HOUR, String.valueOf(hour));
-        propProviderRW.getConfig().putSingle(EVICTION_MINUTE, Integer.toString(eviction.get(MINUTE)));
+        propProviderRW.getConfig().putSingle(EVICTION_MINUTE, String.valueOf(minutes));
         testRealmResource().components().component(propProviderRWId).update(propProviderRW);
+    }
 
-        // now
+
+    /**
+     * Test daily eviction behaviour
+     */
+    @Test
+    public void testDailyEviction() {
+
+        // We need to test both cases: eviction the same day, and eviction the next day
+        // Simplest is to take full control of the clock
+
+        // set clock to 23:30 of current day
+        setTimeOfDay(23, 30, 0);
+
+        // test same day eviction behaviour
+        // set eviction at 23:45
+        setDailyEvictionTime(23, 45);
+
+        // there are users in cache already from before-test import
+        // and they didn't use any time offset clock so they may have timestamps in the 'future'
+
+        // let's clear cache
         testingClient.server().run(session -> {
-            RealmModel realm = session.realms().getRealmByName("test");
+            session.userCache().clear();
-            UserModel user = session.users().getUserByUsername("thor", realm);
         });
 
-        // run twice to make sure its in cache.
-        testingClient.server().run(session -> {
-            RealmModel realm = session.realms().getRealmByName("test");
-            UserModel user = session.users().getUserByUsername("thor", realm);
-            System.out.println("User class: " + user.getClass());
-            Assert.assertTrue(user instanceof CachedUserModel); // should still be cached
-        });
-
-        setTimeOffset(2 * 60 * 60); // 2 hours in future
 
         testingClient.server().run(session -> {
             RealmModel realm = session.realms().getRealmByName("test");
             UserModel user = session.users().getUserByUsername("thor", realm);
-            System.out.println("User class: " + user.getClass());
+            Assert.assertTrue(user instanceof CachedUserModel); // should be newly cached
-            Assert.assertFalse(user instanceof CachedUserModel); // should be evicted
         });
 
+
+        setTimeOfDay(23, 40, 0);
+
+        // lookup user again - make sure it's returned from cache
+        testingClient.server().run(session -> {
+            RealmModel realm = session.realms().getRealmByName("test");
+            UserModel user = session.users().getUserByUsername("thor", realm);
+            Assert.assertTrue(user instanceof CachedUserModel); // should be returned from cache
+        });
+
+
+        setTimeOfDay(23, 50, 0);
+
+        testingClient.server().run(session -> {
+            RealmModel realm = session.realms().getRealmByName("test");
+            UserModel user = session.users().getUserByUsername("thor", realm);
+            Assert.assertFalse(user instanceof CachedUserModel); // should have been invalidated
+        });
+
+
+        testingClient.server().run(session -> {
+            RealmModel realm = session.realms().getRealmByName("test");
+            UserModel user = session.users().getUserByUsername("thor", realm);
+            Assert.assertTrue(user instanceof CachedUserModel); // should have been newly cached
+        });
+
+
+        testingClient.server().run(session -> {
+            RealmModel realm = session.realms().getRealmByName("test");
+            UserModel user = session.users().getUserByUsername("thor", realm);
+            Assert.assertTrue(user instanceof CachedUserModel); // should be returned from cache
+        });
+
+
+        setTimeOfDay(23, 55, 0);
+
+        testingClient.server().run(session -> {
+            RealmModel realm = session.realms().getRealmByName("test");
+            UserModel user = session.users().getUserByUsername("thor", realm);
+            Assert.assertTrue(user instanceof CachedUserModel); // should be returned from cache
+        });
+
+
+        // at 00:30
+        // it's next day now. the daily eviction time is now in the future
+        setTimeOfDay(0, 30, 0, 24 * 60 * 60);
+
+        testingClient.server().run(session -> {
+            RealmModel realm = session.realms().getRealmByName("test");
+            UserModel user = session.users().getUserByUsername("thor", realm);
+            Assert.assertTrue(user instanceof CachedUserModel); // should be returned from cache - it's still good for almost the whole day
+        });
+
+
+        // at 23:30 next day
+        setTimeOfDay(23, 30, 0, 24 * 60 * 60);
+
+        testingClient.server().run(session -> {
+            RealmModel realm = session.realms().getRealmByName("test");
+            UserModel user = session.users().getUserByUsername("thor", realm);
+            Assert.assertTrue(user instanceof CachedUserModel); // should be returned from cache - it's still good until 23:45
+        });
+
+        // at 23:50
+        setTimeOfDay(23, 50, 0, 24 * 60 * 60);
+
+        testingClient.server().run(session -> {
+            RealmModel realm = session.realms().getRealmByName("test");
+            UserModel user = session.users().getUserByUsername("thor", realm);
+            Assert.assertFalse(user instanceof CachedUserModel); // should be invalidated
+        });
+
+        setTimeOfDay(23, 55, 0, 24 * 60 * 60);
+
+        testingClient.server().run(session -> {
+            RealmModel realm = session.realms().getRealmByName("test");
+            UserModel user = session.users().getUserByUsername("thor", realm);
+            Assert.assertTrue(user instanceof CachedUserModel); // should be newly cached
+        });
+
+
+        setTimeOfDay(23, 40, 0, 2 * 24 * 60 * 60);
+
+        testingClient.server().run(session -> {
+            RealmModel realm = session.realms().getRealmByName("test");
+            UserModel user = session.users().getUserByUsername("thor", realm);
+            Assert.assertTrue(user instanceof CachedUserModel); // should be returned from cache
+        });
+
+        setTimeOfDay(23, 50, 0, 2 * 24 * 60 * 60);
+
+        testingClient.server().run(session -> {
+            RealmModel realm = session.realms().getRealmByName("test");
+            UserModel user = session.users().getUserByUsername("thor", realm);
+            Assert.assertFalse(user instanceof CachedUserModel); // should be invalidated
+        });
+
+        testingClient.server().run(session -> {
+            RealmModel realm = session.realms().getRealmByName("test");
+            UserModel user = session.users().getUserByUsername("thor", realm);
+            Assert.assertTrue(user instanceof CachedUserModel); // should be newly cached
+        });
+
+        testingClient.server().run(session -> {
+            RealmModel realm = session.realms().getRealmByName("test");
+            UserModel user = session.users().getUserByUsername("thor", realm);
+            Assert.assertTrue(user instanceof CachedUserModel); // should be returned from cache
+        });
     }
 
     @Test