feature #40403 [Security] Rename UserInterface::getUsername() to getU…

…serIdentifier() (wouterj)

This PR was squashed before being merged into the 5.3-dev branch.

Discussion
----------

[Security] Rename UserInterface::getUsername() to getUserIdentifier()

| Q             | A
| ------------- | ---
| Branch?       | 5.x
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | yes
| Tickets       | Fix part of #39308
| License       | MIT
| Doc PR        | tbd

This continues the great work by @chalasr in #40267 and rounds up the `UserInterface` cleanup.

The `getUsername()` has been a point of confusion for many years. In today's applications, many domains no longer have a username, but instead rely on a user's email address or the like. Even more, this username has to be unique for all Security functionality to work correctly - so it's more confusing in complex applications relying on e.g. "username+company" to be unique.

**This PR proposes to rename the method to `getUserIdentifier()`**, to more clearly indicate the goal of the method (note: I'm open for any other suggestion).

For BC, the `getUsername()` method is deprecated in 5.3 and `getUserIdentifier()` will be added to the `UserInterface` in 6.0. PHPdocs are used to improve type-hinting for 5.3 & 5.4. Both authentication managers (the legacy and experimental one) check the authenticated user object and trigger a deprecation if `getUserIdentifier()` is not implemented.
* [x] **judge whether we need to have better deprecations for calling `getUsername()` in the application code.**

Consistent with these changes, I've also done the same BC change for `TokenInterface::getUsername()`.
* [x] **do the same for remember me's `PersistentTokenInterface::getUsername()`**
* [x] **also rename `UserProviderInterface::loadUserByUsername()` & `UsernameNotFoundException`**
* [x] **also rename `UserLoaderInterface::loadUserByUsername()`**

I'm very much looking forward for feedback and help for this important, but BC-heavy, rename. 😃

Commits
-------

8afd7a3 [Security] Rename UserInterface::getUsername() to getUserIdentifier()

UPGRADE-5.3.md

@@ -9,6 +9,7 @@ Asset
 DoctrineBridge
 --------------
 
+ * Deprecate `UserLoaderInterface::loadUserByUsername()` in favor of `UserLoaderInterface::loadUserByIdentifier()
  * Remove `UuidV*Generator` classes
 
 DomCrawler
@@ -178,6 +179,11 @@ Security
    }
    ```
 
+ * Deprecate `UserInterface::getUsername()` in favor of `UserInterface::getUserIdentifier()`
+ * Deprecate `TokenInterface::getUsername()` in favor of `TokenInterface::getUserIdentifier()`
+ * Deprecate `UserProviderInterface::loadUserByUsername()` in favor of `UserProviderInterface::loadUserByIdentifier()`
+ * Deprecate `UsernameNotFoundException` in favor of `UserNotFoundException` and `getUsername()`/`setUsername()` in favor of `getUserIdentifier()`/`setUserIdentifier()`
+ * Deprecate `PersistentTokenInterface::getUsername()` in favor of `PersistentTokenInterface::getUserIdentifier()`
  * 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

UPGRADE-6.0.md

@@ -6,6 +6,11 @@ Asset
 
  * Removed `RemoteJsonManifestVersionStrategy`, use `JsonManifestVersionStrategy` instead.
 
+DoctrineBridge
+--------------
+
+ * Remove `UserLoaderInterface::loadUserByUsername()` in favor of `UserLoaderInterface::loadUserByIdentifier()
+
 Config
 ------
 
@@ -262,6 +267,11 @@ Security
    }
    ```
 
+ * Remove `UserInterface::getUsername()` in favor of `UserInterface::getUserIdentifier()`
+ * Remove `TokenInterface::getUsername()` in favor of `TokenInterface::getUserIdentifier()`
+ * Remove `UserProviderInterface::loadUserByUsername()` in favor of `UserProviderInterface::loadUserByIdentifier()`
+ * Remove `UsernameNotFoundException` in favor of `UserNotFoundException` and `getUsername()`/`setUsername()` in favor of `getUserIdentifier()`/`setUserIdentifier()`
+ * Remove `PersistentTokenInterface::getUsername()` in favor of `PersistentTokenInterface::getUserIdentifier()`
  * Calling `PasswordUpgraderInterface::upgradePassword()` with a `UserInterface` instance that
    does not implement `PasswordAuthenticatedUserInterface` now throws a `\TypeError`.
  * Calling methods `hashPassword()`, `isPasswordValid()` and `needsRehash()` on `UserPasswordHasherInterface`

src/Symfony/Bridge/Doctrine/CHANGELOG.md

@@ -4,6 +4,7 @@ CHANGELOG
 5.3
 ---
 
+ * Deprecate `UserLoaderInterface::loadUserByUsername()` in favor of `UserLoaderInterface::loadUserByIdentifier()
  * Deprecate `DoctrineTestHelper` and `TestRepositoryFactory`
  * [BC BREAK] Remove `UuidV*Generator` classes
  * Add `UuidGenerator`

src/Symfony/Bridge/Doctrine/Security/RememberMe/DoctrineTokenProvider.php

@@ -119,7 +119,8 @@ public function createNewToken(PersistentTokenInterface $token)
             .' VALUES (:class, :username, :series, :value, :lastUsed)';
         $paramValues = [
             'class' => $token->getClass(),
-            'username' => $token->getUsername(),
+            // @deprecated since 5.3, change to $token->getUserIdentifier() in 6.0
+            'username' => method_exists($token, 'getUserIdentifier') ? $token->getUserIdentifier() : $token->getUsername(),
             'series' => $token->getSeries(),
             'value' => $token->getTokenValue(),
             'lastUsed' => $token->getLastUsed(),

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

@@ -16,7 +16,7 @@
 use Doctrine\Persistence\ObjectManager;
 use Doctrine\Persistence\ObjectRepository;
 use Symfony\Component\Security\Core\Exception\UnsupportedUserException;
-use Symfony\Component\Security\Core\Exception\UsernameNotFoundException;
+use Symfony\Component\Security\Core\Exception\UserNotFoundException;
 use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
 use Symfony\Component\Security\Core\User\PasswordUpgraderInterface;
 use Symfony\Component\Security\Core\User\UserInterface;
@@ -50,21 +50,35 @@ public function __construct(ManagerRegistry $registry, string $classOrAlias, str
      * {@inheritdoc}
      */
     public function loadUserByUsername(string $username)
+    {
+        trigger_deprecation('symfony/doctrine-bridge', '5.3', 'Method "%s()" is deprecated, use loadUserByIdentifier() instead.', __METHOD__);
+
+        return $this->loadUserByIdentifier($username);
+    }
+
+    public function loadUserByIdentifier(string $identifier): UserInterface
     {
         $repository = $this->getRepository();
         if (null !== $this->property) {
-            $user = $repository->findOneBy([$this->property => $username]);
+            $user = $repository->findOneBy([$this->property => $identifier]);
         } else {
             if (!$repository instanceof UserLoaderInterface) {
                 throw new \InvalidArgumentException(sprintf('You must either make the "%s" entity Doctrine Repository ("%s") implement "Symfony\Bridge\Doctrine\Security\User\UserLoaderInterface" or set the "property" option in the corresponding entity provider configuration.', $this->classOrAlias, get_debug_type($repository)));
             }
 
-            $user = $repository->loadUserByUsername($username);
+            // @deprecated since 5.3, change to $repository->loadUserByIdentifier() in 6.0
+            if (method_exists($repository, 'loadUserByIdentifier')) {
+                $user = $repository->loadUserByIdentifier($identifier);
+            } else {
+                trigger_deprecation('symfony/doctrine-bridge', '5.3', 'Not implementing method "loadUserByIdentifier()" in user loader "%s" is deprecated. This method will replace "loadUserByUsername()" in Symfony 6.0.', get_debug_type($repository));
+
+                $user = $repository->loadUserByUsername($identifier);
+            }
         }
 
         if (null === $user) {
-            $e = new UsernameNotFoundException(sprintf('User "%s" not found.', $username));
-            $e->setUsername($username);
+            $e = new UserNotFoundException(sprintf('User "%s" not found.', $identifier));
+            $e->setUserIdentifier($identifier);
 
             throw $e;
         }
@@ -96,8 +110,8 @@ public function refreshUser(UserInterface $user)
 
             $refreshedUser = $repository->find($id);
             if (null === $refreshedUser) {
-                $e = new UsernameNotFoundException('User with id '.json_encode($id).' not found.');
-                $e->setUsername(json_encode($id));
+                $e = new UserNotFoundException('User with id '.json_encode($id).' not found.');
+                $e->setUserIdentifier(json_encode($id));
 
                 throw $e;
             }

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

@@ -22,16 +22,11 @@
  *
  * @see UserInterface
  *
+ * @method UserInterface|null loadUserByIdentifier(string $identifier) loads the user for the given user identifier (e.g. username or email).
+ *                                                                     This method must return null if the user is not found.
+ *
  * @author Michal Trojanowski <michal@kmt-studio.pl>
  */
 interface UserLoaderInterface
 {
-    /**
-     * Loads the user for the given username.
-     *
-     * This method must return null if the user is not found.
-     *
-     * @return UserInterface|null
-     */
-    public function loadUserByUsername(string $username);
 }

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

@@ -37,4 +37,9 @@ public function getUsername(): string
     {
         return $this->username;
     }
+
+    public function getUserIdentifier(): string
+    {
+        return $this->username;
+    }
 }

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

@@ -53,6 +53,11 @@ public function getUsername(): string
         return $this->name;
     }
 
+    public function getUserIdentifier(): string
+    {
+        return $this->name;
+    }
+
     public function eraseCredentials()
     {
     }

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

@@ -21,7 +21,7 @@
 use Symfony\Bridge\Doctrine\Security\User\UserLoaderInterface;
 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\Exception\UserNotFoundException;
 use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
 use Symfony\Component\Security\Core\User\PasswordUpgraderInterface;
 use Symfony\Component\Security\Core\User\UserInterface;
@@ -60,7 +60,7 @@ public function testLoadUserByUsername()
 
         $provider = new EntityUserProvider($this->getManager($em), 'Symfony\Bridge\Doctrine\Tests\Fixtures\User', 'name');
 
-        $this->assertSame($user, $provider->loadUserByUsername('user1'));
+        $this->assertSame($user, $provider->loadUserByIdentifier('user1'));
     }
 
     public function testLoadUserByUsernameWithUserLoaderRepositoryAndWithoutProperty()
@@ -70,7 +70,7 @@ public function testLoadUserByUsernameWithUserLoaderRepositoryAndWithoutProperty
         $repository = $this->createMock(UserLoaderRepository::class);
         $repository
             ->expects($this->once())
-            ->method('loadUserByUsername')
+            ->method('loadUserByIdentifier')
             ->with('user1')
             ->willReturn($user);
 
@@ -82,7 +82,7 @@ public function testLoadUserByUsernameWithUserLoaderRepositoryAndWithoutProperty
             ->willReturn($repository);
 
         $provider = new EntityUserProvider($this->getManager($em), 'Symfony\Bridge\Doctrine\Tests\Fixtures\User');
-        $this->assertSame($user, $provider->loadUserByUsername('user1'));
+        $this->assertSame($user, $provider->loadUserByIdentifier('user1'));
     }
 
     public function testLoadUserByUsernameWithNonUserLoaderRepositoryAndWithoutProperty()
@@ -98,7 +98,7 @@ public function testLoadUserByUsernameWithNonUserLoaderRepositoryAndWithoutPrope
         $em->flush();
 
         $provider = new EntityUserProvider($this->getManager($em), 'Symfony\Bridge\Doctrine\Tests\Fixtures\User');
-        $provider->loadUserByUsername('user1');
+        $provider->loadUserByIdentifier('user1');
     }
 
     public function testRefreshUserRequiresId()
@@ -126,7 +126,7 @@ public function testRefreshInvalidUser()
         $provider = new EntityUserProvider($this->getManager($em), 'Symfony\Bridge\Doctrine\Tests\Fixtures\User', 'name');
 
         $user2 = new User(1, 2, 'user2');
-        $this->expectException(UsernameNotFoundException::class);
+        $this->expectException(UserNotFoundException::class);
         $this->expectExceptionMessage('User with id {"id1":1,"id2":2} not found');
 
         $provider->refreshUser($user2);
@@ -153,7 +153,7 @@ public function testLoadUserByUserNameShouldLoadUserWhenProperInterfaceProvided(
     {
         $repository = $this->createMock(UserLoaderRepository::class);
         $repository->expects($this->once())
-            ->method('loadUserByUsername')
+            ->method('loadUserByIdentifier')
             ->with('name')
             ->willReturn(
                 $this->createMock(UserInterface::class)
@@ -164,7 +164,7 @@ public function testLoadUserByUserNameShouldLoadUserWhenProperInterfaceProvided(
             'Symfony\Bridge\Doctrine\Tests\Fixtures\User'
         );
 
-        $provider->loadUserByUsername('name');
+        $provider->loadUserByIdentifier('name');
     }
 
     public function testLoadUserByUserNameShouldDeclineInvalidInterface()
@@ -177,7 +177,7 @@ public function testLoadUserByUserNameShouldDeclineInvalidInterface()
             'Symfony\Bridge\Doctrine\Tests\Fixtures\User'
         );
 
-        $provider->loadUserByUsername('name');
+        $provider->loadUserByIdentifier('name');
     }
 
     public function testPasswordUpgrades()
@@ -231,6 +231,7 @@ private function createSchema($em)
 
 abstract class UserLoaderRepository implements ObjectRepository, UserLoaderInterface
 {
+    abstract public function loadUserByIdentifier(string $identifier): ?UserInterface;
 }
 
 abstract class PasswordUpgraderRepository implements ObjectRepository, PasswordUpgraderInterface

src/Symfony/Bridge/Monolog/Processor/AbstractTokenProcessor.php

@@ -42,10 +42,16 @@ public function __invoke(array $record): array
 
         if (null !== $token = $this->getToken()) {
             $record['extra'][$this->getKey()] = [
-                'username' => $token->getUsername(),
                 'authenticated' => $token->isAuthenticated(),
                 'roles' => $token->getRoleNames(),
             ];
+
+            // @deprecated since 5.3, change to $token->getUserIdentifier() in 6.0
+            if (method_exists($token, 'getUserIdentifier')) {
+                $record['extra'][$this->getKey()]['username'] = $record['extra'][$this->getKey()]['user_identifier'] = $token->getUserIdentifier();
+            } else {
+                $record['extra'][$this->getKey()]['username'] = $token->getUsername();
+            }
         }
 
         return $record;

src/Symfony/Bridge/Monolog/Tests/Processor/SwitchUserTokenProcessorTest.php

@@ -37,11 +37,15 @@ public function testProcessor()
 
         $expected = [
             'impersonator_token' => [
-                'username' => 'original_user',
                 'authenticated' => true,
                 'roles' => ['ROLE_SUPER_ADMIN'],
+                'username' => 'original_user',
             ],
         ];
-        $this->assertSame($expected, $record['extra']);
+        if (method_exists($originalToken, 'getUserIdentifier')) {
+            $expected['impersonator_token']['user_identifier'] = 'original_user';
+        }
+
+        $this->assertEquals($expected, $record['extra']);
     }
 }

src/Symfony/Bridge/Monolog/Tests/Processor/TokenProcessorTest.php

@@ -23,8 +23,12 @@
  */
 class TokenProcessorTest extends TestCase
 {
-    public function testProcessor()
+    public function testLegacyProcessor()
     {
+        if (method_exists(UsernamePasswordToken::class, 'getUserIdentifier')) {
+            $this->markTestSkipped('This test requires symfony/security-core <5.3');
+        }
+
         $token = new UsernamePasswordToken('user', 'password', 'provider', ['ROLE_USER']);
         $tokenStorage = $this->createMock(TokenStorageInterface::class);
         $tokenStorage->method('getToken')->willReturn($token);
@@ -38,4 +42,24 @@ public function testProcessor()
         $this->assertEquals($token->isAuthenticated(), $record['extra']['token']['authenticated']);
         $this->assertEquals(['ROLE_USER'], $record['extra']['token']['roles']);
     }
+
+    public function testProcessor()
+    {
+        if (!method_exists(UsernamePasswordToken::class, 'getUserIdentifier')) {
+            $this->markTestSkipped('This test requires symfony/security-core 5.3+');
+        }
+
+        $token = new UsernamePasswordToken('user', 'password', 'provider', ['ROLE_USER']);
+        $tokenStorage = $this->createMock(TokenStorageInterface::class);
+        $tokenStorage->method('getToken')->willReturn($token);
+
+        $processor = new TokenProcessor($tokenStorage);
+        $record = ['extra' => []];
+        $record = $processor($record);
+
+        $this->assertArrayHasKey('token', $record['extra']);
+        $this->assertEquals($token->getUserIdentifier(), $record['extra']['token']['user_identifier']);
+        $this->assertEquals($token->isAuthenticated(), $record['extra']['token']['authenticated']);
+        $this->assertEquals(['ROLE_USER'], $record['extra']['token']['roles']);
+    }
 }

src/Symfony/Bundle/FrameworkBundle/Tests/Functional/Bundle/TestBundle/Controller/SecurityController.php

@@ -21,6 +21,6 @@ class SecurityController implements ContainerAwareInterface
 
     public function profileAction()
     {
-        return new Response('Welcome '.$this->container->get('security.token_storage')->getToken()->getUsername().'!');
+        return new Response('Welcome '.$this->container->get('security.token_storage')->getToken()->getUserIdentifier().'!');
     }
 }

src/Symfony/Bundle/SecurityBundle/DataCollector/SecurityDataCollector.php

@@ -97,7 +97,9 @@ public function collect(Request $request, Response $response, \Throwable $except
 
             $impersonatorUser = null;
             if ($token instanceof SwitchUserToken) {
-                $impersonatorUser = $token->getOriginalToken()->getUsername();
+                $originalToken = $token->getOriginalToken();
+                // @deprecated since 5.3, change to $originalToken->getUserIdentifier() in 6.0
+                $impersonatorUser = method_exists($originalToken, 'getUserIdentifier') ? $originalToken->getUserIdentifier() : $originalToken->getUsername();
             }
 
             if (null !== $this->roleHierarchy) {
@@ -126,7 +128,8 @@ public function collect(Request $request, Response $response, \Throwable $except
                 'token' => $token,
                 'token_class' => $this->hasVarDumper ? new ClassStub(\get_class($token)) : \get_class($token),
                 'logout_url' => $logoutUrl,
-                'user' => $token->getUsername(),
+                // @deprecated since 5.3, change to $token->getUserIdentifier() in 6.0
+                'user' => method_exists($token, 'getUserIdentifier') ? $token->getUserIdentifier() : $token->getUsername(),
                 'roles' => $assignedRoles,
                 'inherited_roles' => array_unique($inheritedRoles),
                 'supports_role_hierarchy' => null !== $this->roleHierarchy,

src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/UserProvider/InMemoryFactory.php

@@ -48,8 +48,30 @@ public function addConfiguration(NodeDefinition $node)
             ->fixXmlConfig('user')
             ->children()
                 ->arrayNode('users')
-                    ->useAttributeAsKey('name')
+                    ->useAttributeAsKey('identifier')
                     ->normalizeKeys(false)
+                    ->beforeNormalization()
+                        ->always()
+                        ->then(function ($v) {
+                            $deprecation = false;
+                            foreach ($v as $i => $child) {
+                                if (!isset($child['name'])) {
+                                    continue;
+                                }
+
+                                $deprecation = true;
+
+                                $v[$i]['identifier'] = $child['name'];
+                                unset($v[$i]['name']);
+                            }
+
+                            if ($deprecation) {
+                                trigger_deprecation('symfony/security-bundle', '5.3', 'The "in_memory.user.name" option is deprecated, use "identifier" instead.');
+                            }
+
+                            return $v;
+                        })
+                    ->end()
                     ->prototype('array')
                         ->children()
                             ->scalarNode('password')->defaultNull()->end()

src/Symfony/Bundle/SecurityBundle/Resources/config/schema/security-1.0.xsd

@@ -136,7 +136,8 @@
     </xsd:complexType>
 
     <xsd:complexType name="user">
-        <xsd:attribute name="name" type="xsd:string" use="required" />
+        <xsd:attribute name="identifier" type="xsd:string" />
+        <xsd:attribute name="name" type="xsd:string" />
         <xsd:attribute name="password" type="xsd:string" />
         <xsd:attribute name="roles" type="xsd:string" />
     </xsd:complexType>