Attachment 56545 Details for Bug 17428 – Bug 17428: Move the params check after the authentication check

[patch] Bug 17428: Move the params check after the authentication check

Bug-17428-Move-the-params-check-after-the-authenti.patch (text/plain), 2.49 KB, created by Jonathan Druart on 2016-10-14 14:21:13 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jonathan Druart

Created: 2016-10-14 14:21:13 UTC

Size: 2.49 KB

patch

obsolete

>From f90c8fac7c0945bd491a53be7c8239f81be7c41f 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 17428: 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 ;)
>---
> 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 4fa0df3..76ae180 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.8.1

Actions: View | Diff | Splinter Review

Attachments on bug 17428: 56189 | 56190 | 56191 | 56226 | 56227 | 56228 | 56261 | 56314 | 56501 | 56544 | 56545 | 56548 | 56553 | 56554 | 56555 | 56556 | 56557 | 56558 | 56559 | 56724 | 56741 | 56800 | 56801 | 56802 | 56803 | 56804 | 56807 | 56808 | 56809 | 56810 | 56811 | 56812