From 45b99104006aa05753a6bc75d2807d37fb273374 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 25 Nov 2025 20:31:22 +0000 Subject: [PATCH 1/3] Initial plan From 51b53f5424143b0ff5da87c07f35d552605e0e8c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 25 Nov 2025 20:44:06 +0000 Subject: [PATCH 2/3] Fix OIDC session expiration bug: keep valid sessions, remove expired ones Co-authored-by: nitisht <5156139+nitisht@users.noreply.github.com> --- src/rbac/map.rs | 85 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 84 insertions(+), 1 deletion(-) diff --git a/src/rbac/map.rs b/src/rbac/map.rs index 5377d10d7..c0532d05f 100644 --- a/src/rbac/map.rs +++ b/src/rbac/map.rs @@ -208,7 +208,17 @@ impl Sessions { let Some(sessions) = self.user_sessions.get_mut(user) else { return; }; - sessions.retain(|(_, expiry)| expiry < &now); + let initial_count = sessions.len(); + sessions.retain(|(_, expiry)| expiry > &now); + let removed_count = initial_count - sessions.len(); + if removed_count > 0 { + tracing::debug!( + user = user, + removed = removed_count, + remaining = sessions.len(), + "Removed expired sessions for user" + ); + } } // get permission related to this session @@ -353,3 +363,76 @@ impl From> for UserGroups { map } } + +#[cfg(test)] +mod tests { + use super::*; + use chrono::Days; + + #[test] + fn test_remove_expired_session_keeps_valid_sessions() { + let mut sessions = Sessions::default(); + let user = "test_user".to_string(); + let key = SessionKey::SessionId(ulid::Ulid::new()); + + // Add a session that expires 7 days from now (valid session) + let future_expiry = Utc::now() + Days::new(7); + sessions.track_new(user.clone(), key.clone(), future_expiry, vec![]); + + // Verify session exists + assert!(sessions.get(&key).is_some()); + assert_eq!(sessions.user_sessions.get(&user).unwrap().len(), 1); + + // Trigger expired session cleanup by adding another session + let key2 = SessionKey::SessionId(ulid::Ulid::new()); + sessions.track_new(user.clone(), key2.clone(), future_expiry, vec![]); + + // Both valid sessions should still exist + assert!(sessions.get(&key).is_some()); + assert!(sessions.get(&key2).is_some()); + assert_eq!(sessions.user_sessions.get(&user).unwrap().len(), 2); + } + + #[test] + fn test_remove_expired_session_removes_expired_sessions() { + let mut sessions = Sessions::default(); + let user = "test_user".to_string(); + let key = SessionKey::SessionId(ulid::Ulid::new()); + + // Add a session that expired 1 day ago + let past_expiry = Utc::now() - Days::new(1); + sessions.user_sessions.insert(user.clone(), vec![(key.clone(), past_expiry)]); + sessions.active_sessions.insert(key.clone(), (user.clone(), vec![])); + + // Verify expired session exists before cleanup + assert!(sessions.get(&key).is_some()); + + // Trigger cleanup by adding a new session + let key2 = SessionKey::SessionId(ulid::Ulid::new()); + let future_expiry = Utc::now() + Days::new(7); + sessions.track_new(user.clone(), key2.clone(), future_expiry, vec![]); + + // Expired session should be removed from user_sessions but + // note: the cleanup only removes from user_sessions, not active_sessions + // The original expired session key is still in user_sessions list until cleaned + let user_sess = sessions.user_sessions.get(&user).unwrap(); + // Only the new valid session should remain in user_sessions + assert_eq!(user_sess.len(), 1); + // The new session should be the one with future expiry + assert!(user_sess.iter().any(|(k, _)| k == &key2)); + } + + #[test] + fn test_session_expiry_logic_correctness() { + // This test verifies the fix for the bug where expired sessions + // were being kept and valid sessions were being removed + let now = Utc::now(); + let past = now - Days::new(1); + let future = now + Days::new(1); + + // The retain predicate should keep sessions where expiry > now + // i.e., sessions that have NOT yet expired + assert!(future > now); // Future sessions should be kept + assert!(!(past > now)); // Past sessions should be removed + } +} From b46da3a5d25ed21f9f9dd3896c26da104f8d538c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 25 Nov 2025 20:48:28 +0000 Subject: [PATCH 3/3] Improve test documentation clarity Co-authored-by: nitisht <5156139+nitisht@users.noreply.github.com> --- src/rbac/map.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/rbac/map.rs b/src/rbac/map.rs index c0532d05f..654f0f344 100644 --- a/src/rbac/map.rs +++ b/src/rbac/map.rs @@ -412,9 +412,9 @@ mod tests { let future_expiry = Utc::now() + Days::new(7); sessions.track_new(user.clone(), key2.clone(), future_expiry, vec![]); - // Expired session should be removed from user_sessions but - // note: the cleanup only removes from user_sessions, not active_sessions - // The original expired session key is still in user_sessions list until cleaned + // After cleanup, the expired session should be removed from user_sessions. + // Note: remove_expired_session only cleans up user_sessions, not active_sessions. + // The active_sessions cleanup happens separately when the session is accessed. let user_sess = sessions.user_sessions.get(&user).unwrap(); // Only the new valid session should remain in user_sessions assert_eq!(user_sess.len(), 1);