From 76d1509838cb14dbdcaaa8ec495ebe72fb5db34e Mon Sep 17 00:00:00 2001 From: Charles Farmer Date: Wed, 30 Sep 2015 10:03:05 -0400 Subject: [PATCH] Bug 8753 - Smartmatch substitute, Math::Random::Secure, Perltidy, Passwordrecovery.t This is a collection of changes taken from different comments (but mostly comment 21 and comment 122). Passes qa and prove, on my machine at least. There's also a new test file, Passwordrecovery.t, which covers every method of C4::Passwordrecovery. To test: All normal checks plus : 1/ Receive the email 2/ Click on the link 3/ Change the pwd 4/ Click again on the link 5/ You should immediately get an error message Problems with Math/Random/Secure.pm, is solved in following patch, signing off Signed-off-by: Marc Veron Signed-off-by: Marcel de Rooy --- C4/Passwordrecovery.pm | 111 +++++++------ .../Schema/Result/BorrowerPasswordRecovery.pm | 2 + opac/opac-password-recovery.pl | 84 +++++----- t/db_dependent/Passwordrecovery.t | 157 ++++++++++++++++++ 4 files changed, 267 insertions(+), 87 deletions(-) create mode 100755 t/db_dependent/Passwordrecovery.t diff --git a/C4/Passwordrecovery.pm b/C4/Passwordrecovery.pm index 3d2519e22f..8090eeb190 100644 --- a/C4/Passwordrecovery.pm +++ b/C4/Passwordrecovery.pm @@ -19,6 +19,7 @@ package C4::Passwordrecovery; use Modern::Perl; use C4::Context; +use Math::Random::Secure; use vars qw($VERSION @ISA @EXPORT); @@ -26,12 +27,12 @@ BEGIN { # set the version for version checking $VERSION = 3.07.00.049; require Exporter; - @ISA = qw(Exporter); + @ISA = qw(Exporter); push @EXPORT, qw( - &ValidateBorrowernumber - &SendPasswordRecoveryEmail - &GetValidLinkInfo - &CompletePasswordRecovery + &ValidateBorrowernumber + &SendPasswordRecoveryEmail + &GetValidLinkInfo + &CompletePasswordRecovery ); } @@ -60,14 +61,14 @@ sub ValidateBorrowernumber { my $schema = Koha::Database->new->schema; my $rs = $schema->resultset('BorrowerPasswordRecovery')->search( - { - borrowernumber => $borrower_number, - valid_until => \'> NOW()' - }, { - columns => 'borrowernumber' - }); - - if ($rs->next){ + { + borrowernumber => $borrower_number, + valid_until => \'> NOW()' + }, + { columns => 'borrowernumber' } + ); + + if ( $rs->next ) { return 1; } @@ -82,8 +83,8 @@ sub ValidateBorrowernumber { sub GetValidLinkInfo { my ($uniqueKey) = @_; - my $dbh = C4::Context->dbh; - my $query = ' + my $dbh = C4::Context->dbh; + my $query = ' SELECT borrower_password_recovery.borrowernumber, userid FROM borrower_password_recovery, borrowers WHERE borrowers.borrowernumber = borrower_password_recovery.borrowernumber @@ -97,59 +98,67 @@ sub GetValidLinkInfo { =head2 SendPasswordRecoveryEmail - It creates an email using the templates and send it to the user, using the specified email + It creates an email using the templates and sends it to the user, using the specified email =cut sub SendPasswordRecoveryEmail { - my $borrower = shift; # Koha::Borrower - my $userEmail = shift; #to_address (the one specified in the request) - my $update = shift; + my $borrower = shift; # Koha::Borrower + my $userEmail = shift; #to_address (the one specified in the request) + my $update = shift; my $schema = Koha::Database->new->schema; # generate UUID - my @chars = ("A".."Z", "a".."z", "0".."9"); + my @chars = ( "A" .. "Z", "a" .. "z", "0" .. "9" ); my $uuid_str; - $uuid_str .= $chars[rand @chars] for 1..32; + $uuid_str .= $chars[ rand @chars ] for 1 .. 32; # insert into database - my $expirydate = DateTime->now(time_zone => C4::Context->tz())->add( days => 2 ); - if($update){ - my $rs = $schema->resultset('BorrowerPasswordRecovery')->search( - { - borrowernumber => $borrower->borrowernumber, - }); - $rs->update({uuid => $uuid_str, valid_until => $expirydate->datetime()}); - } else { - my $rs = $schema->resultset('BorrowerPasswordRecovery')->create({ - borrowernumber=>$borrower->borrowernumber, - uuid => $uuid_str, - valid_until=> $expirydate->datetime() - }); + my $expirydate = + DateTime->now( time_zone => C4::Context->tz() )->add( days => 2 ); + if ($update) { + my $rs = + $schema->resultset('BorrowerPasswordRecovery') + ->search( { borrowernumber => $borrower->borrowernumber, } ); + $rs->update( + { uuid => $uuid_str, valid_until => $expirydate->datetime() } ); + } + else { + my $rs = $schema->resultset('BorrowerPasswordRecovery')->create( + { + borrowernumber => $borrower->borrowernumber, + uuid => $uuid_str, + valid_until => $expirydate->datetime() + } + ); } # create link - my $uuidLink = C4::Context->preference( 'OPACBaseURL' ) . "/cgi-bin/koha/opac-password-recovery.pl?uniqueKey=$uuid_str"; + my $uuidLink = C4::Context->preference('OPACBaseURL') + . "/cgi-bin/koha/opac-password-recovery.pl?uniqueKey=$uuid_str"; # prepare the email - my $letter = C4::Letters::GetPreparedLetter ( - module => 'members', + my $letter = C4::Letters::GetPreparedLetter( + module => 'members', letter_code => 'PASSWORD_RESET', - branchcode => $borrower->branchcode, - substitute => {passwordreseturl => $uuidLink, user => $borrower->userid }, + branchcode => $borrower->branchcode, + substitute => + { passwordreseturl => $uuidLink, user => $borrower->userid }, ); # define to/from emails - my $kohaEmail = C4::Context->preference( 'KohaAdminEmailAddress' ); # from + my $kohaEmail = C4::Context->preference('KohaAdminEmailAddress'); # from - C4::Letters::EnqueueLetter( { - letter => $letter, - borrowernumber => $borrower->borrowernumber, - to_address => $userEmail, - from_address => $kohaEmail, - message_transport_type => 'email', - } ); + C4::Letters::EnqueueLetter( + { + letter => $letter, + borrowernumber => $borrower->borrowernumber, + to_address => $userEmail, + from_address => $kohaEmail, + message_transport_type => 'email', + } + ); return 1; } @@ -162,10 +171,12 @@ sub SendPasswordRecoveryEmail { =cut -sub CompletePasswordRecovery{ +sub CompletePasswordRecovery { my $uniqueKey = shift; - my $model = Koha::Database->new->schema->resultset('BorrowerPasswordRecovery'); - my $entry = $model->search({-or => [uuid => $uniqueKey, valid_until => \'< NOW()']}); + my $model = + Koha::Database->new->schema->resultset('BorrowerPasswordRecovery'); + my $entry = $model->search( + { -or => [ uuid => $uniqueKey, valid_until => \'< NOW()' ] } ); return $entry->delete(); } diff --git a/Koha/Schema/Result/BorrowerPasswordRecovery.pm b/Koha/Schema/Result/BorrowerPasswordRecovery.pm index 5b41fbf6e6..85d63434f3 100644 --- a/Koha/Schema/Result/BorrowerPasswordRecovery.pm +++ b/Koha/Schema/Result/BorrowerPasswordRecovery.pm @@ -63,4 +63,6 @@ __PACKAGE__->add_columns( # You can replace this text with custom code or comments, and it will be preserved on regeneration +__PACKAGE__->set_primary_key("borrowernumber"); + 1; diff --git a/opac/opac-password-recovery.pl b/opac/opac-password-recovery.pl index ba7bcb6aa3..1cef3edf8d 100755 --- a/opac/opac-password-recovery.pl +++ b/opac/opac-password-recovery.pl @@ -9,7 +9,8 @@ use C4::Koha; use C4::Members qw(changepassword); use C4::Output; use C4::Context; -use C4::Passwordrecovery qw(SendPasswordRecoveryEmail ValidateBorrowernumber GetValidLinkInfo CompletePasswordRecovery); +use C4::Passwordrecovery + qw(SendPasswordRecoveryEmail ValidateBorrowernumber GetValidLinkInfo CompletePasswordRecovery); use Koha::AuthUtils qw(hash_password); use Koha::Borrowers; my $query = new CGI; @@ -50,48 +51,55 @@ my $errPassNotMatch; my $errPassTooShort; if ( $query->param('sendEmail') || $query->param('resendEmail') ) { + #try with the main email - $email ||= ''; # avoid undef + $email ||= ''; # avoid undef my $borrower; my $search_results; + # Find the borrower by his userid or email - if( $username ){ - $search_results = [ Koha::Borrowers->search({ userid => $username }) ]; + if ($username) { + $search_results = [ Koha::Borrowers->search( { userid => $username } ) ]; } - elsif ( $email ){ - $search_results = [ Koha::Borrowers->search({-or => {email => $email, emailpro=> $email, B_email=>$email }}) ]; + elsif ($email) { + $search_results = [ Koha::Borrowers->search( { -or => { email => $email, emailpro => $email, B_email => $email } } ) ]; } - if ( not $search_results ){ - $hasError = 1; - $errNoBorrowerFound = 1; + if ( not $search_results ) { + $hasError = 1; + $errNoBorrowerFound = 1; } - elsif(scalar @$search_results > 1){ # Many matching borrowers - $hasError = 1; - $errTooManyEmailFound = 1; + elsif ( scalar @$search_results > 1 ) { # Many matching borrowers + $hasError = 1; + $errTooManyEmailFound = 1; } - elsif( $borrower = shift @$search_results ){ # One matching borrower + elsif ( $borrower = shift @$search_results ) { # One matching borrower $username ||= $borrower->userid; my @emails = ( $borrower->email, $borrower->emailpro, $borrower->B_email ); + # Is the given email one of the borrower's ? - if( $email && !($email ~~ @emails) ){ - $hasError = 1; - $errBadEmail = 1; + if ( $email && !( grep { $_ eq $email } @emails ) ) { + $hasError = 1; + $errBadEmail = 1; } - # If we dont have an email yet. Get one of the borrower's email or raise an error. - # FIXME: That ugly shift-grep contraption. - # $email = shift [ grep { length() } @emails ] - # It's supposed to get a non-empty string from the @emails array. There's surely a simpler way - elsif( !$email && !($email = shift [ grep { length() } @emails ]) ){ - $hasError = 1; - $errNoBorrowerEmail = 1; + +# If we dont have an email yet. Get one of the borrower's email or raise an error. +# FIXME: That ugly shift-grep contraption. +# $email = shift [ grep { length() } @emails ] +# It's supposed to get a non-empty string from the @emails array. There's surely a simpler way + elsif ( !$email && !( $email = shift [ grep { length() } @emails ] ) ) { + $hasError = 1; + $errNoBorrowerEmail = 1; } - # Check if a password reset already issued for this borrower AND we are not asking for a new email - elsif( ValidateBorrowernumber( $borrower->borrowernumber ) && !$query->param('resendEmail') ){ + +# Check if a password reset already issued for this borrower AND we are not asking for a new email + elsif ( ValidateBorrowernumber( $borrower->borrowernumber ) + && !$query->param('resendEmail') ) + { $hasError = 1; $errAlreadyStartRecovery = 1; } } - else{ # 0 matching borrower + else { # 0 matching borrower $hasError = 1; $errNoBorrowerFound = 1; } @@ -108,13 +116,13 @@ if ( $query->param('sendEmail') || $query->param('resendEmail') ) { username => $username ); } - elsif ( SendPasswordRecoveryEmail( $borrower, $email, $query->param('resendEmail') ) ) {#generate uuid and send recovery email + elsif ( SendPasswordRecoveryEmail( $borrower, $email, $query->param('resendEmail') ) ) { # generate uuid and send recovery email $template->param( mail_sent => 1, email => $email ); } - else {# if it doesn't work.... + else { # if it doesn't work.... $template->param( password_recovery => 1, sendmailError => 1 @@ -123,11 +131,12 @@ if ( $query->param('sendEmail') || $query->param('resendEmail') ) { } elsif ( $query->param('passwordReset') ) { ( $borrower_number, $username ) = GetValidLinkInfo($uniqueKey); + #validate password length & match if ( ($borrower_number) && ( $password eq $repeatPassword ) && ( length($password) >= $minPassLength ) ) - { #apply changes + { #apply changes changepassword( $username, $borrower_number, hash_password($password) ); CompletePasswordRecovery($uniqueKey); $template->param( @@ -135,14 +144,14 @@ elsif ( $query->param('passwordReset') ) { username => $username ); } - else { #errors - if ( !$borrower_number ) { #parameters not valid + else { #errors + if ( !$borrower_number ) { #parameters not valid $errLinkNotValid = 1; } - elsif ( $password ne $repeatPassword ) { #passwords does not match + elsif ( $password ne $repeatPassword ) { #passwords does not match $errPassNotMatch = 1; } - elsif ( length($password) < $minPassLength ) { #password too short + elsif ( length($password) < $minPassLength ) { #password too short $errPassTooShort = 1; } $template->param( @@ -157,8 +166,8 @@ elsif ( $query->param('passwordReset') ) { ); } } -elsif ($uniqueKey) { #reset password form - #check if the link is valid +elsif ($uniqueKey) { #reset password form + #check if the link is valid ( $borrower_number, $username ) = GetValidLinkInfo($uniqueKey); if ( !$borrower_number ) { @@ -171,10 +180,11 @@ elsif ($uniqueKey) { #reset password form email => $email, uniqueKey => $uniqueKey, username => $username, - errLinkNotValid => $errLinkNotValid + errLinkNotValid => $errLinkNotValid, + hasError => ( $errLinkNotValid ? 1 : 0 ), ); } -else { #password recovery form (to send email) +else { #password recovery form (to send email) $template->param( password_recovery => 1 ); } diff --git a/t/db_dependent/Passwordrecovery.t b/t/db_dependent/Passwordrecovery.t new file mode 100755 index 0000000000..e0ec8ecbaf --- /dev/null +++ b/t/db_dependent/Passwordrecovery.t @@ -0,0 +1,157 @@ +#!/usr/bin/perl + +# This file is part of Koha. +# +# Koha is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# Koha is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Koha; if not, see . + +use Modern::Perl; + +use C4::Context; +use C4::Letters; +use Koha::Database; +use Koha::Borrowers; + +use Test::More tests => 16; + +use_ok('C4::Passwordrecovery'); + +my $schema = Koha::Database->new()->schema(); +$schema->storage->txn_begin(); + +my $dbh = C4::Context->dbh; +$dbh->{RaiseError} = 1; + +# +# Start with fresh data +# + +my $borrowernumber1 = '2000000000'; +my $borrowernumber2 = '2000000001'; +my $userid1 = "I83MFItzRpGPxD3vW0"; +my $userid2 = "Gh5t43980hfSAOcvne"; +my $email1 = $userid1 . '@koha-community.org'; +my $email2 = $userid2 . '@koha-community.org'; +my $uuid1 = "ABCD1234"; +my $uuid2 = "WXYZ0987"; + +my $categorycode = 'S'; # staff +my $branch = $schema->resultset('Branch')->first(); # legit branch from your db + +$schema->resultset('BorrowerPasswordRecovery')->delete_all(); + +$schema->resultset('Borrower')->create( + { + borrowernumber => $borrowernumber1, + surname => '', + address => '', + city => '', + userid => $userid1, + email => $email1, + categorycode => $categorycode, + branchcode => $branch, + } +); +$schema->resultset('Borrower')->create( + { + borrowernumber => $borrowernumber2, + surname => '', + address => '', + city => '', + userid => $userid2, + email => $email2, + categorycode => $categorycode, + branchcode => $branch, + } +); + +$schema->resultset('BorrowerPasswordRecovery')->create( + { + borrowernumber => $borrowernumber1, + uuid => $uuid1, + valid_until => DateTime->now( time_zone => C4::Context->tz() )->add( days => 2 )->datetime() + } +); +$schema->resultset('BorrowerPasswordRecovery')->create( + { + borrowernumber => $borrowernumber2, + uuid => $uuid2, + valid_until => DateTime->now( time_zone => C4::Context->tz() )->subtract( days => 2 )->datetime() + } +); + +can_ok( "C4::Passwordrecovery", qw(ValidateBorrowernumber GetValidLinkInfo SendPasswordRecoveryEmail CompletePasswordRecovery) ); + +################################################ +# C4::Passwordrecovery::ValidateBorrowernumber # +################################################ + +ok( C4::Passwordrecovery::ValidateBorrowernumber($borrowernumber1), "[ValidateBorrowernumber] Borrower has a password recovery entry" ); +ok( ! C4::Passwordrecovery::ValidateBorrowernumber($borrowernumber2), "[ValidateBorrowernumber] Borrower's number is not found; password recovery entry is expired" ); +ok( ! C4::Passwordrecovery::ValidateBorrowernumber(9999), "[ValidateBorrowernumber] Borrower has no password recovery entry" ); + +########################################## +# C4::Passwordrecovery::GetValidLinkInfo # +########################################## + +my ($bnum1, $uname1) = C4::Passwordrecovery::GetValidLinkInfo($uuid1); +my ($bnum2, $uname2) = C4::Passwordrecovery::GetValidLinkInfo($uuid2); +my ($bnum3, $uname3) = C4::Passwordrecovery::GetValidLinkInfo("THISISANINVALIDUUID"); + +is( $bnum1, $borrowernumber1, "[GetValidLinkInfo] Borrower has a valid link" ); +is( $uname1, $userid1, "[GetValidLinkInfo] Borrower's username is fetched when a valid link is found" ); +ok( ! defined($bnum2), "[GetValidLinkInfo] Borrower's link is no longer valid; entry is expired" ); +ok( ! defined($bnum3), "[GetValidLinkInfo] Invalid UUID returns no borrowernumber" ); + +################################################## +# C4::Passwordrecovery::CompletePasswordRecovery # +################################################## + +ok( C4::Passwordrecovery::CompletePasswordRecovery($uuid1) == 2, "[CompletePasswordRecovery] Completing a password recovery deletes the entry and expired entries" ); + +$schema->resultset('BorrowerPasswordRecovery')->create( + { + borrowernumber => $borrowernumber2, + uuid => $uuid2, + valid_until => DateTime->now( time_zone => C4::Context->tz() )->subtract( days => 2 )->datetime() + } +); + +ok( C4::Passwordrecovery::CompletePasswordRecovery($uuid2) == 1, "[CompletePasswordRecovery] An expired or invalid UUID purges expired entries" ); +ok( C4::Passwordrecovery::CompletePasswordRecovery($uuid2) == 0, "[CompletePasswordRecovery] Returns 0 on a clean table" ); + +################################################### +# C4::Passwordrecovery::SendPasswordRecoveryEmail # +################################################### + +my $borrower = shift [ Koha::Borrowers->search( { userid => $userid1 } ) ]; +ok( C4::Passwordrecovery::SendPasswordRecoveryEmail($borrower, $email1, 0) == 1, "[SendPasswordRecoveryEmail] Returns 1 on success" ); +my $letters = C4::Letters::GetQueuedMessages( { borrowernumber => $borrowernumber1, limit => 99 } ); +ok( scalar @$letters == 1, "[SendPasswordRecoveryEmail] There is a letter in the queue for our borrower"); + +my $bpr = $schema->resultset('BorrowerPasswordRecovery')->search( { borrowernumber => $borrowernumber1 } ); +my $tempuuid1 = $bpr->next->uuid; + +C4::Passwordrecovery::SendPasswordRecoveryEmail($borrower, $email1, 1); + +$bpr = $schema->resultset('BorrowerPasswordRecovery')->search( { borrowernumber => $borrowernumber1 } ); +my $tempuuid2 = $bpr->next->uuid; + +$letters = C4::Letters::GetQueuedMessages( { borrowernumber => $borrowernumber1, limit => 99 } ); + +ok( $tempuuid1 ne $tempuuid2, "[SendPasswordRecoveryEmail] UPDATE == ON changes uuid in the database and updates the expirydate"); +ok( scalar @$letters == 2, "[SendPasswordRecoveryEmail] UPDATE == ON sends a new letter with updated uuid"); + +$schema->storage->txn_rollback(); + +1; -- 2.39.5