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

Replace SecurityContextHolder#addListener with SecurityContextHolder#setSecurityContextHolderStrategy #10226

Closed
jzheaux opened this issue Aug 26, 2021 · 0 comments · Fixed by #10246
Assignees
Labels
in: core An issue in spring-security-core type: enhancement A general enhancement
Milestone

Comments

@jzheaux
Copy link
Contributor

jzheaux commented Aug 26, 2021

Since listening for security context changes happens at the strategy level, SecurityContextHolder#addListener leaks that abstraction.

Instead, let's replace addListener with setSecurityContextHolderStrategy and make ListeningSecurityContextHolderStrategy public. This will allow the listening support to remain with the strategy.

ListeningSecurityContextHolderStrategy should not need to peek the SecurityContext since the semantic difference between a null SecurityContext and one with a null Authentication is negligible.

An additional benefit to setSecurityContextHolderStrategy over addListener is that it simplifies removing a custom strategy that has references to objects that need garbage collecting.

@jzheaux jzheaux added in: core An issue in spring-security-core type: enhancement A general enhancement labels Aug 26, 2021
@jzheaux jzheaux added this to the 5.6.0-M3 milestone Aug 26, 2021
@jzheaux jzheaux self-assigned this Aug 26, 2021
jzheaux added a commit to jzheaux/spring-security that referenced this issue Sep 9, 2021
jzheaux added a commit that referenced this issue Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core An issue in spring-security-core type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant