2 files changed, 76 insertions, 49 deletions

diff --git a/src/mailman/rules/moderation.py b/src/mailman/rules/moderation.py
index 355118857..3f10e31bf 100644
--- a/src/mailman/rules/moderation.py
+++ b/src/mailman/rules/moderation.py
@@ -41,21 +41,33 @@ class MemberModeration:
 
     def check(self, mlist, msg, msgdata):
         """See `IRule`."""
+        # The MemberModeration rule misses unconditionally if any of the
+        # senders are banned.
+        ban_manager = IBanManager(mlist)
         user_manager = getUtility(IUserManager)
-        # The MemberModeration rule is unconditionally false if any of
-        # the senders are banned.
         for sender in msg.senders:
-            if IBanManager(mlist).is_banned(sender):
+            if ban_manager.is_banned(sender):
                 return False
+        # For each sender address, try to find a member associated with the
+        # email address.  Start by checking the sender email directly.  If the
+        # sender email is not a member, try to find the user linked to the
+        # email, and then check to see if *that* user, or any of the addresses
+        # linked to that user is a member.  This rule hits of we find a member
+        # and their moderation action is not to defer.
         for sender in msg.senders:
-            # Check for subscribed members linked to the address.
+            # Is the sender email itself a member?
             member = mlist.members.get_member(sender)
             if member is None:
+                # Is the sender email linked to a user?
                 user = user_manager.get_user(sender)
                 if user is not None:
+                    # Are any of the emails linked to this user a member?
                     for address in user.addresses:
