search

spring-projects / spring-security

Spring Security
http://spring.io/projects/spring-security
Apache License 2.0
8.5k stars 5.78k forks source link

Do Not Invalidate Current Session When Its Registered #15066

Closed joaquinjsb closed 1 month ago

joaquinjsb commented 1 month ago

this change prevents from deleting the current session, which ends up creating the need to log in two times.

marcusdacoregio commented 1 month ago

Hi @joaquinjsb, can you elaborate more on what is the problem and how this fixes it? As far as I can tell the invalidation is performed before the current session is registered in the ReactiveSessionRegistry, thus not returning the current session.

joaquinjsb commented 1 month ago

Hi @joaquinjsb, can you elaborate more on what is the problem and how this fixes it? As far as I can tell the invalidation is performed before the current session is registered in the ReactiveSessionRegistry, thus not returning the current session.

what happens in my case is: -> login browser 1 -> login browser 2 -> the ConcurrentSessionControlServerAuthenticationSuccessHandler:handleConcurrency finds currentSession, and the
registeredSessions = sessionTuple.getT2(); contains both the current session and the session from browser 1 and passes it down on line 87.

what my fix does is, removes from the registeredSessions, the current session, so that you don't end up deleting both browser 1 and browser 2 sessions, which prevents the login flow from being "standard" on the user side.

marcusdacoregio commented 1 month ago

There is a test that verifies that behavior here, can you create a new test that reproduces the problem?

joaquinjsb commented 1 month ago

I tried running it with a limit of 1, which behaves correctly as you describe, would the problem be probably on SpringSessionBackedReactiveSessionRegistry? do you have any suggestions on how to proceed?

marcusdacoregio commented 1 month ago

The best approach would be to create a minimal, reproducible sample for the issues, with tests preferably. Then we can identify exactly where the problem is

joaquinjsb commented 1 month ago

The best approach would be to create a minimal, reproducible sample for the issues, with tests preferably. Then we can identify exactly where the problem is

here you have both minimal reproducible, and a test which is failing. https://github.com/joaquinjsb/sessionRedisReactive

joaquinjsb commented 1 month ago

I dived in further, the answer to our question is here: https://github.com/spring-projects/spring-security/blob/a17a599bd54838dd2f166d97555de3240335d07f/config/src/main/java/org/springframework/security/config/web/server/ServerHttpSecurity.java#L2120C5-L2127C6

the delegate.getSession() creates a new session, then it gets saved in the repository, which means that the getSessions() will have both the currentSession and the older ones.

so why it doesn't reflect in the tests is due to the fact that both InMemoryReactiveSessionRegistry & WebSessionStore has their own sessionsMaps.

I can apply my change in the handler, so filters don't need to do that work, or as I did, or do you recommend another path?

marcusdacoregio commented 1 month ago

Thanks @joaquinjsb, this has been closed via https://github.com/spring-projects/spring-security/commit/927840fe88052a40c72fe80a16372f594880e02b