feature #40267 [Security] Decouple passwords from UserInterface (chalasr)

This PR was merged into the 5.3-dev branch.

Discussion
----------

[Security] Decouple passwords from UserInterface

| Q             | A
| ------------- | ---
| Branch?       | 5.x
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | yes
| Tickets       | #23081, helps with #39308
| License       | MIT
| Doc PR        | todo

This PR addresses a long-standing issue of the Security component: UserInterface is coupled to passwords.
It does it by moving the `getPassword()` method from `UserInterface` to a `PasswordAuthenticatedUserInterface`, and the `getSalt()` method to a `LegacyPasswordAuthenticatedUserInterface`.

Steps:
- In 5.3, we add the new interface and, at places where password-based authentication happens, trigger deprecation notices when a `UserInterface` object does not implement the new interface(s). The UserInterface is kept as-is until 6.0.
- In 6.0, we can remove the methods from `UserInterface` as well as support for using password authentication with user objects not implementing the new interface(s).

As a side-effect, some password-related interfaces (`UserPasswordHasherInterface` and `PasswordUpgraderInterface`) must change their signatures to type-hint against the new interface.
That is done in a BC way, which is to make the concerned methods virtual until 6.0, with deprecation notices triggered from callers and concrete implementations.

Benefits:
In 6.0, applications that use password-less authentication (e.g. login links) won't need to write no-op `getPassword()` and `getSalt()` in order to fulfil the `UserInterface` contract.

For applications that do use password-based authentication, they will need to opt-in explicitly by implementing the relevant interface(s).

This build on great discussions with @wouterj and @nicolas-grekas, and it is part of the overall rework of the Security component.

Commits
-------

2764225a38 [Security] Decouple passwords from UserInterface

Author: fabpot

Authentication/Provider/DaoAuthenticationProvider.php

@@ -11,16 +11,18 @@
 
 namespace Symfony\Component\Security\Core\Authentication\Provider;
 
+use Symfony\Component\PasswordHasher\Hasher\PasswordHasherFactoryInterface;
 use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken;
 use Symfony\Component\Security\Core\Encoder\EncoderFactoryInterface;
 use Symfony\Component\Security\Core\Exception\AuthenticationServiceException;
 use Symfony\Component\Security\Core\Exception\BadCredentialsException;
 use Symfony\Component\Security\Core\Exception\UsernameNotFoundException;
+use Symfony\Component\Security\Core\User\LegacyPasswordAuthenticatedUserInterface;
+use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
 use Symfony\Component\Security\Core\User\PasswordUpgraderInterface;
 use Symfony\Component\Security\Core\User\UserCheckerInterface;
 use Symfony\Component\Security\Core\User\UserInterface;
 use Symfony\Component\Security\Core\User\UserProviderInterface;
