Skip to content

Add RememberMeDsl #9411

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 22, 2021
Merged

Add RememberMeDsl #9411

merged 1 commit into from
Feb 22, 2021

Conversation

IvanPavlov1995
Copy link
Contributor

Issue: gh-9319

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 6, 2021
@eleftherias eleftherias self-assigned this Feb 8, 2021
@eleftherias eleftherias added in: config An issue in spring-security-config type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Feb 8, 2021
eleftherias
eleftherias suggested changes Feb 10, 2021
View reviewed changes
Copy link
Contributor

@eleftherias eleftherias 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 @IvanPavlov1995!
I left some comments inline.

config/src/main/kotlin/org/springframework/security/config/web/servlet/RememberMeDsl.kt Outdated
authenticationSuccessHandler?.also { rememberMe.authenticationSuccessHandler(authenticationSuccessHandler) }
key?.also { rememberMe.key(key) }
rememberMeServices?.also { rememberMe.rememberMeServices(rememberMeServices) }
rememberMe.rememberMeParameter(rememberMeParameter)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should only set this value if the user sets it in the Kotlin DSL.
Otherwise, we will let the Java DSL determine the default.

Similarly, for rememberMeCookieName and alwaysRemember .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.
I have a question regarding also syntax. Is there a reason not to use function reference in such configurations? I mean following is shorter and with that I don't have to use !! operator:

useSecureCookie?.also(rememberMe::useSecureCookie)

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no particular reason. We could use function references instead. I will merge this PR as is, but feel free to create an additional PR if you think it's something we should change.

@eleftherias eleftherias merged commit 857830f into spring-projects:master Feb 22, 2021
@eleftherias eleftherias added this to the 5.5.0-M3 milestone Feb 22, 2021
@eleftherias eleftherias added the status: duplicate A duplicate of another issue label Feb 22, 2021
@IvanPavlov1995 IvanPavlov1995 deleted the gh-9319 branch June 15, 2024 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eleftherias eleftherias Awaiting requested review from eleftherias

Assignees

@eleftherias eleftherias

Labels
in: config An issue in spring-security-config status: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
None yet
Milestone
5.5.0-M3
Development

Successfully merging this pull request may close these issues.
3 participants
@IvanPavlov1995 @eleftherias @spring-projects-issues