From d4589500d197ddb543b63b4c84e87e763d050b40 Mon Sep 17 00:00:00 2001
From: Nick Clemens <nick@bywatersolutions.com>
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 <wizzyrea@gmail.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
---
 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