From b09091d7f1459d87d7ddfb5acf47656782400d4d Mon Sep 17 00:00:00 2001 From: Lari Taskula Date: Thu, 11 Aug 2016 14:45:16 +0300 Subject: [PATCH] Bug 14868: Use x-koha-authorization in current routes To test: 1. Run t/db_dependent/api/v1/holds.t 2. Run t/db_dependent/api/v1/patrons.t Signed-off-by: Benjamin Rokseth Signed-off-by: Tomas Cohen Arazi --- Koha/REST/V1/Patron.pm | 10 -- api/v1/swagger/paths/holds.json | 25 ++++ api/v1/swagger/paths/patrons.json | 12 ++ t/db_dependent/api/v1/holds.t | 280 +++++++++++++++++++++++++++++--------- t/db_dependent/api/v1/patrons.t | 57 +++++++- 5 files changed, 307 insertions(+), 77 deletions(-) diff --git a/Koha/REST/V1/Patron.pm b/Koha/REST/V1/Patron.pm index 2851308..a66dbb9 100644 --- a/Koha/REST/V1/Patron.pm +++ b/Koha/REST/V1/Patron.pm @@ -26,9 +26,6 @@ sub list { my ($c, $args, $cb) = @_; my $user = $c->stash('koha.user'); - unless ($user && haspermission($user->userid, {borrowers => 1})) { - return $c->$cb({error => "You don't have the required permission"}, 403); - } my $patrons = Koha::Patrons->search; @@ -40,13 +37,6 @@ sub get { my $user = $c->stash('koha.user'); - unless ( $user - && ( $user->borrowernumber == $args->{borrowernumber} - || haspermission($user->userid, {borrowers => 1}) ) ) - { - return $c->$cb({error => "You don't have the required permission"}, 403); - } - my $patron = Koha::Patrons->find($args->{borrowernumber}); unless ($patron) { return $c->$cb({error => "Patron not found"}, 404); diff --git a/api/v1/swagger/paths/holds.json b/api/v1/swagger/paths/holds.json index cd9713b..9e7ec34 100644 --- a/api/v1/swagger/paths/holds.json +++ b/api/v1/swagger/paths/holds.json @@ -124,6 +124,13 @@ "$ref": "../definitions.json#/error" } } + }, + "x-koha-authorization": { + "allow-owner": true, + "allow-guarantor": true, + "permissions": { + "borrowers": "1" + } } }, "post": { @@ -195,6 +202,12 @@ "$ref": "../definitions.json#/error" } } + }, + "x-koha-authorization": { + "allow-owner": true, + "permissions": { + "reserveforothers": "1" + } } } }, @@ -251,6 +264,13 @@ "$ref": "../definitions.json#/error" } } + }, + "x-koha-authorization": { + "allow-owner": true, + "allow-guarantor": true, + "permissions": { + "reserveforothers": "1" + } } }, "delete": { @@ -274,6 +294,11 @@ "$ref": "../definitions.json#/error" } } + }, + "x-koha-authorization": { + "permissions": { + "reserveforothers": "1" + } } } } diff --git a/api/v1/swagger/paths/patrons.json b/api/v1/swagger/paths/patrons.json index f7c6400..67632e1 100644 --- a/api/v1/swagger/paths/patrons.json +++ b/api/v1/swagger/paths/patrons.json @@ -22,6 +22,11 @@ "$ref": "../definitions.json#/error" } } + }, + "x-koha-authorization": { + "permissions": { + "borrowers": "1" + } } } }, @@ -55,6 +60,13 @@ "$ref": "../definitions.json#/error" } } + }, + "x-koha-authorization": { + "allow-owner": true, + "allow-guarantor": true, + "permissions": { + "borrowers": "1" + } } } } diff --git a/t/db_dependent/api/v1/holds.t b/t/db_dependent/api/v1/holds.t index 52177bf..8a5f393 100644 --- a/t/db_dependent/api/v1/holds.t +++ b/t/db_dependent/api/v1/holds.t @@ -17,8 +17,9 @@ use Modern::Perl; -use Test::More tests => 39; +use Test::More tests => 4; use Test::Mojo; +use t::lib::TestBuilder; use DateTime; @@ -30,19 +31,41 @@ use C4::Reserves; use Koha::Database; use Koha::Patron; +my $builder = t::lib::TestBuilder->new(); + my $dbh = C4::Context->dbh; $dbh->{AutoCommit} = 0; $dbh->{RaiseError} = 1; +$ENV{REMOTE_ADDR} = '127.0.0.1'; my $t = Test::Mojo->new('Koha::REST::V1'); +my $tx; my $categorycode = Koha::Database->new()->schema()->resultset('Category')->first()->categorycode(); my $branchcode = Koha::Database->new()->schema()->resultset('Branch')->first()->branchcode(); +# User without any permissions +my $nopermission = $builder->build({ + source => 'Borrower', + value => { + branchcode => $branchcode, + categorycode => $categorycode, + flags => 0 + } +}); +my $session_nopermission = C4::Auth::get_session(''); +$session_nopermission->param('number', $nopermission->{ borrowernumber }); +$session_nopermission->param('id', $nopermission->{ userid }); +$session_nopermission->param('ip', '127.0.0.1'); +$session_nopermission->param('lasttime', time()); +$session_nopermission->flush; + my $borrower = Koha::Patron->new; $borrower->categorycode( $categorycode ); $borrower->branchcode( $branchcode ); $borrower->surname("Test Surname"); +$borrower->flags(80); #borrowers and reserveforothers flags +$borrower->userid($nopermission->{ userid }."z"); $borrower->store; my $borrowernumber = $borrower->borrowernumber; @@ -50,9 +73,40 @@ my $borrower2 = Koha::Patron->new; $borrower2->categorycode( $categorycode ); $borrower2->branchcode( $branchcode ); $borrower2->surname("Test Surname 2"); +$borrower2->userid($nopermission->{ userid }."x"); +$borrower2->flags(16); # borrowers flag $borrower2->store; my $borrowernumber2 = $borrower2->borrowernumber; +my $borrower3 = Koha::Patron->new; +$borrower3->categorycode( $categorycode ); +$borrower3->branchcode( $branchcode ); +$borrower3->surname("Test Surname 2"); +$borrower3->userid($nopermission->{ userid }."y"); +$borrower3->flags(64); # reserveforothers flag +$borrower3->store; +my $borrowernumber3 = $borrower3->borrowernumber; + +# Get sessions +my $session = C4::Auth::get_session(''); +$session->param('number', $borrower->borrowernumber); +$session->param('id', $borrower->userid); +$session->param('ip', '127.0.0.1'); +$session->param('lasttime', time()); +$session->flush; +my $session2 = C4::Auth::get_session(''); +$session2->param('number', $borrower2->borrowernumber); +$session2->param('id', $borrower2->userid); +$session2->param('ip', '127.0.0.1'); +$session2->param('lasttime', time()); +$session2->flush; +my $session3 = C4::Auth::get_session(''); +$session3->param('number', $borrower3->borrowernumber); +$session3->param('id', $borrower3->userid); +$session3->param('ip', '127.0.0.1'); +$session3->param('lasttime', time()); +$session3->flush; + my $biblionumber = create_biblio('RESTful Web APIs'); my $itemnumber = create_item($biblionumber, 'TEST000001'); @@ -62,59 +116,12 @@ my $reserve_id = C4::Reserves::AddReserve($branchcode, $borrowernumber, $biblionumber, undef, 1, undef, undef, undef, '', $itemnumber); # Add another reserve to be able to change first reserve's rank -C4::Reserves::AddReserve($branchcode, $borrowernumber2, +my $reserve_id2 = C4::Reserves::AddReserve($branchcode, $borrowernumber2, $biblionumber, undef, 2, undef, undef, undef, '', $itemnumber); -$t->get_ok('/api/v1/holds') - ->status_is(200) - ->json_has('/0') - ->json_has('/1') - ->json_hasnt('/2'); - -$t->get_ok('/api/v1/holds?priority=2') - ->status_is(200) - ->json_is('/0/borrowernumber', $borrowernumber2) - ->json_hasnt('/1'); - my $suspend_until = DateTime->now->add(days => 10)->ymd; -my $put_data = { - priority => 2, - suspend_until => $suspend_until, -}; -$t->put_ok("/api/v1/holds/$reserve_id" => json => $put_data) - ->status_is(200) - ->json_is('/reserve_id', $reserve_id) - ->json_is('/suspend_until', $suspend_until . ' 00:00:00') - ->json_is('/priority', 2); - -$t->delete_ok("/api/v1/holds/$reserve_id") - ->status_is(200); - -$t->put_ok("/api/v1/holds/$reserve_id" => json => $put_data) - ->status_is(404) - ->json_has('/error'); - -$t->delete_ok("/api/v1/holds/$reserve_id") - ->status_is(404) - ->json_has('/error'); - - -$t->get_ok("/api/v1/holds?borrowernumber=$borrowernumber") - ->status_is(200) - ->json_is([]); - -my $inexisting_borrowernumber = $borrowernumber2 + 1; -$t->get_ok("/api/v1/holds?borrowernumber=$inexisting_borrowernumber") - ->status_is(200) - ->json_is([]); - -$dbh->do('DELETE FROM issuingrules'); -$dbh->do(q{ - INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed) - VALUES (?, ?, ?, ?) -}, {}, '*', '*', '*', 1); - my $expirationdate = DateTime->now->add(days => 10)->ymd; + my $post_data = { borrowernumber => int($borrowernumber), biblionumber => int($biblionumber), @@ -122,21 +129,169 @@ my $post_data = { branchcode => $branchcode, expirationdate => $expirationdate, }; -$t->post_ok("/api/v1/holds" => json => $post_data) - ->status_is(201) - ->json_has('/reserve_id'); +my $put_data = { + priority => 2, + suspend_until => $suspend_until, +}; + +subtest "Test endpoints without authentication" => sub { + plan tests => 8; + $t->get_ok('/api/v1/holds') + ->status_is(401); + $t->post_ok('/api/v1/holds') + ->status_is(401); + $t->put_ok('/api/v1/holds/0') + ->status_is(401); + $t->delete_ok('/api/v1/holds/0') + ->status_is(401); +}; -$reserve_id = $t->tx->res->json->{reserve_id}; -$t->get_ok("/api/v1/holds?borrowernumber=$borrowernumber") - ->status_is(200) - ->json_is('/0/reserve_id', $reserve_id) - ->json_is('/0/expirationdate', $expirationdate) - ->json_is('/0/branchcode', $branchcode); +subtest "Test endpoints without permission" => sub { + plan tests => 10; + + $tx = $t->ua->build_tx(GET => "/api/v1/holds?borrowernumber=$borrowernumber"); + $tx->req->cookies({name => 'CGISESSID', value => $session_nopermission->id}); + $t->request_ok($tx) # no permission + ->status_is(403); + $tx = $t->ua->build_tx(GET => "/api/v1/holds?borrowernumber=$borrowernumber"); + $tx->req->cookies({name => 'CGISESSID', value => $session3->id}); + $t->request_ok($tx) # reserveforothers permission + ->status_is(403); + $tx = $t->ua->build_tx(POST => "/api/v1/holds" => json => $post_data ); + $tx->req->cookies({name => 'CGISESSID', value => $session_nopermission->id}); + $t->request_ok($tx) # no permission + ->status_is(403); + $tx = $t->ua->build_tx(PUT => "/api/v1/holds/0" => json => $put_data ); + $tx->req->cookies({name => 'CGISESSID', value => $session_nopermission->id}); + $t->request_ok($tx) # no permission + ->status_is(403); + $tx = $t->ua->build_tx(DELETE => "/api/v1/holds/0"); + $tx->req->cookies({name => 'CGISESSID', value => $session_nopermission->id}); + $t->request_ok($tx) # no permission + ->status_is(403); +}; +subtest "Test endpoints without permission, but accessing own object" => sub { + plan tests => 15; + + my $borrno_tmp = $post_data->{'borrowernumber'}; + $post_data->{'borrowernumber'} = int $nopermission->{'borrowernumber'}; + $tx = $t->ua->build_tx(POST => "/api/v1/holds" => json => $post_data); + $tx->req->cookies({name => 'CGISESSID', value => $session_nopermission->id}); + $t->request_ok($tx) # create hold to myself + ->status_is(201) + ->json_has('/reserve_id'); + + $post_data->{'borrowernumber'} = $borrno_tmp; + $tx = $t->ua->build_tx(GET => "/api/v1/holds?borrowernumber=".$nopermission-> { borrowernumber }); + $tx->req->cookies({name => 'CGISESSID', value => $session_nopermission->id}); + $t->request_ok($tx) # get my own holds + ->status_is(200) + ->json_is('/0/borrowernumber', $nopermission->{ borrowernumber }) + ->json_is('/0/biblionumber', $biblionumber) + ->json_is('/0/itemnumber', $itemnumber) + ->json_is('/0/expirationdate', $expirationdate) + ->json_is('/0/branchcode', $branchcode); + + my $reserve_id3 = Koha::Holds->find({ borrowernumber => $nopermission->{borrowernumber} })->reserve_id; + $tx = $t->ua->build_tx(PUT => "/api/v1/holds/$reserve_id3" => json => $put_data); + $tx->req->cookies({name => 'CGISESSID', value => $session_nopermission->id}); + $t->request_ok($tx) # create hold to myself + ->status_is(200) + ->json_is('/reserve_id', $reserve_id3) + ->json_is('/suspend_until', $suspend_until . ' 00:00:00') + ->json_is('/priority', 2); +}; -$t->post_ok("/api/v1/holds" => json => $post_data) - ->status_is(403) - ->json_like('/error', qr/tooManyReserves/); +subtest "Test endpoints with permission" => sub { + plan tests => 42; + + $tx = $t->ua->build_tx(GET => '/api/v1/holds'); + $tx->req->cookies({name => 'CGISESSID', value => $session->id}); + $t->request_ok($tx) + ->status_is(200) + ->json_has('/0') + ->json_has('/1') + ->json_has('/2') + ->json_hasnt('/3'); + + $tx = $t->ua->build_tx(GET => '/api/v1/holds?priority=2'); + $tx->req->cookies({name => 'CGISESSID', value => $session->id}); + $t->request_ok($tx) + ->status_is(200) + ->json_is('/0/borrowernumber', $nopermission->{borrowernumber}) + ->json_hasnt('/1'); + + $tx = $t->ua->build_tx(PUT => "/api/v1/holds/$reserve_id" => json => $put_data); + $tx->req->cookies({name => 'CGISESSID', value => $session3->id}); + $t->request_ok($tx) + ->status_is(200) + ->json_is('/reserve_id', $reserve_id) + ->json_is('/suspend_until', $suspend_until . ' 00:00:00') + ->json_is('/priority', 2); + + $tx = $t->ua->build_tx(DELETE => "/api/v1/holds/$reserve_id"); + $tx->req->cookies({name => 'CGISESSID', value => $session3->id}); + $t->request_ok($tx) + ->status_is(200); + + $tx = $t->ua->build_tx(PUT => "/api/v1/holds/$reserve_id" => json => $put_data); + $tx->req->cookies({name => 'CGISESSID', value => $session3->id}); + $t->request_ok($tx) + ->status_is(404) + ->json_has('/error'); + + $tx = $t->ua->build_tx(DELETE => "/api/v1/holds/$reserve_id"); + $tx->req->cookies({name => 'CGISESSID', value => $session3->id}); + $t->request_ok($tx) + ->status_is(404) + ->json_has('/error'); + + $tx = $t->ua->build_tx(GET => "/api/v1/holds?borrowernumber=".$borrower->borrowernumber); + $tx->req->cookies({name => 'CGISESSID', value => $session2->id}); # get with borrowers flag + $t->request_ok($tx) + ->status_is(200) + ->json_is([]); + + my $inexisting_borrowernumber = $borrowernumber2*2; + $tx = $t->ua->build_tx(GET => "/api/v1/holds?borrowernumber=$inexisting_borrowernumber"); + $tx->req->cookies({name => 'CGISESSID', value => $session->id}); + $t->request_ok($tx) + ->status_is(200) + ->json_is([]); + + $dbh->do('DELETE FROM issuingrules'); + $dbh->do(q{ + INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed) + VALUES (?, ?, ?, ?) + }, {}, '*', '*', '*', 1); + + $tx = $t->ua->build_tx(DELETE => "/api/v1/holds/$reserve_id2"); + $tx->req->cookies({name => 'CGISESSID', value => $session3->id}); + $t->request_ok($tx) + ->status_is(200); + + $tx = $t->ua->build_tx(POST => "/api/v1/holds" => json => $post_data); + $tx->req->cookies({name => 'CGISESSID', value => $session3->id}); + $t->request_ok($tx) + ->status_is(201) + ->json_has('/reserve_id'); + $reserve_id = $t->tx->res->json->{reserve_id}; + + $tx = $t->ua->build_tx(GET => "/api/v1/holds?borrowernumber=$borrowernumber"); + $tx->req->cookies({name => 'CGISESSID', value => $session->id}); + $t->request_ok($tx) + ->status_is(200) + ->json_is('/0/reserve_id', $reserve_id) + ->json_is('/0/expirationdate', $expirationdate) + ->json_is('/0/branchcode', $branchcode); + + $tx = $t->ua->build_tx(POST => "/api/v1/holds" => json => $post_data); + $tx->req->cookies({name => 'CGISESSID', value => $session3->id}); + $t->request_ok($tx) + ->status_is(403) + ->json_like('/error', qr/tooManyReserves/); +}; $dbh->rollback; @@ -160,6 +315,7 @@ sub create_item { my $item = { barcode => $barcode, }; + $dbh->do("DELETE FROM items WHERE barcode='$barcode'") if $barcode; my $itemnumber = C4::Items::AddItem($item, $biblionumber); diff --git a/t/db_dependent/api/v1/patrons.t b/t/db_dependent/api/v1/patrons.t index 6bd4e1a..6286234 100644 --- a/t/db_dependent/api/v1/patrons.t +++ b/t/db_dependent/api/v1/patrons.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 10; +use Test::More tests => 19; use Test::Mojo; use t::lib::TestBuilder; @@ -38,20 +38,67 @@ my $t = Test::Mojo->new('Koha::REST::V1'); my $categorycode = $builder->build({ source => 'Category' })->{ categorycode }; my $branchcode = $builder->build({ source => 'Branch' })->{ branchcode }; +my $guarantor = $builder->build({ + source => 'Borrower', + value => { + branchcode => $branchcode, + categorycode => $categorycode, + flags => 0, + } +}); my $borrower = $builder->build({ source => 'Borrower', value => { branchcode => $branchcode, - categorycode => $categorycode + categorycode => $categorycode, + flags => 0, + guarantorid => $guarantor->{borrowernumber}, } }); $t->get_ok('/api/v1/patrons') - ->status_is(403); + ->status_is(401); $t->get_ok("/api/v1/patrons/" . $borrower->{ borrowernumber }) + ->status_is(401); + +my $session = C4::Auth::get_session(''); +$session->param('number', $borrower->{ borrowernumber }); +$session->param('id', $borrower->{ userid }); +$session->param('ip', '127.0.0.1'); +$session->param('lasttime', time()); +$session->flush; + +my $session2 = C4::Auth::get_session(''); +$session2->param('number', $guarantor->{ borrowernumber }); +$session2->param('id', $guarantor->{ userid }); +$session2->param('ip', '127.0.0.1'); +$session2->param('lasttime', time()); +$session2->flush; + +my $tx = $t->ua->build_tx(GET => '/api/v1/patrons'); +$tx->req->cookies({name => 'CGISESSID', value => $session->id}); +$t->request_ok($tx) + ->status_is(403); + +$tx = $t->ua->build_tx(GET => "/api/v1/patrons/" . ($borrower->{ borrowernumber }-1)); +$tx->req->cookies({name => 'CGISESSID', value => $session->id}); +$t->request_ok($tx) ->status_is(403); +# User without permissions, but is the owner of the object +$tx = $t->ua->build_tx(GET => "/api/v1/patrons/" . $borrower->{borrowernumber}); +$tx->req->cookies({name => 'CGISESSID', value => $session->id}); +$t->request_ok($tx) + ->status_is(200); + +# User without permissions, but is the guarantor of the owner of the object +$tx = $t->ua->build_tx(GET => "/api/v1/patrons/" . $borrower->{borrowernumber}); +$tx->req->cookies({name => 'CGISESSID', value => $session2->id}); +$t->request_ok($tx) + ->status_is(200) + ->json_is('/guarantorid', $guarantor->{borrowernumber}); + my $loggedinuser = $builder->build({ source => 'Borrower', value => { @@ -61,14 +108,14 @@ my $loggedinuser = $builder->build({ } }); -my $session = C4::Auth::get_session(''); +$session = C4::Auth::get_session(''); $session->param('number', $loggedinuser->{ borrowernumber }); $session->param('id', $loggedinuser->{ userid }); $session->param('ip', '127.0.0.1'); $session->param('lasttime', time()); $session->flush; -my $tx = $t->ua->build_tx(GET => '/api/v1/patrons'); +$tx = $t->ua->build_tx(GET => '/api/v1/patrons'); $tx->req->cookies({name => 'CGISESSID', value => $session->id}); $tx->req->env({REMOTE_ADDR => '127.0.0.1'}); $t->request_ok($tx) -- 2.9.3