From 9c4a1b19aa86b246ea2528a6f1a33401d1007fae Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Fri, 17 Sep 2021 15:39:17 -0300 Subject: [PATCH] Bug 27947: (QA follow-up) Clarify permissions Privileged routes need permissions defined. This patch adds the minimum required permission until there are article request-specific permissions in Koha: circulate: circulate_remaining_permissions It is also clarified that interacting with an article request from another patron, but having your own patron_id in the path would return 404 instead of 403, as technically the resource (an article request from the patron, identified.by the supplied id) doesn't exist. Tests are tweaked. Signed-off-by: Tomas Cohen Arazi Signed-off-by: Jonathan Druart --- Koha/REST/V1/ArticleRequests.pm | 6 +++-- api/v1/swagger/paths/article_requests.yaml | 3 +++ t/db_dependent/api/v1/article_requests.t | 27 +++++++++++++--------- 3 files changed, 23 insertions(+), 13 deletions(-) diff --git a/Koha/REST/V1/ArticleRequests.pm b/Koha/REST/V1/ArticleRequests.pm index 77c69a62d6..58e3f4bfef 100644 --- a/Koha/REST/V1/ArticleRequests.pm +++ b/Koha/REST/V1/ArticleRequests.pm @@ -66,7 +66,7 @@ sub cancel { }; } -=head3 patron_cancel +=head3 patron_cancel (public route) Controller function that handles cancelling a patron's Koha::ArticleRequest object @@ -84,6 +84,8 @@ sub patron_cancel { ); } + # patron_id has been validated by the allow-owner check, so the following call to related + # article requests covers the case of article requests not belonging to the patron my $article_request = $patron->article_requests->find( $c->validation->param('article_request_id') ); unless ( $article_request ) { @@ -109,4 +111,4 @@ sub patron_cancel { }; } -1; \ No newline at end of file +1; diff --git a/api/v1/swagger/paths/article_requests.yaml b/api/v1/swagger/paths/article_requests.yaml index 5e27193d0b..bcc0728050 100644 --- a/api/v1/swagger/paths/article_requests.yaml +++ b/api/v1/swagger/paths/article_requests.yaml @@ -51,6 +51,9 @@ description: Under maintenance schema: $ref: ../definitions.json#/error + x-koha-authorization: + permissions: + circulate: circulate_remaining_permissions "/public/patrons/{patron_id}/article_requests/{article_request_id}": delete: x-mojo-to: ArticleRequests#patron_cancel diff --git a/t/db_dependent/api/v1/article_requests.t b/t/db_dependent/api/v1/article_requests.t index 9518f42737..02adbdf489 100755 --- a/t/db_dependent/api/v1/article_requests.t +++ b/t/db_dependent/api/v1/article_requests.t @@ -40,7 +40,7 @@ subtest 'cancel() tests' => sub { my $authorized_patron = $builder->build_object( { class => 'Koha::Patrons', - value => { flags => 1 } + value => { flags => 2 ** 1 } # circulate flag = 1 } ); my $password = 'thePassword123'; @@ -80,7 +80,7 @@ subtest 'cancel() tests' => sub { subtest 'patron_cancel() tests' => sub { - plan tests => 12; + plan tests => 14; t::lib::Mocks::mock_preference( 'RESTPublicAPI', 1 ); t::lib::Mocks::mock_preference( 'RESTBasicAuth', 1 ); @@ -90,7 +90,7 @@ subtest 'patron_cancel() tests' => sub { my $patron = $builder->build_object( { class => 'Koha::Patrons', - value => { privacy_guarantor_checkouts => 0 } + value => { flags => 0 } } ); my $password = 'thePassword123'; @@ -102,15 +102,20 @@ subtest 'patron_cancel() tests' => sub { my $deleted_article_request_id = $deleted_article_request->id; $deleted_article_request->delete; + # delete non existent article request + $t->delete_ok("//$userid:$password@/api/v1/public/patrons/$patron_id/article_requests/$deleted_article_request_id") + ->status_is(404) + ->json_is( { error => "Article request not found" } ); + my $another_patron = $builder->build_object({ class => 'Koha::Patrons' }); my $another_patron_id = $another_patron->id; - $t->delete_ok("//$userid:$password@/api/v1/public/patrons/$another_patron_id/article_requests/$deleted_article_request_id") - ->status_is(403); + my $article_request_2 = $builder->build_object({ class => 'Koha::ArticleRequests', value => { borrowernumber => $another_patron_id } }); - $t->delete_ok("//$userid:$password@/api/v1/public/patrons/$patron_id/article_requests/$deleted_article_request_id") - ->status_is(404) - ->json_is( { error => "Article request not found" } ); + # delete another patron's request + $t->delete_ok("//$userid:$password@/api/v1/public/patrons/$another_patron_id/article_requests/" . $article_request_2->id) + ->status_is(403) + ->json_is( '/error' => 'Authorization failure. Missing required permission(s).' ); my $another_article_request = $builder->build_object( { @@ -119,9 +124,9 @@ subtest 'patron_cancel() tests' => sub { } ); - $t->delete_ok("//$userid:$password@/api/v1/public/patrons/$patron_id/article_requests/$another_article_request") - ->status_is(403); - + $t->delete_ok("//$userid:$password@/api/v1/public/patrons/$patron_id/article_requests/" . $another_article_request->id) + ->status_is(404) + ->json_is( { error => 'Article request not found' } ); my $article_request = $builder->build_object( { -- 2.39.5