[D7] Nothing clears the "5 failed login attempts" security message when a user resets their own password

As drupal7 requires php5.3 the syntax of arrays has to be the old-school one, that's the reason why the patch failed.
I'm posting the fixed patch and sending it to review.

Sure, here you go (nothing else, just that i've mentioned :))
If you want to work on it, i let you, then will review it, fair enough?

Applied patch from #9. Patch applied successfully but found the issue is not yet fixed. When the user gets the message "Sorry, there have been more than 5 failed login attempts for this account. It is temporarily blocked. Try again later or request a new password" and admin re-sets the password in admin panel, user still gets the same error when tried to login with the new password.

Whether changing user password should also reset the flood, is an open question (https://www.drupal.org/node/2881572#comment-12106305).

The patch from #9 is exact backport of #992540: Nothing clears the "5 failed login attempts" security message when a user resets their own password, moving to RTBC

Applied patch from #9 and it worked perfect.Comment #14 clarifies my comment form #11, its a open question Whether changing user password should also reset the flood.

I've also applied it and works like a charm!

+          flood_clear_event('failed_login_attempt_ip');

This looks like it has the same security issue that the Drupal 8 patch had - see https://www.drupal.org/node/992540#comment-12116412

Also, the code comments in the patch are repetitive since they are missing the changes @catch made on commit at https://www.drupal.org/node/992540#comment-12099362

I'm also tagging this for an issue summary update - normally I don't care that much about keeping issue summaries up to date, but this one is really confusing since it actually describes #2881572: User login flood lock doesn't clear when reset password as admin instead :) The bug here is for the case where a user resets their own password.

Updated issue summary.

Updated the patch based on the feedback from David.

I've tested the patch on a local, it works well.
But i'd like to change a small detail in the test in order to properly follow how https://api.drupal.org/api/drupal/modules%21user%21user.module/function/... works as it uses the default value of user_failed_login_user_limit variable 5, so i think it would be better if test could check 5 instead of 3, so i've modified the test in a new patch that i'm attaching now with an interdiff.
-- There is no other changes, only test gets more 'proper'.
Cheers,
Balazs.

I think it does not matter if we test with a 3 or 5 attempts setting - it only makes the test run longer. But I also don't really care :)

Looks good otherwise, we are using this patch in production for a week now without problems.

Applied patch from #22 and it worked proper.

Quick review / small nits:

> passowrd (typo)

+    // A password reset does not clear the global IP flood control.
+    $this->resetUserPassword($user1);
+    $this->drupalLogout();
+    $this->assertFailedLogin($user1, 'ip');

Seems a bit unclear from the comment. Q: Is this order correct?

'There have been more than ' . $user_limit . ' failed login attempts for this account.'

Do we really need to know the number we set previously? (this is the only place where it's used like this in tests once committed i believe)

And:
Let's stick to 5, seems like a good number. (and also used in other tests as a default).

One of my clients just ran into this bug, which took way too long track down and then flush the flood table manually. They were following the given instructions to ‘clear’ the block, so they didn’t call me for several hours, leading to a fairly extreme level of frustration on their part. So...

A few things?

A)

Shouldn't the Title be something like?:

[D7] Clear user specific flood events upon any password reset for that user.

e.g. if an administrator resets the password, the the user specific flood control should be reset as well.

One of their administrators reset the ‘blocked’ administrator’s password, which did NOT clear flood control.

If the website messages say resetting the password clears the block, then any password reset should clear the block, or the messaging needs to be change to identify [see C) though] what exactly does, and does not, clear flood control.

B)

I'd need to dig deeper (and maybe it doesn’t matter for testing?) but should this be hardcoded?

   function testPerUserLoginFloodControl() {
+    $user_limit = 5;

As the Flood control module, https://www.drupal.org/project/flood_control , makes use of a variable for that setting.

See: "Failed login (username) limit" on https://www.drupal.org/files/images/570142-flood-control.png

C)

Probably shouldn’t be addressed here, but:

> 'There have been more than ' . $user_limit . ' failed login attempts for this account.'

Why are we telling anyone, anywhere, how many failed login attempts there were? That just invites bot abuse. Wouldn’t a more security conscious message would be something like:

You account as been blocked for an excessive number of failed login attempts. Please reset your password at {example.com/…} to be able to login again.

Best,
Michael

e.g. if an administrator resets the password, the the user specific flood control should be reset as well.

Could be part of the patch, or a new followup issue. Prefer the latter, because this has been open for some time now and the flood_unblock module provides a simple interface for clearing these already (but implementing this seems logical enough).

Probably shouldn’t be addressed here, but:

That bit is only used in testing.

Hi Rob,

>> if an administrator resets the password, the the user specific flood control should be reset as well.
> Could be part of the patch, or a new followup issue. Prefer the latter,

I'll admit I'm not familiar with the user system (did a quick search can’t find this though), but ...

Is there a single password reset confined to a master/single api call in D7?

If so

flood_clear_event('failed_login_attempt_user', $identifier);

could be moved to there, so that any password reset from anywhere clears flood. Which just makes everyone’s life easier...

If password resets are scattered all over the place (which it somewhat seems like), then I agree, put this patch in as is and I’ll open a separate issue for administrator resets.

Best,
Michael

I'm attaching a patch that corrects the misspelling of password, and nothing else. I'm also setting it for testing, since it's been several months.

Has working tests, spelling is fixed, looks good thanks!

RTBC!

Bumping to 7.61. This didn't make it into 7.60

+++ b/modules/user/user.pages.inc
@@ -156,6 +156,20 @@ function user_pass_reset($form, &$form_state, $uid, $timestamp, $hashed_pass, $a
+          if (variable_get('user_failed_login_identifier_uid_only', FALSE)) {
+            // Clear flood events based on the uid only if configured.
+            $identifier = $account->uid;
+          }
+          else {
+            // The default identifier is a combination of uid and IP address.
+            $identifier = $account->uid . '-' . ip_address();
+          }
+          // Only clear the user specific flood events. We cannot clear the more
+          // broad IP address flood events because that would open a
+          // vulnerability where an attacker with a valid account could use that
...
+          flood_clear_event('failed_login_attempt_user', $identifier);

From our experience, the user can be blocked. For example, the user status can be 0 after 5 failed attempts.

However, the user will try to reset the password.

I think only load the status 1 user is better, and leave the blocked user to a different method.

This hasn't made it into D8 yet.

@pandaski , I don't understand your last comment, can you please clarify, which line of code are you referring to? Steps to reproduce? If you have a suggestion for an improvement, can you make a patch and interdiff for it?

We are using the patch in the latest GovCMS7 distribution

It is good to go, thanks everyone

This patch is still clean.

On a side note, yesterday I came accross an issue in D8 where drush uublk didn't actually remove the records from the flood table for the desired user. Maybe a problem with drush.

these types of usability issues should be fixed in D8/D9/D7, not sure why we leave these types of issues for so long.

people need to be able to reset their passwords, flooding also needs to expire or be unfloodable

Postponed on the D9 issue not being fixed:
#992540: Nothing clears the "5 failed login attempts" security message when a user resets their own password

So why is this issue postponed?

My assumption is that it was set as postponed because the issue should be fixed first in newer versions of Drupal. At least, I've seen that as a reason in the past, namely here: #1699378-66: Allow tokens in entity reference views selection arguments. That implies that the policy is that something cannot be fixed in an older version if it hasn't been fixed yet in the latest version.
Maybe that made sense back in 2013, when Views was in the process of being ported to Drupal 8 - and then it could hold up people upgrading from D7 to D8 if the D8 version would lack a feature that the D7 version had? But in this case, I would vote for setting this back to RTBC. Also because it is a bug and not a feature - so I doubt that this particular thing would cause someone upgrade issues if the bug only gets fixed in D7 for now.

The D10 issue is in, so we can continue here to backport that for D7 :)

Patch #30 does not apply anymore, so it needs reroll. We also need to check if the patch from #30 matches what was committed in D10.

straight up re-roll of patch 30, no need for interdiff, I made zero change.

it had fuzz, that's it, now this patch resolves the fuzz.

Patch was rerolled, there was only fuzz, no interdiff needed.

Justification for RTBC:

• Fixed in D10.0.x
• Fixed in D10.1.x
• Fixed in D9.5.x
• This patch has a test and passes all tests.
• The original issue was created 8 Dec 2010 at 10:16 EST
• Another win for the team!

Thanks! I think this looks overally good. I have only two points / nitpicks:

diff --git a/modules/user/user.test b/modules/user/user.test
@@ -371,16 +371,23 @@ class UserLoginTestCase extends DrupalWebTestCase {
   function testPerUserLoginFloodControl() {
+    $user_limit = 5;
+
     // Set a high global limit out so that it is not relevant in the test.
     variable_set('user_failed_login_ip_limit', 4000);
     // Set the per-user login limit.
-    variable_set('user_failed_login_user_limit', 3);
+    variable_set('user_failed_login_user_limit', $user_limit);

1. Is there a reason why we are increasing the limit from 3 to 5 in this test? It does not seems the same as in D10 patch, so I am curious if there is a reason for this change.

diff --git a/modules/user/user.test b/modules/user/user.test
@@ -371,16 +371,23 @@ class UserLoginTestCase extends DrupalWebTestCase {
+    // A password reset does not clear the global IP flood control.
+    $this->resetUserPassword($user1);
+    $this->drupalLogout();
+    $this->assertFailedLogin($user1, 'ip');

2. We should probably include the full comment from D10 patch (e.g. // A login attempt after resetting the password should still fail, since the IP-based flood control count is not cleared after a password reset.).

-------------------

Comparing the patch with D10 there is also one other difference, where D10 is clearing also user.http_login flood event, but it seems like we do not have such flood event in D7, so it should be OK as it is.

diff --git a/core/modules/user/src/Controller/UserController.php b/core/modules/user/src/Controller/UserController.php
@@ -235,6 +235,17 @@ public function resetPassLogin($uid, $timestamp, $hash, Request $request) {
+    $this->flood->clear('user.failed_login_user', $identifier);
+    $this->flood->clear('user.http_login', $identifier);

I have added this issue to the list of potential candidates for the next release. If we can check/fix points from #48, I think this have a fair chance to be committed.

I have gone ahead and updated changes in tests to match the D10 version more closely (other than that the patch is the same). I have also reverted the change which increased the number of failed logins from 3 to 5. This was explained in #22, but even so I think we should keep the changes as simple as possible. And given that this change was not in the D10 commit, I suggest to open a follow-up to make such a change in D10 + D7, if needed. Let's focus here only on changes relevant to the issue summary. Thanks!

For the reference, here is the D10 commit: https://git.drupalcode.org/project/drupal/-/commit/a8729aad07ec430e0c217...

looks good @poker10,

1. tests only fails and proves the need,
2. fix with test passes on every supported version of PHP,
3. aligned with D10,

thanks so much!

We'll take the win!

Thanks. Adding a tag for a final review.

One thing about this is bothering me slightly.. in the changes to user.test

-    // A successful login will reset the per-user flood control count.
+    // We're not going to test resetting the password which should clear the
+    // flood table and allow the user to log in again.
     $this->drupalLogin($user1);
     $this->drupalLogout();

Perhaps it's just been a long week, but I don't understand the reason for this change to the comment.

It was committed in the parent issue, and looks like it was added here:

https://www.drupal.org/project/drupal/issues/992540#comment-13887545

...but I'm still not entirely sure why.

Good catch @mcdruid! Not sure if @joseph.olstad recalls from the D10 issue why this was changed, but I reviewed the user_login_final_validate() and the flood control for user is reset in case of successfull login:

  elseif (isset($form_state['flood_control_user_identifier'])) {
    // Clear past failures for this user so as not to block a user who might
    // log in and out more than once in an hour.
    flood_clear_event('failed_login_attempt_user', $form_state['flood_control_user_identifier']);
  }

Therefore the original comment seems correct and as there are no changes to the code around, I think that you are right and we should skip this comment change.

I saw my name being mentioned.

Here's a new patch with interdiff to address the concern.

Thanks @joseph.olstad - we were wondering if you remembered the reason for the change to that comment?

On the other hand, sometimes I don't remember what I had for lunch.

We should file a follow-up to restore the original comment in D10/11.

Thanks everybody!

we were wondering if you remembered the reason for the change to that comment?

The answer to that is because alexpott says this in his review from #992540-172: Nothing clears the "5 failed login attempts" security message when a user resets their own password, point 3:

+++ b/core/modules/user/tests/src/Functional/UserLoginTest.php
@@ -109,6 +123,12 @@ public function testPerUserLoginFloodControl() {
+    $this->resetUserPassword($user1);
+    $this->drupalLogout();

This results in a successful login and logout. Which doesn't match the comment above. We need a new comment saying we're no going to test resetting the password which should clear the flood table and allow the user to log in again.

I haven't checked the context in which this was said, but the comment change appears to be based on that part of the review.

Ah, well spotted @MegaChriz

I have a feeling perhaps there was a small typo in that suggested comment and it should have said "we're now going to test.." whereas instead it was mistakenly corrected to say "we're not going to test.." - would that make sense?

Reading @alexpott's comment again and looking at the context - yes, it looks like a typo (missing letter) and also it looks like that the comment was misplaced. Alex referenced this part of the code:

+    $this->resetUserPassword($user1);
+    $this->drupalLogout();

And the note was saing:

We need a new comment

. So I think this new comment was meant to be placed above these two lines, which would make sense. And the original comment should have been untouched, because there is no password reset there.