Bug 27916

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

Hi Kyle, I think bug 25508 would also resolve this issue without needing to touch SIP..  could you perhaps varify that for me?

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>

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

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 ;)

Any chance of a unit test here Kyle?

I hope also that we could get unit test for this, i wasn't able to trigger the error manually.

(In reply to Martin Renvoize from comment #6)
> Any chance of a unit test here Kyle?

I'll see what I can do!

Created attachment 119718 [details] [review]
Bug 27916: Add unit tests

(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

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?

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.

QAing

    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

(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.

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.

(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!

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.