Bug 36575: Return correct patron when there is a shared userid / cardnumber
This patch moves some patron fetching code in C4/Auth to use to patron returned from the validation
methods and only try to fetch the patron (to check if locked, update attempts, etc) if we didn't authenticate
To test:
1 - Set a user to have userid = BANANA password = Password1
2 - Set a user to have cardnumber = BANANA password = Password2
3 - Hit the patron authentication API:
http://localhost:8080/api/v1/auth/password/validation
with data:
{ "identifier": "BANANA", "password":"Password1" }
and:
{ "identifier": "BANANA", "password":"Password2" }
4 - Note you receive the same response for both
5 - Apply patch, restart all
6 - Repeat the API and confirm you get the correct patron for the password submitted
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(cherry picked from commit ff4e0c4293486d2db31d2f48d9f6f31d6470965a)
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
This commit is contained in:
Nick Clemens
Fridolin Somers
parent
c97b32a60e
commit
39aeec0438
35
C4/Auth.pm
35
C4/Auth.pm
|
|
@ -1982,23 +1982,17 @@ sub checkpw {
|
|||
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;
|
||||
}
|
||||
my $check_internal_as_fallback = 0;
|
||||
my $passwd_ok = 0;
|
||||
my $patron;
|
||||
|
||||
|
||||
# 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 ) ) {
|
||||
|
||||
# Nothing to check, account is locked
|
||||
} elsif ( $ldap && defined($password) ) {
|
||||
if ( $ldap && defined($password) ) {
|
||||
my ( $retval, $retcard, $retuserid );
|
||||
( $retval, $retcard, $retuserid, $patron ) = checkpw_ldap(@_); # EXTERNAL AUTH
|
||||
if ( $retval == 1 ) {
|
||||
|
|
@ -2012,7 +2006,8 @@ 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 ) =
|
||||
my ( $retval, $retcard, $retuserid, $cas_ticket );
|
||||
( $retval, $retcard, $retuserid, $cas_ticket, $patron ) =
|
||||
checkpw_cas( $ticket, $query, $type ); # EXTERNAL AUTH
|
||||
if ($retval) {
|
||||
@return = ( $retval, $retcard, $retuserid, $patron, $cas_ticket );
|
||||
|
|
@ -2033,7 +2028,8 @@ sub checkpw {
|
|||
|
||||
# Then, we check if it matches a valid koha user
|
||||
if ($shib_login) {
|
||||
my ( $retval, $retcard, $retuserid, $patron ) =
|
||||
my ( $retval, $retcard, $retuserid );
|
||||
( $retval, $retcard, $retuserid, $patron ) =
|
||||
C4::Auth_with_shibboleth::checkpw_shib($shib_login); # EXTERNAL AUTH
|
||||
if ($retval) {
|
||||
@return = ( $retval, $retcard, $retuserid, $patron );
|
||||
|
|
@ -2044,20 +2040,29 @@ sub checkpw {
|
|||
$check_internal_as_fallback = 1;
|
||||
}
|
||||
|
||||
if ( $check_internal_as_fallback ){
|
||||
# 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
|
||||
$patron = Koha::Patrons->find({ cardnumber => $return[1] }) if $passwd_ok;
|
||||
push @return, $patron if $patron;
|
||||
}
|
||||
|
||||
if ( defined $userid && !$patron ) {
|
||||
$patron = Koha::Patrons->find( { userid => $userid } );
|
||||
$patron = Koha::Patrons->find( { cardnumber => $userid } ) unless $patron;
|
||||
push @return, $patron if $check_internal_as_fallback;
|
||||
}
|
||||
|
||||
if ($patron) {
|
||||
if ($passwd_ok) {
|
||||
if( $patron->account_locked ){
|
||||
@return = ();
|
||||
} elsif ($passwd_ok) {
|
||||
$patron->update( { login_attempts => 0 } );
|
||||
if ( $patron->password_expired ) {
|
||||
@return = ( -2, $patron );
|
||||
}
|
||||
} elsif ( !$patron->account_locked ) {
|
||||
} else {
|
||||
$patron->update( { login_attempts => $patron->login_attempts + 1 } );
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -7,7 +7,7 @@ use CGI qw ( -utf8 );
|
|||
use Test::MockObject;
|
||||
use Test::MockModule;
|
||||
use List::MoreUtils qw/all any none/;
|
||||
use Test::More tests => 22;
|
||||
use Test::More tests => 23;
|
||||
use Test::Warn;
|
||||
use t::lib::Mocks;
|
||||
use t::lib::TestBuilder;
|
||||
|
|
@ -458,7 +458,7 @@ subtest 'checkpw lockout tests' => sub {
|
|||
( $checkpw, undef, undef ) = checkpw( $patron->userid, $password, undef, undef, 1 );
|
||||
is( $checkpw, undef, 'checkpw returns undef with right password when account locked' );
|
||||
( $checkpw, undef, undef ) = checkpw( $patron->cardnumber, $password, undef, undef, 1 );
|
||||
is( $checkpw, undef, 'checkpw returns undefwith right password when logging in via cardnumber if account locked' );
|
||||
is( $checkpw, undef, 'checkpw returns undef with right password when logging in via cardnumber if account locked' );
|
||||
|
||||
};
|
||||
|
||||
|
|
@ -1565,6 +1565,31 @@ subtest 'AutoSelfCheckAllowed' => sub {
|
|||
is( $loggedinuser, $sco_patron->id );
|
||||
};
|
||||
|
||||
subtest 'checkpw for users with shared cardnumber / userid ' => sub {
|
||||
|
||||
plan tests => 8;
|
||||
|
||||
t::lib::Mocks::mock_preference( 'RequireStrongPassword', 0 );
|
||||
my $library = $builder->build_object( { class => 'Koha::Libraries' } );
|
||||
my $patron_1 = $builder->build_object( { class => 'Koha::Patrons' } );
|
||||
$patron_1->set_password( { password => "OnePassword" } );
|
||||
my $patron_2 = $builder->build_object( { class => 'Koha::Patrons', value => { userid => $patron_1->cardnumber } } );
|
||||
$patron_2->set_password( { password => "PasswordTwo" } );
|
||||
|
||||
my ( $checkpw, $cardnumber, $userid, $patron ) = checkpw( $patron_1->cardnumber, "OnePassword", undef, undef, 1 );
|
||||
ok( $checkpw, 'checkpw returns true for right password when logging in via cardnumber' );
|
||||
is( $cardnumber, $patron_1->cardnumber, 'checkpw returns correct cardnumber' );
|
||||
is( $userid, $patron_1->userid, 'checkpw returns correct userid' );
|
||||
is( $patron->id, $patron_1->id, 'checkpw returns correct patron' );
|
||||
|
||||
( $checkpw, $cardnumber, $userid, $patron ) = checkpw( $patron_2->userid, "PasswordTwo", undef, undef, 1 );
|
||||
ok( $checkpw, 'checkpw returns true for right password when logging in via userid' );
|
||||
is( $cardnumber, $patron_2->cardnumber, 'checkpw returns correct cardnumber' );
|
||||
is( $userid, $patron_2->userid, 'checkpw returns correct userid' );
|
||||
is( $patron->id, $patron_2->id, 'checkpw returns correct patron' );
|
||||
|
||||
};
|
||||
|
||||
sub set_weak_password {
|
||||
my ($patron) = @_;
|
||||
my $password = 'password';
|
||||
|
|
|
|||
|
|
@ -18,7 +18,7 @@
|
|||
|
||||
use Modern::Perl;
|
||||
|
||||
use Test::More tests => 5;
|
||||
use Test::More tests => 6;
|
||||
use Test::Mojo;
|
||||
|
||||
use t::lib::TestBuilder;
|
||||
|
|
@ -239,4 +239,63 @@ subtest 'password validation - expired password' => sub {
|
|||
$schema->storage->txn_rollback;
|
||||
};
|
||||
|
||||
subtest 'password validation - users with shared cardnumber / userid' => sub {
|
||||
|
||||
plan tests => 12;
|
||||
|
||||
$schema->storage->txn_begin;
|
||||
|
||||
my $patron_1 = $builder->build_object(
|
||||
{
|
||||
class => 'Koha::Patrons',
|
||||
value => { }
|
||||
}
|
||||
);
|
||||
my $patron_password_1 = 'thePassword123';
|
||||
$patron_1->set_password( { password => $patron_password_1, skip_validation => 1 } );
|
||||
|
||||
my $patron_2 = $builder->build_object(
|
||||
{
|
||||
class => 'Koha::Patrons',
|
||||
value => { userid => $patron_1->cardnumber }
|
||||
}
|
||||
);
|
||||
my $patron_password_2 = 'thePassword345';
|
||||
$patron_2->set_password( { password => $patron_password_2, skip_validation => 1 } );
|
||||
|
||||
my $json = {
|
||||
identifier => $patron_1->cardnumber,
|
||||
password => $patron_password_1,
|
||||
};
|
||||
|
||||
$t->post_ok( "//$userid:$password@/api/v1/auth/password/validation" => json => $json )->status_is(201)
|
||||
->json_is({ cardnumber => $patron_1->cardnumber, patron_id => $patron_1->borrowernumber, userid => $patron_1->userid} );
|
||||
|
||||
$json = {
|
||||
identifier => $patron_2->userid,
|
||||
password => $patron_password_2,
|
||||
};
|
||||
|
||||
$t->post_ok( "//$userid:$password@/api/v1/auth/password/validation" => json => $json )->status_is(201)
|
||||
->json_is({ cardnumber => $patron_2->cardnumber, patron_id => $patron_2->borrowernumber, userid => $patron_2->userid} );
|
||||
|
||||
my $json = {
|
||||
userid => $patron_1->cardnumber,
|
||||
password => $patron_password_1,
|
||||
};
|
||||
|
||||
$t->post_ok( "//$userid:$password@/api/v1/auth/password/validation" => json => $json )->status_is(201)
|
||||
->json_is({ cardnumber => $patron_1->cardnumber, patron_id => $patron_1->borrowernumber, userid => $patron_1->userid} );
|
||||
|
||||
$json = {
|
||||
userid => $patron_2->userid,
|
||||
password => $patron_password_2,
|
||||
};
|
||||
|
||||
$t->post_ok( "//$userid:$password@/api/v1/auth/password/validation" => json => $json )->status_is(201)
|
||||
->json_is({ cardnumber => $patron_2->cardnumber, patron_id => $patron_2->borrowernumber, userid => $patron_2->userid} );
|
||||
|
||||
$schema->storage->txn_rollback;
|
||||
};
|
||||
|
||||
$schema->storage->txn_rollback;
|
||||
|
|
|
|||
Loading…
Reference in New Issue