From 8fafcd4b697977241704a9b6220434dc0fe177d5 Mon Sep 17 00:00:00 2001 From: Martin Renvoize 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" %]
  • The patron has a debt of [% balance | $Price %].
  • -
    - [% INCLUDE 'csrf-token.inc' %] - - - - -
    + [% IF Koha.Preference('AllowFineOverrideRenewing') %] +
    + [% INCLUDE 'csrf-token.inc' %] + + + + +
    + [% END %] [% ELSIF error == "on_reserve" %]

    [% INCLUDE 'biblio-title.inc' biblio=item.biblio link = 1 %] ( [% item.barcode | html %] ): This item is on hold for another patron.

    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 += "" + __( - "Cannot renew, he patron has a debt of " + + "Cannot renew, the patron has a debt of " + parseFloat(oObj.fine).format_price() ) + ""; 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