5 files changed, 80 insertions, 19 deletions

diff --git a/src/mailman/rules/data/__init__.py b/src/mailman/rules/data/__init__.py
new file mode 100644
index 000000000..e69de29bb
--- /dev/null
+++ b/src/mailman/rules/data/__init__.py
diff --git a/src/mailman/rules/data/org_domain b/src/mailman/rules/data/org_domain
new file mode 100644
index 000000000..4e6f7816e
--- /dev/null
+++ b/src/mailman/rules/data/org_domain
@@ -0,0 +1,23 @@
+// Test data for dmarc organizational domains.
+
+// There are just a few domain entries here for testing purposes.
+
+// biz : https://en.wikipedia.org/wiki/.biz
+biz
+
+// com : https://en.wikipedia.org/wiki/.com
+com
+
+// org : https://en.wikipedia.org/wiki/.org
+org
+
+// jp geographic type names
+// http://jprs.jp/doc/rule/saisoku-1.html
+*.kobe.jp
+!city.kobe.jp
+
+// A line with a trailing comment for testing
+co.uk  some nonesence
+
+// A line with leading white space (ignored)
+  example.biz
diff --git a/src/mailman/rules/dmarc.py b/src/mailman/rules/dmarc.py
index 26f1183e6..a94d28889 100644
--- a/src/mailman/rules/dmarc.py
+++ b/src/mailman/rules/dmarc.py
@@ -51,8 +51,11 @@ def _get_suffixes(url):
         elog.error('Unable to retrieve data from %s: %s', url, e.reason)
         return
     for line in d.readlines():
-        line = str(line, encoding='utf-8').strip()
-        if not line or line.startswith(' ') or line.startswith('//'):
+        line = str(line, encoding='utf-8')
+        if not line.strip() or line.startswith('//'):
+            continue
+        line = re.sub('\s.*', '', line)
+        if not line:
             continue
         parts = line.lower().split('.')
         if parts[0].startswith('!'):
diff --git a/src/mailman/rules/docs/dmarc-mitigation.rst b/src/mailman/rules/docs/dmarc-mitigation.rst
index 7118a5be4..9a6fa5b9b 100644
--- a/src/mailman/rules/docs/dmarc-mitigation.rst
+++ b/src/mailman/rules/docs/dmarc-mitigation.rst
@@ -20,6 +20,12 @@ This returns p=reject for the example.biz domain and not for any others.
     >>> from mailman.rules.tests.test_dmarc import get_dns_resolver
     >>> patcher = get_dns_resolver()
 
+And we do a similar thing to mock the organizational domain data.
+
+    >>> from mailman.rules.tests.test_dmarc import get_org_data
+    >>> patcher2 = get_org_data()
+
+
 A message From: a domain without a DMARC policy does not set any flags.
 
     >>> from mailman.interfaces.mailinglist import DMARCMitigateAction
@@ -31,7 +37,7 @@ A message From: a domain without a DMARC policy does not set any flags.
     ...
     ... """)
     >>> msgdata = {}
-    >>> with patcher as Resolver:
+    >>> with patcher as Resolver, patcher2 as urlopen:
     ...     rule.check(mlist, msg, msgdata)
     False
     >>> msgdata == {}
@@ -48,7 +54,7 @@ action is no_mitigation.
     ...
     ... """)
     >>> msgdata = {}
-    >>> with patcher as Resolver:
+    >>> with patcher as Resolver, patcher2 as urlopen:
     ...     rule.check(mlist, msg, msgdata)
     False
     >>> msgdata == {}
@@ -64,7 +70,7 @@ But with a different list setting, the message is flagged.
     ...
     ... """)
     >>> msgdata = {}
-    >>> with patcher as Resolver:
+    >>> with patcher as Resolver, patcher2 as urlopen:
     ...     rule.check(mlist, msg, msgdata)
     False
     >>> msgdata['dmarc']
@@ -79,7 +85,7 @@ Subdomains which don't have a policy will check the organizational domain.
     ...
     ... """)
     >>> msgdata = {}
-    >>> with patcher as Resolver:
+    >>> with patcher as Resolver, patcher2 as urlopen:
     ...     rule.check(mlist, msg, msgdata)
     False
     >>> msgdata['dmarc']
