User login flood lock doesn't clear when reset password as admin

@vijaycs85 Does the patch from #2 has the fix for the issue.

I would argue that the current behavior (not clearing flood after changing user password) is the correct one, and automatic resetting of flood after changing user password by admin leads to many questions: who is identified as admin? How do we distinguish changing password from user account form from saving User entity?

I would argue that the current behavior (not clearing flood after changing user password) is the correct one, and automatic resetting of flood after changing user password by admin leads to many questions: who is identified as admin?

Anyone with permissions to change someone else's password should be counted as a user admin.

How do we distinguish changing password from user account form from saving User entity?

We should be able to detect that the password field value has changed and only reset the flood table in that case.

Just ran into this one, but the patch here is quite out of date and I'm not sure about adding an event here. We already have user CRUD hooks, and the event could be confused with those, also a password reset isn't CRUD.

Instead I've just copied the logic wholesale. I think if we want to make this more generic, we should instead add a UserLoginFlood service with helpers for clearing the different identifiers based on configuration and etc. - that could then encapsulate setting the flood records as well as clearing them.

There's a further problem here that when uid_only is disabled, the identifier includes the IP address - and you need the full identifier to clear the flood records - since we don't have the user's IP address, we're stuck. So... we'd need to add a method to the flood service to clear identifiers by prefix - which would let us use $uid . '-' and clear all records for that user. We don't need that when the user themselves requests a password reset, so a working version of the patch would not have 1-1 matching logic anyway. This is easy for the database flood backend, but not necessarily for alternatives - and it'd need a new interface.

It would be possible to have a patch and test that works for uid_only and open a follow-up for the uid + ip version, depending on the API addition to flood.

Here's patch (up to where I realised the IP address problem), just added a @todo, and some initial test coverage that should fail.

Slight expansion of the test coverage so it tests the uid_only case too, and first.

Using the latest patch from #16, there are still a small edge issue.

As user 3, I reset user 3’s password. User 3’s user account is locked out after trying (in another browser) the wrong password. The password reset doesn’t reset the flood status on user 3.

As user 3, I reset user 4’s password. User 4’s account was locked after trying (in another browser) the wrong password. The password reset did reset the flood status on user 4.

So #992540: Nothing clears the "5 failed login attempts" security message when a user resets their own password means that in practice I don't think people would run into #19 - however I can't think of any reason not to clear the flood control when a user changes their own password.

Adding new patch to clear the flood when user changes their own password.
It also fixes the test case.

Hi @paulocs, Please review the patch it will fix the test case issues.

There's already an issue at #992540: Nothing clears the "5 failed login attempts" security message when a user resets their own password covering users resetting their own password, this issue is for when admins reset.

I have applied #24, #22, #16 but not working getting error can u tell me sharing screenshot ---

I can confirm patch #24 worked for me. Also for the password reset problem, where the flood table wasn't being cleared as well.

Patch #24 confirmed on core 9.2.1. Flood control cleared on user password reset, admin pw reset and admin pw reset when old pw == new pw.

NOTE: Had to manually apply patch due to file differences.

The last patch failed to apply on 9.3 so re-rolled it for 9.3

I set back the logic to reset the flood for only when the user password is changed by another user.

I also fixed the tests for it.

Looks good, a few nitpicks:

1.

