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

Clear null authentication to fix ThreadLocal leak #9877

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -94,7 +95,12 @@ <T> CoreSubscriber<T> createSubscriberIfNecessary(CoreSubscriber<T> 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;
}

Expand All @@ -107,7 +113,11 @@ private static Map<Object, Object> 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();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,14 @@ public static SecurityContext getContext() {
return strategy.getContext();
}

/**
* Peeks the current <code>SecurityContext</code>.
* @return the security context (may be <code>null</code>)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For each new public method that you introduce, will you please add @since 5.6 to the JavaDoc?

*/
public static SecurityContext peekContext() {
return strategy.peekContext();
}

/**
* Primarily for troubleshooting purposes, this method shows how many times the class
* has re-initialized its <code>SecurityContextHolderStrategy</code>.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,12 @@ public interface SecurityContextHolderStrategy {
*/
SecurityContext getContext();

/**
* Peeks the current context without creating an empty context.
* @return a context (may be <code>null</code>)
*/
SecurityContext peekContext();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For backward compatibility, this will need to have a default implementation. Please see #1890 (comment) for more details on the recommended implementation.


/**
* Sets the current context.
* @param context to the new argument (should never be <code>null</code>, although
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,11 @@ public SecurityContext getContext() {
return ctx;
}

@Override
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will you please update the copyright message for each edited file to include the year 2021?

public SecurityContext peekContext() {
return contextHolder.get();
}

@Override
public void setContext(SecurityContext context) {
Assert.notNull(context, "Only non-null SecurityContext instances are permitted");
Expand Down