Password reset json endpoint reveals whether an email or username is in use

Hi there, I have reviewed and tested this patch.

Set up local testing site, Drupal version 9.4.0-dev, PHP 8.0.15.

Created test user Joe Blogs, ran the curl commands got the expected responses back (successfully recreated error).

Applied patch #6, ran curl commands again.
Running curl command for non existent user returns empty 200 and logs message in watchdog.
Running curl command for existing Joe Blogs user also returns 200 and logs to watchdog.
Running curl command for locked Joe Blogs account returns {"message":"The user has not been activated or is blocked."} as expected.

Looked over the patch code changes, seems ok to me but less confident in reviewing this.

The fail in #9 was a known random #3269085: [random test failure] Random test fail in EntityAutocompleteTest.
ShaunDychko added re-test on March 17th that passed.

Restoring #8's RTBC

Thanks for working on this important oversight from previous efforts to fix this bug! Hate to do this, but I don't think this is ready to commit. Point 1 is a dubious nit. Points 2 and 3 are in support of RTBC. But point 4 is why I think this at least needs more review, and possibly more work.

1. +++ b/core/modules/user/src/Controller/UserAuthenticationController.php
   @@ -244,10 +244,13 @@ public function resetPassword(Request $request) {
   +      $identifier = $credentials['name'];
          $users = $this->userStorage->loadByProperties(['name' => trim($credentials['name'])]);
   

   If we're going to add this as a local var, why not use it on the very next line?

2. +++ b/core/modules/user/src/Controller/UserAuthenticationController.php
   @@ -270,7 +273,10 @@ public function resetPassword(Request $request) {
   +    $this->logger->error('Unable to send password reset email for unrecognized username or email address %identifier.', [
   +      '%identifier' => $identifier,
   +    ]);
   

   At first, I wasn't sure this was in scope, but it's mentioned in the summary. We added similar logging at #1521996. This seems good.

3. +++ b/core/modules/user/src/Controller/UserAuthenticationController.php
   @@ -270,7 +273,10 @@ public function resetPassword(Request $request) {
   +    return new Response();
   

   I guess this is the equivalent of:

       // Make sure the status text is displayed even if no email was sent. This
       // message is deliberately the same as the success message for privacy.
       $this->messenger()
         ->addStatus($this->t('If %identifier is a valid account, an email will be sent with instructions to reset your password.', [
           '%identifier' => $form_state->getValue('name'),
         ]));
   

   which is what we added.

4. +++ b/core/modules/user/tests/src/Functional/UserLoginHttpTest.php
   @@ -527,10 +527,10 @@ protected function doTestPasswordReset($format, $account) {
        $account
          ->block()
   

   The next few lines of this test are:

       $response = $this->passwordRequest(['name' => $account->getAccountName()], $format);
       $this->assertHttpResponseWithMessage($response, 400, 'The user has not been activated or is blocked\
   .', $format);
   
       $response = $this->passwordRequest(['mail' => $account->getEmail()], $format);
       $this->assertHttpResponseWithMessage($response, 400, 'The user has not been activated or is blocked\
   .', $format);
   

   Doesn't that reveal the email address for blocked users exists? Is that not also a problem? Shouldn't we fix this as part of the same issue? That was part of what was originally fixed at #1521996: Password reset form reveals whether an email or username is in use for the web UI case.

Thanks again!
-Derek

I processed dww's feedback and pushed everything to a MR. I also replaced the The user has not been activated or is blocked error message with a 200 response and an error log. I updated the tests to check for 200 responses, but shouldn't we also test whether the errors are logged? Not sure how to do that though, I came across #3274834: Allow functional tests to fail or expect logged errors but that doesn't seem like an option yet.

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Ran the tests locally without to the fix to make sure they failed. The response was really long but did fail

Caused by
 ErrorException: Method "IteratorAggregate::getIterator()" might add "\Traversable" as a native return type declaration in the future. Do the same in implementation "Drupal\Core\Entity\ContentEntityBase" now to avoid errors or add an explicit @return annotation to suppress this message.

shouldn't we also test whether the errors are logged?

If that's something you want to test you could probably query the database.

Will move to NW for that but if it's actually not needed please send back.

Everything else looks good.

Rerolled for 11.x and added the missing test scenarios

Seems the reroll caused test fialures.

Wasn't the reroll, was the new test I added, I fixed it. Got an unrelated fail though, seems random, trying again.

Test bot seems to like this after all, back to review :)

Change looks good but I imagine there may be some pushback on

$logged = Database::getConnection()->select('watchdog')

But not sure if there's a trait that can be used but will see

Good work everyone!

@smustgrave I found it used like that 10 times across 4 files, not including the new addition, that's why I did it this way.

core/modules/comment/tests/src/Kernel/CommentIntegrationTest.php
core/modules/dblog/tests/src/Functional/DbLogTest.php
core/modules/dblog/tests/src/Kernel/DbLogTest.php
core/modules/system/tests/src/Functional/Form/StorageTest.php

Have no issue with it. Just have seen that get kicked back before.

Hiding patches and updating issue credits

Committed 98618df and pushed to 11.x. Thanks!

Backported to 10.1.x as this is an important security fix.

Added #3377275: Move \Drupal\Tests\system\Functional\Module\ModuleTestBase::assertLogMessage to a trait as a followup