Problem/Motivation

After multiple failed login attempts (default set to 5 tries), a user can no longer login until a certain amount is time passed (default set to 6 hours), and instead sees the message "There have been more than 5 failed login attempts for this account. It is temporarily blocked. Try again later or request a new password.". When a user uses the one-time login link to log in, then changes his/her password and logs out, the temporary block is still in place, the user cannot still cannot log in until the window has passed.

There is no way (other than removing the flood records from the database, or through contrib solutions ) to remove the temporary ban (implemented through the Flood API) from the account.

This issue is about lifting the ban after a successful login through the reset password functionality. There is a separate issue to lift the ban after an account's password is changed (#2881572: User login flood lock doesn't clear when reset password as admin).

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

Proposed resolution

When a user logs in using the one-time login link, the temporary ban on the account should be lifted. The IP-based ban, if present, should remain in place (#35).

Remaining tasks

Patch review.

User interface changes

None.

API changes

None.

Data model changes

None.

Original report by jazzdrive3

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".

CommentFileSizeAuthor
#200 992540-200.patch3.77 KBquietone
#200 diff-197-200.txt402 bytesquietone
#197 992540-197.patch3.77 KBlouis-cuny
#195 992540-195.patch3.97 KBpradhumanjain2311
#191 interdiff_190-191.txt518 bytesranjith_kumar_k_u
#191 992540-191.patch3.97 KBranjith_kumar_k_u
#190 reroll_diff_188_190.txt2.46 KBevilehk
#190 992540-190.patch4.31 KBevilehk
#188 992540-188.patch4.79 KBpaulocs
#188 interdiff-181-188.txt1.06 KBpaulocs
#181 992540-181.patch5.13 KBpaulocs
#181 interdiff-178-181.txt540 bytespaulocs
#178 interdiff_177-178.txt1.34 KBravi.shankar
#178 992540-178.patch5.4 KBravi.shankar
#177 D9.2.x_issue-992540-176.patch5.41 KBjoseph.olstad
#177 992540-interdiff_169_to_176.txt2.08 KBjoseph.olstad
#176 992540-interdiff_169_to_175.txt2.08 KBjoseph.olstad
#176 D9.2.x_issue-992540-175.patch5.41 KBjoseph.olstad
#169 992540-169.patch5.56 KBmr.baileys
#169 interdiff-165-169.txt1.1 KBmr.baileys
#168 interdiff-992540-161-165.txt2.36 KBagrochal
#165 992540-165.patch4.99 KBpguillard
#161 992540-161.patch6.69 KBvalthebald
#161 interdiff-992540-157-161.txt7.82 KBvalthebald
#157 interdiff-156-157.patch1.05 KBmr.baileys
#157 992540-157-clear-user-flood-limit.patch12.06 KBmr.baileys
#156 992540-156-test-only.patch11.95 KBvoleger
#156 interdiff-992540-152-156.txt7.5 KBvoleger
#156 992540-156.patch11.9 KBvoleger
#152 992540-152.patch6.78 KBpguillard
#149 992540-149.patch6.77 KBpguillard
#143 interdiff-992540-127-143.txt546 bytesvalthebald
#143 992540-143.patch6.37 KBvalthebald
#142 992540-141.patch6.38 KBvalthebald
#141 interdiff-992540-127-141.txt1.27 KBvalthebald
#133 992540-133.patch5.32 KBvijaycs85
#133 992540-diff-131-133.txt1.29 KBvijaycs85
#131 992540-131.patch5.52 KBvijaycs85
#131 992540-diff-127-131.txt1.45 KBvijaycs85
#127 interdiff-992540-121-127.txt985 bytesvalthebald
#127 992540-127.patch6.42 KBvalthebald
#121 992540-118-121.diff.txt2.14 KBvalthebald
#121 992540-121.patch6.43 KBvalthebald
#118 992540-118.patch8.04 KBndobromirov
#116 992540-116.patch8.84 KBvalthebald
#116 diff-992549-115-116.txt1.07 KBvalthebald
#115 interdiff.txt510 bytesMunavijayalakshmi
#115 nothing_clears_the_5-992540-115.patch8.78 KBMunavijayalakshmi
#112 nothing_clears_the_5-992540-112.patch8.78 KBgaurav.kapoor
#108 interdiff-102-108.txt3 KBndobromirov
#108 drupal-nothing-clears-the-5-failed-log-in-attempts-992540-108.patch8.48 KBndobromirov
#105 nothing_clears_the_5-992540-105.patch8.67 KByogeshmpawar
#102 interdiff-96-102.txt5.37 KBndobromirov
#102 drupal-nothing-clears-the-5-failed-log-in-attempts-992540-102.patch8.75 KBndobromirov
#96 drupal-nothing-clears-the-5-failed-log-in-attempts-992540-96.patch9.49 KBndobromirov
#96 interdiff-93-96.txt3.03 KBndobromirov
#93 drupal-nothing-clears-the-5-failed-log-in-attempts-992540.patch7.72 KBndobromirov
#93 interdiff-89-93.patch5.69 KBndobromirov
#89 992540-88.patch4.76 KBvalthebald
#89 interdiff-992540-80-88.txt2.21 KBvalthebald
#88 interdiff-992540-80-86.txt6.99 KBvalthebald
#86 interdiff-992540-80-86.txt6.12 KBvalthebald
#86 992540-86.patch9.35 KBvalthebald
#80 992540-80.patch3.94 KBaerozeppelin
#80 test-only-fail-992540-80.patch2.32 KBaerozeppelin
#75 992540-poc-with-tests-updated-75.patch2.17 KBndobromirov
#73 992540-poc-73.patch1.31 KBvalthebald
#72 992540-d7-72.patch684 bytesvalthebald
#70 reset_flood_limit_on_password_reset-992540-70.patch962 bytesklidifia
#50 reset_flood_limit_on_password_reset-992540-50.patch4.47 KBkid_icarus
#50 interdiff.txt3.42 KBkid_icarus
#49 reset_flood_limit_on_password_reset-992540-49.patch4.49 KBkid_icarus
#45 reset_flood_limit_on_password_reset-992540-44.patch6.57 KBkid_icarus
#45 interdiff.txt3.74 KBkid_icarus
#44 reset_flood_limit_on_password_reset-992540-44.patch4.22 KBrickmanelius
#44 interdiff.txt1.23 KBrickmanelius
#43 reset_flood_limit_on_password_reset-992540-43.patch4.22 KBrickmanelius
#41 reset_flood_limit_on_password_reset-992540-41.patch4.23 KBrickmanelius
#36 reset_flood_limit_on_password_reset-992540-36.patch4.09 KBMatt V.
#34 reset_flood_limit_on_password_reset-992540-34.patch4.21 KBMatt V.
#23 992540-3-reset_flood_limit_on_password_reset-drush.patch4.15 KBjec006
#20 992540-3-reset_flood_limit_on_password_reset.patch4.16 KBjec006
#18 992540-2-reset_flood_limit_on_password_reset.patch4.2 KBjec006
#16 992540-2-reset_flood_limit_on_password_reset.patch4.2 KBjec006
#14 992540-reset_flood_limit_on_password_reset.patch1.01 KBjec006
#8 992540.patch4.32 KBswentel
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

usa2k’s picture

Subscribing. (Using D7rc4 here.)

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

marcingy’s picture

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,

usa2k’s picture

Thank you for that info!

swentel’s picture

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 ?

usa2k’s picture

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

Damien Tournoud’s picture

Category: bug » feature

I agree. Would someone work on a patch?

swentel’s picture

Assigned: Unassigned » swentel

I'll take this up tonight.

swentel’s picture

Status: Active » Needs review
FileSize
4.32 KB

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

cashwilliams’s picture

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?

marcingy’s picture

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

grendzy’s picture

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.

cashwilliams’s picture

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?

grendzy’s picture

Status: Needs work » Needs review

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

jec006’s picture

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.

jec006’s picture

#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.

jec006’s picture

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.

jec006’s picture

Status: Needs work » Needs review
FileSize
4.2 KB

Trying again

Status: Needs review » Needs work

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

jec006’s picture

Status: Needs work » Needs review
FileSize
4.16 KB

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

swentel’s picture

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)

