[Bug 41094] New: search_anonymize_candidates returns too many candidates when FailedLoginAttempts is empty
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 Bug ID: 41094 Summary: search_anonymize_candidates returns too many candidates when FailedLoginAttempts is empty Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: major Priority: P5 - low Component: Patrons Assignee: koha-bugs@lists.koha-community.org Reporter: m.de.rooy@rijksmuseum.nl QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle@bywatersolutions.com
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 ] } ]; -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david.liddle@wycliff.de, | |katrin.fischer@bsz-bw.de -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 --- Comment #1 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@gmail.com --- Comment #2 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Did we always update login_attempts when the pref was empty btw? -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 --- Comment #3 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- And we might do a check if FailedLoginAttempts is negative. Which it should not be obviously.. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |m.de.rooy@rijksmuseum.nl |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 Lauren Denny <lauren_denny@sil.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lauren_denny@sil.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 --- Comment #4 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 188511 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=188511&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 --- Comment #5 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 188512 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=188512&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 --- Comment #6 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 188515 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=188515&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #188512|0 |1 is obsolete| | --- Comment #7 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 188516 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=188516&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #188511|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 --- Comment #8 from David Liddle <david.liddle@wycliff.de> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #188515|0 |1 is obsolete| | --- Comment #9 from David Nind <david@davidnind.com> --- Created attachment 188525 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=188525&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #188516|0 |1 is obsolete| | --- Comment #10 from David Nind <david@davidnind.com> --- Created attachment 188526 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=188526&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |release-notes-needed CC| |david@davidnind.com --- Comment #11 from 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 --- Comment #12 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to David Nind from comment #10)
Signed-off-by: David Nind <david@davidnind.com>
Great. Thx -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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++ -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@openfifth.c | |o.uk QA Contact|testopia@bugs.koha-communit |martin.renvoize@openfifth.c |y.org |o.uk -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 --- Comment #14 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 189686 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=189686&action=edit 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> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 --- Comment #15 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 189687 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=189687&action=edit 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> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #188525|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #188526|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #16 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- All makes sense to me. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to main Version(s)| |25.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 --- Comment #17 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Nice work everyone! Pushed to main for 25.11 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|25.11.00 |25.11.00,25.05.06 released in| | Status|Pushed to main |Pushed to stable -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 --- Comment #18 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Nice work everyone! Pushed to 25.05.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 --- Comment #19 from David Liddle <david.liddle@wycliff.de> --- Thank you, all, for giving your attention to this bug and resolving it! Please apply the changes to 24.11 LTS as well. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |Pushed to oldstable Version(s)|25.11.00,25.05.06 |25.11.00,25.05.06,24.11.11 released in| | CC| |fridolin.somers@biblibre.co | |m --- Comment #20 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to 24.11.x for 24.11.11 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the| |This bug report fixes the release notes| |selection of borrowers to | |be anonymized when that | |feature is enabled but | |FailedLoginAttempts is | |empty. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|release-notes-needed | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41094 Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to oldstable |Needs documenting Version(s)|25.11.00,25.05.06,24.11.11 |25.11.00,25.05.06,24.11.11, released in| |22.11.26 CC| |wainuiwitikapark@catalyst.n | |et.nz --- Comment #21 from Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> --- Applied to 22.11 for 22.11.36 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org