Bug 26705

Summary: System preference NoticeBcc not working
Product: Koha Reporter: Kyle M Hall <kyle>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Kyle M Hall <kyle>
Severity: major    
Priority: P5 - low CC: andrewfh, dcook, fridolin.somers, jonathan.druart, martin.renvoize, mik, tomascohen
Version: MainKeywords: regression
Hardware: All   
OS: All   
URL: https://github.com/rjbs/email-sender/wiki/Bcc
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28017
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28023
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
The Email::Stuffer library we use, doesn't handle Bcc as Mail::Sendmail does. So Bcc handling wasn't working as expected. This patchset adds support for explicitly handling Bcc (including the NoticeBcc feature) to our Koha::Email library.
Version(s) released in:
21.05.00,20.11.05
Bug Depends on: 22343    
Bug Blocks: 29365    
Attachments: Bug 26705: Regression tests
Bug 26705: Make Koha::Email->send_or_die handle Bcc
Bug 26705: Regression tests
Bug 26705: Make Koha::Email->send_or_die handle Bcc
Bug 26705: (QA follow-up) Clarify test result
Bug 26705: (QA follow-up) Clarify test result
Bug 26705: (QA follow-up) Do not rely on latest Email::MIME
Bug 26705: Regression tests
Bug 26705: Make Koha::Email->send_or_die handle Bcc
Bug 26705: (QA follow-up) Clarify test result
Bug 26705: (QA follow-up) Do not rely on latest Email::MIME

Description Kyle M Hall 2020-10-16 11:04:31 UTC
It appears that the system preference NoticBcc is not working. We have tested it on two separate instances of Koha 19.11.

Can anyone confirm if this syspref is working on 19.11, 20.05 and/or master?
Comment 1 Jonathan Druart 2020-11-12 10:17:49 UTC
It seems to work for CHECKOUT on master for me.
Which notices did you try?
Comment 2 Martin Renvoize 2021-01-14 14:52:27 UTC
Any updates on this Kyle?... I'm going to downgrade it to 'normal' and neither Jonathan nor I can replicate it right now.
Comment 3 Michael Kuhn 2021-03-19 20:31:36 UTC
According to the setting in Koha system preference "NoticeBcc" all
notices should be sent to the given e-mail address. But after the update from Koha 19.11.05 to 20.11.03 this stopped working and no e-mails to the BCC-address were created or sent anymore.
Comment 4 Katrin Fischer 2021-03-21 12:18:16 UTC
We now have a second report of this, I am setting a higher priority again. 

I can't test this as I don't have a system sending email and I believe the BCC is not visible otherwise - could someone try in 20.11 maybe?
Comment 5 Tomás Cohen Arazi 2021-03-21 14:55:27 UTC
I'll provide a fix by Monday
Comment 6 Tomás Cohen Arazi 2021-03-21 15:03:50 UTC
Are the reporters using multiple addresses on the syspref?
Comment 7 Michael Kuhn 2021-03-21 15:12:11 UTC
In our case there is (and always was) only one address in system preference "NoticeBcc".

...but - not directly related to the "NoticeBcc" problem - there seems to be a problem with script "process_message_queue.pl" if it finds two e-mail addresses in field "message_queue.to_address" which leads to the following message:

Invalid 'to' parameter: abc@def.com,xyz@def.comxxx: 11 status returned by "/usr/share/koha/bin/cronjobs/process_message_queue.pl"
Comment 8 Tomás Cohen Arazi 2021-03-22 18:07:23 UTC
Ok, I need help here. Specially from email server admins that can see their logs or understand this behavior :-D

What I did:
# Install postfix (and mutt) in KTD:
  $ apt install postfix mutt # it removed exim4
# Add a line to /etc/postfix/main.cf
  $ echo "header_checks = regexp:/etc/postfix/header_checks" >> /etc/postfix/main.cf
# Add a header_checks file:
  $ echo '/.*:/     WARN' > /etc/postfix/header_checks
  $ postmap /etc/postfix/header_checks
# Restart postfix
  $ service postfix restart
# Watch the logs on a terminal:
  $ tail -f /var/log/mail.*

With that setup, I noticed we needed bug 28017 so we can check things without emails failing because of (say) Gmail policy validations.

I generated a 'koha' user in KTD:
  $ adduser koha

Then set things so my user has a check-in notice, and set its address to kohadev-koha@localhost

Set KohaAdminEmailAddress to root@localhost
Set NoticeBcc to koha@localhost

Then run:
   $ kshell
  k$ misc/cronjobs/process_message_queue.pl -v

