feature #59682 [Security] Deprecate UserInterface & TokenInterface's eraseCredentials() (chalasr, nicolas-grekas)

This PR was merged into the 7.3 branch.

Discussion
----------

[Security] Deprecate UserInterface & TokenInterface's `eraseCredentials()`

| Q             | A
| ------------- | ---
| Branch?       | 7.3
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | yes
| Issues        | Fix #57842
| License       | MIT

As promised, this PR adds a commit on top of #59106 to improve the BC layer. This approach didn't fit in a review comment :) /cc `@chalasr`

This PR leverages the new `#[\Deprecated]` attribute to decide if some `eraseCredentials()` method is to be called or not.

My target DX here is to save us all (the community) from having to add `erase_credentials: false` configuration in all our apps.

So, instead of having to opt-out from the deprecation using this config option, the opt-out is done by adding the attribute on the method:

Before:
```php
public function eraseCredentials(): void
{
}
```

After:
```php
#[\Deprecated]
public function eraseCredentials(): void
{
}

// If your eraseCredentials() method was used to empty a "password" property:
public function __serialize(): array
{
    $data = (array) $this;
    unset($data["\0".self::class."\0password"]);

    return $data;
}
```

This should provide a smoother upgrade path (and maker-bundle could be updated right-away).

Commits
-------

e5c94e62283 [Security] Improve BC-layer to deprecate eraseCredentials methods
e5566065783 [Security] Deprecate `UserInterface` & `TokenInterface`'s `eraseCredentials()`

Author: nicolas-grekas

Authentication/AuthenticatorManager.php

@@ -14,6 +14,7 @@
 use Psr\Log\LoggerInterface;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Response;
+use Symfony\Component\Security\Core\Authentication\Token\AbstractToken;
 use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface;
 use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
 use Symfony\Component\Security\Core\AuthenticationEvents;
@@ -208,8 +209,8 @@ private function executeAuthenticator(AuthenticatorInterface $authenticator, Req
             // announce the authentication token
             $authenticatedToken = $this->eventDispatcher->dispatch(new AuthenticationTokenCreatedEvent($authenticatedToken, $passport))->getAuthenticatedToken();
 
-            if (true === $this->eraseCredentials) {
-                $authenticatedToken->eraseCredentials();
+            if ($this->eraseCredentials) {
+                self::checkEraseCredentials($authenticatedToken)?->eraseCredentials();
             }
 
             $this->eventDispatcher->dispatch(new AuthenticationSuccessEvent($authenticatedToken), AuthenticationEvents::AUTHENTICATION_SUCCESS);
@@ -287,4 +288,41 @@ private function isSensitiveException(AuthenticationException $exception): bool
 
         return false;
     }
+
+    /**
+     * @deprecated since Symfony 7.3
+     */
+    private static function checkEraseCredentials(TokenInterface|UserInterface|null $token): TokenInterface|UserInterface|null
+    {
+        if (!$token || !method_exists($token, 'eraseCredentials')) {
+            return null;
+        }
+
+        static $genericImplementations = [];
+        $m = null;
+
+        if (!isset($genericImplementations[$token::class])) {
+            $m = new \ReflectionMethod($token, 'eraseCredentials');
+            $genericImplementations[$token::class] = AbstractToken::class === $m->class;
+        }
+
+        if ($genericImplementations[$token::class]) {
+            return self::checkEraseCredentials($token->getUser());
+        }
+
+        static $deprecatedImplementations = [];
+
+        if (!isset($deprecatedImplementations[$token::class])) {
+            $m ??= new \ReflectionMethod($token, 'eraseCredentials');
+            $deprecatedImplementations[$token::class] = !$m->getAttributes(\Deprecated::class);
+        }
+
+        if ($deprecatedImplementations[$token::class]) {
+            trigger_deprecation('symfony/security-http', '7.3', 'Implementing "%s::eraseCredentials()" is deprecated since Symfony 7.3; add the #[\Deprecated] attribute on the method to signal its either empty or that you moved the logic elsewhere, typically to the "__serialize()" method.', get_debug_type($token));
+
+            return $token;
+        }
+
+        return null;
+    }
 }

Tests/Authentication/AuthenticatorManagerTest.php

@@ -15,9 +15,11 @@
 use PHPUnit\Framework\TestCase;
 use Psr\Log\AbstractLogger;
 use Psr\Log\LoggerInterface;
+use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait;
 use Symfony\Component\EventDispatcher\EventDispatcher;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Response;
+use Symfony\Component\Security\Core\Authentication\Token\AbstractToken;
 use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface;
 use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
 use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken;
@@ -40,6 +42,8 @@
 
 class AuthenticatorManagerTest extends TestCase
 {
+    use ExpectDeprecationTrait;
+
     private MockObject&TokenStorageInterface $tokenStorage;
     private EventDispatcher $eventDispatcher;
     private Request $request;
@@ -184,6 +188,8 @@ public function testAllRequiredBadgesPresent()
     }
 
     /**
+     * @group legacy
+     *
      * @dataProvider provideEraseCredentialsData
      */
     public function testEraseCredentials($eraseCredentials)
