SEC-1993: Provide explicit method to remove SecurityContext for custom SecurityContextRepository's

[spring-projects-issues]

Jonathan Baker (Migrated from SEC-1993) said:

Updated Description

My problem is that there is an abstraction called SecurityContextRepository (currently the primary implementation stores the security context in the session)
When you call clear context, that removes the security context from the holder, but not from the security context repository. By invalidating the session you are essentially clearing the security context from the security context repository via a backdoor. It would be better if you explicitly removed the security context from the security repository via a method on the SecurityContextRepository interface. This is because not everyone stores their security context in the http session. Some people have implemented their own security context repository. For people with a non-session-based SecurityContext repository invalidating the session does nothing, and we have to code some hack to actually get the security context cleared out.
It may be that you still need to invalidate the http session but you should add an explicit call to clear the security context repository first.

h1. Original Description

One of the nice features of spring security is that it has an abstraction for SecurityContextRepository. The default implementation is in the HttpSession, but it was nice that there was a layer there in case people wanted to use some other persistence mechanism for the security context. We are one of the people who created a service to store the security context so it can be shared across servers.

However the SecurityContextLogoutHandler accesses the http session to invalidate it directly. It would be nice if this class removed the security context from the security context repository instead of (or in addition to) invalidating the session.

This could be driven by a boolean property as well cleanContextFromContextRepository or something like that...

[spring-projects-issues]

Rob Winch said:

I'm not sure I understand the request. The SecurityContextLogoutHandler will invoke SecurityContextHolder.clearContext(). It also can optionally (default true) invalidate the HttpSession. If you do not want it to invalidate the session you can set invalidateHttpSession to false. Can you please clarify how the proposal differs from the existing functionality?

[spring-projects-issues]

Rob Winch said:

Resolving as invalid per previous comment and no further feedback given.

[spring-projects-issues]

Jonathan Baker said:

I'm sorry, for some reason I never got notified that you had made a comment on the jira issue. I am guessing postini blocked it. I hope its not too late.

My problem is that there is an abstraction called SecurityContextRepository (currently the primary implementation stores the security context in the session)

When you call clear context, that removes the security context from the holder, but not from the security context repository. By invalidating the session you are essentially clearing the security context from the security context repository via a backdoor. It would be better if you explicitly removed the security context from the security repository via a method on the SecurityContextRepository interface. This is because not everyone stores their security context in the http session. Some people have implemented their own security context repository. For people with a non-session-based SecurityContext repository invalidating the session does nothing, and we have to code some hack to actually get the security context cleared out.

It may be that you still need to invalidate the http session but you should add an explicit call to clear the security context repository first.

[spring-projects-issues]

Rob Winch said:

It is never too late :) We can always revisit issues, I just like to try to keep JIRA cleaned up so if I feel I have explained something well and there is no feedback I tend to resolve it. Since you appear to still providing feedback I will reopen it while we discuss.

When you call clear context, that removes the security context from the holder, but not from the security context repository.

This is not entirely true. The SecurityContextPersistenceFilter will see that the SecurityContextHolder has been cleared out and will invoke SecurityContextRepository.saveContext(null, HttpServletRequest, HttpServletResponse). This should indicate the SecurityContext associated with the current HttpServletRequest should be removed. This is not quite as explicit as a remove method. However, it should work since the original SecurityContext must have some sort of association to the HttpServletRequest and HttpServletResponse since this is how the load method was implemented in the first place.

If this does not work, you can still implement a LogoutHandler which passes in the previous Authentication object allowing you to remove it from your repository (i.e. your SecurityContextRepositoryImpl could implement both SecurityContextRepository and LogoutHandler).

[spring-projects-issues]

Rob Winch said:

Reopening due to response from reporter

[spring-projects-issues]

Jonathan Baker said:

I don't think that the PersistenceFilter calls saveContext(null, ...) I think the default behavior from the holder is to create a new emtpy context, so the filter essentially ends up calling saveContext(new SecurityContectImpl(), ...)

That may just be picking nits though at this point.

I guess I should state that we aren't actually experiencing a problem at this time. We have everything working, I was just surprised to not see an explicit call to the security context repo.

I marked this issue as an improvement because I think that explicitly talking to the repo to clear things out would be better than saving a null or empty as an implicit "clear"

[spring-projects-issues]

Rob Winch said:

In reply to comment #6:

I don't think that the PersistenceFilter calls saveContext(null, ...) I think
the default behavior from the holder is to create a new emtpy context, so the
filter essentially ends up calling saveContext(new SecurityContectImpl(), ...)

You are correct the SecurityContext will not be null, but the Authentication will be (the rest of the logic should still apply).

[spring-projects-issues]

Rob Winch said:

I updated the subject and description based upon your last comment. I have also given some more thought to your situation.

First we cannot add a new method to the SecurityContextRepository as this would be a non-passive change. Second it seems like the functionality you need is already available in the suggestion I provided. Specifically you can implement both the LogoutHandler and SecurityContextRepository interfaces. If the Authentication is not enough for you (which is the argument to the LogoutHandler), the SecurityContext can be accessed in the logout method using the SecurityContextHolder so long as you have not cleared the SecurityContextHolder first (i.e. order matters).

In summary, I'm closing as "Won't Fix" because the LogoutHandler provides a concise interface for logging a user out (i.e. clearing the SecurityContextRepository).

[spring-projects-issues]

Jonathan Baker said:

That makes sense. Thanks for the consideration, and the suggestion.