Bug 41094 - search_anonymize_candidates returns too many candidates when FailedLoginAttempts is empty
Summary: search_anonymize_candidates returns too many candidates when FailedLoginAttem...
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords: release-notes-needed
Depends on:
Blocks:
 
Reported: 2025-10-24 09:05 UTC by Marcel de Rooy
Modified: 2025-10-29 10:02 UTC (History)
7 users (show)

See Also:
GIT URL:
Initiative type: ---
Sponsorship status: ---
Crowdfunding goal: 0
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 41094: Improve test of Fix search_anonymize_candidates/account_locked (5.67 KB, patch)
2025-10-28 14:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 41094: Fix search_anonymize_candidates, locked_account (2.88 KB, patch)
2025-10-28 14:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 41094: Improve test of search_anonymize_candidates/account_locked (5.66 KB, patch)
2025-10-28 14:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 41094: Fix search_anonymize_candidates, locked_account (2.88 KB, patch)
2025-10-28 14:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 41094: Improve test of search_anonymize_candidates/account_locked (5.67 KB, patch)
2025-10-28 18:50 UTC, David Nind
Details | Diff | Splinter Review
Bug 41094: Fix search_anonymize_candidates, locked_account (2.88 KB, patch)
2025-10-28 18:50 UTC, David Nind
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2025-10-24 09:05:48 UTC
From a discussion on Mattermost with David Little.

The arguable lines are:

my $fails = C4::Context->preference('FailedLoginAttempts') || 0;
$cond->{login_attempts} =
    [ -and => { '!=' => undef }, { -not_in => [ 0, 1 .. $fails - 1 ] } ];
Comment 1 Marcel de Rooy 2025-10-24 09:09:57 UTC
Thinking about this, if someone has the pref empty, we should better only select the -1 cases where an administrative lockout was placed (e.g. when unsubscribing GDPR consent). Since all positive values should not be viewed as locked.
Comment 2 Marcel de Rooy 2025-10-24 09:10:29 UTC
Did we always update login_attempts when the pref was empty btw?
Comment 3 Marcel de Rooy 2025-10-24 09:21:26 UTC
And we might do a check if FailedLoginAttempts is negative. Which it should not be obviously..
Comment 4 Marcel de Rooy 2025-10-28 14:20:33 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2025-10-28 14:20:35 UTC Comment hidden (obsolete)
Comment 6 Marcel de Rooy 2025-10-28 14:42:12 UTC
Created attachment 188515 [details] [review]
Bug 41094: Improve test of search_anonymize_candidates/account_locked

Test plan:
Run the test without applying the follow-up patch.
The test should fail. This shows that we need the fix and will help
avoid future regressions.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2025-10-28 14:42:14 UTC
Created attachment 188516 [details] [review]
Bug 41094: Fix search_anonymize_candidates, locked_account

We should not allow anonymizing patrons with failed login attempts
when the pref FailedLoginAttempts has not been set.

Test plan:
Run the test again from the previous patch. This should pass now.
Choose a patron with a positive value (say 55) in borrowers.login_attempts
or modify one. (NOTE: This patron will be anonymized in the test plan.)
Empty pref value for FailedLoginAttempts.
Set PatronAnonymizeDelay to 1.
Fill dateexpiry for this patron to yesterday.
Run cleanupdatabase.pl -confirm -v and check if the patron was NOT anonymized.
* Anonymized 0 patrons

Now set FailedLoginAttempts to a value (say 55) so that the patron is
considered as locked. (This unrealistic value is for testing only.)
MAKE SURE that you will not be anonymizing more patrons than you like
in the next step! (Set login_attempts to value < 55 for all other patrons.)
Run cleanupdatabase.pl -confirm -v again and check if the patron was anonymized.
* Anonymized 1 patrons
Restore the changed pref values.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 David Liddle 2025-10-28 16:02:26 UTC
(In reply to Marcel de Rooy from comment #0)
> From a discussion on Mattermost with David Little.
> 
> The arguable lines are:
> 
> my $fails = C4::Context->preference('FailedLoginAttempts') || 0;
> $cond->{login_attempts} =
>     [ -and => { '!=' => undef }, { -not_in => [ 0, 1 .. $fails - 1 ] } ];

FYI, that's LIDDLE.
Comment 9 David Nind 2025-10-28 18:50:09 UTC
Created attachment 188525 [details] [review]
Bug 41094: Improve test of search_anonymize_candidates/account_locked

Test plan:
Run the test without applying the follow-up patch.
The test should fail. This shows that we need the fix and will help
avoid future regressions.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Comment 10 David Nind 2025-10-28 18:50:13 UTC
Created attachment 188526 [details] [review]
Bug 41094: Fix search_anonymize_candidates, locked_account

We should not allow anonymizing patrons with failed login attempts
when the pref FailedLoginAttempts has not been set.

Test plan:
Run the test again from the previous patch. This should pass now.
Choose a patron with a positive value (say 55) in borrowers.login_attempts
or modify one. (NOTE: This patron will be anonymized in the test plan.)
Empty pref value for FailedLoginAttempts.
Set PatronAnonymizeDelay to 1.
Fill dateexpiry for this patron to yesterday.
Run cleanupdatabase.pl -confirm -v and check if the patron was NOT anonymized.
* Anonymized 0 patrons

Now set FailedLoginAttempts to a value (say 55) so that the patron is
considered as locked. (This unrealistic value is for testing only.)
MAKE SURE that you will not be anonymizing more patrons than you like
in the next step! (Set login_attempts to value < 55 for all other patrons.)
Run cleanupdatabase.pl -confirm -v again and check if the patron was anonymized.
* Anonymized 1 patrons
Restore the changed pref values.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Comment 11 David Nind 2025-10-28 18:55:31 UTC
Testing notes (using KTD):

1. I used Mary Burton's account for testing (borrowernumber=49).

2. Test plan step: Choose a patron with a positive value (say 55) in borrowers.login_attempts:
   - use this SQL to update:
     . koha-mysql kohadev
     . update borrowers set login_attempts=55 where borrowernumber='49';

3. Test plan step: Fill dateexpiry for this patron to yesterday:
   - Edit the patron and change the 'Expiry date (...)' field under the library management section

4. Test plan step: Run cleanupdatabase.pl....:
   - misc/cronjobs/cleanup_database.pl -confirm -v
Comment 12 Marcel de Rooy 2025-10-29 10:00:19 UTC
(In reply to David Nind from comment #10)
> Signed-off-by: David Nind <david@davidnind.com>

Great. Thx
Comment 13 Marcel de Rooy 2025-10-29 10:02:18 UTC
(In reply to David Liddle from comment #8)
> (In reply to Marcel de Rooy from comment #0)
> > From a discussion on Mattermost with David Little.
> > 
> > The arguable lines are:
> > 
> > my $fails = C4::Context->preference('FailedLoginAttempts') || 0;
> > $cond->{login_attempts} =
> >     [ -and => { '!=' => undef }, { -not_in => [ 0, 1 .. $fails - 1 ] } ];
> 
> FYI, that's LIDDLE.

Sorry, failed_name_attempt++