From c08d0eb7f34bfdeccbbd9ef25e7c9ee484533152 Mon Sep 17 00:00:00 2001 From: Katherine Zaoral Date: Thu, 27 Jan 2022 16:45:04 -0300 Subject: [PATCH 1/2] [FIX] mail_outbound_static: valid email_from parseaddr does separate the part of an email address and the name but actually: 1. does not check if the email address is a valid one. 2. does not properly work if the incomming Message['From'] is in a wrong format. Thanks to this, when we are trying to get the domain of the email_from a really generic exception is raised. "IndexError: list index out of range" only because the email_from is not a valid email address. To avoid this we check the email_from address is a valid one first. The test case that brings this change was this one: Message['From'] = "Name, Last Name . Since we have the name_from with a comma but without a "" surroding the parseaddr result was ('', 'Name') Now the proper raise will be shown to the user. --- mail_outbound_static/models/ir_mail_server.py | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/mail_outbound_static/models/ir_mail_server.py b/mail_outbound_static/models/ir_mail_server.py index 0311cf5dcf..aee2099dc9 100644 --- a/mail_outbound_static/models/ir_mail_server.py +++ b/mail_outbound_static/models/ir_mail_server.py @@ -3,9 +3,10 @@ import re from email.utils import formataddr, parseaddr +from email_validator import validate_email, EmailSyntaxError from odoo import _, api, fields, models, tools -from odoo.exceptions import ValidationError +from odoo.exceptions import ValidationError, UserError class IrMailServer(models.Model): @@ -72,12 +73,21 @@ def send_email( self, message, mail_server_id=None, smtp_server=None, *args, **kwargs ): # Get email_from and name_from - if message["From"].count("<") > 1: + name_from, email_from = parseaddr(message["From"]) + + # Validate that the email address is a valid one + try: + validate_email(email_from) + except EmailSyntaxError: + # Not able to process the email from, try to fix it manually split_from = message["From"].rsplit(" <", 1) name_from = split_from[0] email_from = split_from[-1].replace(">", "") - else: - name_from, email_from = parseaddr(message["From"]) + try: + validate_email(email_from) + except EmailSyntaxError as exp: + # email is not valid, exception message is human-readable + raise UserError('Invalid email address "%s". ' % message["From"] + str(exp)) email_domain = email_from.split("@")[1] From 65792c1d1ba4225226b817f3423b046277830648 Mon Sep 17 00:00:00 2001 From: Katherine Zaoral Date: Mon, 21 Feb 2022 09:44:08 -0300 Subject: [PATCH 2/2] [WIP][ADD] mail_outbound_static: unit tests --- .../tests/test_ir_mail_server.py | 50 +++++++++++++++---- 1 file changed, 39 insertions(+), 11 deletions(-) diff --git a/mail_outbound_static/tests/test_ir_mail_server.py b/mail_outbound_static/tests/test_ir_mail_server.py index 435ab9a399..9eb61d1d68 100644 --- a/mail_outbound_static/tests/test_ir_mail_server.py +++ b/mail_outbound_static/tests/test_ir_mail_server.py @@ -9,7 +9,7 @@ from mock import MagicMock import odoo.tools as tools -from odoo.exceptions import ValidationError +from odoo.exceptions import ValidationError, UserError from odoo.tests.common import TransactionCase _logger = logging.getLogger(__name__) @@ -19,15 +19,13 @@ class TestIrMailServer(TransactionCase): def setUp(self): super(TestIrMailServer, self).setUp() self.email_from = "derp@example.com" - self.email_from_another = "another@example.com" self.Model = self.env["ir.mail_server"] - self.parameter_model = self.env["ir.config_parameter"] + self.parameter_model = self.env["ir.config_parameter"].sudo() self._delete_mail_servers() - self.Model.create( + self.mail_server_wo_whitelist = self.Model.create( { "name": "localhost", "smtp_host": "localhost", - "smtp_from": self.email_from, } ) message_file = os.path.join( @@ -59,7 +57,7 @@ def _init_mail_server_domain_whilelist_based(self): "name": "sandbox domainthree", "smtp_host": "localhost", "smtp_from": "notifications@domainthree.com", - "domain_whitelist": "domainthree.com,domainmulti.com", + "domain_whitelist": "domainthree.com,domainfour.com", } ) @@ -91,12 +89,20 @@ def _send_mail(self, message=None, mail_server_id=None, smtp_server=None): send_from, send_to, message_string = connect().sendmail.call_args[0] return message_from_string(message_string) + def test_send_email_with_smtp_from(self): + """It should inject the FROM header correctly when no canonical name. + """ + self.mail_server_wo_whitelist.smtp_from = "derp@example.com" + self.message.replace_header("From", "test@example.com") + message = self._send_mail() + self.assertEqual(message["From"], "derp@example.com") + def test_send_email_injects_from_no_canonical(self): """It should inject the FROM header correctly when no canonical name. """ self.message.replace_header("From", "test@example.com") message = self._send_mail() - self.assertEqual(message["From"], self.email_from) + self.assertEqual(message["From"], "test@example.com") def test_send_email_injects_from_with_canonical(self): """It should inject the FROM header correctly with a canonical name. @@ -115,11 +121,28 @@ def test_send_email_injects_from_with_canonical(self): # Also check passing mail_server_id mail_server_id = self.Model.sudo().search([], order="sequence", limit=1)[0].id message = self._send_mail(mail_server_id=mail_server_id) - self.assertEqual(message["From"], '"{}" <{}>'.format(user, self.email_from)) + self.assertEqual(message["From"], 'Test < User ') self.assertEqual( - message["Return-Path"], '"{}" <{}>'.format(user, self.email_from) + message["Return-Path"], 'Test < User ' ) + def test_send_email_injects_from_with_bad_canonical(self): + """It should inject the FROM header correctly with a canonical name. + + Note that there is an extra `,` in the canonical name to test for + proper handling in the split. """ + user = "Test, User" + self.message.replace_header("From", "%s " % user) + message = self._send_mail() + self.assertEqual(message["From"], 'Test, User ') + + def test_send_email_injects_from_with_bad_email_from(self): + """It will raise and exception because not valid FROM """ + user = "Test, User" + self.message.replace_header("From", "%s test@example.com" % user) + with self.assertRaisesRegex(UserError, "'Invalid email address"): + self._send_mail() + def test_01_from_outgoing_server_domainone(self): self._init_mail_server_domain_whilelist_based() domain = "domainone.com" @@ -186,7 +209,9 @@ def test_04_from_outgoing_server_none_use_config(self): self._delete_mail_servers() - # Find config values + tools.config["smtp_from"] = "domainfive.com" + tools.config["smtp_domain_whitelist"] = "domainfive.com" + config_smtp_from = tools.config.get("smtp_from") config_smtp_domain_whitelist = tools.config.get("smtp_domain_whitelist") if not config_smtp_from or not config_smtp_domain_whitelist: @@ -208,6 +233,9 @@ def test_04_from_outgoing_server_none_use_config(self): def test_05_from_outgoing_server_none_same_domain(self): self._init_mail_server_domain_whilelist_based() + tools.config["smtp_from"] = "info@domainfive.com" + tools.config["smtp_domain_whitelist"] = "domainfive.com" + # Find config values config_smtp_from = tools.config.get("smtp_from") config_smtp_domain_whitelist = domain = tools.config.get( @@ -271,7 +299,7 @@ def test_07_from_outgoing_server_multidomain_1(self): def test_08_from_outgoing_server_multidomain_3(self): self._init_mail_server_domain_whilelist_based() - domain = "domainmulti.com" + domain = "domainfour.com" email_from = "test@%s" % domain expected_mail_server = self.mail_server_domainthree