From 86eb9838e7b7e1312bf3bec69575d25c64ae3b93 Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Fri, 1 Dec 2023 06:29:19 -0500 Subject: [PATCH] Bug 34893: (QA follow-up) Tidy code for qa script Signed-off-by: Victor Grousset/tuxayo Signed-off-by: Tomas Cohen Arazi --- C4/Auth.pm | 54 +++++++++++++++-------------- C4/Auth_with_cas.pm | 10 +++--- t/db_dependent/ILSDI_Services.t | 60 ++++++++++++++++++--------------- 3 files changed, 67 insertions(+), 57 deletions(-) diff --git a/C4/Auth.pm b/C4/Auth.pm index 98e0d8198b9..bcf65844a41 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -1940,29 +1940,31 @@ sub checkpw { $type = 'opac' unless $type; # Get shibboleth login attribute - my $shib = C4::Context->config('useshibboleth') && shib_ok(); + my $shib = C4::Context->config('useshibboleth') && shib_ok(); my $shib_login = $shib ? get_login_shib() : undef; my @return; my $patron; - if ( defined $userid ){ - $patron = Koha::Patrons->find({ userid => $userid }); - $patron = Koha::Patrons->find({ cardnumber => $userid }) unless $patron; + if ( defined $userid ) { + $patron = Koha::Patrons->find( { userid => $userid } ); + $patron = Koha::Patrons->find( { cardnumber => $userid } ) unless $patron; } my $check_internal_as_fallback = 0; - my $passwd_ok = 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) - if ( $patron and ( $patron->account_locked ) ) { + if ( $patron and ( $patron->account_locked ) ) { + # Nothing to check, account is locked - } elsif ($ldap && defined($password)) { + } elsif ( $ldap && defined($password) ) { my ( $retval, $retcard, $retuserid ); ( $retval, $retcard, $retuserid, $patron ) = checkpw_ldap(@_); # EXTERNAL AUTH if ( $retval == 1 ) { - @return = ( $retval, $retcard, $retuserid, $patron ); + @return = ( $retval, $retcard, $retuserid, $patron ); $passwd_ok = 1; } $check_internal_as_fallback = 1 if $retval == 0; @@ -1971,9 +1973,10 @@ sub checkpw { # In case of a CAS authentication, we use the ticket instead of the password my $ticket = $query->param('ticket'); - $query->delete('ticket'); # remove ticket to come back to original URL - my ( $retval, $retcard, $retuserid, $cas_ticket, $patron ) = checkpw_cas( $ticket, $query, $type ); # EXTERNAL AUTH - if ( $retval ) { + $query->delete('ticket'); # remove ticket to come back to original URL + my ( $retval, $retcard, $retuserid, $cas_ticket, $patron ) = + checkpw_cas( $ticket, $query, $type ); # EXTERNAL AUTH + if ($retval) { @return = ( $retval, $retcard, $retuserid, $patron, $cas_ticket ); } else { @return = (0); @@ -1992,8 +1995,9 @@ sub checkpw { # Then, we check if it matches a valid koha user if ($shib_login) { - my ( $retval, $retcard, $retuserid, $patron ) = C4::Auth_with_shibboleth::checkpw_shib($shib_login); # EXTERNAL AUTH - if ( $retval ) { + my ( $retval, $retcard, $retuserid, $patron ) = + C4::Auth_with_shibboleth::checkpw_shib($shib_login); # EXTERNAL AUTH + if ($retval) { @return = ( $retval, $retcard, $retuserid, $patron ); } $passwd_ok = $retval; @@ -2003,27 +2007,27 @@ sub checkpw { } # INTERNAL AUTH - if ( $check_internal_as_fallback ) { - @return = checkpw_internal( $userid, $password, $no_set_userenv); - push(@return, $patron); - $passwd_ok = 1 if $return[0] > 0; # 1 or 2 + if ($check_internal_as_fallback) { + @return = checkpw_internal( $userid, $password, $no_set_userenv ); + push( @return, $patron ); + $passwd_ok = 1 if $return[0] > 0; # 1 or 2 } - if( $patron ) { - if ( $passwd_ok ) { - $patron->update({ login_attempts => 0 }); - if( $patron->password_expired ){ + if ($patron) { + if ($passwd_ok) { + $patron->update( { login_attempts => 0 } ); + if ( $patron->password_expired ) { @return = (-2); } - } elsif( !$patron->account_locked ) { - $patron->update({ login_attempts => $patron->login_attempts + 1 }); + } elsif ( !$patron->account_locked ) { + $patron->update( { login_attempts => $patron->login_attempts + 1 } ); } } # Optionally log success or failure - if( $patron && $passwd_ok && C4::Context->preference('AuthSuccessLog') ) { + if ( $patron && $passwd_ok && C4::Context->preference('AuthSuccessLog') ) { logaction( 'AUTH', 'SUCCESS', $patron->id, "Valid password for $userid", $type ); - } elsif( !$passwd_ok && C4::Context->preference('AuthFailureLog') ) { + } elsif ( !$passwd_ok && C4::Context->preference('AuthFailureLog') ) { logaction( 'AUTH', 'FAILURE', $patron ? $patron->id : 0, "Wrong password for $userid", $type ); } diff --git a/C4/Auth_with_cas.pm b/C4/Auth_with_cas.pm index e1cb1a97b25..68c2c48a47f 100644 --- a/C4/Auth_with_cas.pm +++ b/C4/Auth_with_cas.pm @@ -117,13 +117,13 @@ sub checkpw_cas { # we should store the CAS ticekt too, we need this for single logout https://apereo.github.io/cas/4.2.x/protocol/CAS-Protocol-Specification.html#233-single-logout # Does it match one of our users ? - my $dbh = C4::Context->dbh; - my $patron = Koha::Patrons->find({ userid => $userid }); - if ( $patron ) { + my $dbh = C4::Context->dbh; + my $patron = Koha::Patrons->find( { userid => $userid } ); + if ($patron) { return ( 1, $patron->cardnumber, $patron->userid, $ticket, $patron ); } - $patron = Koha::Patrons->find({ cardnumber => $userid }); - if ( $patron ) { + $patron = Koha::Patrons->find( { cardnumber => $userid } ); + if ($patron) { return ( 1, $patron->cardnumber, $patron->userid, $ticket, $patron ); } diff --git a/t/db_dependent/ILSDI_Services.t b/t/db_dependent/ILSDI_Services.t index 9fb01180a42..442f1c24022 100755 --- a/t/db_dependent/ILSDI_Services.t +++ b/t/db_dependent/ILSDI_Services.t @@ -1052,41 +1052,47 @@ subtest 'Bug 34893: ILS-DI can return the wrong patron for AuthenticatePatron' = my $plain_password = 'tomasito'; - $builder->build({ - source => 'Borrower', - value => { - cardnumber => undef, + $builder->build( + { + source => 'Borrower', + value => { + cardnumber => undef, + } } - }); + ); - my $borrower0 = $builder->build({ - source => 'Borrower', - value => { - cardnumber => "cardnumber1", - userid => undef, - password => Koha::AuthUtils::hash_password( $plain_password ), - lastseen => "2001-01-01 12:34:56" + my $borrower0 = $builder->build( + { + source => 'Borrower', + value => { + cardnumber => "cardnumber1", + userid => undef, + password => Koha::AuthUtils::hash_password($plain_password), + lastseen => "2001-01-01 12:34:56" + } } - }); + ); - my $borrower = $builder->build({ - source => 'Borrower', - value => { - cardnumber => "cardnumber2", - userid => undef, - password => Koha::AuthUtils::hash_password( $plain_password ), - lastseen => "2001-01-01 12:34:56" + my $borrower = $builder->build( + { + source => 'Borrower', + value => { + cardnumber => "cardnumber2", + userid => undef, + password => Koha::AuthUtils::hash_password($plain_password), + lastseen => "2001-01-01 12:34:56" + } } - }); + ); my $query = CGI->new; - $query->param( 'username', $borrower->{cardnumber}); - $query->param( 'password', $plain_password); + $query->param( 'username', $borrower->{cardnumber} ); + $query->param( 'password', $plain_password ); - my $reply = C4::ILSDI::Services::AuthenticatePatron( $query ); - is( $reply->{id}, $borrower->{borrowernumber}, "userid and password - Patron authenticated" ); - is( $reply->{code}, undef, "Error code undef"); - my $seen_patron = Koha::Patrons->find({ borrowernumber => $reply->{id} }); + my $reply = C4::ILSDI::Services::AuthenticatePatron($query); + is( $reply->{id}, $borrower->{borrowernumber}, "userid and password - Patron authenticated" ); + is( $reply->{code}, undef, "Error code undef" ); + my $seen_patron = Koha::Patrons->find( { borrowernumber => $reply->{id} } ); $schema->storage->txn_rollback; }; -- 2.43.0