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

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 the test changes to match the D10 version more closely. 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...

The last submitted patch, 50: 2880910-50_test-only.patch, failed testing. View results →

looks good @poker10,

1. tests prove the need,
2. 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 🐛 Nothing clears the "5 failed login attempts" security message when a user resets their own password Fixed

...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 the @alexpott 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.

Automatically closed - issue fixed for 2 weeks with no activity.