Bug 27860 - Bad KohaAdminEmailAddress breaks patron self registration and password reset feature
Summary: Bad KohaAdminEmailAddress breaks patron self registration and password reset ...
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-03-04 13:34 UTC by Katrin Fischer
Modified: 2021-03-23 18:37 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.04


Attachments
Bug 27860: Add message_id param to SendQueuedMessages (4.60 KB, patch)
2021-03-05 09:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27860: Fix self-registration if messages with incorrect email are inqueued (2.30 KB, patch)
2021-03-05 09:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27860: Fix password recovery (2.03 KB, patch)
2021-03-05 09:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27860: Add message_id param to SendQueuedMessages (4.67 KB, patch)
2021-03-08 16:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27860: Fix self-registration if messages with incorrect email are inqueued (2.37 KB, patch)
2021-03-08 16:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27860: Fix password recovery (2.10 KB, patch)
2021-03-08 16:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27860: Add message_id param to SendQueuedMessages (4.73 KB, patch)
2021-03-12 14:30 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27860: Fix self-registration if messages with incorrect email are inqueued (2.42 KB, patch)
2021-03-12 14:30 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27860: Fix password recovery (2.15 KB, patch)
2021-03-12 14:30 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2021-03-04 13:34:39 UTC
When the patron self registration form is used with email verification and a bad entry in the KohaAdminEmailAddress system preference an error will be shown, but the message is still created and added to the message_queue table.

Now every time a patron tries to register the system tries to send the already generated message with the wrong from address, which fails. This means the error will remain, although the pref has been corrected meanwhile.

Settings:
- PatronSelfRegistrationVerifyByEmail = Require
- KohaAdminEmailAdress like koha@localhost
- PatronSelfRegistration activated
Comment 1 Jonathan Druart 2021-03-04 13:42:04 UTC
The fix is to retrieve message_id from EnqueueLetter then pass it to SendQueueMessages

_get_unsent_messages needs adjustments to get message_id as parameter.
Comment 2 Katrin Fischer 2021-03-04 16:26:29 UTC
This might also affect the password reset feature as it uses the same mechanism for sending the email without delay.
Comment 3 Jonathan Druart 2021-03-05 09:18:37 UTC
Created attachment 117796 [details] [review]
Bug 27860: Add message_id param to SendQueuedMessages

We could use C4::Letter::GetMessage, but the query in
_get_unsent_messages join on borrowers and return the branchcode.
To prevent any regressions it's preferable to not modify
SendQueuedMessages.

Ideally we obviously need a Koha methods to have better and clean code..
Comment 4 Jonathan Druart 2021-03-05 09:18:40 UTC
Created attachment 117797 [details] [review]
Bug 27860: Fix self-registration if messages with incorrect email are inqueued

If the pref KohaAdminEmailAddress was not set correctly (invalid email)
and has been used to generate message, the message_queue table can contain
some messages that are not sent (pending) and will be processed each
time a new self-reg is done.
The PatronSelfRegistrationVerifyByEmail feature must send only the
notice we just generated, not the whole pending queue.

