From 8ebffcdb106d46c9f8b1877a086407ecfb1cbae1 Mon Sep 17 00:00:00 2001
From: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Date: Fri, 14 Oct 2016 14:26:58 +0100
Subject: [PATCH] Bug 17445: Move the params check after the authentication
 check

If the user is not authorised to call this route, we would prefer to
raise a 403 instead of 400

Note that we wanted to submit tests for this change but the city code
does not let use do that (we are allowed to list/show cities even
without any permissions). The patrons.t is not complete enought and the
holds.t tests do not pass...

Tomas plans to submit tests but we reach the end of the hackfest ;)

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
---
 Koha/REST/V1.pm | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/Koha/REST/V1.pm b/Koha/REST/V1.pm
index 4fa0df3a7c..76ae180792 100644
--- a/Koha/REST/V1.pm
+++ b/Koha/REST/V1.pm
@@ -91,21 +91,36 @@ sub authenticate_api_request {
         ) if $cookie and $action_spec->{'x-koha-authorization'};
     }
 
-    my @errors = validate_parameters( $c, $action_spec );
-    return $c->render_swagger({}, \@errors, 400) if @errors;
 
-    return $next->($c) unless $action_spec->{'x-koha-authorization'};
+    # Then check the parameters
+    my @query_errors = validate_query_parameters( $c, $action_spec );
+
+    # We do not need any authorization
+    unless ( $action_spec->{'x-koha-authorization'} ) {
+        return @query_errors
+            ? $c->render_swagger({}, \@query_errors, 400)
+            : $next->($c);
+    }
+
     unless ($user) {
         return $c->render_swagger({ error => "Authentication required." },{},401);
     }
 
     my $authorization = $action_spec->{'x-koha-authorization'};
     my $permissions = $authorization->{'permissions'};
+
+    # Check if the user is authorized
     if ( C4::Auth::haspermission($user->userid, $permissions)
         or allow_owner($c, $authorization, $user)
         or allow_guarantor($c, $authorization, $user) ) {
+
+        # Return the query errors if exist
+        return $c->render_swagger({}, \@query_errors, 400) if @query_errors;
+
+        # Everything is ok
         return $next->($c)
     }
+
     return $c->render_swagger(
         { error => "Authorization failure. Missing required permission(s).",
           required_permissions => $permissions },
@@ -114,7 +129,7 @@ sub authenticate_api_request {
     );
 }
 
-sub validate_parameters {
+sub validate_query_parameters {
     my ( $c, $action_spec ) = @_;
 
     # Check for malformed query parameters
-- 
2.39.5