6 files changed, 102 insertions, 61 deletions

diff --git a/src/mailman/database/alembic/versions/3002bac0c25a_dmarc_attributes.py b/src/mailman/database/alembic/versions/3002bac0c25a_dmarc_attributes.py
index b0c2297a0..06fcecf0a 100644
--- a/src/mailman/database/alembic/versions/3002bac0c25a_dmarc_attributes.py
+++ b/src/mailman/database/alembic/versions/3002bac0c25a_dmarc_attributes.py
@@ -10,7 +10,7 @@ import sqlalchemy as sa
 
 from alembic import op
 from mailman.database.helpers import exists_in_db
-from mailman.database.types import Enum, SAUnicode, SAUnicodeLarge
+from mailman.database.types import Enum, SAUnicode
 from mailman.interfaces.mailinglist import DMARCModerationAction, FromIsList
 
 
@@ -71,10 +71,6 @@ def upgrade():
         dmarc_wrapped_message_text=op.inline_literal(''),
         from_is_list=op.inline_literal(FromIsList.none),
         )))
-    # Adding another rule can make the rule Hits/Misses too long for MySQL
-    # SaUnicode.
-    with op.batch_alter_table('pendedkeyvalue') as batch_op:
-        batch_op.alter_column('value', type_=SAUnicodeLarge)
 
 
 def downgrade():
@@ -85,5 +81,3 @@ def downgrade():
         batch_op.drop_column('dmarc_moderation_notice')
         batch_op.drop_column('dmarc_wrapped_message_text')
         batch_op.drop_column('from_is_list')
-    with op.batch_alter_table('pendedkeyvalue') as batch_op:
-        batch_op.alter_column('value', type_=SAUnicode)
diff --git a/src/mailman/handlers/cook_headers.py b/src/mailman/handlers/cook_headers.py
index 672738bf9..413304ea2 100644
--- a/src/mailman/handlers/cook_headers.py
+++ b/src/mailman/handlers/cook_headers.py
@@ -114,7 +114,7 @@ def process(mlist, msg, msgdata):
             d[lcaddr] = pair
             new.append(pair)
         # List admin wants an explicit Reply-To: added
-        if mlist.reply_goes_to_list == ReplyToMunging.explicit_header:
+        if mlist.reply_goes_to_list is ReplyToMunging.explicit_header:
             add(parseaddr(mlist.reply_to_address))
         # If we're not first stripping existing Reply-To: then we need to add
         # the original Reply-To:'s to the list we're building up.  In both
diff --git a/src/mailman/handlers/dmarc.py b/src/mailman/handlers/dmarc.py
index e1f6f86b7..21b122990 100644
--- a/src/mailman/handlers/dmarc.py
+++ b/src/mailman/handlers/dmarc.py
@@ -15,7 +15,14 @@
 # You should have received a copy of the GNU General Public License along with
 # GNU Mailman.  If not, see <http://www.gnu.org/licenses/>.
 
-"""Do DMARC Munge From and Wrap Message actions."""
+"""Do DMARC Munge From and Wrap Message actions.
+
+This does the work of modifying the messages From: and Cc: or Reply-To: or
+wrapping the message in an outer message with From: and Cc: or Reply-To:
+as appropriate to avoid issues because of the original From: domain's DMARC
+policy.  It does this either to selected messages flagged by the DMARC
+moderation rule based on list settings and the original From: domain's DMARC
+policy or to all messages based on list settings."""
 
 import re
 import copy
@@ -42,19 +49,41 @@ NONASCII = re.compile('[^\s!-~]')
 # Headers from the original that we want to keep in the wrapper.  These are
 # actually regexps matched with re.match so they match anything that starts
 # with the given string unless they end with '$'.
-KEEPERS = ('archived-at',
-           'date',
-           'in-reply-to',
-           'list-',
-           'precedence',
-           'references',
-           'subject',
-           'to',
-           'x-mailman-',
-           )
+KEEPERS = (
+    'archived-at',
+    'date',
+    'in-reply-to',
+    'list-',
+    'precedence',
+    'references',
+    'subject',
+    'to',
+    'x-mailman-',
+    )
 
 
 def munged_headers(mlist, msg, msgdata):