@@ -193,12 +199,25 @@ public function testEraseCredentials($eraseCredentials)
 
         $authenticator->expects($this->any())->method('authenticate')->willReturn(new SelfValidatingPassport(new UserBadge('wouter', fn () => $this->user)));
 
-        $authenticator->expects($this->any())->method('createToken')->willReturn($this->token);
+        $token = new class extends AbstractToken {
+            public $erased = false;
+
+            public function eraseCredentials(): void
+            {
+                $this->erased = true;
+            }
+        };
 
-        $this->token->expects($eraseCredentials ? $this->once() : $this->never())->method('eraseCredentials');
+        $authenticator->expects($this->any())->method('createToken')->willReturn($token);
+
+        if ($eraseCredentials) {
+            $this->expectDeprecation(\sprintf('Since symfony/security-http 7.3: Implementing "%s@anonymous::eraseCredentials()" is deprecated since Symfony 7.3; add the #[\Deprecated] attribute on the method to signal its either empty or that you moved the logic elsewhere, typically to the "__serialize()" method.', AbstractToken::class));
+        }
 
         $manager = $this->createManager([$authenticator], 'main', $eraseCredentials, exposeSecurityErrors: ExposeSecurityLevel::None);
         $manager->authenticateRequest($this->request);
+
+        $this->assertSame($eraseCredentials, $token->erased);
     }
 
     public static function provideEraseCredentialsData()
@@ -403,7 +422,7 @@ public function log($level, $message, array $context = []): void
             }
         };
 
-        $manager = $this->createManager([$authenticator], 'main', true, [], $logger, exposeSecurityErrors: ExposeSecurityLevel::None);
+        $manager = $this->createManager([$authenticator], 'main', false, [], $logger, exposeSecurityErrors: ExposeSecurityLevel::None);
         $response = $manager->authenticateRequest($this->request);
         $this->assertSame($this->response, $response);
         $this->assertStringContainsString($authenticator::class, $logger->logContexts[0]['authenticator']);
@@ -422,7 +441,7 @@ private static function createDummySupportsAuthenticator(?bool $supports = true)
         return new DummySupportsAuthenticator($supports);
     }
 
-    private function createManager($authenticators, $firewallName = 'main', $eraseCredentials = true, array $requiredBadges = [], ?LoggerInterface $logger = null, ExposeSecurityLevel $exposeSecurityErrors = ExposeSecurityLevel::AccountStatus)
+    private function createManager($authenticators, $firewallName = 'main', $eraseCredentials = false, array $requiredBadges = [], ?LoggerInterface $logger = null, ExposeSecurityLevel $exposeSecurityErrors = ExposeSecurityLevel::AccountStatus)
     {
         return new AuthenticatorManager($authenticators, $this->tokenStorage, $this->eventDispatcher, $firewallName, $logger, $eraseCredentials, $exposeSecurityErrors, $requiredBadges);
     }

Tests/EventListener/PasswordMigratingListenerTest.php

@@ -150,8 +150,6 @@ public function loadUserByUsername(string $username): UserInterface
 abstract class TestPasswordAuthenticatedUser implements UserInterface, PasswordAuthenticatedUserInterface
 {
     abstract public function getPassword(): ?string;
-
-    abstract public function getSalt(): ?string;
 }
 
 class DummyTestPasswordAuthenticatedUser extends TestPasswordAuthenticatedUser
@@ -161,16 +159,12 @@ public function getPassword(): ?string
         return null;
     }
 
-    public function getSalt(): ?string
-    {
-        return null;
-    }
-
     public function getRoles(): array
     {
         return [];
     }
 
+    #[\Deprecated]
     public function eraseCredentials(): void
     {
     }

Tests/Firewall/ContextListenerTest.php

@@ -563,21 +563,11 @@ public function __serialize(): array
         return [$this->user, $this->roles];
     }
 
-    public function serialize(): string
-    {
-        return serialize($this->__serialize());
-    }
-
     public function __unserialize(array $data): void
     {
         [$this->user, $this->roles] = $data;
     }
 
-    public function unserialize($serialized)
-    {
-        $this->__unserialize(\is_array($serialized) ? $serialized : unserialize($serialized));
-    }
-
     public function __toString(): string
     {
         return $this->user->getUserIdentifier();
@@ -603,6 +593,7 @@ public function getUserIdentifier(): string
         return $this->getUserIdentifier();
     }
 
+    #[\Deprecated]
     public function eraseCredentials(): void
     {
     }

Tests/LoginLink/LoginLinkHandlerTest.php

@@ -284,16 +284,12 @@ public function getPassword(): string
         return $this->passwordProperty;
     }
 
-    public function getSalt(): string
-    {
-        return '';
-    }
-
     public function getUserIdentifier(): string
     {
         return $this->username;
     }
 
+    #[\Deprecated]
     public function eraseCredentials(): void
     {
     }

composer.json

@@ -22,7 +22,7 @@
         "symfony/http-kernel": "^6.4|^7.0",
         "symfony/polyfill-mbstring": "~1.0",
         "symfony/property-access": "^6.4|^7.0",
-        "symfony/security-core": "^7.2",
+        "symfony/security-core": "^7.3",
         "symfony/service-contracts": "^2.5|^3"
     },
     "require-dev": {