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>
(cherry picked from commit 0d5058b7b29d90cc7c8e533ee56388fbb5a96d52)
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
(cherry picked from commit a6dc145ce966338ec0ae96f7e32968149647c02f)

Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>

diff --git a/Koha/REST/V1/Auth.pm b/Koha/REST/V1/Auth.pm
index aed9e2f..512ed5d 100644 (file)
--- 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 ) = @_;