From 0d5058b7b29d90cc7c8e533ee56388fbb5a96d52 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

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
---
 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.39.5