From 74ed90c35ff85fe4244a1b4163bcbaabe45325c9 Mon Sep 17 00:00:00 2001
From: Liz Rea <liz@catalyst.net.nz>
Date: Tue, 31 Jan 2017 21:59:01 +0000
Subject: [PATCH] Bug 18025 - Expired password recovery links cause sql crash
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When a user gets an email, but doesn't act or visit it within two days,
     attempting to create a new one causes a collision. We should just
     delete the old one, assuming they still want to reset their
     password.

To test:
create yourself a borrower with a userid and password.
Attempt a password recovery on the OPAC
update the entry in the database for that user to have an expired token
e.g. update borrower_password_recovery set valid_until = '2017-01-25
03:25:26' where borrowernumber = 12;
Attempt another password recovery operation - should error
apply the patch
Try it again - no error, new token is generated and additional email
with new link is sent.

Issue reproduced without patch; resolved with patch.
Signed-off-by: Marc Véron <veron@veron.ch>
---
 Koha/Patron/Password/Recovery.pm  | 21 +++++++++++++++++--
 t/db_dependent/Passwordrecovery.t | 43 +++++++++++++++++++++++++++++++++++++--
 2 files changed, 60 insertions(+), 4 deletions(-)

diff --git a/Koha/Patron/Password/Recovery.pm b/Koha/Patron/Password/Recovery.pm
index 04491e6..c5def32 100644
--- a/Koha/Patron/Password/Recovery.pm
+++ b/Koha/Patron/Password/Recovery.pm
@@ -66,11 +66,10 @@ sub ValidateBorrowernumber {
         },
         { columns => 'borrowernumber' }
     );
-
     if ( $rs->next ) {
         return 1;
     }
-
+    DeleteExpiredPasswordRecovery($borrower_number);
     return 0;
 }
 
@@ -181,6 +180,24 @@ sub CompletePasswordRecovery {
     return $entry->delete();
 }
 
+=head2 DeleteExpiredPasswordRecovery
+
+    $bool = DeleteExpiredPasswordRecovery($borrowernumber)
+
+    Deletes an expired password recovery entry.
+
+=cut
+
+sub DeleteExpiredPasswordRecovery {
+    my $borrower_number = shift;
+    my $model =
+      Koha::Database->new->schema->resultset('BorrowerPasswordRecovery');
+    my $entry = $model->search(
+        { -or => [ borrowernumber => $borrower_number ] } );
+    return $entry->delete();
+}
+
+
 END { }    # module clean-up code here (global destructor)
 
 1;
diff --git a/t/db_dependent/Passwordrecovery.t b/t/db_dependent/Passwordrecovery.t
index 92cdf4b..9067e38 100755
--- a/t/db_dependent/Passwordrecovery.t
+++ b/t/db_dependent/Passwordrecovery.t
@@ -22,7 +22,7 @@ use C4::Letters;
 use Koha::Database;
 use Koha::Patrons;
 
-use Test::More tests => 16;
+use Test::More tests => 18;
 
 use_ok('Koha::Patron::Password::Recovery');
 
@@ -38,12 +38,16 @@ $dbh->{RaiseError} = 1;
 
 my $borrowernumber1 = '2000000000';
 my $borrowernumber2 = '2000000001';
+my $borrowernumber3 = '2000000002';
 my $userid1 = "I83MFItzRpGPxD3vW0";
 my $userid2 = "Gh5t43980hfSAOcvne";
+my $userid3 = "adsfada80hfSAOcvne";
 my $email1  = $userid1 . '@koha-community.org';
 my $email2  = $userid2 . '@koha-community.org';
+my $email3  = $userid3 . '@koha-community.org';
 my $uuid1   = "ABCD1234";
 my $uuid2   = "WXYZ0987";
+my $uuid3   = "LMNO4561";
 
 my $categorycode = 'S'; #  staff
 my $branch       = $schema->resultset('Branch')->first(); #  legit branch from your db
@@ -74,6 +78,18 @@ $schema->resultset('Borrower')->create(
         branchcode      => $branch,
     }
 );
+$schema->resultset('Borrower')->create(
+    {
+        borrowernumber => $borrowernumber3,
+        surname        => '',
+        address        => '',
+        city           => '',
+        userid         => $userid3,
+        email          => $email3,
+        categorycode   => $categorycode,
+        branchcode     => $branch,
+    }
+);
 
 $schema->resultset('BorrowerPasswordRecovery')->create(
     {
@@ -89,6 +105,14 @@ $schema->resultset('BorrowerPasswordRecovery')->create(
         valid_until    => DateTime->now( time_zone => C4::Context->tz() )->subtract( days => 2 )->datetime()
     }
 );
+$schema->resultset('BorrowerPasswordRecovery')->create(
+    {
+        borrowernumber => $borrowernumber3,
+        uuid           => $uuid3,
+        valid_until    => DateTime->now( time_zone => C4::Context->tz() )->subtract( days => 3 )->datetime()
+    }
+);
+
 
 can_ok( "Koha::Patron::Password::Recovery", qw(ValidateBorrowernumber GetValidLinkInfo SendPasswordRecoveryEmail CompletePasswordRecovery) );
 
@@ -117,7 +141,7 @@ ok( ! defined($bnum3), "[GetValidLinkInfo] Invalid UUID returns no borrowernumbe
 # Koha::Patron::Password::Recovery::CompletePasswordRecovery #
 ##############################################################
 
-ok( Koha::Patron::Password::Recovery::CompletePasswordRecovery($uuid1) == 2, "[CompletePasswordRecovery] Completing a password recovery deletes the entry and expired entries" );
+ok( Koha::Patron::Password::Recovery::CompletePasswordRecovery($uuid1) == 2, "[CompletePasswordRecovery] Completing a password recovery deletes the used entry" );
 
 $schema->resultset('BorrowerPasswordRecovery')->create(
     {
@@ -130,6 +154,21 @@ $schema->resultset('BorrowerPasswordRecovery')->create(
 ok( Koha::Patron::Password::Recovery::CompletePasswordRecovery($uuid2) == 1, "[CompletePasswordRecovery] An expired or invalid UUID purges expired entries" );
 ok( Koha::Patron::Password::Recovery::CompletePasswordRecovery($uuid2) == 0, "[CompletePasswordRecovery] Returns 0 on a clean table" );
 
+###################################################################
+# Koha::Patron::Password::Recovery::DeleteExpiredPasswordRecovery #
+###################################################################
+
+$schema->resultset('BorrowerPasswordRecovery')->create(
+    {
+        borrowernumber => $borrowernumber3,
+        uuid           => $uuid3,
+        valid_until    => DateTime->now( time_zone => C4::Context->tz() )->subtract( days => 3 )->datetime()
+    }
+);
+
+ok( Koha::Patron::Password::Recovery::DeleteExpiredPasswordRecovery($borrowernumber3) == 1, "[DeleteExpiredPasswordRecovery] we can delete the unused entry" );
+ok( Koha::Patron::Password::Recovery::DeleteExpiredPasswordRecovery($borrowernumber3) == 0, "[DeleteExpiredPasswordRecovery] Returns 0 on a clean table" );
+
 ###############################################################
 # Koha::Patron::Password::Recovery::SendPasswordRecoveryEmail #
 ###############################################################
-- 
2.1.4