I have a user who forgot his password, and he started getting the "5 failed attempts" message. So I go in and reset the password manually as an admin.

But the new password will not work, and he continues to get the "5 failed attempts" message. The only thing we could do was delete his user, then recreate it.

Once their password has been changed in the interface by an admin, it should clear the security block, correct? Or is there a manual way to clear the security block? Because the user still says "active".

Files: 
CommentFileSizeAuthor
#50 reset_flood_limit_on_password_reset-992540-50.patch4.47 KBkid_icarus
PASSED: [[SimpleTest]]: [MySQL] 40,241 pass(es).
[ View ]
#50 interdiff.txt3.42 KBkid_icarus
#49 reset_flood_limit_on_password_reset-992540-49.patch4.49 KBkid_icarus
PASSED: [[SimpleTest]]: [MySQL] 40,240 pass(es).
[ View ]
#45 reset_flood_limit_on_password_reset-992540-44.patch6.57 KBkid_icarus
FAILED: [[SimpleTest]]: [MySQL] 40,240 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#45 interdiff.txt3.74 KBkid_icarus
#44 reset_flood_limit_on_password_reset-992540-44.patch4.22 KBrickmanelius
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch reset_flood_limit_on_password_reset-992540-44.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#44 interdiff.txt1.23 KBrickmanelius
#43 reset_flood_limit_on_password_reset-992540-43.patch4.22 KBrickmanelius
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch reset_flood_limit_on_password_reset-992540-43.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#41 reset_flood_limit_on_password_reset-992540-41.patch4.23 KBrickmanelius
PASSED: [[SimpleTest]]: [MySQL] 40,147 pass(es).
[ View ]
#36 reset_flood_limit_on_password_reset-992540-36.patch4.09 KBMatt V.
PASSED: [[SimpleTest]]: [MySQL] 35,437 pass(es).
[ View ]
#34 reset_flood_limit_on_password_reset-992540-34.patch4.21 KBMatt V.
PASSED: [[SimpleTest]]: [MySQL] 35,449 pass(es).
[ View ]
#23 992540-3-reset_flood_limit_on_password_reset-drush.patch4.15 KBjec006
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 992540-3-reset_flood_limit_on_password_reset-drush.patch. See the log in the details link for more information.
[ View ]
#20 992540-3-reset_flood_limit_on_password_reset.patch4.16 KBjec006
PASSED: [[SimpleTest]]: [MySQL] 33,013 pass(es).
[ View ]
#18 992540-3-reset_flood_limit_on_password_reset.patch4.2 KBjec006
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 992540-2-reset_flood_limit_on_password_reset_0.patch.
[ View ]
#16 992540-2-reset_flood_limit_on_password_reset.patch4.2 KBjec006
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 992540-2-reset_flood_limit_on_password_reset.patch.
[ View ]
#14 992540-reset_flood_limit_on_password_reset.patch1.01 KBjec006
PASSED: [[SimpleTest]]: [MySQL] 35,788 pass(es).
[ View ]
#8 992540.patch4.32 KBswentel
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable SimpleTest.
[ View ]

Comments

Subscribing. (Using D7rc4 here.)

How long is "try later"? After getting locked out?
Can this be reset via PHPAdmin?

The default is 21600 or 6 hours. The time be set as less by adding a $conf variable to settings.php for the user_failed_login_user_window variable. There is no UI in core to clear the flood, although contribute could introduce such a feature,

Thank you for that info!

Version:7.0-rc1» 7.x-dev

Well system_cron() clears the flood table. So when the expiration time for the failed attempts are old enough, they will be gone. We might turn this into a feature request to clear the flood if a user

a) requests a new password
b) or an administrator changes the user

thoughts ?

@swentel
Seems like some kind of ability to clear a user for access to a site prior to the imposed expiry would be a great feature IMHO.

Category:bug» feature

I agree. Would someone work on a patch?

Assigned:Unassigned» swentel

I'll take this up tonight.

Status:Active» Needs review
StatusFileSize
new4.32 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable SimpleTest.
[ View ]

Here's a patch. Clears the flood event on password reset validation and on the user/%/edit screen. Comes with tests.

I've tested and reviewed this patch, everything looks in order.
However, it won't pass tests on my machine, not sure if its the test or my environment. Anyone else?

#8: 992540.patch queued for re-testing.

Status:Needs review» Needs work

