From 2e6fb40ef8296ba474e8338c4a55cc2804aa73be Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Wed, 18 Jul 2018 19:54:34 -0300 Subject: [PATCH] Bug 21087: Hash passwords in ->update_password Signed-off-by: Tomas Cohen Arazi Signed-off-by: John Doe Signed-off-by: Tomas Cohen Arazi --- C4/Auth_with_ldap.pm | 4 ++-- Koha/Patron.pm | 30 ++++++++++++------------ members/member-password.pl | 4 ++-- members/memberentry.pl | 1 + opac/opac-password-recovery.pl | 3 +-- t/db_dependent/Auth.t | 2 +- t/db_dependent/Koha/Patrons.t | 6 ++--- t/db_dependent/Search/History.t | 4 +--- t/db_dependent/selenium/authentication.t | 6 ++--- t/db_dependent/selenium/regressions.t | 3 +-- 10 files changed, 29 insertions(+), 34 deletions(-) diff --git a/C4/Auth_with_ldap.pm b/C4/Auth_with_ldap.pm index 83b0072eca..53f0eb65ff 100644 --- a/C4/Auth_with_ldap.pm +++ b/C4/Auth_with_ldap.pm @@ -344,10 +344,10 @@ sub _do_changepassword { $sth->execute($borrowerid); return $cardnum; } - my $digest = hash_password($password); + my $digest = hash_password($password); $debug and print STDERR "changing local password for borrowernumber=$borrowerid to '$digest'\n"; - Koha::Patrons->find($borrowerid)->update_password( $userid, $digest ); + Koha::Patrons->find($borrowerid)->update_password( $userid, $password ); my ($ok, $cardnum) = checkpw_internal(C4::Context->dbh, $userid, $password); return $cardnum if $ok; diff --git a/Koha/Patron.pm b/Koha/Patron.pm index 9524874aca..27499fc4c1 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -28,6 +28,7 @@ use Text::Unaccent qw( unac_string ); use C4::Context; use C4::Log; +use Koha::AuthUtils; use Koha::Checkouts; use Koha::Database; use Koha::DateUtils; @@ -267,20 +268,9 @@ sub store { if C4::Context->preference("BorrowersLog"); } else { #ModMember - # test to know if you must update or not the borrower password - if ( defined $self->password ) { - if ( $self->password eq '****' or $self->password eq '' ) { - $self->password(undef); - } else { - if ( C4::Context->preference('NorwegianPatronDBEnable') && C4::Context->preference('NorwegianPatronDBEnable') == 1 ) { - # Update the hashed PIN in borrower_sync.hashed_pin, before Koha hashes it - Koha::NorwegianPatronDB::NLUpdateHashedPIN( $self->borrowernumber, $self->password ); - } - $self->password(Koha::AuthUtils::hash_password($self->password)); - } - } + # We could add a test here to make sure the password is not update (?) - # Come from ModMember, but should not be possible (?) + # Come from ModMember, but should not be possible (?) $self->dateenrolled(undef) unless $self->dateenrolled; $self->dateexpiry(undef) unless $self->dateexpiry; @@ -687,14 +677,24 @@ 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 + + return 0 if $password eq '****' or $password eq ''; # Do we need that? + + if ( C4::Context->preference('NorwegianPatronDBEnable') && C4::Context->preference('NorwegianPatronDBEnable') == 1 ) { + # Update the hashed PIN in borrower_sync.hashed_pin, before Koha hashes it + Koha::NorwegianPatronDB::NLUpdateHashedPIN( $self->borrowernumber, $password ); + } + + my $digest = Koha::AuthUtils::hash_password($password); $self->update( { - password => $password, + password => $digest, login_attempts => 0, } ); + logaction( "MEMBERS", "CHANGE PASS", $self->borrowernumber, "" ) if C4::Context->preference("BorrowersLog"); - return 1; + return $digest; } =head3 renew_account diff --git a/members/member-password.pl b/members/member-password.pl index 115f8045df..490e1df375 100755 --- a/members/member-password.pl +++ b/members/member-password.pl @@ -78,10 +78,10 @@ if ( $newpassword and not @errors) { token => scalar $input->param('csrf_token'), }); - my $digest = Koha::AuthUtils::hash_password( scalar $input->param('newpassword') ); my $uid = $input->param('newuserid') || $bor->{userid}; + my $password = $input->param('newpassword'); my $dbh = C4::Context->dbh; - if ( Koha::Patrons->find( $member )->update_password($uid, $digest) ) { + if ( Koha::Patrons->find( $member )->update_password($uid, $password) ) { $template->param( newpassword => $newpassword ); if ( $destination eq 'circ' ) { print $input->redirect("/cgi-bin/koha/circ/circulation.pl?findborrower=$cardnumber"); diff --git a/members/memberentry.pl b/members/memberentry.pl index e5090b2f6e..ff77fbef0a 100755 --- a/members/memberentry.pl +++ b/members/memberentry.pl @@ -537,6 +537,7 @@ if ((!$nok) and $nodouble and ($op eq 'insert' or $op eq 'save')){ # updating any columns in the borrowers table, # which can happen if we're only editing the # patron attributes or messaging preferences sections + $patron->update_password($newdata{password}); # If != **** or '' # test here? if (C4::Context->preference('ExtendedPatronAttributes') and $input->param('setting_extended_patron_attributes')) { C4::Members::Attributes::SetBorrowerAttributes($borrowernumber, $extended_patron_attributes); } diff --git a/opac/opac-password-recovery.pl b/opac/opac-password-recovery.pl index 456aa97ff5..e553744263 100755 --- a/opac/opac-password-recovery.pl +++ b/opac/opac-password-recovery.pl @@ -10,7 +10,6 @@ use C4::Context; use Koha::Patron::Password::Recovery qw(SendPasswordRecoveryEmail ValidateBorrowernumber GetValidLinkInfo CompletePasswordRecovery DeleteExpiredPasswordRecovery); use Koha::Patrons; -use Koha::AuthUtils qw(hash_password); use Koha::Patrons; my $query = new CGI; use HTML::Entities; @@ -156,7 +155,7 @@ elsif ( $query->param('passwordReset') ) { $error = 'password_too_weak' if $err eq 'too_weak'; $error = 'password_has_whitespaces' if $err eq 'has_whitespaces'; } else { - Koha::Patrons->find($borrower_number)->update_password( $username, hash_password($password) ); + Koha::Patrons->find($borrower_number)->update_password( $username, $password ); CompletePasswordRecovery($uniqueKey); $template->param( password_reset_done => 1, diff --git a/t/db_dependent/Auth.t b/t/db_dependent/Auth.t index 8f980ee261..1610b805ca 100644 --- a/t/db_dependent/Auth.t +++ b/t/db_dependent/Auth.t @@ -120,7 +120,7 @@ my $hash2 = hash_password('password'); { # tests no_set_userenv parameter my $patron = $builder->build( { source => 'Borrower' } ); - Koha::Patrons->find( $patron->{borrowernumber} )->update_password( $patron->{userid}, $hash1 ); + Koha::Patrons->find( $patron->{borrowernumber} )->update_password( $patron->{userid}, 'password' ); my $library = $builder->build( { source => 'Branch', diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index c102aaf4c2..6a966223b5 100644 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -173,9 +173,9 @@ subtest 'update_password' => sub { 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' ); + my $digest = $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' ); + is( Koha::Patrons->find( $new_patron_1->borrowernumber )->password, $digest, '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' ); @@ -1444,7 +1444,7 @@ subtest '->store' => sub { # Test password my $password = 'password'; - $patron_1->password($password)->store; + $patron_1->update_password($patron_1->userid, $password); like( $patron_1->password, qr|^\$2|, 'Password should be hashed using bcrypt (start with $2)' ); my $digest = $patron_1->password; $patron_1->surname('xxx')->store; diff --git a/t/db_dependent/Search/History.t b/t/db_dependent/Search/History.t index 0e74e2d68a..b8c912545e 100644 --- a/t/db_dependent/Search/History.t +++ b/t/db_dependent/Search/History.t @@ -9,7 +9,6 @@ use t::lib::Mocks; use t::lib::TestBuilder; use C4::Auth; -use Koha::AuthUtils qw/hash_password/; use Koha::Database; use Test::More tests => 27; @@ -369,9 +368,8 @@ my $schema = Koha::Database->schema; my $builder = t::lib::TestBuilder->new; # Borrower Creation -my $hash = hash_password('password'); our $patron = $builder->build( { source => 'Borrower' } ); -Koha::Patrons->find( $patron->{borrowernumber} )->update_password( $patron->{userid}, $hash ); +Koha::Patrons->find( $patron->{borrowernumber} )->update_password( $patron->{userid}, 'password' ); my $session = C4::Auth::get_session(""); $session->flush; diff --git a/t/db_dependent/selenium/authentication.t b/t/db_dependent/selenium/authentication.t index 193b7a95b2..0ce4052dc2 100644 --- a/t/db_dependent/selenium/authentication.t +++ b/t/db_dependent/selenium/authentication.t @@ -47,9 +47,8 @@ SKIP: { like( $driver->get_title, qr(Log in to Koha), 'Hitting the main page should redirect to the login form'); my $password = Koha::AuthUtils::generate_password(); - my $digest = Koha::AuthUtils::hash_password( $password ); my $patron = $builder->build_object({ class => 'Koha::Patrons', value => { flags => 0 }}); - $patron->update_password( $patron->userid, $digest ); + $patron->update_password( $patron->userid, $password ); # Patron does not have permission to access staff interface $s->auth( $patron->userid, $password ); @@ -79,9 +78,8 @@ SKIP: { like( $driver->get_title, qr(Koha online catalog), 'Hitting the main page should not redirect to the login form'); my $password = Koha::AuthUtils::generate_password(); - my $digest = Koha::AuthUtils::hash_password( $password ); my $patron = $builder->build_object({ class => 'Koha::Patrons', value => { flags => 0 }}); - $patron->update_password( $patron->userid, $digest ); + $patron->update_password( $patron->userid, $password ); # Using the modal $driver->find_element('//a[@class="login-link loginModal-trigger"]')->click; diff --git a/t/db_dependent/selenium/regressions.t b/t/db_dependent/selenium/regressions.t index cdb49ff51f..5ea6d22bc3 100644 --- a/t/db_dependent/selenium/regressions.t +++ b/t/db_dependent/selenium/regressions.t @@ -41,8 +41,7 @@ subtest 'OPAC - borrowernumber and branchcode as html attributes' => sub { my $patron = $builder->build_object( { class => 'Koha::Patrons', value => { flags => 1 } } ); my $password = Koha::AuthUtils::generate_password(); - my $digest = Koha::AuthUtils::hash_password($password); - $patron->update_password( $patron->userid, $digest ); + $patron->update_password( $patron->userid, $password ); $s->opac_auth( $patron->userid, $password ); my $elt = $driver->find_element('//span[@class="loggedinusername"]'); is( $elt->get_attribute('data-branchcode'), $patron->library->branchcode, -- 2.39.5