jec006’s picture

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

so .. hopeful

jec006’s picture

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.

LarsKramer’s picture

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

jec006’s picture

Status: Needs work » Needs review

Patch in 20 works.

Everett Zufelt’s picture

Version: 7.x-dev » 8.x-dev
Assigned: swentel » Unassigned
Everett Zufelt’s picture

Everett Zufelt’s picture

Status: Needs review » Needs work

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

Everett Zufelt’s picture

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?

jec006’s picture

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.

jec006’s picture

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

Matt V.’s picture

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.

Heine’s picture

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.

Matt V.’s picture

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

MHLut’s picture

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.

basvredeling’s picture

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.

robertstaddon’s picture

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

rickmanelius’s picture

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.

rickmanelius’s picture

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! :)

kid_icarus’s picture

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.

rickmanelius’s picture

Status: Needs work » Needs review
FileSize
4.22 KB

Clearing out whitespace nastiness as noted from #42. Resubmitting

rickmanelius’s picture

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

kid_icarus’s picture

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

kid_icarus’s picture

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.

kid_icarus’s picture

Status: Needs work » Needs review
FileSize
4.49 KB

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? :)

kid_icarus’s picture

Some coding standards/commenting standards cleanup.

ro-no-lo’s picture

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.

MHLut’s picture

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

rickmanelius’s picture

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?

ro-no-lo’s picture

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

rickmanelius’s picture

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.

rickmanelius’s picture

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?

rickmanelius’s picture

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?

rickmanelius’s picture

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!

catch’s picture

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.

rickmanelius’s picture

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?

camilo.mejia’s picture

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

rickmanelius’s picture

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.

doitDave’s picture

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!

Anybody’s picture

Issue summary: View changes

Hi @ all,
the problem still exists for Drupal 7 and is really major in my eyes. A clear bug.
As described already, the user can login via link, change the password but is locked out again after log-out. This is an important bug in my eyes. Shouldn't it get a higher priority? No action since 7 month??

Please backport this to D7.
Thanks a lot to all of you!

doitDave’s picture

@65 obviously not, so "selbst ist der Mann", as they say here in Germany. Unfollowing.

mackie1’s picture

Flood_unblock
Flood_control

These two modules address this issue. Alternatively you can clear the `flood` table in the database. DELETE * FROM `FLOOD`;

A work around if you cant do either of these would be:
1)Cancel Account - Disable account, but keep content (There is four options, choose this one as no data is affected)
2)Once account has been cancelled, go back in and set account to active.
3) Wait 6 hours for IP lock to expire.

klidifia’s picture

These two modules do not address the issue sufficiently - seemingly you cannot use them to automatically unlock an account once a successful reset has taken place. Requiring manual intervention is far less than ideal.

klidifia’s picture

Also, what is the reasoning behind clearing the entries in the table when the user profile form is updated? I would have thought it would be sufficient to do this only when the one-time login link is used to login. I don't think that delete query should be in user_profile_form_submit.

There seems to be an issue with this line in the test, it is not emulating the password reset/one-time login:
$this->drupalPost(user_pass_reset_url($user1), array(), t('Log in'));

klidifia’s picture

My view is that:

  1. flood_unblock and flood_control basically serve as attempts to mitigate the adverse affects caused by this issue, and are not an ideal solution.
  2. The flood table should not be touched when a profile form is saved; the current patches do this.