(re-posted from 1065464): This patch would allow anyone with an account to completely bypass IP-based flood protection. Clearing the per-user flood controls should be fine, but if you clear the IP limit it means that Alice can have unlimited guesses at Bob's password by resetting her own password every 50 attempts. One resolution might be to add another per-IP flood limit on password resets.

I see the point grendzy is making in #11, however in the example, Bob's account itself would still be locked out for the 6 hour time frame regardless of how many times Alice reset her own password. Right?

Status:Needs work» Needs review

Oh, I see. I think we're covered then.

StatusFileSize
new1.01 KB
PASSED: [[SimpleTest]]: [MySQL] 35,788 pass(es).
[ View ]

I've attached a patch that does what was discussed in 11

It ONLY clears the flood for the user - not for the full IP, when the link is clicked to reset the password.

#fail patch with debug code ... will reroll - want to actually incorporate a bit more out of the previous patch - Think it needs to check the variable for which user $identifier it should use though.

WIll also attach the simple tests.

StatusFileSize
new4.2 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 992540-2-reset_flood_limit_on_password_reset.patch.
[ View ]

Alright - here is a new patch that does what was being done in 8 but with checking for the correct type of identifier to clear.

Status:Needs review» Needs work

The last submitted patch, 992540-2-reset_flood_limit_on_password_reset.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.2 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 992540-2-reset_flood_limit_on_password_reset_0.patch.
[ View ]

Trying again

Status:Needs review» Needs work

The last submitted patch, 992540-2-reset_flood_limit_on_password_reset.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.16 KB
PASSED: [[SimpleTest]]: [MySQL] 33,013 pass(es).
[ View ]

Ok, fixed all the issues this time I think ... sorry for the back and forth

It will fail again I'm afraid, you need to make sure you create the right patch format (either git apply or with something else, but I'm missing the handbook pages)

Yah, before I was using diff - which made it fail - this one applied cleanly on my environment with git apply -v

so .. hopeful

StatusFileSize
new4.15 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 992540-3-reset_flood_limit_on_password_reset-drush.patch. See the log in the details link for more information.
[ View ]

Attaching a version that will work with drush make

Status:Needs review» Needs work

The last submitted patch, 992540-3-reset_flood_limit_on_password_reset-drush.patch, failed testing.

Subscribing. Shoudn't this be applied to 8.x-dev first?

Status:Needs work» Needs review

Patch in 20 works.

Version:7.x-dev» 8.x-dev
Assigned:swentel» Unassigned

Status:Needs review» Needs work

The last submitted patch, 992540-3-reset_flood_limit_on_password_reset-drush.patch, failed testing.

Status:Needs work» Needs review

#20 still applies against 8.x

Is the first chunk of this patch clearing the flood limit when the user requests a password reset, or when the properly respond to the email by activating the link w/ the token?

It is in the function that is called to log the user in with the link from the reset email - so simply requesting will not remove the flood limit.

Can we get a RTBC for the patch against 8.x so this can get in?

StatusFileSize
new4.21 KB
PASSED: [[SimpleTest]]: [MySQL] 35,449 pass(es).
[ View ]

I modified the last patch, just adjusting the paths slightly, to get it to apply to the latest 8.x-dev. From there, it worked great for me.

I also tried applying the tests separately. Before the rest of the patch gets applied, the tests fail. Once the rest of the patch gets applied, the tests pass.

I re-rolled the patch, against the latest 8.x-dev. It looks RTBC to me, but I'll leave it up to someone else to decide, since I re-rolled it.

The IP-limit should not be cleared. Clearing the IP flood events would allow an attacker with a valid account on a target site to try a list of weak passwords for every user account on the site. Everytime the IP flood limit would be reached, the attacker only has to login / follow a password reset link to a his valid account and continue.

StatusFileSize
new4.09 KB
PASSED: [[SimpleTest]]: [MySQL] 35,437 pass(es).
[ View ]

Here's an updated patch with the code for clearing the IP flood event removed.

This issue occurred for me as well today in D7.14... on user 1! I tried to unblock the account through the people list, though nowhere on the user's profile the block was stated, but this didn't work. Now waiting for the six hour default time to pass and see if this is fixed.

Please fix this in the stable core releases asap! I recommend upping the priority of this issue and make it a bug report rather than a feature request.

