SEC-2002: Added events to notify of session ID change

[beamerblvd]

Session fixation protection, whether by clean new session or
migrated session, now publishes an event when a session is
migrated or its ID is changed. This enables application developers
to keep track of the session ID of a particular authentication
from the time the authentication is successful until the time
of logout. Previously this was not possible since session
migration changed the session ID and there was no way to
reliably detect that.

[beamerblvd]

I have signed and agree to the terms of the SpringSource Individual Contributor License Agreement.

[rwinch]

We should change the class to initialize applicationEventPubisher to a do nothing implementation like NullEventPublisher in ProviderManager to ensure passivity. While this implements ApplicationEventPublisherAware, applicationEventPublisher may not be initialized if a user instantiates an instance of SessionFixationProtectionStrategy outside of the Spring container.

[rwinch]

PS: Please add a test for this change (i.e. instantiate SessionFixationProtectionStrategy manually and ensure the onSessionChange method does not throw a NullPointerException

[beamerblvd]

Makes sense. Will make this change and update. Do I just make another commit to my fork and then submit a new pull request with both commits in it? Or add my new commit to this pull request? I'm very new to this Git thing...

[rwinch]

Since no one is building on your repository, you can just make the changes and amend the commit:

git add .
git commit --amend

Then you can force push (typically bad, but since no one is using your fork it is ok). Assuming origin is pointed at your repo it would look like

git push -f origin master

Note: typically it is best to create a branch for any work you do. So in your instance you would have wanted to create a branch named SEC-2002 to do all your work on. If you read the contributor guidelines it talks about this and gives some links. I don't view this as critical, but some projects may be more strict about this than others. It certainly helps if you need to rebase (although if you know what you are doing you can still rebase with no problems)

[beamerblvd]

Yea I couldn't figure out the difference between fork and clone and branch. I wasn't sure whether I was doing what I was supposed to or not. Do I have to fork spring-security and THEN branch my fork? Or can I just branch directly from spring-security?

[rwinch]

The order of forking and branching doesn't matter, but it is typically easier to:

• Fork
• Clone your fork on your local machine
• create a branch git checkout -b SEC-2002
• do your work
• commit your code git add .; git commit -m 'SEC-2002: did some things'
• push your code to your repo git push origin SEC-2002
• create a pull request from your branch

You might read the contributor guidelines which points to github doc. Alternatively, this is a rather streamlined article that might help get you going http://blog.springsource.org/2010/12/21/social-coding-in-spring-projects/

[beamerblvd]

Rob, there is no such thing as NullEventPublisher in Spring Framework or Spring Security. There are no implementations of ApplicationEventPublisher that I can find in either framework that do what you're suggesting. Additionally, Spring Security classes AbstractSecurityInterceptor, AbstractJaasAuthenticationProvider and many others in Spring Security initialize their ApplicationEventPublishers to null.

However, what they DO all do is check to make sure the event publisher isn't null before publishing an event. I can do that to ensure that a NPE is never thrown, and add an appropriate test as well.

[rwinch]

Rob, there is no such thing as NullEventPublisher in Spring Framework or Spring Security. There are no implementations of ApplicationEventPublisher that I can find in either framework that do what you're suggesting.

It is private, but NullEventPublisher does exist. You will want to copy the implementation and keep it private. It can be found inside Eclipse use Shift+Ctrl+T and inside IntelliJ use Ctrl+N You can also enter the following search into github to find it @SpringSource/spring-security NullEventPublisher

Additionally, Spring Security classes ...

The concern here is passivity. As the code stands today if I created SessionFixationProtectionStrategy outside of the Spring container I would not need to inject an ApplicationEventPublisher for it to work. With these changes I would need to inject it to prevent a NullPointerException

[beamerblvd]

See the new code for this in the new commit below:

protected void onSessionChange(String originalSessionId, HttpSession newSession, Authentication auth) {
    if(applicationEventPublisher != null) {
        applicationEventPublisher.publishEvent(new SessionFixationProtectionMigrationEvent(
            auth, originalSessionId, newSession.getId(), migrateSessionAttributes
        ));
    }
}

Should never result in a NPE. Is that sufficient, or is the NullEventPublisher really needed here for something I'm not seeing?

[beamerblvd]

Okay, I have pushed my amended commit and the pull request appears to have updated automatically with my new commit. However, I'm very confused. I made one push, not two. However, there are two commits there. One of them has a comment "Merge remote-tracking branch 'origin/master'." But I did not write that comment or make that commit. And the contents are confusing. Can you just ignore it? I can't seem to make it go away. All you need to worry about is the new "SEC-2002: Added events to notify of session ID change ...".

I swear, svn diff was way easier... :-(

[rwinch]

You do not need to worry about this so long as it says "this pull request can be automatically merged" as git is creating a commit to merge your commit with the latest code. The reason is your commit is based upon an older version of Spring Security (i.e. I have pushed out changes since you started your work). This is where (once you understand it) git is awesome as it can merge very well.

In general, I prefer a linear history so I will perform a rebase to remove the merge commit. The rebase is easy for me to do so long as it says it can be merged automatically so no need to worry about it.

[beamerblvd]

Excellent. Well assuming that you agree to my resolution of the NPE (checking to make sure applicationEventPublisher != null before publishing the event), I believe it should be ready to pull. :-)

[beamerblvd]

Man. 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.

Aren't file renames one of the problems people always complained about with SVN? I figured surely Git would have at least fixed that. :-P

[beamerblvd]

I don't understand why everything I do against my fork is ending up on this pull request. All I did was pull from upstream and push to origin to get my fork up to date. It has nothing to do with this pull request. scratches head

[rwinch]

Ensure to branch your code when you are working on a distinct feature. This is a problem with submitting a pull request from master. If you haven't already, I highly recommend reading http://blog.springsource.org/2010/12/21/social-coding-in-spring-projects/ and http://git-scm.com/book (an awesome free Git book)

PS: If it makes you feel better, it took me a while to understand Git too....but now that I do I would not go back to SVN, CVS, etc

[beamerblvd]

Yea, at the time I didn't know the difference between forking and branching in Git. I do a little better now, and for SEC-2135 I will be branching. However, I can't get started on that until this is committed. :-)

