SEC-1667: Consider adding a method to SecurityContextHolderStrategy that returns the current state of the SecurityContext without creating a new one

[spring-projects-issues]

Kyrill Alyoshin (Migrated from SEC-1667) said:

We do have scenarios when we have to call SecurityContextHolder#getContext outside of web requests. (Obviously, the context will not be populated in such cases.) What will happen though is a new empty SecurityContext will be created and put on a ThreadLocal without being cleared by the servlet filter (as is the case during web requests). This will, of course, lead to class loader based memory leaks on hot redeploys.

It sure would be nice to add, say, getExistingContext() method to SecurityContextHolderStrategy, which would not create a context if it is not available, and just return an existing one or null otherwise. Then we can call SecurityContextHolder.getContextHolderStrategy().getExistingContext() and
we're safe.

What do you think?

[rwinch]

This does not make sense to me. If you are accessing the SecurityContext you need to ensure it is cleared out. If you are outside of the context of a request then you should not be accessing the SecurityContext because it won't be populated. If you want to ensure it is populated, then whatever populates the context should clear it out. A likely option is to use Spring's concurrency support. In particular, you could use DelegatingSecurityContextRunnable or one of the higher level support classes.

[jzheaux]

Given #9877, I think this is something that should be reconsidered. In summary, Spring Security supplies a reactive hook that inspects the security context, and this hook is invoked during shutdown, causing a memory leak like the one described in this ticket.

More generally, it seems reasonable when operating at the framework level that there will be circumstances when performing a peek is valuable, somewhat similar to how Spring Security will often ask for an instance of the HttpSession without wanting to create it.

Instead of getExistingContext(), though, I believe peekContext() will be more descriptive.

To maintain backward compatibility, it would need to be a default method on SecurityContextHolderStrategy that returns getContext(). This is so that peekContext() == null means that there is no context. That said, peekContext() should be overridden by all implementations to return the context.