This patch will remove the flood events for the user who has just successfully used the one time login link to login after clicking on the link in the email generated from the (lost) password request.

I cannot see any reason that this is not the ideal and optimal way to handle this issue, so in the absence of something better, I will be applying this.

If someone wants to write a test that works that would be great, the existing tests are not generating the correct URL, and hence are not seeing the 'Log in' link but are rather being presented with the password reset form again.

valthebald’s picture

Version: 8.0.x-dev » 8.1.x-dev
Issue tags: +needs backport to 8.0.x

Bumping version

valthebald’s picture

Here's the patch for D7, and I hope to review/provide 8.1.x one soon

valthebald’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
1.31 KB

This patch is POC only, I set to Needs review only for the testbot.
The issue needs a test, tagging

ndobromirov’s picture

Assigned: Unassigned » ndobromirov
Issue tags: +SprintWeekend2016
ndobromirov’s picture

Hi, I've updated the test to incorporate the changes from the patch.

valthebald’s picture

Issue tags: -Needs tests

Removing "Needs tests" as we have them now

Status: Needs review » Needs work

The last submitted patch, 75: 992540-poc-with-tests-updated-75.patch, failed testing.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

aerozeppelin’s picture

Status: Needs work » Needs review
FileSize
2.32 KB
3.94 KB

Updates to patch #75.

The last submitted patch, 80: test-only-fail-992540-80.patch, failed testing.

valthebald’s picture

Status: Needs review » Reviewed & tested by the community

I dare RTBC that

The last submitted patch, 70: reset_flood_limit_on_password_reset-992540-70.patch, failed testing.

The last submitted patch, 72: 992540-d7-72.patch, failed testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/user/src/Controller/UserController.php
@@ -222,6 +222,20 @@ public function resetPassLogin($uid, $timestamp, $hash) {
+        $identifier = $user->id() . '-' . \Drupal::request()->getClientIP();

Should be injected into the controller. I've debated whether or not this is the right place for this change with @WimLeers. The thing is that we need a request for the IP address so I've come round to thinking this is the right place and if we ever do implement an REST or RPC endpoint for password reset via an API then we'd have to account for flood there too.

valthebald’s picture

Status: Needs work » Needs review
FileSize
9.35 KB
6.12 KB

@alexpott: something like this?

valthebald’s picture

valthebald’s picture

FileSize
6.99 KB
valthebald’s picture

Pardon, unneeded files submitted. Removed

Status: Needs review » Needs work

The last submitted patch, 89: 992540-88.patch, failed testing.

valthebald’s picture

Status: Needs work » Needs review

Testbot hiccup?

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

ndobromirov’s picture

I've tested the patch from #89 and I could RTBC it for it's fix of the issue, though I've noticed something. There were uses of the deprecated t function in the tests implementation.

I am attaching a patch and a diff, with the translations done through the StringTranslationTrait instead of the legacy way and some re-flowing of code to improve readability.

Original functionality of the patch is unchanged and it was already working.
Either way :) still stays in needs review.

lachezar.valchev’s picture

Status: Needs review » Reviewed & tested by the community

I have reviewed and tested the patch in #93

Looks good to me.
I have also tested the functionality and it works as expected.

