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 <veron@veron.ch>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
This commit is contained in:
Charles Farmer 2015-09-30 10:03:05 -04:00 committed by Brendan A Gallagher
parent 70898717c1
commit 76d1509838
4 changed files with 266 additions and 86 deletions

View File

@ -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'
});
{
borrowernumber => $borrower_number,
valid_until => \'> NOW()'
},
{ columns => 'borrowernumber' }
);
if ($rs->next){
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();
}

View File

@ -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;

View File

@ -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 );
}

157
t/db_dependent/Passwordrecovery.t Executable file
View File

@ -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 <http://www.gnu.org/licenses>.
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;