Clear null authentication to fix ThreadLocal leak #9877
Conversation
|
@shirosaki Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
|
@shirosaki Thank you for signing the Contributor License Agreement! |
spring-projects-issues
added
the
status: waiting-for-triage
eleftherias
added
in: web
|
Hi, @shirosaki, thanks for the PR! I don't think that we can safely clear the context here. This code doesn't know if it's the one to have populated the context. This may work in the special case of destroying a context. But, under normal operations, this may inadvertently log out a user. Perhaps instead #1890 should be reopened and we consider adding a method to |
|
@jzheaux thanks for comments. |
To get current context without creating a new context. Creating a new context may cause ThreadLocal leak.
Use SecurityContextHolder.peekContext() so that it doesn't create an empty object in ThreadLocal.
shirosaki
force-pushed
the
fix_null_authentication
branch
from
f5cfe73
to
aeb6f5f
Compare
There was a problem hiding this comment.
Thanks, @shirosaki! I've left some feedback inline.
Also, in each of your two commit messages will you please include the phrase "Closes gh-xxxx" where xxxx is the related ticket?
| * Peeks the current context without creating an empty context. | ||
| * @return a context (may be <code>null</code>) | ||
| */ | ||
| SecurityContext peekContext(); |
There was a problem hiding this comment.
For backward compatibility, this will need to have a default implementation. Please see #1890 (comment) for more details on the recommended implementation.
| @@ -45,6 +45,11 @@ public SecurityContext getContext() { | |||
| return ctx; | |||
| } | |||
|
|
|||
| @Override | |||
There was a problem hiding this comment.
Will you please update the copyright message for each edited file to include the year 2021?
| @@ -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>) | |||
There was a problem hiding this comment.
For each new public method that you introduce, will you please add @since 5.6 to the JavaDoc?
|
Thanks, @shirosaki! I've merged your contribution via 809ff88 and added some of the changes I specified in 78857c6. Note also that upon some deeper investigation, |
Fix #9841