Bug 34731 - C4::Letters::SendQueuedMessages can be triggered with an undef message_id
Summary: C4::Letters::SendQueuedMessages can be triggered with an undef message_id
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low critical (vote)
Assignee: Kyle M Hall
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-09-06 19:51 UTC by Kyle M Hall
Modified: 2023-11-13 14:32 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes an issue where generating a notice that is undefined (for example, where it is empty) will trigger the sending of any pending messages, even though the message queue cronjob isn't run. This can cause an issue for libraries that expect emails and SMS messages to be processed at specific times.
Version(s) released in:
23.11.00,23.05.04


Attachments
Bug 34731: Throw exception if SendQueuedMessages is passed a bad message_id (2.81 KB, patch)
2023-09-07 15:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 34731: Don't call SendQueuedMessages if message_id is bad (9.28 KB, patch)
2023-09-07 15:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 34731: Use Koha::Notice::Message in EnqueueLetter (3.01 KB, patch)
2023-09-07 15:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 34731: Throw exception if SendQueuedMessages is passed a bad message_id (2.85 KB, patch)
2023-09-08 19:48 UTC, David Nind
Details | Diff | Splinter Review
Bug 34731: Don't call SendQueuedMessages if message_id is bad (9.30 KB, patch)
2023-09-08 19:48 UTC, David Nind
Details | Diff | Splinter Review
Bug 34731: Use Koha::Notice::Message in EnqueueLetter (3.05 KB, patch)
2023-09-08 19:48 UTC, David Nind
Details | Diff | Splinter Review
Bug 34731: Throw exception if SendQueuedMessages is passed a bad message_id (2.91 KB, patch)
2023-09-11 11:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34731: Don't call SendQueuedMessages if message_id is bad (9.36 KB, patch)
2023-09-11 11:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34731: Use Koha::Notice::Message in EnqueueLetter (3.12 KB, patch)
2023-09-11 11:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34731: (follow-up) Fix conditional variable declaration (989 bytes, patch)
2023-09-11 11:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
[22.11.x] Bug 34731: Throw exception if SendQueuedMessages is passed a bad message_id (2.96 KB, patch)
2023-11-01 15:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
[22.11.x] Bug 34731: Use Koha::Notice::Message in EnqueueLetter (3.12 KB, patch)
2023-11-01 15:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
[22.11.x] Bug 34731: (follow-up) Fix conditional variable declaration (993 bytes, patch)
2023-11-01 15:26 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2023-09-06 19:51:40 UTC
I've discovered that sometimes C4::Letters::EnqueueLetter will return undef instead of a valid message id. In Koha::Patron::set_password we call C4::Letters::EnqueueLetter and assume a message_id exists which we pass to C4::Letters::SendQueuedMessages. Because message_id is undef, we then *process all pending messages*. This is very very bad considering libraries often have very specific time frames they wish to send message ( especially sms and phone ). In addition, this bypasses the before_send_messages hook.

I have not been able to recreate on demand yet, but I think it is either random or caused by calls to the /api/v1/patrons/<patron_id>/password rest api endpoint.
Comment 1 Kyle M Hall 2023-09-07 15:52:37 UTC
Created attachment 155325 [details] [review]
Bug 34731: Throw exception if SendQueuedMessages is passed a bad message_id

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Letters.t
Comment 2 Kyle M Hall 2023-09-07 15:52:43 UTC
Created attachment 155326 [details] [review]
Bug 34731: Don't call SendQueuedMessages if message_id is bad
Comment 3 Kyle M Hall 2023-09-07 15:52:45 UTC
Created attachment 155327 [details] [review]
Bug 34731: Use Koha::Notice::Message in EnqueueLetter

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Letters.t
Comment 4 Kyle M Hall 2023-09-07 16:48:46 UTC
We have found a way to recreate this bug. If you update the PASSWORD_CHANGE letter to something like:
[%- IF 0 %][% END %]
or anything else that would generate a notice, Koha attempts to enqueue the blank notice in Koha::Patron::set_password, but EnqueueLetter returns if the rendered content is empty. Then we call SendQueuedMessages with message_id => undef!

