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 ] } ];
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.
Did we always update login_attempts when the pref was empty btw?
And we might do a check if FailedLoginAttempts is negative. Which it should not be obviously..
Created attachment 188511 [details] [review] Bug 41094: Improve test of Fix 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>
Created attachment 188512 [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>
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>
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>
(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.
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>
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>
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
(In reply to David Nind from comment #10) > Signed-off-by: David Nind <david@davidnind.com> Great. Thx
(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++