From 053eb7a4e76a95da38c981b2f0371f92d4942bda Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi 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 Signed-off-by: Nick Clemens (cherry picked from commit 0d5058b7b29d90cc7c8e533ee56388fbb5a96d52) Signed-off-by: Martin Renvoize (cherry picked from commit a6dc145ce966338ec0ae96f7e32968149647c02f) Signed-off-by: Jesse Maseto --- 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.2