+    # This returns a list of tuples (header, content) where header is the
+    # name of a header to be added to or replaced in the wrapper or message
+    # for DMARC mitigation.  It sets From: to the string
+    # 'original From: display name' via 'list name' <list posting address>
+    # and adds the original From: to Reply-To: or Cc: per the following.
+    # Our goals for this process are not completely compatible, so we do
+    # the best we can.  Our goals are:
+    # 1) as long as the list is not anonymous, the original From: address
+    #    should be obviously exposed, i.e. not just in a header that MUAs
+    #    don't display.
+    # 2) the original From: address should not be in a comment or display
+    #    name in the new From: because it is claimed that multiple domains
+    #    in any fields in From: are indicative of spamminess.  This means
+    #    it should be in Reply-To: or Cc:.
+    # 3) the behavior of an MUA doing a 'reply' or 'reply all' should be
+    #    consistent regardless of whether or not the From: is munged.
+    # Goal 3) implies sometimes the original From: should be in Reply-To:
+    # and sometimes in Cc:, and even so, this goal won't be achieved in
+    # all cases with all MUAs.  In cases of conflict, the above ordering of
+    # goals is priority order.
+    #
     # Be as robust as possible here.
     faddrs = getaddresses(msg.get_all('from', []))
     # Strip the nulls and bad emails.
@@ -67,13 +96,13 @@ def munged_headers(mlist, msg, msgdata):
         realname = ''
         email = msgdata['original_sender']
         o_from = (realname, email)
-    if not realname:
+    if len(realname) == 0:
         member = mlist.members.get_member(email)
         if member:
             realname = member.display_name or email
         else:
             realname = email
-    # Remove domain from realname if it looks like an email address
+    # Remove domain from realname if it looks like an email address.
     realname = re.sub(r'@([^ .]+\.)+[^ .]+$', '---', realname)
     # Make a display name and RFC 2047 encode it if necessary.  This is
     # difficult and kludgy. If the realname came from From: it should be
@@ -99,7 +128,7 @@ def munged_headers(mlist, msg, msgdata):
     dn = str(Header(via, mlist.preferred_language.charset))
     retn = [('From', formataddr((dn, mlist.posting_address)))]
     # We've made the munged From:.  Now put the original in Reply-To: or Cc:
-    if mlist.reply_goes_to_list == ReplyToMunging.no_munging:
+    if mlist.reply_goes_to_list is ReplyToMunging.no_munging:
         # Add original from to Reply-To:
         add_to = 'Reply-To'
     else:
@@ -108,7 +137,7 @@ def munged_headers(mlist, msg, msgdata):
     orig = getaddresses(msg.get_all(add_to, []))
     if o_from[1] not in [x[1] for x in orig]:
         orig.append(o_from)
-    retn.append((add_to, COMMASPACE.join([formataddr(x) for x in orig])))
+    retn.append((add_to, COMMASPACE.join(formataddr(x) for x in orig)))
     return retn
 
 
@@ -125,7 +154,7 @@ def wrap_message(mlist, msg, msgdata, dmarc_wrap=False):
     # make a copy of the msg, then delete almost everything and set/copy
     # what we want.
     omsg = copy.deepcopy(msg)
-    for key in msg.keys():
+    for key in msg:
         keep = False
         for keeper in KEEPERS:
             if re.match(keeper, key, re.I):
@@ -139,7 +168,7 @@ def wrap_message(mlist, msg, msgdata, dmarc_wrap=False):
         msg[k] = v
     # Are we including dmarc_wrapped_message_text?  I.e., do we have text and
     # are we wrapping because of dmarc_moderation_action?
-    if mlist.dmarc_wrapped_message_text and dmarc_wrap:
+    if len(mlist.dmarc_wrapped_message_text) > 0 and dmarc_wrap:
         part1 = MIMEText(wrap(mlist.dmarc_wrapped_message_text),
                          'plain',
                          mlist.preferred_language.charset)
@@ -158,31 +187,31 @@ def wrap_message(mlist, msg, msgdata, dmarc_wrap=False):
 def process(mlist, msg, msgdata):
     """Process DMARC actions."""
     if ((not msgdata.get('dmarc') or
-            mlist.dmarc_moderation_action == DMARCModerationAction.none) and
-            mlist.from_is_list == FromIsList.none):
+            mlist.dmarc_moderation_action is DMARCModerationAction.none) and
+            mlist.from_is_list is FromIsList.none):
         return
     if mlist.anonymous_list:
         # DMARC mitigation is not required for anonymous lists.
         return
