From 9c4a1b19aa86b246ea2528a6f1a33401d1007fae 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>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
---
 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