Bug 25936

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.

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 :)

No movement here ?

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>

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.

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.

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

QAing

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>

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>

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>

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>

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>

Thanks for the quick QA and follow-ups Marcel :)

Pushed to master for 22.11.

Nice work everyone, thanks!

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!

On it, good call for creations not sending the notice.. I didn't even consider that case, Doh!

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.

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.

Follow-up patches pushed. Thanks, Martin!

I didn't realize that this was pushed. Very nice!