-    if (mlist.dmarc_moderation_action != DMARCModerationAction.none and
+    if (mlist.dmarc_moderation_action is not DMARCModerationAction.none and
             msgdata.get('dmarc')):
-        if mlist.dmarc_moderation_action == DMARCModerationAction.munge_from:
+        if mlist.dmarc_moderation_action is DMARCModerationAction.munge_from:
             munge_from(mlist, msg, msgdata)
-        elif (mlist.dmarc_moderation_action ==
+        elif (mlist.dmarc_moderation_action is
                 DMARCModerationAction.wrap_message):
             wrap_message(mlist, msg, msgdata, dmarc_wrap=True)
         else:
-            assert False, (
-                'handlers/dmarc.py: dmarc_moderation_action = {0}'.format(
+            raise AssertionError(
+                'handlers/dmarc.py: dmarc_moderation_action = {}'.format(
                     mlist.dmarc_moderation_action))
     else:
-        if mlist.from_is_list == FromIsList.munge_from:
+        if mlist.from_is_list is FromIsList.munge_from:
             munge_from(mlist, msg, msgdata)
-        elif mlist.from_is_list == FromIsList.wrap_message:
+        elif mlist.from_is_list is FromIsList.wrap_message:
             wrap_message(mlist, msg, msgdata)
         else:
-            assert False, (
-                'handlers/dmarc.py: from_is_list = {0}'.format(
+            raise AssertionError(
+                'handlers/dmarc.py: from_is_list = {}'.format(
                     mlist.from_is_list))
 
 
diff --git a/src/mailman/handlers/docs/dmarc-mitigations.rst b/src/mailman/handlers/docs/dmarc-mitigations.rst
index ee7dd98e0..2f87ecedb 100644
--- a/src/mailman/handlers/docs/dmarc-mitigations.rst
+++ b/src/mailman/handlers/docs/dmarc-mitigations.rst
@@ -2,11 +2,11 @@
 DMARC Mitigations
 =================
 
-In order to mitigate the effects of DMARC on mailing list traffic, list
+In order to mitigate the effects of DMARC_ on mailing list traffic, list
 administrators have the ability to apply transformations to messages delivered
 to list members.  These transformations are applied only to individual messages
-sent to list members and not to messages in digests, archives or gated to
-usenet.
+sent to list members and not to messages in digests, archives, or gated to
+USENET.
 
 The messages can be transformed by either munging the From: header and putting
 original From: in Cc: or Reply-To: or by wrapping the original message in an
@@ -16,23 +16,35 @@ Exactly what transformations are applied depends on a number of list settings.
 
 The settings and their effects are:
 
- * anonymous_list: If True, no mitigations are ever applied because the message
+``anonymous_list``
+   If True, no mitigations are ever applied because the message
    is already From: the list.
- * dmarc_moderation_action: The action to apply to messages From: a domain
-   publishing a DMARC policy of reject and possibly quarantine or none.
- * dmarc_quarantine_moderation_action: A flag to apply dmarc_moderation_action
-   to messages From: a domain publishing a DMARC policy of quarantine.
- * dmarc_none_moderation_action: A flag to apply dmarc_moderation_action to
+``dmarc_moderation_action``
+   The action to apply to messages From: a domain
+   publishing a DMARC policy of reject and possibly quarantine or none
+   depending on the next two settings.
+``dmarc_quarantine_moderation_action``
+   A flag to apply the above dmarc_moderation_action
+   to messages From: a domain publishing a DMARC policy of quarantine in
+   addition to domains publishing a DMARC policy of reject.
+``dmarc_none_moderation_action``
+   A flag to apply the above dmarc_moderation_action to
    messages From: a domain publishing a DMARC policy of none, but only when
    dmarc_quarantine_moderation_action is also true.
- * dmarc_moderation_notice: Text to include in any rejection notice to be sent
-   when dmarc_moderation_action of reject applies.
- * dmarc_wrapped_message_text: Text to be added as a separate text/plain MIME
+``dmarc_moderation_notice``
+   Text to include in any rejection notice to be sent
+   when dmarc_moderation_action of reject applies.  This overrides the bult-in
+   default text.
+``dmarc_wrapped_message_text``
+   Text to be added as a separate text/plain MIME
    part preceding the original message part in the wrapped message when
-   dmarc_moderation_action of wrap_message applies.
- * from_is_list: The action to be applied to all messages for which
+   dmarc_moderation_action of wrap_message applies.  If this is not provided
+   the separate text/plain MIME part is not added.
+``from_is_list``
+   The action to be applied to all messages for which
    dmarc_moderation_action is none or not applicable.
- * reply_goes_to_list: If this is set to other than no_munging of Reply-To,
+``reply_goes_to_list``
+   If this is set to other than no_munging of Reply-To,
    the original From: goes in Cc: rather than Reply-To:.  This is intended to
    make MUA functions of reply and reply-all have the same effect with
    messages to which mitigations have been applied as they do with other
@@ -40,18 +52,23 @@ The settings and their effects are:
 
 The possible actions for both dmarc_moderation_action and from_is_list are:
 
- * none: Make no transformation to the message.
- * munge_from: Change the From: header and put the original From: in Reply-To:
+``none``
+   Make no transformation to the message.
+``munge_from``
+   Change the From: header and put the original From: in Reply-To:
    or in some cases Cc:
- * wrap_message: Wrap the message in an outer message with headers as in
+``wrap_message``
+   Wrap the message in an outer message with headers as in
    munge_from.
 
 In addition, there are two more possible actions (actually processed by the
 dmarc-moderation rule) for dmarc_moderation_action only:
 
- * reject: Bounce the message back to the sender with a default reason or one
+``reject``
+   Bounce the message back to the sender with a default reason or one
    supplied in dmarc_moderation_notice.
- * discard: Silently discard the message.
+``discard``
+   Silently discard the message.
 
 Here's what happens when we munge the From.
 
@@ -143,3 +160,4 @@ And here's a wrapped message with an added text part.
     --...--
     <BLANKLINE>
 
+.. _DMARC: https://wikipedia.org/wiki/DMARC
diff --git a/src/mailman/model/pending.py b/src/mailman/model/pending.py
index 60be56e8f..5889f9cc1 100644
--- a/src/mailman/model/pending.py
+++ b/src/mailman/model/pending.py
@@ -23,7 +23,7 @@ from lazr.config import as_timedelta
 from mailman.config import config
 from mailman.database.model import Model
 from mailman.database.transaction import dbconnection
-from mailman.database.types import SAUnicode, SAUnicodeLarge
+from mailman.database.types import SAUnicode
 from mailman.interfaces.pending import (
     IPendable, IPended, IPendedKeyValue, IPendings)
 from mailman.utilities.datetime import now
@@ -47,7 +47,7 @@ class PendedKeyValue(Model):
 
     id = Column(Integer, primary_key=True)
     key = Column(SAUnicode, index=True)
-    value = Column(SAUnicodeLarge, index=True)
+    value = Column(SAUnicode, index=True)
     pended_id = Column(Integer, ForeignKey('pended.id'), index=True)
 
     def __init__(self, key, value):
diff --git a/src/mailman/rules/dmarc.py b/src/mailman/rules/dmarc.py
index 30c356c21..b9781fd40 100644
--- a/src/mailman/rules/dmarc.py
+++ b/src/mailman/rules/dmarc.py
@@ -239,7 +239,7 @@ class DMARCModeration:
 
     def check(self, mlist, msg, msgdata):
         """See `IRule`."""
-        if mlist.dmarc_moderation_action == DMARCModerationAction.none:
+        if mlist.dmarc_moderation_action is DMARCModerationAction.none:
             # Don't bother to check if we're not going to do anything.
             return False
         dn, addr = parseaddr(msg.get('from'))
@@ -249,10 +249,10 @@ class DMARCModeration:
             # Otherwise, the rule misses but sets a flag for the dmarc handler
             # to do the appropriate action.
             msgdata['dmarc'] = True
-            if mlist.dmarc_moderation_action == DMARCModerationAction.discard:
+            if mlist.dmarc_moderation_action is DMARCModerationAction.discard:
                 msgdata['moderation_action'] = 'discard'
                 msgdata['moderation_reasons'] = [_('DMARC moderation')]
-            elif mlist.dmarc_moderation_action == DMARCModerationAction.reject:
+            elif mlist.dmarc_moderation_action is DMARCModerationAction.reject:
                 listowner = mlist.owner_address       # noqa F841
                 reason = (mlist.dmarc_moderation_notice or
                           _('You are not allowed to post to this mailing '