-                        if mlist.members.get_member(address.email) is not None:
-                            member = mlist.members.get_member(address.email)
+                        member = mlist.members.get_member(address.email)
+                        if member is not None:
+                            # We found a member, so we don't need to check any
+                            # of the other linked addresses.
+                            break
             if member is None:
                 return False
             action = (mlist.default_member_action
@@ -65,8 +77,8 @@ class MemberModeration:
                 # The regular moderation rules apply.
                 return False
             elif action is not None:
-                # We must stringify the moderation action so that
-                # it can be stored in the pending request table.
+                # We must stringify the moderation action so that it can be
+                # stored in the pending request table.
                 msgdata['moderation_action'] = action.name
                 msgdata['moderation_sender'] = sender
                 msgdata.setdefault('moderation_reasons', []).append(
@@ -94,30 +106,37 @@ class NonmemberModeration:
 
     def check(self, mlist, msg, msgdata):
         """See `IRule`."""
+        ban_manager = IBanManager(mlist)
         user_manager = getUtility(IUserManager)
-        # Initial check. If any of the senders are banned, we bail.
+        # The NonmemberModeration rule misses unconditionally if any of the
+        # senders are banned.
         for sender in msg.senders:
-            if IBanManager(mlist).is_banned(sender):
+            if ban_manager.is_banned(sender):
                 return False
-        # First ensure that all senders are already either members or
-        # nonmembers.  If they are not subscribed in some role to the mailing
-        # list, make them nonmembers.
-        # Maintain a record of which senders have linked subscribed users.
+        # Every sender must somehow be a member or nonmember.  The sender
+        # email can have one of those roles directly, or a user that the email
+        # is linked to can have one of those roles indirectly, or any address
+        # linked to one of those users can have one of those roles.
+        #
+        # If the sender is not somehow a member or nonmember, make them a
+        # nonmember.  We maintain a record of which senders are members, and
+        # then the ones that aren't are made nonmembers.
         found_linked_membership = set()
         for sender in msg.senders:
             member = mlist.members.get_member(sender)
-            if member is not None:
-                found_linked_membership.add(sender)
-            else:
+            if member is None:
                 user = user_manager.get_user(sender)
                 if user is not None:
                     for address in user.addresses:
                         if mlist.members.get_member(address.email) is not None:
                             found_linked_membership.add(sender)
-            if (mlist.nonmembers.get_member(sender) is None and
-                    sender not in found_linked_membership):
-                # The address is neither a member nor nonmember
-                # and has no linked subscribed user.
+            else:
+                found_linked_membership.add(sender)
+            # Now we know whether the sender is somehow linked to a member or
+            # not.  If not, and the email also isn't already a nonmember, make
+            # them a nonmember.
+            if (mlist.nonmembers.get_member(sender) is None
+                    and sender not in found_linked_membership):   # noqa
                 address = user_manager.get_address(sender)
                 assert address is not None, (
                     'Posting address is not registered: {}'.format(sender))
diff --git a/src/mailman/rules/tests/test_moderation.py b/src/mailman/rules/tests/test_moderation.py
index 7da07b200..197bb6c7d 100644
--- a/src/mailman/rules/tests/test_moderation.py
+++ b/src/mailman/rules/tests/test_moderation.py
@@ -210,15 +210,19 @@ A message body.
         self.assertTrue(result)
         self.assertEqual(msgdata.get('moderation_action'), 'hold')
 
-    def test_linked_address_nonmembermoderation(self):
+    def test_linked_address_nonmembermoderation_misses(self):
+        # Anne subscribes to a mailing list as a user with her preferred
+        # address.  She also has a secondary linked address, and she uses this
+        # to post to the mailing list.  The NonmemberModeration rule misses
+        # because Anne is not a nonmember.
         user_manager = getUtility(IUserManager)
         anne = user_manager.create_user('anne@example.com')
         set_preferred(anne)
         self._mlist.subscribe(anne, MemberRole.member)
-        anne.link(user_manager.create_address('anne2@example.com'))
+        anne.link(user_manager.create_address('anne.person@example.com'))
         rule = moderation.NonmemberModeration()
         msg = mfs("""\
-From: anne2@example.com
+From: anne.person@example.com
 To: test@example.com
 Subject: A test message
 Message-ID: <ant>
@@ -226,22 +230,23 @@ MIME-Version: 1.0
 
 A message body.
 """)
-        # The NonmemberModeration rule should evaluate
-        # to False since the linked user is subscribed to the list
-        msgdata = {}
-        result = rule.check(self._mlist, msg, msgdata)
+        result = rule.check(self._mlist, msg, {})
         self.assertFalse(result)
 
-    def test_linked_address_membermoderation(self):
+    def test_linked_address_membermoderation_hits(self):
+        # Anne subscribes to a mailing list as a user with her preferred
+        # address.  She also has a secondary linked address, and she uses this
+        # to post to the mailing list.  The MemberModeration rule hits because
+        # Anne is a member.
         self._mlist.default_member_action = Action.accept
         user_manager = getUtility(IUserManager)
         anne = user_manager.create_user('anne@example.com')
         set_preferred(anne)
         self._mlist.subscribe(anne, MemberRole.member)
-        anne.link(user_manager.create_address('anne2@example.com'))
+        anne.link(user_manager.create_address('anne.person@example.com'))
         rule = moderation.MemberModeration()
         msg = mfs("""\
-From: anne2@example.com
+From: anne.person@example.com
 To: test@example.com
 Subject: A test message
 Message-ID: <ant>
@@ -249,22 +254,23 @@ MIME-Version: 1.0
 
 A message body.
 """)
-        # The MemberModeration rule should evaluate
-        # to True since the linked user is subscribed to the list
-        msgdata = {}
-        result = rule.check(self._mlist, msg, msgdata)
+        result = rule.check(self._mlist, msg, {})
         self.assertTrue(result)
 
     def test_banned_address_linked_to_user(self):
+        # Anne is subscribed to a mailing list as a user with her preferred
+        # address.  She also has a secondary address which is banned and which
+        # she uses to post to the mailing list.  Both the MemberModeration and
+        # NonmemberModeration rules miss because the posting address is
+        # banned.
         user_manager = getUtility(IUserManager)
         anne = user_manager.create_user('anne@example.com')
         set_preferred(anne)
         self._mlist.subscribe(anne, MemberRole.member)
-        anne.link(user_manager.create_address('anne2@example.com'))
-        IBanManager(self._mlist).ban('anne2@example.com')
-        rule = moderation.MemberModeration()
+        anne.link(user_manager.create_address('anne.person@example.com'))
+        IBanManager(self._mlist).ban('anne.person@example.com')
         msg = mfs("""\
-From: anne2@example.com
+From: anne.person@example.com
 To: test@example.com
 Subject: A test message
 Message-ID: <ant>
@@ -272,22 +278,25 @@ MIME-Version: 1.0
 
 A message body.
 """)
-        msgdata = {}
-        result = rule.check(self._mlist, msg, msgdata)
+        rule = moderation.MemberModeration()
+        result = rule.check(self._mlist, msg, {})
         self.assertFalse(result)
         rule = moderation.NonmemberModeration()
-        result = rule.check(self._mlist, msg, msgdata)
+        result = rule.check(self._mlist, msg, {})
         self.assertFalse(result)
 
     def test_banned_sender_among_multiple_senders(self):
+        # Two addresses are created, one of which is banned.  Even though the
+        # The Nonmember moderation rule misses if any of the banned addresses
+        # appear in the 'senders' headers of the message.
         user_manager = getUtility(IUserManager)
-        user_manager.create_address('ted@example.com')
-        user_manager.create_address('cris@example.com')
-        IBanManager(self._mlist).ban('cris@example.com')
+        user_manager.create_address('anne@example.com')
+        user_manager.create_address('bart@example.com')
+        IBanManager(self._mlist).ban('bart@example.com')
         rule = moderation.NonmemberModeration()
         msg = mfs("""\
-From: ted@example.com
-Sender: cris@example.com
+From: anne@example.com
+Sender: bart@example.com
 To: test@example.com
 Subject: A test message
 Message-ID: <ant>
@@ -295,6 +304,5 @@ MIME-Version: 1.0
 
 A message body.
 """)
-        msgdata = {}
-        result = rule.check(self._mlist, msg, msgdata)
+        result = rule.check(self._mlist, msg, {})
         self.assertFalse(result)