Bug 18025 - Expired password recovery links cause sql crash

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 - is resolved by patch
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

diff --git a/Koha/Patron/Password/Recovery.pm b/Koha/Patron/Password/Recovery.pm
index 04491e6..7c35c5d 100644 (file)
--- a/Koha/Patron/Password/Recovery.pm
+++ b/Koha/Patron/Password/Recovery.pm
@@ -32,6 +32,7 @@ BEGIN {
       &SendPasswordRecoveryEmail
       &GetValidLinkInfo
       &CompletePasswordRecovery
+      &DeleteExpiredPasswordRecovery
     );
 }
 
@@ -66,11 +67,9 @@ sub ValidateBorrowernumber {
         },
         { columns => 'borrowernumber' }
     );
-
     if ( $rs->next ) {
         return 1;
     }
-
     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(
+        { borrowernumber => $borrower_number } );
+    return $entry->delete();
+}
+
+
 END { }    # module clean-up code here (global destructor)
 
 1;

diff --git a/opac/opac-password-recovery.pl b/opac/opac-password-recovery.pl
index 9560c38..2099caf 100755 (executable)
--- a/opac/opac-password-recovery.pl
+++ b/opac/opac-password-recovery.pl
@@ -8,7 +8,7 @@ use C4::Koha;
 use C4::Output;
 use C4::Context;
 use Koha::Patron::Password::Recovery
-  qw(SendPasswordRecoveryEmail ValidateBorrowernumber GetValidLinkInfo CompletePasswordRecovery);
+  qw(SendPasswordRecoveryEmail ValidateBorrowernumber GetValidLinkInfo CompletePasswordRecovery DeleteExpiredPasswordRecovery);
 use Koha::Patrons;
 use Koha::AuthUtils qw(hash_password);
 use Koha::Patrons;
@@ -96,6 +96,11 @@ if ( $query->param('sendEmail') || $query->param('resendEmail') ) {
             $hasError                = 1;
             $errAlreadyStartRecovery = 1;
         }
+        elsif ( !ValidateBorrowernumber($borrower->borrowernumber)
+            && !$query->param('resendEmail') )
+        {
+            DeleteExpiredPasswordRecovery($borrower->borrowernumber);
+        }
     }
     else {    # 0 matching borrower
         $hasError           = 1;

diff --git a/t/db_dependent/Passwordrecovery.t b/t/db_dependent/Passwordrecovery.t
index 92cdf4b..9067e38 100755 (executable)
--- 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 #
 ###############################################################