Attachment 83676 Details for Bug 22071 – Bug 22071: (follow-up) Simplify code

[patch] Bug 22071: (follow-up) Simplify code

Bug-22071-follow-up-Simplify-code.patch (text/plain), 5.97 KB, created by Tomás Cohen Arazi (tcohen) on 2019-01-07 10:39:15 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Tomás Cohen Arazi (tcohen)

Created: 2019-01-07 10:39:15 UTC

Size: 5.97 KB

patch

obsolete

>From 6ab6ce25c3be69ef5fcb35ffad55daa3a5f6c2a1 Mon Sep 17 00:00:00 2001
>From: Tomas Cohen Arazi <tomascohen@theke.io>
>Date: Mon, 7 Jan 2019 07:31:43 -0300
>Subject: [PATCH] Bug 22071: (follow-up) Simplify code
>
>In order to add features to this method, the current code would force us
>to do it for each authentication method.
>
>There's duplicated code that could be simplified. This patch makes the
>authentication code just set $user on each block (oauth and cookie
>authentication) and moves the final permissions check to the end of the
>authenticate_api_request method.
>
>Overall, the behaviour remains unchanged.
>
>To test:
>- Run:
>  $ kshell
> k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t \
>          t/db_dependent/api/v1/oauth.t
>=> SUCCESS: Tests pass! Nothing changed!
>- Sign off :-D
>---
> Koha/REST/V1/Auth.pm | 106 ++++++++++++++++++++-----------------------
> 1 file changed, 48 insertions(+), 58 deletions(-)
>
>diff --git a/Koha/REST/V1/Auth.pm b/Koha/REST/V1/Auth.pm
>index aed9e2f4fa..512ed5d6ce 100644
>--- a/Koha/REST/V1/Auth.pm
>+++ b/Koha/REST/V1/Auth.pm
>@@ -113,6 +113,8 @@ if authorization is required and user has required permissions to access.
> sub authenticate_api_request {
>     my ( $c ) = @_;
> 
>+    my $user;
>+
>     my $spec = $c->match->endpoint->pattern->defaults->{'openapi.op_spec'};
>     my $authorization = $spec->{'x-koha-authorization'};
> 
>@@ -138,69 +140,56 @@ sub authenticate_api_request {
>         );
> 
>         if ($valid_token) {
>-            my $patron_id   = Koha::ApiKeys->find( $valid_token->{client_id} )->patron_id;
>-            my $patron      = Koha::Patrons->find($patron_id);
>-            $c->stash('koha.user' => $patron);
>-
>-            my $permissions = $authorization->{'permissions'};
>-            # Check if the patron is authorized
>-            if ( haspermission($patron->userid, $permissions)
>-                or allow_owner($c, $authorization, $patron)
>-                or allow_guarantor($c, $authorization, $patron) ) {
>-
>-                validate_query_parameters( $c, $spec );
>-
>-                # Everything is ok
>-                return 1;
>-            }
>-
>-            Koha::Exceptions::Authorization::Unauthorized->throw(
>-                error => "Authorization failure. Missing required permission(s).",
>-                required_permissions => $permissions,
>+            my $patron_id = Koha::ApiKeys->find( $valid_token->{client_id} )->patron_id;
>+            $user         = Koha::Patrons->find($patron_id);
>+        }
>+        else {
>+            # If we have "Authorization: Bearer" header and oauth authentication
>+            # failed, do not try other authentication means
>+            Koha::Exceptions::Authentication::Required->throw(
>+                error => 'Authentication failure.'
>             );
>         }
>-
>-        # If we have "Authorization: Bearer" header and oauth authentication
>-        # failed, do not try other authentication means
>-        Koha::Exceptions::Authentication::Required->throw(
>-            error => 'Authentication failure.'
>-        );
>-    }
>-
>-    my $cookie = $c->cookie('CGISESSID');
>-    my ($session, $user);
>-    # Mojo doesn't use %ENV the way CGI apps do
>-    # Manually pass the remote_address to check_auth_cookie
>-    my $remote_addr = $c->tx->remote_address;
>-    my ($status, $sessionID) = check_cookie_auth(
>-                                            $cookie, undef,
>-                                            { remote_addr => $remote_addr });
>-    if ($status eq "ok") {
>-        $session = get_session($sessionID);
>-        $user = Koha::Patrons->find($session->param('number'));
>-        $c->stash('koha.user' => $user);
>-    }
>-    elsif ($status eq "maintenance") {
>-        Koha::Exceptions::UnderMaintenance->throw(
>-            error => 'System is under maintenance.'
>-        );
>     }
>-    elsif ($status eq "expired" and $authorization) {
>-        Koha::Exceptions::Authentication::SessionExpired->throw(
>-            error => 'Session has been expired.'
>-        );
>-    }
>-    elsif ($status eq "failed" and $authorization) {
>-        Koha::Exceptions::Authentication::Required->throw(
>-            error => 'Authentication failure.'
>-        );
>-    }
>-    elsif ($authorization) {
>-        Koha::Exceptions::Authentication->throw(
>-            error => 'Unexpected authentication status.'
>-        );
>+    else {
>+
>+        my $cookie = $c->cookie('CGISESSID');
>+
>+        # Mojo doesn't use %ENV the way CGI apps do
>+        # Manually pass the remote_address to check_auth_cookie
>+        my $remote_addr = $c->tx->remote_address;
>+        my ($status, $sessionID) = check_cookie_auth(
>+                                                $cookie, undef,
>+                                                { remote_addr => $remote_addr });
>+        if ($status eq "ok") {
>+            my $session = get_session($sessionID);
>+            $user = Koha::Patrons->find($session->param('number'));
>+            # $c->stash('koha.user' => $user);
>+        }
>+        elsif ($status eq "maintenance") {
>+            Koha::Exceptions::UnderMaintenance->throw(
>+                error => 'System is under maintenance.'
>+            );
>+        }
>+        elsif ($status eq "expired" and $authorization) {
>+            Koha::Exceptions::Authentication::SessionExpired->throw(
>+                error => 'Session has been expired.'
>+            );
>+        }
>+        elsif ($status eq "failed" and $authorization) {
>+            Koha::Exceptions::Authentication::Required->throw(
>+                error => 'Authentication failure.'
>+            );
>+        }
>+        elsif ($authorization) {
>+            Koha::Exceptions::Authentication->throw(
>+                error => 'Unexpected authentication status.'
>+            );
>+        }
>     }
> 
>+    $c->stash('koha.user' => $user);
>+
>     # We do not need any authorization
>     unless ($authorization) {
>         # Check the parameters
>@@ -225,6 +214,7 @@ sub authenticate_api_request {
>         required_permissions => $permissions,
>     );
> }
>+
> sub validate_query_parameters {
>     my ( $c, $action_spec ) = @_;
> 
>-- 
>2.20.1

Actions: View | Diff | Splinter Review

Attachments on bug 22071: 83638 | 83639 | 83641 | 83642 | 83651 | 83652 | 83676 | 83677 | 83686 | 83687 | 83688 | 83689