fix: a password reset request answers the same either way

src/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveRequestController.php

@@ -45,17 +45,56 @@ 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)]
     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.')]
+        );
     }
 }

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,14 +91,92 @@ public function aDisabledAccountIsRefused()
      * @throws NotFoundExceptionInterface
      */
     #[Test]
-    #[BodyChecker('outputCheckerContactAdministrator')]
+    #[BodyChecker('outputCheckerIndistinguishable')]
     public function anLdapAccountIsRefused()
     {
         $this->givenAUser(['email' => self::REGISTERED_EMAIL, 'isLdap' => true]);
 
         $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<string, mixed> $properties
      */
@@ -114,38 +193,34 @@ private function givenAUser(array $properties): void
      * @throws Exception
      * @throws NotFoundExceptionInterface
      */
-    private function whenRequesting(string $login, string $email): void
+    /**
+     * @param array<string, mixed> $definitionsOverride
+     */
+    private function whenRequesting(string $login, string $email, array $definitionsOverride = []): void
     {
         $container = $this->buildContainer(
             IntegrationTestCase::buildRequest(
                 'post',
                 '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);
     }
 }

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);
     }
 
     /**