Test plan:
1. Do not apply the patches
2. Set KohaAdminEmailAddress to an invalid email (root@localhost for
instance)
3. Turn on PatronSelfRegistrationVerifyByEmail
4. Self-reg a patron
5. Boom (that must be fixed on a separate bug report)
6. Set KohaAdminEmailAddress to a valid email (root@example.org)
7. Self-reg a patron
8. Still Boom! (because it's processing the first invalid email)
9. Apply the patches, restart_all
10. Self-reg a patron
=> The email is sent!
Notice the change in DB, the first email is still there with status
"pending" and second email is sent!

QA: at step 10. TODO first email must be 'failed' (highlight in tests)
Comment 5 Jonathan Druart 2021-03-05 09:18:43 UTC
Created attachment 117798 [details] [review]
Bug 27860: Fix password recovery

Same as the previous patch but for the password recovery feature.

Test plan:
1. Do not apply the patches
2. Set KohaAdminEmailAddress to an invalid email (root@localhost for
instance)
3. Use the password recovery feature
5. Boom (that must be fixed on a separate bug report)
6. Set KohaAdminEmailAddress to a valid email (root@example.org)
7. Use again the password recovery for a different user
8. Still Boom! (because it's processing the first invalid email)
9. Apply the patches, restart_all
10. Try again the password recovery
=> The email is sent!
Notice the change in DB, the first email is still there with status
"pending" and second email is sent!
Comment 6 Martin Renvoize 2021-03-08 08:46:20 UTC
Should we also perhaps add a warning to the about page for queued messages with bad from addresses?
Comment 7 Martin Renvoize 2021-03-08 16:01:37 UTC
Created attachment 117931 [details] [review]
Bug 27860: Add message_id param to SendQueuedMessages

We could use C4::Letter::GetMessage, but the query in
_get_unsent_messages join on borrowers and return the branchcode.
To prevent any regressions it's preferable to not modify
SendQueuedMessages.

Ideally we obviously need a Koha methods to have better and clean code..

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2021-03-08 16:01:40 UTC
Created attachment 117932 [details] [review]
Bug 27860: Fix self-registration if messages with incorrect email are inqueued

If the pref KohaAdminEmailAddress was not set correctly (invalid email)
and has been used to generate message, the message_queue table can contain
some messages that are not sent (pending) and will be processed each
time a new self-reg is done.
The PatronSelfRegistrationVerifyByEmail feature must send only the
notice we just generated, not the whole pending queue.

Test plan:
1. Do not apply the patches
2. Set KohaAdminEmailAddress to an invalid email (root@localhost for
instance)
3. Turn on PatronSelfRegistrationVerifyByEmail
4. Self-reg a patron
5. Boom (that must be fixed on a separate bug report)
6. Set KohaAdminEmailAddress to a valid email (root@example.org)
7. Self-reg a patron
8. Still Boom! (because it's processing the first invalid email)
9. Apply the patches, restart_all
10. Self-reg a patron
=> The email is sent!
Notice the change in DB, the first email is still there with status
"pending" and second email is sent!

QA: at step 10. TODO first email must be 'failed' (highlight in tests)
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2021-03-08 16:01:43 UTC
Created attachment 117933 [details] [review]
Bug 27860: Fix password recovery

Same as the previous patch but for the password recovery feature.

Test plan:
1. Do not apply the patches
2. Set KohaAdminEmailAddress to an invalid email (root@localhost for
instance)
3. Use the password recovery feature
5. Boom (that must be fixed on a separate bug report)
6. Set KohaAdminEmailAddress to a valid email (root@example.org)
7. Use again the password recovery for a different user
8. Still Boom! (because it's processing the first invalid email)
9. Apply the patches, restart_all
10. Try again the password recovery
=> The email is sent!
Notice the change in DB, the first email is still there with status
"pending" and second email is sent!

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2021-03-08 16:02:11 UTC
Works as expected.. adding a warning on the about page is a 'nice to have'.

Signing off
Comment 11 Jonathan Druart 2021-03-10 09:11:06 UTC
(In reply to Martin Renvoize from comment #10)
> Works as expected.. adding a warning on the about page is a 'nice to have'.

We have that already, it's enough IMO:
  commit 8850b7cf68dada380b8d7e5a8f6c83b1da76ebff
  Bug 22343: Add warning to the about page if KohaAdminEmailAddress is invalid
Comment 12 Martin Renvoize 2021-03-10 16:50:36 UTC
(In reply to Jonathan Druart from comment #11)
> (In reply to Martin Renvoize from comment #10)
> > Works as expected.. adding a warning on the about page is a 'nice to have'.
> 
> We have that already, it's enough IMO:
>   commit 8850b7cf68dada380b8d7e5a8f6c83b1da76ebff
>   Bug 22343: Add warning to the about page if KohaAdminEmailAddress is
> invalid

It doesn't warn on existing data within the queue.. but actually, I think perhaps that's an excessive for the user really.
Comment 13 Nick Clemens 2021-03-12 14:30:42 UTC
Created attachment 118201 [details] [review]
Bug 27860: Add message_id param to SendQueuedMessages

We could use C4::Letter::GetMessage, but the query in
_get_unsent_messages join on borrowers and return the branchcode.
To prevent any regressions it's preferable to not modify
SendQueuedMessages.

Ideally we obviously need a Koha methods to have better and clean code..

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 14 Nick Clemens 2021-03-12 14:30:45 UTC
Created attachment 118202 [details] [review]
Bug 27860: Fix self-registration if messages with incorrect email are inqueued

If the pref KohaAdminEmailAddress was not set correctly (invalid email)
and has been used to generate message, the message_queue table can contain
some messages that are not sent (pending) and will be processed each
time a new self-reg is done.
The PatronSelfRegistrationVerifyByEmail feature must send only the
notice we just generated, not the whole pending queue.

Test plan:
1. Do not apply the patches
2. Set KohaAdminEmailAddress to an invalid email (root@localhost for
instance)
3. Turn on PatronSelfRegistrationVerifyByEmail
4. Self-reg a patron
5. Boom (that must be fixed on a separate bug report)
6. Set KohaAdminEmailAddress to a valid email (root@example.org)
7. Self-reg a patron
8. Still Boom! (because it's processing the first invalid email)
9. Apply the patches, restart_all
10. Self-reg a patron
=> The email is sent!
Notice the change in DB, the first email is still there with status
"pending" and second email is sent!

QA: at step 10. TODO first email must be 'failed' (highlight in tests)
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 15 Nick Clemens 2021-03-12 14:30:48 UTC
Created attachment 118203 [details] [review]
Bug 27860: Fix password recovery

Same as the previous patch but for the password recovery feature.

Test plan:
1. Do not apply the patches
2. Set KohaAdminEmailAddress to an invalid email (root@localhost for
instance)
3. Use the password recovery feature
5. Boom (that must be fixed on a separate bug report)
6. Set KohaAdminEmailAddress to a valid email (root@example.org)
7. Use again the password recovery for a different user
8. Still Boom! (because it's processing the first invalid email)
9. Apply the patches, restart_all
10. Try again the password recovery
=> The email is sent!
Notice the change in DB, the first email is still there with status
"pending" and second email is sent!

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 16 Jonathan Druart 2021-03-16 15:11:10 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 17 Fridolin Somers 2021-03-19 15:15:09 UTC
Pushed to 20.11.x for 20.11.04
Comment 18 Andrew Fuerste-Henry 2021-03-23 18:37:57 UTC
This didn't apply cleanly to 20.05 and then failed a test when I cleared the conflicts. Please rebase if needed for 20.05. Here's the error:
root@kohadevbox:koha(rmain2005)$ prove t/db_dependent/Letters.t
t/db_dependent/Letters.t .. 6/74 Fake sendmail at t/db_dependent/Letters.t line 33.
Fake sendmail at t/db_dependent/Letters.t line 33.
Fake sendmail at t/db_dependent/Letters.t line 33.
Fake sendmail at t/db_dependent/Letters.t line 33.
Fake sendmail at t/db_dependent/Letters.t line 33.

    #   Failed test 'Exception thrown if invalid email is passed'
    #   at t/db_dependent/Letters.t line 901.
    # expecting: Koha::Exceptions::BadParameter
    # found: normal exit

    #   Failed test 'Invalid KohaAdminEmailAddress => status pending'
    #   at t/db_dependent/Letters.t line 905.
    #          got: 'sent'
    #     expected: 'pending'
Fake sendmail at t/db_dependent/Letters.t line 33.

    #   Failed test 'Message 1 status is unchanged'
    #   at t/db_dependent/Letters.t line 912.
    #          got: 'sent'
    #     expected: 'pending'
    # Looks like you planned 5 tests but ran 4.
    # Looks like you failed 3 tests of 4 run.

#   Failed test 'Test message_id parameter for SendQueuedMessages'
#   at t/db_dependent/Letters.t line 914.
# Looks like you failed 1 test of 74.
t/db_dependent/Letters.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/74 subtests 

Test Summary Report
-------------------
t/db_dependent/Letters.t (Wstat: 256 Tests: 74 Failed: 1)
  Failed test:  74
  Non-zero exit status: 1