Closed spring-projects-issues closed 10 years ago
spring-projects-issues
commented
11 years ago Nick Williams said:
I meant for the title of this bug to be "AuthenticationSuccessEvents called before session is migrated" but apparently messed that up somehow. Since I can't seem to find an edit function, please affect this change for me.
spring-projects-issues
commented
11 years ago Nick Williams said:
Correction:
In order to work around this issue, I changed session-fixation-protection from "none" to "migrateSession." It made no difference. The session ID still gets changed after the event gets called. So, this is just plain broke; session fixation protection doesn't matter.
spring-projects-issues
commented
11 years ago Rob Winch said:
As Luke mentioned this is not a bug. The intent is that the Authentication is the result of what caused the event (as per the javadoc). The event was caused by successfully authenticating the Authentication with the WebAuthenticationDetails that has a JSESSIONID before the session was changed. Therefore the JSESSIONID will be different than the JSESSIONID that is present in these events.
spring-projects-issues
commented
11 years ago Nick Williams said:
Respectfully, I disagree. How is an AuthenticationSuccessEvent any less valid after the session is migrated than before it is migrated? (Answer: it isn't.) Therefore, if the AuthenticationSuccessEvent would have more accurate information after session migration than it does before session migration, then why not issue the event after migrating the session.
The answer is, the AuthenticationSuccessEvent should be issued AFTER the session is migrated. It hurts nothing to do so, and doing so results in more accurate information at event issue time. There are no downsides to this. Why not fix it?
spring-projects-issues
commented
11 years ago Rob Winch said:
The behavior is working as documented and this feature has long since shipped. The information is accurate as documented. Specifically the Authentication is contains details about the session id at time of authentication. These details are not intended to be the current session. We cannot change the behavior since it is working as documented and others are currently using it. While it makes little sense for your use case, there are many Spring Security users and we cannot make assumptions about how they are using the framework or what their requirements are. For example, they may want to know the JSESSIONID and ipaddress of the user at time authentication to track where the original authentication came from (this is the intent behind this feature).
I can see how the session id after session migration can be valuable to others as well and so we can consider an enhancement to the code base and/or documentation to make this easier. However, the existing behavior must stay the same for the reasons listed above. One option to obtain the current HttpServletRequest information is using Spring's RequestContextHolder. In order to use the RequestContextHolder outside of the DispatcherServlet (i.e. Spring MVC controllers) ensure that you register the RequestContextListener or RequestContextFilter in your web.xml.
spring-projects-issues
commented
11 years ago Nick Williams said:
Valid point. As hard as it is for me to imagine a scenario where someone would be relying on the event being published before the session is migrated, I wouldn't want to break functionality developers are currently relying on. So, instead of changing the existing authentication event, here are some alternatives:
What do you think of these alternatives?
spring-projects-issues
commented
11 years ago Rob Winch said:
Rather than discussing how it is to be done, I'd like to first try to figure out what you need done. For example, it sounded as though I could summarize your requirements as
Are there other things you would expect? For example, what about the ipaddress at these times (which would also be stale on the WebAuthenticationDetails)? There may be other things you are looking for and I want to make sure we iron out these details before we decide how we do it.
Why doesn't the JSESSIONID from when the user submitted their username and password not work for you? If you have additional attributes why do you need those vs what is already provided. I'm not doubting your needs, but is important that I understand the whole scenario so that we can come to the best outcome.
Once we know everything that needs to be done, we can discuss ways we can accomplish the requirements.
Cheers, Rob
spring-projects-issues
commented
11 years ago Nick Williams said:
I'm trying to track login/logout history. So, I at AuthenticationSuccessEvent time, I insert a record into the database with user ID, session ID, IP address and login time. Then, at SessionDestroyedEvent time, I update the record with the matching session ID and no logout date to indicate when they logged out (that way, even if their session just times out instead of them clicking the "logout" button, I still close their login history record).
A SessionMigratedEvent that contains the old and new session IDs would solve my problem, because it would allow me to find the record with the old session ID and update it to the new session ID, but that isn't necessarily ideal because then I have to (arguably unnecessarily) update a record I just inserted. However, a SessionMigratedEvent might be more useful for the general community.
An AuthenticationSuccessAfterMigrationEvent would also solve my problem as long as it still contained the WebAuthenticationDetails object (because I need IP address, session ID and principal).
Does this help?
spring-projects-issues
commented
11 years ago Rob Winch said:
Thank you this helps. I will need to sit on it a bit to figure out how to ensure this works for the general population.
spring-projects-issues
commented
11 years ago Nick Williams said:
Okeydokey.
spring-projects-issues
commented
11 years ago Nick Williams said:
Rob,
It's been several months since I've seen an update on this. I noticed that Spring 3.2 is in Milestone 1. I don't know whether M2 is next, or RC1, or what, but I would really love to see this resolved in 3.2.
I would be willing to contribute a patch if possible, if it will make the difference between it getting done in 3.2 or not. If this will work, let me know. Meanwhile, I'll investigate the source code to see what it'll take.
Thanks,
Nick
P.S. I just realized that one of the comments made in this bug was never addressed (possibly because I accidentally stated it incorrectly). I tried to work around this problem by disabling session migration. I know that ultimately session migration is the most secure, but this application is only available through a VPN, so I wasn't as concerned with that. I changed session-fixation-protection from "migrateSession" to "none." It made no difference. The session ID is STILL getting changed after the event gets called. The documentation (http://static.springsource.org/spring-security/site/docs/3.1.x/reference/ns-config.html) clearly states that setting session-fixation-protection to "none" disables session migration ("none - Don't do anything. The original session will be retained.), so since my sessions are still getting migrated, I believe at least that part is actually a bug.
spring-projects-issues
commented
11 years ago Rob Winch said:
Nick,
I think these are two distinct issues. The first is that you would like to be able to access the session id and the second is that session-fixation-protection="none" is not working for you. Please feel free to submit a new JIRA (as a bug) with a sample application that demonstrates this issue with clear instructions on how to reproduce it.
If you are able to put together a pull request, this will certainly help to ensure this issue is included in M2 (the next release). In fact, sending a pull request will assist with getting any issue resolved faster. Please be sure to read the Contributor Guidelines to make sure the pull request can be merged as easily as possible https://github.com/SpringSource/spring-security/wiki/Contributor-Guidelines
spring-projects-issues
commented
11 years ago Nick Williams said:
I have submitted a pull request. Includes two new classes, one new method, one changed method, three changed unit tests and two new unit tests. Please let me know if there is anything else I need to do.
spring-projects-issues
commented
11 years ago Nick Williams said:
By the way, I have signed and agree to the terms of the SpringSource Individual Contributor License Agreement. I forgot to add that comment to my pull request. I have added the comment now.
spring-projects-issues
commented
11 years ago Nick Williams said:
Is there anything more I can do? Did I follow all of the contributor guidelines sufficiently? Is my code formatted correctly? :-)
spring-projects-issues
commented
11 years ago Rob Winch said:
First of all thank you for your contribution and sorry for not getting back to you. I haven't had the opportunity to look over this yet. I will try and get back to you on Monday.
spring-projects-issues
commented
11 years ago Nick Williams said:
Okeydokey.
By the way I'm going to go ahead and file an enhancement request that's related to the SessionFixationProtectionStrategy but unrelated to the changes I proposed in this bug. I'd like to start the conversation now and see what needs to be done to get the enhancement implemented correctly. However, getting this pull request accepted is currently my top priority.
Thanks.
spring-projects-issues
commented
11 years ago Rob Winch said:
I have posted my feedback on the pull request
spring-projects-issues
commented
11 years ago Nick Williams said:
I have revised and updated the pull request.
spring-projects-issues
commented
11 years ago Nick Williams said:
The previous commit did not rename the events properly. The old events remained behind and the new ones were just added instead of replacing them. Looks like it happened in the merge, too, because when I updated my local repository the old events reappeared. I have deleted them with another commit. NOW it should be ready for you to pull.
Also I have created SEC-2137 regarding inability to turn off session fixation protection (session-fixation-protection="none" is not working).
spring-projects-issues
commented
11 years ago Nick Williams said:
Hey, Rob. Is there any progress to report on getting this pulled in? Do I need to further revise my commit in any way?
Thanks,
Nick
spring-projects-issues
commented
11 years ago Rob Winch said:
I have been working on some other things and I'm still working my way back around to this issue (currently this is scheduled for Monday).
spring-projects-issues
commented
11 years ago Nick Williams said:
Okay. The comment you made on the pull request an hour ago confused me, so if you could respond to my question that would be great. Thanks!
(P.S. Sorry if it seems like I'm nagging. I know y'all are busy, so I also know I have to be persistent to avoid getting lost in the shuffle. :-) )
spring-projects-issues
commented
11 years ago Rob Winch said:
You are not nagging...it is GREAT to have contributions! I think I replied (I just responded to the latest comment but there are quite a few so perhaps it was a different one), but if you don't see it please send me a link to the comment.
spring-projects-issues
commented
11 years ago Nick Williams said:
Yes, you replied, I'm just still not clear on whether you NEED me to re-do this pull request from a branch (originally you said it would be fine this time), or whether you're just telling me for future reference again.
spring-projects-issues
commented
11 years ago Rob Winch said:
No I do not need you to do it, but thought it would make working on multiple issues at the same time easier for you.
spring-projects-issues
commented
10 years ago Rob Winch said:
Resolved per https://github.com/SpringSource/spring-security/pull/33 Thanks for contributing Nick!
Nick Williams (Migrated from SEC-2002) said:
Both types of successful authentication events (AuthenticationSuccessEvent AND InteractiveAuthenticationSuccessEvent) are fired BEFORE the session fixation protection migrates the session data. This is INVALID, as the session ID contained within the data provided by the event is no longer valid immediately after the event completes processing.
So, for example, one cannot use ApplicationListeners for AuthenticationSuccessEvent and SessionDestroyedEvent to maintain a log of login/logout activity, because the session ID changes between when the success event is called and the destroyed event is called.
There was a discussion about this in the Spring forums (attached). The conclusion reached in this discussion is, I believe, also invalid. Spring Security classes shouldn't have to be overridden in order to make them behave correctly. It seems a trivial matter for the authentication event to be called AFTER the session has been migrated, which would provide accurate information in objects provided, through events, to a consuming application.