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 #67) 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).
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".
Comment | File | Size | Author |
---|---|---|---|
#200 | 992540-200.patch | 3.77 KB | quietone |
| |||
#200 | diff-197-200.txt | 402 bytes | quietone |
#197 | 992540-197.patch | 3.77 KB | louis-cuny |
#195 | 992540-195.patch | 3.97 KB | pradhumanjain2311 |
| |||
#191 | interdiff_190-191.txt | 518 bytes | ranjith_kumar_k_u |
Comments
Comment #1
usa2k CreditAttribution: usa2k commentedSubscribing. (Using D7rc4 here.)
How long is "try later"? After getting locked out?
Can this be reset via PHPAdmin?
Comment #2
marcingy CreditAttribution: marcingy commentedThe 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,
Comment #3
usa2k CreditAttribution: usa2k commentedThank you for that info!
Comment #4
swentel CreditAttribution: swentel commentedWell 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 ?
Comment #5
usa2k CreditAttribution: usa2k commented@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.
Comment #6
Damien Tournoud CreditAttribution: Damien Tournoud commentedI agree. Would someone work on a patch?
Comment #7
swentel CreditAttribution: swentel commentedI'll take this up tonight.
Comment #8
swentel CreditAttribution: swentel commentedHere's a patch. Clears the flood event on password reset validation and on the user/%/edit screen. Comes with tests.
Comment #9
cashwilliams CreditAttribution: cashwilliams commentedI'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?
Comment #10
marcingy CreditAttribution: marcingy commented#8: 992540.patch queued for re-testing.
Comment #11
grendzy CreditAttribution: grendzy commented(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.
Comment #12
cashwilliams CreditAttribution: cashwilliams commentedI 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?
Comment #13
grendzy CreditAttribution: grendzy commentedOh, I see. I think we're covered then.
Comment #14
jec006 CreditAttribution: jec006 commentedI'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.
Comment #15
jec006 CreditAttribution: jec006 commented#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.
Comment #16
jec006 CreditAttribution: jec006 commentedAlright - here is a new patch that does what was being done in 8 but with checking for the correct type of identifier to clear.
Comment #18
jec006 CreditAttribution: jec006 commentedTrying again
Comment #20
jec006 CreditAttribution: jec006 commentedOk, fixed all the issues this time I think ... sorry for the back and forth
Comment #21
swentel CreditAttribution: swentel commentedIt 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)
Comment #22
jec006 CreditAttribution: jec006 commentedYah, before I was using diff - which made it fail - this one applied cleanly on my environment with git apply -v
so .. hopeful
Comment #23
jec006 CreditAttribution: jec006 commentedAttaching a version that will work with drush make
Comment #25
LarsKramer CreditAttribution: LarsKramer commentedSubscribing. Shoudn't this be applied to 8.x-dev first?
Comment #26
jec006 CreditAttribution: jec006 commentedPatch in 20 works.
Comment #27
Everett Zufelt CreditAttribution: Everett Zufelt commentedComment #28
Everett Zufelt CreditAttribution: Everett Zufelt commented#23: 992540-3-reset_flood_limit_on_password_reset-drush.patch queued for re-testing.
Comment #29
Everett Zufelt CreditAttribution: Everett Zufelt commented#20: 992540-3-reset_flood_limit_on_password_reset.patch queued for re-testing.
Comment #31
Everett Zufelt CreditAttribution: Everett Zufelt commented#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?
Comment #32
jec006 CreditAttribution: jec006 commentedIt 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.
Comment #33
jec006 CreditAttribution: jec006 commentedCan we get a RTBC for the patch against 8.x so this can get in?
Comment #34
Matt V. CreditAttribution: Matt V. commentedI 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.
Comment #35
Heine CreditAttribution: Heine commentedThe 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.
Comment #36
Matt V. CreditAttribution: Matt V. commentedHere's an updated patch with the code for clearing the IP flood event removed.
Comment #37
MHLut CreditAttribution: MHLut commentedThis 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.
Comment #38
basvredelingApplied 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.
Comment #39
robertstaddon CreditAttribution: robertstaddon commentedI would really like to see #20 get implemented in the D7.15 release. What would it take to do this?
Comment #40
rickmanelius CreditAttribution: rickmanelius commentedI'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.
Comment #41
rickmanelius CreditAttribution: rickmanelius commentedTechnically 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! :)
Comment #42
kid_icarus CreditAttribution: kid_icarus commentedThanks @rickmanelius!
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.
Comment #43
rickmanelius CreditAttribution: rickmanelius commentedClearing out whitespace nastiness as noted from #42. Resubmitting
Comment #44
rickmanelius CreditAttribution: rickmanelius commentedTrying my hand at interdiffs... resubmitting as patch 44. Thanks @kid_icarus for the information on how/why do to that! :)
Comment #45
kid_icarus CreditAttribution: kid_icarus commented@rickmanelius Sweet! The interdiff looks good. It looks like #44 didn't apply cleanly to HEAD. Re-rolled.
Comment #47
kid_icarus CreditAttribution: kid_icarus commented#45: reset_flood_limit_on_password_reset-992540-44.patch queued for re-testing.
Comment #49
kid_icarus CreditAttribution: kid_icarus commentedOk, 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? :)
Comment #50
kid_icarus CreditAttribution: kid_icarus commentedSome coding standards/commenting standards cleanup.
Comment #51
ro-no-lo CreditAttribution: ro-no-lo commentedAs 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.
Comment #53
MHLut CreditAttribution: MHLut commentedActually, the superuser gets unblocked when you clear the flood table, but that's not desired behaviour, as you'll unblock everyone then.
Comment #54
rickmanelius CreditAttribution: rickmanelius commentedHey @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?
Comment #55
ro-no-lo CreditAttribution: ro-no-lo commented@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.
Comment #56
rickmanelius CreditAttribution: rickmanelius commentedHi @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.
Comment #57
rickmanelius CreditAttribution: rickmanelius commentedPatch 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?
Comment #58
rickmanelius CreditAttribution: rickmanelius commentedOne final comment. In the patch we see the following method to clear the
But we do know the IP address. For example, here are two records from one of my databases;
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?
Comment #59
rickmanelius CreditAttribution: rickmanelius commentedActually 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!
Comment #60
catchLet's add a new API function to the flood API rather than accessing the database directly in user module.
Comment #61
rickmanelius CreditAttribution: rickmanelius commentedWell 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?
Comment #62
camilo.mejia CreditAttribution: camilo.mejia commentedIt will be ported to drupal 7? it,s a big problem on sites with many users.
Comment #63
rickmanelius CreditAttribution: rickmanelius commentedI'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.
Comment #64
doitDave CreditAttribution: doitDave commentedSubscribing 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!
Comment #65
AnybodyHi @ 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!
Comment #66
doitDave CreditAttribution: doitDave commented@65 obviously not, so "selbst ist der Mann", as they say here in Germany. Unfollowing.
Comment #67
mackie1 CreditAttribution: mackie1 commentedFlood_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.
Comment #68
klidifia CreditAttribution: klidifia commentedThese 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.
Comment #69
klidifia CreditAttribution: klidifia commentedAlso, 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'));
Comment #70
klidifia CreditAttribution: klidifia commentedMy view is that:
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.
Comment #71
valthebaldBumping version
Comment #72
valthebaldHere's the patch for D7, and I hope to review/provide 8.1.x one soon
Comment #73
valthebaldThis patch is POC only, I set to Needs review only for the testbot.
The issue needs a test, tagging
Comment #74
ndobromirov CreditAttribution: ndobromirov at FFW commentedComment #75
ndobromirov CreditAttribution: ndobromirov at FFW commentedHi, I've updated the test to incorporate the changes from the patch.
Comment #76
valthebaldRemoving "Needs tests" as we have them now
Comment #80
aerozeppelin CreditAttribution: aerozeppelin at California State University San Bernardino commentedUpdates to patch #75.
Comment #82
valthebaldI dare RTBC that
Comment #85
alexpottShould 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.
Comment #86
valthebald@alexpott: something like this?
Comment #87
valthebaldComment #88
valthebaldComment #89
valthebaldPardon, unneeded files submitted. Removed
Comment #91
valthebaldTestbot hiccup?
Comment #93
ndobromirov CreditAttribution: ndobromirov at FFW commentedI'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.
Comment #94
lachezar.valchev CreditAttribution: lachezar.valchev as a volunteer commentedI 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
Comment #95
tstoecklerThis should be injected.
Comment #96
ndobromirov CreditAttribution: ndobromirov at FFW commentedHere 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.
Comment #97
lachezar.valchev CreditAttribution: lachezar.valchev as a volunteer commentedLatest patch tested.
Looks good.
RTBC
Comment #98
xjm@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!
Comment #99
valthebaldThis 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
Comment #100
xjmThanks @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.
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.These changes appear to be out of scope for the issue.
Thanks!
Comment #101
xjm(Also saving additional issue credit.)
Comment #102
ndobromirov CreditAttribution: ndobromirov at FFW commentedHere is a new patch with all the translation stuff gone + diff.
Comment #103
valthebald@ndobromirov
No need for strtr() here IMO
Can be smth like
Comment #104
ndobromirov CreditAttribution: ndobromirov at FFW commentedI 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.
Comment #105
yogeshmpawarRerolled the patch because previous patch failed to apply & also added a suggestion as per comment #103.
Comment #107
ndobromirov CreditAttribution: ndobromirov at FFW commented@Yogesh
What has changed?
Comment #108
ndobromirov CreditAttribution: ndobromirov at FFW commentedHere is a re-roll with no strtr in tests against the patch from 102.
Comment #110
yogeshmpawar@ndobromirov - your current & previous patch failed to apply, that's why i uploaded patch. also #108 patch failed to apply, please check
Comment #111
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedComment #112
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedComment #113
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedComment #114
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedComment #115
Munavijayalakshmi CreditAttribution: Munavijayalakshmi at Valuebound commenteduse short array syntax (new coding standard).
Fixed the short array syntax error and attached new patch.
Comment #116
valthebaldReplaced (deprecated) \Drupal::url() call
Comment #117
valthebaldAlso, tagging
Comment #118
ndobromirov CreditAttribution: ndobromirov at FFW commentedHere 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.
Comment #119
valthebald@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
Comment #120
cilefen CreditAttribution: cilefen commentedThese apparently out-of-scope changes remain as mentioned in #100.
Comment #121
valthebald@cilefen I've removed irrelevant part, ensured that only newly added strings are not translated
Comment #122
ndobromirov CreditAttribution: ndobromirov at FFW commentedThe change reflects the last review and it fixes the issues.
Moving back to RTBC.
Comment #123
yogeshmpawarAny Update on this issue ?
Comment #124
cilefen CreditAttribution: cilefen commentedIn the last four days? No.
Comment #126
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedNitpick: description of the
$flood
parameter should be:A flood service instance.
Comment #127
valthebald@MegaChriz thanks for the comment! Made parameter and member comments consistent with Drupal\user\Form\UserLoginForm
Comment #128
ndobromirov CreditAttribution: ndobromirov at FFW commentedIssues from #126 are resolved in #127.
Moving back to RTBC.
Comment #129
catchThis 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:
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.
Comment #131
vijaycs85Here is a patch for 8.3.x as per #129
Created #2880910: [D7] Nothing clears the "5 failed login attempts" security message when a user resets their own password for 7.x port.
Comment #132
vijaycs85As we have a separate issue for 7.x, removing the tag.
Comment #133
vijaycs85Just missed the comment updates in #129
Comment #134
valthebaldI 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
Comment #135
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI 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.
Comment #136
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedAlso 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.
Comment #138
cilefen CreditAttribution: cilefen commentedThank you for the heads-up @David_Rothstein. I reverted it.
Comment #139
tedbowComment #140
tedbowSorry 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?
Comment #141
valthebaldAddressing comments #35 and #135
Comment #142
valthebaldComment #143
valthebaldRemoved erroneous comment changes
Comment #148
borisson_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.
Comment #149
pguillard CreditAttribution: pguillard commentedPatch rerolled, I added use of AssertMailTrait.
I guess one error remains.
Comment #151
savkaviktor16@gmail.com CreditAttribution: savkaviktor16@gmail.com at Skilld commentedComment #152
pguillard CreditAttribution: pguillard commentedThis patch needed another reroll
Comment #155
borisson_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
This is the part of the code that is not working.
We also still need the issue summary to be updated.
Comment #156
volegerRemoved 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).
Here the login by the new user, and it's blocked.
Looks like ip blocking triggered.
Comment #157
mr.baileysThe 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.
Comment #159
valthebaldI dare mark this RTBC
Comment #160
alexpott@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.
Wow this is icky - why is this logic not on the flood service? We should have issue about this somewhere.
Out-of-scope.
This all looks out-of-scope.
Whilst these changes make sense I'm not sure they are required or in scope.
This looks unnecessary.
I think we can use
$this->assertSession->elementNotExists()
but also I'm not sure these changes are in scope.As far as I can see these changes are not need to make this change.
Comment #161
valthebaldRerolled the patch, taking into account comments from @alexpott
Comment #164
joseph.olstadpatch needs reroll , again. For both 8.8.x and 8.9.x
Comment #165
pguillard CreditAttribution: pguillard commentedThis is the rerolled patch.
Comment #167
heddnStill has failing tests.
Comment #168
agrochal CreditAttribution: agrochal at Google Code-In commentedI attach interdiff file for last two patches(#161 and #165).
Comment #169
mr.baileysTest 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.xComment #170
heddnManually tested the patch. It works fine.
Comment #171
catchLooks 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.
Comment #172
alexpottThe 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) oruser.http_login
anduser.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 touser.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.
Fixing this call to D10 deprecated code is not necessary.
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 think this should be log in and not login.
This is a negative assertion - let's do a positive assertion
Comment #174
joseph.olstadLatest 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?
Comment #176
joseph.olstadTest
Comment #177
joseph.olstad@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
Comment #178
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedHere I have fixed one failed test of patch #177 and fixed some CS issues as well.
Comment #179
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedI think now it's ready for review.
Comment #180
joseph.olstadThanks Ravi, I think this is it.
Comment #181
paulocsRemoving unused variable $user_limit. I'll let it as RTBC as only a small change was made.
Comment #182
joseph.olstadThanks @paulocs good eyes, I think it is finally ready for @alexpott / @catch or another core maintainer.
Comment #183
joseph.olstad9 years, 10 months and counting ... No pressure or anything, but I've gotten a lot more gray hairs in the past 9 years.
Comment #185
joseph.olstadpatch is still good, the noise is from HEAD and/or a testbot glitch
Comment #186
catchThis needs a change record - controllers aren't API, but it's not impossible that someone has extended it.
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.
Comment #188
paulocsI addressed #186. The link with the CR: https://www.drupal.org/node/3224905
Do we need to write a deprecation notice for the
resetPassLogin
method?Comment #190
evilehk CreditAttribution: evilehk at Breakthrough Technologies, LLC for Breakthrough Technologies, LLC commentedThe 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.
Comment #191
ranjith_kumar_k_u CreditAttribution: ranjith_kumar_k_u at Zyxware Technologies commentedRemoved following parameter doc from the patch because which is already exists
Comment #194
nod_Patch looks good but doesn't apply to 10.1.x
Comment #195
pradhumanjain2311 CreditAttribution: pradhumanjain2311 at OpenSense Labs for DrupalFit commentedFix Patch #191 for 10.1.x.
Please review.
Comment #196
pradhumanjain2311 CreditAttribution: pradhumanjain2311 at OpenSense Labs for DrupalFit commentedComment #197
louis-cuny CreditAttribution: louis-cuny at Daille-daille commentedPatches didn't apply to drupal 9.5
Here is a d9.5 compatible patch, rerolled from 191
Comment #198
catchThe 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.
Comment #199
catchNeeds a reroll for 10.1.x
Comment #200
quietone CreditAttribution: quietone at PreviousNext commentedRerolled for 10.1.x
Comment #201
sahilgidwani CreditAttribution: sahilgidwani at Axelerant commented#200 is working fine for Drupal version 10.1.x
Comment #205
catchThe 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.
Comment #206
sahilgidwani CreditAttribution: sahilgidwani at Axelerant commentedComment #207
Anybody@catch WHAO! Thank you so much! Still plenty of such issues to solve ... ;) Hello frustration.