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?

Comment From: 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.

Comment From: jzheaux

Given https://github.com/spring-projects/spring-security/pull/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.

Comment From: jzheaux

Note that a6841156 addresses the use case I referenced in https://github.com/spring-projects/spring-security/issues/1890#issuecomment-860929424 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.

Comment From: 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 https://github.com/spring-projects/spring-security/issues/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 https://github.com/spring-projects/spring-security/issues/1890#issuecomment-860929424 applies for adding peekContext.

Comment From: xenoterracide

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