From 04573dc6d7c6a7b2c5f303204b499f30c625030e Mon Sep 17 00:00:00 2001 From: silverbackdan Date: Sat, 31 Oct 2026 15:54:51 +0000 Subject: [PATCH 1/3] Trim the identifier in user repository lookups, so a trailing space still signs in and finds the user (#574) --- CLAUDE.md | 2 +- .../User/FindUserByUsernameTrait.php | 2 +- src/Repository/User/UserRepository.php | 10 +-- tests/Functional/User/ForgotPasswordTest.php | 11 +++ tests/Functional/User/SecurityTest.php | 23 +++++++ .../User/VerifyEmailAddressTest.php | 12 ++++ .../User/FindUserByUsernameTraitTest.php | 63 +++++++++++++++++ tests/Repository/User/UserRepositoryTest.php | 69 +++++++++++++++++++ 8 files changed, 185 insertions(+), 7 deletions(-) create mode 100644 tests/Repository/User/FindUserByUsernameTraitTest.php create mode 100644 tests/Repository/User/UserRepositoryTest.php diff --git a/CLAUDE.md b/CLAUDE.md index d900bd891..cb494e86b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -301,7 +301,7 @@ Resources are fetched and cached **individually**. Never embed related data outs - **A failed send returns false whether or not a logger is wired** (#340): the logger is optional in `UserMailer`'s service locator, so check `has('logger')`; `get()` on a missing id throws. - **The `…RequestedAt` timestamp starts the throttle, so `UserMailer` sets it only after `send()` succeeds**, and flushes it with the token then. A failed send (503) saves nothing: the action refreshes the user, so the token is not replaced by one nobody received. - Actions calling `UserDataProcessor::findUserByUsername()` must catch its `InvalidArgumentException` and return 404; it throws rather than returning null. -- **Code typed against `UserRepositoryInterface` may call only what it declares** (#266, #336). The `@method` tags are not a contract, and PHPStan trusts them, so it will not catch a call to `find()` or `findOneBy()`. `loadUserByIdentifier()` matches username *or* email, so check the returned username and treat `NonUniqueResultException` as not found; `Repository\User\FindUserByUsernameTrait` does both. Test against `tests/Command/InMemoryUserRepository.php`, which implements only the interface. +- **Code typed against `UserRepositoryInterface` may call only what it declares** (#266, #336). The `@method` tags are not a contract, and PHPStan trusts them, so it will not catch a call to `find()` or `findOneBy()`. `loadUserByIdentifier()` matches username *or* email, trimmed and lower-cased like every `UserRepository` lookup (#574: a mobile keyboard's trailing space), so check the returned username, compared the same way and treat `NonUniqueResultException` as not found; `Repository\User\FindUserByUsernameTrait` does both. Test against `tests/Command/InMemoryUserRepository.php`, which implements only the interface. - `user:create` throws `ValidationFailedException` rather than writing an invalid or duplicate user (#254). There is no DB unique constraint on username or email. - Security routes: one path each. A duplicate path fails nothing and one route silently never runs; check with `debug:router`. - `GET /_/health` (#312) is a plain Symfony route (not an AP operation, which would add JSON-LD, cache headers and tags), `no-store`, 200 or 503 with the DBAL message logged at warning and never returned. It is imported by `routing/all.php`. diff --git a/src/Repository/User/FindUserByUsernameTrait.php b/src/Repository/User/FindUserByUsernameTrait.php index 271c70665..a87af1b93 100644 --- a/src/Repository/User/FindUserByUsernameTrait.php +++ b/src/Repository/User/FindUserByUsernameTrait.php @@ -24,6 +24,6 @@ private function findUserByUsernameIn(UserRepositoryInterface $repository, strin return null; } - return null !== $user && strtolower((string) $user->getUsername()) === strtolower($username) ? $user : null; + return null !== $user && strtolower((string) $user->getUsername()) === strtolower(trim($username)) ? $user : null; } } diff --git a/src/Repository/User/UserRepository.php b/src/Repository/User/UserRepository.php index 54bb29aa6..5420f18ac 100644 --- a/src/Repository/User/UserRepository.php +++ b/src/Repository/User/UserRepository.php @@ -43,7 +43,7 @@ public function findOneByEmail(string $value): ?AbstractUser { return $this->createQueryBuilder('u') ->andWhere('LOWER(u.emailAddress) = :val') - ->setParameter('val', strtolower($value)) + ->setParameter('val', strtolower(trim($value))) ->getQuery() ->getOneOrNullResult(); } @@ -57,7 +57,7 @@ public function findOneWithPasswordResetToken(string $username): ?AbstractUser ->andWhere('LOWER(u.username) = :username') ->andWhere('u.newPasswordConfirmationToken IS NOT NULL') ->andWhere('u.passwordRequestedAt > :minimumDateTime') - ->setParameter('username', strtolower($username)) + ->setParameter('username', strtolower(trim($username))) ->setParameter('minimumDateTime', $minimumRequestDateTime) ->getQuery() ->getOneOrNullResult(); @@ -73,8 +73,8 @@ public function findOneByUsernameAndNewEmailAddress(string $username, string $em ->andWhere('LOWER(u.newEmailAddress) = :email') ->andWhere('u.newEmailConfirmationToken IS NOT NULL') ->andWhere('u.newEmailAddressChangeRequestedAt > :minimumDateTime') - ->setParameter('username', strtolower($username)) - ->setParameter('email', strtolower($email)) + ->setParameter('username', strtolower(trim($username))) + ->setParameter('email', strtolower(trim($email))) ->setParameter('minimumDateTime', $minimumRequestDateTime) ->getQuery() ->getOneOrNullResult(); @@ -90,7 +90,7 @@ public function loadUserByIdentifier(string $identifier): ?AbstractUser return $this->createQueryBuilder('u') ->andWhere('LOWER(u.username) = :username') ->orWhere('LOWER(u.emailAddress) = :username') - ->setParameter('username', strtolower($identifier)) + ->setParameter('username', strtolower(trim($identifier))) ->getQuery() ->getOneOrNullResult(); } diff --git a/tests/Functional/User/ForgotPasswordTest.php b/tests/Functional/User/ForgotPasswordTest.php index 04c5d69be..55f91badf 100644 --- a/tests/Functional/User/ForgotPasswordTest.php +++ b/tests/Functional/User/ForgotPasswordTest.php @@ -32,6 +32,17 @@ public function test_i_can_request_a_new_password(): void self::assertEmailOfTypeSent('password_reset', 'test.user@example.com'); } + public function test_a_username_with_a_trailing_space_still_requests_a_new_password(): void + { + $client = static::createClient(); + self::createUserWithRole('my_username', self::generatedSecret(), 'ROLE_USER'); + + self::requestJsonLd($client, 'GET', '/password/reset/request/my_username%20', null, self::referer()); + + self::assertResponseStatusCodeSame(200); + self::assertEmailOfTypeSent('password_reset', 'test.user@example.com'); + } + public function test_i_get_the_appropriate_status_code_if_the_user_is_not_found(): void { $client = static::createClient(); diff --git a/tests/Functional/User/SecurityTest.php b/tests/Functional/User/SecurityTest.php index b5879e311..f76fb7cd1 100644 --- a/tests/Functional/User/SecurityTest.php +++ b/tests/Functional/User/SecurityTest.php @@ -63,6 +63,29 @@ public function test_a_successful_login(): void self::assertMercureCookieAndTokenExpireIn(604800); } + #[DataProvider('identifiersWithSurroundingSpace')] + public function test_a_login_identifier_with_surrounding_space_still_signs_in(string $identifier): void + { + $client = static::createClient(); + $password = self::generatedSecret(); + self::createUserWithRole('user', $password, 'ROLE_USER', 'user@example.com'); + + self::login($client, $identifier, $password); + + self::assertResponseStatusCodeSame(204); + self::assertRefreshTokenCount(1); + } + + /** + * @return iterable + */ + public static function identifiersWithSurroundingSpace(): iterable + { + yield 'a username with a trailing space' => ['user ']; + yield 'a username with a leading space and capitals' => [' USER']; + yield 'an email address with a trailing space' => ['user@example.com ']; + } + public function test_a_logged_in_user_can_fetch_a_list_of_all_users(): void { $client = static::createClient(); diff --git a/tests/Functional/User/VerifyEmailAddressTest.php b/tests/Functional/User/VerifyEmailAddressTest.php index 689cc0791..58c9a12b7 100644 --- a/tests/Functional/User/VerifyEmailAddressTest.php +++ b/tests/Functional/User/VerifyEmailAddressTest.php @@ -73,6 +73,18 @@ public function test_i_can_request_a_new_email_address_verification_email(): voi self::assertEmailOfTypeSent('verify_email'); } + public function test_a_username_with_a_trailing_space_still_requests_a_new_verification_email(): void + { + $client = static::createClient(); + $user = self::createUserWithRole('my_username', self::generatedSecret(), 'ROLE_USER', 'user@example.com'); + self::unverifyEmail($user); + + self::requestJsonLd($client, 'GET', '/resend-verify-email/my_username%20', null, self::REFERER); + + self::assertResponseStatusCodeSame(200); + self::assertEmailOfTypeSent('verify_email'); + } + public function test_requesting_a_verification_email_for_an_unknown_username_is_a_404_not_an_error(): void { $client = static::createClient(); diff --git a/tests/Repository/User/FindUserByUsernameTraitTest.php b/tests/Repository/User/FindUserByUsernameTraitTest.php new file mode 100644 index 000000000..1ba1afb62 --- /dev/null +++ b/tests/Repository/User/FindUserByUsernameTraitTest.php @@ -0,0 +1,63 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Silverback\ApiComponentsBundle\Tests\Repository\User; + +use Doctrine\ORM\NonUniqueResultException; +use PHPUnit\Framework\Attributes\DataProvider; +use PHPUnit\Framework\TestCase; +use Silverback\ApiComponentsBundle\Entity\User\AbstractUser; +use Silverback\ApiComponentsBundle\Repository\User\FindUserByUsernameTrait; +use Silverback\ApiComponentsBundle\Repository\User\UserRepositoryInterface; +use Silverback\ApiComponentsBundle\Tests\Functional\TestBundle\Entity\User; + +final class FindUserByUsernameTraitTest extends TestCase +{ + /** + * @return iterable + */ + public static function usernames(): iterable + { + yield 'the username' => ['my_username', true]; + yield 'the username in capitals with surrounding space' => [' MY_USERNAME ', true]; + yield 'the email address the identifier also matches' => ['user@example.com', false]; + } + + #[DataProvider('usernames')] + public function test_only_the_users_own_username_finds_them(string $username, bool $found): void + { + $user = new User('my_username', 'user@example.com'); + $repository = $this->createStub(UserRepositoryInterface::class); + $repository->method('loadUserByIdentifier')->willReturn($user); + + self::assertSame($found ? $user : null, $this->finder()->find($repository, $username)); + } + + public function test_a_username_matching_several_users_finds_none(): void + { + $repository = $this->createStub(UserRepositoryInterface::class); + $repository->method('loadUserByIdentifier')->willThrowException(new NonUniqueResultException()); + + self::assertNull($this->finder()->find($repository, 'my_username')); + } + + private function finder(): object + { + return new class { + use FindUserByUsernameTrait; + + public function find(UserRepositoryInterface $repository, string $username): ?AbstractUser + { + return $this->findUserByUsernameIn($repository, $username); + } + }; + } +} diff --git a/tests/Repository/User/UserRepositoryTest.php b/tests/Repository/User/UserRepositoryTest.php new file mode 100644 index 000000000..25f77a6aa --- /dev/null +++ b/tests/Repository/User/UserRepositoryTest.php @@ -0,0 +1,69 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Silverback\ApiComponentsBundle\Tests\Repository\User; + +use PHPUnit\Framework\Attributes\DataProvider; +use Silverback\ApiComponentsBundle\Repository\User\UserRepository; +use Silverback\ApiComponentsBundle\Tests\Functional\TestBundle\Entity\User; +use Silverback\ApiComponentsBundle\Tests\Helper\OrphanedResource\OrphanedResourceDatabaseTestCase; + +final class UserRepositoryTest extends OrphanedResourceDatabaseTestCase +{ + /** + * @return iterable + */ + public static function lookupsWithSurroundingSpace(): iterable + { + yield 'a username identifier' => [static fn (UserRepository $repository) => $repository->loadUserByIdentifier(" My_Username \t")]; + yield 'an email address identifier' => [static fn (UserRepository $repository) => $repository->loadUserByIdentifier(' User@Example.com ')]; + yield 'a username through the deprecated loader' => [static fn (UserRepository $repository) => $repository->loadUserByUsername('my_username ')]; + yield 'an email address' => [static fn (UserRepository $repository) => $repository->findOneByEmail(' user@example.com ')]; + yield 'a username with a password reset token' => [static fn (UserRepository $repository) => $repository->findOneWithPasswordResetToken(' my_username ')]; + yield 'a username and new email address' => [static fn (UserRepository $repository) => $repository->findOneByUsernameAndNewEmailAddress(' my_username ', ' New@Example.com ')]; + } + + #[DataProvider('lookupsWithSurroundingSpace')] + public function test_a_lookup_ignores_surrounding_whitespace_and_case(\Closure $lookup): void + { + $user = $this->persistUser(); + + self::assertSame($user, $lookup($this->repository())); + } + + public function test_an_identifier_that_matches_no_one_finds_no_user(): void + { + $this->persistUser(); + + self::assertNull($this->repository()->loadUserByIdentifier('my user')); + } + + private function persistUser(): User + { + $user = new User('my_username', 'user@example.com'); + $user + ->setNewPasswordConfirmationToken('token') + ->setPasswordRequestedAt(new \DateTime()) + ->setNewEmailAddress('new@example.com') + ->setNewEmailConfirmationToken('token') + ->setNewEmailAddressChangeRequestedAt(new \DateTime()); + $this->timestampedDataPersister->persistTimestampedFields($user, true); + $this->entityManager->persist($user); + $this->entityManager->flush(); + + return $user; + } + + private function repository(): UserRepository + { + return new UserRepository($this->registry, User::class, 3600, 3600); + } +} From 1e34ea30eb4304e5dba822c19a14a2db57923fe5 Mon Sep 17 00:00:00 2001 From: silverbackdan Date: Sat, 31 Oct 2026 15:55:03 +0000 Subject: [PATCH 2/3] Add the changelog line (#574) --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c32523ff..ced657537 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,9 @@ Every change that reaches `main` adds a line under **Unreleased**. When a versio ## Unreleased +### Fixed +- User lookups trim the identifier, so a username or email address with a trailing space (a mobile keyboard's) still signs in and still finds the user for password reset and resend verification ([#578](https://github.com/components-web-app/api-components-bundle/pull/578)) + ## [2.0.0-alpha.39](https://github.com/components-web-app/api-components-bundle/compare/2.0.0-alpha.38...2.0.0-alpha.39) - 2026-10-30 ### Added From 140c1e7abbca3ae60f1f7e38ba376fc3cf49ceca Mon Sep 17 00:00:00 2001 From: silverbackdan Date: Sat, 31 Oct 2026 16:05:51 +0000 Subject: [PATCH 3/3] Look users up with capitals in the unit tests, so lower-casing is pinned (#574) --- .../Repository/User/FindUserByUsernameTraitTest.php | 13 +++++++------ tests/Repository/User/UserRepositoryTest.php | 8 ++++---- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/tests/Repository/User/FindUserByUsernameTraitTest.php b/tests/Repository/User/FindUserByUsernameTraitTest.php index 1ba1afb62..0abd86291 100644 --- a/tests/Repository/User/FindUserByUsernameTraitTest.php +++ b/tests/Repository/User/FindUserByUsernameTraitTest.php @@ -22,19 +22,20 @@ final class FindUserByUsernameTraitTest extends TestCase { /** - * @return iterable + * @return iterable */ public static function usernames(): iterable { - yield 'the username' => ['my_username', true]; - yield 'the username in capitals with surrounding space' => [' MY_USERNAME ', true]; - yield 'the email address the identifier also matches' => ['user@example.com', false]; + yield 'the username' => ['my_username', 'my_username', true]; + yield 'the username in capitals with surrounding space' => ['my_username', ' MY_USERNAME ', true]; + yield 'a username stored in capitals' => ['My_Username', 'my_username', true]; + yield 'the email address the identifier also matches' => ['my_username', 'user@example.com', false]; } #[DataProvider('usernames')] - public function test_only_the_users_own_username_finds_them(string $username, bool $found): void + public function test_only_the_users_own_username_finds_them(string $storedUsername, string $username, bool $found): void { - $user = new User('my_username', 'user@example.com'); + $user = new User($storedUsername, 'user@example.com'); $repository = $this->createStub(UserRepositoryInterface::class); $repository->method('loadUserByIdentifier')->willReturn($user); diff --git a/tests/Repository/User/UserRepositoryTest.php b/tests/Repository/User/UserRepositoryTest.php index 25f77a6aa..daf31f9a3 100644 --- a/tests/Repository/User/UserRepositoryTest.php +++ b/tests/Repository/User/UserRepositoryTest.php @@ -25,10 +25,10 @@ public static function lookupsWithSurroundingSpace(): iterable { yield 'a username identifier' => [static fn (UserRepository $repository) => $repository->loadUserByIdentifier(" My_Username \t")]; yield 'an email address identifier' => [static fn (UserRepository $repository) => $repository->loadUserByIdentifier(' User@Example.com ')]; - yield 'a username through the deprecated loader' => [static fn (UserRepository $repository) => $repository->loadUserByUsername('my_username ')]; - yield 'an email address' => [static fn (UserRepository $repository) => $repository->findOneByEmail(' user@example.com ')]; - yield 'a username with a password reset token' => [static fn (UserRepository $repository) => $repository->findOneWithPasswordResetToken(' my_username ')]; - yield 'a username and new email address' => [static fn (UserRepository $repository) => $repository->findOneByUsernameAndNewEmailAddress(' my_username ', ' New@Example.com ')]; + yield 'a username through the deprecated loader' => [static fn (UserRepository $repository) => $repository->loadUserByUsername('MY_username ')]; + yield 'an email address' => [static fn (UserRepository $repository) => $repository->findOneByEmail(' User@Example.COM ')]; + yield 'a username with a password reset token' => [static fn (UserRepository $repository) => $repository->findOneWithPasswordResetToken(' My_Username ')]; + yield 'a username and new email address' => [static fn (UserRepository $repository) => $repository->findOneByUsernameAndNewEmailAddress(' My_Username ', ' New@Example.com ')]; } #[DataProvider('lookupsWithSurroundingSpace')]