Erik Onnen(Migrated from SEC-942) said:
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.
Luke Taylor said:
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?
Erik Onnen said:
“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.
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.
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.