Spring Security
  1. Spring Security
  2. SEC-1188

add getter to SecurityContextHolder to retrieve SecurityContextHolderStrategy instance, to enable injection

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.0.0 M1
    • Fix Version/s: 3.0.0 RC1
    • Component/s: Core
    • Labels:
      None

      Description

      It's difficult to unit test classes that call directly to SecurityContextHolder.getContext(). I would rather have a SecurityContextHolderStrategy property in my POJO that is injected. Since SCHS is an interface, unit testing the POJO would be much simpler because a mock SCHS can be used to test with (and therefore a mock SecurityContext, a mock Authentication, etc).

      A static getter method would allow me to pull the call to SecurityContextHolder.getContext() into spring config using 'factory-method' attribute, something like this:

      <bean id="fooPOJO" class="org.foo.Foo">
      <property name="securityContextHolderStrategy" >
      <bean class="org.springframework.security.context.SecurityContextHolder"
      factory-method="getContexHolderStrategyt">
      </bean>
      </property>
      </bean>

      I've always been a little puzzled by the widespread practice of directly calling SecurityContextHolder.getContext() from within Java code. Having to call a static method and to access static, vm-wide state directly like that is exactly the sort of thing that dependency injection, and Spring, is supposed to help a developer avoid doing.

      Just so I'm completely clear, let me review the pain of unit testing a class that directly calls SecurityContextHolder.getContext(). If you want to unit test with a mock SecurityContext instance, you have to set that mock instance directly on the SecurityContextHolder, then remember to clean it up during test tear down because it's global, vm-wide state. If your test is running in a large suite of other tests, you may even have to record the current state of SecurityContextHolder.getContext() before setting your mock, then remember to restore the original state because tests later on may be relying on it. Obviously that's both ugly and error-prone. These are all well-understood downsides to having an API based on static methods. On the other hand, if my POJO were to instead have a SecurityContextHolderStrategy property that was injected, unit testing it becomes much cleaner.

        Activity

        Hide
        Luke Taylor added a comment -

        The default strategy uses a ThreadLocal to store the SecurityContext, so the context is not "static VM-wide state". You cannot assume that all classes which require access to the security context have been created through a Spring DI container since Spring Security doesn't mandate that you are using Spring in your application.

        For testing purposes, it's a simple matter to add a

        @After
        public void clearContext()

        { SecurityContextHolder.clearContext(); }

        method to your test class. Writing tests which depend on state from other tests in a suite is a bad idea so I don't agree with your point about having to store the context and restore it afterwards. Since a ThreadLocal is used, running tests in parallel shouldn't be a problem either.

        Having said that it is a straightforward enough request to add a method to read the strategy so I don't see any real reason against it.

        Show
        Luke Taylor added a comment - The default strategy uses a ThreadLocal to store the SecurityContext, so the context is not "static VM-wide state". You cannot assume that all classes which require access to the security context have been created through a Spring DI container since Spring Security doesn't mandate that you are using Spring in your application. For testing purposes, it's a simple matter to add a @After public void clearContext() { SecurityContextHolder.clearContext(); } method to your test class. Writing tests which depend on state from other tests in a suite is a bad idea so I don't agree with your point about having to store the context and restore it afterwards. Since a ThreadLocal is used, running tests in parallel shouldn't be a problem either. Having said that it is a straightforward enough request to add a method to read the strategy so I don't see any real reason against it.
        Hide
        Luke Taylor added a comment -

        getContexHolderStrategy() method added.

        Show
        Luke Taylor added a comment - getContexHolderStrategy() method added.

          People

          • Assignee:
            Luke Taylor
            Reporter:
            Scott Bale
          • Votes:
            1 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: