From 60bfc9c69ceb688c4029750302518c46ccbd167b Mon Sep 17 00:00:00 2001 From: Liz Rea 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 - is resolved by patch Signed-off-by: Marc VĂ©ron Signed-off-by: Jonathan Druart --- Koha/Patron/Password/Recovery.pm | 21 +++++++++++++++++-- opac/opac-password-recovery.pl | 7 ++++++- t/db_dependent/Passwordrecovery.t | 43 +++++++++++++++++++++++++++++++++++++-- 3 files changed, 66 insertions(+), 5 deletions(-) diff --git a/Koha/Patron/Password/Recovery.pm b/Koha/Patron/Password/Recovery.pm index 04491e6..7c35c5d 100644 --- 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 --- 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 --- 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.9.3