Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SEC-1188: add getter to SecurityContextHolder to retrieve SecurityContextHolderStrategy instance, to enable injection #1436

Closed
spring-issuemaster opened this issue Jun 19, 2009 · 2 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link

commented Jun 19, 2009

Scott Bale (Migrated from SEC-1188) said:

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.

@spring-issuemaster

This comment has been minimized.

Copy link
Author

commented Aug 12, 2009

Luke Taylor said:

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.

@spring-issuemaster

This comment has been minimized.

Copy link
Author

commented Aug 22, 2009

Luke Taylor said:

getContexHolderStrategy() method added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.