feature #40267 [Security] Decouple passwords from UserInterface (chal…

…asr)

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

2764225 [Security] Decouple passwords from UserInterface

UPGRADE-5.3.md

@@ -80,6 +80,88 @@ Routing
 Security
 --------
 
+ * Deprecate `UserInterface::getPassword()`
+   If your `getPassword()` method does not return `null` (i.e. you are using password-based authentication),
+   you should implement `PasswordAuthenticatedUserInterface`.
+
+   Before:
+   ```php
+   use Symfony\Component\Security\Core\User\UserInterface;
+
+   class User implements UserInterface
+   {
+       // ...
+
+       public function getPassword()
+       {
+           return $this->password;
+       }
+   }
+   ```
+
+   After:
+   ```php
+   use Symfony\Component\Security\Core\User\UserInterface;
+   use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
+
+   class User implements UserInterface, PasswordAuthenticatedUserInterface
+   {
+       // ...
+
+       public function getPassword(): ?string
+       {
+           return $this->password;
+       }
+   }
+   ```
+
+ * Deprecate `UserInterface::getSalt()`
+   If your `getSalt()` method does not return `null` (i.e. you are using password-based authentication with an old password hash algorithm that requires user-provided salts),
+   implement `LegacyPasswordAuthenticatedUserInterface`.
+
+   Before:
+   ```php
+   use Symfony\Component\Security\Core\User\UserInterface;
+
+   class User implements UserInterface
+   {
+       // ...
+
+       public function getPassword()
+       {
+           return $this->password;
+       }
+
+       public function getSalt()
+       {
+           return $this->salt;
+       }
+   }
+   ```
+
+   After:
+   ```php
+   use Symfony\Component\Security\Core\User\UserInterface;
+   use Symfony\Component\Security\Core\User\LegacyPasswordAuthenticatedUserInterface;
+
+   class User implements UserInterface, LegacyPasswordAuthenticatedUserInterface
+   {
+       // ...
+
+       public function getPassword(): ?string
+       {
+           return $this->password;
+       }
+
+       public function getSalt(): ?string
+       {
+           return $this->salt;
+       }
+   }
+   ```
+
+ * Deprecate calling `PasswordUpgraderInterface::upgradePassword()` with a `UserInterface` instance that does not implement `PasswordAuthenticatedUserInterface`
+ * Deprecate calling methods `hashPassword()`, `isPasswordValid()` and `needsRehash()` on `UserPasswordHasherInterface` with a `UserInterface` instance that does not implement `PasswordAuthenticatedUserInterface`
  * Deprecate all classes in the `Core\Encoder\`  sub-namespace, use the `PasswordHasher` component instead
  * Deprecated voters that do not return a valid decision when calling the `vote` method
 

UPGRADE-6.0.md

@@ -172,6 +172,90 @@ Routing
 Security
 --------
 
+ * Remove `UserInterface::getPassword()`
+   If your `getPassword()` method does not return `null` (i.e. you are using password-based authentication),
+   you should implement `PasswordAuthenticatedUserInterface`.
+
+   Before:
+   ```php
+   use Symfony\Component\Security\Core\User\UserInterface;
+
+   class User implements UserInterface
+   {
+       // ...
+
+       public function getPassword()
+       {
+           return $this->password;
+       }
+   }
+   ```
+
+   After:
+   ```php
+   use Symfony\Component\Security\Core\User\UserInterface;
+   use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
+
+   class User implements UserInterface, PasswordAuthenticatedUserInterface
+   {
+       // ...
+
+       public function getPassword(): ?string
+       {
+           return $this->password;
+       }
+   }
+   ```
+
+ * Remove `UserInterface::getSalt()`
+   If your `getSalt()` method does not return `null` (i.e. you are using password-based authentication with an old password hash algorithm that requires user-provided salts),
+   implement `LegacyPasswordAuthenticatedUserInterface`.
+
+   Before:
+   ```php
+   use Symfony\Component\Security\Core\User\UserInterface;
+
+   class User implements UserInterface
+   {
+       // ...
+
+       public function getPassword()
+       {
+           return $this->password;
+       }
+
+       public function getSalt()
+       {
+           return $this->salt;
+       }
+   }
+   ```
+
+   After:
+   ```php
+   use Symfony\Component\Security\Core\User\UserInterface;
+   use Symfony\Component\Security\Core\User\LegacyPasswordAuthenticatedUserInterface;
+
+   class User implements UserInterface, LegacyPasswordAuthenticatedUserInterface
+   {
+       // ...
+
+       public function getPassword(): ?string
+       {
+           return $this->password;
+       }
+
+       public function getSalt(): ?string
+       {
+           return $this->salt;
+       }
+   }
+   ```
+
+ * Calling `PasswordUpgraderInterface::upgradePassword()` with a `UserInterface` instance that
+   does not implement `PasswordAuthenticatedUserInterface` now throws a `\TypeError`.
+ * Calling methods `hashPassword()`, `isPasswordValid()` and `needsRehash()` on `UserPasswordHasherInterface`
+   with a `UserInterface` instance that does not implement `PasswordAuthenticatedUserInterface` now throws a `\TypeError`
  * Drop all classes in the `Core\Encoder\`  sub-namespace, use the `PasswordHasher` component instead
  * Drop support for `SessionInterface $session` as constructor argument of `SessionTokenStorage`, inject a `\Symfony\Component\HttpFoundation\RequestStack $requestStack` instead
  * Drop support for `session` provided by the ServiceLocator injected in `UsageTrackingTokenStorage`, provide a `request_stack` service instead

src/Symfony/Bridge/Doctrine/Security/User/EntityUserProvider.php

@@ -17,6 +17,7 @@
 use Doctrine\Persistence\ObjectRepository;
 use Symfony\Component\Security\Core\Exception\UnsupportedUserException;
 use Symfony\Component\Security\Core\Exception\UsernameNotFoundException;
+use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
 use Symfony\Component\Security\Core\User\PasswordUpgraderInterface;
 use Symfony\Component\Security\Core\User\UserInterface;
 use Symfony\Component\Security\Core\User\UserProviderInterface;
@@ -115,9 +116,15 @@ public function supportsClass(string $class)
 
     /**
      * {@inheritdoc}
+     *
+     * @final
      */
     public function upgradePassword(UserInterface $user, string $newEncodedPassword): void
     {
+        if (!$user instanceof PasswordAuthenticatedUserInterface) {
+            trigger_deprecation('symfony/doctrine-bridge', '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));
+        }
+
         $class = $this->getClass();
         if (!$user instanceof $class) {
             throw new UnsupportedUserException(sprintf('Instances of "%s" are not supported.', get_debug_type($user)));

src/Symfony/Bridge/Doctrine/Tests/Fixtures/User.php

@@ -14,10 +14,11 @@
 use Doctrine\ORM\Mapping\Column;
 use Doctrine\ORM\Mapping\Entity;
 use Doctrine\ORM\Mapping\Id;
+use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
 use Symfony\Component\Security\Core\User\UserInterface;
 
 /** @Entity */
-class User implements UserInterface
+class User implements UserInterface, PasswordAuthenticatedUserInterface
 {
     /** @Id @Column(type="integer") */
     protected $id1;

src/Symfony/Bridge/Doctrine/Tests/Security/User/EntityUserProviderTest.php

@@ -22,6 +22,7 @@
 use Symfony\Bridge\Doctrine\Tests\DoctrineTestHelper;
 use Symfony\Bridge\Doctrine\Tests\Fixtures\User;
 use Symfony\Component\Security\Core\Exception\UsernameNotFoundException;
+use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
 use Symfony\Component\Security\Core\User\PasswordUpgraderInterface;
 use Symfony\Component\Security\Core\User\UserInterface;
 
@@ -234,4 +235,5 @@ abstract class UserLoaderRepository implements ObjectRepository, UserLoaderInter
 
 abstract class PasswordUpgraderRepository implements ObjectRepository, PasswordUpgraderInterface
 {
+    abstract public function upgradePassword(PasswordAuthenticatedUserInterface $user, string $newHashedPassword): void;
 }

src/Symfony/Bridge/Doctrine/composer.json

@@ -38,7 +38,7 @@
         "symfony/property-access": "^4.4|^5.0",
         "symfony/property-info": "^5.0",
         "symfony/proxy-manager-bridge": "^4.4|^5.0",
-        "symfony/security-core": "^5.0",
+        "symfony/security-core": "^5.3",
         "symfony/expression-language": "^4.4|^5.0",
         "symfony/uid": "^5.1",
         "symfony/validator": "^5.2",
@@ -60,7 +60,7 @@
         "symfony/messenger": "<4.4",
         "symfony/property-info": "<5",
         "symfony/security-bundle": "<5",
-        "symfony/security-core": "<5",
+        "symfony/security-core": "<5.3",
         "symfony/validator": "<5.2"
     },
     "suggest": {

src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php

@@ -42,6 +42,7 @@
 use Symfony\Component\Security\Core\Encoder\NativePasswordEncoder;
 use Symfony\Component\Security\Core\Encoder\SodiumPasswordEncoder;
 use Symfony\Component\Security\Core\User\ChainUserProvider;
+use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
 use Symfony\Component\Security\Core\User\UserProviderInterface;
 use Symfony\Component\Security\Http\Event\CheckPassportEvent;
 
@@ -664,6 +665,9 @@ private function createEncoders(array $encoders, ContainerBuilder $container)
     {
         $encoderMap = [];
         foreach ($encoders as $class => $encoder) {
+            if (class_exists($class) && !is_a($class, PasswordAuthenticatedUserInterface::class, true)) {
+                trigger_deprecation('symfony/security-bundle', '5.3', 'Configuring an encoder for a user class that does not implement "%s" is deprecated, class "%s" should implement it.', PasswordAuthenticatedUserInterface::class, $class);
+            }
             $encoderMap[$class] = $this->createEncoder($encoder);
         }
 
@@ -775,6 +779,10 @@ private function createHashers(array $hashers, ContainerBuilder $container)
     {
         $hasherMap = [];
         foreach ($hashers as $class => $hasher) {
+            // @deprecated since Symfony 5.3, remove the check in 6.0
+            if (class_exists($class) && !is_a($class, PasswordAuthenticatedUserInterface::class, true)) {
+                trigger_deprecation('symfony/security-bundle', '5.3', 'Configuring a password hasher for a user class that does not implement "%s" is deprecated, class "%s" should implement it.', PasswordAuthenticatedUserInterface::class, $class);
+            }
             $hasherMap[$class] = $this->createHasher($hasher);
         }
 

src/Symfony/Bundle/SecurityBundle/Tests/Functional/SecurityTest.php

@@ -13,6 +13,7 @@
 
 use Symfony\Bundle\SecurityBundle\Tests\Functional\Bundle\SecuredPageBundle\Security\Core\User\ArrayUserProvider;
 use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken;
+use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
 use Symfony\Component\Security\Core\User\User;
 use Symfony\Component\Security\Core\User\UserInterface;
 
@@ -78,7 +79,7 @@ public function testUserWillBeMarkedAsChangedIfRolesHasChanged(UserInterface $us
     }
 }
 
-final class UserWithoutEquatable implements UserInterface
+final class UserWithoutEquatable implements UserInterface, PasswordAuthenticatedUserInterface
 {
     private $username;
     private $password;
@@ -119,7 +120,7 @@ public function getRoles()
     /**
      * {@inheritdoc}
      */
-    public function getPassword()
+    public function getPassword(): ?string
     {
         return $this->password;
     }

src/Symfony/Component/Ldap/Security/CheckLdapCredentialsListener.php

@@ -17,6 +17,7 @@
 use Symfony\Component\Ldap\LdapInterface;
 use Symfony\Component\Security\Core\Exception\BadCredentialsException;
 use Symfony\Component\Security\Core\Exception\LogicException;
+use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
 use Symfony\Component\Security\Http\Authenticator\Passport\Credentials\PasswordCredentials;
 use Symfony\Component\Security\Http\Authenticator\Passport\UserPassportInterface;
 use Symfony\Component\Security\Http\Event\CheckPassportEvent;
@@ -68,6 +69,11 @@ public function onCheckPassport(CheckPassportEvent $event)
             throw new BadCredentialsException('The presented password cannot be empty.');
         }
 
+        $user = $passport->getUser();
+        if (!$user instanceof PasswordAuthenticatedUserInterface) {
+            trigger_deprecation('symfony/ldap', '5.3', 'Not implementing the "%s" interface in class "%s" while using password-based authenticators is deprecated.', PasswordAuthenticatedUserInterface::class, get_debug_type($user));
+        }
+
         /** @var LdapInterface $ldap */
         $ldap = $this->ldapLocator->get($ldapBadge->getLdapServiceId());
         try {
@@ -77,7 +83,7 @@ public function onCheckPassport(CheckPassportEvent $event)
                 } else {
                     throw new LogicException('Using the "query_string" config without using a "search_dn" and a "search_password" is not supported.');
                 }
-                $username = $ldap->escape($passport->getUser()->getUsername(), '', LdapInterface::ESCAPE_FILTER);
+                $username = $ldap->escape($user->getUsername(), '', LdapInterface::ESCAPE_FILTER);
                 $query = str_replace('{username}', $username, $ldapBadge->getQueryString());
                 $result = $ldap->query($ldapBadge->getDnString(), $query)->execute();
                 if (1 !== $result->count()) {
@@ -86,7 +92,7 @@ public function onCheckPassport(CheckPassportEvent $event)
 
                 $dn = $result[0]->getDn();
             } else {
-                $username = $ldap->escape($passport->getUser()->getUsername(), '', LdapInterface::ESCAPE_DN);
+                $username = $ldap->escape($user->getUsername(), '', LdapInterface::ESCAPE_DN);
                 $dn = str_replace('{username}', $username, $ldapBadge->getDnString());
             }
 

src/Symfony/Component/Ldap/Security/LdapUser.php

@@ -13,14 +13,15 @@
 
 use Symfony\Component\Ldap\Entry;
 use Symfony\Component\Security\Core\User\EquatableInterface;
+use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
 use Symfony\Component\Security\Core\User\UserInterface;
 
 /**
  * @author Robin Chalas <robin.chalas@gmail.com>
  *
  * @final
  */
-class LdapUser implements UserInterface, EquatableInterface
+class LdapUser implements UserInterface, PasswordAuthenticatedUserInterface, EquatableInterface
 {
     private $entry;
     private $username;

src/Symfony/Component/Ldap/Security/LdapUserProvider.php

@@ -122,6 +122,8 @@ public function refreshUser(UserInterface $user)
 
     /**
      * {@inheritdoc}
+     *
+     * @final
      */
     public function upgradePassword(UserInterface $user, string $newEncodedPassword): void
     {

src/Symfony/Component/Ldap/composer.json

@@ -22,11 +22,11 @@
         "ext-ldap": "*"
     },
     "require-dev": {
-        "symfony/security-core": "^5.0"
+        "symfony/security-core": "^5.3"
     },
     "conflict": {
         "symfony/options-resolver": "<4.4",
-        "symfony/security-core": "<5"
+        "symfony/security-core": "<5.3"
     },
     "autoload": {
         "psr-4": { "Symfony\\Component\\Ldap\\": "" },

src/Symfony/Component/PasswordHasher/Hasher/PasswordHasherFactoryInterface.php

@@ -12,6 +12,7 @@
 namespace Symfony\Component\PasswordHasher\Hasher;
 
 use Symfony\Component\PasswordHasher\PasswordHasherInterface;
+use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
 use Symfony\Component\Security\Core\User\UserInterface;
 
 /**
@@ -25,7 +26,7 @@ interface PasswordHasherFactoryInterface
     /**
      * Returns the password hasher to use for the given user.
      *
-     * @param UserInterface|string $user A UserInterface instance or a class name
+     * @param PasswordAuthenticatedUserInterface|UserInterface|string $user A PasswordAuthenticatedUserInterface/UserInterface instance or a class name
      *
      * @throws \RuntimeException When no password hasher could be found for the user
      */