[Security] Improve BC-layer to deprecate eraseCredentials methods

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;
@@ -69,10 +70,6 @@ public function __construct(
         }
 
         $this->exposeSecurityErrors = $exposeSecurityErrors;
-
-        if ($eraseCredentials) {
-            trigger_deprecation('symfony/security-http', '7.3', sprintf('Passing true as "$eraseCredentials" argument to "%s::__construct()" is deprecated and won\'t have any effect in 8.0, pass "false" instead and use your own erasing logic if needed.', __CLASS__));
-        }
     }
 
     /**
@@ -212,9 +209,8 @@ private function executeAuthenticator(AuthenticatorInterface $authenticator, Req
             // announce the authentication token
             $authenticatedToken = $this->eventDispatcher->dispatch(new AuthenticationTokenCreatedEvent($authenticatedToken, $passport))->getAuthenticatedToken();
 
-            // @deprecated since Symfony 7.3, remove the if statement in 8.0
-            if (true === $this->eraseCredentials) {
-                $authenticatedToken->eraseCredentials();
+            if ($this->eraseCredentials) {
+                self::checkEraseCredentials($authenticatedToken)?->eraseCredentials();
             }
 
             $this->eventDispatcher->dispatch(new AuthenticationSuccessEvent($authenticatedToken), AuthenticationEvents::AUTHENTICATION_SUCCESS);
@@ -292,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;
+    }
 }

CHANGELOG.md

@@ -6,8 +6,6 @@ CHANGELOG
 
  * Add encryption support to `OidcTokenHandler` (JWE)
  * Replace `$hideAccountStatusExceptions` argument with `$exposeSecurityErrors` in `AuthenticatorManager` constructor
- * Deprecate passing `true` for the `$eraseCredentials` parameter of `AuthenticatorManager::__construct()`, erase credentials
-  on your own e.g. upon `AuthenticationTokenCreatedEvent` instead. Passing it won't thave any effect in 8.0.
 
 7.2
 ---

Tests/Authentication/AuthenticatorManagerTest.php

@@ -19,6 +19,7 @@
 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;
@@ -166,7 +167,7 @@ public function testRequiredBadgeMissing()
 
         $authenticator->expects($this->once())->method('onAuthenticationFailure')->with($this->anything(), $this->callback(fn ($exception) => 'Authentication failed; Some badges marked as required by the firewall config are not available on the passport: "'.CsrfTokenBadge::class.'".' === $exception->getMessage()));
 
-        $manager = $this->createManager([$authenticator], 'main', false, [CsrfTokenBadge::class], exposeSecurityErrors: ExposeSecurityLevel::None);
+        $manager = $this->createManager([$authenticator], 'main', true, [CsrfTokenBadge::class], exposeSecurityErrors: ExposeSecurityLevel::None);
         $manager->authenticateRequest($this->request);
     }
 
@@ -182,12 +183,13 @@ public function testAllRequiredBadgesPresent()
 
         $authenticator->expects($this->once())->method('onAuthenticationSuccess');
 
-        $manager = $this->createManager([$authenticator], 'main', false, [CsrfTokenBadge::class], exposeSecurityErrors: ExposeSecurityLevel::None);
+        $manager = $this->createManager([$authenticator], 'main', true, [CsrfTokenBadge::class], exposeSecurityErrors: ExposeSecurityLevel::None);
         $manager->authenticateRequest($this->request);
     }
 
     /**
      * @group legacy
+     *
      * @dataProvider provideEraseCredentialsData
      */
     public function testEraseCredentials($eraseCredentials)
@@ -197,16 +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('Since symfony/security-http 7.3: Passing true as "$eraseCredentials" argument to "Symfony\Component\Security\Http\Authentication\AuthenticatorManager::__construct()" is deprecated and won\'t have any effect in 8.0, pass "false" instead and use your own erasing logic if needed.');
+            $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()

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
     {
     }