From a2d08d02e06f848d00afde635db5f8e07bb0a457 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>
---
 C4/Auth.pm | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/C4/Auth.pm b/C4/Auth.pm
index 0c60dbd0c7c..3c37966cb5a 100644
--- a/C4/Auth.pm
+++ b/C4/Auth.pm
@@ -828,6 +828,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');
@@ -842,7 +843,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,
@@ -878,12 +878,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 } );
@@ -892,10 +895,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 )
@@ -907,7 +908,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 ) )
@@ -933,9 +934,8 @@ sub checkauth {
                 my $sessiontype = $session->param('sessiontype') || '';
                 unless ( $sessiontype && $sessiontype eq 'anon' ) {    #if this is an anonymous session, we want to update the session, but not behave as if they are logged in...
                     $flags = haspermission( $userid, $flagsrequired );
-                    if ($flags) {
-                        $loggedin = 1;
-                    } else {
+                    unless ( $flags ) {
+                        $auth_state = 'failed';
                         $info{'nopermission'} = 1;
                     }
                 }
@@ -951,7 +951,7 @@ sub checkauth {
         }
     }
 
-    if ( ( !$loggedin && !$additional_auth_needed ) || $logout ) {
+    if ( $auth_state eq 'failed' || $logout ) {
         $sessionID = undef;
         $session   = undef;
         $userid    = undef;
@@ -980,7 +980,7 @@ sub checkauth {
         $sessionID = undef;
         $session   = undef;
         $userid    = undef;
-        $additional_auth_needed = 0;
+        $auth_state = 'logout';
     }
 
     unless ( $userid ) {
@@ -1130,7 +1130,7 @@ sub checkauth {
             if ($return) {
 
                 if ( $flags = haspermission( $userid, $flagsrequired ) ) {
-                    $loggedin = 1;
+                    $auth_state = "logged_in";
                 }
                 else {
                     $info{'nopermission'} = 1;
@@ -1272,18 +1272,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(
@@ -1377,7 +1380,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