Earlier in the week you told me that normally I would need to branch before committing but that it was okay for this one time. Do I now need to go back and re-do this with a branch and a new pull request? That would certainly be inconvenient. :-/

[rwinch]

Yea, at the time I didn't know the difference between forking and branching in Git. I do a little better now, and for SEC-2135 I will be branching. However, I can't get started on that until this is committed. :-)

You can. Try the following:

git checkout -b SEC-2135 origin/master

That says to create a new branch that is based on the latest code in origin/master (i.e. origin is the name of a remote repository and master is that remote repository's master branch)

If you want to move this code into a branch that is possible too. In your master branch (where this code is present) perform the following:

git checkout master
git checkout -b SEC-2002

It should now have all your changes for this commit. Verify they are present. Once you have verified that they are present, change to master and reset it to origin/master.

git checkout master
# this will remove all your changes in your master branch but you have a copy in SEC-2002
git reset --hard origin/master your newly created branch

You can now work on SEC-2002 if you need and then push your changes in the new branch to your remote:

git checkout SEC-2002
git push origin SEC-2002

Now you can close this pull request and open a new one based on your SEC-2002 branch. At this point you can work in each of your branches independently. If you have time the pro git book will talk about how to rebase too which means adding the changes from the Spring Security repository to your local repository and pretending that your commit happened after the commit pushed at a later time.

[beamerblvd]

It has taken me a long time to get around to finishing this. I'm finally going to do that today. I'd going to start clean and do it the "right way" (fork, branch, clone, push, pull). Deleting my spring-security fork, so this pull request might go away automatically (I don't know how that works). If it doesn't, I'll delete it after posting my new pull request.

[rwinch]

Closing since merged with #33

[beamerblvd]

Previous comment did not close. Closing since merged with #33.