From 08360400a9d9d8062a53d62b0630b2f527cf0f78 Mon Sep 17 00:00:00 2001 From: Antoine Nguyen Date: Fri, 25 Sep 2020 21:27:24 +0200 Subject: [PATCH] Improved notifications management in MX checks. (#2015) * Improved notifications management in MX checks. see #1820 * Increased alarm internal code length. * Force greenlet 0.4.16 due to a binary issue with 0.4.17. * Improved coverage. --- .../management/commands/subcommands/_mx.py | 136 +++++++++++------- modoboa/admin/migrations/0017_alarm.py | 2 +- modoboa/admin/models/alarm.py | 2 +- modoboa/admin/tests/test_mx.py | 30 +++- modoboa/admin/tests/utils.py | 8 +- modoboa/policyd/core.py | 14 +- requirements.txt | 1 + 7 files changed, 130 insertions(+), 63 deletions(-) diff --git a/modoboa/admin/management/commands/subcommands/_mx.py b/modoboa/admin/management/commands/subcommands/_mx.py index 93189a140..964643d18 100644 --- a/modoboa/admin/management/commands/subcommands/_mx.py +++ b/modoboa/admin/management/commands/subcommands/_mx.py @@ -6,10 +6,10 @@ import gevent from gevent import socket from django.conf import settings -from django.core import mail from django.core.mail import EmailMessage from django.core.management.base import BaseCommand from django.template.loader import render_to_string +from django.utils import timezone from django.utils.encoding import smart_text from django.utils.functional import cached_property from django.utils.translation import ugettext as _ @@ -71,7 +71,7 @@ class CheckMXRecords(BaseCommand): for ip, mxs in mx_list.items(): try: ip = ipaddress.ip_address(smart_text(ip)) - except ValueError as e: + except ValueError: continue else: delim = "." if ip.version == 4 else ":" @@ -86,8 +86,11 @@ class CheckMXRecords(BaseCommand): return provider, results def store_dnsbl_result(self, domain, provider, results, **options): - """Store DNSBL provider results for domain.""" - alerts = {} + """Store DNSBL provider results for domain. + + Return a list of alerts. + """ + alerts = [] to_create = [] for mx, result in list(results.items()): if not result: @@ -108,13 +111,29 @@ class CheckMXRecords(BaseCommand): dnsbl_result.save() if not dnsbl_result.status and result: trigger = True + alarm_name = "domain_mx_in_dnsbl_{}".format(provider) if trigger: - if domain not in alerts: - alerts[domain] = [] - alerts[domain].append((provider, mx)) + title = _("MX {} listed by DNSBL provider {}").format( + mx.name, provider) + domain.alarms.create( + internal_name=alarm_name, + status=constants.ALARM_OPENED, + title=title + ) + alerts.append((provider, mx.name)) + else: + domain.alarms.filter( + internal_name=alarm_name, + status=constants.ALARM_OPENED + ).update( + status=constants.ALARM_CLOSED, + closed=timezone.now() + ) models.DNSBLResult.objects.bulk_create(to_create) - if not alerts: - return + return alerts + + def send_alert_notifications(self, domain, alerts, subject, tpl, **options): + """Send email notifications about given alerts.""" emails = list(options["email"]) if not options["skip_admin_emails"]: emails.extend( @@ -123,19 +142,9 @@ class CheckMXRecords(BaseCommand): ) if not len(emails): return - with mail.get_connection() as connection: - for domain, providers in list(alerts.items()): - content = render_to_string( - "admin/notifications/domain_in_dnsbl.html", { - "domain": domain, "alerts": providers - }) - subject = _("[modoboa] DNSBL issue(s) for domain {}").format( - domain.name) - msg = EmailMessage( - subject, content.strip(), self.sender, emails, - connection=connection - ) - msg.send() + content = render_to_string(tpl, {"domain": domain, "alerts": alerts}) + msg = EmailMessage(subject, content.strip(), self.sender, emails) + msg.send() def check_valid_mx(self, domain, mx_list, **options): """Check that domain's MX record exist. @@ -149,38 +158,57 @@ class CheckMXRecords(BaseCommand): for mx in mx_list] valid_mxs = self.valid_mxs if not mxs: - alerts.append(_("Domain {} has no MX record").format(domain)) - elif valid_mxs: - for subnet in valid_mxs: - for mx, addr in mxs: - if addr in subnet: - mx.managed = check = True - mx.save() - if check is False: - mx_names = [ - "{0.name} ({0.address})".format(mx) for mx in mx_list] + alarm, created = domain.alarms.get_or_create( + internal_name="domain_has_no_mx", + status=constants.ALARM_OPENED, + defaults={"title": _("Domain has no MX record")} + ) + if created: alerts.append( - _("MX record for domain {0} is invalid: {1}").format( - domain, ", ".join(mx_names)) - ) + _("Domain {} has no MX record").format(domain.name)) + else: + # Close opened alarm + domain.alarms.filter( + internal_name="domain_has_no_mx", + status=constants.ALARM_OPENED).update( + status=constants.ALARM_CLOSED, closed=timezone.now()) + if valid_mxs: + for subnet in valid_mxs: + for mx, addr in mxs: + if addr in subnet: + mx.managed = check = True + mx.save() + if check is False: + mx_names = [ + "{0.name} ({0.address})".format(mx) for mx in mx_list] + alarm, created = domain.alarms.get_or_create( + internal_name="domain_invalid_mx", + status=constants.ALARM_OPENED, + defaults={ + "title": _("Invalid MX record: {}").format( + ", ".join(mx_names)) + } + ) + if created: + alerts.append( + _("MX record for domain {0} is invalid: {1}") + .format(domain, ", ".join(mx_names)) + ) + else: + # Close opened alarm + domain.alarms.filter( + internal_name="domain_invalid_mx", + status=constants.ALARM_OPENED + ).update( + status=constants.ALARM_CLOSED, + closed=timezone.now() + ) if not alerts: return - emails = list(options["email"]) - if not options["skip_admin_emails"]: - emails.extend( - domain.admins.exclude(mailbox__isnull=True) - .values_list("email", flat=True) - ) - if not len(emails): - return - content = render_to_string( - "admin/notifications/domain_invalid_mx.html", { - "domain": domain, "alerts": alerts - }) subject = _("[modoboa] MX issue(s) for domain {}").format( domain.name) - msg = EmailMessage(subject, content.strip(), self.sender, emails) - msg.send() + tpl = "admin/notifications/domain_invalid_mx.html" + self.send_alert_notifications(domain, alerts, subject, tpl, **options) def check_domain(self, domain, timeout=3, ttl=7200, **options): """Check specified domain.""" @@ -226,11 +254,19 @@ class CheckMXRecords(BaseCommand): gevent.spawn(self.query_dnsbl, mx_by_ip, provider) for provider in self.providers] gevent.joinall(jobs, timeout) + alerts = [] for job in jobs: if not job.successful(): continue provider, results = job.value - self.store_dnsbl_result(domain, provider, results, **options) + alerts += self.store_dnsbl_result( + domain, provider, results, **options) + if not alerts: + return + subject = _("[modoboa] DNSBL issue(s) for domain {}").format( + domain.name) + tpl = "admin/notifications/domain_in_dnsbl.html" + self.send_alert_notifications(domain, alerts, subject, tpl, **options) def handle(self, *args, **options): """Command entry point.""" diff --git a/modoboa/admin/migrations/0017_alarm.py b/modoboa/admin/migrations/0017_alarm.py index 91afe96eb..ea955c897 100644 --- a/modoboa/admin/migrations/0017_alarm.py +++ b/modoboa/admin/migrations/0017_alarm.py @@ -20,7 +20,7 @@ class Migration(migrations.Migration): ('closed', models.DateTimeField(blank=True, null=True)), ('status', models.IntegerField(choices=[(1, 'Opened'), (2, 'Closed')], db_index=True, default=1)), ('title', models.CharField(max_length=150)), - ('internal_name', models.CharField(max_length=15)), + ('internal_name', models.CharField(max_length=120)), ('domain', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='alarms', to='admin.Domain')), ('mailbox', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='alarms', to='admin.Mailbox')), ], diff --git a/modoboa/admin/models/alarm.py b/modoboa/admin/models/alarm.py index e99c74fd4..12ea82d39 100644 --- a/modoboa/admin/models/alarm.py +++ b/modoboa/admin/models/alarm.py @@ -34,7 +34,7 @@ class Alarm(models.Model): ) title = models.CharField(max_length=150) - internal_name = models.CharField(max_length=15) + internal_name = models.CharField(max_length=120) objects = models.Manager.from_queryset(AlarmQuerySet)() diff --git a/modoboa/admin/tests/test_mx.py b/modoboa/admin/tests/test_mx.py index 32e7119f4..1c8cf36ec 100644 --- a/modoboa/admin/tests/test_mx.py +++ b/modoboa/admin/tests/test_mx.py @@ -99,6 +99,31 @@ class MXTestCase(ModoTestCase): management.call_command( "modo", "check_mx", "--domain", "toto.com") + @mock.patch("gevent.socket.gethostbyname") + @mock.patch("socket.getaddrinfo") + @mock.patch.object(dns.resolver.Resolver, "query") + def test_invalid_mx( + self, mock_query, mock_getaddrinfo, mock_g_gethostbyname): + """Test to check if invalid MX records are detected.""" + mock_query.side_effect = utils.mock_dns_query_result + mock_getaddrinfo.side_effect = utils.mock_ip_query_result + mock_g_gethostbyname.return_value = "1.2.3.4" + domain = factories.DomainFactory(name="invalid-mx.com") + # Add domain admin with mailbox + mb = factories.MailboxFactory( + address="admin", domain=domain, + user__username="admin@{}".format(domain.name), + user__groups=("DomainAdmins", ) + ) + domain.add_admin(mb.user) + management.call_command( + "modo", "check_mx", "--domain", domain.name, "--no-dnsbl") + self.assertEqual(domain.alarms.count(), 1) + management.call_command( + "modo", "check_mx", "--domain", domain.name, "--no-dnsbl") + self.assertEqual(domain.alarms.count(), 1) + self.assertEqual(len(mail.outbox), 1) + @mock.patch("socket.getaddrinfo") @mock.patch.object(dns.resolver.Resolver, "query") def test_get_mx_list_dns_server(self, mock_query, mock_getaddrinfo): @@ -154,7 +179,7 @@ class DNSBLTestCase(ModoTestCase): @classmethod def setUpTestData(cls): # NOQA:N802 """Create some data.""" - super(DNSBLTestCase, cls).setUpTestData() + super().setUpTestData() cls.domain = factories.DomainFactory(name="modoboa.org") factories.DomainFactory(name="does-not-exist.example.com") cls.domain2 = factories.DomainFactory( @@ -195,9 +220,6 @@ class DNSBLTestCase(ModoTestCase): mock_g_gethostbyname.return_value = "1.2.3.4" management.call_command( "modo", "check_mx", "--email", "user@example.test") - if len(mail.outbox) != 2: - for message in mail.outbox: - print(message.subject) self.assertEqual(len(mail.outbox), 2) @mock.patch("gevent.socket.gethostbyname") diff --git a/modoboa/admin/tests/utils.py b/modoboa/admin/tests/utils.py index e85f3c41d..b15939d30 100644 --- a/modoboa/admin/tests/utils.py +++ b/modoboa/admin/tests/utils.py @@ -24,6 +24,7 @@ class RRsetInvalid(object): _A_RECORD = A("IN", "A", "1.2.3.4") _MX_RECORD_1 = MX("IN", "MX", 10, Name("mx.example.com".split("."))) _MX_RECORD_2 = MX("IN", "MX", 10, Name("mx2.example.com".split("."))) +_MX_RECORD_3 = MX("IN", "MX", 10, Name("mx3.example.com".split("."))) _SPF_RECORD = TXT("IN", "TXT", ["v=spf1 mx -all"]) _DMARC_RECORD = TXT("IN", "TXT", ["v=DMARC1 p=reject"]) _DKIM_RECORD = TXT("IN", "TXT", ["v=DKIM1 p=XXXXX"]) @@ -49,6 +50,7 @@ _POSSIBLE_DNS_RESULTS = { "no-lookup.example.com": [_DNE_MX_RECORD], "no-answer.example.com": [_DNE_MX_RECORD], "dns-checks.com": [_MX_RECORD_2], + "invalid-mx.com": [_MX_RECORD_3] } _POSSIBLE_DNS_RESULTS_NO_MX = { @@ -62,6 +64,7 @@ _POSSIBLE_DNS_RESULTS_NO_MX = { "_dmarc.dns-checks.com": [_DMARC_RECORD, ], "autoconfig.dns-checks.com": [_A_RECORD], "autodiscover.dns-checks.com": [_A_RECORD], + "mx3.example.com": [_A_RECORD] } _POSSIBLE_IP_RESULTS = { "test3.com": [_IPV4_RECORD_2], @@ -79,10 +82,9 @@ def mock_dns_query_result(qname, *args, **kwargs): if qname in _POSSIBLE_DNS_RESULTS: return get_mock_dns_query_response(_POSSIBLE_DNS_RESULTS, qname) return _MX_RECORDS - elif qname in _POSSIBLE_DNS_RESULTS_NO_MX: + if qname in _POSSIBLE_DNS_RESULTS_NO_MX: return get_mock_dns_query_response(_POSSIBLE_DNS_RESULTS_NO_MX, qname) - else: - return _IP_SET_RECORDS + return _IP_SET_RECORDS def get_mock_dns_query_response(responses, attributeName): diff --git a/modoboa/policyd/core.py b/modoboa/policyd/core.py index 110514893..917d98234 100644 --- a/modoboa/policyd/core.py +++ b/modoboa/policyd/core.py @@ -225,8 +225,11 @@ def get_domains_to_reset(): """ qset = admin_models.Domain.objects.filter(message_limit__isnull=False) admin_models.Alarm.objects.filter( - internal_name="limit_reached", domain__in=qset).update( - status=admin_constants.ALARM_CLOSED, closed=timezone.now()) + internal_name="limit_reached", domain__in=qset, + status=admin_constants.ALARM_OPENED + ).update( + status=admin_constants.ALARM_CLOSED, closed=timezone.now() + ) return qset @@ -242,8 +245,11 @@ def get_mailboxes_to_reset(): .select_related("domain") ) admin_models.Alarm.objects.filter( - internal_name="limit_reached", mailbox__in=qset).update( - status=admin_constants.ALARM_CLOSED, closed=timezone.now()) + internal_name="limit_reached", mailbox__in=qset, + status=admin_constants.ALARM_OPENED + ).update( + status=admin_constants.ALARM_CLOSED, closed=timezone.now() + ) return qset diff --git a/requirements.txt b/requirements.txt index 5178fdaa1..8f0be3bb1 100644 --- a/requirements.txt +++ b/requirements.txt @@ -15,6 +15,7 @@ bcrypt # Requires libffi-dev and python-dev dnspython==1.16.0 feedparser==5.2.1 +greenlet==0.4.16 gevent==1.5.0 jsonfield==2.0.2 Pillow # Optional dependency for django-ckeditor