[PATCH 3 of 5] e-mail: allow specifying a different noreply e-mail than app_email_from
Mads Kiilerich
mads at kiilerich.com
Mon Jul 13 15:32:20 EDT 2015
On 07/13/2015 10:45 AM, Thomas De Schampheleire wrote:
> # HG changeset patch
> # User Thomas De Schampheleire <thomas.de.schampheleire at gmail.com>
> # Date 1436729808 -7200
> # Sun Jul 12 21:36:48 2015 +0200
> # Node ID bdca2f365e3feb81ba7329b8f42eada6a342a811
> # Parent 95c18c5fdb59f2d985e2cd8ac6db093cb3e5f884
> e-mail: allow specifying a different noreply e-mail than app_email_from
>
> The configuration setting app_email_from is currently used both for the SMTP
> envelope sender as for the From header inside the e-mail (what mail clients
> show). However, some SMTP servers require a valid, existing address for the
> envelope sender.
Actually, all SMTP servers kind of require a valid envelope sender. They
can however usually not verify it and nobody will notice until the mail
for some reason can't be delivered (deleted or full account or
spam/virus filters or whatever) and the attempt to bounce it back will fail.
IMO, the bounce and 'no reply' mails _should_ be monitored by someone.
Clueful users will know not to use it, and clueless users will use it
and need help.
If you really don't want reliably delivery, you can perhaps use a null
sender address "<>". (But I will not recommend it any more than I will
recommend using an invalid header from address.)
> When no anonymous real address can be created (e.g. due to
> corporate IT rules), app_email_from would need to be that of a real (admin)
> user, which would thus also be used in the From header of e-mails and is
> therefore undesired.
The hidden reasoning here seems to be that you actually do want to use
an invalid email address inside the mail? Please make that design goal
explicit so we have a chance to disagree with it ;-)
/Mads
>
> Instead, add an extra configuration setting app_email_noreply that is used
> for the From header. Setting app_email_from remains being used for the SMTP
> envelope, as well as for error e-mails to the specified administrator.
>
> diff --git a/kallithea/bin/template.ini.mako b/kallithea/bin/template.ini.mako
> --- a/kallithea/bin/template.ini.mako
> +++ b/kallithea/bin/template.ini.mako
> @@ -14,9 +14,12 @@ pdebug = false
> ## ##
> ## email_to: The e-mail address to send error reports to. ##
> ## error_email_from: The sender of error e-mails. ##
> -## app_email_from: The sender of mails originating from Kallithea, for ##
> -## example to notify users about new comments, pull requests, ##
> -## etc. ##
> +## app_email_noreply: The sender of mails originating from Kallithea, for ##
> +## example to notify users about new comments, pull ##
> +## requests, etc. ##
> +## app_email_from: The SMTP sender of mails originating from Kallithea. This ##
> +## address will be used when communicating with the SMTP ##
> +## server only. ##
I don't think the "for communication with the SMTP server" description
is correct and it is thus not so helpful.
More correctly: The envelope sender is used for the actual mail delivery
(for example anti spam sender validation and bounce processing all the
way to the recipient(s)) and is often not shown to the user.
Reading these two descriptions, I'm still confused about what they
actually are used for and which formats are accepted.
app_email_from is usually also a 'noreply' address, just as much as your
app_email_noreply is?
Also, I think it should be made clear that 'app_email_noreply' is
optional and usually not necessary and only for overriding
'app_email_from' in the mail headers. In general, I don't think people
will or should use it and it should thus also not have too much
mentioning. Most admins should not need to know about or care about this
setting.
> ## email_prefix: The subject prefix for mails originating from Kallithea. ##
> ## ##
> ## Note: your SMTP server may require app_email_from/error_email_from to be ##
> @@ -25,6 +28,7 @@ pdebug = false
> #email_to = admin at localhost
> #error_email_from = kallithea-noreply at localhost
> app_email_from = kallithea-noreply at localhost
> +app_email_noreply = kallithea-noreply at localhost
> email_prefix = [Kallithea]
>
> #smtp_server = mail.server.com
> diff --git a/kallithea/config/deployment.ini_tmpl b/kallithea/config/deployment.ini_tmpl
> --- a/kallithea/config/deployment.ini_tmpl
> +++ b/kallithea/config/deployment.ini_tmpl
> @@ -15,9 +15,12 @@ pdebug = false
> ## ##
> ## email_to: The e-mail address to send error reports to. ##
> ## error_email_from: The sender of error e-mails. ##
> -## app_email_from: The sender of mails originating from Kallithea, for ##
> -## example to notify users about new comments, pull requests, ##
> -## etc. ##
> +## app_email_noreply: The sender of mails originating from Kallithea, for ##
> +## example to notify users about new comments, pull ##
> +## requests, etc. ##
> +## app_email_from: The SMTP sender of mails originating from Kallithea. This ##
> +## address will be used when communicating with the SMTP ##
> +## server only. ##
> ## email_prefix: The subject prefix for mails originating from Kallithea. ##
> ## ##
> ## Note: your SMTP server may require app_email_from/error_email_from to be ##
> @@ -26,6 +29,7 @@ pdebug = false
> #email_to = admin at localhost
> #error_email_from = kallithea-noreply at localhost
> app_email_from = kallithea-noreply at localhost
> +app_email_noreply = kallithea-noreply at localhost
> email_prefix = [Kallithea]
>
> #smtp_server = mail.server.com
> diff --git a/kallithea/lib/celerylib/tasks.py b/kallithea/lib/celerylib/tasks.py
> --- a/kallithea/lib/celerylib/tasks.py
> +++ b/kallithea/lib/celerylib/tasks.py
> @@ -279,19 +279,18 @@ def send_email(recipients, subject, body
> # SMTP sender
> envelope_from = email_config.get('app_email_from', 'Kallithea')
> # From header
> + noreply_addr = email_config.get('app_email_noreply', envelope_from)
> if author is None:
> - headers['From'] = envelope_from
> + headers['From'] = noreply_addr
> else:
> # set From header based on author but with a generic e-mail address
> # In case app_email_from is in "Some Name <e-mail>" format, we first
> # extract the e-mail address.
> import re
> - m = re.match('.*<(.*)>', envelope_from)
> + m = re.match('.*<(.*)>', noreply_addr)
> if m is not None:
> - envelope_addr = m.group(1)
> - else:
> - envelope_addr = envelope_from
> - headers['From'] = '%s <%s>' % (author.full_name_or_username, envelope_addr)
> + noreply_addr = m.group(1)
> + headers['From'] = '%s <%s>' % (author.full_name_or_username, noreply_addr)
>
> user = email_config.get('smtp_username')
> passwd = email_config.get('smtp_password')
> diff --git a/kallithea/tests/other/test_mail.py b/kallithea/tests/other/test_mail.py
> --- a/kallithea/tests/other/test_mail.py
> +++ b/kallithea/tests/other/test_mail.py
> @@ -92,3 +92,54 @@ class TestMail(BaseTestCase):
> self.assertIn('Subject: %s' % subject, smtplib_mock.lastmsg)
> self.assertIn(body, smtplib_mock.lastmsg)
> self.assertIn(html_body, smtplib_mock.lastmsg)
> +
> + def test_send_mail_with_author_noreply(self):
> + mailserver = 'smtp.mailserver.org'
> + recipients = ['rcpt1', 'rcpt2']
> + envelope_from = 'noreply at mailserver.org'
> + noreply_addr = 'another at mailserver.org'
> + subject = 'subject'
> + body = 'body'
> + html_body = 'html_body'
> + author = User.get_by_username(TEST_USER_REGULAR_LOGIN)
> +
> + config_mock = {
> + 'smtp_server': mailserver,
> + 'app_email_from': envelope_from,
> + 'app_email_noreply': noreply_addr,
> + }
> + with mock.patch('kallithea.lib.celerylib.tasks.config', config_mock):
> + t.send_email(recipients, subject, body, html_body, author=author)
> +
> + self.assertSetEqual(smtplib_mock.lastdest, set(recipients))
> + self.assertEqual(smtplib_mock.lastsender, envelope_from)
> + self.assertIn('From: Kallithea Admin <%s>' % noreply_addr, smtplib_mock.lastmsg)
> + self.assertIn('Subject: %s' % subject, smtplib_mock.lastmsg)
> + self.assertIn(body, smtplib_mock.lastmsg)
> + self.assertIn(html_body, smtplib_mock.lastmsg)
> +
> + def test_send_mail_with_author_full_mail_noreply(self):
> + mailserver = 'smtp.mailserver.org'
> + recipients = ['rcpt1', 'rcpt2']
> + envelope_from = 'noreply at mailserver.org'
> + noreply_addr = 'another at mailserver.org'
> + noreply_full = 'Some Name <%s>' % noreply_addr
> + subject = 'subject'
> + body = 'body'
> + html_body = 'html_body'
> + author = User.get_by_username(TEST_USER_REGULAR_LOGIN)
> +
> + config_mock = {
> + 'smtp_server': mailserver,
> + 'app_email_from': envelope_from,
> + 'app_email_noreply': noreply_full,
> + }
> + with mock.patch('kallithea.lib.celerylib.tasks.config', config_mock):
> + t.send_email(recipients, subject, body, html_body, author=author)
> +
> + self.assertSetEqual(smtplib_mock.lastdest, set(recipients))
> + self.assertEqual(smtplib_mock.lastsender, envelope_from)
> + self.assertIn('From: Kallithea Admin <%s>' % noreply_addr, smtplib_mock.lastmsg)
> + self.assertIn('Subject: %s' % subject, smtplib_mock.lastmsg)
> + self.assertIn(body, smtplib_mock.lastmsg)
> + self.assertIn(html_body, smtplib_mock.lastmsg)
More information about the kallithea-general
mailing list