Patron registration- unable to log in with password entered during registration

[Christine Burns]

Tested on EG 2.10

Register new patron -> record password entered on registration -> attempt to log in to staff client / OPAC with new account -> "Login failed. Please check your Server Hostname, Username, Password, and your CAPS LOCK key." -> password entered on registration screen does not work

Retrieve patron F1 -> edit -> edit password -> Save - > attempt to log in to staff client / OPAC -> successful

I have tested the following

1) use system generated password (random 4 digit code shown in password field during registration) = Login failed
2) enter specific password in both password fields during registration = Login failed
3) password from phone number (Library setting - Patron password from phone# -> enter daytime phone # -> password field is populated with last 4 digits of daytime phone #) = Login failed

[Jennifer Pringle (jpringle-u)]

We've tested this on our own testing servers running 2.10.1 and 2.10.2 and I also tested using Equinox's community server running 2.10.2 (regular client and web client) and the bug exists on all the servers.

[Jennifer Pringle (jpringle-u)]

I have also tested this using MassLNC's 2.9.2 community server and can confirm that this is not an issue in 2.9.2.

[Galen Charlton (gmc)]

For the sake of anybody else doing some initial investigation, here are comments I made on IRC:

[18:26:04] <gmcharlt> yep, it's the password hashing
[18:26:40] <gmcharlt> during registration, the hashed password is getting set to CRYPT( MD5( pw_salt || MD5(MD5(real_password)) ), pw_salt )
[18:26:56] <gmcharlt> when comparisons are expecting CRYPT( MD5( pw_salt || MD5(real_password) ), pw_salt )
[18:27:08] <gmcharlt> I'll poke more after dinner
[18:30:26] <gmcharlt> ok, seeing more: during the open-ils.actor.patron.update dance, a new actor.usr is created
[18:30:50] <gmcharlt> the password trigger on actor.usr sets au.passwd to md5(real_password)
[18:31:22] <gmcharlt> *then*, the new row gets retrieved and passed on to the rest of the process, including a call to actor.set_passwd
[18:31:32] <gmcharlt> and that's where the extraneous md5 is coming from
[18:31:59] <gmcharlt> more anon

[Galen Charlton (gmc)]

Here's a hacky patch to have the password work for a newly-registered patron:

diff --git a/Open-ILS/src/perlmods/lib/OpenILS/Application/Actor.pm b/Open-ILS/src/perlmods/lib/OpenILS/Application/Actor.pm
index 1e0593d..dfd0562 100644
--- a/Open-ILS/src/perlmods/lib/OpenILS/Application/Actor.pm
+++ b/Open-ILS/src/perlmods/lib/OpenILS/Application/Actor.pm
@@ -408,12 +408,15 @@ sub update_patron {
     my $barred_hook = '';

     if($patron->isnew()) {
+ # store original password to avoid double-hashing it later
+ my $saved_passwd = $patron->passwd;
         ( $new_patron, $evt ) = _add_patron($e, _clone_patron($patron));
         return $evt if $evt;
         if($U->is_true($patron->barred)) {
             return $e->die_event unless
                 $e->allowed('BAR_PATRON', $patron->home_ou);
         }
+ $new_patron->passwd($saved_passwd);
     } else {
         $new_patron = $patron;

[Galen Charlton (gmc)]

I've pushed a better patch to the collab/gmcharlt/lp1579225_patron_registration_password_mgmt branch in the working repository. Feedback welcome; this still seems pretty hacky to me, and I wonder if we're not better off using database triggers to populate and update actor.passwd.

[Dan Wells (dbw2)]

I am certainly not opposed to a db-level solution, but this could at least be an interim fix. Tested the cases listed in the patch, and also a number of cases involving existing users with non-migrated passwords, including whether the passwords still migrate successfully on login. Everything I tested worked as expected.

I still want others to have a chance to weigh in, but here is my signoff:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/collab/dbwells/lp1579225_patron_registration_password_mgmt_signoff

Thanks, Galen!

[Mike Rylander (mrylander)]

I'd normally say that a pure in-db solution would be preferable to maintain as much separation between the business logic code and the stored passwords, but Galen's approach changes the least amount of code to get the job done, and that's more important for a quick release. Less lines of new code means fewer chances for bugs.

Here's my pile-on sign-off:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/collab/miker/lp1579225_patron_registration_password_mgmt_signoff2

[Kathy Lussier (klussier)]

I'll add my sign-off to the pile too. I tested new patron registrations, updating records with password changes, updating passwords without password changes. It all worked as expected.

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/collab/kmlussier/lp1579225_patron_registration_password_mgmt_signoff3

[Galen Charlton (gmc)]

Thanks for the testing, everybody! I've pushed the patch to master and rel_2_10, along with a live_t regression test I wrote as part of doing a final check of the patch.