From a0a4082376646bc3b988d85e270d8cab68a5e658 Mon Sep 17 00:00:00 2001 From: Lari Taskula Date: Fri, 17 Jun 2016 13:10:18 +0300 Subject: [PATCH] Bug 14868: Use x-koha-permission in current routes Adds missing permission checking to holds. Adds 6 tests to patrons.t, adds 30 tests to holds.t. --- Koha/REST/V1/Patron.pm | 10 --- api/v1/swagger/paths/holds.json | 22 +++++ api/v1/swagger/paths/patrons.json | 11 +++ t/db_dependent/api/v1/holds.t | 177 +++++++++++++++++++++++++++++++++----- t/db_dependent/api/v1/patrons.t | 32 ++++++- 5 files changed, 216 insertions(+), 36 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 770565a..1371239 100644 --- a/api/v1/swagger/paths/holds.json +++ b/api/v1/swagger/paths/holds.json @@ -21,6 +21,12 @@ "$ref": "../definitions.json#/error" } } + }, + "x-koha-permission": { + "allow-owner": "1", + "permissions": { + "borrowers": "1" + } } }, "post": { @@ -92,6 +98,12 @@ "$ref": "../definitions.json#/error" } } + }, + "x-koha-permission": { + "allow-owner": "1", + "permissions": { + "reserveforothers": "1" + } } } }, @@ -148,6 +160,11 @@ "$ref": "../definitions.json#/error" } } + }, + "x-koha-permission": { + "permissions": { + "reserveforothers": "1" + } } }, "delete": { @@ -171,6 +188,11 @@ "$ref": "../definitions.json#/error" } } + }, + "x-koha-permission": { + "permissions": { + "reserveforothers": "1" + } } } } diff --git a/api/v1/swagger/paths/patrons.json b/api/v1/swagger/paths/patrons.json index f7c6400..3db7fe6 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-permission": { + "permissions": { + "borrowers": "1" + } } } }, @@ -55,6 +60,12 @@ "$ref": "../definitions.json#/error" } } + }, + "x-koha-permission": { + "allow-owner": "1", + "permissions": { + "borrowers": "1" + } } } } diff --git a/t/db_dependent/api/v1/holds.t b/t/db_dependent/api/v1/holds.t index 0ce67d5..11a54a3 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 => 30; +use Test::More tests => 60; use Test::Mojo; +use t::lib::TestBuilder; use DateTime; @@ -30,29 +31,86 @@ 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 $categorycode = Koha::Database->new()->schema()->resultset('Category')->first()->categorycode(); my $branchcode = Koha::Database->new()->schema()->resultset('Branch')->first()->branchcode(); +# Create test users + +# 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; + +# User with borrowers and reserveforothers my $borrower = Koha::Patron->new; $borrower->categorycode( $categorycode ); $borrower->branchcode( $branchcode ); $borrower->surname("Test Surname"); +$borrower->flags(80); # borrowers and reserveforothers flag +$borrower->userid($nopermission->{ userid }."z"); $borrower->store; my $borrowernumber = $borrower->borrowernumber; +# User with borrowers 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; +# User with reserveforothers +my $borrower3 = $builder->build({ + source => 'Borrower', + value => { + branchcode => $branchcode, + categorycode => $categorycode, + flags => 64 # reserveforothers flag + } +}); + +# 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'); @@ -60,38 +118,111 @@ 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); my $suspend_until = DateTime->now->add(days => 10)->ymd; +my $expirationdate = DateTime->now->add(days => 10)->ymd; + +my $post_data = { + borrowernumber => int($borrowernumber), + biblionumber => int($biblionumber), + itemnumber => int($itemnumber), + branchcode => $branchcode, + expirationdate => $expirationdate, +}; my $put_data = { priority => 2, suspend_until => $suspend_until, }; -$t->put_ok("/api/v1/holds/$reserve_id" => json => $put_data) + +# Test endpoints without authentication, expecting 401 Unauthorized +$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); + +# Test endpoints with authentication, but without permissions, expecting 403 Forbidden +my $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); + +# Test endpoints with no permission, but accessing own object, expecting success +my $borrno_tmp = $post_data->{'borrowernumber'}; +$post_data->{'borrowernumber'} = $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); + +# Test endpoints with permission, expecting success +$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); -$t->delete_ok("/api/v1/holds/$reserve_id") +$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); -$t->put_ok("/api/v1/holds/$reserve_id" => json => $put_data) +$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'); -$t->delete_ok("/api/v1/holds/$reserve_id") +$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'); - -$t->get_ok("/api/v1/holds?borrowernumber=$borrowernumber") +$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 + 1; -$t->get_ok("/api/v1/holds?borrowernumber=$inexisting_borrowernumber") +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(404) ->json_has('/error'); @@ -101,27 +232,29 @@ $dbh->do(q{ VALUES (?, ?, ?, ?) }, {}, '*', '*', '*', 1); -my $expirationdate = DateTime->now->add(days => 10)->ymd; -my $post_data = { - borrowernumber => int($borrowernumber), - biblionumber => int($biblionumber), - itemnumber => int($itemnumber), - branchcode => $branchcode, - expirationdate => $expirationdate, -}; -$t->post_ok("/api/v1/holds" => json => $post_data) +$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}; -$t->get_ok("/api/v1/holds?borrowernumber=$borrowernumber") +$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); -$t->post_ok("/api/v1/holds" => json => $post_data) +$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/); diff --git a/t/db_dependent/api/v1/patrons.t b/t/db_dependent/api/v1/patrons.t index ded6f1f..a21c0b1 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 => 16; use Test::Mojo; use t::lib::TestBuilder; @@ -42,7 +42,8 @@ my $borrower = $builder->build({ source => 'Borrower', value => { branchcode => $branchcode, - categorycode => $categorycode + categorycode => $categorycode, + flags => 0 } }); @@ -52,6 +53,29 @@ $t->get_ok('/api/v1/patrons') $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 $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); + my $loggedinuser = $builder->build({ source => 'Borrower', value => { @@ -61,14 +85,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) -- 1.9.1