From 73b1ea9c3bef52be9076c9ac8e9593982fb951ec Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Tue, 11 Jul 2017 12:24:36 -0300 Subject: [PATCH] Bug 18880: Fix authentication fallback for external authentications A regression in commit cfc484b17 / bug #18314 breaks the local authentication fallback for all external authentications like LDAP, CAS and Shibboleth. The regression itself is a logical error as "@return = (0)" is considered to be "false" when checked with "unless" (line 1814). That's wrong as "unless" tests the number of elements in a list. Thus the "falsy" condition has to established with "@return = ()" instead. This patch tries to simplify the logic by adding a $passwd_ok and $check_internal_as_fallback flags to be more verbose and hopefully more understandable. The goal here is simply to restore back the same logic as before cfc484b17 --- C4/Auth.pm | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/C4/Auth.pm b/C4/Auth.pm index b1e7824d4f..e6acda9dd0 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -1763,17 +1763,24 @@ sub checkpw { my @return; my $patron = Koha::Patrons->find({ userid => $userid }); + my $check_internal_as_fallback = 0; + my $passwd_ok = 0; + # Note: checkpw_* routines returns: + # 1 if auth is ok + # 0 if auth is nok + # -1 if user bind failed (LDAP only) + # 2 if DB user is used (internal only) if ( $patron and $patron->account_locked ) { - @return = (0); + # Nothing to check, account is locked } elsif ($ldap) { $debug and print STDERR "## checkpw - checking LDAP\n"; my ( $retval, $retcard, $retuserid ) = checkpw_ldap(@_); # EXTERNAL AUTH if ( $retval ) { @return = ( $retval, $retcard, $retuserid ); - } else { - @return = (0); } + $passwd_ok = 1 if $retval == 1; + $check_internal_as_fallback = 1 if $retval == 0; } elsif ( $cas && $query && $query->param('ticket') ) { $debug and print STDERR "## checkpw - checking CAS\n"; @@ -1784,9 +1791,8 @@ sub checkpw { my ( $retval, $retcard, $retuserid ) = checkpw_cas( $dbh, $ticket, $query, $type ); # EXTERNAL AUTH if ( $retval ) { @return = ( $retval, $retcard, $retuserid ); - } else { - @return = (0); } + $passwd_ok = $retval; } # If we are in a shibboleth session (shibboleth is enabled, and a shibboleth match attribute is present) @@ -1805,23 +1811,25 @@ sub checkpw { my ( $retval, $retcard, $retuserid ) = C4::Auth_with_shibboleth::checkpw_shib($shib_login); # EXTERNAL AUTH if ( $retval ) { @return = ( $retval, $retcard, $retuserid ); - } else { - @return = (0); } + $passwd_ok = $retval; } + } else { + $check_internal_as_fallback = 1; } # INTERNAL AUTH - @return = checkpw_internal( $dbh, $userid, $password, $no_set_userenv) unless @return; + if ( $check_internal_as_fallback ) { + @return = checkpw_internal( $dbh, $userid, $password, $no_set_userenv); + $passwd_ok = 1 if $return[0] > 0; # 1 or 2 + } - if ( $return[0] == 0 ) { + unless ( $passwd_ok ) { # Auth failed $patron->update({ login_attempts => $patron->login_attempts + 1 }) if $patron; - } elsif ( $return[0] == 1 ) { - if ( $patron ) { - # FIXME Koha::Object->update should return a Koha::Object to allow chaining - $patron->update({ login_attempts => 0 }); - $patron->store; - } + } elsif ( $patron ) { + # FIXME Koha::Object->update should return a Koha::Object to allow chaining + $patron->update({ login_attempts => 0 }); + $patron->store; } return @return; } -- 2.11.0