Bug 27916 - Fee paid message can crash SIP server if paying fee that is not "renewable"
Summary: Fee paid message can crash SIP server if paying fee that is not "renewable"
Status: CLOSED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: Main
Hardware: All All
: P5 - low blocker (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 14825
  Show dependency treegraph
 
Reported: 2021-03-10 14:09 UTC by Kyle M Hall
Modified: 2021-12-13 21:10 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 27916: Fee paid message can crash SIP server if paying fee that is not "renewable" (2.34 KB, patch)
2021-03-10 14:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27916: Fee paid message can crash SIP server if paying fee that is not "renewable" (2.39 KB, patch)
2021-03-25 11:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27916: Add unit tests (3.03 KB, patch)
2021-04-16 12:06 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2021-03-10 14:09:45 UTC
I'm not entirely sure how this is possible, but I've confirmed my fix works on two separate occasions.

In the Koha::Account::pay, if a fine is "renewable" ( as determined by the method "renewable" the response to from the method "renew_item" is stored and returned by by pay().

Later in C4::SIP::Sip::MsgType::handle_fee_paid we assume that each element of the list in $pay_response->{renew_result} is a hashref that had an itemnumber. This is not true. We need to check for the itemnumber and then the item. If either do not exist, the SIP server will crash without responding.

My suspicion is that this is caused by Koha::Account::Line::renewable returning true while the corrosponding call to renew_item contains a simple "return;" statement if RenewAccruingItemWhenPaid is not enabled ( or there is not related item or patron ).

What happens here, at least for Bibliotheca self checks is that the machine will take payment, and send the fee paid to Koha via SIP. When Koha's SIP crashes and doesn't return a response, the self check machine will refund the money to the patron, *but* the payment still exists in Koha! Considering payments may be in the tens or even hundreds of dollars, this makes for a critical SIP bug.
Comment 1 Kyle M Hall 2021-03-10 14:14:30 UTC
Created attachment 118024 [details] [review]
Bug 27916: Fee paid message can crash SIP server if paying fee that is not "renewable"

I'm not entirely sure how this is possible, but I've confirmed my fix works on two separate occasions.

In the Koha::Account::pay, if a fine is "renewable" ( as determined by the method "renewable" the response to from the method "renew_item" is stored and returned by by pay().

Later in C4::SIP::Sip::MsgType::handle_fee_paid we assume that each element of the list in $pay_response->{renew_result} is a hashref that had an itemnumber. This is not true. We need to check for the itemnumber and then the item. If either do not exist, the SIP server will crash without responding.

My suspicion is that this is caused by Koha::Account::Line::renewable returning true while the corrosponding call to renew_item contains a simple "return;" statement if RenewAccruingItemWhenPaid is not enabled ( or there is not related item or patron ).

What happens here, at least for Bibliotheca self checks is that the machine will take payment, and send the fee paid to Koha via SIP. When Koha's SIP crashes and doesn't return a response, the self check machine will refund the money to the patron, *but* the payment still exists in Koha! Considering payments may be in the tens or even hundreds of dollars, this makes for a critical SIP bug.

I have been unable to recreate this bug on demand. So the test plan is:
1) Inspect the changes in the patch
2) Note that they make sense and prevent the SIP server from crashing
   if the itemnumber key has no data or the item is not found
Comment 2 Martin Renvoize 2021-03-25 10:39:29 UTC
Hi Kyle, I think bug 25508 would also resolve this issue without needing to touch SIP..  could you perhaps varify that for me?
Comment 3 Martin Renvoize 2021-03-25 11:17:48 UTC
Created attachment 118779 [details] [review]
Bug 27916: Fee paid message can crash SIP server if paying fee that is not "renewable"

I'm not entirely sure how this is possible, but I've confirmed my fix works on two separate occasions.

In the Koha::Account::pay, if a fine is "renewable" ( as determined by the method "renewable" the response to from the method "renew_item" is stored and returned by by pay().

Later in C4::SIP::Sip::MsgType::handle_fee_paid we assume that each element of the list in $pay_response->{renew_result} is a hashref that had an itemnumber. This is not true. We need to check for the itemnumber and then the item. If either do not exist, the SIP server will crash without responding.

My suspicion is that this is caused by Koha::Account::Line::renewable returning true while the corrosponding call to renew_item contains a simple "return;" statement if RenewAccruingItemWhenPaid is not enabled ( or there is not related item or patron ).

What happens here, at least for Bibliotheca self checks is that the machine will take payment, and send the fee paid to Koha via SIP. When Koha's SIP crashes and doesn't return a response, the self check machine will refund the money to the patron, *but* the payment still exists in Koha! Considering payments may be in the tens or even hundreds of dollars, this makes for a critical SIP bug.

I have been unable to recreate this bug on demand. So the test plan is:
1) Inspect the changes in the patch
2) Note that they make sense and prevent the SIP server from crashing
   if the itemnumber key has no data or the item is not found

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 4 Martin Renvoize 2021-03-25 11:18:51 UTC
OK, I've dug further into this and agree that Kyles patch is still required.. even with bug 25508.

Works as expected and the logic is sound.

Signing off
Comment 5 Martin Renvoize 2021-03-25 11:21:07 UTC
Note for QA...

The cases that can throw the issue here are

