-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Override the key to avoid CookieTheftException #5509
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
Override the key to avoid CookieTheftException #5509
Conversation
People may confused why those `token.keyhash()` not equals, for they forget to config the key twice. So, override the key for them, make it more convenient to custom RememberMeServices.
@XieEDeHeiShou Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
@XieEDeHeiShou Thank you for signing the Contributor License Agreement! |
Remove trailing whitespace to avoid travis CI failure.
The CI says it cannot get a jar from spring repository and then stopped with error:
I checked the file manually, exists. |
nothing else, just try trigger the CI.
There was a problem hiding this 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 @XieEDeHeiShou! I have left some comments inline.
Additionally, please provide a test for this change.
...ava/org/springframework/security/config/annotation/web/configurers/RememberMeConfigurer.java
Outdated
Show resolved
Hide resolved
...ava/org/springframework/security/config/annotation/web/configurers/RememberMeConfigurer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @XieEDeHeiShou, I have left one comment about the code formatting.
Also, please add an additional test for the changes to remember me.
...ava/org/springframework/security/config/annotation/web/configurers/RememberMeConfigurer.java
Outdated
Show resolved
Hide resolved
Thank you for the changes @XieEDeHeiShou. Please add a test for any changes in this PR. |
@eleftherias I'd like to provide test cases for these changes, and I do need some help, can you please guide me? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@XieEDeHeiShou The new test will look very similar to getWhenRememberMeCookieThenAuthenticationIsRememberMeAuthenticationToken
in the class RememberMeConfigurerTests
.
The assertions will be the same, and only the configuration will change.
In the configuration of the new test we should set a custom rememberMeServices
.rememberMe()
.rememberMeServices(rememberMeServices())
where rememberMeServices()
can be an instance of TokenBasedRememberMeServices
.
The key is that this new test should fail without the changes you have made, and pass when we add the changes from this PR.
I'm happy to provide any further guidance if you get stuck, or if this is unclear.
Additionally, I noticed an issue in the PR as it stands now, I left a comment inline.
...ava/org/springframework/security/config/annotation/web/configurers/RememberMeConfigurer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick turnaround again @XieEDeHeiShou. I have left a few more comments inline.
...ava/org/springframework/security/config/annotation/web/configurers/RememberMeConfigurer.java
Outdated
Show resolved
Hide resolved
...rg/springframework/security/config/annotation/web/configurers/RememberMeConfigurerTests.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more comment @XieEDeHeiShou
@@ -189,7 +191,8 @@ protected void configure(HttpSecurity http) throws Exception { | |||
@Bean | |||
public UserDetailsService userDetailsService() { | |||
return new InMemoryUserDetailsManager( | |||
User.withDefaultPasswordEncoder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change necessary? User.withDefaultPasswordEncoder
is deprecated, but it is acceptable to use in tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IDEA told me there is a warning when I commit this file, so I changed it. Should I revert this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please revert this change.
The IDE is correct to show a warning, but in this specific case it is allowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
This reverts commit 2f529e7
Thanks for the PR @XieEDeHeiShou! This is now merged into master. |
People may confused why those
token.keyhash()
not equals, for they forget to config the key twice.So, override the key for them, make it more convenient to custom RememberMeServices.
See #4140;