Attachment 132147 Details for Bug 28786 – Bug 28786: Improve readability in C4::Auth::checkauth

[patch] Bug 28786: Improve readability in C4::Auth::checkauth

Bug-28786-Improve-readability-in-C4Authcheckauth.patch (text/plain), 5.80 KB, created by Jonathan Druart on 2022-03-24 13:30:17 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jonathan Druart

Created: 2022-03-24 13:30:17 UTC

Size: 5.80 KB

patch

obsolete

>From f6c8bc0739093df475ff74348d9c0bf576dc0813 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 | 50 ++++++++++++++++++++++++++++----------------------
> 1 file changed, 28 insertions(+), 22 deletions(-)
>
>diff --git a/C4/Auth.pm b/C4/Auth.pm
>index 0d4168f7852..fba86594100 100644
>--- a/C4/Auth.pm
>+++ b/C4/Auth.pm
>@@ -846,6 +846,7 @@ sub checkauth {
> 
>     # state variables
>     my $loggedin = 0;
>+    my $auth_state = 'failed';
>     my %info;
>     my ( $userid, $cookie, $sessionID, $flags );
>     $cookie = [];
>@@ -861,7 +862,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,
>@@ -897,12 +897,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 } );
>@@ -911,10 +914,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 )
>@@ -926,7 +927,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 ) )
>@@ -952,9 +953,8 @@ sub checkauth {
>                 ));
> 
>                 $flags = haspermission( $userid, $flagsrequired );
>-                if ($flags) {
>-                    $loggedin = 1;
>-                } else {
>+                unless ( $flags ) {
>+                    $auth_state = 'failed';
>                     $info{'nopermission'} = 1;
>                 }
>             }
>@@ -969,7 +969,7 @@ sub checkauth {
>         }
>     }
> 
>-    if ( ( !$loggedin && !$additional_auth_needed ) || $logout ) {
>+    if ( $auth_state eq 'failed' || $logout ) {
>         $sessionID = undef;
>         $userid    = undef;
>     }
>@@ -996,7 +996,7 @@ sub checkauth {
>         }
> 
>         $session   = undef;
>-        $additional_auth_needed = 0;
>+        $auth_state = 'logout';
>     }
> 
>     unless ( $userid ) {
>@@ -1148,7 +1148,7 @@ sub checkauth {
>             if ($return) {
> 
>                 if ( $flags = haspermission( $userid, $flagsrequired ) ) {
>-                    $loggedin = 1;
>+                    $auth_state = "logged_in";
>                 }
>                 else {
>                     $info{'nopermission'} = 1;
>@@ -1298,18 +1298,24 @@ sub checkauth {
>         $session->flush;
>     }    # END unless ($userid)
> 
>-    if ( $require_2FA && ( $loggedin && !$auth_challenge_complete)) {
>-        my $patron = Koha::Patrons->find({userid => $userid});
>-        if ( $patron->auth_method eq 'two-factor' ) {
>-            # Ask for the OTP token
>-            $additional_auth_needed = 1;
>-            $session->param('waiting-for-2FA', 1);
>-            %info = ();# We remove the warnings/errors we may have set incorrectly before
>+
>+    if ( $auth_state eq 'logged_in' ) {
>+        $auth_state = 'completed';
>+
>+        # Auth is completed unless an additional auth is needed
>+        if ( $require_2FA ) {
>+            my $patron = Koha::Patrons->find({userid => $userid});
>+            if ( $patron->auth_method eq 'two-factor' ) {
>+                # Ask for the OTP token
>+                $auth_state = 'additional-auth-needed';
>+                $session->param('waiting-for-2FA', 1);
>+                %info = ();# We remove the warnings/errors we may have set incorrectly before
>+            }
>         }
>     }
> 
>     # finished authentification, now respond
>-    if ( ( $loggedin || $authnotrequired ) && !$additional_auth_needed ) {
>+    if ( $auth_state eq 'completed' || $authnotrequired ) {
>         # successful login
>         unless (@$cookie) {
>             $cookie = $cookie_mgr->replace_in_list( $cookie, $query->cookie(
>@@ -1401,7 +1407,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

Actions: View | Diff | Splinter Review

Attachments on bug 28786: 123375 | 123376 | 123377 | 123378 | 123379 | 123380 | 123381 | 123397 | 123398 | 123399 | 123400 | 123401 | 123402 | 123403 | 124569 | 124770 | 124771 | 124844 | 124845 | 124846 | 124847 | 128841 | 128842 | 129383 | 129384 | 129474 | 129521 | 129557 | 129558 | 129559 | 129560 | 129561 | 129562 | 129563 | 129564 | 129565 | 129566 | 129567 | 129568 | 129569 | 129654 | 129655 | 129656 | 129657 | 129658 | 129659 | 129660 | 129661 | 129662 | 129665 | 131940 | 131941 | 131942 | 131943 | 131944 | 131945 | 131946 | 131947 | 131948 | 131949 | 131950 | 131951 | 131952 | 131953 | 131954 | 131955 | 131956 | 131957 | 131958 | 132142 | 132143 | 132144 | 132145 | 132146 | 132147 | 132148 | 132149 | 132150 | 132153 | 132154 | 132155 | 132156 | 132157 | 133222 | 133516 | 133522 | 133531 | 133533 | 133541 | 133548 | 133643 | 133686