Bugzilla – Attachment 43017 Details for
Bug 8753
Add forgot password link to OPAC
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 8753 - Smartmatch substitute, Math::Random::Secure, Perltidy, Passwordrecovery.t
Bug-8753---Smartmatch-substitute-MathRandomSecure-.patch (text/plain), 20.80 KB, created by
Charles Farmer
on 2015-10-01 15:58:11 UTC
(
hide
)
Description:
Bug 8753 - Smartmatch substitute, Math::Random::Secure, Perltidy, Passwordrecovery.t
Filename:
MIME Type:
Creator:
Charles Farmer
Created:
2015-10-01 15:58:11 UTC
Size:
20.80 KB
patch
obsolete
>From 4497cbed8b64c1130776b64f1005ccf2de2c94f8 Mon Sep 17 00:00:00 2001 >From: Charles Farmer <charles.farmer@inlibro.com> >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 >--- > C4/Passwordrecovery.pm | 111 +++++++++-------- > Koha/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 3d2519e..8090eeb 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 5b41fbf..85d6343 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 ba7bcb6..1cef3ed 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 0000000..e0ec8ec >--- /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 <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; >-- >1.9.1
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=43017">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 8753
:
21502
|
21512
|
21521
|
21849
|
22175
|
22430
|
24586
|
24780
|
24781
|
24846
|
24853
|
24854
|
24855
|
24862
|
24906
|
24907
|
24908
|
25038
|
25039
|
25051
|
29111
|
33175
|
33176
|
34472
|
36819
|
37492
|
39049
|
39144
|
39165
|
39166
|
39167
|
39168
|
39827
|
41181
|
41192
|
41193
|
41455
|
41686
|
41687
|
41688
|
41706
|
43017
|
43183
|
43576
|
44305
|
44306
|
44307
|
44308
|
44309
|
44310
|
44311
|
44323
|
44324
|
44325
|
44326
|
44327
|
44329
|
44330
|
45209
|
45210
|
45211
|
45212
|
45213
|
45214
|
45215
|
45223
|
45224
|
45225
|
45226
|
45227
|
45228
|
45229
|
45230
|
46379
|
47164
|
47165
|
47166
|
47167
|
47168
|
47169
|
47170
|
47171
|
47357
|
47366