Bug 29783 - Account lockout message appears incorrectly for blank userid
Summary: Account lockout message appears incorrectly for blank userid
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-01-04 05:02 UTC by David Cook
Modified: 2023-04-03 05:54 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 29783: Add account lockout patron userid lookup condition (1.64 KB, patch)
2022-01-04 05:09 UTC, David Cook
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2022-01-04 05:02:14 UTC
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...
Comment 1 David Cook 2022-01-04 05:09:55 UTC
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.
Comment 2 Katrin Fischer 2022-01-08 22:32:55 UTC
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?
Comment 3 David Cook 2022-01-10 03:20:08 UTC
(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.
Comment 4 Fridolin Somers 2022-03-22 23:59:56 UTC
This may be unlikly to happen but could be a data leak issue.
Comment 5 Fridolin Somers 2022-03-23 00:13:25 UTC
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 ?
Comment 6 Marcel de Rooy 2022-04-04 19:56:29 UTC
+    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 :)
Comment 7 David Cook 2023-03-28 06:46:14 UTC
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()...
Comment 8 David Cook 2023-04-03 05:54:37 UTC
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...