Applied the patch from #20 to a D7 installation to fix this issue. This is a pretty serious issue on sites with many user accounts like community sites. Would be very nice if this could be patched in D7 before it's ported to D8.

I would really like to see #20 get implemented in the D7.15 release. What would it take to do this?

I'm pinging this not to 'subscribe', but to put it at the top of my heap as I need this as well and wil review shortly.

StatusFileSize
new4.23 KB
PASSED: [[SimpleTest]]: [MySQL] 40,147 pass(es).
[ View ]

Technically patch #36 works in that the code applied to D8 dev does result in a successful simple test result and I can validate that it does clear failed login attempts, etc. But the test file moved from core/modules/user/user.test to core/modules/user/lib/Drupal/user/Tests/UserLoginTest.php. So I'm attaching a slightly edited re-roll of #36 to account for this.

Technically I'd like to mark this RBTC, but the slight edits require another person to check this out for D8 so we can get the D7 backport ASAP! :)

Status:Needs review» Needs work

Thanks @rickmanelius!

+++ b/core/modules/user/lib/Drupal/user/Tests/UserLoginTest.php
@@ -97,6 +97,67 @@ class UserLoginTest extends WebTestBase {
+  ¶

There's trailing whitespace here and in a few other spots.

Also, I couldn't get the patch to apply cleanly at HEAD which as of now is 6e7a13a. I'm tired though, so gremlins could be messing with me.

Status:Needs work» Needs review
StatusFileSize
new4.22 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch reset_flood_limit_on_password_reset-992540-43.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Clearing out whitespace nastiness as noted from #42. Resubmitting

StatusFileSize
new1.23 KB
new4.22 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch reset_flood_limit_on_password_reset-992540-44.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Trying my hand at interdiffs... resubmitting as patch 44. Thanks @kid_icarus for the information on how/why do to that! :)

StatusFileSize
new3.74 KB
new6.57 KB
FAILED: [[SimpleTest]]: [MySQL] 40,240 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

@rickmanelius Sweet! The interdiff looks good. It looks like #44 didn't apply cleanly to HEAD. Re-rolled.

Status:Needs review» Needs work

The last submitted patch, reset_flood_limit_on_password_reset-992540-44.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, reset_flood_limit_on_password_reset-992540-44.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.49 KB
PASSED: [[SimpleTest]]: [MySQL] 40,240 pass(es).
[ View ]

Ok, so it looks like #1499596: Introduce a basic entity form controller got in the way of this one. This makes the porting a little bit trickier. See http://drupal.org/node/1734556 for more information

I think this patch should do the trick, here the re-roll. I don't really know if an interdiff is possible on this one? :)

StatusFileSize
new3.42 KB
new4.47 KB
PASSED: [[SimpleTest]]: [MySQL] 40,241 pass(es).
[ View ]

Some coding standards/commenting standards cleanup.

As a sitenote, even the superadmin gets blocked for 6 hours by default. And as mentioned nothing clears that. No reset, password reset or what ever. The superadmin (user 1) should never blocked for more that 5 minutes or so.

If your patch could test for uid:1 and if the flooding protection is in effect set the timeout for uid:1 to 5 minutes or so. I had a hard time to test all kinds of password resets just to find out that this is a well known issue. Drives me mad.

Actually, the superuser gets unblocked when you clear the flood table, but that's not desired behaviour, as you'll unblock everyone then.

Hey @ronan.orb. Perhaps we can get this in as is (already RBTC) and spin off a second issue to address the uid 1. The reason is because there might be some additional reasons for keeping it that long. Besides, can you create drush one time login link?

@rickmanelius:

I tried:

* copy the password from a local installation to the development server
* reset password via drush
* create onetime login via drush
* set the password as an other user with similar role based rights
* creating a password string via php file as mentioned on other websites.
* skipping the use of user_login_final_validate(..) (I searched for the error phrase)

Nothing except cleaning the flood table let me logged in as superadmin. There was not even mentioned how long I was gonna be blocked. When I found out that the default ist 6 hours I was kind of shocked.

Hi @ronan.orb,

I think that's going to have to get spun off to a separate ticket as it's going to require some discussion and possibly a new feature whereas this ticket is simply making sure the password resetting is working for the general use case of a user. The user 1 scenario will require tests specific to that as well.

