Bug 25936 - Notify users if their password has changed
Summary: Notify users if their password has changed
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords: Manual, release-notes-needed
Depends on:
Blocks:
 
Reported: 2020-07-06 02:21 UTC by David Cook
Modified: 2022-03-22 11:33 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 25936: Add option to send password change notices (7.29 KB, patch)
2022-03-03 12:06 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25936: Add option to send password change notices (7.29 KB, patch)
2022-03-03 12:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25936: Add option to send password change notices (7.30 KB, patch)
2022-03-03 12:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25936: Add option to send password change notices (7.40 KB, patch)
2022-03-03 12:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 25936: Add option to send password change notices (7.36 KB, patch)
2022-03-09 12:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25936: (follow-up) Rename notice and fix indentation (3.17 KB, patch)
2022-03-09 12:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25935: (QA follow-up) Add to translated installers (3.69 KB, patch)
2022-03-09 12:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25936: (QA follow-up) Add to translated installers (3.64 KB, patch)
2022-03-10 11:13 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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
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
Comment 4 Marcel de Rooy 2022-03-03 12:45:09 UTC
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>
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
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>
Comment 9 Martin Renvoize 2022-03-09 12:28:58 UTC
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.
Comment 10 Martin Renvoize 2022-03-09 12:29:02 UTC
Created attachment 131521 [details] [review]
Bug 25935: (QA follow-up) Add to translated installers
Comment 11 Martin Renvoize 2022-03-10 11:13:01 UTC
Created attachment 131543 [details] [review]
Bug 25936: (QA follow-up) Add to translated installers
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 :)