From bf61f5f6261f29fa250aceb47b7a66c980647379 Mon Sep 17 00:00:00 2001
From: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Date: Fri, 30 Jul 2021 12:36:46 +0200
Subject: [PATCH] Bug 28786: Improve readability in C4::Auth::checkauth

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
---
 C4/Auth.pm | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/C4/Auth.pm b/C4/Auth.pm
index d37f69d83a2..c4cd1dc73a6 100644
--- a/C4/Auth.pm
+++ b/C4/Auth.pm
@@ -827,6 +827,7 @@ sub checkauth {
 
     # state variables
     my $loggedin = 0;
+    my $auth_state = 'failed';
     my %info;
     my ( $userid, $cookie, $sessionID, $flags );
     my $logout = $query->param('logout.x');
@@ -841,7 +842,6 @@ sub checkauth {
     my $session;
     my $invalid_otp_token;
     my $require_2FA = ( C4::Context->preference('TwoFactorAuthentication') && $type ne "OPAC" ) ? 1 : 0;
-    my $auth_challenge_complete;
 
     # Basic authentication is incompatible with the use of Shibboleth,
     # as Shibboleth may return REMOTE_USER as a Shibboleth attribute,
@@ -877,12 +877,15 @@ sub checkauth {
             $userid = $session->param('id');
         }
 
-        $additional_auth_needed = ( $return eq 'additional-auth-needed' ) ? 1 : 0;
+        $auth_state =
+            $return eq 'ok'                     ? 'completed'
+          : $return eq 'additional-auth-needed' ? 'additional-auth-needed'
+          :                                       'failed';
 
         # We are at the second screen if the waiting-for-2FA is set in session
         # and otp_token param has been passed
         if (   $require_2FA
-            && $additional_auth_needed
+            && $auth_state eq 'additional-auth-needed'
             && ( my $otp_token = $query->param('otp_token') ) )
         {
             my $patron    = Koha::Patrons->find( { userid => $userid } );
@@ -891,10 +894,8 @@ sub checkauth {
             $auth->clear;
             if ( $verified ) {
                 # The token is correct, the user is fully logged in!
-                $additional_auth_needed = 0;
+                $auth_state = 'completed';
                 $session->param( 'waiting-for-2FA', 0 );
-                $return = "ok";
-                $auth_challenge_complete = 1;
 
                # This is an ugly trick to pass the test
                # $query->param('koha_login_context') && ( $q_userid ne $userid )
@@ -906,7 +907,7 @@ sub checkauth {
             }
         }
 
-        if ( $return eq 'ok' ) {
+        if ( $auth_state eq 'completed' ) {
             Koha::Logger->get->debug(sprintf "AUTH_SESSION: (%s)\t%s %s - %s", map { $session->param($_) || q{} } qw(cardnumber firstname surname branch));
 
             if ( ( $query->param('koha_login_context') && ( $q_userid ne $userid ) )
@@ -930,9 +931,8 @@ sub checkauth {
                 );
 
                 $flags = haspermission( $userid, $flagsrequired );
-                if ($flags) {
-                    $loggedin = 1;
-                } else {
+                unless ( $flags ) {
+                    $auth_state = 'failed';
                     $info{'nopermission'} = 1;
                 }
             }
@@ -947,7 +947,7 @@ sub checkauth {
         }
     }
 
-    if ( ( !$loggedin && !$additional_auth_needed ) || $logout ) {
+    if ( $auth_state eq 'failed' || $logout ) {
         $sessionID = undef;
         $session   = undef;
         $userid    = undef;
@@ -976,7 +976,7 @@ sub checkauth {
         $sessionID = undef;
         $session   = undef;
         $userid    = undef;
-        $additional_auth_needed = 0;
+        $auth_state = 'logout';
     }
 
     unless ( $userid ) {
@@ -1126,7 +1126,7 @@ sub checkauth {
             if ($return) {
 
                 if ( $flags = haspermission( $userid, $flagsrequired ) ) {
-                    $loggedin = 1;
+                    $auth_state = "logged_in";
                 }
                 else {
                     $info{'nopermission'} = 1;
@@ -1268,18 +1268,21 @@ sub checkauth {
         }
     }    # END unless ($userid)
 
-    if ( $require_2FA && ( $loggedin && !$auth_challenge_complete)) {
+
+    if ( $require_2FA && $auth_state eq 'logged_in' ) {
         my $patron = Koha::Patrons->find({userid => $userid});
         if ( $patron->auth_method eq 'two-factor' ) {
             # Ask for the OTP token
-            $additional_auth_needed = 1;
+            $auth_state = 'additional-auth-needed';
             $session->param('waiting-for-2FA', 1);
             %info = ();# We remove the warnings/errors we may have set incorrectly before
+        } else {
+            $auth_state = 'completed';
         }
     }
 
     # finished authentification, now respond
-    if ( ( $loggedin || $authnotrequired ) && !$additional_auth_needed ) {
+    if ( $auth_state eq 'completed' || $authnotrequired ) {
         # successful login
         unless ($cookie) {
             $cookie = $query->cookie(
@@ -1371,7 +1374,7 @@ sub checkauth {
     $template->param( SCI_login => 1 ) if ( $query->param('sci_user_login') );
     $template->param( OpacPublic => C4::Context->preference("OpacPublic") );
     $template->param( loginprompt => 1 ) unless $info{'nopermission'};
-    if ( $additional_auth_needed ) {
+    if ( $auth_state eq 'additional-auth-needed' ) {
         $template->param(
             TwoFA_prompt => 1,
             invalid_otp_token => $invalid_otp_token,
-- 
2.25.1