Spring Security
  1. Spring Security
  2. SEC-942

HttpSessionContextIntegrationFilter Bypasses the SessionContext creation pattern followed by the SecurityContextHolderStrategy implementations

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.0.6, 1.0.7, 2.0.3
    • Fix Version/s: 3.0.0 M1
    • Component/s: Core
    • Labels:
      None

      Description

      The HttpSessionContextIntegrationFilter uses a different mechanism for creating SecurityContext implementations than the SecurityContextHolder. The former calls getInstance in generateNewContext(), the latter uses the SecurityContextHolderStrategy. Initial thought is that the HTSCIF should simply refer to the SCH thereby making the creational behavior consistent.

      While most applications will use the HTSCIF, this has caused me problems in tests where tests go directly to the SCH and are not operating in the context of a web request run through the HTSCIF. Users don't expect to need to change both values.

        Activity

        Hide
        Luke Taylor added a comment -

        The purpose of the SecurityContextHolderStrategy isn't intended to be for the creation of different security context types. It's mainly about the way the context is stored - whether there is one context per VM, whether a ThreadLocal is used etc. Do you have a custom strategy class which creates a different default context type?

        I can see how this might potentially be a way to make things a bit cleaner (both have developed for different historical reasons) but it doesn't seem like a bug. Can you expand on what the problems were that you were encountering?

        Show
        Luke Taylor added a comment - The purpose of the SecurityContextHolderStrategy isn't intended to be for the creation of different security context types. It's mainly about the way the context is stored - whether there is one context per VM, whether a ThreadLocal is used etc. Do you have a custom strategy class which creates a different default context type? I can see how this might potentially be a way to make things a bit cleaner (both have developed for different historical reasons) but it doesn't seem like a bug. Can you expand on what the problems were that you were encountering?
        Hide
        Erik Onnen added a comment -

        "The purpose of the SecurityContextHolderStrategy isn't intended to be for the creation of different security context types" fair enough, but because of the way it simply news a SecrurityContextImpl, it's inconsistent with how the HSCIF creates new SecurityContext impls.

        So in my case, I need a custom SecurityContext impl. I set that class in the HSCIF bean definition. But, I have code that go right at the SecurityContextHolder whose default strategy is to new the default SecurityContextImpl. In the case where my code needs the custom impl and the current thread has not passed through the HSCIF, this then fails.

        It's mainly an issue for test code, but it also applies to things like worker threads which will not pass through the HSCIF.

        Ideally, I would just define the SecurityContext impl class once and be done with. As it stands, to get the behavior I need, I had to subclass ThreadLocalSecurityContextHolderStrategy and change the way contexts are created by getContext to prevent the default strategy from handing out the wrong SecurityContext implementation. That even wouldn't be so bad, but I have to keep it in sync with the HSCIF. The creation pattern for SecurityContext should be the same across the system, maintaining it through two different bean config mechanisms is error prone.

        Show
        Erik Onnen added a comment - "The purpose of the SecurityContextHolderStrategy isn't intended to be for the creation of different security context types" fair enough, but because of the way it simply news a SecrurityContextImpl, it's inconsistent with how the HSCIF creates new SecurityContext impls. So in my case, I need a custom SecurityContext impl. I set that class in the HSCIF bean definition. But, I have code that go right at the SecurityContextHolder whose default strategy is to new the default SecurityContextImpl. In the case where my code needs the custom impl and the current thread has not passed through the HSCIF, this then fails. It's mainly an issue for test code, but it also applies to things like worker threads which will not pass through the HSCIF. Ideally, I would just define the SecurityContext impl class once and be done with. As it stands, to get the behavior I need, I had to subclass ThreadLocalSecurityContextHolderStrategy and change the way contexts are created by getContext to prevent the default strategy from handing out the wrong SecurityContext implementation. That even wouldn't be so bad, but I have to keep it in sync with the HSCIF. The creation pattern for SecurityContext should be the same across the system, maintaining it through two different bean config mechanisms is error prone.
        Hide
        Luke Taylor added a comment -

        We'll see if we can improve things in the next major release. There may be some changes to HttpSCIF then anyway as it is a class which has got a bit out of control over the years.

        Show
        Luke Taylor added a comment - We'll see if we can improve things in the next major release. There may be some changes to HttpSCIF then anyway as it is a class which has got a bit out of control over the years.
        Hide
        Luke Taylor added a comment -

        I've added a createEmptyContext method to the SecurityContextHolder and to the SecurityContextHolderStrategy. HttpSessionSecurityContextRepository now calls this method when creating a new context instead of creating a SecurityContextImpl.

        Show
        Luke Taylor added a comment - I've added a createEmptyContext method to the SecurityContextHolder and to the SecurityContextHolderStrategy. HttpSessionSecurityContextRepository now calls this method when creating a new context instead of creating a SecurityContextImpl.

          People

          • Assignee:
            Luke Taylor
            Reporter:
            Erik Onnen
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: