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

Spring security session registry #473

Merged
merged 3 commits into from Sep 2, 2016

Conversation

Projects
None yet
5 participants
@jkuipers
Contributor

jkuipers commented Apr 9, 2016

This provides support to use a Spring Session-backed SessionRepository for Spring Security, which allows concurrent session control in a clustered environment. It relates to #65.

I've created this PR per Rob's request: I've tried to clean up my sample code that I wrote earlier a bit and have added some documentation on configuration and limitiations of the provided solution. I hope this helps!

I have already signed the Contributor License Agreement for Spring in the past.

@rwinch

This comment has been minimized.

Show comment
Hide comment
@rwinch

rwinch Apr 11, 2016

Member

@jkuipers Thanks for the PR!

I'm wondering if we would be able to support the concept of marking the session as expired by setting a session attribute rather than deleting the session. Then when Spring Session obtains the SessionInformation it would read this attribute to determine if the session is expired.

Honestly, I do like the fact that the current implementation deletes the session, but this might be a nice feature for users who need the ability to display a more useful error message.

Thoughts?

Member

rwinch commented Apr 11, 2016

@jkuipers Thanks for the PR!

I'm wondering if we would be able to support the concept of marking the session as expired by setting a session attribute rather than deleting the session. Then when Spring Session obtains the SessionInformation it would read this attribute to determine if the session is expired.

Honestly, I do like the fact that the current implementation deletes the session, but this might be a nice feature for users who need the ability to display a more useful error message.

Thoughts?

@rwinch rwinch added this to the 1.3.0 M1 milestone Apr 11, 2016

@rwinch

This comment has been minimized.

Show comment
Hide comment
@rwinch

rwinch Apr 11, 2016

Member

Note I'm assigning this to 1.3.0 M1 because we plan to go GA with Spring Session 1.2 next and we need have at least one RC of a new feature before going GA.

Member

rwinch commented Apr 11, 2016

Note I'm assigning this to 1.3.0 M1 because we plan to go GA with Spring Session 1.2 next and we need have at least one RC of a new feature before going GA.

@jkuipers

This comment has been minimized.

Show comment
Hide comment
@jkuipers

jkuipers Apr 11, 2016

Contributor

A custom session attribute to support "Spring Security expiration" makes a lot of sense. I'll update the implementation and docs to ensure that this in place. This will also cause the filtering on expired session to actually do something useful, since it won't conflate Spring Security's notion of an expired session with that of Spring Session: they're different things,
I could also support repositories that return plain Sessions rather than ExpiringSessions this way: the SessionRegistry already gets a callback to update the last accessed timestamp of a session, so for plain Sessions (that don't provide this information themselves like ExpiringSessions do) we could store the info in another custom session attribute. The code could determine and cache whether plain or expiring sessions are being returned by the repository, so the additional overhead of updating the Session with a last accessed attribute would only be incurred for plain Sessions. Do you think that this is worth the trouble, or will all relevant implementations implement ExpiringSession anyway, in your opinion?

Contributor

jkuipers commented Apr 11, 2016

A custom session attribute to support "Spring Security expiration" makes a lot of sense. I'll update the implementation and docs to ensure that this in place. This will also cause the filtering on expired session to actually do something useful, since it won't conflate Spring Security's notion of an expired session with that of Spring Session: they're different things,
I could also support repositories that return plain Sessions rather than ExpiringSessions this way: the SessionRegistry already gets a callback to update the last accessed timestamp of a session, so for plain Sessions (that don't provide this information themselves like ExpiringSessions do) we could store the info in another custom session attribute. The code could determine and cache whether plain or expiring sessions are being returned by the repository, so the additional overhead of updating the Session with a last accessed attribute would only be incurred for plain Sessions. Do you think that this is worth the trouble, or will all relevant implementations implement ExpiringSession anyway, in your opinion?

@rwinch

This comment has been minimized.

Show comment
Hide comment
@rwinch

rwinch Apr 11, 2016

Member

Thanks for the feedback. I think all relevant implementations will implement ExpiringSession. Honestly, the split between the two APIs was probably a mistake. However, something that cannot really be changed now.

Member

rwinch commented Apr 11, 2016

Thanks for the feedback. I think all relevant implementations will implement ExpiringSession. Honestly, the split between the two APIs was probably a mistake. However, something that cannot really be changed now.

@jkuipers

This comment has been minimized.

Show comment
Hide comment
@jkuipers

jkuipers Apr 11, 2016

Contributor

OK, I've pushed an updated implementation that tracks expiry in the Spring Security sense through a custom Session attribute. When Spring Security's ConcurrentSessionFilter finds an expired SessionInformation it will call all registered LogoutHandler beans, which typically would lead to the Session being deleted. Even if this wouldn't happen, then the Session will simply expire (in the Spring Session sense) eventually.

Contributor

jkuipers commented Apr 11, 2016

OK, I've pushed an updated implementation that tracks expiry in the Spring Security sense through a custom Session attribute. When Spring Security's ConcurrentSessionFilter finds an expired SessionInformation it will call all registered LogoutHandler beans, which typically would lead to the Session being deleted. Even if this wouldn't happen, then the Session will simply expire (in the Spring Session sense) eventually.

jkuipers added some commits Apr 9, 2016

Spring Security Concurrent Session Integration #65
 add SpringSessionBackedSessionRegistry
Spring Security Concurrent Session Integration #65
support marking SessionInformations as expired before deleting the Session
@edethoor

This comment has been minimized.

Show comment
Hide comment
@edethoor

edethoor Jul 18, 2016

@rwinch have an idea of the release date of 1.3.0 ?
I have to develop this feature but if it is supported out of the box in the next release, I prefer to wait.

Thanks.

edethoor commented Jul 18, 2016

@rwinch have an idea of the release date of 1.3.0 ?
I have to develop this feature but if it is supported out of the box in the next release, I prefer to wait.

Thanks.

@jkuipers

This comment has been minimized.

Show comment
Hide comment
@jkuipers

jkuipers Jul 18, 2016

Contributor

Note that you can take the code from this pull request and use it as-is with the current version of spring-session: that should provide you with a smooth upgrade path if the request is indeed merged for 1.3.

Contributor

jkuipers commented Jul 18, 2016

Note that you can take the code from this pull request and use it as-is with the current version of spring-session: that should provide you with a smooth upgrade path if the request is indeed merged for 1.3.

@rwinch

This comment has been minimized.

Show comment
Hide comment
@rwinch

rwinch Jul 19, 2016

Member

@edethoor Unfortunately we don't have an ETA on 1.3.0 at this time. As @jkubrynski suggests, you should be able to take the code from this PR as is and then remove it when 1.3.0 comes around

Member

rwinch commented Jul 19, 2016

@edethoor Unfortunately we don't have an ETA on 1.3.0 at this time. As @jkubrynski suggests, you should be able to take the code from this PR as is and then remove it when 1.3.0 comes around

@rwinch rwinch self-assigned this Sep 2, 2016

@rwinch rwinch merged commit 2724b33 into spring-projects:master Sep 2, 2016

2 checks passed

ci/pivotal-cla Thank you for signing the Contributor License Agreement!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rwinch

This comment has been minimized.

Show comment
Hide comment
@rwinch

rwinch Sep 2, 2016

Member

Thanks for the PR @jkuipers This is now merged into master

Member

rwinch commented Sep 2, 2016

Thanks for the PR @jkuipers This is now merged into master

@jkuipers jkuipers deleted the jkuipers:spring-security-session-registry branch Sep 14, 2016

@codecracker2014

This comment has been minimized.

Show comment
Hide comment
@codecracker2014

codecracker2014 Dec 31, 2016

@jkuipers i'm using infinispan for cache in cluster which provides Map based cache implementation. i need to use SpringSessionBackedSessionRegistry with MapSessionRepository but MapSessionRepository doesn't implement FindByIndexNameSessionRepository. How can i achieve this.

codecracker2014 commented Dec 31, 2016

@jkuipers i'm using infinispan for cache in cluster which provides Map based cache implementation. i need to use SpringSessionBackedSessionRegistry with MapSessionRepository but MapSessionRepository doesn't implement FindByIndexNameSessionRepository. How can i achieve this.

@jkuipers

This comment has been minimized.

Show comment
Hide comment
@jkuipers

jkuipers Jan 2, 2017

Contributor

@codecracker2014 It wouldn't be hard to add support for the FindByIndexNameSessionRepository to the MapSessionRepository: a naive implementation could simply loop through the stored ExpiringSessions and find the ones with the given principal name, a more optimized impl could simply have a map from principal names to a collection of corresponding sessions to make lookups efficient. Not sure what would be best for Infinispan, since I'm not familiar with that.

Contributor

jkuipers commented Jan 2, 2017

@codecracker2014 It wouldn't be hard to add support for the FindByIndexNameSessionRepository to the MapSessionRepository: a naive implementation could simply loop through the stored ExpiringSessions and find the ones with the given principal name, a more optimized impl could simply have a map from principal names to a collection of corresponding sessions to make lookups efficient. Not sure what would be best for Infinispan, since I'm not familiar with that.

@AmitMathur

This comment has been minimized.

Show comment
Hide comment
@AmitMathur

AmitMathur Jul 12, 2018

@jkuipers
this is great... thanks for the blog it was helpful.
I have a situation, where I want to show user a custom page if he reaches the max limit for concurrent session. On that page, when the user clicks on Continue, that's when we invalidate the oldest one and create a new one.
I wanted to see if that is something supported now?

Appreciate your help!!!

AmitMathur commented Jul 12, 2018

@jkuipers
this is great... thanks for the blog it was helpful.
I have a situation, where I want to show user a custom page if he reaches the max limit for concurrent session. On that page, when the user clicks on Continue, that's when we invalidate the oldest one and create a new one.
I wanted to see if that is something supported now?

Appreciate your help!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment