From d4589500d197ddb543b63b4c84e87e763d050b40 Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Tue, 19 Nov 2019 13:01:56 +0000 Subject: [PATCH] Bug 24065: Fail shib login if multiple users matched Ideally you could test against active shib, but is a small code change and covered by tests and should be readable To test: prove -v t/Auth_with_shibboleth.t Signed-off-by: Liz Rea Signed-off-by: Marcel de Rooy Signed-off-by: Martin Renvoize --- C4/Auth_with_shibboleth.pm | 11 ++++++++--- t/Auth_with_shibboleth.t | 31 ++++++++++++++++++++++++++++--- 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/C4/Auth_with_shibboleth.pm b/C4/Auth_with_shibboleth.pm index 55cee73569..0c493f14b4 100644 --- a/C4/Auth_with_shibboleth.pm +++ b/C4/Auth_with_shibboleth.pm @@ -103,9 +103,14 @@ sub checkpw_shib { $debug and warn "User Shibboleth-authenticated as: $match"; # Does the given shibboleth attribute value ($match) match a valid koha user ? - my $borrower = - Koha::Database->new()->schema()->resultset('Borrower') - ->find( { $config->{matchpoint} => $match } ); + my $borrowers = Koha::Patrons->search( { $config->{matchpoint} => $match } ); + if ( $borrowers->count > 1 ){ + # If we have more than 1 borrower the matchpoint is not unique + # we cannot know which patron is the correct one, so we should fail + $debug and warn "There are several users with $config->{matchpoint} of $match, matchpoints must be unique"; + return 0; + } + my $borrower = $borrowers->next; if ( defined($borrower) ) { if ($config->{'sync'}) { _sync($borrower->borrowernumber, $config, $match); diff --git a/t/Auth_with_shibboleth.t b/t/Auth_with_shibboleth.t index 9e95adf0e1..817394c883 100644 --- a/t/Auth_with_shibboleth.t +++ b/t/Auth_with_shibboleth.t @@ -166,7 +166,7 @@ subtest "get_login_shib tests" => sub { ## checkpw_shib subtest "checkpw_shib tests" => sub { - plan tests => 21; + plan tests => 24; my $shib_login; my ( $retval, $retcard, $retuserid ); @@ -174,8 +174,10 @@ subtest "checkpw_shib tests" => sub { # Setup Mock Database Data fixtures_ok [ 'Borrower' => [ - [qw/cardnumber userid surname address city/], - [qw/testcardnumber test1234 renvoize myaddress johnston/], + [qw/cardnumber userid surname address city email/], + [qw/testcardnumber test1234 renvoize myaddress johnston /], + [qw/testcardnumber1 test12345 clamp1 myaddress quechee kid@clamp.io/], + [qw/testcardnumber2 test123456 clamp2 myaddress quechee kid@clamp.io/], ], 'Category' => [ [qw/categorycode default_privacy/], [qw/S never/], ] ], @@ -202,6 +204,29 @@ subtest "checkpw_shib tests" => sub { [], "bad user with no debug"; is( $retval, "0", "user not authenticated" ); + # duplicated matchpoint + $matchpoint = 'email'; + $mapping{'email'} = { is => 'email' }; + $shib_login = 'kid@clamp.io'; + warnings_are { + ( $retval, $retcard, $retuserid ) = checkpw_shib($shib_login); + } + [], "bad user with no debug"; + is( $retval, "0", "user not authenticated if duplicated matchpoint" ); + $C4::Auth_with_shibboleth::debug = '1'; + warnings_are { + ( $retval, $retcard, $retuserid ) = checkpw_shib($shib_login); + } + [ + q/checkpw_shib/, + q/koha borrower field to match: email/, + q/shibboleth attribute to match: email/, + q/User Shibboleth-authenticated as: kid@clamp.io/, + q/There are several users with email of kid@clamp.io, matchpoints must be unique/ + ], "duplicated matchpoint warned with debug"; + $C4::Auth_with_shibboleth::debug = '0'; + reset_config(); + # autocreate user $autocreate = 1; $shib_login = 'test4321'; -- 2.39.5