1. Deleted item for a fine on a patrons account when the fee is paid via sip
2. Deleted patron for a fine on a patrons account when the fee is paid via sip (I think this isn't actually possible)
3. Disabled 'RenewAccruingItemWhenPaid' preference

Point three is by far the simplest case to test ;)
Comment 6 Martin Renvoize 2021-03-30 12:56:37 UTC
Any chance of a unit test here Kyle?
Comment 7 Joonas Kylmälä 2021-04-01 10:33:30 UTC
I hope also that we could get unit test for this, i wasn't able to trigger the error manually.
Comment 8 Kyle M Hall 2021-04-01 11:42:18 UTC
(In reply to Martin Renvoize from comment #6)
> Any chance of a unit test here Kyle?

I'll see what I can do!
Comment 9 Kyle M Hall 2021-04-16 12:06:36 UTC
Created attachment 119718 [details] [review]
Bug 27916: Add unit tests
Comment 10 Kyle M Hall 2021-04-16 12:08:14 UTC
(In reply to Kyle M Hall from comment #8)
> (In reply to Martin Renvoize from comment #6)
> > Any chance of a unit test here Kyle?
> 
> I'll see what I can do!

I've spent a good amount of time trying to create a unit test for handle_fee_payment but something is going sideways. I've attached my non-functional unit tests if anyone wants to have a try at fixing them!

https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119718&action=diff
Comment 11 Victor Grousset/tuxayo 2021-04-16 14:16:34 UTC
I don't get how to pay a fee via SIP:
https://wiki.koha-community.org/wiki/Koha_SIP2_server_setup#Koha_SIP_CLI_emulator

Is that the Fee Paid thing in page 5 of SIP spec? https://wiki.koha-community.org/wiki/Koha_SIP2_server_setup

> The cases that can throw the issue here are
> 
> 1. Deleted item for a fine on a patrons account when the fee is paid via sip

What does deleting something for a fine means?
I got some hints in the chat. Create a lost fine tied to an item and then delete the lost fine. Is that correct? At which moment should the fee be payed?

> 3. Disabled 'RenewAccruingItemWhenPaid' preference

Given that syspref set, which operation would trigger the issue?
Comment 12 Kyle M Hall 2021-04-22 11:00:54 UTC
I'm upping the severity on this. Just had another partner lose $119 do to this bug. This is real money we are talking about.
Comment 13 Marcel de Rooy 2021-04-23 08:40:45 UTC
QAing
Comment 14 Marcel de Rooy 2021-04-23 09:38:12 UTC
    foreach my $result( @{$pay_response->{renew_result}} ) {
        next unless $result->{itemnumber};
        my $item = Koha::Items->find({ itemnumber => $result->{itemnumber} });
        next unless $item;
        if ($result->{success}) {
            push @success, '"' . $item->biblio->title . '"';
        } else {
            push @fail, '"' . $item->biblio->title . '" : ' . $failmap->{$result->{error}};
        }
    }

    my $msg = "";
    if (scalar @success > 0) {
        $msg.="The following items were renewed: " . join(", ", @success) . ". ";
    }
    if (scalar @fail > 0) {
        $msg.="The following items were not renewed: " . join(", ", @fail) . ".";
    }
    if (length $msg > 0) {
        $status->screen_msg($status->screen_msg . " $msg");
    }

It is clear that SIP should not crash on the lack of an itemnumber or an invalid one. But the current adjustments are just silent ignores.
I would at least recommend to not just jump to the next renew outcome but include these cases in the msg.

When looking at SIP and Accounts, I noticed that renew_item should already crash on an invalid itemnumber:
    my $itemnumber = $self->item->itemnumber;
So I am wondering if your adjustments are enough? Or should we already do something here too?

Is there any more information from syslog or so that you could share here about the actual bug?
If the problem is caused by an invalid itemnumber, do we miss some FKs somewhere in Accounts?
If the problem is related to the RenewAccruingItem preferences and the resulting empty return, should we touch renew_fine too? 

Needs some feedback/adjustments
Comment 15 Martin Renvoize 2021-04-23 10:04:16 UTC
(In reply to Marcel de Rooy from comment #14)
> When looking at SIP and Accounts, I noticed that renew_item should already
> crash on an invalid itemnumber:
>     my $itemnumber = $self->item->itemnumber;
> So I am wondering if your adjustments are enough? Or should we already do
> something here too?

In theory, one should always have called 'is_renewable' prior to calling 'renew_item'.. and 'is_renewable' performs the check for existing item.. so we shouldn't hit the problem you're foreseeing.. in theory.

I'm going to investigate the unit tests side again and hopefully at the same time I will work out where things are going.

Thanks for reviewing Marcel, this code is all rather complex and all the eyes and perspective we get are useful.
Comment 16 Martin Renvoize 2021-04-23 10:51:33 UTC
Nope I'm stuck again.. with bug 25508 applied, I can't see how this patch has an effect any more at all...

Assuming the constraints are set as Koha expects (Which I've now verified with Kyle) we shouldn't be able to get into this state any more.
Comment 17 Kyle M Hall 2021-05-13 13:38:03 UTC
(In reply to Martin Renvoize from comment #16)
> Nope I'm stuck again.. with bug 25508 applied, I can't see how this patch
> has an effect any more at all...
> 
> Assuming the constraints are set as Koha expects (Which I've now verified
> with Kyle) we shouldn't be able to get into this state any more.

I think you are correct, the affected partner will be upgraded to 20.05.11 asap. I'll let you know how it goes!
Comment 18 Victor Grousset/tuxayo 2021-05-13 21:17:47 UTC
So, do we conclude with won't fix?

IIUC
Someone stuck in Koha < 19.11.x (soon oldoldoldstable) and motivated enough could backport bug 23051 (just checked, quite hard but doesn't seem completely impossible)
and then backport bug 25508
Likely not worth the effort for ensuring the thing to not be risky.