@@ -97,7 +103,7 @@ message.
     ...
     ... """)
     >>> msgdata = {}
-    >>> with patcher as Resolver:
+    >>> with patcher as Resolver, patcher2 as urlopen:
     ...     rule.check(mlist, msg, msgdata)
     True
     >>> msgdata['dmarc']
@@ -116,7 +122,7 @@ We can reject the message with a default reason.
     ...
     ... """)
     >>> msgdata = {}
-    >>> with patcher as Resolver:
+    >>> with patcher as Resolver, patcher2 as urlopen:
     ...     rule.check(mlist, msg, msgdata)
     True
     >>> msgdata['dmarc']
@@ -137,7 +143,7 @@ And, we can reject with a custom message.
     ...
     ... """)
     >>> msgdata = {}
-    >>> with patcher as Resolver:
+    >>> with patcher as Resolver, patcher2 as urlopen:
     ...     rule.check(mlist, msg, msgdata)
     True
     >>> msgdata['dmarc']
diff --git a/src/mailman/rules/tests/test_dmarc.py b/src/mailman/rules/tests/test_dmarc.py
index 13b9d5173..63520156b 100644
--- a/src/mailman/rules/tests/test_dmarc.py
+++ b/src/mailman/rules/tests/test_dmarc.py
@@ -18,6 +18,8 @@
 """Provides support for mocking dnspython calls from dmarc rules and some
 organizational domain tests."""
 
+import os
+
 from contextlib import ExitStack
 from dns.rdatatype import TXT
 from dns.resolver import NXDOMAIN, NoAnswer
@@ -31,6 +33,7 @@ from public import public
 from unittest import TestCase
 from unittest.mock import patch
 from urllib.error import URLError
+from urllib.request import urlopen
 
 
 @public
@@ -91,6 +94,28 @@ def get_dns_resolver():
     return patcher
 
 
+@public
+def get_org_data():
+    """Create a mock to load the organizational domain data from our local
+    test data.
+    """
+    class oururlopen:
+        def __init__(self):
+            pass
+
+        def open(url):
+            datapath = os.path.join(
+                os.path.split(dmarc.__file__)[0],
+                'data',
+                'org_domain',
+                )
+            org_data_url = 'file:///{}'.format(datapath)
+            # Ensure we load the data.
+            dmarc.s_dict.clear()
+            return urlopen(org_data_url)
+    return patch('mailman.rules.dmarc.request.urlopen', oururlopen.open)
+
+
 class TestDMARCRules(TestCase):
     """Test organizational domain determination."""
 
@@ -109,19 +134,22 @@ class TestDMARCRules(TestCase):
         self.assertEqual(len(self.cache), 0)
 
     def test_no_data_for_domain(self):
-        self.assertEqual(
-            dmarc._get_org_dom('sub.dom.example.nxtld'),
-            'example.nxtld')
+        with get_org_data() as urlopen:    # noqa  F841
+            self.assertEqual(
+                dmarc._get_org_dom('sub.dom.example.nxtld'),
+                'example.nxtld')
 
     def test_domain_with_wild_card(self):
-        self.assertEqual(
-            dmarc._get_org_dom('ssub.sub.foo.kobe.jp'),
-            'sub.foo.kobe.jp')
+        with get_org_data() as urlopen:    # noqa  F841
+            self.assertEqual(
+                dmarc._get_org_dom('ssub.sub.foo.kobe.jp'),
+                'sub.foo.kobe.jp')
 
     def test_exception_to_wild_card(self):
-        self.assertEqual(
-            dmarc._get_org_dom('ssub.sub.city.kobe.jp'),
-            'city.kobe.jp')
+        with get_org_data() as urlopen:    # noqa  F841
+            self.assertEqual(
+                dmarc._get_org_dom('ssub.sub.city.kobe.jp'),
+                'city.kobe.jp')
 
     def test_no_publicsuffix_dot_org(self):
         mark = LogFileMark('mailman.error')
@@ -139,7 +167,8 @@ class TestDMARCRules(TestCase):
     def test_no_at_sign_in_from_address(self):
         # If there's no @ sign in the From: address, the rule can't hit.
         mlist = create_list('ant@example.com')
-        mlist.dmarc_mitigate_action = DMARCMitigateAction.munge_from
+        # Use action reject.  The rule only hits on reject and discard.
+        mlist.dmarc_mitigate_action = DMARCMitigateAction.reject
         msg = mfs("""\
 From: anne
 To: ant@example.com