This patch set will resolve this issue for the affected code and prevent any regressions.
Comment 5 David Nind 2023-09-08 19:48:29 UTC
Created attachment 155459 [details] [review]
Bug 34731: Throw exception if SendQueuedMessages is passed a bad message_id

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Letters.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 6 David Nind 2023-09-08 19:48:31 UTC
Created attachment 155460 [details] [review]
Bug 34731: Don't call SendQueuedMessages if message_id is bad

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 David Nind 2023-09-08 19:48:34 UTC
Created attachment 155461 [details] [review]
Bug 34731: Use Koha::Notice::Message in EnqueueLetter

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Letters.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 David Nind 2023-09-08 20:05:23 UTC
Testing notes (using KTD):

1. Set up KTD so that email can be sent - see [1].

2. Add an email address to the koha user for an account you can access.

3. Test that emails are queued:
   . Edit the koha user patron messaging preferences so that an email is sent when an item is checked out.
   . Checkout an item to the koha user.
   . Note that under notices for the koha user that there is a Checkouts notice with status of pending.
     OR
   . Check the message queue (koha-mysql kohadev + select * from message_queue;) and note that the email status is pending.

4. Edit the CART notice so that the contents for email message is: [%- IF 0 %][% END %]

5. Add some records to the cart and send the cart to an email address.

6. You will receive a message about the checkout, even though the cronjob for sending messages wasn't run (you can also check that in the notices tab for the koha user that the checkouts notice is now showing as sent). In addition, you won't receive a message with the cart details.

7. Apply the patch etc., and repeat steps 3 + 5: this time the checkouts notice isn't sent - the status under notices for the koha user and in the database message queue will stay as pending, and you won't have received the checkout notice.

Note: Possibly don't need to set up email sending - probably possible to only look at the notices tab for the koha user.


[1] To test sending emails using a Google account, edit /etc/koha/sites/kohadev/koha-conf.xml and add this configuration near the end (where <user_name> = your Google email address; <password> = your APP password, not your Google account password), and then restart_all:

 <smtp_server>
    <host>smtp.gmail.com</host>
    <port>587</port>
    <timeout>5</timeout>
    <ssl_mode>STARTTLS</ssl_mode>
    <user_name>GOOGLEACCOUNTUSER</user_name>
    <password>GOOGLEAPPPASSWORD</password>
    <debug>1</debug>
 </smtp_server>
Comment 9 David Nind 2023-09-08 20:11:22 UTC
I've had a go at the release notes - but I'm not sure I've gotten it right!
Comment 10 Martin Renvoize 2023-09-11 11:53:18 UTC
Created attachment 155478 [details] [review]
Bug 34731: Throw exception if SendQueuedMessages is passed a bad message_id

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Letters.t

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2023-09-11 11:53:21 UTC
Created attachment 155479 [details] [review]
Bug 34731: Don't call SendQueuedMessages if message_id is bad

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2023-09-11 11:53:23 UTC
Created attachment 155480 [details] [review]
Bug 34731: Use Koha::Notice::Message in EnqueueLetter

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Letters.t

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2023-09-11 11:53:26 UTC
Created attachment 155481 [details] [review]
Bug 34731: (follow-up) Fix conditional variable declaration

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2023-09-11 11:53:46 UTC
Working well, good fix.

Passing QA
Comment 15 Tomás Cohen Arazi 2023-09-12 12:43:52 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 16 Fridolin Somers 2023-09-14 17:57:20 UTC
Pushed to 23.05.x for 23.05.04
Comment 17 Matt Blenkinsop 2023-09-15 10:07:16 UTC
Missing code from bug 33360 in 22.11.x - not backporting

Nice work everyone!
Comment 18 Kyle M Hall 2023-11-01 15:26:04 UTC
Created attachment 158178 [details] [review]
[22.11.x] Bug 34731: Throw exception if SendQueuedMessages is passed a bad message_id

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Letters.t

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Kyle M Hall 2023-11-01 15:26:12 UTC
Created attachment 158179 [details] [review]
[22.11.x] Bug 34731: Use Koha::Notice::Message in EnqueueLetter

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Letters.t

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Kyle M Hall 2023-11-01 15:26:14 UTC
Created attachment 158180 [details] [review]
[22.11.x] Bug 34731: (follow-up) Fix conditional variable declaration

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Kyle M Hall 2023-11-01 15:27:54 UTC
(In reply to Matt Blenkinsop from comment #17)
> Missing code from bug 33360 in 22.11.x - not backporting
> 
> Nice work everyone!

22.11.x editions attached, please consider backporting!
Comment 22 Matt Blenkinsop 2023-11-13 14:31:36 UTC
Thanks Kyle, backported to 22.11.x