From 706294a620a2973095c31729013c6c2a508fd532 Mon Sep 17 00:00:00 2001 From: blaipr Date: Tue, 8 Sep 2026 01:33:58 +0200 Subject: [PATCH] fix: changing a password spends the reset links outstanding for that user MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit toggleUsedByHash() consumes only the token being redeemed, called from one place, and nothing consumed the rest — no method existed that could: the repository offered getAttemptsByUserId, add, toggleUsedByHash and getUserIdForHash, and nothing else. requestForUserId() allows up to MAX_PASS_RECOVER_LIMIT = 3 outstanding tokens per user, each valid for an hour. So somebody who obtained a reset link they should not have kept a working 'set this account's password' capability, and the obvious response did not revoke it: neither the user changing their own password nor an administrator resetting it touched the outstanding tokens. toggleUsedByUserId() spends every unused token for a user, called from User::updatePass() after a successful write — the one method both the administrator's edit and the completion of a reset go through, so a third caller cannot forget. updateOnLogin() (LDAP sync) and migrateUserPassById() are deliberately left alone: the first belongs to directory users, whose password lives in the directory and who have no local reset flow, and the second re-hashes the same password rather than changing it. --- .../User/Ports/UserPassRecoverService.php | 11 ++++ src/Application/User/Services/User.php | 15 ++++- .../User/Services/UserPassRecover.php | 14 +++++ .../User/Ports/UserPassRecoverRepository.php | 11 ++++ .../Out/User/Repositories/UserPassRecover.php | 33 +++++++++++ .../Application/User/Services/UserTest.php | 59 ++++++++++++++++++- 6 files changed, 141 insertions(+), 2 deletions(-) diff --git a/src/Application/User/Ports/UserPassRecoverService.php b/src/Application/User/Ports/UserPassRecoverService.php index 9f8bcafe6..f9886b5ab 100644 --- a/src/Application/User/Ports/UserPassRecoverService.php +++ b/src/Application/User/Ports/UserPassRecoverService.php @@ -42,6 +42,17 @@ interface UserPassRecoverService * @throws SPException * @throws ServiceException */ + /** + * Spend every token outstanding for this user, because their password has just changed. + * + * @param int $userId + * + * @return int how many were still outstanding + * @throws ConstraintException + * @throws QueryException + */ + public function toggleUsedByUserId(int $userId): int; + public function toggleUsedByHash(string $hash): void; /** diff --git a/src/Application/User/Services/User.php b/src/Application/User/Services/User.php index eca1521be..701b36a02 100644 --- a/src/Application/User/Services/User.php +++ b/src/Application/User/Services/User.php @@ -40,6 +40,7 @@ use SP\Domain\User\Models\User as UserModel; use SP\Domain\User\Models\UserPreferences; use SP\Application\User\Ports\UserMasterPassService; +use SP\Application\User\Ports\UserPassRecoverService; use SP\Domain\User\Ports\UserRepository; use SP\Application\User\Ports\UserService; use SP\Domain\Core\Exceptions\DuplicatedItemException; @@ -59,7 +60,8 @@ final class User extends Service implements UserService public function __construct( Application $application, private readonly UserRepository $userRepository, - private readonly UserMasterPassService $userMasterPassService + private readonly UserMasterPassService $userMasterPassService, + private readonly UserPassRecoverService $userPassRecoverService ) { parent::__construct($application); } @@ -277,6 +279,17 @@ public function updatePass(int $userId, string $pass): void if ($this->userRepository->updatePassById($user) === 0) { throw ServiceException::error(__u('Error while updating the password')); } + + // A password that has just changed makes every outstanding reset link for this user + // meaningless, and they were left live. `toggleUsedByHash()` consumes only the one token + // being redeemed; up to three may be outstanding at once, for up to an hour. So somebody + // who had obtained a reset link — a forwarded mail, a shared machine — kept a working + // "set this account's password" capability across the very action taken in response, by + // the user or by an administrator. + // + // Here rather than in the two controllers, because this is the one method both the + // administrator's edit and the completion of a reset go through. + $this->userPassRecoverService->toggleUsedByUserId($userId); } /** diff --git a/src/Application/User/Services/UserPassRecover.php b/src/Application/User/Services/UserPassRecover.php index 737744236..7d416a4b6 100644 --- a/src/Application/User/Services/UserPassRecover.php +++ b/src/Application/User/Services/UserPassRecover.php @@ -87,6 +87,20 @@ public static function getMailMessage(string $hash, string $baseUri): MailMessag * @throws SPException * @throws ServiceException */ + /** + * Spend every token outstanding for this user, because their password has just changed. + * + * @param int $userId + * + * @return int how many were still outstanding + * @throws ConstraintException + * @throws QueryException + */ + public function toggleUsedByUserId(int $userId): int + { + return $this->userPassRecoverRepository->toggleUsedByUserId($userId); + } + public function toggleUsedByHash(string $hash): void { $time = time() - self::MAX_PASS_RECOVER_TIME; diff --git a/src/Domain/User/Ports/UserPassRecoverRepository.php b/src/Domain/User/Ports/UserPassRecoverRepository.php index aa33edfb6..0297821ed 100644 --- a/src/Domain/User/Ports/UserPassRecoverRepository.php +++ b/src/Domain/User/Ports/UserPassRecoverRepository.php @@ -72,6 +72,17 @@ public function add(int $userId, string $hash): QueryResult; * @return int The updated rows. If no rows are updated, it means that the hash doesn't exist or it's expired * @throws SPException */ + /** + * Spend every token outstanding for this user. + * + * @param int $userId + * + * @return int + * @throws ConstraintException + * @throws QueryException + */ + public function toggleUsedByUserId(int $userId): int; + public function toggleUsedByHash(string $hash, int $time): int; /** diff --git a/src/Infrastructure/Adapter/Out/User/Repositories/UserPassRecover.php b/src/Infrastructure/Adapter/Out/User/Repositories/UserPassRecover.php index 79fc47527..22b8606d9 100644 --- a/src/Infrastructure/Adapter/Out/User/Repositories/UserPassRecover.php +++ b/src/Infrastructure/Adapter/Out/User/Repositories/UserPassRecover.php @@ -102,6 +102,39 @@ public function add(int $userId, string $hash): QueryResult * @return int * @throws SPException */ + /** + * Spend every token outstanding for this user. + * + * `toggleUsedByHash()` consumes the one token being redeemed, and nothing consumed the rest — + * so a password change by any other route left them live. Up to three may be outstanding at + * once (`MAX_PASS_RECOVER_LIMIT`) for up to an hour, and an administrator resetting the + * password is the obvious response to suspecting a link has leaked; that response did not + * revoke it. + * + * No date bound, unlike the method below: an expired token is already refused by + * `getUserIdForHash()`, and marking it used costs nothing and leaves less to reason about. + * + * @param int $userId + * + * @return int + * @throws ConstraintException + * @throws QueryException + */ + public function toggleUsedByUserId(int $userId): int + { + $query = $this->queryFactory + ->newUpdate() + ->table(UserPassRecoverModel::TABLE) + ->cols(['used' => 1]) + ->where('userId = :userId', ['userId' => $userId]) + ->where('used = 0'); + + $queryData = QueryData::build($query); + $queryData->setOnErrorMessage(__u('Error while checking hash')); + + return $this->db->runQuery($queryData)->getAffectedNumRows(); + } + public function toggleUsedByHash(string $hash, int $time): int { $query = $this->queryFactory diff --git a/tests/Unit/Application/User/Services/UserTest.php b/tests/Unit/Application/User/Services/UserTest.php index c5a0f9d7b..c8962309d 100644 --- a/tests/Unit/Application/User/Services/UserTest.php +++ b/tests/Unit/Application/User/Services/UserTest.php @@ -25,6 +25,7 @@ namespace SP\Tests\Unit\Application\User\Services; +use SP\Application\User\Ports\UserPassRecoverService; use JsonException; use PHPUnit\Framework\Attributes\AllowMockObjectsWithoutExpectations; use PHPUnit\Framework\Attributes\Group; @@ -59,6 +60,7 @@ class UserTest extends UnitaryTestCase private MockObject|UserRepository $userRepository; private MockObject|UserMasterPassService $userMasterPassService; + private UserPassRecoverService|MockObject $userPassRecoverService; private User $user; /** @@ -290,6 +292,54 @@ public function testUpdatePass() $this->user->updatePass(100, 'a_password'); } + /** + * Changing a password spends every reset link outstanding for that user. + * + * `toggleUsedByHash()` consumes only the token being redeemed, and nothing consumed the rest, + * so up to three (`MAX_PASS_RECOVER_LIMIT`) stayed live for up to an hour. Somebody holding a + * reset link they should not have — a forwarded mail, a shared machine — kept a working "set + * this account's password" capability across the very action taken in response, whether by the + * user or by an administrator. + * + * Asserted here rather than in the two controllers because `updatePass()` is the one method + * both the administrator's edit and the completion of a reset go through. + * + * @throws ConstraintException + * @throws ServiceException + * @throws QueryException + */ + public function testUpdatePassSpendsTheUsersOutstandingResetTokens() + { + $this->userRepository->expects($this->once())->method('updatePassById')->willReturn(1); + + $this->userPassRecoverService + ->expects($this->once()) + ->method('toggleUsedByUserId') + ->with(100) + ->willReturn(2); + + $this->user->updatePass(100, 'a_password'); + } + + /** + * ...and a password that was not changed spends nothing. Without this the assertion above + * would also be satisfied by revoking a user's links on a write that failed. + * + * @throws ConstraintException + * @throws ServiceException + * @throws QueryException + */ + public function testAFailedPasswordChangeSpendsNoResetToken() + { + $this->userRepository->expects($this->once())->method('updatePassById')->willReturn(0); + + $this->userPassRecoverService->expects($this->never())->method('toggleUsedByUserId'); + + $this->expectException(ServiceException::class); + + $this->user->updatePass(100, 'a_password'); + } + /** * @throws ConstraintException * @throws ServiceException @@ -701,6 +751,13 @@ protected function setUp(): void $this->userRepository = $this->createMock(UserRepository::class); $this->userMasterPassService = $this->createMock(UserMasterPassService::class); - $this->user = new User($this->application, $this->userRepository, $this->userMasterPassService); + $this->userPassRecoverService = $this->createMock(UserPassRecoverService::class); + + $this->user = new User( + $this->application, + $this->userRepository, + $this->userMasterPassService, + $this->userPassRecoverService + ); } }