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.

[jzheaux]

Note that a684115 addresses the use case I referenced in #1890 (comment) by deferring the SecurityContext lookup. Given that, I'd like to see if #10913 is a better fit to clean up that use case before implementing this ticket.

[jzheaux]

After some additional review of #10913, it appears that the most consistent approach is for Spring Security's servlet-based ExchangeFilterFunctions to anticipate a Supplier<Authentication> instead of an Authentication in the set of reactor context attributes. Spring Security code should only call for Authentication at the time of use.

As such, I've created #11885 to change the servlet-based ExchangeFilterFunctions to read a Supplier<Authentication> from the reactor context instead of an Authentication.

Given that, I don't think that the use case in #1890 (comment) applies for adding peekContext.

[xenoterracide]

hah, I've asked for this before because not every application is a web application.