In fact, currently, tracked events are only deleted on cron run and on hook_user.
The patch will delete old entries on every form submission (prior to being consulted). I've included the delete code in the login_security_validate function
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | 499788_login_security_time_tracking_fix_4.patch | 8.7 KB | ilo |
| #5 | 499788_login_security_time_tracking_fix_3.patch | 8.8 KB | ilo |
| #4 | 499788_login_security_time_tracking_fix_2.patch | 7.8 KB | ilo |
| #2 | login_security_time_track_fix_1.patch | 1.62 KB | ilo |
| login_security_time_track_fix.patch | 1.09 KB | ilo |
Comments
Comment #1
deekayen commentedShould this be abstracted out as a function rather than repeated over and over again?
Comment #2
ilo commentedI guess you are right. Should I also consider the other DELETE queries to be abstracted? or will they wait for the 2.0..
Comment #3
deekayen commentedI think the others ought to be, too.
Comment #4
ilo commentedmmm.. several changes..
1st - Renamed function to support remove or expire tracked events with args, to be called here and there.
2nd - Default track time now is 1. This took me crazy for half an hour.. tests were working just because the DELETE part was not working at all, and having a default track time 0 it is imposible to have more than one event tracked: all events will be deleted the next submission.
3rd - I've also moved the place where the event is stored in the database. Before this fix, the event was stored after the user logs in, therefore, a valid login and a logout would leave a mark in the tracking table, instead of a clean table.
4th - Only clean tracked events on hoo_user 'update' if it's not being blocked, to keep the soft-blocking count still working.
5th - and last, I ve fixed in this patch also a minor issue for #495226: Login attempts message inconsistent, to remove messages like this: "you have used 4 of 2 attempts..".
In the .test file I just initialized the tracking time to 1.
It looks pretty stable for me now, I've tested by hand and verified the .test cases.
Comment #5
ilo commentedarfm.. Sorry, that was not the final patch. This one is. In the previous one I forgot to include the change in default track time value and change a test to verify messages are showing correctly.
This one gives: Overall results: 252 passes, 0 fails, and 0 exceptions
Comment #6
ilo commentedEven worse, forget the #4 and #5 patches, this is the good one. This is the reason why I hate to touch anything such late as now..
Comment #7
deekayen commented#6 looks ok, with the exception that I'm not a fan of restricting features turning themselves on by default without an admin knowing about it (i.e. the time tracking).
Comment #8
ilo commentedHaving time tracking window being 0 in the beginning will avoid events to be kept in the database since the installation, and being set to 1 hour, would ensure the last hour events are kept in case of an admin needing to activate any of the protections urgently.
On the other hand, no feature is set to on in this case, because all protections still remain deactivated, so no check/change is performed.
In any case we can put to 0 again, because tests now set this value from the begining.
Deekayen, will you be able to review it? or may I do other commits and reroll this one later?
Comment #9
deekayen commenteddo your commits
Comment #10
deekayen commented499788_login_security_time_tracking_fix_4.patch no longer applies cleanly
Comment #11
ilo commentedmm it's working for me.
- Verified as:
removing old folder
downloading a new "6-dev branch" copy
applying the patch
If you give more information I could try to see where are the changes, so you don't have to review the patch, or I could commit and go on.
Comment #12
ilo commentedCommited to DRUPAL-6--1