Patch review question. Since myself and @kid_icarus are essentially creating/modifying the current version of the patch, can either of us review this and set as RBTC or do we need a 3rd person now to do the final pass on this?

If not, anyone else want to give this a quick scan and review?

One final comment. In the patch we see the following method to clear the

// Clear the flood table. Since we don't know the IP address for this user
// we can't use flood_clear_event because we need to use the LIKE operator.
$identifier = $account->uid .'-%';
db_delete('flood')
  ->condition('event', 'failed_login_attempt_user')
  ->condition('identifier', $identifier, 'LIKE')
  ->execute();

But we do know the IP address. For example, here are two records from one of my databases;

+--------+---------------------------+----------------------+------------+------------+
| fid    | event                     | identifier           | timestamp  | expiration |
+--------+---------------------------+----------------------+------------+------------+
| 705691 | failed_login_attempt_ip   | 178.34.134.41        | 1346077836 | 1346081436 |
| 705696 | failed_login_attempt_user | 41031-178.34.134.41  | 1346077836 | 1346099436 |
+--------+---------------------------+----------------------+------------+------------+

We could in fact search for the 41031-178.34.134.41 record using $identifier = $account->uid .'-%'; and then pull out the IP address and clear those as well. I'm not sure there would be any additional security concerns because this already assumes the one time login link was used and a new password was saved. Any bot trying to brute force it could (if it REALLY knew Drupal) create a second user with the same IP address and once it got locked out of user one, reset the password for user #2, thereby clearly the IP flood control limit for both users. Such an attack would require a bit of coordination as an automated attack would still need to use the one time login link and save the new profile password for user #2.

So I think one of two things would have to happen:
1. We use the strategy of extracting the IP address from the failed_login_attempt_user record and clear all failed_login_attempt_ip records associated with that IP... despite knowing that it could open the door for a coordinated brute force login attempt.
2. We don't use that because the IP login limit is already 10X the user login limit (50 attempts for an IP versus 5 for a user). This is enough of a buffer than most use cases would not require clearing the IP records.

Thoughts? Leave out IP address clearing for security reasons? Or clear out IP addresses because there are enough cases to warrant it?

Status:Needs review» Reviewed & tested by the community

Actually upon further reflection, the issues I brought up in #58 would best be served as a different ticket... and the option 2 I suggested (e.g. doing nothing with the IP address entries in the flood table) is probably best for 99% of the use cases out there.

Because @kid_icarus originally reviewed the patch I posted and basically applied minor formatting... and since I've reviewed those changes and tested both manually and with simpletest, I'm going to mark as RBTC and wait to see if any core contributors/committers have any issues with the patch as is. Thanks!

Status:Reviewed & tested by the community» Needs work
Issue tags:+needs backport to D7

Let's add a new API function to the flood API rather than accessing the database directly in user module.

Well isn't my face red :)

http://api.drupal.org/api/drupal/core%21includes%21common.inc/function/f...

And the code is this function is almost the same as what's in the patch. The key difference is that the patch uses

->condition('identifier', $identifier, 'LIKE')

verus the current flood_clear_event function that has

->condition('identifier', $identifier)

The issue I have in not having the ability to use "like" it would require a query to the flood tables before hand to get all the records of said user from various IP addresses and then loop through them one by one. In reality, if I wanted to clear any event for a given user [UID]-[IP] identifier, I'd want to do that in one pass and be done with it.

Perhaps the flood_clear_event() could be given an optional parameter to use a "like" comparison versus an assumed '=' so one could target a UID or an IP address by itself in those cases. I assume this warrants another ticket that would be blocking this patch for the reasons mentioned above.

Thoughts?

It will be ported to drupal 7? it,s a big problem on sites with many users.

I'm not sure what the policy is on this (and please send me a link), but considering D8's flood control is changing, is it possible to keep the patches in #49 and #50 for D7 and then allow this thread to focus on if it's still and issue in D8?

flood_clear_event is gone. And it D8, a quick search led me to FloodInterface
http://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Flood%21Fl...

The reason I'm asking is this continues to be an issue we have to patch on in D7 and any new API's we add (as per @catch's suggestion in 60) won't be relevant for D8.

Subscribing for really needing this for D7 asap.

It is absolutely illogical for a user to remain blocked after resetting successfully. And even worse, I personally log out after such a reset and then login again with the new password - in order to update my browser's password manager. And for sure I am not the only one doing so. So please, get this into D7!