Password reset form error makes no sense when the account is locked

An issue summary needs to describe what the bug is and provide any information necessary to replicate the issue on a plain Drupal site.

Also, looking at the provided patch, this does not seem a bug report.

       if (!$this->flood->isAllowed('user.password_request_user', $flood_config->get('user_limit'), $flood_config->get('user_window'), $identifier)) {
+        $form_state->setErrorByName('name', $this->t('Too many password recovery requests from your IP address. It is temporarily blocked. Try again later or contact the site administrator.'));
         return;
       }

user.password_request_user is not a limit per IP, but per user. The Too many password recovery requests from your IP address. message is wrong; it should say the user has done too many password recovery requests.

thanks apaderno
i will update summary.

fix message

There is no concept of locking in core. Please update the issue to use 'blocked'/'blocking'/'active' where appropriate.

+++ b/core/modules/user/src/Form/UserPasswordForm.php
@@ -194,6 +194,7 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
+        $form_state->setErrorByName('name', $this->t('Too many password recovery requests from your account. It is temporarily blocked. Try again later or contact the site administrator.'));

Grammar nits.

Its not 'from' your account, it would be 'for', as the user is presumably not authenticated as this account.

And we do not know it is 'your' account, use 'this', as we do not know the user owns this account. This contrasts to the IP address language this message was likely based from, where we can roughly confirm it is 'your IP address'.

Needs tests.

hi dpi
the flood check identifier is a user account id. so not ip address

// Register flood events based on the uid only, so they apply for any
// IP address. This allows them to be cleared on successful reset (from
// any IP).

Was tagged for issue summary update and tests which still need to happen.

For the issue summary if stuck would recommend using the default template for issues.

update new patch name and fix message

Ticket was tagged for issue summary update and test both which still appear needed. Thanks

thanks smustgrave
The summary and test steps have been updated

Thanks but need an actual test case or assertion in the code.

add test case.

new patch

Thanks. FYI you don't have to run all the tests just the default one is fine.

Also all patches should be attached with an interdiff.

+  public function testUserResetPasswordEmailFloodControl() {
+    $account = $this->drupalCreateUser();
+    for ($i = 0; $i < 5; $i++) {
+      $this->drupalGet('user/password');
+      $edit = ['name' => $account->getEmail()];
+      $this->submitForm($edit, 'Submit');
+    }
+
+    // The next request should trigger flood control.
+    $this->drupalGet('user/password');
+    $edit = ['name' => $account->getEmail()];
+    $this->submitForm($edit, 'Submit');
+    $this->assertPasswordEmailOrUsernameFlood();
+
+    // delete test account.
+    $account->delete();
+  }
+
+  /**
+   * Tests password reset flood control for one username.
+   */
+  public function testUserResetPasswordUsernameFloodControl() {
+    $account = $this->drupalCreateUser();
+    for ($i = 0; $i < 5; $i++) {
+      $this->drupalGet('user/password');
+      $edit = ['name' => $account->getAccountName()];
+      $this->submitForm($edit, 'Submit');
+    }
+
+    // The next request should trigger flood control.
+    $this->drupalGet('user/password');
+    $edit = ['name' => $account->getAccountName()];
+    $this->submitForm($edit, 'Submit');
+    $this->assertPasswordEmailOrUsernameFlood();
+
+    // delete test account.
+    $account->delete();
+  }

So way the functional tests work each method is it's own bootstrap and install. So could this be converted to kernel tests? If not could they be added to existing tests? At minimum should combine these two

I referred to two methods

File: core/modules/user/tests/src/Functional/UserPasswordResetTest.php

1. testUserResetPasswordIpFloodControl
2. testUserResetPasswordUserFloodControl

i think this can be add to kernel test

There is an attach interdiff file below. thanks smustgrave

The last submitted patch, 25: 3353185-password-reset-form-interdiff.patch, failed testing. View results →

If you were trying to upload a test only patch should also include the full patch.

Wording for the patch should also change.

Typically [issue-number]-[optional:summary]-[comment-number].patch

Tests only patches will be [issue-number]-[optional:summary]-[comment-number]-tests-only.patch

Uploaded all patches again

I have applied the #29 📌 Password reset form error makes no sense when the account is locked Needs work patch on 9.5.x dev version and it works fine .

