Bug 25936

Summary: Notify users if their password has changed
Product: Koha Reporter: David Cook <dcook>
Component: AuthenticationAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: caroline.cyr-la-rose, dpavlin, george, jon.turner, jonathan.druart, katrin.fischer, lucas, m.de.rooy, martin.renvoize, sally.healey, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27812
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30206
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Caroline Cyr La Rose Documentation submission: https://gitlab.com/koha-community/koha-manual/-/merge_requests/632
Text to go in the release notes:
When the system preference NotifyPasswordChange is set to 'Notify' a notification will be sent to the user when their password is changed. The new notification uses the letter code PASSCHANGE. **Sponsored by** *PTFS Europe*
Version(s) released in:
22.11.00
Bug Depends on:    
Bug Blocks: 32917    
Attachments: Bug 25936: Add option to send password change notices
Bug 25936: Add option to send password change notices
Bug 25936: Add option to send password change notices
Bug 25936: Add option to send password change notices
Bug 25936: Add option to send password change notices
Bug 25936: (follow-up) Rename notice and fix indentation
Bug 25935: (QA follow-up) Add to translated installers
Bug 25936: (QA follow-up) Add to translated installers
Bug 25936: Add option to send password change notices
Bug 25936: (follow-up) Rename notice and fix indentation
Bug 25936: Unit tests
Bug 25936: Add option to send password change notices
Bug 25936: (follow-up) Rename notice and fix indentation
Bug 25936: Unit tests
Bug 25936: (QA follow-up) Spelling and wording
Bug 25936: (QA follow-up) Enable feature for new installs
Bug 25936: (follow-up) Fix ../api/v1/patrons.t
Bug 25936: (follow-up) Unit test for store

Description David Cook 2020-07-06 02:21:03 UTC
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.
Comment 1 Martin Renvoize 2022-03-03 12:06:26 UTC Comment hidden (obsolete)
Comment 2 Martin Renvoize 2022-03-03 12:08:12 UTC Comment hidden (obsolete)
Comment 3 Martin Renvoize 2022-03-03 12:13:01 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2022-03-03 12:45:09 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2022-03-03 12:49:09 UTC
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.
Comment 6 Martin Renvoize 2022-03-03 16:23:54 UTC
(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 ;).
Comment 7 Katrin Fischer 2022-03-06 13:41:54 UTC
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.
Comment 8 Martin Renvoize 2022-03-09 12:28:53 UTC Comment hidden (obsolete)
Comment 9 Martin Renvoize 2022-03-09 12:28:58 UTC Comment hidden (obsolete)
Comment 10 Martin Renvoize 2022-03-09 12:29:02 UTC Comment hidden (obsolete)
Comment 11 Martin Renvoize 2022-03-10 11:13:01 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2022-03-22 11:33:23 UTC
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 :)
Comment 13 Marcel de Rooy 2022-09-28 12:33:40 UTC
No movement here ?
Comment 14 Martin Renvoize 2022-09-29 11:46:41 UTC
Created attachment 141080 [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>
Comment 15 Martin Renvoize 2022-09-29 11:46:46 UTC
Created attachment 141081 [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.
Comment 16 Martin Renvoize 2022-09-29 11:46:51 UTC
Created attachment 141082 [details] [review]
Bug 25936: Unit tests

This patch adds a unit test to check that the notice is enqueued and
sending is attempted immediately.
Comment 17 Martin Renvoize 2022-09-29 11:48:14 UTC
Rebased, Dropped the patch for translated installers as they no longer exist and removed the eval inline as we now catch such conditions inside SendQueuedMessages instead.

Unit tests added
Comment 18 Marcel de Rooy 2022-09-30 06:23:00 UTC
QAing
Comment 19 Marcel de Rooy 2022-09-30 07:15:42 UTC
Created attachment 141104 [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>
Comment 20 Marcel de Rooy 2022-09-30 07:15:46 UTC
Created attachment 141105 [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.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 21 Marcel de Rooy 2022-09-30 07:15:51 UTC
Created attachment 141106 [details] [review]
Bug 25936: Unit tests

This patch adds a unit test to check that the notice is enqueued and
sending is attempted immediately.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Marcel de Rooy 2022-09-30 07:15:55 UTC
Created attachment 141107 [details] [review]
Bug 25936: (QA follow-up) Spelling and wording

[1] NotifyPassowrdChange => NotifyPasswordChange
[2] Someone has changed your library user account password.
    Scary? Changed it myself. Reworded it a bit.
[3] is( $THE_notice->status, 'failed', "The notice was sent immediately");
    Note that THE_notice is imo horrible :) Wont touch it though.
    But I think we should reword 'sent' since it just failed.
    And we actually should mock sending mail here. Adding FIXME.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 23 Marcel de Rooy 2022-09-30 07:15:59 UTC
Created attachment 141108 [details] [review]
Bug 25936: (QA follow-up) Enable feature for new installs

It feels safer to enable this rightaway for new installs.
Since we always love to "keep current behavior", I do not really
mind to keep it disabled at upgrade time.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 24 Martin Renvoize 2022-09-30 10:41:47 UTC
Thanks for the quick QA and follow-ups Marcel :)
Comment 25 Tomás Cohen Arazi 2022-10-03 17:37:11 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 26 Tomás Cohen Arazi 2022-10-03 21:30:02 UTC
Hi Martin and Marcel. This broke the api/v1/patrons.t tests, which felt pretty trivial to fix by disabling the NotifyPasswordChange syspref on the tests but...

... it feels like patron creation shouldn't trigger the notice. So: can you please fix that? Tests for this little situation are welcome too. I can help tomorrow, but right now I've gotta call it a day.

Thanks in advance!
Comment 27 Martin Renvoize 2022-10-04 10:09:05 UTC
On it, good call for creations not sending the notice.. I didn't even consider that case, Doh!
Comment 28 Martin Renvoize 2022-10-04 13:17:21 UTC
Created attachment 141306 [details] [review]
Bug 25936: (follow-up) Fix ../api/v1/patrons.t

Disable password change notifications for the api test to negate the
need for setting up notices.
Comment 29 Martin Renvoize 2022-10-04 13:17:30 UTC
Created attachment 141307 [details] [review]
Bug 25936: (follow-up) Unit test for store

This unit test confirms that we are not triggering the Password Change
notification notice upon patron creations.
Comment 30 Tomás Cohen Arazi 2022-10-06 02:49:42 UTC
Follow-up patches pushed. Thanks, Martin!
Comment 31 David Cook 2023-03-02 23:01:43 UTC
I didn't realize that this was pushed. Very nice!