From afc008b2fa8cf5b409407b4d46585800467b39d3 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Mon, 4 Jul 2016 14:00:58 +0100 Subject: [PATCH] Bug 16853: Move changepassword to Koha::Patron->update_password MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit This patch moves the code from C4::Members::changepassword to Koha::Patron->update_password Test plan: Change your password at the OPAC and the staff interface This should work as before Signed-off-by: Marc Véron Signed-off-by: Tomas Cohen Arazi I rebased this on top of 16849 because they were conflicting. Tests pass, code looks good (as usual) and I checked both OPAC and staff password change work as expected. Signed-off-by: Kyle M Hall --- C4/Auth_with_ldap.pm | 5 +++-- C4/Members.pm | 29 ----------------------------- Koha/Patron.pm | 20 ++++++++++++++++++++ members/member-password.pl | 2 +- opac/opac-password-recovery.pl | 4 ++-- t/db_dependent/Auth.t | 3 ++- t/db_dependent/Koha/Patrons.t | 31 ++++++++++++++++++++++++++++++- 7 files changed, 58 insertions(+), 36 deletions(-) diff --git a/C4/Auth_with_ldap.pm b/C4/Auth_with_ldap.pm index c50df76601..03e81e1c67 100644 --- a/C4/Auth_with_ldap.pm +++ b/C4/Auth_with_ldap.pm @@ -23,11 +23,12 @@ use Carp; use C4::Debug; use C4::Context; -use C4::Members qw(AddMember changepassword); +use C4::Members qw(AddMember); use C4::Members::Attributes; use C4::Members::AttributeTypes; use C4::Members::Messaging; use C4::Auth qw(checkpw_internal); +use Koha::Patrons; use Koha::AuthUtils qw(hash_password); use List::MoreUtils qw( any ); use Net::LDAP; @@ -334,7 +335,7 @@ sub _do_changepassword { my $digest = hash_password($password); $debug and print STDERR "changing local password for borrowernumber=$borrowerid to '$digest'\n"; - changepassword($userid, $borrowerid, $digest); + Koha::Patrons->find($borrowerid)->update_password( $userid, $digest ); my ($ok, $cardnum) = checkpw_internal(C4::Context->dbh, $userid, $password); return $cardnum if $ok; diff --git a/C4/Members.pm b/C4/Members.pm index d4b09af2ec..cd1e78ec78 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -814,35 +814,6 @@ sub Generate_Userid { return $newuid; } -sub changepassword { - my ( $uid, $member, $digest ) = @_; - my $dbh = C4::Context->dbh; - -#Make sure the userid chosen is unique and not theirs if non-empty. If it is not, -#Then we need to tell the user and have them create a new one. - my $resultcode; - my $sth = - $dbh->prepare( - "SELECT * FROM borrowers WHERE userid=? AND borrowernumber != ?"); - $sth->execute( $uid, $member ); - if ( ( $uid ne '' ) && ( my $row = $sth->fetchrow_hashref ) ) { - $resultcode=0; - } - else { - #Everything is good so we can update the information. - $sth = - $dbh->prepare( - "update borrowers set userid=?, password=? where borrowernumber=?"); - $sth->execute( $uid, $digest, $member ); - $resultcode=1; - } - - logaction("MEMBERS", "CHANGE PASS", $member, "") if C4::Context->preference("BorrowersLog"); - return $resultcode; -} - - - =head2 fixup_cardnumber Warning: The caller is responsible for locking the members table in write diff --git a/Koha/Patron.pm b/Koha/Patron.pm index e65cf90221..42b066b60c 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -23,6 +23,7 @@ use Modern::Perl; use Carp; use C4::Context; +use C4::Log; use Koha::Database; use Koha::DateUtils; use Koha::Issues; @@ -188,6 +189,25 @@ sub is_debarred { return; } +=head2 update_password + +my $updated = $patron->update_password( $userid, $password ); + +Update the userid and the password of a patron. +If the userid already exists, returns and let DBIx::Class warns +This will add an entry to action_logs if BorrowersLog is set. + +=cut + +sub update_password { + my ( $self, $userid, $password ) = @_; + eval { $self->userid($userid)->store; }; + return if $@; # Make sure the userid is not already in used by another patron + $self->password($password)->store; + logaction( "MEMBERS", "CHANGE PASS", $self->borrowernumber, "" ) if C4::Context->preference("BorrowersLog"); + return 1; +} + =head3 type =cut diff --git a/members/member-password.pl b/members/member-password.pl index 85e4b976ec..64d6347d8a 100755 --- a/members/member-password.pl +++ b/members/member-password.pl @@ -66,7 +66,7 @@ if ( $newpassword && !scalar(@errors) ) { my $digest = Koha::AuthUtils::hash_password( $input->param('newpassword') ); my $uid = $input->param('newuserid'); my $dbh = C4::Context->dbh; - if ( changepassword( $uid, $member, $digest ) ) { + if ( Koha::Patrons->find( $member )->update_password($uid, $digest) ) { $template->param( newpassword => $newpassword ); if ( $destination eq 'circ' ) { print $input->redirect("/cgi-bin/koha/circ/circulation.pl?findborrower=$cardnumber"); diff --git a/opac/opac-password-recovery.pl b/opac/opac-password-recovery.pl index 257ca92505..9560c388ce 100755 --- a/opac/opac-password-recovery.pl +++ b/opac/opac-password-recovery.pl @@ -5,11 +5,11 @@ use CGI; use C4::Auth; use C4::Koha; -use C4::Members qw(changepassword); use C4::Output; use C4::Context; use Koha::Patron::Password::Recovery qw(SendPasswordRecoveryEmail ValidateBorrowernumber GetValidLinkInfo CompletePasswordRecovery); +use Koha::Patrons; use Koha::AuthUtils qw(hash_password); use Koha::Patrons; my $query = new CGI; @@ -135,7 +135,7 @@ elsif ( $query->param('passwordReset') ) { && ( $password eq $repeatPassword ) && ( length($password) >= $minPassLength ) ) { #apply changes - changepassword( $username, $borrower_number, hash_password($password) ); + Koha::Patrons->find($borrower_number)->update_password( $username, hash_password($password) ); CompletePasswordRecovery($uniqueKey); $template->param( password_reset_done => 1, diff --git a/t/db_dependent/Auth.t b/t/db_dependent/Auth.t index aa703ec622..233b2beba2 100644 --- a/t/db_dependent/Auth.t +++ b/t/db_dependent/Auth.t @@ -17,6 +17,7 @@ use C4::Auth qw(checkpw); use C4::Members; use Koha::AuthUtils qw/hash_password/; use Koha::Database; +use Koha::Patrons; BEGIN { use_ok('C4::Auth'); @@ -32,7 +33,7 @@ my $hash2 = hash_password('password'); { # tests no_set_userenv parameter my $patron = $builder->build( { source => 'Borrower' } ); - changepassword( $patron->{userid}, $patron->{borrowernumber}, $hash1 ); + Koha::Patrons->find( $patron->{borrowernumber} )->update_password( $patron->{userid}, $hash1 ); my $library = $builder->build( { source => 'Branch', diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index fafd8294c0..980bf849a2 100644 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -19,13 +19,15 @@ use Modern::Perl; -use Test::More tests => 5; +use Test::More tests => 6; +use Test::Warn; use Koha::Patron; use Koha::Patrons; use Koha::Database; use t::lib::TestBuilder; +use t::lib::Mocks; my $schema = Koha::Database->new->schema; $schema->storage->txn_begin; @@ -40,6 +42,7 @@ my $new_patron_1 = Koha::Patron->new( categorycode => $category->{categorycode}, surname => 'surname for patron1', firstname => 'firstname for patron1', + userid => 'a_nonexistent_userid_1', } )->store; my $new_patron_2 = Koha::Patron->new( @@ -48,6 +51,7 @@ my $new_patron_2 = Koha::Patron->new( categorycode => $category->{categorycode}, surname => 'surname for patron2', firstname => 'firstname for patron2', + userid => 'a_nonexistent_userid_2', } )->store; @@ -98,6 +102,31 @@ subtest 'siblings' => sub { $retrieved_guarantee_1->delete; }; +subtest 'update_password' => sub { + plan tests => 7; + + t::lib::Mocks::mock_preference( 'BorrowersLog', 1 ); + my $original_userid = $new_patron_1->userid; + my $original_password = $new_patron_1->password; + warning_like { $retrieved_patron_1->update_password( $new_patron_2->userid, 'another_password' ) } + qr{Duplicate entry}, + 'Koha::Patron->update_password should warn if the userid is already used by another patron'; + is( Koha::Patrons->find( $new_patron_1->borrowernumber )->userid, $original_userid, 'Koha::Patron->update_password should not have updated the userid' ); + is( Koha::Patrons->find( $new_patron_1->borrowernumber )->password, $original_password, 'Koha::Patron->update_password should not have updated the userid' ); + + $retrieved_patron_1->update_password( 'another_nonexistent_userid_1', 'another_password' ); + is( Koha::Patrons->find( $new_patron_1->borrowernumber )->userid, 'another_nonexistent_userid_1', 'Koha::Patron->update_password should have updated the userid' ); + is( Koha::Patrons->find( $new_patron_1->borrowernumber )->password, 'another_password', 'Koha::Patron->update_password should have updated the password' ); + + my $number_of_logs = $schema->resultset('ActionLog')->search( { module => 'MEMBERS', action => 'CHANGE PASS', object => $new_patron_1->borrowernumber } )->count; + is( $number_of_logs, 1, 'With BorrowerLogs, Koha::Patron->update_password should have logged' ); + + t::lib::Mocks::mock_preference( 'BorrowersLog', 0 ); + $retrieved_patron_1->update_password( 'yet_another_nonexistent_userid_1', 'another_password' ); + $number_of_logs = $schema->resultset('ActionLog')->search( { module => 'MEMBERS', action => 'CHANGE PASS', object => $new_patron_1->borrowernumber } )->count; + is( $number_of_logs, 1, 'With BorrowerLogs, Koha::Patron->update_password should not have logged' ); +}; + $retrieved_patron_1->delete; is( Koha::Patrons->search->count, $nb_of_patrons + 1, 'Delete should have deleted the patron' ); -- 2.39.5