diff --git a/oiosaml/src/main/java/dk/gov/oio/saml/session/SessionCleanerTask.java b/oiosaml/src/main/java/dk/gov/oio/saml/session/SessionCleanerTask.java index 29b498f..ddad266 100644 --- a/oiosaml/src/main/java/dk/gov/oio/saml/session/SessionCleanerTask.java +++ b/oiosaml/src/main/java/dk/gov/oio/saml/session/SessionCleanerTask.java @@ -20,7 +20,7 @@ public SessionCleanerTask(long maxInactiveIntervalSeconds) { @Override public void run() { - log.debug("Cleaning session data, time: {}, timeout: {}", System.currentTimeMillis(), maxInactiveIntervalSeconds * 1000); + log.debug("Cleaning session data, time: {}, timeout (seconds): {}", System.currentTimeMillis(), maxInactiveIntervalSeconds); try { SessionHandler sessionHandler = OIOSAML3Service.getSessionHandlerFactory().getHandler(); sessionHandler.cleanup(maxInactiveIntervalSeconds); diff --git a/oiosaml/src/main/java/dk/gov/oio/saml/session/SessionHandler.java b/oiosaml/src/main/java/dk/gov/oio/saml/session/SessionHandler.java index 4c0b627..3c80cb4 100644 --- a/oiosaml/src/main/java/dk/gov/oio/saml/session/SessionHandler.java +++ b/oiosaml/src/main/java/dk/gov/oio/saml/session/SessionHandler.java @@ -134,7 +134,7 @@ default String getSessionId(HttpSession session) { /** * Clean stored ids and sessions. * - * @param maxInactiveIntervalSeconds Milliseconds to store session, before it should be invalidated. + * @param maxInactiveIntervalSeconds Seconds to store session, before it should be invalidated. */ void cleanup(long maxInactiveIntervalSeconds); } diff --git a/oiosaml/src/main/java/dk/gov/oio/saml/session/database/DatabaseSessionHandler.java b/oiosaml/src/main/java/dk/gov/oio/saml/session/database/DatabaseSessionHandler.java index 6c0390b..1a0b98c 100644 --- a/oiosaml/src/main/java/dk/gov/oio/saml/session/database/DatabaseSessionHandler.java +++ b/oiosaml/src/main/java/dk/gov/oio/saml/session/database/DatabaseSessionHandler.java @@ -351,7 +351,7 @@ public void logout(HttpSession session, AssertionWrapper assertion) { /** * Clean stored ids and sessions. * - * @param maxInactiveIntervalSeconds Milliseconds to store session, before it should be invalidated. + * @param maxInactiveIntervalSeconds Seconds to store session, before it should be invalidated. */ @Override public void cleanup(final long maxInactiveIntervalSeconds) { diff --git a/oiosaml/src/main/java/dk/gov/oio/saml/session/inmemory/InMemorySessionHandler.java b/oiosaml/src/main/java/dk/gov/oio/saml/session/inmemory/InMemorySessionHandler.java index 53b5773..6a1af6b 100644 --- a/oiosaml/src/main/java/dk/gov/oio/saml/session/inmemory/InMemorySessionHandler.java +++ b/oiosaml/src/main/java/dk/gov/oio/saml/session/inmemory/InMemorySessionHandler.java @@ -252,18 +252,23 @@ public void logout(HttpSession session, AssertionWrapper assertion) { /** * Clean stored ids and sessions. * - * @param maxInactiveIntervalSeconds Milliseconds to store session, before it should be invalidated. + * @param maxInactiveIntervalSeconds Seconds to store session, before it should be invalidated. */ @Override public void cleanup(long maxInactiveIntervalSeconds) { + // TimeOutWrapper.isExpired() compares against System.currentTimeMillis(), so the timeout + // must be in milliseconds. The interval is supplied in seconds (see SessionHandler#cleanup), + // so convert before comparing - otherwise sessions expire 1000x too early (issue #76). + long maxInactiveIntervalMillis = maxInactiveIntervalSeconds * 1000L; + // Trim usedAssertionIds to size with sessionHandlerNumTrackedSessionIds while (!usedAssertionIds.isEmpty() && usedAssertionIds.size() > sessionHandlerNumTrackedSessionIds) { usedAssertionIds.remove(usedAssertionIds.pollFirst()); } - cleanup(sessionIndexMap, maxInactiveIntervalSeconds, "SessionIndexMap"); - cleanup(assertions, maxInactiveIntervalSeconds, "Assertions"); - cleanup(authnRequests, maxInactiveIntervalSeconds, "AuthnRequests"); - cleanup(logoutRequests, maxInactiveIntervalSeconds, "LogoutRequests"); + cleanup(sessionIndexMap, maxInactiveIntervalMillis, "SessionIndexMap"); + cleanup(assertions, maxInactiveIntervalMillis, "Assertions"); + cleanup(authnRequests, maxInactiveIntervalMillis, "AuthnRequests"); + cleanup(logoutRequests, maxInactiveIntervalMillis, "LogoutRequests"); } private void cleanup(Map> map, long cleanupDelay, String msg) { diff --git a/oiosaml/src/test/java/dk/gov/oio/saml/session/inmemory/InMemorySessionHandlerTest.java b/oiosaml/src/test/java/dk/gov/oio/saml/session/inmemory/InMemorySessionHandlerTest.java index 3f50ec7..9547e3a 100644 --- a/oiosaml/src/test/java/dk/gov/oio/saml/session/inmemory/InMemorySessionHandlerTest.java +++ b/oiosaml/src/test/java/dk/gov/oio/saml/session/inmemory/InMemorySessionHandlerTest.java @@ -152,6 +152,23 @@ void testStoreAssertionTimeout() throws Exception { Assertions.assertNull(assertionWrapperOutput); } + @DisplayName("Test that stored Assertion survives until its timeout (seconds, not millis) - issue #76") + @Test + void testStoreAssertionNotRemovedBeforeTimeout() throws Exception { + AssertionWrapper assertionWrapperInput = new AssertionWrapper(createAssertion()); + + sessionHandler.storeAssertion(session, assertionWrapperInput); + + // Wait far longer than the timeout-interpreted-as-milliseconds (the #76 bug: 5s -> 5ms), + // but far less than the real 5 second timeout. The assertion must still be present. + Thread.sleep(100); + sessionHandler.cleanup(5); // 5 seconds + + AssertionWrapper assertionWrapperOutput = sessionHandler.getAssertion(session); + Assertions.assertNotNull(assertionWrapperOutput, + "Assertion must survive cleanup until its timeout elapses (timeout is in seconds, not milliseconds)"); + } + @DisplayName("Test that stored Assertion is removed after timeout and can not be retrieved using session index") @Test void testStoreAssertionTimeoutSessionIndex() throws Exception {