=> SUCCESS: The logs show:
Mar 22 17:14:07 kohadevbox postfix/smtpd[63414]: connect from localhost[127.0.0.1]
Mar 22 17:14:07 kohadevbox postfix/smtpd[63414]: 8D73D2604A6: client=localhost[127.0.0.1]
Mar 22 17:14:07 kohadevbox postfix/cleanup[63417]: 8D73D2604A6: warning: header Received: from localhost.localdomain (localhost [127.0.0.1])??by kohadevbox (Postfix) with ESMTP id 8D73D2604A6??for <kohadev-koha@localhost>; Mon, 22 Mar 2021 17:14:07 +0000 (UTC) from localhost[127.0.0.1]; from=<root@localhost> to=<kohadev-koha@localhost> proto=ESMTP helo=<localhost.localdomain>
Mar 22 17:14:07 kohadevbox postfix/cleanup[63417]: 8D73D2604A6: warning: header Date: Mon, 22 Mar 2021 17:14:07 +0000 from localhost[127.0.0.1]; from=<root@localhost> to=<kohadev-koha@localhost> proto=ESMTP helo=<localhost.localdomain>
Mar 22 17:14:07 kohadevbox postfix/cleanup[63417]: 8D73D2604A6: warning: header MIME-Version: 1.0 from localhost[127.0.0.1]; from=<root@localhost> to=<kohadev-koha@localhost> proto=ESMTP helo=<localhost.localdomain>
Mar 22 17:14:07 kohadevbox postfix/cleanup[63417]: 8D73D2604A6: warning: header Content-Type: text/plain; charset="utf-8" from localhost[127.0.0.1]; from=<root@localhost> to=<kohadev-koha@localhost> proto=ESMTP helo=<localhost.localdomain>
Mar 22 17:14:07 kohadevbox postfix/cleanup[63417]: 8D73D2604A6: warning: header Content-Transfer-Encoding: quoted-printable from localhost[127.0.0.1]; from=<root@localhost> to=<kohadev-koha@localhost> proto=ESMTP helo=<localhost.localdomain>
Mar 22 17:14:07 kohadevbox postfix/cleanup[63417]: 8D73D2604A6: warning: header From: root@localhost from localhost[127.0.0.1]; from=<root@localhost> to=<kohadev-koha@localhost> proto=ESMTP helo=<localhost.localdomain>
Mar 22 17:14:07 kohadevbox postfix/cleanup[63417]: 8D73D2604A6: warning: header Subject: Check-ins from localhost[127.0.0.1]; from=<root@localhost> to=<kohadev-koha@localhost> proto=ESMTP helo=<localhost.localdomain>
Mar 22 17:14:07 kohadevbox postfix/cleanup[63417]: 8D73D2604A6: warning: header Bcc: koha@localhost from localhost[127.0.0.1]; from=<root@localhost> to=<kohadev-koha@localhost> proto=ESMTP helo=<localhost.localdomain>
Mar 22 17:14:07 kohadevbox postfix/cleanup[63417]: 8D73D2604A6: warning: header To: kohadev-koha@localhost from localhost[127.0.0.1]; from=<root@localhost> to=<kohadev-koha@localhost> proto=ESMTP helo=<localhost.localdomain>
Mar 22 17:14:07 kohadevbox postfix/cleanup[63417]: 8D73D2604A6: warning: header X-Mailer: Koha from localhost[127.0.0.1]; from=<root@localhost> to=<kohadev-koha@localhost> proto=ESMTP helo=<localhost.localdomain>
Mar 22 17:14:07 kohadevbox postfix/cleanup[63417]: 8D73D2604A6: warning: header Message-ID: <16164332470.BFB7954cB.63410@kohadevbox> from localhost[127.0.0.1]; from=<root@localhost> to=<kohadev-koha@localhost> proto=ESMTP helo=<localhost.localdomain>
Mar 22 17:14:07 kohadevbox postfix/cleanup[63417]: 8D73D2604A6: message-id=<16164332470.BFB7954cB.63410@kohadevbox>
Mar 22 17:14:07 kohadevbox postfix/qmgr[63226]: 8D73D2604A6: from=<root@localhost>, size=1071, nrcpt=1 (queue active)
Mar 22 17:14:07 kohadevbox postfix/smtpd[63414]: disconnect from localhost[127.0.0.1] ehlo=1 mail=1 rcpt=1 data=1 quit=1 commands=5
Mar 22 17:14:07 kohadevbox postfix/local[63418]: 8D73D2604A6: to=<kohadev-koha@localhost>, relay=local, delay=0.06, delays=0.05/0.01/0/0, dsn=2.0.0, status=sent (delivered to mailbox)
Mar 22 17:14:07 kohadevbox postfix/qmgr[63226]: 8D73D2604A6: removed

