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.

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."""

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')),
             ],

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)()
 

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")

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):

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
 
 

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