I am changing it to RTBC

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/user/src/Controller/UserController.php
@@ -222,6 +224,20 @@ public function resetPassLogin($uid, $timestamp, $hash) {
+      \Drupal::flood()->clear('user.failed_login_ip');
+      \Drupal::flood()->clear('user.failed_login_user', $identifier);

This should be injected.

ndobromirov’s picture

Here is the flood service injected.

Is it going to pose overhead to the rest of the controller calls, as it currently is, the flood service is needed only by one of them.

An idea is to make the flood service lazy, so it will really be initialized fully when used, sparing the overhead for the other controller calls.

lachezar.valchev’s picture

Status: Needs review » Reviewed & tested by the community

Latest patch tested.

Looks good.

RTBC

xjm’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: -needs backport to 8.0.x

@lachezar.valchev, when you test a patch and mark it RTBC, please provide more information than "Looks good". Document what you tested, and what the results at each step were. Screenshots are often a good idea. Also, a code review should also be done on the updated patch. See https://www.drupal.org/patch/review for more ideas on how to provide a helpful review. Thanks!

valthebald’s picture

Status: Needs review » Reviewed & tested by the community

This ancient issue has attracted our attention at sprint weekend (see #72), where @ndobromirov, @lachezar.valchev and myself discussed the solution. @ndobromirov has added the test, others tested that it indeed fixes the problem of both IP- and user-based flood.
Since the test is (IMO) self-explanatory, and 2 patches were provided (one with test-only, which fails, and combined, which passes), I don't think there is a need in screenshots.
@alexpott (#85) has asked to use request from container, and @tstoeckler has pointed out that flood should also be injected. In the last patch @ndobromirov also adopted the fix to the changes in API that happened since January 2016.

Since all requests from @alexpott and @tstoeckler were addressed, and the patch still does its job, I dare return it to RTBC

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Thanks @valthebald for clarifying! If possible, can you pass the suggestions on to @lachezar.valchev for future review comments?

Meanwhile, granting issue credit to @lachezar.valchev based on @valthebald's confirmation as a mentor and his summary of their review of the issue.

  1. +++ b/core/modules/user/src/Tests/UserLoginTest.php
    @@ -4,6 +4,7 @@
    +use Drupal\Core\StringTranslation\StringTranslationTrait;
    
    @@ -12,6 +13,8 @@
    +  use StringTranslationTrait;
    

    Hmm. In general, we should be removing reliance on string translation in tests, not adding more. We did a lot of work on eliminating it in the last year before the release of 8.0.0. See #2549805: Remove all usage of FormattableMarkup in tests apart from explicit tests of that API for example. Obviously a lot of that cleanup is still outstanding, as can be seen from all the t() calls in this test, but we can at least not add more.

  2. +++ b/core/modules/user/src/Tests/UserLoginTest.php
    @@ -24,7 +27,7 @@ function testLoginCacheTagsAndDestination() {
    -    $this->drupalPostForm(NULL, $edit, t('Log in'));
    +    $this->drupalPostForm(NULL, $edit, $this->t('Log in'));
    
    @@ -159,20 +179,46 @@ function assertFailedLogin($account, $flood_trigger = NULL) {
    -    $this->drupalPostForm('user/login', $edit, t('Log in'));
    +    $this->drupalPostForm('user/login', $edit, $this->t('Log in'));
    ...
    -        $this->assertRaw(t('Too many failed login attempts from your IP address. This IP address is temporarily blocked. Try again later or <a href=":url">request a new password</a>.', array(':url' => \Drupal::url('user.pass'))));
    +        $this->assertRaw($this->t('Too many failed login attempts from your IP address. This IP address is temporarily blocked. Try again later or <a href=":url">request a new password</a>.', [
    +          ':url' => \Drupal::url('user.pass'),
    +        ]));
    ...
    -      $this->assertText(t('Unrecognized username or password. Forgot your password?'));
    +      $this->assertText($this->t('Unrecognized username or password. Forgot your password?'));
    

    These changes appear to be out of scope for the issue.

Thanks!

xjm’s picture

(Also saving additional issue credit.)

ndobromirov’s picture

Here is a new patch with all the translation stuff gone + diff.

valthebald’s picture

Status: Needs review » Needs work

@ndobromirov

+++ b/core/modules/user/src/Tests/UserLoginTest.php
@@ -103,6 +110,15 @@ function testPerUserLoginFloodControl() {
+    $this->assertNoRaw(strtr('There have been more than @count failed login attempts for this account. It is temporarily blocked. Try again later or <a href=":url">request a new password</a>.', [

No need for strtr() here IMO
Can be smth like

$this->assertNoRaw('There have been more than ' . $this->config('user.flood')->get('user_limit')
 . ' failed login attempts'...
ndobromirov’s picture

I was aiming to preserve the patch as close to the original as possible.

@valthebald
I will make a new one sometime this weekend to reflect your comments.

yogeshmpawar’s picture

Status: Needs work » Needs review
FileSize
8.67 KB

Rerolled the patch because previous patch failed to apply & also added a suggestion as per comment #103.

Status: Needs review » Needs work

The last submitted patch, 105: nothing_clears_the_5-992540-105.patch, failed testing.

ndobromirov’s picture

@Yogesh
What has changed?

ndobromirov’s picture

Here is a re-roll with no strtr in tests against the patch from 102.

Status: Needs review » Needs work
yogeshmpawar’s picture

@ndobromirov - your current & previous patch failed to apply, that's why i uploaded patch. also #108 patch failed to apply, please check

gaurav.kapoor’s picture

Assigned: Unassigned » gaurav.kapoor
gaurav.kapoor’s picture

gaurav.kapoor’s picture

Status: Needs work » Needs review
gaurav.kapoor’s picture

Assigned: gaurav.kapoor » Unassigned
Munavijayalakshmi’s picture

+++ b/core/modules/user/src/Tests/UserLoginTest.php
@@ -159,20 +172,44 @@ public function assertFailedLogin($account, $flood_trigger = NULL) {
+    $urls = array();

use short array syntax (new coding standard).

Fixed the short array syntax error and attached new patch.

valthebald’s picture

Replaced (deprecated) \Drupal::url() call

valthebald’s picture

Issue tags: +DCTransylvania

Also, tagging

ndobromirov’s picture

Here is a re-roll of the patch from 108.
It is re-based on top of the latest dev of 8.4.x.

@Munavijayalakshmi Thanks for pointing that out. This is also included.

@valthebald your changes from #116 are not included, as the URL is not part of the string check now.
This is implemented in that way to remove the strtr that we've discussed with you in #102-#103.

valthebald’s picture

Status: Needs review » Reviewed & tested by the community

@ndobromirov yes, good point about removing strtr, this wasn't addressed in #109-#115
Returning to RTBC, since the point by @xjm (in #100) about not adding translations is addressed, otherwise no changes from patch in #96

cilefen’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/user/src/Tests/UserLoginTest.php
@@ -159,20 +173,39 @@ public function assertFailedLogin($account, $flood_trigger = NULL) {
-    $this->drupalPostForm('user/login', $edit, t('Log in'));
+    $this->drupalPostForm('user/login', $edit, 'Log in');
     $this->assertNoFieldByXPath("//input[@name='pass' and @value!='']", NULL, 'Password value attribute is blank.');
     if (isset($flood_trigger)) {
       if ($flood_trigger == 'user') {
-        $this->assertRaw(\Drupal::translation()->formatPlural($this->config('user.flood')->get('user_limit'), 'There has been more than one failed login attempt for this account. It is temporarily blocked. Try again later or <a href=":url">request a new password</a>.', 'There have been more than @count failed login attempts for this account. It is temporarily blocked. Try again later or <a href=":url">request a new password</a>.', [':url' => \Drupal::url('user.pass')]));
+        $this->assertRaw('There have been more than ' . $this->config('user.flood')->get('user_limit') . ' failed login attempts for this account.');
       }
       else {
         // No uid, so the limit is IP-based.
-        $this->assertRaw(t('Too many failed login attempts from your IP address. This IP address is temporarily blocked. Try again later or <a href=":url">request a new password</a>.', [':url' => \Drupal::url('user.pass')]));
+        $this->assertRaw('Too many failed login attempts from your IP address. This IP address is temporarily blocked.');
       }
     }
     else {
-      $this->assertText(t('Unrecognized username or password. Forgot your password?'));
+      $this->assertText('Unrecognized username or password. Forgot your password?');

These apparently out-of-scope changes remain as mentioned in #100.

valthebald’s picture

Status: Needs work » Needs review
FileSize
6.43 KB
2.14 KB

@cilefen I've removed irrelevant part, ensured that only newly added strings are not translated

ndobromirov’s picture

Status: Needs review » Reviewed & tested by the community

The change reflects the last review and it fixes the issues.
Moving back to RTBC.

yogeshmpawar’s picture

Any Update on this issue ?

cilefen’s picture

In the last four days? No.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 121: 992540-121.patch, failed testing.

MegaChriz’s picture

+++ b/core/modules/user/src/Controller/UserController.php
@@ -59,12 +67,15 @@ class UserController extends ControllerBase {
+   * @param Drupal\Core\Flood\FloodInterface $flood
+   *   A logger instance.

Nitpick: description of the $flood parameter should be:
A flood service instance.

valthebald’s picture

Status: Needs work » Needs review
FileSize
6.42 KB
985 bytes

@MegaChriz thanks for the comment! Made parameter and member comments consistent with Drupal\user\Form\UserLoginForm

ndobromirov’s picture

Status: Needs review » Reviewed & tested by the community

Issues from #126 are resolved in #127.
Moving back to RTBC.

catch’s picture

Category: Feature request » Bug report
Status: Reviewed & tested by the community » Fixed

This is really a bug, a manual user password reset by an admin should obviously reset the flood control.

Made the following documentation change on commit - the inline comment were copy/pasted from where we register the flood events, but here we're just clearing them:

diff --git a/core/modules/user/src/Controller/UserController.php b/core/modules/user/src/Controller/UserController.php
index 71b6a8a..73c8fb3 100644
--- a/core/modules/user/src/Controller/UserController.php
+++ b/core/modules/user/src/Controller/UserController.php
@@ -238,14 +238,11 @@ public function resetPassLogin(Request $request, $uid, $timestamp, $hash) {
     elseif ($user->isAuthenticated() && ($timestamp >= $user->getLastLoginTime()) && ($timestamp <= $current) && Crypt::hashEquals($hash, user_pass_rehash($user, $timestamp))) {
       $flood_config = $this->config('user.flood');
       if ($flood_config->get('uid_only')) {
-        // Register flood events based on the uid only, so they apply for any
-        // IP address. This is the most secure option.
+        // Clear flood events based on the uid only if configured.
         $identifier = $user->id();
       }
       else {
-        // The default identifier is a combination of uid and IP address. This
-        // is less secure but more resistant to denial-of-service attacks that
-        // could lock out all users with public user names.
+        // The default identifier is a combination of uid and IP address.
         $identifier = $user->id() . '-' . $request->getClientIP();
       }
       $this->flood->clear('user.failed_login_ip');

Committed/pushed to 8.4.x, thanks!

Due to the UserController constructor change, we can't commit this to 8.3.x as-is. If someone would like to backport the patch to 8.4.x with a \Drupal:: call instead, please re-open this issue and set 'patch to be ported'.

The Drupal 7 fix for this issue should be worked on in a new issue against the 7.x branch - please open that issue and link back to this one if you plan to work on it.

  • catch committed cade2c6 on 8.4.x
    Issue #992540 by valthebald, ndobromirov, jec006, kid_icarus,...
vijaycs85’s picture

Version: 8.4.x-dev » 8.3.x-dev
Status: Fixed » Patch (to be ported)
FileSize
1.45 KB
5.52 KB
vijaycs85’s picture

Issue tags: -Needs backport to D7

As we have a separate issue for 7.x, removing the tag.

vijaycs85’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.29 KB
5.32 KB

Just missed the comment updates in #129

valthebald’s picture

I wonder if adding $request as a new parameter to resetPassLogin() is allowed change in 8.3.x
Maybe \Drupal::request() is more appropriate for BC

David_Rothstein’s picture

Version: 8.3.x-dev » 8.4.x-dev
Priority: Normal » Critical
Status: Needs review » Needs work

I think this needs to be rolled back for 8.4.x. The committed patch did not wind up taking into account Heine's comment in #35 and therefore it opens up a security hole.

I noticed this while reviewing the Drupal 7 backport issue, so I tested Drupal 8.4.x and confirmed that you can easily bypass the IP-based flood limit via the method Heine described. 8.3.x is not affected since it doesn't have this patch.

David_Rothstein’s picture

Title: Nothing Clears the "5 Failed Login Attempts" Security message » Nothing clears the "5 failed login attempts" security message when a user resets their own password
Issue tags: +Needs issue summary update

Also worth flagging this for an issue summary update, since the patch that was committed here had nothing to do with passwords reset by an administrator (that looks like it got moved to #2881572: User login flood lock doesn't clear when reset password as admin instead) - rather it was about a user resetting their own password.

  • cilefen committed 9634371 on 8.4.x
    Revert "Issue #992540 by valthebald, ndobromirov, jec006, kid_icarus,...
cilefen’s picture

Priority: Critical » Normal

Thank you for the heads-up @David_Rothstein. I reverted it.

tedbow’s picture

tedbow’s picture

Sorry commented wrong issue. Guess there is no validation for setting the current issue has "related" to itself. whoops ;)

Meant to comment here to say that refactoring some of the changes on this issue here #2847708-40: RPC endpoint to reset user password

We created a new UserResetEmailTestTrait. Because we also needed this logic there.

Maybe whichever issue gets committed first could use this trait?

valthebald’s picture

Status: Needs work » Needs review
FileSize
1.27 KB

Addressing comments #35 and #135

valthebald’s picture

valthebald’s picture

Removed erroneous comment changes

The last submitted patch, 142: 992540-141.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 143: 992540-143.patch, failed testing. View results

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

borisson_’s picture

Issue tags: +Needs reroll

This patch doesn't apply anymore, because the tests have moved from simpletest to BTB tests. In the last patch, they also didn't all pass.

Let's start by getting this rerolled and get the tests to pass.

pguillard’s picture

Status: Needs work » Needs review
FileSize
6.77 KB

Patch rerolled, I added use of AssertMailTrait.
I guess one error remains.

Status: Needs review » Needs work

The last submitted patch, 149: 992540-149.patch, failed testing. View results

savkaviktor16@gmail.com’s picture

Issue tags: -Needs reroll
pguillard’s picture

Status: Needs work » Needs review
Issue tags: +DevDaysLisbon
FileSize
6.78 KB

This patch needed another reroll

Status: Needs review » Needs work

The last submitted patch, 152: 992540-152.patch, failed testing. View results

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

borisson_’s picture

We still have that test-failure, we can keep on rerolling this patch untill the inevitable heath death of the universe but we should probably get that failure fixed.

The failure is in UserLoginTest::testGlobalLoginFloodControl

+++ b/core/modules/user/tests/src/Functional/UserLoginTest.php
@@ -63,16 +67,23 @@ public function testGlobalLoginFloodControl() {
+    // Try to login as user 1, it should be successful.
+    $this->drupalLogin($user1);
+    $this->assertNoRaw('Too many failed login attempts from your IP address.');

This is the part of the code that is not working.

We also still need the issue summary to be updated.

voleger’s picture

Removed deprecation notices during the local test run.

An interesting thing that even new user can't be logged in after change password by the blocked user (see test only patch).

+++ b/core/modules/user/tests/src/Functional/UserLoginTest.php
@@ -63,12 +69,20 @@ public function testGlobalLoginFloodControl() {
+    $this->drupalLogin($this->createUser());

Here the login by the new user, and it's blocked.

Looks like ip blocking triggered.

mr.baileys’s picture

The failing test is a left-over from the patch that was committed earlier, and which included resetting the IP flood limit on succesful one-time login link usage. Since the IP-limit should not be cleared, the test should instead verify that, even though a user has used the one-time login link, the temporary IP ban remains in place. Patch attached.

I have updated the issue summary.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

valthebald’s picture

Status: Needs review » Reviewed & tested by the community

I dare mark this RTBC

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

@valthebald unfortauntely it needs a reroll. Also looking at the patch there is quite a lot that is out-of-scope and cosmetic improvements to a test. This issue should be tightly scoped to fixing the bug.

  1. +++ b/core/modules/user/src/Controller/UserController.php
    @@ -227,6 +241,20 @@ public function resetPassLogin($uid, $timestamp, $hash) {
    +      if ($flood_config->get('uid_only')) {
    +        // Register flood events based on the uid only, so they apply for any
    +        // IP address. This is the most secure option.
    +        $identifier = $user->id();
    +      }
    +      else {
    +        // The default identifier is a combination of uid and IP address. This
    +        // is less secure but more resistant to denial-of-service attacks that
    +        // could lock out all users with public user names.
    +        $identifier = $user->id() . '-' . $request->getClientIP();
    +      }
    

    Wow this is icky - why is this logic not on the flood service? We should have issue about this somewhere.

  2. +++ b/core/modules/user/tests/src/Functional/UserLoginTest.php
    @@ -19,13 +24,14 @@ public function testLoginCacheTagsAndDestination() {
    -    $this->assertUrl('foo', [], 'Redirected to the correct URL');
    +    // Redirected to the correct URL.
    +    $this->assertSession()->addressEquals('foo');
    

    Out-of-scope.

  3. +++ b/core/modules/user/tests/src/Functional/UserLoginTest.php
    @@ -103,27 +118,35 @@ public function testPerUserLoginFloodControl() {
    -    // Determine default log2 for phpass hashing algorithm
    +    // Determine default log2 for phpass hashing algorithm.
    ...
    -    // Retrieve instance of password hashing algorithm
    +    // Retrieve instance of password hashing algorithm.
    ...
    +    /** @var \Drupal\user\UserInterface $account */
    

    This all looks out-of-scope.

  4. +++ b/core/modules/user/tests/src/Functional/UserLoginTest.php
    @@ -103,27 +118,35 @@ public function testPerUserLoginFloodControl() {
    -    // Load the stored user. The password hash should reflect $default_count_log2.
    -    $user_storage = $this->container->get('entity.manager')->getStorage('user');
    -    $account = User::load($account->id());
    -    $this->assertIdentical($password_hasher->getCountLog2($account->getPassword()), $default_count_log2);
    +    // Load the stored user. The password hash should reflect
    +    // $default_count_log2.
    +    $user_storage = $this->container->get('entity_type.manager')->getStorage('user');
    +    $account = $user_storage->load($account->id());
    +    $this->assertSame($default_count_log2, $password_hasher->getCountLog2($account->getPassword()));
    

    Whilst these changes make sense I'm not sure they are required or in scope.

  5. +++ b/core/modules/user/tests/src/Functional/UserLoginTest.php
    @@ -137,7 +160,7 @@ public function testPasswordRehashOnLogin() {
    -    $this->assertIdentical($password_hasher->getCountLog2($account->getPassword()), $overridden_count_log2);
    +    $this->assertSame($overridden_count_log2, $password_hasher->getCountLog2($account->getPassword()));
    

    This looks unnecessary.

  6. +++ b/core/modules/user/tests/src/Functional/UserLoginTest.php
    @@ -160,19 +183,39 @@ public function assertFailedLogin($account, $flood_trigger = NULL) {
    -    $this->assertNoFieldByXPath("//input[@name='pass' and @value!='']", NULL, 'Password value attribute is blank.');
    +    $pass = $this->xpath("//input[@name='pass' and @value!='']");
    +    $this->assertEmpty($pass, 'Password value attribute is blank.');
    

    I think we can use $this->assertSession->elementNotExists() but also I'm not sure these changes are in scope.

  7. +++ b/core/modules/user/tests/src/Functional/UserLoginTest.php
    @@ -160,19 +183,39 @@ public function assertFailedLogin($account, $flood_trigger = NULL) {
    -        $this->assertRaw(\Drupal::translation()->formatPlural($this->config('user.flood')->get('user_limit'), 'There has been more than one failed login attempt for this account. It is temporarily blocked. Try again later or <a href=":url">request a new password</a>.', 'There have been more than @count failed login attempts for this account. It is temporarily blocked. Try again later or <a href=":url">request a new password</a>.', [':url' => \Drupal::url('user.pass')]));
    +        $this->assertSession()->responseContains($this->container->get('string_translation')->formatPlural($this->config('user.flood')->get('user_limit'), 'There has been more than one failed login attempt for this account. It is temporarily blocked. Try again later or <a href=":url">request a new password</a>.', 'There have been more than @count failed login attempts for this account. It is temporarily blocked. Try again later or <a href=":url">request a new password</a>.', [':url' => Url::fromRoute('user.pass')->toString()]));
    ...
    -        $this->assertRaw(t('Too many failed login attempts from your IP address. This IP address is temporarily blocked. Try again later or <a href=":url">request a new password</a>.', [':url' => \Drupal::url('user.pass')]));
    +        $this->assertSession()->responseContains(t('Too many failed login attempts from your IP address. This IP address is temporarily blocked. Try again later or <a href=":url">request a new password</a>.', [':url' => Url::fromRoute('user.pass')->toString()]));
    ...
    -      $this->assertText(t('Unrecognized username or password. Forgot your password?'));
    +      $this->assertSession()->pageTextContains('Unrecognized username or password. Forgot your password?');
    

    As far as I can see these changes are not need to make this change.

valthebald’s picture

Rerolled the patch, taking into account comments from @alexpott

Status: Needs review » Needs work

The last submitted patch, 161: 992540-161.patch, failed testing. View results

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

joseph.olstad’s picture

patch needs reroll , again. For both 8.8.x and 8.9.x

pguillard’s picture

Status: Needs work » Needs review
FileSize
4.99 KB

This is the rerolled patch.

Status: Needs review » Needs work

The last submitted patch, 165: 992540-165.patch, failed testing. View results

heddn’s picture

Still has failing tests.

agrochal’s picture

FileSize
2.36 KB

I attach interdiff file for last two patches(#161 and #165).

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
1.1 KB
5.56 KB

Test failures are caused by \Drupal\Tests\user\Functional\UserLoginTest::resetUserPassword() and the import for \Drupal\user\Entity\User being dropped from the patch in #161. Readded them, and rerolled the patch against 8.9.x

heddn’s picture

Status: Needs review » Reviewed & tested by the community

Manually tested the patch. It works fine.

catch’s picture

Looks good to me too. I looked again at #160.1 and while it does look a bit icky, it's because user module itself makes the flood identifier granularity configurable, I don't think there's anything that can be improved at the flood service level itself here.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/user/src/Controller/UserController.php
    @@ -244,6 +246,19 @@ public function resetPassLogin($uid, $timestamp, $hash) {
    +      $flood_config = $this->config('user.flood');
    +      if ($flood_config->get('uid_only')) {
    +        // Register flood events based on the uid only, so they apply for any
    +        // IP address. This is the most secure option.
    +        $identifier = $user->id();
    +      }
    +      else {
    +        // The default identifier is a combination of uid and IP address. This
    +        // is less secure but more resistant to denial-of-service attacks that
    +        // could lock out all users with public user names.
    +        $identifier = $user->id() . '-' . $request->getClientIP();
    +      }
    +      $this->flood->clear('user.failed_login_user', $identifier);
    
    +++ b/core/modules/user/tests/src/Functional/UserLoginTest.php
    @@ -69,12 +74,21 @@ public function testGlobalLoginFloodControl() {
    +    // A login attempt after resetting the password should still fail, since the
    +    // IP-based flood control count is not cleared after a password reset.
    

    The test comment and code don't appear to match. I think maybe we need to be specific about whether we're dealing with user.failed_login_ip (which is IP only and what is failing here) or user.http_login and user.failed_login_user (which depending on config is either user ID or user ID + IP address).

    Thinking about this some more I think we need to also clear user.http_login here too - to bring us inline with \Drupal\user\Controller\UserAuthenticationController() and so that any blocks due to user.http_login are also cleared by resetting the password.

    One thing this is making me feel is that maybe we should open a follow-up issue to add these flood clears to user_login_finalize() so that a successful user login always clears the two user specific floods and maybe in that issue we could discuss whether having two separate floods is really correct. It doesn't really feel correct.

  2. +++ b/core/modules/user/tests/src/Functional/UserLoginTest.php
    @@ -25,7 +30,7 @@ public function testLoginCacheTagsAndDestination() {
    -    $this->assertCacheTag('config:system.site');
    +    $this->assertSession()->responseHeaderContains('X-Drupal-Cache-Tags', 'config:system.site');
    

    Fixing this call to D10 deprecated code is not necessary.

  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.

  4. +++ b/core/modules/user/tests/src/Functional/UserLoginTest.php
    @@ -109,6 +123,12 @@ public function testPerUserLoginFloodControl() {
    +    // Try to login as user 1, it should be successful.
    

    I think this should be log in and not login.

  5. +++ b/core/modules/user/tests/src/Functional/UserLoginTest.php
    @@ -109,6 +123,12 @@ public function testPerUserLoginFloodControl() {
    +    $this->drupalLogin($user1);
    +    $this->assertSession()->responseNotContains('There have been more than ' . $user_limit . ' failed login attempts for this account.');
    

    This is a negative assertion - let's do a positive assertion

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

joseph.olstad’s picture

Latest patch still applies cleanly to 9.0.x , 9.1.x and 8.9.x

however it needs the changes as per Alexpott comment #172

anyone want to capture the glory today?

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

joseph.olstad’s picture

joseph.olstad’s picture

@alexpott comment #172

1. a) Done, see interdiff
b) create followup issue after commit. (Not done, but should not prevent commit).

2. D9 already has this in HEAD, DONE

3. Done see interdiff

4. Done see interdiff

5. Done see interdiff

ravi.shankar’s picture

Here I have fixed one failed test of patch #177 and fixed some CS issues as well.

ravi.shankar’s picture

Status: Needs work » Needs review

I think now it's ready for review.

joseph.olstad’s picture

Status: Needs review » Reviewed & tested by the community

Thanks Ravi, I think this is it.

paulocs’s picture

Removing unused variable $user_limit. I'll let it as RTBC as only a small change was made.

joseph.olstad’s picture

Thanks @paulocs good eyes, I think it is finally ready for @alexpott / @catch or another core maintainer.

joseph.olstad’s picture

9 years, 10 months and counting ... No pressure or anything, but I've gotten a lot more gray hairs in the past 9 years.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 181: 992540-181.patch, failed testing. View results

joseph.olstad’s picture

Status: Needs work » Reviewed & tested by the community

patch is still good, the noise is from HEAD and/or a testbot glitch

catch’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/user/src/Controller/UserController.php
    @@ -219,7 +221,7 @@ public function getResetPassForm(Request $request, $uid) {
        *   If $uid is for a blocked user or invalid user ID.
        */
    -  public function resetPassLogin($uid, $timestamp, $hash) {
    +  public function resetPassLogin(Request $request, $uid, $timestamp, $hash) {
         // The current user is not logged in, so check the parameters.
         $current = REQUEST_TIME;
    

    This needs a change record - controllers aren't API, but it's not impossible that someone has extended it.

  2. +++ b/core/modules/user/src/Controller/UserController.php
    @@ -240,6 +242,20 @@ public function resetPassLogin($uid, $timestamp, $hash) {
    +      $flood_config = $this->config('user.flood');
    +      if ($flood_config->get('uid_only')) {
    +        // Register flood events based on the uid only, so they apply for any
    +        // IP address. This is the most secure option.
    +        $identifier = $user->id();
    +      }
    +      else {
    +        // The default identifier is a combination of uid and IP address. This
    +        // is less secure but more resistant to denial-of-service attacks that
    +        // could lock out all users with public user names.
    +        $identifier = $user->id() . '-' . $request->getClientIP();
    

    This comment has been copied from other places with this logic, but when it's copied here it's saying the wrong thing - we're not registering a flood event here but clearing one. I think we can just drop the inline comments since they already exist elsewhere.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

paulocs’s picture

Status: Needs work » Needs review
Issue tags: +Bug Smash Initiative
FileSize
1.06 KB
4.79 KB

I addressed #186. The link with the CR: https://www.drupal.org/node/3224905

This needs a change record - controllers aren't API, but it's not impossible that someone has extended it.

Do we need to write a deprecation notice for the resetPassLogin method?

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

evilehk’s picture

The attached files are a reroll of the patch from comment 188, and a diff of the patch files. This patch works against the latest 9.3.x branch and 9.4.x.

ranjith_kumar_k_u’s picture

Removed following parameter doc from the patch because which is already exists

   * @param \Symfony\Component\HttpFoundation\Request $request
   *   The request.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

nod_’s picture

Status: Needs review » Needs work

Patch looks good but doesn't apply to 10.1.x

pradhumanjain2311’s picture

Fix Patch #191 for 10.1.x.
Please review.

pradhumanjain2311’s picture

Status: Needs work » Needs review
louis-cuny’s picture

Patches didn't apply to drupal 9.5
Here is a d9.5 compatible patch, rerolled from 191

catch’s picture

Status: Needs review » Reviewed & tested by the community

The CR I asked for in #186 is no-longer necessary because that parameter has been added to the controller method in 9.5 and 10.1 already, went ahead and deleted it.

Patch still looks good otherwise, I think we could backport this to 10.0.x and 9.5.x patch releases given there's no controller change now too.

Since this was RTBC before I marked it needs work and the only thing since (since 2020...) was removing a comment + re-rolls, I'll go ahead and commit this in a day or so if there's no further feedback ehre.

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll for 10.1.x

quietone’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
402 bytes
3.77 KB

Rerolled for 10.1.x

sahilgidwani’s picture

Status: Needs review » Reviewed & tested by the community

#200 is working fine for Drupal version 10.1.x

  • catch committed a8729aad on 10.1.x
    Issue #992540 by valthebald, ndobromirov, jec006, kid_icarus,...

  • catch committed f7b910aa on 10.0.x
    Issue #992540 by valthebald, ndobromirov, jec006, kid_icarus,...

  • catch committed 10a0113c on 9.5.x
    Issue #992540 by valthebald, ndobromirov, jec006, kid_icarus,...
catch’s picture

Version: 10.1.x-dev » 9.5.x-dev
Status: Reviewed & tested by the community » Fixed

The 10.1.x patch cherry-picked cleanly to 10.0.x, and the 9.5.x patch still applied, so I've gone ahead and committed/pushed this to all three branches.

Really nice to close a 12-year-old issue and I know from client-sites this is very annoying when users hit it.

Hopefully see people over on #2881572: User login flood lock doesn't clear when reset password as admin.

Did my best with issue credit, but it's very challenging on an issue with this many contributors so apologies for any oversights.

sahilgidwani’s picture

Anybody’s picture

@catch WHAO! Thank you so much! Still plenty of such issues to solve ... ;) Hello frustration.

Status: Fixed » Closed (fixed)

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