From c6670b833c375de371d9fa5912368e1d07bf000d Mon Sep 17 00:00:00 2001 From: Hiroshi Shirosaki Date: Wed, 16 Jun 2021 16:53:13 +0900 Subject: [PATCH 1/2] Add SecurityContextHolder.peekContext() To get current context without creating a new context. Creating a new context may cause ThreadLocal leak. --- .../core/context/GlobalSecurityContextHolderStrategy.java | 5 +++++ ...heritableThreadLocalSecurityContextHolderStrategy.java | 5 +++++ .../security/core/context/SecurityContextHolder.java | 8 ++++++++ .../core/context/SecurityContextHolderStrategy.java | 6 ++++++ .../context/ThreadLocalSecurityContextHolderStrategy.java | 5 +++++ 5 files changed, 29 insertions(+) diff --git a/core/src/main/java/org/springframework/security/core/context/GlobalSecurityContextHolderStrategy.java b/core/src/main/java/org/springframework/security/core/context/GlobalSecurityContextHolderStrategy.java index d8367c4ebda..330afdb7430 100644 --- a/core/src/main/java/org/springframework/security/core/context/GlobalSecurityContextHolderStrategy.java +++ b/core/src/main/java/org/springframework/security/core/context/GlobalSecurityContextHolderStrategy.java @@ -44,6 +44,11 @@ public SecurityContext getContext() { return contextHolder; } + @Override + public SecurityContext peekContext() { + return contextHolder; + } + @Override public void setContext(SecurityContext context) { Assert.notNull(context, "Only non-null SecurityContext instances are permitted"); diff --git a/core/src/main/java/org/springframework/security/core/context/InheritableThreadLocalSecurityContextHolderStrategy.java b/core/src/main/java/org/springframework/security/core/context/InheritableThreadLocalSecurityContextHolderStrategy.java index cb415500ca3..d08b221c288 100644 --- a/core/src/main/java/org/springframework/security/core/context/InheritableThreadLocalSecurityContextHolderStrategy.java +++ b/core/src/main/java/org/springframework/security/core/context/InheritableThreadLocalSecurityContextHolderStrategy.java @@ -44,6 +44,11 @@ public SecurityContext getContext() { return ctx; } + @Override + public SecurityContext peekContext() { + return contextHolder.get(); + } + @Override public void setContext(SecurityContext context) { Assert.notNull(context, "Only non-null SecurityContext instances are permitted"); diff --git a/core/src/main/java/org/springframework/security/core/context/SecurityContextHolder.java b/core/src/main/java/org/springframework/security/core/context/SecurityContextHolder.java index 810edef7c93..871e3507d89 100644 --- a/core/src/main/java/org/springframework/security/core/context/SecurityContextHolder.java +++ b/core/src/main/java/org/springframework/security/core/context/SecurityContextHolder.java @@ -110,6 +110,14 @@ public static SecurityContext getContext() { return strategy.getContext(); } + /** + * Peeks the current SecurityContext. + * @return the security context (may be null) + */ + public static SecurityContext peekContext() { + return strategy.peekContext(); + } + /** * Primarily for troubleshooting purposes, this method shows how many times the class * has re-initialized its SecurityContextHolderStrategy. diff --git a/core/src/main/java/org/springframework/security/core/context/SecurityContextHolderStrategy.java b/core/src/main/java/org/springframework/security/core/context/SecurityContextHolderStrategy.java index 4954db70aac..2a29566fae8 100644 --- a/core/src/main/java/org/springframework/security/core/context/SecurityContextHolderStrategy.java +++ b/core/src/main/java/org/springframework/security/core/context/SecurityContextHolderStrategy.java @@ -38,6 +38,12 @@ public interface SecurityContextHolderStrategy { */ SecurityContext getContext(); + /** + * Peeks the current context without creating an empty context. + * @return a context (may be null) + */ + SecurityContext peekContext(); + /** * Sets the current context. * @param context to the new argument (should never be null, although diff --git a/core/src/main/java/org/springframework/security/core/context/ThreadLocalSecurityContextHolderStrategy.java b/core/src/main/java/org/springframework/security/core/context/ThreadLocalSecurityContextHolderStrategy.java index 801f5c82074..84f23bbe22d 100644 --- a/core/src/main/java/org/springframework/security/core/context/ThreadLocalSecurityContextHolderStrategy.java +++ b/core/src/main/java/org/springframework/security/core/context/ThreadLocalSecurityContextHolderStrategy.java @@ -45,6 +45,11 @@ public SecurityContext getContext() { return ctx; } + @Override + public SecurityContext peekContext() { + return contextHolder.get(); + } + @Override public void setContext(SecurityContext context) { Assert.notNull(context, "Only non-null SecurityContext instances are permitted"); From aeb6f5f3787700f086a85fffecea817c98151354 Mon Sep 17 00:00:00 2001 From: Hiroshi Shirosaki Date: Thu, 3 Jun 2021 16:37:01 +0900 Subject: [PATCH 2/2] Fix ThreadLocal leak with SecurityContextHolder Use SecurityContextHolder.peekContext() so that it doesn't create an empty object in ThreadLocal. --- .../SecurityReactorContextConfiguration.java | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/config/src/main/java/org/springframework/security/config/annotation/web/configuration/SecurityReactorContextConfiguration.java b/config/src/main/java/org/springframework/security/config/annotation/web/configuration/SecurityReactorContextConfiguration.java index 2783cb358bc..4210e90fe61 100644 --- a/config/src/main/java/org/springframework/security/config/annotation/web/configuration/SecurityReactorContextConfiguration.java +++ b/config/src/main/java/org/springframework/security/config/annotation/web/configuration/SecurityReactorContextConfiguration.java @@ -36,6 +36,7 @@ import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.security.core.Authentication; +import org.springframework.security.core.context.SecurityContext; import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.web.context.request.RequestAttributes; import org.springframework.web.context.request.RequestContextHolder; @@ -94,7 +95,12 @@ CoreSubscriber createSubscriberIfNecessary(CoreSubscriber delegate) { } private static boolean contextAttributesAvailable() { - return SecurityContextHolder.getContext().getAuthentication() != null + SecurityContext context = SecurityContextHolder.peekContext(); + Authentication authentication = null; + if (context != null) { + authentication = context.getAuthentication(); + } + return authentication != null || RequestContextHolder.getRequestAttributes() instanceof ServletRequestAttributes; } @@ -107,7 +113,11 @@ private static Map getContextAttributes() { servletRequest = servletRequestAttributes.getRequest(); servletResponse = servletRequestAttributes.getResponse(); // possible null } - Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); + SecurityContext context = SecurityContextHolder.peekContext(); + Authentication authentication = null; + if (context != null) { + authentication = context.getAuthentication(); + } if (authentication == null && servletRequest == null) { return Collections.emptyMap(); }