The person who provided the patch cannot change status to Reviewed & tested by the community.

       $identifier = $account->id();
       if (!$this->flood->isAllowed('user.password_request_user', $flood_config->get('user_limit'), $flood_config->get('user_window'), $identifier)) {
+        $form_state->setErrorByName('name', $this->t('Too many password recovery requests for this username or email. It is temporarily blocked. Try again later or contact the site administrator.'));
         return;

It is temporarily blocked. is about the account, but the previous sentence ends with for this username or email, which makes it a substitution for this username or email.

I have added patch for Drupal 9.5.x and address #33..
please review..

Fixed #34 Cc Failed

Fixing custom command failure issues in #35, please review it.

Thanks!

The last submitted patch, 37: 3353185-37.patch, failed testing. View results →

I fixed issue #41 Please review

       if (!$this->flood->isAllowed('user.password_request_user', $flood_config->get('user_limit'), $flood_config->get('user_window'), $identifier)) {
+        $form_state->setErrorByName('name', $this->t('Too many password recovery requests for this (%name) username or email. It is temporarily blocked. Try again later or contact the site administrator.', ['%name' => $name]));
         return;

It is preferable to say The account has been temporarily blocked. Otherwise the sentence would be understood as This username or email is temporarily blocked. which does not make much sense since it is the account that is blocked.
I find it preferable to say Too many password recovery requests for %name.

Agree with #43

I have updated the patch with latest update suggested by user @apaderno and user @smustgrave, I would appreciate if someone can review these updates and provide feedback

       if (!$this->flood->isAllowed('user.password_request_user', $flood_config->get('user_limit'), $flood_config->get('user_window'), $identifier)) {
+        $form_state->setErrorByName('name', $this->t('Too many password recovery requests for (%name) this username or email. The account has been temporarily blocked. Try again later or contact the site administrator.', ['%name' => $name]));
         return;
       }

The suggested words for the first sentence were Too many password recovery requests for %name. Actually, to make it a sentence, the words should be Too many password recovery have been requested for %name.

Let me work on the #46

I have fixed the #46 issue. Please review.

       if (!$this->flood->isAllowed('user.password_request_user', $flood_config->get('user_limit'), $flood_config->get('user_window'), $identifier)) {
+        $form_state->setErrorByName('name', $this->t('Too many password recovery have been requested for (%name) this username or email. The account is temporarily blocked. Try again later or contact the site administrator.', ['%name' => $name]));
         return;

I apologize what I suggested was not clear: The string passes to $this->t() (or t()) needs to contain the following text (and only this).

Too many password recovery have been requested for %name. The account is temporarily blocked. Try again later or contact the site administrator.

I have fixed the #49 issue. Please review.

The patch is fine for me. Let us wait for somebody else's opinion.

You cannot write "The account has been temporarily blocked." because then you say that this account exists and you have a privacy data breach!

But this means you have to count the password-requests for every mail address - regardless of whether this account exists or not. If you count only the requests for existing accounts a malicious robot can examine which user accounts exists or not. Imagine this e.g. for a HIV/AIDS Selfhelp portal!

Please be careful with such information to avoid an information disclosure and a privacy violation.

Agreed will have to tweak the language

In that case, the message should not even say Too many password recovery have been requested for %name. because that could possibly give information about the account.

Imagine I tried once a password recovery using a username or an email and I got that message. I would gather that somebody else tried to recover the password using the same username or email, which could make me think the username or the email is effectively used (or that trying that username/email is a common practice for somebody who tries to find out which accounts exist on a site). If then I know the site considers attempts made from the same IP address, and I am at my work place, that message could help me to guess username or email used by a colleague.

To avoid giving information about existing accounts, the message should be the same in any case, whether the username/email is not associated to any account, too many attempts have done, or the username/email is associated with an account and the recovery email has been sent. For example, showing this message in every case would not allow to understand if the username/email is effectively associated with an account.

If %identifier is a valid account, an email will be sent with instructions to reset your password, if there have not been too many attempts to recover the password.

We may (optionally) consider sending an email when an account is suspended.
Then we have no information disclosure and the account is informed that someone tried something bad and can inform the administrator.

Actually, the account for which the password recovery is asked is never blocked. The code in UserPasswordForm.php increases the number of requests done to recover the password; that eventually avoids further requests can be done.
This means that the The account is temporarily blocked. part is wrong. Taking off that part, the message would be Too many password recovery have been requested for %name. Try again later or contact the site administrator. which could still give out information about a possible account.

As for sending an email when the number of password recovery requests reached the limit, that would mean flooding the account's email address with a new email each time a new password recovery request is done after the limit is reached. An email is already sent to the account's email address for the password recovery, and that email says:

[user:display-name],

A request to reset the password for your account has been made at [site:name].

You may now log in by clicking this link or copying and pasting it into your browser:

[user:one-time-login-url]

This link can only be used once to log in and will lead you to a page where you can set your password. It expires after one day and nothing will happen if it's not used.

-- [site:name] team

Eventually, that email message should also contain a sentence that explains what to do when the person who is reading the email did not request the password recovery (If you did not request a password recovery […].).

One suggestion that I think of to frame the error message considering all the constraints and concerns raised in the thread is :

"If {account ID} is a valid and active account, an email will be sent with instructions to reset your password."

account ID will be the value entered by user in "username or Email" field.

Kindly share thoughts on using this... or some thing on this line.

In case you find this issue trying to figure out what "Password reset form was submitted ..." means, and realize you need to clear the flood table, you can do it with this:

drush sql:query "DELETE FROM flood;".

Re: #58

"If {account ID} is a valid and active account, an email will be sent with instructions to reset your password."

I am not sure if this is entirely correct. Account under flood control is active and its status is not "blocked", as mentioned by @apaderno in #57. So we are missing at least one possible scenario in the proposed text.

The idea with sending an email to user from #56 is interesting, however I think we would need to ensure that the email is sent only once. Otherwise it would be possible to spam user with such emails.