So the Bcc header is there. I'm not sure why it isn't sending the email afterwards. But I'm sure the header is correctly added.

Note: the non-advertised feature of accepting comma separated addresses is still not working. I can solve it on a separate bug report if someone files it. It needs to specify which fields could be multi-address.
Comment 9 Katrin Fischer 2021-03-23 07:20:46 UTC
> Note: the non-advertised feature of accepting comma separated addresses is
> still not working. I can solve it on a separate bug report if someone files
> it. It needs to specify which fields could be multi-address.

I think officially we don't support this, at least not from the GUI where often you can only get past validation entering one address. Maybe the system preferences would have allowed it in the past, but I am not aware of documentation. The report on the mailing list was a custom script that allows to send to multiple addresses, but if we can make that work again without too much trouble, I think it would be nice.
Comment 10 Martin Renvoize 2021-03-23 10:14:51 UTC
I've just had a report from a customer that this may also be affecting the reply-to addresses...

I'm going to do some digging this morning.
Comment 11 Tomás Cohen Arazi 2021-03-23 11:12:03 UTC
Just found out that the Mail::Sendmail lib we used before, does 'Example 2' from the link I pasted above:

https://metacpan.org/release/Mail-Sendmail/source/lib/Mail/Sendmail.pm#L331
Comment 12 Tomás Cohen Arazi 2021-03-23 14:06:46 UTC
Created attachment 118659 [details] [review]
Bug 26705: Regression tests
Comment 13 Tomás Cohen Arazi 2021-03-23 14:06:50 UTC
Created attachment 118660 [details] [review]
Bug 26705: Make Koha::Email->send_or_die handle Bcc

Koha used to rely on Mail::Sendmail for sending emails. As an SMTP
client, the library took the job of extracting Bcc headers (and removing
them) to pass them along with the recipients listed on To: and Cc: to
the SMTP protocol in the form of RCPT TO: lines. [1]

This was overlooked when we moved to Email::Stuffer/Email::Simple and
there's a different behavior, that is a design decision [2].

This patchset re-introduces the behavior from Mail::Sendmail by
overriding the send_or_die method locally (in Koha::Email) and doing the
right thing.

Unless an explicit {to} parameter is passed, it extracts the recipients
from the headers, as Mail::Sendmail does, and calls
$self->SUPER::send_or_die with the right parameters.

To test:
1. Apply the regression tests
2. Run:
   $ kshell
  k$ prove t/Koha/Email.t
=> FAIL: Bcc is not handled correctly!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass! The recipients list is correct! No Bcc header
sent!
5. Sign off :-D

[1] https://metacpan.org/release/Mail-Sendmail/source/lib/Mail/Sendmail.pm#L331
[2] https://metacpan.org/pod/Email::Sender::Manual::QuickStart#Hey,-where's-my-Bcc-support
Comment 14 Martin Renvoize 2021-03-23 14:27:05 UTC
Created attachment 118663 [details] [review]
Bug 26705: Regression tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2021-03-23 14:27:09 UTC
Created attachment 118664 [details] [review]
Bug 26705: Make Koha::Email->send_or_die handle Bcc

Koha used to rely on Mail::Sendmail for sending emails. As an SMTP
client, the library took the job of extracting Bcc headers (and removing
them) to pass them along with the recipients listed on To: and Cc: to
the SMTP protocol in the form of RCPT TO: lines. [1]

This was overlooked when we moved to Email::Stuffer/Email::Simple and
there's a different behavior, that is a design decision [2].

This patchset re-introduces the behavior from Mail::Sendmail by
overriding the send_or_die method locally (in Koha::Email) and doing the
right thing.

Unless an explicit {to} parameter is passed, it extracts the recipients
from the headers, as Mail::Sendmail does, and calls
$self->SUPER::send_or_die with the right parameters.

To test:
1. Apply the regression tests
2. Run:
   $ kshell
  k$ prove t/Koha/Email.t
=> FAIL: Bcc is not handled correctly!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass! The recipients list is correct! No Bcc header
sent!
5. Sign off :-D

[1] https://metacpan.org/release/Mail-Sendmail/source/lib/Mail/Sendmail.pm#L331
[2] https://metacpan.org/pod/Email::Sender::Manual::QuickStart#Hey,-where's-my-Bcc-support

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2021-03-23 14:27:12 UTC
Created attachment 118665 [details] [review]
Bug 26705: (QA follow-up) Clarify test result

Simple output string improvement to fix the copy/paste of the previous
tests comment string.
Comment 17 Martin Renvoize 2021-03-23 14:28:01 UTC
Nice one, well done tracking it down.

