diff --git a/CHANGELOG.md b/CHANGELOG.md index 77a396d6..890f3416 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ Every change that reaches `main` adds a line under **Unreleased**. When a versio ### Fixed - A user referenced by IRI from another resource's write is no longer processed as a new registration, which reset their `emailAddressVerified` ([#575](https://github.com/components-web-app/api-components-bundle/pull/575)) - A `plainPassword` set through the API is validated (6 to 4096 characters, 422 on `plainPassword`) before it is hashed, and setting a password no longer marks the email address verified; only a password reset does ([#577](https://github.com/components-web-app/api-components-bundle/pull/577)) +- 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 diff --git a/CLAUDE.md b/CLAUDE.md index fa18ad5e..dc00a0e8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -304,7 +304,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 271c7066..a87af1b9 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 54bb29aa..5420f18a 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 04c5d69b..55f91bad 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 b5879e31..f76fb7cd 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 689cc079..58c9a12b 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 00000000..0abd8629 --- /dev/null +++ b/tests/Repository/User/FindUserByUsernameTraitTest.php @@ -0,0 +1,64 @@ + + * + * 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', '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 $storedUsername, string $username, bool $found): void + { + $user = new User($storedUsername, '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 00000000..daf31f9a --- /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); + } +}