Skip to content

REF-8: Fix seconds/milliseconds mixup in in-memory session cleanup (issue #76) #87

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
thomasnymand merged 1 commit into from
Jul 6, 2026
Merged

REF-8: Fix seconds/milliseconds mixup in in-memory session cleanup (issue #76) #87

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion oiosaml/src/main/java/dk/gov/oio/saml/session/SessionCleanerTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion oiosaml/src/main/java/dk/gov/oio/saml/session/SessionHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
2 changes: 1 addition & 1 deletion oiosaml/src/main/java/dk/gov/oio/saml/session/database/DatabaseSessionHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
15 changes: 10 additions & 5 deletions oiosaml/src/main/java/dk/gov/oio/saml/session/inmemory/InMemorySessionHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {

@mthiim mthiim Jul 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only concern is if someone previously called this method and is relying on the old behavior (maybe they found the error and compensated by passing in milliseconds, meaning now they will overshoot by 1000x). Probably not a big issue - either the current method could be kept as is and a new cleanup addded, but maybe better to just document it clearly when releasing?

@thomasnymand thomasnymand Jul 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are perfectly right :) I suggest handling by adding clear and obvious documentation in release note.

// 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 <E, T> void cleanup(Map<E, TimeOutWrapper<T>> map, long cleanupDelay, String msg) {
Expand Down
17 changes: 17 additions & 0 deletions oiosaml/src/test/java/dk/gov/oio/saml/session/inmemory/InMemorySessionHandlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Loading