Attachment 125016 Details for Bug 27947 – Bug 27947: (QA follow-up) Clarify permissions

[patch] Bug 27947: (QA follow-up) Clarify permissions

Bug-27947-QA-follow-up-Clarify-permissions.patch (text/plain), 5.34 KB, created by Tomás Cohen Arazi (tcohen) on 2021-09-17 19:26:19 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Tomás Cohen Arazi (tcohen)

Created: 2021-09-17 19:26:19 UTC

Size: 5.34 KB

patch

obsolete

>From 25f5ec395a181b1b069474a8289e9e017b977582 Mon Sep 17 00:00:00 2001
>From: Tomas Cohen Arazi <tomascohen@theke.io>
>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 <tomascohen@theke.io>
>---
> 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.33.0

Actions: View | Diff | Splinter Review

Attachments on bug 27947: 124063 | 124064 | 124065 | 124849 | 124850 | 124851 | 124952 | 124953 | 124954 | 124955 | 124956 | 124993 | 124994 | 124995 | 124996 | 124997 | 124998 | 124999 | 125002 | 125003 | 125004 | 125005 | 125006 | 125007 | 125008 | 125009 | 125010 | 125016 | 125038 | 125039 | 125040 | 125041 | 125042 | 125043 | 125044 | 125045 | 125046 | 125047 | 125165 | 125166 | 125167 | 125168 | 125169 | 125170 | 125171 | 125172 | 125173 | 125174 | 125739 | 125740