Bugzilla – Attachment 77201 Details for
Bug 21087
Patron's password is hashed twice when the object is saved
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 21087: Hash passwords in ->update_password
Bug-21087-Hash-passwords-in--updatepassword.patch (text/plain), 12.54 KB, created by
Tomás Cohen Arazi (tcohen)
on 2018-07-23 19:06:19 UTC
(
hide
)
Description:
Bug 21087: Hash passwords in ->update_password
Filename:
MIME Type:
Creator:
Tomás Cohen Arazi (tcohen)
Created:
2018-07-23 19:06:19 UTC
Size:
12.54 KB
patch
obsolete
>From 009c001b07687a94c650fcfd5433793f036bdb0a Mon Sep 17 00:00:00 2001 >From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >Date: Wed, 18 Jul 2018 19:54:34 -0300 >Subject: [PATCH] Bug 21087: Hash passwords in ->update_password > >Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> >--- > 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.18.0
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 21087
:
77095
|
77096
|
77200
|
77201
|
77254
|
77255
|
77273
|
77284
|
77285
|
77286
|
77287
|
77288
|
77327