Split registration (subscription) and unsubscription:

* events
* notices
* handlers

Also, be sure to do only one `leave` command per email.

14 files changed, 147 insertions, 65 deletions

diff --git a/src/mailman/app/events.py b/src/mailman/app/events.py
index d039a13d6..e29eee35c 100644
--- a/src/mailman/app/events.py
+++ b/src/mailman/app/events.py
@@ -38,6 +38,7 @@ def initialize():
         passwords.handle_ConfigurationUpdatedEvent,
         style_manager.handle_ConfigurationUpdatedEvent,
         subscriptions.handle_ListDeletingEvent,
-        subscriptions.handle_ConfirmationNeededEvent,
+        subscriptions.handle_RegistrationConfirmationNeededEvent,
+        subscriptions.handle_UnsubscriptionConfirmationNeededEvent,
         switchboard.handle_ConfigurationUpdatedEvent,
         ])
diff --git a/src/mailman/app/subscriptions.py b/src/mailman/app/subscriptions.py
index 49a83c293..2790a6751 100644
--- a/src/mailman/app/subscriptions.py
+++ b/src/mailman/app/subscriptions.py
@@ -36,8 +36,9 @@ from mailman.interfaces.mailinglist import SubscriptionPolicy
 from mailman.interfaces.member import MembershipIsBannedError, NotAMemberError
 from mailman.interfaces.pending import IPendable, IPendings
 from mailman.interfaces.subscriptions import (
-    ConfirmationNeededEvent, ISubscriptionManager, ISubscriptionService,
-    SubscriptionPendingError, TokenOwner)
+    ISubscriptionManager, ISubscriptionService,
+    RegistrationConfirmationNeededEvent, SubscriptionPendingError, TokenOwner,
+    UnsubscriptionConfirmationNeededEvent)
 from mailman.interfaces.template import ITemplateLoader
 from mailman.interfaces.user import IUser
 from mailman.interfaces.usermanager import IUserManager
@@ -303,7 +304,7 @@ class SubscriptionWorkflow(Workflow):
         self.push('do_confirm_verify')
         self.save()
         # Triggering this event causes the confirmation message to be sent.