-use Symfony\Component\PasswordHasher\Hasher\PasswordHasherFactoryInterface;
 
 /**
  * DaoAuthenticationProvider uses a UserProviderInterface to retrieve the user
@@ -67,11 +69,20 @@ protected function checkAuthentication(UserInterface $user, UsernamePasswordToke
                 throw new BadCredentialsException('The presented password is invalid.');
             }
 
+            if (!$user instanceof PasswordAuthenticatedUserInterface) {
+                trigger_deprecation('symfony/security-core', '5.3', 'Using password-based authentication listeners while not implementing "%s" interface from class "%s" is deprecated.', PasswordAuthenticatedUserInterface::class, get_debug_type($user));
+            }
+
+            $salt = $user->getSalt();
+            if ($salt && !$user instanceof LegacyPasswordAuthenticatedUserInterface) {
+                trigger_deprecation('symfony/security-core', '5.3', 'Returning a string from "getSalt()" without implementing the "%s" interface is deprecated, the "%s" class should implement it.', LegacyPasswordAuthenticatedUserInterface::class, get_debug_type($user));
+            }
+
             // deprecated since Symfony 5.3
             if ($this->hasherFactory instanceof EncoderFactoryInterface) {
                 $encoder = $this->hasherFactory->getEncoder($user);
 
-                if (!$encoder->isPasswordValid($user->getPassword(), $presentedPassword, $user->getSalt())) {
+                if (!$encoder->isPasswordValid($user->getPassword(), $presentedPassword, $salt)) {
                     throw new BadCredentialsException('The presented password is invalid.');
                 }
 
@@ -84,12 +95,12 @@ protected function checkAuthentication(UserInterface $user, UsernamePasswordToke
 
             $hasher = $this->hasherFactory->getPasswordHasher($user);
 
-            if (!$hasher->verify($user->getPassword(), $presentedPassword, $user->getSalt())) {
+            if (!$hasher->verify($user->getPassword(), $presentedPassword, $salt)) {
                 throw new BadCredentialsException('The presented password is invalid.');
             }
 
             if ($this->userProvider instanceof PasswordUpgraderInterface && $hasher->needsRehash($user->getPassword())) {
-                $this->userProvider->upgradePassword($user, $hasher->hash($presentedPassword, $user->getSalt()));
+                $this->userProvider->upgradePassword($user, $hasher->hash($presentedPassword, $salt));
             }
         }
     }

Authentication/Token/AbstractToken.php

@@ -12,6 +12,7 @@
 namespace Symfony\Component\Security\Core\Authentication\Token;
 
 use Symfony\Component\Security\Core\User\EquatableInterface;
+use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
 use Symfony\Component\Security\Core\User\UserInterface;
 
 /**
@@ -262,10 +263,12 @@ private function hasUserChanged(UserInterface $user): bool
             return !(bool) $this->user->isEqualTo($user);
         }
 
+        // @deprecated since Symfony 5.3, check for PasswordAuthenticatedUserInterface on both user objects before comparing passwords
         if ($this->user->getPassword() !== $user->getPassword()) {
             return true;
         }
 
+        // @deprecated since Symfony 5.3, check for LegacyPasswordAuthenticatedUserInterface on both user objects before comparing salts
         if ($this->user->getSalt() !== $user->getSalt()) {
             return true;
         }

Encoder/UserPasswordEncoder.php

@@ -12,6 +12,8 @@
 namespace Symfony\Component\Security\Core\Encoder;
 
 use Symfony\Component\PasswordHasher\Hasher\UserPasswordHasher;
+use Symfony\Component\Security\Core\User\LegacyPasswordAuthenticatedUserInterface;
+use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
 use Symfony\Component\Security\Core\User\UserInterface;
 
 trigger_deprecation('symfony/security-core', '5.3', sprintf('The "%s" class is deprecated, use "%s" instead.', UserPasswordEncoder::class, UserPasswordHasher::class));
@@ -39,6 +41,15 @@ public function encodePassword(UserInterface $user, string $plainPassword)
     {
         $encoder = $this->encoderFactory->getEncoder($user);
 
+        if (!$user instanceof PasswordAuthenticatedUserInterface) {
+            trigger_deprecation('symfony/password-hasher', '5.3', 'Not implementing the "%s" interface while using "%s" is deprecated, the "%s" class should implement it.', PasswordAuthenticatedUserInterface::class, __CLASS__, get_debug_type($user));
+        }
+
+        $salt = $user->getSalt();
+        if ($salt && !$user instanceof LegacyPasswordAuthenticatedUserInterface) {
+            trigger_deprecation('symfony/password-hasher', '5.3', 'Returning a string from "getSalt()" without implementing the "%s" interface is deprecated, the "%s" class should implement it.', LegacyPasswordAuthenticatedUserInterface::class, get_debug_type($user));
+        }
+
         return $encoder->encodePassword($plainPassword, $user->getSalt());
     }
 

Tests/Authentication/Provider/DaoAuthenticationProviderTest.php

@@ -12,6 +12,9 @@
 namespace Symfony\Component\Security\Core\Tests\Authentication\Provider;
 
 use PHPUnit\Framework\TestCase;
+use Symfony\Component\PasswordHasher\Hasher\PasswordHasherFactoryInterface;
+use Symfony\Component\PasswordHasher\Hasher\PlaintextPasswordHasher;
+use Symfony\Component\PasswordHasher\PasswordHasherInterface;
 use Symfony\Component\Security\Core\Authentication\Provider\DaoAuthenticationProvider;
 use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken;
 use Symfony\Component\Security\Core\Encoder\EncoderFactoryInterface;
@@ -23,9 +26,6 @@
 use Symfony\Component\Security\Core\User\UserCheckerInterface;
 use Symfony\Component\Security\Core\User\UserInterface;
 use Symfony\Component\Security\Core\User\UserProviderInterface;
-use Symfony\Component\PasswordHasher\Hasher\PasswordHasherFactoryInterface;
-use Symfony\Component\PasswordHasher\Hasher\PlaintextPasswordHasher;
-use Symfony\Component\PasswordHasher\PasswordHasherInterface;
 
 class DaoAuthenticationProviderTest extends TestCase
 {
@@ -380,4 +380,5 @@ public function eraseCredentials()
 }
 interface PasswordUpgraderProvider extends UserProviderInterface, PasswordUpgraderInterface
 {
+    public function upgradePassword(UserInterface $user, string $newHashedPassword): void;
 }

Tests/User/ChainUserProviderTest.php

@@ -15,6 +15,7 @@
 use Symfony\Component\Security\Core\Exception\UnsupportedUserException;
 use Symfony\Component\Security\Core\Exception\UsernameNotFoundException;
 use Symfony\Component\Security\Core\User\ChainUserProvider;
+use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
 use Symfony\Component\Security\Core\User\PasswordUpgraderInterface;
 use Symfony\Component\Security\Core\User\User;
 use Symfony\Component\Security\Core\User\UserInterface;
@@ -251,14 +252,14 @@ public function testPasswordUpgrades()
     {
         $user = new User('user', 'pwd');
 
-        $provider1 = $this->createMock(PasswordUpgraderInterface::class);
+        $provider1 = $this->getMockForAbstractClass(MigratingProvider::class);
         $provider1
             ->expects($this->once())
             ->method('upgradePassword')
             ->willThrowException(new UnsupportedUserException('unsupported'))
         ;
 
-        $provider2 = $this->createMock(PasswordUpgraderInterface::class);
+        $provider2 = $this->getMockForAbstractClass(MigratingProvider::class);
         $provider2
             ->expects($this->once())
             ->method('upgradePassword')
@@ -269,3 +270,8 @@ public function testPasswordUpgrades()
         $provider->upgradePassword($user, 'foobar');
     }
 }
+
+abstract class MigratingProvider implements PasswordUpgraderInterface
+{
+    abstract public function upgradePassword(PasswordAuthenticatedUserInterface $user, string $newHashedPassword): void;
+}

User/ChainUserProvider.php

@@ -73,7 +73,7 @@ public function refreshUser(UserInterface $user)
 
         foreach ($this->providers as $provider) {
             try {
-                if (!$provider->supportsClass(\get_class($user))) {
+                if (!$provider->supportsClass(get_debug_type($user))) {
                     continue;
                 }
 
@@ -110,10 +110,16 @@ public function supportsClass(string $class)
     }
 
     /**
+     * @param PasswordAuthenticatedUserInterface $user
+     *
      * {@inheritdoc}
      */
