Bug 26506 - Koha::Account::pay will fail if $userenv is not set
Summary: Koha::Account::pay will fail if $userenv is not set
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Matthias Meusburger
QA Contact: Martin Renvoize
URL:
Keywords:
: 26332 (view as bug list)
Depends on: 22008
Blocks: 14825
  Show dependency treegraph
 
Reported: 2020-09-22 13:14 UTC by Matthias Meusburger
Modified: 2020-11-17 03:34 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.06


Attachments
Bug 26506: Fix Koha::Account::pay if called when no user is logged-in (1.09 KB, patch)
2020-09-22 13:19 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 26506: Fix Koha::Account::pay if called when no user is logged-in (1.32 KB, patch)
2020-09-23 08:39 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 26506: Fix Koha::Account::pay if called when no user is logged-in (1.38 KB, patch)
2020-10-01 13:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26506: (QA follow-up) Add Unit Test (1.39 KB, patch)
2020-10-01 13:35 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Matthias Meusburger 2020-09-22 13:14:31 UTC
Koha::Account::pay will fail if $userenv is not set.

The context would be a payment plugin for the opac, in which the payment system will call a callback URL to confirm the payment has been made. In this situation, no user is logged in when the plugin uses Koha::Account::pay.

The problem is that the following line in Koha::Account::pay:

my $manager_id = $userenv ? $userenv->{number} : 0;

will result in an SQL error, since $userenv->{number} is not set, and 0 is not an acceptable value because of foreign key constraint:

DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_hkr`.`accountlines`, CONSTRAINT `accountlines_ibfk_borrowers_2` FOREIGN KEY (`manager_id`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE SET NULL ON UPDATE CASCADE)

By the way, if you have a look at C4/Accounts.pm, in the manualinvoice function:

my $manager_id = C4::Context->userenv ? C4::Context->userenv->{'number'} : undef;

$manager_id will be set to undef, which will not fail, since undef is an acceptable value regarding to SQL foreign key constraints.

It's hard to provide a test plan, since it relies on a plugin, sorry, but I will try to answer to any question :)
Comment 1 Matthias Meusburger 2020-09-22 13:19:40 UTC Comment hidden (obsolete)
Comment 2 Magnus Enger 2020-09-23 08:39:08 UTC
Created attachment 110581 [details] [review]
Bug 26506: Fix Koha::Account::pay if called when no user is logged-in

Currently, Koha::Account::pay will fail if no user is logged-in when it is
called.

In this case, this patch will use undef instead of 0 for $manager_id in order
to comply with the foreign key constraint on manager_id.

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Biblibre and Libriotech have created a OPAC payment plugin that was
having problems because of this bug. Applying the fix in this patch
to a live server solved the problem.
Comment 3 Magnus Enger 2020-09-23 08:40:46 UTC
I have signed off the patch because I know it solves a problem. However, I have been involved in the work that Biblibre has done to uncover and fix the bug, so if my signoff is considered to be "not independent enough" please set the status back to NSO.
Comment 4 Martin Renvoize 2020-09-27 06:38:00 UTC
Exactly the same patch as attached to bug 26332.

As this one is signed off, I think I can safely QA this version even though I wrote the duplicate.
Comment 5 Martin Renvoize 2020-09-27 06:47:22 UTC
Whilst I agree with this patch and will PQA it once back at a computer, I do feel it perhaps highlight some issues with the plugin code in question (as I highlighted on my version of the bug and it prompted me to fix my own payments plugin).

Lots of Koha functions still exist that expect a userenv to always be present so in general I feel developers should always attempt to set at least the minimum of branch code and interface in a skeleton userenv if they can't catch an existing environment.

Just some food for though Matthias 😉.  For an example of this see my commit on one of the payment plugins we maintain: https://github.com/PTFS-Europe/koha-plugin-wpm-online_payments/commit/0a0ee1ae7dd2477d1b55d40f974a2c0dcf4e621a
Comment 6 Jonathan Druart 2020-09-28 12:51:44 UTC
Don't forget to write a test.
Comment 7 Martin Renvoize 2020-09-28 19:47:29 UTC
*** Bug 26332 has been marked as a duplicate of this bug. ***
Comment 8 Martin Renvoize 2020-09-28 19:48:36 UTC
Good catch, it needs a unit test.
Comment 9 Martin Renvoize 2020-10-01 13:34:58 UTC
Created attachment 111045 [details] [review]
Bug 26506: Fix Koha::Account::pay if called when no user is logged-in

Currently, Koha::Account::pay will fail if no user is logged-in when it is
called.

In this case, this patch will use undef instead of 0 for $manager_id in order
to comply with the foreign key constraint on manager_id.

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Biblibre and Libriotech have created a OPAC payment plugin that was
having problems because of this bug. Applying the fix in this patch
to a live server solved the problem.
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2020-10-01 13:35:06 UTC
Created attachment 111046 [details] [review]
Bug 26506: (QA follow-up) Add Unit Test

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Jonathan Druart 2020-10-08 14:21:03 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 12 Lucas Gass 2020-11-09 20:33:39 UTC
backported to 20.05.x for 20.05.06
Comment 13 Aleisha Amohia 2020-11-17 03:34:48 UTC
tests fail on 19.11.x. not backported.

$ prove t/db_dependent/Koha/Account.t 
t/db_dependent/Koha/Account.t .. 7/11     # Looks like you planned 5 tests but ran 4.

#   Failed test 'pay() tests'
#   at t/db_dependent/Koha/Account.t line 724.
Can't use string ("71") as a HASH ref while "strict refs" in use at t/db_dependent/Koha/Account.t line 720.
# Looks like your test exited with 255 just after 8.
t/db_dependent/Koha/Account.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 4/11 subtests 

Test Summary Report
-------------------
t/db_dependent/Koha/Account.t (Wstat: 65280 Tests: 8 Failed: 1)
  Failed test:  8
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 11 tests but ran 8.
Files=1, Tests=8,  3 wallclock secs ( 0.02 usr  0.01 sys +  2.42 cusr  0.84 csys =  3.29 CPU)
Result: FAIL

if needed, please fix.

There also doesn't seem to be a test plan here so I'm not sure how to test this.