Skip to content
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.

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

SessionRegistryImpl is now aware of SessionIdChangedEvent #5439

Closed
wants to merge 1 commit into from

Conversation

aj-jaswanth
Copy link
Contributor

Fixes #5438

@pivotal-issuemaster
Copy link

@aj-jaswanth Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster
Copy link

@aj-jaswanth Thank you for signing the Contributor License Agreement!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 7, 2019
Copy link
Contributor

@eleftherias eleftherias left a comment

Choose a reason for hiding this comment

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

@aj-jaswanth I realize this is an old PR, but I have added some comments inline if you are still interested in having it merged.
Furthermore, can you explain how to reproduce the case where SessionRegistryImpl will not be aware of the session ID change and continues to have the older sessionID.

@@ -40,7 +41,7 @@
* @author Luke Taylor
*/
public class SessionRegistryImpl implements SessionRegistry,
ApplicationListener<SessionDestroyedEvent> {
ApplicationListener<ApplicationEvent> {
Copy link
Contributor

Choose a reason for hiding this comment

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

ApplicationEvent may be too generic to be used in this class. Perhaps we can create a new interface (e.g SessionEvent) that both SessionDestroyedEvent and SessionIdChangedEvent can implement.

@@ -0,0 +1,48 @@
/*
* Copyright 2004, 2005, 2006 Acegi Technology Pty Limited
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the new license header for new classes.

*/
public class HttpSessionIdChangedEvent extends SessionIdChangedEvent {
private final String oldSessionId;
private final String newSessionid;
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a small typo here, newSessionid should be newSessionId.

@eleftherias eleftherias added in: config An issue in spring-security-config status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels Nov 4, 2019
@eleftherias eleftherias self-assigned this Nov 4, 2019
@eleftherias
Copy link
Contributor

Merged via 5fc6414.
Polished via b2ea0ba.

@eleftherias eleftherias closed this Mar 6, 2020
@eleftherias eleftherias added type: enhancement A general enhancement status: duplicate A duplicate of another issue and removed status: waiting-for-feedback We need additional information before we can continue labels Mar 6, 2020
@eleftherias eleftherias added this to the 5.4.0 milestone Mar 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: config An issue in spring-security-config status: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SessionRegistryImpl is not aware of SessionIdChange events.
4 participants