From f51e6c720899e24652fabf48b1fef0645b77f668 Mon Sep 17 00:00:00 2001 From: Lari Taskula Date: Fri, 3 Feb 2017 14:54:42 +0200 Subject: [PATCH] [SIGNED-OFF] Bug 15165: Set amount parameter required, define its data type and confirm amount > 0 This patch enforces "amount" to be required when paying towards accountline(s). It also specifies the data type of amount, so that nonsense amounts like "strings" cannot go through. Also, defines minimum amount as 0 and exclusiveMinimum true so that the amount must be greater than 0 (and not equal). This patch also wraps the operations in try/catch as now recommended. Possible enhancements to Koha::Account->pay could also be done so that we would catch Koha::Exceptions in the case of invalid input. However, this is probably outside the scope of this bug and would require more testing on other components as well. After all, we can handle the input nicely with Swagger alone. To test: 1. prove t/db_dependent/api/v1/accountlines.t 2. prove t/db_dependent/api/v1/patrons.t Signed-off-by: Josef Moravec --- Koha/REST/V1/Accountline.pm | 57 ++++++++++++++++++++-------------- Koha/REST/V1/Patron.pm | 53 ++++++++++++++++++------------- api/v1/swagger/paths/accountlines.json | 21 +++++++++---- api/v1/swagger/paths/patrons.json | 11 +++++-- t/db_dependent/api/v1/accountlines.t | 25 +++++++++++++-- 5 files changed, 110 insertions(+), 57 deletions(-) diff --git a/Koha/REST/V1/Accountline.pm b/Koha/REST/V1/Accountline.pm index 57f83dd..f65a13b 100644 --- a/Koha/REST/V1/Accountline.pm +++ b/Koha/REST/V1/Accountline.pm @@ -25,6 +25,8 @@ use C4::Auth qw( haspermission ); use Koha::Account::Lines; use Koha::Account; +use Try::Tiny; + sub list { my ($c, $args, $cb) = @_; @@ -55,33 +57,40 @@ sub edit { sub pay { my ($c, $args, $cb) = @_; - my $accountline = Koha::Account::Lines->find($args->{accountlines_id}); - unless ($accountline) { - return $c->$cb({error => "Accountline not found"}, 404); - } - - my $body = $c->req->json; - my $amount = $body->{amount}; - my $note = $body->{note} || ''; - - if ($amount && !looks_like_number($amount)) { - return $c->$cb({error => "Invalid amount"}, 400); - } + return try { + my $accountline = Koha::Account::Lines->find($args->{accountlines_id}); + unless ($accountline) { + return $c->$cb({error => "Accountline not found"}, 404); + } - Koha::Account->new( - { - patron_id => $accountline->borrowernumber, + my $body = $c->req->json; + my $amount = $body->{amount}; + my $note = $body->{note} || ''; + + Koha::Account->new( + { + patron_id => $accountline->borrowernumber, + } + )->pay( + { + lines => [$accountline], + amount => $amount, + note => $note, + } + ); + + $accountline = Koha::Account::Lines->find($args->{accountlines_id}); + return $c->$cb($accountline->unblessed(), 200); + } catch { + if ($_->isa('DBIx::Class::Exception')) { + return $c->$cb({ error => $_->msg }, 500); } - )->pay( - { - lines => [$accountline], - amount => $amount, - note => $note, + else { + return $c->$cb({ + error => 'Something went wrong, check the logs.' + }, 500); } - ); - - $accountline = Koha::Account::Lines->find($args->{accountlines_id}); - return $c->$cb($accountline->unblessed(), 200); + }; } diff --git a/Koha/REST/V1/Patron.pm b/Koha/REST/V1/Patron.pm index 3fbb05f..f229d98 100644 --- a/Koha/REST/V1/Patron.pm +++ b/Koha/REST/V1/Patron.pm @@ -25,6 +25,8 @@ use C4::Auth qw( haspermission ); use Koha::Patrons; use Koha::Account; +use Try::Tiny; + sub list { my ($c, $args, $cb) = @_; @@ -51,31 +53,38 @@ sub get { sub pay { my ($c, $args, $cb) = @_; - my $patron = Koha::Patrons->find($args->{borrowernumber}); - unless ($patron) { - return $c->$cb({error => "Patron not found"}, 404); - } - - my $body = $c->req->json; - my $amount = $body->{amount}; - my $note = $body->{note} || ''; - - unless ($amount && looks_like_number($amount)) { - return $c->$cb({error => "Invalid amount"}, 400); - } + return try { + my $patron = Koha::Patrons->find($args->{borrowernumber}); + unless ($patron) { + return $c->$cb({error => "Patron not found"}, 404); + } - Koha::Account->new( - { - patron_id => $args->{borrowernumber}, + my $body = $c->req->json; + my $amount = $body->{amount}; + my $note = $body->{note} || ''; + + Koha::Account->new( + { + patron_id => $args->{borrowernumber}, + } + )->pay( + { + amount => $amount, + note => $note, + } + ); + + return $c->$cb('', 204); + } catch { + if ($_->isa('DBIx::Class::Exception')) { + return $c->$cb({ error => $_->msg }, 500); } - )->pay( - { - amount => $amount, - note => $note, + else { + return $c->$cb({ + error => 'Something went wrong, check the logs.' + }, 500); } - ); - - return $c->$cb('', 204); + }; } 1; diff --git a/api/v1/swagger/paths/accountlines.json b/api/v1/swagger/paths/accountlines.json index 1d262e0..f520c12 100644 --- a/api/v1/swagger/paths/accountlines.json +++ b/api/v1/swagger/paths/accountlines.json @@ -55,13 +55,16 @@ "type": "object", "properties": { "amount": { - "description": "Amount" + "description": "Amount", + "type": ["number", "null"] }, "amountoutstanding": { - "description": "Amount outstanding" + "description": "Amount outstanding", + "type": ["number", "null"] }, "note": { - "description": "Accountline note" + "description": "Accountline note", + "type": ["string", "null"] } } } @@ -108,17 +111,23 @@ { "name": "body", "in": "body", + "required": true, "description": "A JSON object containing fields to modify", "schema": { "type": "object", "properties": { "amount": { - "description": "Amount to pay" + "description": "Amount to pay. If not given, the whole accountline will be paid.", + "type": "number", + "exclusiveMinimum": true, + "minimum": 0 }, "note": { - "description": "Payment note" + "description": "Payment note", + "type": "string" } - } + }, + "required": ["amount"] } } ], diff --git a/api/v1/swagger/paths/patrons.json b/api/v1/swagger/paths/patrons.json index 5b3253a..695c016 100644 --- a/api/v1/swagger/paths/patrons.json +++ b/api/v1/swagger/paths/patrons.json @@ -88,12 +88,17 @@ "type": "object", "properties": { "amount": { - "description": "Amount to pay" + "description": "Amount to pay", + "type": "number", + "exclusiveMinimum": true, + "minimum": 0 }, "note": { - "description": "Payment note" + "description": "Payment note", + "type": ["string", "null"] } - } + }, + "required": ["amount"] } } ], diff --git a/t/db_dependent/api/v1/accountlines.t b/t/db_dependent/api/v1/accountlines.t index a91b160..79ef067 100644 --- a/t/db_dependent/api/v1/accountlines.t +++ b/t/db_dependent/api/v1/accountlines.t @@ -18,7 +18,7 @@ use Modern::Perl; -use Test::More tests => 37; +use Test::More tests => 43; use Test::Mojo; use t::lib::TestBuilder; @@ -160,7 +160,7 @@ is($accountline_edited->{amountoutstanding}, '-19.000000'); # Payment tests -$tx = $t->ua->build_tx(POST => "/api/v1/accountlines/4562765765/payment"); +$tx = $t->ua->build_tx(POST => "/api/v1/accountlines/4562765765/payment" => json => { amount => 1000 }); $tx->req->cookies({name => 'CGISESSID', value => $session->id}); $tx->req->env({REMOTE_ADDR => '127.0.0.1'}); $t->request_ok($tx) @@ -171,6 +171,27 @@ $tx = $t->ua->build_tx(POST => "/api/v1/accountlines/$accountline_to_pay->{accou $tx->req->cookies({name => 'CGISESSID', value => $session->id}); $tx->req->env({REMOTE_ADDR => '127.0.0.1'}); $t->request_ok($tx) + ->status_is(400); + +$tx = $t->ua->build_tx(POST => "/api/v1/accountlines/$accountline_to_pay->{accountlines_id}/payment" + => json => { amount => 0 }); +$tx->req->cookies({name => 'CGISESSID', value => $session->id}); +$tx->req->env({REMOTE_ADDR => '127.0.0.1'}); +$t->request_ok($tx) + ->status_is(400); + +$tx = $t->ua->build_tx(POST => "/api/v1/accountlines/$accountline_to_pay->{accountlines_id}/payment" + => json => { amount => "no" }); +$tx->req->cookies({name => 'CGISESSID', value => $session->id}); +$tx->req->env({REMOTE_ADDR => '127.0.0.1'}); +$t->request_ok($tx) + ->status_is(400); + +$tx = $t->ua->build_tx(POST => "/api/v1/accountlines/$accountline_to_pay->{accountlines_id}/payment" + => json => { amount => 20 }); +$tx->req->cookies({name => 'CGISESSID', value => $session->id}); +$tx->req->env({REMOTE_ADDR => '127.0.0.1'}); +$t->request_ok($tx) ->status_is(200); #$t->content_is('toto'); -- 2.1.4