--- a/core/modules/user/src/Entity/User.php
+++ b/core/modules/user/src/Entity/User.php
@@ -125,6 +125,19 @@ public function postSave(EntityStorageInterface $storage, $update = TRUE) {
[...]
+          // @todo: this doesn't work, and flood doesn't have a method to
+          // clear entries by prefix.
+          // Otherwise use the uid and IP address.

Not too sure about this @todo, or at least the phrasing. For me now it looks like the next lines won't work (which they do) and I don't see a real action about what needs to be done in the @todo.

I, disclaimer: Not a native speaker, would leave out the 2 lines of the @todo all together and rephrase it into something like

+          // Since flood doesn't have a method to clear entries by prefix, we 
+          // use the uid and IP address.

2.

--- a/core/modules/user/tests/src/Functional/UserPasswordResetTest.php
+++ b/core/modules/user/tests/src/Functional/UserPasswordResetTest.php
@@ -485,6 +485,56 @@ public function testUserResetPasswordUserFloodControlIsCleared() {
+    // The next request should *not* trigger flood control, since the
+    // password change should have cleared flood events for this user.
[...]
+    $this->drupalGet('user/login');
+    $this->submitForm($edit, 'Log in');
+    $this->assertSession()->pageTextNotContains('There have been more than 3 failed login attempts for this account. It is temporarily blocked.');

We now have successfully confirmed the error message isn't there any more, but still haven't got confirmation the previously-flood-blocked user has now successfully logged in. I think we need to add a $this->assertSession()->pageTextContains() looking for text that is there after a successful login?

We can't remove the @todo but it needs a better explanation and linking to an issue.

+++ b/core/modules/user/src/Entity/User.php
@@ -125,6 +125,19 @@ public function postSave(EntityStorageInterface $storage, $update = TRUE) {
+        }
+        else {
+          // @todo: this doesn't work, and flood doesn't have a method to
+          // clear entries by prefix.
+          // Otherwise use the uid and IP address.
+          $identifier = $this->id() . '-' . \Drupal::request()->getClientIP();

The problem here is it really doesn't work - the IP address of the administrator isn't going to be the same as the IP address of the person trying to log in, so we'll be clearing a flood entry that doesn't exist, and leaving the one that's blocking login.

I think we need a follow-up for this (which might depend on adding a 'clearByPrefix' method to the flood API).

Here is the follow-up issue #3225354: Add a clearByPrefix() method to the flood API

Reroll for #32.

Able to reproduce the issue by using the steps in IS
Tested and applied patch #47 on Drupal 10.1.x-dev
The patch applied cleanly.

The patch did work and the issue got resolved after applying the patch.
The user login flood lock gets cleared now after resetting the password by admin. And the user is able to login with the new password successfully.

Refer to the attached screenshots

The @todo should link to #3225354: Add a clearByPrefix() method to the flood API per #36/#37.

I'd also agree with #35 that we should assert something positive

Maybe we can postpone this on #3225354: Add a clearByPrefix() method to the flood API (which is currently RTBC) so we can truly say "User login flood lock doesn't clear when reset password as admin" is solved when we commit this?

#3225354: Add a clearByPrefix() method to the flood API is in, so we can drop the @todo here and use the new method instead. Will need an instanceof check.

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.

Reviewing MR 3241 - hiding the files to avoid confusion

Following the steps in the issue summary I can confirm the issue
Applying the patch fixes it.

MR has no open threads.
Test-only patch shows the test coverage works.

Looks good.

Posted few comments on the MR

Left a comment on MR.

I believe this is more confirmative that user is logged in. Than just asserting the response message on page.

$this->drupalGet('user/' . $this->account->id() . '/edit');
$this->assertSession()->statusCodeEquals(200);

Or could just use

$this->assertSession()->addressEquals('user/' . $this->account->id());

Don't have to check response message or do an unnecessary drupalGet

And @paulocs sorry didn't respond to the last message

Thanks!

Left a couple of questions/suggestions on the MR

Updated test as per review comment.

> Should we make sure the user isn't being blocked and that is what triggered the save event? I'm not sure blocking the user should clear flood entries.

I will leave it for more knowledgable person to answer it.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rebased MR #3241 to 11.x , attached patch for backup.

Think this is ready for committer review.

Though wonder if there is any security implication.

There sort of is but it's fine - if a user completely locks themselves out of their account, then they might contact an admin to reset their password, and once they've done so, that user needs to be able to log in again without waiting x amount of time.

The only security implication is that an admin edits a user account that's for some reason already locked out as a co-incidence, and this wipes the flood attempts - but we still have flood control on the account overall, so it'll just kick in again on the next three incorrect attempts. As a user admin you'd have to continually be resaving user accounts to bypass it overall.