Bugzilla – Attachment 52594 Details for
Bug 14868
REST API: Swagger2-driven permission checking
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 14868: Use x-koha-permission in current routes
Bug-14868-Use-x-koha-permission-in-current-routes.patch (text/plain), 15.22 KB, created by
Lari Taskula
on 2016-06-20 14:04:18 UTC
(
hide
)
Description:
Bug 14868: Use x-koha-permission in current routes
Filename:
MIME Type:
Creator:
Lari Taskula
Created:
2016-06-20 14:04:18 UTC
Size:
15.22 KB
patch
obsolete
>From 82df618b0dc94b1d5c1bee3f4de1f43c42174bc4 Mon Sep 17 00:00:00 2001 >From: Lari Taskula <larit@student.uef.fi> >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
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 14868
:
42761
|
52577
|
52578
|
52579
|
52592
|
52593
|
52594
|
52602
|
52603
|
52604
|
52963
|
52964
|
52965
|
52966
|
52967
|
52981
|
52992
|
52993
|
52994
|
52995
|
52996
|
54457
|
54458
|
54459
|
54631
|
54632
|
54633
|
54765
|
54766
|
54767
|
54770
|
54771
|
54894
|
54895
|
54896
|
54897
|
55084
|
55085
|
55086
|
55087
|
55088
|
55089
|
55099
|
55100
|
55101
|
55102
|
55103
|
55104
|
55105