Bug 41094

Summary: search_anonymize_candidates returns too many candidates when FailedLoginAttempts is empty
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: PatronsAssignee: Marcel de Rooy <m.de.rooy>
Status: Signed Off --- QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: david.liddle, david, gmcharlt, jonathan.druart, katrin.fischer, kyle, lauren_denny
Version: MainKeywords: release-notes-needed
Hardware: All   
OS: All   
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
Bug 41094: Fix search_anonymize_candidates, locked_account
Bug 41094: Improve test of search_anonymize_candidates/account_locked
Bug 41094: Fix search_anonymize_candidates, locked_account
Bug 41094: Improve test of search_anonymize_candidates/account_locked
Bug 41094: Fix search_anonymize_candidates, locked_account

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++