-    public function upgradePassword(UserInterface $user, string $newEncodedPassword): void
+    public function upgradePassword($user, string $newEncodedPassword): void
     {
+        if (!$user instanceof PasswordAuthenticatedUserInterface) {
+            trigger_deprecation('symfony/security-core', '5.3', 'The "%s::upgradePassword()" method expects an instance of "%s" as first argument, the "%s" class should implement it.', PasswordUpgraderInterface::class, PasswordAuthenticatedUserInterface::class, get_debug_type($user));
+        }
+
         foreach ($this->providers as $provider) {
             if ($provider instanceof PasswordUpgraderInterface) {
                 try {

User/LegacyPasswordAuthenticatedUserInterface.php

@@ -0,0 +1,28 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <[email protected]>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Security\Core\User;
+
+/**
+ * For users that can be authenticated using a password/salt couple.
+ *
+ * Once all password hashes have been upgraded to a modern algorithm via password migrations,
+ * implement {@see PasswordAuthenticatedUserInterface} instead.
+ *
+ * @author Robin Chalas <[email protected]>
+ */
+interface LegacyPasswordAuthenticatedUserInterface extends PasswordAuthenticatedUserInterface
+{
+    /**
+     * Returns the salt that was originally used to hash the password.
+     */
+    public function getSalt(): ?string;
+}

User/PasswordAuthenticatedUserInterface.php

@@ -0,0 +1,30 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <[email protected]>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Security\Core\User;
+
+/**
+ * For users that can be authenticated using a password.
+ *
+ * @author Robin Chalas <[email protected]>
+ * @author Wouter de Jong <[email protected]>
+ */
+interface PasswordAuthenticatedUserInterface
+{
+    /**
+     * Returns the hashed password used to authenticate the user.
+     *
+     * Usually on authentication, a plain-text password will be compared to this value.
+     *
+     * @return string|null The hashed password or null (if not set or erased)
+     */
+    public function getPassword(): ?string;
+}

User/PasswordUpgraderInterface.php

@@ -13,15 +13,12 @@
 
 /**
  * @author Nicolas Grekas <[email protected]>
+ *
+ * @method void upgradePassword(PasswordAuthenticatedUserInterface|UserInterface $user, string $newHashedPassword) Upgrades the hashed password of a user, typically for using a better hash algorithm.
+ *                                                                                                                 This method should persist the new password in the user storage and update the $user object accordingly.
+ *                                                                                                                 Because you don't want your users not being able to log in, this method should be opportunistic:
+ *                                                                                                                 it's fine if it does nothing or if it fails without throwing any exception.
  */
 interface PasswordUpgraderInterface
 {
-    /**
-     * Upgrades the hashed password of a user, typically for using a better hash algorithm.
-     *
-     * This method should persist the new password in the user storage and update the $user object accordingly.
-     * Because you don't want your users not being able to log in, this method should be opportunistic:
-     * it's fine if it does nothing or if it fails without throwing any exception.
-     */
-    public function upgradePassword(UserInterface $user, string $newHashedPassword): void;
 }

User/User.php

@@ -18,7 +18,7 @@
  *
  * @author Fabien Potencier <[email protected]>
  */
-final class User implements UserInterface, EquatableInterface
+final class User implements UserInterface, PasswordAuthenticatedUserInterface, EquatableInterface
 {
     private $username;
     private $password;