Signing off
Comment 18 Kyle M Hall 2021-03-23 17:03:15 UTC
Email::MIME::Header needs to be added to our dependencies.
Comment 19 Tomás Cohen Arazi 2021-03-23 17:17:23 UTC
(In reply to Kyle M Hall from comment #18)
> Email::MIME::Header needs to be added to our dependencies.

The deps are

Email::Stuffer <- Email::MIME > Email::MIME::Header

we could explicitly require Email::MIME, but it is already a dependency for Email::Stuffer.
Comment 20 Kyle M Hall 2021-03-23 17:53:08 UTC
It appears that the problem is that stretch has Email::Mime::Header 1.937 in packages, and buster has 1.949. Unit tests fail on 1.937 but pass on 1.949.
Comment 21 Tomás Cohen Arazi 2021-03-23 19:22:04 UTC
(In reply to Kyle M Hall from comment #20)
> It appears that the problem is that stretch has Email::Mime::Header 1.937 in
> packages, and buster has 1.949. Unit tests fail on 1.937 but pass on 1.949.

Gosh, I love having tests for everything new we add!
Comment 22 Tomás Cohen Arazi 2021-03-23 20:03:04 UTC
Created attachment 118704 [details] [review]
Bug 26705: (QA follow-up) Clarify test result

Simple output string improvement to fix the copy/paste of the previous
tests comment string.
Comment 23 Tomás Cohen Arazi 2021-03-23 20:03:07 UTC
Created attachment 118705 [details] [review]
Bug 26705: (QA follow-up) Do not rely on latest Email::MIME

This patch makes the implementation use lower-level methods to deal with
headers. So the Email::MIME library in Debian 9 works.

Params are sorted for testing purposes.

To test:
1. Apply this patch
2. Run the tests
=> SUCCESS: Things pass in D9+
3. Sign off :-D

Thanks Kyle!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 24 Tomás Cohen Arazi 2021-03-23 20:04:35 UTC
(In reply to Martin Renvoize from comment #16)
> Created attachment 118665 [details] [review] [review]
> Bug 26705: (QA follow-up) Clarify test result
> 
> Simple output string improvement to fix the copy/paste of the previous
> tests comment string.

Thanks for that!
Comment 25 Kyle M Hall 2021-03-24 10:41:06 UTC
Created attachment 118713 [details] [review]
Bug 26705: Regression tests

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 26 Kyle M Hall 2021-03-24 10:41:16 UTC
Created attachment 118714 [details] [review]
Bug 26705: Make Koha::Email->send_or_die handle Bcc

Koha used to rely on Mail::Sendmail for sending emails. As an SMTP
client, the library took the job of extracting Bcc headers (and removing
them) to pass them along with the recipients listed on To: and Cc: to
the SMTP protocol in the form of RCPT TO: lines. [1]

This was overlooked when we moved to Email::Stuffer/Email::Simple and
there's a different behavior, that is a design decision [2].

This patchset re-introduces the behavior from Mail::Sendmail by
overriding the send_or_die method locally (in Koha::Email) and doing the
right thing.

Unless an explicit {to} parameter is passed, it extracts the recipients
from the headers, as Mail::Sendmail does, and calls
$self->SUPER::send_or_die with the right parameters.

To test:
1. Apply the regression tests
2. Run:
   $ kshell
  k$ prove t/Koha/Email.t
=> FAIL: Bcc is not handled correctly!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass! The recipients list is correct! No Bcc header
sent!
5. Sign off :-D

[1] https://metacpan.org/release/Mail-Sendmail/source/lib/Mail/Sendmail.pm#L331
[2] https://metacpan.org/pod/Email::Sender::Manual::QuickStart#Hey,-where's-my-Bcc-support

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 27 Kyle M Hall 2021-03-24 10:41:19 UTC
Created attachment 118715 [details] [review]
Bug 26705: (QA follow-up) Clarify test result

Simple output string improvement to fix the copy/paste of the previous
tests comment string.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 28 Kyle M Hall 2021-03-24 10:41:22 UTC
Created attachment 118716 [details] [review]
Bug 26705: (QA follow-up) Do not rely on latest Email::MIME

This patch makes the implementation use lower-level methods to deal with
headers. So the Email::MIME library in Debian 9 works.

Params are sorted for testing purposes.

To test:
1. Apply this patch
2. Run the tests
=> SUCCESS: Things pass in D9+
3. Sign off :-D

Thanks Kyle!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 29 Jonathan Druart 2021-04-01 16:53:43 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 30 Fridolin Somers 2021-04-07 09:45:19 UTC
Pushed to 20.11.x for 20.11.05
Comment 31 Andrew Fuerste-Henry 2021-04-24 12:20:21 UTC
Doesn't apply cleanly to 20.05, please rebase for backport.