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 <gmc@esilibrary.com>
Signed-off-by: Dan Wells <dbw2@calvin.edu>
Signed-off-by: Mike Rylander <mrylander@gmail.com>
Signed-off-by: Kathy Lussier <klussier@masslnc.org>
Signed-off-by: Galen Charlton <gmc@esilibrary.com>

diff --git a/Open-ILS/src/perlmods/lib/OpenILS/Application/Actor.pm b/Open-ILS/src/perlmods/lib/OpenILS/Application/Actor.pm
index 1e0593d..309dd3d 100644 (file)
--- 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;