-        notify(ConfirmationNeededEvent(
+        notify(RegistrationConfirmationNeededEvent(
             self.mlist, self.token, self.address.email))
         # Now we wait for the confirmation.
         raise StopIteration
@@ -463,7 +464,7 @@ class UnSubscriptionWorkflow(Workflow):
         self._set_token(TokenOwner.subscriber)
         self.push('do_confirm_verify')
         self.save()
-        notify(ConfirmationNeededEvent(
+        notify(UnsubscriptionConfirmationNeededEvent(
             self.mlist, self.token, self.address.email))
         raise StopIteration
 
@@ -618,21 +619,12 @@ class UnsubscriptionManager(BaseSubscriptionManager):
         return workflow.token, workflow.token_owner, workflow.member
 
 
-@public
-def handle_ConfirmationNeededEvent(event):
-    if not isinstance(event, ConfirmationNeededEvent):
-        return
-    # There are three ways for a user to confirm their subscription.  They
-    # can reply to the original message and let the VERP'd return address
-    # encode the token, they can reply to the robot and keep the token in
-    # the Subject header, or they can click on the URL in the body of the
-    # message and confirm through the web.
+def _handle_confirmation_needed_events(event, template_name):
     subject = 'confirm {}'.format(event.token)
     confirm_address = event.mlist.confirm_address(event.token)
     email_address = event.email
     # Send a verification email to the address.
-    template = getUtility(ITemplateLoader).get(
-        'list:user:action:confirm', event.mlist)
+    template = getUtility(ITemplateLoader).get(template_name, event.mlist)
     text = expand(template, event.mlist, dict(
         token=event.token,
         subject=subject,
@@ -649,6 +641,20 @@ def handle_ConfirmationNeededEvent(event):
 
 
 @public
+def handle_RegistrationConfirmationNeededEvent(event):
+    if not isinstance(event, RegistrationConfirmationNeededEvent):
+        return
+    _handle_confirmation_needed_events(event, 'list:user:action:subscribe')
+
+
+@public
+def handle_UnsubscriptionConfirmationNeededEvent(event):
+    if not isinstance(event, UnsubscriptionConfirmationNeededEvent):
+        return
+    _handle_confirmation_needed_events(event, 'list:user:action:unsubscribe')
+
+
+@public
 def handle_ListDeletingEvent(event):
     """Delete a mailing list's members when the list is being deleted."""
 
diff --git a/src/mailman/app/tests/test_subscriptions.py b/src/mailman/app/tests/test_subscriptions.py
index 42b60b552..9f02593a9 100644
--- a/src/mailman/app/tests/test_subscriptions.py
+++ b/src/mailman/app/tests/test_subscriptions.py
@@ -286,8 +286,8 @@ class TestSubscriptionWorkflow(unittest.TestCase):
     def test_moderation_checks_approval_required(self):
         # The moderator must approve the subscription.
         self._mlist.subscription_policy = SubscriptionPolicy.moderate
-        anne = self._user_manager.create_address(self._anne, pre_verified=True)
-        workflow = SubscriptionWorkflow(self._mlist, anne)
+        anne = self._user_manager.create_address(self._anne)
+        workflow = SubscriptionWorkflow(self._mlist, anne, pre_verified=True)
         workflow.run_thru('moderation_checks')
         with patch.object(workflow, '_step_get_moderator_approval') as step:
             next(workflow)
diff --git a/src/mailman/commands/eml_membership.py b/src/mailman/commands/eml_membership.py
index 223552ea5..9194ebe21 100644
--- a/src/mailman/commands/eml_membership.py
+++ b/src/mailman/commands/eml_membership.py
@@ -182,11 +182,20 @@ You may be asked to confirm your request.""")
             if member is not None:
                 break
         else:
-            # None of the user's addresses are subscribed to this mailing list.
-            print(_(
-                '$self.name: $email is not a member of $mlist.fqdn_listname'),
-                file=results)
+            # There are two possible situations.  Either none of the user's
+            # addresses are subscribed to this mailing list, or this command
+            # email *already* unsubscribed the user from the mailing list.
+            # E.g. if a message was sent to the -leave address and it
+            # contained the 'leave' command.  Don't send a bogus response in
+            # this case, just ignore subsequent leaves of the same address.
+            print(_('$self.name: $email is not a member of '
+                    '$mlist.fqdn_listname'), file=results)
             return ContinueProcessing.no
+        already_left = msgdata.setdefault('leaves', set())
+        if email in already_left:
+            return ContinueProcessing.yes
+        # Ignore any subsequent 'leave' commands.
+        already_left.add(email)
         manager = getAdapter(mlist, ISubscriptionManager, name='unsubscribe')
         token, token_owner, member = manager.unregister(user_address)
         person = formataddr((user.display_name, email))   # noqa
diff --git a/src/mailman/interfaces/subscriptions.py b/src/mailman/interfaces/subscriptions.py
index 9a20b9c0d..fb13d3d42 100644
--- a/src/mailman/interfaces/subscriptions.py
+++ b/src/mailman/interfaces/subscriptions.py
@@ -78,7 +78,7 @@ class TokenOwner(Enum):
 
 
 @public
-class ConfirmationNeededEvent:
+class RegistrationConfirmationNeededEvent:
     """Triggered when an address needs confirmation.
 
     Addresses must be verified before they can receive messages or post
@@ -92,6 +92,19 @@ class ConfirmationNeededEvent:
 
 
 @public
+class UnsubscriptionConfirmationNeededEvent:
+    """Triggered when an unsubscription request needs confirmation.
+
+    The confirmation message is sent to the user when this event is
+    triggered.
+    """
+    def __init__(self, mlist, token, email):
+        self.mlist = mlist
+        self.token = token
+        self.email = email
+
+
+@public
 class ISubscriptionService(Interface):
     """General subscription services."""
 
diff --git a/src/mailman/interfaces/template.py b/src/mailman/interfaces/template.py
index 442f1eeda..e576eb882 100644
--- a/src/mailman/interfaces/template.py
+++ b/src/mailman/interfaces/template.py
@@ -171,7 +171,7 @@ ALL_TEMPLATES = {
         'list:admin:notice:unrecognized',
         'list:admin:notice:unsubscribe',
         'list:member:digest:masthead',
-        'list:user:action:confirm',
+        'list:user:action:subscribe',
         'list:user:action:unsubscribe',
         'list:user:notice:hold',
         'list:user:notice:no-more-today',
diff --git a/src/mailman/rest/docs/templates.rst b/src/mailman/rest/docs/templates.rst
index 7ac0670e1..eefff09b7 100644
--- a/src/mailman/rest/docs/templates.rst
+++ b/src/mailman/rest/docs/templates.rst
@@ -479,7 +479,7 @@ below.  Here are all the supported template names:
       (e.g. "French", "English", "Italian")
     * ``user_name`` - the recipient's display name if available
 
-* ``list:user:action:confirm``
+* ``list:user:action:subscribe``
     The message sent to subscribers when a subscription confirmation is
     required.
 
@@ -490,6 +490,17 @@ below.  Here are all the supported template names:
       to; this corresponds to the ``Reply-To`` header
     * ``user_email`` - the email address being confirmed
 
+* ``list:user:action:unsubscribe``
+    The message sent to subscribers when an unsubscription confirmation is
+    required.
+
+    * ``token`` - the unique confirmation token
+    * ``subject`` - the ``Subject`` heading for the confirmation email, which
+      includes the confirmation token
+    * ``confirm_email`` - the email address to send the confirmation response
+      to; this corresponds to the ``Reply-To`` header
+    * ``user_email`` - the email address being confirmed
+
 * ``list:user:notice:goodbye``
     The notice sent to a member when they unsubscribe from a mailing list.
 
diff --git a/src/mailman/rest/tests/test_domains.py b/src/mailman/rest/tests/test_domains.py
index 41723b07c..c707eaadb 100644
--- a/src/mailman/rest/tests/test_domains.py
+++ b/src/mailman/rest/tests/test_domains.py
@@ -288,7 +288,7 @@ class TestDomainTemplates(unittest.TestCase):
                 'list:member:digest:masthead': '',
                 'list:member:regular:footer': 'http://example.org/footer',
                 'list:member:regular:header': 'http://example.org/header',
-                'list:user:action:confirm': '',
+                'list:user:action:subscribe': '',
                 'list:user:action:unsubscribe': '',
                 'list:user:notice:goodbye': 'http://example.org/goodbye',
                 'list:user:notice:hold': '',
diff --git a/src/mailman/rest/tests/test_lists.py b/src/mailman/rest/tests/test_lists.py
index 787009855..da6e95f3b 100644
--- a/src/mailman/rest/tests/test_lists.py
+++ b/src/mailman/rest/tests/test_lists.py
@@ -674,7 +674,7 @@ class TestListTemplates(unittest.TestCase):
                 'list:member:digest:masthead': '',
                 'list:member:regular:footer': 'http://example.org/footer',
                 'list:member:regular:header': 'http://example.org/header',
-                'list:user:action:confirm': '',
+                'list:user:action:subscribe': '',
                 'list:user:action:unsubscribe': '',
                 'list:user:notice:goodbye': 'http://example.org/goodbye',
                 'list:user:notice:hold': '',
diff --git a/src/mailman/rest/tests/test_root.py b/src/mailman/rest/tests/test_root.py
index 840212825..eb246cb1d 100644
--- a/src/mailman/rest/tests/test_root.py
+++ b/src/mailman/rest/tests/test_root.py
@@ -288,7 +288,7 @@ class TestSiteTemplates(unittest.TestCase):
                 'list:member:digest:masthead': '',
                 'list:member:regular:footer': 'http://example.org/footer',
                 'list:member:regular:header': 'http://example.org/header',
-                'list:user:action:confirm': '',
+                'list:user:action:subscribe': '',
                 'list:user:action:unsubscribe': '',
                 'list:user:notice:goodbye': 'http://example.org/goodbye',
                 'list:user:notice:hold': '',
diff --git a/src/mailman/runners/tests/test_leave.py b/src/mailman/runners/tests/test_leave.py
index f54ab1ce8..1b601f0c2 100644
--- a/src/mailman/runners/tests/test_leave.py
+++ b/src/mailman/runners/tests/test_leave.py
@@ -34,23 +34,77 @@ from mailman.testing.helpers import set_preferred
 from zope.component import getUtility
 
 
+def confirmation_line(msg):
+    confirmation_lines = []
+    in_results = False
+    for line in body_line_iterator(msg):
+        line = line.strip()
+        if in_results:
+            if line.startswith('- Done'):
+                break
+            if len(line) > 0:
+                confirmation_lines.append(line)
+        if line.strip() == '- Results:':
+            in_results = True
+    # There should be exactly one confirmation line.
+    assert len(confirmation_lines) == 1, confirmation_lines
+    return confirmation_lines[0]
+
+
 class TestLeave(unittest.TestCase):
     """Test mailing list un-subscriptions"""
 
     layer = ConfigLayer
 
     def setUp(self):
-        self._mlist = create_list('test@example.com')
-        self._mlist.send_welcome_message = False
-        self._mlist.unsubscription_policy = SubscriptionPolicy.open
+        with transaction():
+            self._mlist = create_list('test@example.com')
+            self._mlist.send_goodbye_message = False
+            self._mlist.send_welcome_message = False
         self._commandq = config.switchboards['command']
         self._runner = make_testable_runner(CommandRunner, 'command')
 
     def test_leave(self):
         with transaction():
+            self._mlist.unsubscription_policy = SubscriptionPolicy.confirm
+            anne = getUtility(IUserManager).create_user('anne@example.org')
+            set_preferred(anne)
+            self._mlist.subscribe(anne.preferred_address)
+        msg = mfs("""\
+From: anne@example.org
+To: test-leave@example.com
+
+leave
+""")
+        self._commandq.enqueue(msg, dict(listid='test.example.com',
+                                         subaddress='leave'))
+        self._runner.run()
+        # Two messages have been sent, both to anne@example.org.  The first
+        # asks for her confirmation of her unsubscription event.  The second
+        # is the results of her email command.
+        items = get_queue_messages('virgin', sort_on='subject',
+                                   expected_count=2)
+        if items[0].msg['from'] == 'test-bounces@example.com':
+            results = items[0].msg
+            confirmation = items[1].msg
+        else:
+            results = items[1].msg
+            confirmation = items[0].msg
+        self.assertTrue(str(confirmation['subject']).startswith('confirm'))
+        line = confirmation_line(results)
+        # The confirmation line should name Anne's email address.
+        self.assertIn('anne@example.org', line)
+
+    def test_double_leave(self):
+        # In this case, the user can be unsubscribed immediately because the
+        # policy does not require confirmation, however because the email is
+        # sent to the -leave address and it contains the 'leave' command, we
+        # should only process one command per email.
+        with transaction():
+            self._mlist.unsubscription_policy = SubscriptionPolicy.open
             anne = getUtility(IUserManager).create_user('anne@example.org')
             set_preferred(anne)
-        self._mlist.subscribe(list(anne.addresses)[0])
+            self._mlist.subscribe(anne.preferred_address)
         msg = mfs("""\
 From: anne@example.org
 To: test-leave@example.com
@@ -62,19 +116,5 @@ leave
         self._runner.run()
         items = get_queue_messages('virgin', sort_on='subject',
                                    expected_count=1)
-        self.assertTrue(str(items[0].msg['subject']).startswith('confirm'))
-        confirmation_lines = []
-        in_results = False
-        for line in body_line_iterator(items[0].msg):
-            line = line.strip()
-            if in_results:
-                if line.startswith('- Done'):
-                    break
-                if len(line) > 0:
-                    confirmation_lines.append(line)
-            if line.strip() == '- Results:':
-                in_results = True
-                # There should be exactly one confirmation line.
-        self.assertEqual(len(confirmation_lines), 1)
-        # And the confirmation line should name Anne's email address.
-        self.assertIn('anne@example.org', confirmation_lines[0])
+        line = confirmation_line(items[0].msg)
+        self.assertEqual(line, 'anne@example.org left test@example.com')
diff --git a/src/mailman/templates/en/list:user:action:confirm.txt b/src/mailman/templates/en/list:user:action:subscribe.txt
index ce384fea2..10ec5a3b5 100644
--- a/src/mailman/templates/en/list:user:action:confirm.txt
+++ b/src/mailman/templates/en/list:user:action:subscribe.txt
@@ -10,7 +10,7 @@ Before you can start using GNU Mailman at this site, you must first confirm
 that this is your email address.  You can do this by replying to this message,
 keeping the Subject header intact.
 
-If you do not wish to register this email address simply disregard this
+If you do not wish to register this email address, simply disregard this
 message.  If you think you are being maliciously subscribed to the list, or
 have any other questions, you may contact
 
diff --git a/src/mailman/templates/en/list:user:action:unsubscribe.txt b/src/mailman/templates/en/list:user:action:unsubscribe.txt
index 97ebd30b2..583b83c82 100644
--- a/src/mailman/templates/en/list:user:action:unsubscribe.txt
+++ b/src/mailman/templates/en/list:user:action:unsubscribe.txt
@@ -1,19 +1,17 @@
-Mailing list removal confirmation notice for mailing list $listname
+Email Address Unsubscription Confirmation
 
-We have received a request for the removal of your email address, "${email}"
-from the $listaddr mailing list.  To confirm that you want to be removed from
-this mailing list, simply reply to this message, keeping the Subject header
-intact.
+Hello, this is the GNU Mailman server at $domain.
+
+We have received an unsubscription request for the email address
 
-Or include the following line -- and only the following line -- in a
-message to $requestaddr:
+    $user_email
 
-    confirm $cookie
+Before GNU Mailman can unsubscribe you, you must first confirm your request.
+You can do this by replying to this message, keeping the Subject header
+intact.
 
-Note that simply sending a reply to this message should work from most mail
-readers, since that usually leaves the Subject line in the right form
-(additional "Re:" text in the Subject is okay).
+If you do not wish to unsubscribe this email address, simply disregard this
+message.  If you think you are being maliciously unsubscribed from the list,
+or have any other questions, you may contact
 
-If you do not wish to be removed from this list, please simply disregard this
-message.  If you think you are being maliciously removed from the list, or
-have any other questions, send them to $listadmin
+    $owner_email
diff --git a/src/mailman/testing/helpers.py b/src/mailman/testing/helpers.py
index 6496484ca..d9cf974f5 100644
--- a/src/mailman/testing/helpers.py
+++ b/src/mailman/testing/helpers.py
@@ -18,6 +18,7 @@
 """Various test helpers."""
 
 import os
+import sys
 import json
 import time
 import uuid
@@ -118,8 +119,11 @@ def get_queue_messages(queue_name, sort_on=None, expected_count=None):
         messages.append(_Bag(msg=msg, msgdata=msgdata))
         queue.finish(filebase)
     if expected_count is not None:
-        assert len(messages) == expected_count, 'Wanted {}, got {}'.format(
-            expected_count, len(messages))
+        if len(messages) != expected_count:
+            for item in messages:
+                print(item.msg, file=sys.stderr)
+            raise AssertionError('Wanted {}, got {}'.format(
+                expected_count, len(messages)))
     if sort_on is not None:
         messages.sort(key=lambda item: str(item.msg[sort_on]))
     return messages