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

Use SessionAuthenticationStrategy for Remember-Me authentication #15748

Merged

Conversation

xhaggi
Copy link
Contributor

@xhaggi xhaggi commented Sep 6, 2024

As described in gh-2253, the RememberMeAuthenticationFilter does not call the configured SessionAuthenticationStrategy if there is one, and therefore concurrent session control does not work properly.

This PR adapts the RememberMeAuthenticationFilter so that it calls the SessionAuthenticationStrategy on successful authentication as the AbstractAuthenticationProcessingFilter does. The shared SessionAuthenticationStrategy is then used in the RememberMeConfigurer to configure the filter like it is done in FormLoginConfigurer.

Let me know if anything is missing!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 6, 2024
Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, @xhaggi! Could you add a unit test that confirms the desired behavior (concurrency control) works, for example when the DSL configures a max number of sessions and the remember me filter exceeds that?

@jzheaux jzheaux self-assigned this Sep 17, 2024
@jzheaux jzheaux added type: enhancement A general enhancement status: waiting-for-feedback We need additional information before we can continue in: web An issue in web modules (web, webmvc) in: config An issue in spring-security-config and removed status: waiting-for-triage An issue we've not yet triaged in: web An issue in web modules (web, webmvc) labels Sep 17, 2024
@xhaggi xhaggi force-pushed the fix/rememeber-me-session-strategy branch 4 times, most recently from 5e86336 to a9147d0 Compare October 1, 2024 08:13
@xhaggi
Copy link
Contributor Author

xhaggi commented Oct 1, 2024

@jzheaux of course. I have added the unit test.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Oct 1, 2024
@xhaggi xhaggi requested a review from jzheaux October 1, 2024 08:17
@jzheaux jzheaux removed the status: feedback-provided Feedback has been provided label Oct 14, 2024
@jzheaux jzheaux added this to the 6.4.0-RC1 milestone Oct 14, 2024
@xhaggi xhaggi force-pushed the fix/rememeber-me-session-strategy branch 5 times, most recently from d3861e5 to 1717d59 Compare October 15, 2024 09:11
@xhaggi
Copy link
Contributor Author

xhaggi commented Oct 15, 2024

@jzheaux the build failed because formatting violations were found in the files I changed. I fixed it and force-pushed the changes.

@xhaggi
Copy link
Contributor Author

xhaggi commented Oct 15, 2024

BTW is there any chance that this will be backported to earlier versions?

@jzheaux jzheaux force-pushed the fix/rememeber-me-session-strategy branch from 1717d59 to f48a277 Compare October 15, 2024 17:50
@jzheaux
Copy link
Contributor

jzheaux commented Oct 15, 2024

Hi, @xhaggi, no, it's not likely, given that there is a behavioral change and a new public API. That said, what version were you hoping to backport it to and what is preventing you from upgrading?

@jzheaux jzheaux merged commit 7f53724 into spring-projects:main Oct 15, 2024
6 checks passed
@xhaggi
Copy link
Contributor Author

xhaggi commented Oct 16, 2024

what version were you hoping to backport

I was thinking of the last stable version 6.3.x.

what is preventing you from upgrading

Nothing, but we have to wait for the next stable 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: config An issue in spring-security-config type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants