Spring Security
  1. Spring Security
  2. SEC-1587

HttpSessionSecurityContextRepository should clear session context when current context is anonmous or empty

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 3.0.3
    • Fix Version/s: 3.0.5, 3.1.0.M2
    • Component/s: None
    • Labels:
      None

      Description

      I have written a LogoutFilter which does not redirect to a logout page. It just logs out the user and goes on with the filter chain.
      (it's needed because we have different areas and the user should be logged out when switching to another "area")

      Before it comes to saving the security context with HttpSessionSecurityContextRepository the anonymousFilter sets an AnonymousAuthentication. So when it comes to saving the context, it is not saved.

      This is because of this code in HttpSessionSecurityContextRepository

      protected void saveContext(SecurityContext context) {
      // See SEC-776
      if (authenticationTrustResolver.isAnonymous(context.getAuthentication())) {
      if (logger.isDebugEnabled())

      { logger.debug("SecurityContext contents are anonymous - context will not be stored in HttpSession. "); }

      return;
      }

      BTW my LogoutFilter is configured with invalidateHttpSession=false otherwise this bug wouldn't occur i guess.

      So imho, there should be another check to see what is actually saved in the session.

      regards
      Janning

        Activity

        Hide
        Luke Taylor added a comment -

        I don't really see what the bug is. It is intended that anonymous authentication tokens aren't store in the session.

        Show
        Luke Taylor added a comment - I don't really see what the bug is. It is intended that anonymous authentication tokens aren't store in the session.
        Hide
        Janning Vygen added a comment -

        hmm, it's quite clear to me i'll try it again, if you still don't see the bug, i will try to create a test project.

        • we have a userAuthentication saved in the session.
        • user clicks "logout"
        • first HttpSessionSecurityContextRepository loads the userAuthentication from the session into the SecurityContext
        • logoutfilter clears the securityContext in memory, but not in the session because of invalidateHttpSession=false
        • logoutfilter does not "return", but calls filterChain.doFilter()
        • anonymousfilter sets anonymousAuthentication into SecurityContext
        • HttpSessionSecurityContextRepository does not save the current context to the session because its anonymous
        • userAuthentication is still saved in the session

        on the next page HttpSessionSecurityContextRepository loads the userAuthentication from the session. user is still logged in.

        I think this bug only occurs if your logoutfilter does not call "return" but filter.doChain(). Maybe this is a bug in my LogoutFilter implementation.
        But i think HttpSessionSecurityContextRepository should do an additional check to prevent situation like the one described above.

        Show
        Janning Vygen added a comment - hmm, it's quite clear to me i'll try it again, if you still don't see the bug, i will try to create a test project. we have a userAuthentication saved in the session. user clicks "logout" first HttpSessionSecurityContextRepository loads the userAuthentication from the session into the SecurityContext logoutfilter clears the securityContext in memory, but not in the session because of invalidateHttpSession=false logoutfilter does not "return", but calls filterChain.doFilter() anonymousfilter sets anonymousAuthentication into SecurityContext HttpSessionSecurityContextRepository does not save the current context to the session because its anonymous userAuthentication is still saved in the session on the next page HttpSessionSecurityContextRepository loads the userAuthentication from the session. user is still logged in. I think this bug only occurs if your logoutfilter does not call "return" but filter.doChain(). Maybe this is a bug in my LogoutFilter implementation. But i think HttpSessionSecurityContextRepository should do an additional check to prevent situation like the one described above.
        Hide
        Luke Taylor added a comment -

        Ok, I see what you mean now. The problem is that the original context is retained in the session when it should be removed. I'll take a look at how that can be accomodated. As a workaround you can probably just directly remove the context attribute from the session in your LogoutFilter (rather than invalidating the session as most users would require for a logout).

        Show
        Luke Taylor added a comment - Ok, I see what you mean now. The problem is that the original context is retained in the session when it should be removed. I'll take a look at how that can be accomodated. As a workaround you can probably just directly remove the context attribute from the session in your LogoutFilter (rather than invalidating the session as most users would require for a logout).
        Hide
        Luke Taylor added a comment -

        Hmm. Looking at this again, I'm still not quite sure why it would happen (unless you are using clustering). The SecurityContext in the ThreadLocal should be the same instance as the one in the session (since that's where it's loaded from), so when it is overwritten, the one in the session should be also. It probably still makes sense to add a fix with an explicit removeAttribute() call when clustering is being used, but it would be useful if you could check again what is actually happening in your case.

        Show
        Luke Taylor added a comment - Hmm. Looking at this again, I'm still not quite sure why it would happen (unless you are using clustering). The SecurityContext in the ThreadLocal should be the same instance as the one in the session (since that's where it's loaded from), so when it is overwritten, the one in the session should be also. It probably still makes sense to add a fix with an explicit removeAttribute() call when clustering is being used, but it would be useful if you could check again what is actually happening in your case.
        Hide
        Luke Taylor added a comment -

        I've added appropriate checks in the context saving logic to explicitly call session.removeAttribute() when the security context is empty or anonymous.

        Show
        Luke Taylor added a comment - I've added appropriate checks in the context saving logic to explicitly call session.removeAttribute() when the security context is empty or anonymous.
        Hide
        Janning Vygen added a comment -

        From your previous post, i was just about starting to look at this again. But i think you already fixed it, right? If not, please send me a mail.

        Show
        Janning Vygen added a comment - From your previous post, i was just about starting to look at this again. But i think you already fixed it, right? If not, please send me a mail.
        Hide
        Luke Taylor added a comment -

        Yeah, I've added the fix, but I'm still not sure why it would be needed in a single-VM situation.

        Show
        Luke Taylor added a comment - Yeah, I've added the fix, but I'm still not sure why it would be needed in a single-VM situation.
        Hide
        Janning Vygen added a comment -

        I have attached a maven test project. i hope it helps. if not please ask.

        the main reason for this to happen is:

        • LogoutFilter does not return after logout but continues with doChain
        • LogoutFilter does not invalidate session

        please take a loolk at the source code. project is very small.
        tets fails with 3.0.3, it succeeds with 3.0.5

        Show
        Janning Vygen added a comment - I have attached a maven test project. i hope it helps. if not please ask. the main reason for this to happen is: LogoutFilter does not return after logout but continues with doChain LogoutFilter does not invalidate session please take a loolk at the source code. project is very small. tets fails with 3.0.3, it succeeds with 3.0.5
        Hide
        Janning Vygen added a comment -

        Maven test project

        Show
        Janning Vygen added a comment - Maven test project

          People

          • Assignee:
            Luke Taylor
            Reporter:
            Janning Vygen
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: