[Security] Allow RememberMeHandler to use a custom RememberMeDetails class

[tyrelcher]

Q	A
Branch?	6.1
Bug fix?	no
New feature?	yes
Deprecations?	yes
Tickets	Fix #44168
License	MIT
Doc PR	-

When writing a custom RememberMeHandler it 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 $value parameter would seem like more of a dirty workaround.

This PR adds support for the RememberMeAuthenticator and RememberMeHandler to use a different RememberMeDetails class when needed.

Currently I have not yet added an interface for RememberMeDetails to implement so it would be necessary to extend the class directly. This is because I'm thinking of making an RFC first for changing the RememberMeDetails implementation for the different available strategies.

[carsonbot]

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:

• Always add tests
• Keep backward compatibility (see https://symfony.com/bc).
• Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
• Features and deprecations must be submitted against the 6.1 branch.

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!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

[chalasr]

Thanks for the PR. Here are some random comments. Don't forget to add tests 😉

[tyrelcher]

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?

[wouterj]

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):

/**
 * @group legacy
 */
public function testSomeDeprecatedBehavior()
{
}

In these legacy tests, you can also use the ExpectDeprecationTrait to test that the deprecations are triggered correctly (docs).

[wouterj]

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.

[tyrelcher]

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?

[tyrelcher]

@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.