If a Koha instance has a borrower with an blank/empty userid and login_attempts over the threshold defined by FailedLoginAttempts, a user will see "Error: This account has been locked!" when visiting the staff interface for the first time and without supplying any login details. This is because we're doing a Koha::Patrons->find with an empty string! The solution is very simple...
Created attachment 128984 [details] [review] Bug 29783: Add account lockout patron userid lookup condition This patch checks that $q_userid is not blank before trying to do a patron lookup. This prevents a spurrious account locked message from appearing to the user when there is a user in Koha that has an empty userid and too many login_attempts. Test plan: 0) Do not apply the patch yet 1) Set system preference FailedLoginAttempts to 3 2) Modify a borrower to have an empty userid and 10 login_attempts e.g. update borrowers set userid = '', login_attempts = 10 where cardnumber = 23529001223636; 3) Visit the staff interface (e.g. http://localhost:8081/) 4) Note the message "Error: This account has been locked!" even though you have not tried to log in 5) Apply the patch 6) Visit the staff interface (e.g. http://localhost:8081/) 7) Note that there is no error message about your account being locked 8) Profit.
Hi David, 2 things here: 1) How did you manage to add a user with an empty userid? Using the GUI and using the patron import tool this should always be automatically generated if not given. We can use SQL for the test of course. 2) You write in your test plan: 1) Set system preference FailedLoginAttempts to 3 2) Modify a borrower to have an empty userid and 10 login_attempts So in this case the user account SHOULD be locked, shouldn't it?
(In reply to Katrin Fischer from comment #2) > Hi David, > > 2 things here: > > 1) How did you manage to add a user with an empty userid? Using the GUI and > using the patron import tool this should always be automatically generated > if not given. We can use SQL for the test of course. I'm not sure how it happened. My guess would be some migration script. For testing, I used SQL. > 2) > You write in your test plan: > 1) Set system preference FailedLoginAttempts to 3 > 2) Modify a borrower to have an empty userid and 10 login_attempts > > So in this case the user account SHOULD be locked, shouldn't it? Yes, the user account should be locked. The purpose of that test is to see whether or not that user account is being used automatically by Koha without any user input.
This may be unlikly to happen but could be a data leak issue.
I think we should not fetch Koha::Patron at this place. For logged-in patron, its datas are fetch here : https://git.koha-community.org/Koha-community/Koha/src/commit/2f39f426b5e748973dc8d8348f1304dde64ac909/C4/Auth.pm#L1118 We should add 'account_locked' there no ?
+ my $patron = Koha::Patrons->find({ userid => $q_userid }) if $q_userid; "my $var if condition" does not meet the coding guidelines, at least I hope so :)
Looks like the problematic line is gone now... although I think it may have broken some template variables... In this scope, I don't think $patorn is even initialized... too_many_login_attempts => ( $patron and $patron->account_locked ), password_has_expired => ( $patron and $patron->password_expired ), We seem to fetch the patron object quite a few times in checkauth()...
Looks like I just made an error there. The line seems to just be in a different spot or something. Anyway, this has fallen down the list of priorities in any case...