[Security] Allow RememberMeHandler to use a custom RememberMeDetails class #44459
Conversation
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
src/Symfony/Component/Security/Http/RememberMe/RememberMeHandlerInterface.php
Outdated
Show resolved
Hide resolved
|
I've added some tests. This includes 2 tests which test the deprecated code paths still work, but this does naturally result in self deprecation notices even when the tests succeed. Considering the PR checks seem to fail because of this it seems like the tests are not supposed to cause any of these. As this is the first time I'm making a PR I'm not quite sure what the policy on tests like these are. Am I supposed to test these a different way, should deprecated code paths simply not be tested, or can these failures in this case be ignored? |
These tests have to be in the legacy group (deprecations in legacy tests are ignored): In these legacy tests, you can also use the |
There was a problem hiding this comment.
Thanks for the PR - it's looking very complete for a first contribution!
I'm a bit unsure about the getUserIdentifierForCookie() method and whether it should be moved to a separate interface. But as 99% of the remember me handlers extend from AbstractRememberMeHandler, which has the default behavior, it's probably OK.
src/Symfony/Component/Security/Http/Authenticator/RememberMeAuthenticator.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Http/Authenticator/RememberMeAuthenticator.php
Outdated
Show resolved
Hide resolved
…erMeDetails implementation
9804417 to
99b49d0
Compare
|
I rebased the PR to 6.2 and solved the resulting merge conflicts. There are some failing tests, but it seems like those are unrelated to the changes of this PR. No new feedback has come since the last feedback was implemented. Is there anything else needed from my side to get this merged? |
|
@wouterj I'll wait for the 6.3 window and rebase, but is there anything I still need to do aside from that? Like I mentioned previously, all the feedback I got has already been implemented. |
When writing a custom
RememberMeHandlerit is not possible to change anything about what is stored in the cookie or how. Not every implementation would for example need to include the user class or identifier in the cookie or may not even want to. Maybe you might want to add some other value to it, in which case concatenating it to the$valueparameter would seem like more of a dirty workaround.This PR adds support for the
RememberMeAuthenticatorandRememberMeHandlerto use a differentRememberMeDetailsclass when needed.Currently I have not yet added an interface for
RememberMeDetailsto implement so it would be necessary to extend the class directly. This is because I'm thinking of making an RFC first for changing theRememberMeDetailsimplementation for the different available strategies.