diff --git a/src/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveRequestController.php b/src/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveRequestController.php index 9440eee85..a7eb45338 100644 --- a/src/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveRequestController.php +++ b/src/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveRequestController.php @@ -45,6 +45,32 @@ final class SaveRequestController extends UserPassResetSaveBase { /** + * Ask for a password-recovery link. + * + * Every outcome answers the same thing. This endpoint needs no session, so whatever it + * distinguishes it distinguishes for anybody: it used to answer "User not found" for a login + * that does not exist, "Wrong data" for one that does with the wrong address, "Unable to reset + * the password" for a disabled or LDAP account, and "Request sent" when it worked. That is an + * oracle over the whole user table — whether a login exists, which address is on file for it, + * and whether it is usable — offered to an unauthenticated caller, one request at a time. + * + * Half of it had already been closed: the disabled and LDAP refusals were collapsed into one + * message for exactly this reason, and the test that pinned them says so. Collapsing two of + * four still left the first question answerable, so this finishes it. + * + * The sibling gets it right and settles the shape: Login answers "Wrong login" for an unknown + * user and for a wrong password alike, rather than saying which. + * + * The rate limit is deliberately still distinguishable. "Attempts exceeded" is about the + * caller's own behaviour and reveals nothing about any account, and hiding it would leave + * somebody who has locked themselves out with no way to find out why. + * + * What is lost is the message telling an honest user they mistyped their address, and the one + * telling a disabled user to contact an administrator. Both are recoverable — the address is + * theirs to check, and a disabled account is a conversation with an administrator either way — + * whereas an enumerable user list is not. The real outcome is still recorded in the event log, + * where an administrator can see it and a stranger cannot. + * * @return ActionResponse */ #[Action(ResponseType::JSON)] @@ -52,10 +78,23 @@ public function saveRequestAction(): ActionResponse { try { $this->checkTracking(); + } catch (Exception $e) { + processException($e); + + // Still counted. checkTracking() throws once the limit is already reached, and + // recording the attempt anyway is what makes further hammering extend the block + // rather than sit out a window that stops growing. + $this->addTracking(); - $login = $this->request->analyzeString('login'); - $email = $this->request->analyzeEmail('email'); + $this->eventDispatcher->notify(new Event('exception', $e)); + + return ActionResponse::error($e->getMessage()); + } + $login = $this->request->analyzeString('login'); + $email = $this->request->analyzeEmail('email'); + + try { $userData = $this->userService->getByLogin($login); if ($userData->getEmail() !== $email) { @@ -85,19 +124,19 @@ public function saveRequestAction(): ActionResponse $email, UserPassRecover::getMailMessage($hash, $this->uriContext->getWebUri()) ); - - return ActionResponse::ok( - __u('Request sent'), - [__u('You will receive an email to complete the request shortly.')] - ); } catch (Exception $e) { + // Recorded and counted, not reported. The tracking still runs, so guessing is still + // rate limited; only the answer the guesser gets back is the same either way. processException($e); $this->addTracking(); $this->eventDispatcher->notify(new Event('exception', $e)); - - return ActionResponse::error($e->getMessage()); } + + return ActionResponse::ok( + __u('Request sent'), + [__u('You will receive an email to complete the request shortly.')] + ); } } diff --git a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveRequestRefusalsTest.php b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveRequestRefusalsTest.php index c51108f4e..18474ff86 100644 --- a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveRequestRefusalsTest.php +++ b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveRequestRefusalsTest.php @@ -34,6 +34,7 @@ use SP\Domain\Common\Dtos\QueryResult; use SP\Domain\User\Models\User; use SP\Tests\Support\BodyChecker; +use SP\Application\Notification\Ports\MailService; use SP\Tests\Support\IntegrationTestCase; /** @@ -57,7 +58,7 @@ class SaveRequestRefusalsTest extends IntegrationTestCase * @throws NotFoundExceptionInterface */ #[Test] - #[BodyChecker('outputCheckerWrongData')] + #[BodyChecker('outputCheckerIndistinguishable')] public function anAddressThatIsNotTheAccountsIsRefused() { $this->givenAUser(['email' => self::REGISTERED_EMAIL]); @@ -73,7 +74,7 @@ public function anAddressThatIsNotTheAccountsIsRefused() * @throws NotFoundExceptionInterface */ #[Test] - #[BodyChecker('outputCheckerContactAdministrator')] + #[BodyChecker('outputCheckerIndistinguishable')] public function aDisabledAccountIsRefused() { $this->givenAUser(['email' => self::REGISTERED_EMAIL, 'isDisabled' => true]); @@ -90,7 +91,7 @@ public function aDisabledAccountIsRefused() * @throws NotFoundExceptionInterface */ #[Test] - #[BodyChecker('outputCheckerContactAdministrator')] + #[BodyChecker('outputCheckerIndistinguishable')] public function anLdapAccountIsRefused() { $this->givenAUser(['email' => self::REGISTERED_EMAIL, 'isLdap' => true]); @@ -98,6 +99,84 @@ public function anLdapAccountIsRefused() $this->whenRequesting('someone', self::REGISTERED_EMAIL); } + /** + * A login nobody has is answered exactly like one that exists. + * + * This is the half that was still open. The disabled and LDAP refusals had already been + * collapsed into a single message for this reason; an unknown login still answered "User not + * found" while a known one with the wrong address answered "Wrong data", so the first question + * anybody would ask — does this login exist — was answerable by anyone, unauthenticated. + * + * @throws ContainerExceptionInterface + * @throws Exception + * @throws NotFoundExceptionInterface + */ + #[Test] + #[BodyChecker('outputCheckerIndistinguishable')] + public function aLoginThatDoesNotExistIsAnsweredLikeOneThatDoes() + { + $this->givenNoSuchUser(); + + $this->whenRequesting('nobody', self::REGISTERED_EMAIL); + } + + /** + * And so is a request that actually works, or the answer would separate the successes from + * everything else instead — which tells a caller just as much. + * + * @throws ContainerExceptionInterface + * @throws Exception + * @throws NotFoundExceptionInterface + */ + #[Test] + #[BodyChecker('outputCheckerIndistinguishable')] + public function aRequestThatSucceedsIsAnsweredTheSameWay() + { + $this->givenAUser(['email' => self::REGISTERED_EMAIL]); + + $this->whenRequesting('someone', self::REGISTERED_EMAIL); + } + + /** + * The request that works still sends the mail. + * + * Every outcome now answers the same string, which is the point — but a response that says + * "Request sent" whatever happened is also exactly what an endpoint that had quietly stopped + * sending anything would produce. Nothing else here would notice, because the failure path is + * deliberately swallowed. So this asserts the mail itself, at the address on the account. + * + * @throws ContainerExceptionInterface + * @throws Exception + * @throws NotFoundExceptionInterface + */ + #[Test] + #[BodyChecker('outputCheckerIndistinguishable')] + public function aRequestThatSucceedsStillSendsTheMail() + { + $this->givenAUser(['email' => self::REGISTERED_EMAIL]); + + $sentTo = []; + + $mailService = $this->createStub(MailService::class); + $mailService->method('send')->willReturnCallback( + static function (string $subject, string $to) use (&$sentTo): void { + $sentTo[] = $to; + } + ); + + $this->whenRequesting('someone', self::REGISTERED_EMAIL, [MailService::class => $mailService]); + + self::assertSame([self::REGISTERED_EMAIL], $sentTo, 'the recovery mail must still go out'); + } + + /** + * No row for the login, which is what the service turns into its "User not found". + */ + private function givenNoSuchUser(): void + { + $this->addDatabaseMapperResolver(User::class, new QueryResult([])); + } + /** * @param array $properties */ @@ -114,7 +193,10 @@ private function givenAUser(array $properties): void * @throws Exception * @throws NotFoundExceptionInterface */ - private function whenRequesting(string $login, string $email): void + /** + * @param array $definitionsOverride + */ + private function whenRequesting(string $login, string $email, array $definitionsOverride = []): void { $container = $this->buildContainer( IntegrationTestCase::buildRequest( @@ -122,30 +204,23 @@ private function whenRequesting(string $login, string $email): void 'index.php', ['r' => 'userPassReset/saveRequest'], ['login' => $login, 'email' => $email] - ) + ), + $definitionsOverride ); IntegrationTestCase::runApp($container); } - private function outputCheckerWrongData(string $output): void - { - $json = json_decode($output); - - self::assertNotEquals('OK', $json->status); - self::assertSame('Wrong data', $json->description); - } - /** - * Both refusals answer identically. The reply does not say which of the two applied, so an - * unauthenticated caller learns nothing about the account from asking — which is why the - * assertion is on the exact message rather than on it merely failing. + * Every outcome is asserted against the same string, deliberately. That is the whole point of + * the change these pin: an unauthenticated caller must not be able to tell a login that does + * not exist from one that does, nor either from a request that actually sent a mail. */ - private function outputCheckerContactAdministrator(string $output): void + private function outputCheckerIndistinguishable(string $output): void { $json = json_decode($output); - self::assertNotEquals('OK', $json->status); - self::assertSame('Unable to reset the password', $json->description); + self::assertSame('OK', $json->status); + self::assertSame('Request sent', $json->description); } } diff --git a/tests/Unit/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/RefusalsTest.php b/tests/Unit/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/RefusalsTest.php index e1657f6a8..1101c5798 100644 --- a/tests/Unit/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/RefusalsTest.php +++ b/tests/Unit/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/RefusalsTest.php @@ -139,10 +139,15 @@ public function savingAResetIsRefusedWhenAttemptsAreExceeded(): void /** * What the action does when the work behind it fails, once tracking allows it through. * + * It answers exactly what it answers when the request succeeds. This endpoint is reachable + * without a session, so anything it distinguishes it distinguishes for anybody — a failure + * reported as itself told an unauthenticated caller whether the login existed. The failure is + * still recorded and still counted against the caller; only the answer is the same. + * * @throws Exception */ #[Test] - public function savingARequestReportsAFailureBehindItRatherThanEscaping(): void + public function savingARequestReportsAFailureBehindItTheSameWayAsASuccess(): void { $request = $this->createStub(RequestService::class); $request->method('isAjax')->willReturn(false); @@ -172,8 +177,8 @@ public function savingARequestReportsAFailureBehindItRatherThanEscaping(): void $trackService ))->saveRequestAction(); - self::assertSame(ResponseStatus::ERROR, $response->status); - self::assertSame('the user could not be read', $response->subject); + self::assertSame(ResponseStatus::OK, $response->status); + self::assertSame('Request sent', $response->subject); } /**