From b447b2a8a404ad66e25baeaffd3686e1d804399a Mon Sep 17 00:00:00 2001 From: Jonathan Druart 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 --- 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