We should email/SMS users if their password has changed. We shouldn't include any information about the old or new password, but just alert them that it has been changed. This is useful in the event that someone has compromised an account and changed the password.
Created attachment 131316 [details] [review] Bug 25936: Add option to send password change notices This patch add a new security notice to allow sending notification of password changes to patrons. If enabled, the 'PASSCHANGE' notice will be sent to respective patrons whenever their password is updated. Test plan 1) Run the database updates 2) Enable the new feature by setting 'NotifyPasswordChange' to 'Notify' 3) Change a users password 4) Check that the notice appears in the patrons notices
Created attachment 131317 [details] [review] Bug 25936: Add option to send password change notices This patch add a new security notice to allow sending notification of password changes to patrons. If enabled, the 'PASSCHANGE' notice will be sent to respective patrons whenever their password is updated. Test plan 1) Run the database updates 2) Enable the new feature by setting 'NotifyPasswordChange' to 'Notify' 3) Change a users password 4) Check that the notice appears in the patrons notices
Created attachment 131318 [details] [review] Bug 25936: Add option to send password change notices This patch add a new security notice to allow sending notification of password changes to patrons. If enabled, the 'PASSCHANGE' notice will be sent to respective patrons whenever their password is updated. Test plan 1) Run the database updates 2) Enable the new feature by setting 'NotifyPasswordChange' to 'Notify' 3) Change a users password 4) Check that the notice appears in the patrons notices
Created attachment 131319 [details] [review] Bug 25936: Add option to send password change notices This patch add a new security notice to allow sending notification of password changes to patrons. If enabled, the 'PASSCHANGE' notice will be sent to respective patrons whenever their password is updated. Test plan 1) Run the database updates 2) Enable the new feature by setting 'NotifyPasswordChange' to 'Notify' 3) Change a users password 4) Check that the notice appears in the patrons notices Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Great addition. Security++ Only thing I could say about the code: if ( !C4::Auth::checkpw_hash( $password, $self_from_storage->password ) ) { I understand this change but it should not really be bound to notify yes or no. Could imagine that set_password should check that always and report it? Currently the interface lets you change your password while not entering a new one.
(In reply to Marcel de Rooy from comment #5) > Great addition. Security++ > > Only thing I could say about the code: > > if ( !C4::Auth::checkpw_hash( $password, $self_from_storage->password ) ) { > > I understand this change but it should not really be bound to notify yes or > no. Could imagine that set_password should check that always and report it? > Currently the interface lets you change your password while not entering a > new one. I went through the same thought process when I wrote that line. I chose to put it inside the check for two reasons. One, to not require the extra DB hit unless necessary and Two, I was sure what to do with an exception if we did throw one. I decided in the end that it should be handled in the UI if we want to perform such a check, but should that be in opac and staff, or just opac, and what about imports ;).
Love the activity that discussion about ACCTDETAILS has caused and this is really a good enhancement! Suggestions: * We already have PASSWORD_RESET, maybe we could use PASSWORD_CHANGE here instead of PASSCHANGE to better match them up. Blockers: 1) Sadly, we still have to deal with some installers. Could you please add an SQL version of the new notice to those? Especially for UNIMARC users we cannot switch them over yet as we still have to deal with some framework issues. There is a test in xt to see if you have caught them all! 2) When using updatedatabase for adding the notice, there are some strange indentations Dear [% borrower.firstname %] [% borrower.surname %], Someone has changed your library user account password. If this is unexpected, please contact the library. I think the database update needs to use \n in the SQL. Question: * It looks like this will only work when the 'set_password' method is used, but not when you change the password using 'edit patron', correct? I don't see this as a blocker, but wanted to verify.
Created attachment 131519 [details] [review] Bug 25936: Add option to send password change notices This patch add a new security notice to allow sending notification of password changes to patrons. If enabled, the 'PASSCHANGE' notice will be sent to respective patrons whenever their password is updated. Test plan 1) Run the database updates 2) Enable the new feature by setting 'NotifyPasswordChange' to 'Notify' 3) Change a users password 4) Check that the notice appears in the patrons notices Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 131520 [details] [review] Bug 25936: (follow-up) Rename notice and fix indentation This patch renames the notice from PASSCHANGE to PASSWORD_CHANGE which is both clearer and more consistent. It also fixes the indentation issues mentioned in the atomicupdate.
Created attachment 131521 [details] [review] Bug 25935: (QA follow-up) Add to translated installers
Created attachment 131543 [details] [review] Bug 25936: (QA follow-up) Add to translated installers
1. Missing tests 2. I would move the code to a separate method, to prevent set_password to grow too much 3. There is an eval without "catch" (and eval should be try actually). That's almost never a good idea :)