From 9289743a74491deb7f4b960340eda2ff14ede493 Mon Sep 17 00:00:00 2001 From: Galen Charlton Date: Fri, 6 May 2016 21:40:12 -0400 Subject: [PATCH] LP#1579225: fix handling of passwords in patron registration This patch improves how the new password hashing is invoked by open-ils.actor.patron.update; in particular, it fixes a problem whereby newly registered patrons could not log in. It also fixes other issues: - actor.usr.passwd would be set to an MD5 of the password for new users, vitiating the strong hashes in actor.passwd - certain types of updates via patron registration, such as adding or deleting addresses, could result in the patron's password getting doubly-hashed, thereby locking them out of their account. To test ------- [1] Register a new patron; verify that they can log in. [2] Edit an existing patron and change their password; verify that they can log in. [3] Edit an existing patron but do NOT change their password; verify that they can still log in. [4] Inspect the actor.usr rows for these patrons and verify that actor.usr.passwd is set to the value MD5(''), not the MD5 of their password. Signed-off-by: Galen Charlton Signed-off-by: Dan Wells Signed-off-by: Mike Rylander Signed-off-by: Kathy Lussier Signed-off-by: Galen Charlton --- .../perlmods/lib/OpenILS/Application/Actor.pm | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/Open-ILS/src/perlmods/lib/OpenILS/Application/Actor.pm b/Open-ILS/src/perlmods/lib/OpenILS/Application/Actor.pm index 1e0593dbf3..309dd3d9e0 100644 --- a/Open-ILS/src/perlmods/lib/OpenILS/Application/Actor.pm +++ b/Open-ILS/src/perlmods/lib/OpenILS/Application/Actor.pm @@ -428,6 +428,13 @@ sub update_patron { $barred_hook = $U->is_true($new_patron->barred) ? 'au.barred' : 'au.unbarred'; } + + # update the password by itself to avoid the password protection magic + if ($patron->passwd && $patron->passwd ne $old_patron->passwd) { + modify_migrated_user_password($e, $patron->id, $patron->passwd); + $new_patron->passwd(''); # subsequent update will set + # actor.usr.passwd to MD5('') + } } ( $new_patron, $evt ) = _add_update_addresses($e, $patron, $new_patron); @@ -580,7 +587,12 @@ sub _add_patron { $logger->info("Creating new user in the DB with username: ".$patron->usrname()); + # do a dance to get the password hashed securely + my $saved_password = $patron->passwd; + $patron->passwd(''); $e->create_actor_user($patron) or return $e->die_event; + modify_migrated_user_password($e, $patron->id, $saved_password); + my $id = $patron->id; # added by CStoreEditor $logger->info("Successfully created new user [$id] in DB"); @@ -651,12 +663,6 @@ sub _update_patron { unless $e->allowed('UPDATE_USER', $patron->home_ou); } - # update the password by itself to avoid the password protection magic - if( $patron->passwd ) { - modify_migrated_user_password($e, $patron->id, $patron->passwd); - $patron->clear_passwd; - } - if(!$patron->ident_type) { $patron->clear_ident_type; $patron->clear_ident_value; -- 2.43.2