Bugzilla – Attachment 188338 Details for
Bug 23415
Notify patron fines when renewing
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 23415: (QA follow-up) Fix critical logic and security issues
Bug-23415-QA-follow-up-Fix-critical-logic-and-secu.patch (text/plain), 9.20 KB, created by
Martin Renvoize (ashimema)
on 2025-10-23 11:25:33 UTC
(
hide
)
Description:
Bug 23415: (QA follow-up) Fix critical logic and security issues
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2025-10-23 11:25:33 UTC
Size:
9.20 KB
patch
obsolete
>From 8fafcd4b697977241704a9b6220434dc0fe177d5 Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@openfifth.co.uk> >Date: Thu, 23 Oct 2025 11:44:52 +0100 >Subject: [PATCH] Bug 23415: (QA follow-up) Fix critical logic and security > issues > >This commit addresses several critical issues found in QA review: > >1. Fixed critical logic bug in C4::Circulation::CanBookBeRenewed > - Removed incorrect check of AllowFineOverrideRenewing preference > - The function now always returns 'too_much_oweing' error when > patron balance exceeds FineNoRenewals limit > - AllowFineOverrideRenewing should only control UI override > capability, not the core renewal check > >2. Added permission check in circ/renew.pl > - Override is now only allowed when both override_debt parameter > is set AND AllowFineOverrideRenewing preference is enabled > - Prevents security issue where staff could bypass the preference > by crafting POST requests > >3. Fixed template to conditionally display override button > - Override button in renew.tt now only shows when > AllowFineOverrideRenewing is enabled > - Prevents confusion when override is not permitted > >4. Added comprehensive test coverage > - Tests verify CanBookBeRenewed behavior with AllowFineOverrideRenewing > both enabled and disabled > - Confirms error is always returned regardless of preference setting > - Increased test count from 116 to 120 > >5. Fixed minor issues > - Fixed typo: "he patron" -> "the patron" in checkouts.js > - Fixed typo: OPACFineNoRenewalsIncludeCredit -> > OPACFineNoRenewalsIncludeCredits in test > >All tests pass successfully. >--- > C4/Circulation.pm | 15 +++++++-------- > circ/renew.pl | 2 +- > .../intranet-tmpl/prog/en/modules/circ/renew.tt | 16 +++++++++------- > koha-tmpl/intranet-tmpl/prog/js/checkouts.js | 2 +- > t/db_dependent/Circulation.t | 17 +++++++++++++++-- > 5 files changed, 33 insertions(+), 19 deletions(-) > >diff --git a/C4/Circulation.pm b/C4/Circulation.pm >index 04d8f447a77..85e5d6a1619 100644 >--- a/C4/Circulation.pm >+++ b/C4/Circulation.pm >@@ -3238,13 +3238,12 @@ sub CanBookBeRenewed { > $final_unseen_renewal = ( C4::Context->preference('UnseenRenewals') > && $issuing_rule->{unseen_renewals_allowed} == ( $issue->unseen_renewals + 1 ) ) ? 1 : 0; > >- my $overduesblockrenewing = C4::Context->preference('OverduesBlockRenewing'); >- my $finesblockrenewing = C4::Context->preference("FineNoRenewals"); >- my $restrictionblockrenewing = C4::Context->preference('RestrictionBlockRenewing'); >- my $allowfineoverriderenewing = C4::Context->preference("AllowFineOverrideRenewing"); >- my $restricted = $patron->is_debarred; >- my $hasoverdues = $patron->has_overdues; >- my $balance = $patron->account->balance; >+ my $overduesblockrenewing = C4::Context->preference('OverduesBlockRenewing'); >+ my $finesblockrenewing = C4::Context->preference("FineNoRenewals"); >+ my $restrictionblockrenewing = C4::Context->preference('RestrictionBlockRenewing'); >+ my $restricted = $patron->is_debarred; >+ my $hasoverdues = $patron->has_overdues; >+ my $balance = $patron->account->balance; > > if ( $restricted and $restrictionblockrenewing ) { > return ( 0, 'restriction' ); >@@ -3252,7 +3251,7 @@ sub CanBookBeRenewed { > || ( $issue->is_overdue and $overduesblockrenewing eq 'blockitem' ) ) > { > return ( 0, 'overdue' ); >- } elsif ( $finesblockrenewing && $balance > $finesblockrenewing && !$allowfineoverriderenewing ) { >+ } elsif ( $finesblockrenewing && $balance > $finesblockrenewing ) { > return ( 0, 'too_much_oweing' ); > } > >diff --git a/circ/renew.pl b/circ/renew.pl >index af41f311ae1..ecbc33e105c 100755 >--- a/circ/renew.pl >+++ b/circ/renew.pl >@@ -96,7 +96,7 @@ if ( $op eq 'cud-renew' && $barcode ) { > } > > if ( $error && ( $error eq 'too_much_oweing' or $error eq 'auto_too_much_oweing' ) ) { >- if ($override_debt) { >+ if ( $override_debt && C4::Context->preference("AllowFineOverrideRenewing") ) { > $can_renew = 1; > $error = undef; > } else { >diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/renew.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/renew.tt >index 46f7f6fe83c..45593c5bc43 100644 >--- a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/renew.tt >+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/renew.tt >@@ -141,13 +141,15 @@ > [% END %] > [% ELSIF error == "too_much_oweing" %] > <li>The patron has a debt of [% balance | $Price %].</li> >- <form method="post" action="/cgi-bin/koha/circ/renew.pl"> >- [% INCLUDE 'csrf-token.inc' %] >- <input type="hidden" name="barcode" value="[% item.barcode | html %]" /> >- <input type="hidden" name="override_debt" value="1" /> >- <input type="hidden" name="op" value="cud-renew" /> >- <button type="submit" class="approve"><i class="fa fa-check"></i>Renew checkout(s)</button> >- </form> >+ [% IF Koha.Preference('AllowFineOverrideRenewing') %] >+ <form method="post" action="/cgi-bin/koha/circ/renew.pl"> >+ [% INCLUDE 'csrf-token.inc' %] >+ <input type="hidden" name="barcode" value="[% item.barcode | html %]" /> >+ <input type="hidden" name="override_debt" value="1" /> >+ <input type="hidden" name="op" value="cud-renew" /> >+ <button type="submit" class="approve"><i class="fa fa-check"></i>Renew checkout(s)</button> >+ </form> >+ [% END %] > [% ELSIF error == "on_reserve" %] > <p>[% INCLUDE 'biblio-title.inc' biblio=item.biblio link = 1 %] ( [% item.barcode | html %] ): This item is on hold for another patron.</p> > >diff --git a/koha-tmpl/intranet-tmpl/prog/js/checkouts.js b/koha-tmpl/intranet-tmpl/prog/js/checkouts.js >index 52de8b770a0..c6088f24b78 100644 >--- a/koha-tmpl/intranet-tmpl/prog/js/checkouts.js >+++ b/koha-tmpl/intranet-tmpl/prog/js/checkouts.js >@@ -476,7 +476,7 @@ function LoadIssuesTable() { > msg += > "<span class='renewals-disabled'>" + > __( >- "Cannot renew, he patron has a debt of " + >+ "Cannot renew, the patron has a debt of " + > parseFloat(oObj.fine).format_price() > ) + > "</span>"; >diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t >index ab60ab75b00..8aef1fb44df 100755 >--- a/t/db_dependent/Circulation.t >+++ b/t/db_dependent/Circulation.t >@@ -498,7 +498,7 @@ subtest "GetIssuingCharges tests" => sub { > > my ( $reused_itemnumber_1, $reused_itemnumber_2 ); > subtest "CanBookBeRenewed tests" => sub { >- plan tests => 116; >+ plan tests => 120; > > C4::Context->set_preference( 'ItemsDeniedRenewal', '' ); > >@@ -1354,7 +1354,7 @@ subtest "CanBookBeRenewed tests" => sub { > ); > C4::Context->set_preference( 'OPACFineNoRenewalsBlockAutoRenew', '1' ); > C4::Context->set_preference( 'FineNoRenewals', '10' ); >- C4::Context->set_preference( 'OPACFineNoRenewalsIncludeCredit', '1' ); >+ C4::Context->set_preference( 'OPACFineNoRenewalsIncludeCredits', '1' ); > my $fines_amount = 5; > my $account = Koha::Account->new( { patron_id => $renewing_borrowernumber } ); > $account->add_debit( >@@ -1905,6 +1905,19 @@ subtest "CanBookBeRenewed tests" => sub { > ( $renewokay, $error ) = CanBookBeRenewed( $renewing_borrower_obj, $issue_1 ); > is( $renewokay, 0, 'Cannot renew, too much debt and FineNoRenewals=1' ); > is( $error, 'too_much_oweing', 'Cannot renew, debt not allowed (returned code is too_much_oweing)' ); >+ >+ # AllowFineOverrideRenewing should not affect CanBookBeRenewed behavior >+ # The preference only controls whether staff can override in the UI >+ t::lib::Mocks::mock_preference( 'AllowFineOverrideRenewing', 1 ); >+ ( $renewokay, $error ) = CanBookBeRenewed( $renewing_borrower_obj, $issue_1 ); >+ is( $renewokay, 0, 'Cannot renew, too much debt even with AllowFineOverrideRenewing enabled' ); >+ is( $error, 'too_much_oweing', 'Error code still too_much_oweing with AllowFineOverrideRenewing enabled' ); >+ >+ t::lib::Mocks::mock_preference( 'AllowFineOverrideRenewing', 0 ); >+ ( $renewokay, $error ) = CanBookBeRenewed( $renewing_borrower_obj, $issue_1 ); >+ is( $renewokay, 0, 'Cannot renew, too much debt with AllowFineOverrideRenewing disabled' ); >+ is( $error, 'too_much_oweing', 'Error code still too_much_oweing with AllowFineOverrideRenewing disabled' ); >+ > C4::Context->dbh->do("DELETE FROM accountlines"); > }; > >-- >2.51.0
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 23415
:
92039
|
92541
|
98442
|
98634
|
98996
|
98997
|
105893
|
105894
|
108392
|
109871
|
109872
|
109876
|
109880
|
116919
|
116920
|
116921
|
116922
|
118175
|
118176
|
118177
|
118178
|
118183
|
119300
|
119301
|
119302
|
119303
|
121472
|
139670
|
139671
|
139672
|
139673
|
139674
|
146415
|
148198
|
148199
|
148200
|
148201
|
148202
|
152776
|
152777
|
152778
|
152779
|
152780
|
152781
|
154595
|
154596
|
154597
|
154598
|
154599
|
154600
|
158302
|
158303
|
158304
|
158305
|
158306
|
158307
|
161964
|
161965
|
161966
|
161967
|
161968
|
161969
|
162171
|
162172
|
162173
|
162174
|
162175
|
162176
|
162177
|
162178
|
162179
|
162180
|
171620
|
171621
|
171622
|
171623
|
171983
|
171984
|
171985
|
171986
|
172906
|
172907
|
172908
|
172909
|
172910
|
185270
|
185271
|
185272
|
185273
|
185274
|
185275
|
186014
|
188331
|
188332
|
188333
|
188334
|
188335
|
188336
|
188337
| 188338 |
188339
|
188340
|
188341
|
188342