Merge lp:~barry/mailman/requests into lp:mailman

[Barry Warsaw]

Fix subscription holds exposure to REST.

[lp:~barry/mailman/requests]

7327. By Barry Warsaw on 2015-04-17

Check pointing new subscription moderation REST API.

=== modified file 'src/mailman/app/subscriptions.py'
--- src/mailman/app/subscriptions.py	2015-04-16 02:51:39 +0000
+++ src/mailman/app/subscriptions.py	2015-04-17 15:30:55 +0000
@@ -24,7 +24,6 @@
     ]
 
 
-
 import uuid
 import logging
 
@@ -169,7 +168,10 @@
             return
         pendable = Pendable(
             list_id=self.mlist.list_id,
-            address=self.address.email,
+            email=self.address.email,
+            display_name=self.address.display_name,
+            when=now().replace(microsecond=0).isoformat(),
+            token_owner=token_owner.name,
             )
         self.token = getUtility(IPendings).add(pendable, timedelta(days=3650))
 
 
=== modified file 'src/mailman/app/tests/test_subscriptions.py'
--- src/mailman/app/tests/test_subscriptions.py	2015-04-16 14:42:40 +0000
+++ src/mailman/app/tests/test_subscriptions.py	2015-04-17 15:30:55 +0000
@@ -57,6 +57,23 @@
         self.assertEqual(workflow.token_owner, TokenOwner.no_one)
         self.assertIsNone(workflow.member)
 
+    def test_pended_data(self):
+        # There is a Pendable associated with the held request, and it has
+        # some data associated with it.
+        anne = self._user_manager.create_address(self._anne)
+        workflow = SubscriptionWorkflow(self._mlist, anne)
+        try:
+            workflow.run_thru('send_confirmation')
+        except StopIteration:
+            pass
+        self.assertIsNotNone(workflow.token)
+        pendable = getUtility(IPendings).confirm(workflow.token, expunge=False)
+        self.assertEqual(pendable['list_id'], 'test.example.com')
+        self.assertEqual(pendable['email'], '[e-mail]')
+        self.assertEqual(pendable['display_name'], '')
+        self.assertEqual(pendable['when'], '2005-08-01T07:49:23')
+        self.assertEqual(pendable['token_owner'], 'subscriber')
+
     def test_user_or_address_required(self):
         # The `subscriber` attribute must be a user or address.
         workflow = SubscriptionWorkflow(self._mlist)
 
=== modified file 'src/mailman/interfaces/pending.py'
--- src/mailman/interfaces/pending.py	2015-04-15 14:05:35 +0000
+++ src/mailman/interfaces/pending.py	2015-04-17 15:30:55 +0000
@@ -95,4 +95,10 @@
     def evict():
         """Remove all pended items whose lifetime has expired."""
 
+    def __iter__():
+        """An iterator over all pendables.
+
+        Each element is a 2-tuple of the form (token, dict).
+        """
+
     count = Attribute('The number of pendables in the pendings database.')
 
=== modified file 'src/mailman/interfaces/registrar.py'
--- src/mailman/interfaces/registrar.py	2015-04-16 02:51:39 +0000
+++ src/mailman/interfaces/registrar.py	2015-04-17 15:30:55 +0000
@@ -75,12 +75,13 @@
 
         :param subscriber: The user or address to subscribe.
         :type email: ``IUser`` or ``IAddress``
-        :return: None if the workflow completes with the member being
-            subscribed.  If the workflow is paused for user confirmation or
-            moderator approval, a 3-tuple is returned where the first element
-            is a ``TokenOwner`` the second element is the token hash, and the
-            third element is the subscribed member.
-        :rtype: None or 2-tuple of (TokenOwner, str)
+        :return: A 3-tuple is returned where the first element is the token
+            hash, the second element is a ``TokenOwner`, and the third element
+            is the subscribed member.  If the subscriber got subscribed
+            immediately, the token will be None and the member will be
+            an ``IMember``.  If the subscription got held, the token
+            will be a hash and the member will be None.
+        :rtype: (str-or-None, ``TokenOwner``, ``IMember``-or-None)
         :raises MembershipIsBannedError: when the address being subscribed
             appears in the global or list-centric bans.
         """
@@ -94,9 +95,13 @@
 
         :param token: A token matching a workflow.
         :type token: string
-        :return: The new token for any follow up confirmation, or None if the
-            user was subscribed.
-        :rtype: str or None
+        :return: A 3-tuple is returned where the first element is the token
+            hash, the second element is a ``TokenOwner`, and the third element
+            is the subscribed member.  If the subscriber got subscribed
+            immediately, the token will be None and the member will be
+            an ``IMember``.  If the subscription is still being held, the token
+            will be a hash and the member will be None.
+        :rtype: (str-or-None, ``TokenOwner``, ``IMember``-or-None)
         :raises LookupError: when no workflow is associated with the token.
         """
 
 
=== modified file 'src/mailman/model/docs/pending.rst'
--- src/mailman/model/docs/pending.rst	2015-04-15 14:05:35 +0000
+++ src/mailman/model/docs/pending.rst	2015-04-17 15:30:55 +0000
@@ -43,10 +43,9 @@
     >>> pendingdb.count
     1
 
-There's not much you can do with tokens except to *confirm* them, which
-basically means returning the `IPendable` structure (as a dictionary) from the
-database that matches the token.  If the token isn't in the database, None is
-returned.
+You can *confirm* the pending, which means returning the `IPendable` structure
+(as a dictionary) from the database that matches the token.  If the token
+isn't in the database, None is returned.
 
     >>> pendable = pendingdb.confirm(b'missing')
     >>> print(pendable)
@@ -83,6 +82,18 @@
     >>> print(pendingdb.confirm(token_1))
     None
 
+You can iterate over all the pendings in the database.
+
+    >>> pendables = list(pendingdb)
+    >>> def sort_key(item):
+    ...     token, pendable = item
+    ...     return pendable['type']
+    >>> sorted_pendables = sorted(pendables, key=sort_key)
+    >>> for token, pendable in sorted_pendables:
+    ...     print(pendable['type'])
+    three
+    two
+
 An event can be given a lifetime when it is pended, otherwise it just uses a
 default lifetime.
 
 
=== modified file 'src/mailman/model/pending.py'
--- src/mailman/model/pending.py	2015-04-15 14:05:35 +0000
+++ src/mailman/model/pending.py	2015-04-17 15:30:55 +0000
@@ -166,6 +166,11 @@
                     store.delete(keyvalue)
                 store.delete(pending)
 
+    @dbconnection
+    def __iter__(self, store):
+        for pending in store.query(Pended).all():
+            yield pending.token, self.confirm(pending.token, expunge=False)
+
     @property
     @dbconnection
     def count(self, store):
 
=== renamed file 'src/mailman/rest/docs/moderation.rst' => 'src/mailman/rest/docs/post-moderation.rst'
--- src/mailman/rest/docs/moderation.rst	2015-03-29 20:30:30 +0000
+++ src/mailman/rest/docs/post-moderation.rst	2015-04-17 15:30:55 +0000
@@ -1,18 +1,13 @@
-==========
-Moderation
-==========
-
-There are two kinds of moderation tasks a list administrator may need to
-perform.  Messages which are held for approval can be accepted, rejected,
-discarded, or deferred.  Subscription (and sometimes unsubscription) requests
-can similarly be accepted, discarded, rejected, or deferred.
-
-
-Message moderation
-==================
+===============
+Post Moderation
+===============
+
+Messages which are held for approval can be accepted, rejected, discarded, or
+deferred by the list moderators.
+
 
 Viewing the list of held messages
----------------------------------
+=================================
 
 Held messages can be moderated through the REST API.  A mailing list starts
 with no held messages.
@@ -90,7 +85,7 @@
 
 
 Disposing of held messages
---------------------------
+==========================
 
 Individual messages can be moderated through the API by POSTing back to the
 held message's resource.   The POST data requires an action of one of the
@@ -196,166 +191,3 @@
     1
     >>> print(messages[0].msg['subject'])
     Request to mailing list "Ant" rejected
-
-
-Subscription moderation
-=======================
-
-Viewing subscription requests
------------------------------
-
-Subscription and unsubscription requests can be moderated via the REST API as
-well.  A mailing list starts with no pending subscription or unsubscription
-requests.
-
-    >>> ant.admin_immed_notify = False
-    >>> dump_json('http://localhost:9001/3.0/lists/[e-mail]/requests')
-    http_etag: "..."
-    start: 0
-    total_size: 0
-
-When Anne tries to subscribe to the Ant list, her subscription is held for
-moderator approval.
-::
-
-    >>> from mailman.app.moderator import hold_subscription
-    >>> from mailman.interfaces.member import DeliveryMode
-    >>> from mailman.interfaces.subscriptions import RequestRecord
-
-    >>> sub_req_id = hold_subscription(
-    ...     ant, RequestRecord('[e-mail]', 'Anne Person',
-    ...                        DeliveryMode.regular, 'en'))
-    >>> transaction.commit()
-
-The subscription request is available from the mailing list.
-
-    >>> dump_json('http://localhost:9001/3.0/lists/[e-mail]/requests')
-    entry 0:
-        delivery_mode: regular
-        display_name: Anne Person
-        email: [e-mail]
-        http_etag: "..."
-        language: en
-        request_id: ...
-        type: subscription
-        when: 2005-08-01T07:49:23
-    http_etag: "..."
-    start: 0
-    total_size: 1
-
-
-Viewing unsubscription requests
--------------------------------
-
-Bart tries to leave a mailing list, but he may not be allowed to.
-
-    >>> from mailman.testing.helpers import subscribe
-    >>> from mailman.app.moderator import hold_unsubscription
-    >>> bart = subscribe(ant, 'Bart', email='[e-mail]')
-    >>> unsub_req_id = hold_unsubscription(ant, '[e-mail]')
-    >>> transaction.commit()
-
-The unsubscription request is also available from the mailing list.
-
-    >>> dump_json('http://localhost:9001/3.0/lists/[e-mail]/requests')
-    entry 0:
-        delivery_mode: regular
-        display_name: Anne Person
-        email: [e-mail]
-        http_etag: "..."
-        language: en
-        request_id: ...
-        type: subscription
-        when: 2005-08-01T07:49:23
-    entry 1:
-        email: [e-mail]
-        http_etag: "..."
-        request_id: ...
-        type: unsubscription
-    http_etag: "..."
-    start: 0
-    total_size: 2
-
-
-Viewing individual requests
----------------------------
-
-You can view an individual membership change request by providing the
-request id.  Anne's subscription request looks like this.
-
-    >>> dump_json('http://localhost:9001/3.0/lists/[e-mail]/'
-    ...           'requests/{}'.format(sub_req_id))
-    delivery_mode: regular
-    display_name: Anne Person
-    email: [e-mail]
-    http_etag: "..."
-    language: en
-    request_id: ...
-    type: subscription
-    when: 2005-08-01T07:49:23
-
-Bart's unsubscription request looks like this.
-
-    >>> dump_json('http://localhost:9001/3.0/lists/[e-mail]/'
-    ...           'requests/{}'.format(unsub_req_id))
-    email: [e-mail]
-    http_etag: "..."
-    request_id: ...
-    type: unsubscription
-
-
-Disposing of subscription requests
-----------------------------------
-
-Similar to held messages, you can dispose of held subscription and
-unsubscription requests by POSTing back to the request's resource.  The POST
-data requires an action of one of the following:
-
- * discard - throw the request away.
- * reject - the request is denied and a notification is sent to the email
-            address requesting the membership change.
- * defer - defer any action on this membership change (continue to hold it).
- * accept - accept the membership change.
-
-Anne's subscription request is accepted.
-
-    >>> dump_json('http://localhost:9001/3.0/lists/'
-    ...           '[e-mail]/requests/{}'.format(sub_req_id),
-    ...           {'action': 'accept'})
-    content-length: 0
-    date: ...
-    server: ...
-    status: 204
-
-Anne is now a member of the mailing list.
-
-    >>> transaction.abort()
-    >>> ant.members.get_member('[e-mail]')
-    <Member: Anne Person <[e-mail]> on [e-mail]
-             as MemberRole.member>
-    >>> transaction.abort()
-
-Bart's unsubscription request is discarded.
-
-    >>> dump_json('http://localhost:9001/3.0/lists/'
-    ...           '[e-mail]/requests/{}'.format(unsub_req_id),
-    ...           {'action': 'discard'})
-    content-length: 0
-    date: ...
-    server: ...
-    status: 204
-
-Bart is still a member of the mailing list.
-
-    >>> transaction.abort()
-    >>> print(ant.members.get_member('[e-mail]'))
-    <Member: Bart Person <[e-mail]> on [e-mail]
-             as MemberRole.member>
-    >>> transaction.abort()
-
-There are no more membership change requests.
-
-    >>> dump_json('http://localhost:9001/3.0/lists/[e-mail]/requests')
-    http_etag: "..."
-    start: 0
-    total_size: 0
 
=== added file 'src/mailman/rest/docs/sub-moderation.rst'
--- src/mailman/rest/docs/sub-moderation.rst	1970-01-01 00:00:00 +0000
+++ src/mailman/rest/docs/sub-moderation.rst	2015-04-17 15:30:55 +0000
@@ -0,0 +1,113 @@
+=========================
+ Subscription moderation
+=========================
+
+Subscription (and sometimes unsubscription) requests can similarly be
+accepted, discarded, rejected, or deferred by the list moderators.
+
+
+Viewing subscription requests
+=============================
+
+A mailing list starts with no pending subscription or unsubscription requests.
+
+    >>> ant = create_list('[e-mail]')
+    >>> ant.admin_immed_notify = False
+    >>> from mailman.interfaces.mailinglist import SubscriptionPolicy
+    >>> ant.subscription_policy = SubscriptionPolicy.moderate
+    >>> transaction.commit()
+    >>> dump_json('http://localhost:9001/3.0/lists/[e-mail]/requests')
+    http_etag: "..."
+    start: 0
+    total_size: 0
+
+When Anne tries to subscribe to the Ant list, her subscription is held for
+moderator approval.
+
+    >>> from mailman.interfaces.registrar import IRegistrar
+    >>> from mailman.interfaces.usermanager import IUserManager
+    >>> from zope.component import getUtility
+    >>> registrar = IRegistrar(ant)
+    >>> manager = getUtility(IUserManager)
+    >>> anne = manager.create_address('[e-mail]', 'Anne Person')
+    >>> token, token_owner, member = registrar.register(
+    ...     anne, pre_verified=True, pre_confirmed=True)
+    >>> print(member)
+    None
+
+The message is being held for moderator approval.
+
+    >>> print(token_owner.name)
+    moderator
+
+The subscription request can be viewed in the REST API.
+
+    >>> transaction.commit()
+    >>> dump_json('http://localhost:9001/3.0/lists/[e-mail]/requests')
+    entry 0:
+        display_name: Anne Person
+        email: [e-mail]
+        http_etag: "..."
+        list_id: ant.example.com
+        token: ...
+        token_owner: moderator
+        when: 2005-08-01T07:49:23
+    http_etag: "..."
+    start: 0
+    total_size: 1
+
+
+Viewing individual requests
+===========================
+
+You can view an individual membership change request by providing the token
+(a.k.a. request id).  Anne's subscription request looks like this.
+
+    >>> dump_json('http://localhost:9001/3.0/lists/[e-mail]/'
+    ...           'requests/{}'.format(token))
+    delivery_mode: regular
+    display_name: Anne Person
+    email: [e-mail]
+    http_etag: "..."
+    language: en
+    request_id: ...
+    type: subscription
+    when: 2005-08-01T07:49:23
+
+
+Disposing of subscription requests
+==================================
+
+Moderators can dispose of held subscription requests by POSTing back to the
+request's resource.  The POST data requires an action of one of the following:
+
+ * discard - throw the request away.
+ * reject - the request is denied and a notification is sent to the email
+            address requesting the membership change.
+ * defer - defer any action on this membership change (continue to hold it).
+ * accept - accept the membership change.
+
+Anne's subscription request is accepted.
+
+    >>> dump_json('http://localhost:9001/3.0/lists/'
+    ...           '[e-mail]/requests/{}'.format(token),
+    ...           {'action': 'accept'})
+    content-length: 0
+    date: ...
+    server: ...
+    status: 204
+
+Anne is now a member of the mailing list.
+
+    >>> transaction.abort()
+    >>> ant.members.get_member('[e-mail]')
+    <Member: Anne Person <[e-mail]> on [e-mail]
+             as MemberRole.member>
+    >>> transaction.abort()
+
+There are no more membership change requests.
+
+    >>> dump_json('http://localhost:9001/3.0/lists/[e-mail]/requests')
+    http_etag: "..."
+    start: 0
+    total_size: 0
 
=== modified file 'src/mailman/rest/lists.py'
--- src/mailman/rest/lists.py	2015-04-06 23:07:42 +0000
+++ src/mailman/rest/lists.py	2015-04-17 15:30:55 +0000
@@ -42,7 +42,8 @@
     CollectionMixin, GetterSetter, NotFound, bad_request, child, created,
     etag, no_content, not_found, okay, paginate, path_to)
 from mailman.rest.members import AMember, MemberCollection
-from mailman.rest.moderation import HeldMessages, SubscriptionRequests
+from mailman.rest.post_moderation import HeldMessages
+from mailman.rest.sub_moderation import SubscriptionRequests
 from mailman.rest.validator import Validator
 from operator import attrgetter
 from zope.component import getUtility
 
=== renamed file 'src/mailman/rest/moderation.py' => 'src/mailman/rest/post_moderation.py'
--- src/mailman/rest/moderation.py	2015-01-05 01:22:39 +0000
+++ src/mailman/rest/post_moderation.py	2015-04-17 15:30:55 +0000
@@ -15,18 +15,15 @@
 # You should have received a copy of the GNU General Public License along with
 # GNU Mailman.  If not, see <http://www.gnu.org/licenses/>.
 
-"""REST API for Message moderation."""
+"""REST API for held message moderation."""
 
 __all__ = [
     'HeldMessage',
     'HeldMessages',
-    'MembershipChangeRequest',
-    'SubscriptionRequests',
     ]
 
 
-from mailman.app.moderator import (
-    handle_message, handle_subscription, handle_unsubscription)
+from mailman.app.moderator import handle_message
 from mailman.interfaces.action import Action
 from mailman.interfaces.messages import IMessageStore
 from mailman.interfaces.requests import IListRequests, RequestType
@@ -36,16 +33,11 @@
 from zope.component import getUtility
 
 
-HELD_MESSAGE_REQUESTS = (RequestType.held_message,)
-MEMBERSHIP_CHANGE_REQUESTS = (RequestType.subscription,
-                              RequestType.unsubscription)
-
-
 
 
 class _ModerationBase:
     """Common base class."""
 
-    def _make_resource(self, request_id, expected_request_types):
+    def _make_resource(self, request_id):
         requests = IListRequests(self._mlist)
         results = requests.get_request(request_id)
         if results is None:
@@ -57,9 +49,9 @@
         # Check for a matching request type, and insert the type name into the
         # resource.
         request_type = RequestType[resource.pop('_request_type')]
-        if request_type not in expected_request_types:
+        if request_type is not RequestType.held_message:
             return None
-        resource['type'] = request_type.name
+        resource['type'] = RequestType.held_message.name
         # This key isn't what you think it is.  Usually, it's the Pendable
         # record's row id, which isn't helpful at all.  If it's not there,
         # that's fine too.
@@ -72,8 +64,7 @@
     """Held messages are a little different."""
 
     def _make_resource(self, request_id):
-        resource = super(_HeldMessageBase, self)._make_resource(
-            request_id, HELD_MESSAGE_REQUESTS)
+        resource = super(_HeldMessageBase, self)._make_resource(request_id)
         if resource is None:
             return None
         # Grab the message and insert its text representation into the
@@ -162,91 +153,3 @@
     @child(r'^(?P<id>[^/]+)')
     def message(self, request, segments, **kw):
         return HeldMessage(self._mlist, kw['id'])
-
-
-
 
-class MembershipChangeRequest(_ModerationBase):
-    """Resource for moderating a membership change."""
-
-    def __init__(self, mlist, request_id):
-        self._mlist = mlist
-        self._request_id = request_id
-
-    def on_get(self, request, response):
-        try:
-            request_id = int(self._request_id)
-        except ValueError:
-            bad_request(response)
-            return
-        resource = self._make_resource(request_id, MEMBERSHIP_CHANGE_REQUESTS)
-        if resource is None:
-            not_found(response)
-        else:
-            # Remove unnecessary keys.
-            del resource['key']
-            okay(response, etag(resource))
-
-    def on_post(self, request, response):
-        try:
-            validator = Validator(action=enum_validator(Action))
-            arguments = validator(request)
-        except ValueError as error:
-            bad_request(response, str(error))
-            return
-        requests = IListRequests(self._mlist)
-        try:
-            request_id = int(self._request_id)
-        except ValueError:
-            bad_request(response)
-            return
-        results = requests.get_request(request_id)
-        if results is None:
-            not_found(response)
-            return
-        key, data = results
-        try:
-            request_type = RequestType[data['_request_type']]
-        except ValueError:
-            bad_request(response)
-            return
-        if request_type is RequestType.subscription:
-            handle_subscription(self._mlist, request_id, **arguments)
-        elif request_type is RequestType.unsubscription:
-            handle_unsubscription(self._mlist, request_id, **arguments)
-        else:
-            bad_request(response)
-            return
-        no_content(response)
-
-
-class SubscriptionRequests(_ModerationBase, CollectionMixin):
-    """Resource for membership change requests."""
-
-    def __init__(self, mlist):
-        self._mlist = mlist
-        self._requests = None
-
-    def _resource_as_dict(self, request):
-        """See `CollectionMixin`."""
-        resource = self._make_resource(request.id, MEMBERSHIP_CHANGE_REQUESTS)
-        # Remove unnecessary keys.
-        del resource['key']
-        return resource
-
-    def _get_collection(self, request):
-        requests = IListRequests(self._mlist)
-        self._requests = requests
-        items = []
-        for request_type in MEMBERSHIP_CHANGE_REQUESTS:
-            for request in requests.of_type(request_type):
-                items.append(request)
-        return items
-
-    def on_get(self, request, response):
-        """/lists/listname/requests"""
-        resource = self._make_collection(request)
-        okay(response, etag(resource))
-
-    @child(r'^(?P<id>[^/]+)')
-    def subscription(self, request, segments, **kw):
-        return MembershipChangeRequest(self._mlist, kw['id'])
 
=== added file 'src/mailman/rest/sub_moderation.py'
--- src/mailman/rest/sub_moderation.py	1970-01-01 00:00:00 +0000
+++ src/mailman/rest/sub_moderation.py	2015-04-17 15:30:55 +0000
@@ -0,0 +1,138 @@
+# Copyright (C) 2012-2015 by the Free Software Foundation, Inc.
+#
+# This file is part of GNU Mailman.
+#
+# GNU Mailman is free software: you can redistribute it and/or modify it under
+# the terms of the GNU General Public License as published by the Free
+# Software Foundation, either version 3 of the License, or (at your option)
+# any later version.
+#
+# GNU Mailman is distributed in the hope that it will be useful, but WITHOUT
+# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+# FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+# more details.
+#
+# You should have received a copy of the GNU General Public License along with
+# GNU Mailman.  If not, see <http://www.gnu.org/licenses/>.
+
+"""REST API for held subscription requests."""
+
+__all__ = [
+    'SubscriptionRequests',
+    ]
+
+
+from mailman.interfaces.action import Action
+from mailman.interfaces.pending import IPendings
+from mailman.interfaces.registrar import IRegistrar
+from mailman.rest.helpers import (
+    CollectionMixin, bad_request, child, etag, no_content, not_found, okay)
+from mailman.rest.validator import Validator, enum_validator
+from zope.component import getUtility
+
+
+
 
+class _ModerationBase:
+    """Common base class."""
+
+    def __init__(self):
+        self._pendings = getUtility(IPendings)
+
+    def _resource_as_dict(self, token):
+        pendable = self._pendings.confirm(token, expunge=False)
+        if pendable is None:
+            # This token isn't in the database.
+            raise LookupError
+        resource = dict(token=token)
+        resource.update(pendable)
+        return resource
+
+
+
 
+class IndividualRequest(_ModerationBase):
+    """Resource for moderating a membership change."""
+
+    def __init__(self, mlist, token):
+        super().__init__()
+        self._mlist = mlist
+        self._registrar = IRegistrar(self._mlist)
+        self._token = token
+
+    def on_get(self, request, response):
+        # Get the pended record associated with this token, if it exists in
+        # the pending table.
+        try:
+            resource = self._resource_as_dict(self._token)
+        except LookupError:
+            not_found(response)
+            return
+        okay(response, etag(resource))
+
+    def on_post(self, request, response):
+        try:
+            validator = Validator(action=enum_validator(Action))
+            arguments = validator(request)
+        except ValueError as error:
+            bad_request(response, str(error))
+            return
+        action = arguments['action']
+        if action is Action.defer:
+            # At least see if the token is in the database.
+            pendable = self._pendings.confirm(self._token, expunge=False)
+            if pendable is None:
+                not_found(response)
+            else:
+                no_content(response)
+        elif action is Action.accept:
+            try:
+                self._registrar.confirm(self._token)
+            except LookupError:
+                not_found(response)
+            else:
+                no_content(response)
+        elif action is Action.discard:
+            # At least see if the token is in the database.
+            pendable = self._pendings.confirm(self._token, expunge=True)
+            if pendable is None:
+                not_found(response)
+            else:
+                no_content(response)
+        elif action is Action.reject:
+            # XXX
+            no_content(response)
+
+
+
 
+class SubscriptionRequests(_ModerationBase, CollectionMixin):
+    """Resource for membership change requests."""
+
+    def __init__(self, mlist):
+        super().__init__()
+        self._mlist = mlist
+
+    def _get_collection(self, request):
+        # There's currently no better way to query the pendings database for
+        # all the entries that are associated with subscription holds on this
+        # mailing list.  Brute force iterating over all the pendables.
+        collection = []
+        for token, pendable in getUtility(IPendings):
+            if 'token_owner' not in pendable:
+                # This isn't a subscription hold.
+                continue
+            list_id = pendable.get('list_id')
+            if list_id != self._mlist.list_id:
+                # Either there isn't a list_id field, in which case it can't
+                # be a subscription hold, or this is a hold for some other
+                # mailing list.
+                continue
+            collection.append(token)
+        return collection
+
+    def on_get(self, request, response):
+        """/lists/listname/requests"""
+        resource = self._make_collection(request)
+        okay(response, etag(resource))
+
+    @child(r'^(?P<token>[^/]+)')
+    def subscription(self, request, segments, **kw):
+        return IndividualRequest(self._mlist, kw['token'])
 
=== modified file 'src/mailman/rest/tests/test_moderation.py'
--- src/mailman/rest/tests/test_moderation.py	2015-03-29 20:30:30 +0000
+++ src/mailman/rest/tests/test_moderation.py	2015-04-17 15:30:55 +0000
@@ -18,26 +18,28 @@
 """REST moderation tests."""
 
 __all__ = [
-    'TestModeration',
+    'TestPostModeration',
+    'TestSubscriptionModeration',
     ]
 
 
 import unittest
 
 from mailman.app.lifecycle import create_list
-from mailman.app.moderator import hold_message, hold_subscription
-from mailman.config import config
+from mailman.app.moderator import hold_message
 from mailman.database.transaction import transaction
-from mailman.interfaces.member import DeliveryMode
-from mailman.interfaces.subscriptions import RequestRecord
+from mailman.interfaces.registrar import IRegistrar
+from mailman.interfaces.usermanager import IUserManager
 from mailman.testing.helpers import (
-    call_api, specialized_message_from_string as mfs)
+    call_api, get_queue_messages, specialized_message_from_string as mfs)
 from mailman.testing.layers import RESTLayer
+from mailman.utilities.datetime import now
 from urllib.error import HTTPError
+from zope.component import getUtility
 
 
 
 
-class TestModeration(unittest.TestCase):
+class TestPostModeration(unittest.TestCase):
     layer = RESTLayer
 
     def setUp(self):
@@ -71,24 +73,6 @@
             call_api('http://localhost:9001/3.0/lists/[e-mail]/held/99')
         self.assertEqual(cm.exception.code, 404)
 
-    def test_subscription_request_as_held_message(self):
-        # Provide the request id of a subscription request using the held
-        # message API returns a not-found even though the request id is
-        # in the database.
-        held_id = hold_message(self._mlist, self._msg)
-        subscribe_id = hold_subscription(
-            self._mlist,
-            RequestRecord('[e-mail]', 'Bart Person',
-                          DeliveryMode.regular, 'en'))
-        config.db.store.commit()
-        url = 'http://localhost:9001/3.0/lists/[e-mail]/held/{0}'
-        with self.assertRaises(HTTPError) as cm:
-            call_api(url.format(subscribe_id))
-        self.assertEqual(cm.exception.code, 404)
-        # But using the held_id returns a valid response.
-        response, content = call_api(url.format(held_id))
-        self.assertEqual(response['message_id'], '<alpha>')
-
     def test_bad_held_message_action(self):
         # POSTing to a held message with a bad action.
         held_id = hold_message(self._mlist, self._msg)
@@ -99,34 +83,6 @@
         self.assertEqual(cm.exception.msg,
                          b'Cannot convert parameters: action')
 
-    def test_bad_subscription_request_id(self):
-        # Bad request when request_id is not an integer.
-        with self.assertRaises(HTTPError) as cm:
-            call_api('http://localhost:9001/3.0/lists/[e-mail]/'
-                     'requests/bogus')
-        self.assertEqual(cm.exception.code, 400)
-
-    def test_missing_subscription_request_id(self):
-        # Bad request when the request_id is not in the database.
-        with self.assertRaises(HTTPError) as cm:
-            call_api('http://localhost:9001/3.0/lists/[e-mail]/'
-                     'requests/99')
-        self.assertEqual(cm.exception.code, 404)
-
-    def test_bad_subscription_action(self):
-        # POSTing to a held message with a bad action.
-        held_id = hold_subscription(
-            self._mlist,
-            RequestRecord('[e-mail]', 'Cris Person',
-                          DeliveryMode.regular, 'en'))
-        config.db.store.commit()
-        url = 'http://localhost:9001/3.0/lists/[e-mail]/requests/{0}'
-        with self.assertRaises(HTTPError) as cm:
-            call_api(url.format(held_id), {'action': 'bogus'})
-        self.assertEqual(cm.exception.code, 400)
-        self.assertEqual(cm.exception.msg,
-                         b'Cannot convert parameters: action')
-
     def test_discard(self):
         # Discarding a message removes it from the moderation queue.
         with transaction():
@@ -139,3 +95,211 @@
         with self.assertRaises(HTTPError) as cm:
             call_api(url, dict(action='discard'))
         self.assertEqual(cm.exception.code, 404)
+
+
+
 
+class TestSubscriptionModeration(unittest.TestCase):
+    layer = RESTLayer
+
+    def setUp(self):
+        with transaction():
+            self._mlist = create_list('[e-mail]')
+            self._registrar = IRegistrar(self._mlist)
+            manager = getUtility(IUserManager)
+            self._anne = manager.create_address(
+                '[e-mail]', 'Anne Person')
+            self._bart = manager.make_user(
+                '[e-mail]', 'Bart Person')
+            preferred = list(self._bart.addresses)[0]
+            preferred.verified_on = now()
+            self._bart.preferred_address = preferred
+
+    def test_no_such_list(self):
+        # Try to get the requests of a nonexistent list.
+        with self.assertRaises(HTTPError) as cm:
+            call_api('http://localhost:9001/3.0/lists/[e-mail]/'
+                     'requests')
+        self.assertEqual(cm.exception.code, 404)
+
+    def test_no_such_subscription_token(self):
+        # Bad request when the token is not in the database.
+        with self.assertRaises(HTTPError) as cm:
+            call_api('http://localhost:9001/3.0/lists/[e-mail]/'
+                     'requests/missing')
+        self.assertEqual(cm.exception.code, 404)
+
+    def test_bad_subscription_action(self):
+        # POSTing to a held message with a bad action.
+        token, token_owner, member = self._registrar.register(self._anne)
+        # Anne's subscription request got held.
+        self.assertIsNone(member)
+        # Let's try to handle her request, but with a bogus action.
+        url = 'http://localhost:9001/3.0/lists/[e-mail]/requests/{}'
+        with self.assertRaises(HTTPError) as cm:
+            call_api(url.format(token), dict(
+                action='bogus',
+                ))
+        self.assertEqual(cm.exception.code, 400)
+        self.assertEqual(cm.exception.msg,
+                         b'Cannot convert parameters: action')
+
+    def test_list_held_requests(self):
+        # We can view all the held requests.
+        with transaction():
+            token_1, token_owner, member = self._registrar.register(self._anne)
+            # Anne's subscription request got held.
+            self.assertIsNotNone(token_1)
+            self.assertIsNone(member)
+            token_2, token_owner, member = self._registrar.register(self._bart)
+            self.assertIsNotNone(token_2)
+            self.assertIsNone(member)
+        content, response = call_api(
+            'http://localhost:9001/3.0/lists/[e-mail]/requests')
+        self.assertEqual(response.status, 200)
+        self.assertEqual(content['total_size'], 2)
+        tokens = set(json['token'] for json in content['entries'])
+        self.assertEqual(tokens, {token_1, token_2})
+        emails = set(json['email'] for json in content['entries'])
+        self.assertEqual(emails, {'[e-mail]', '[e-mail]'})
+
+    def test_individual_request(self):
+        # We can view an individual request.
+        with transaction():
+            token, token_owner, member = self._registrar.register(self._anne)
+            # Anne's subscription request got held.
+            self.assertIsNotNone(token)
+            self.assertIsNone(member)
+        url = 'http://localhost:9001/3.0/lists/[e-mail]/requests/{}'
+        content, response = call_api(url.format(token))
+        self.assertEqual(response.status, 200)
+        self.assertEqual(content['token'], token)
+        self.assertEqual(content['token_owner'], token_owner.name)
+        self.assertEqual(content['email'], '[e-mail]')
+
+    def test_accept(self):
+        # POST to the request to accept it.
+        with transaction():
+            token, token_owner, member = self._registrar.register(self._anne)
+        # Anne's subscription request got held.
+        self.assertIsNone(member)
+        url = 'http://localhost:9001/3.0/lists/[e-mail]/requests/{}'
+        content, response = call_api(url.format(token), dict(
+            action='accept',
+            ))
+        self.assertEqual(response.status, 204)
+        # Anne is a member.
+        self.assertEqual(
+            self._mlist.members.get_member('[e-mail]').address,
+            self._anne)
+        # The request URL no longer exists.
+        with self.assertRaises(HTTPError) as cm:
+            call_api(url.format(token), dict(
+                action='accept',
+                ))
+        self.assertEqual(cm.exception.code, 404)
+
+    def test_accept_bad_token(self):
+        # Try to accept a request with a bogus token.
+        with self.assertRaises(HTTPError) as cm:
+            call_api('http://localhost:9001/3.0/lists/[e-mail]'
+                     '/requests/bogus',
+                     dict(action='accept'))
+        self.assertEqual(cm.exception.code, 404)
+
+    def test_discard(self):
+        # POST to the request to discard it.
+        with transaction():
+            token, token_owner, member = self._registrar.register(self._anne)
+        # Anne's subscription request got held.
+        self.assertIsNone(member)
+        url = 'http://localhost:9001/3.0/lists/[e-mail]/requests/{}'
+        content, response = call_api(url.format(token), dict(
+            action='discard',
+            ))
+        self.assertEqual(response.status, 204)
+        # Anne is not a member.
+        self.assertIsNone(self._mlist.members.get_member('[e-mail]'))
+        # The request URL no longer exists.
+        with self.assertRaises(HTTPError) as cm:
+            call_api(url.format(token), dict(
+                action='discard',
+                ))
+        self.assertEqual(cm.exception.code, 404)
+
+    def test_defer(self):
+        # Defer the decision for some other moderator.
+        with transaction():
+            token, token_owner, member = self._registrar.register(self._anne)
+        # Anne's subscription request got held.
+        self.assertIsNone(member)
+        url = 'http://localhost:9001/3.0/lists/[e-mail]/requests/{}'
+        content, response = call_api(url.format(token), dict(
+            action='defer',
+            ))
+        self.assertEqual(response.status, 204)
+        # Anne is not a member.
+        self.assertIsNone(self._mlist.members.get_member('[e-mail]'))
+        # The request URL still exists.
+        content, response = call_api(url.format(token), dict(
+            action='defer',
+            ))
+        self.assertEqual(response.status, 204)
+        # And now we can accept it.
+        content, response = call_api(url.format(token), dict(
+            action='accept',
+            ))
+        self.assertEqual(response.status, 204)
+        # Anne is a member.
+        self.assertEqual(
+            self._mlist.members.get_member('[e-mail]').address,
+            self._anne)
+        # The request URL no longer exists.
+        with self.assertRaises(HTTPError) as cm:
+            call_api(url.format(token), dict(
+                action='accept',
+                ))
+        self.assertEqual(cm.exception.code, 404)
+
+    def test_defer_bad_token(self):
+        # Try to accept a request with a bogus token.
+        with self.assertRaises(HTTPError) as cm:
+            call_api('http://localhost:9001/3.0/lists/[e-mail]'
+                     '/requests/bogus',
+                     dict(action='defer'))
+        self.assertEqual(cm.exception.code, 404)
+
+    def test_reject(self):
+        # POST to the request to reject it.  This leaves a bounce message in
+        # the virgin queue.
+        with transaction():
+            token, token_owner, member = self._registrar.register(self._anne)
+        # Anne's subscription request got held.
+        self.assertIsNone(member)
+        # There are currently no messages in the virgin queue.
+        items = get_queue_messages('virgin')
+        self.assertEqual(len(items), 0)
+        url = 'http://localhost:9001/3.0/lists/[e-mail]/requests/{}'
+        content, response = call_api(url.format(token), dict(
+            action='reject',
+            ))
+        self.assertEqual(response.status, 204)
+        # Anne is not a member.
+        self.assertIsNone(self._mlist.members.get_member('[e-mail]'))
+        # The request URL no longer exists.
+        with self.assertRaises(HTTPError) as cm:
+            call_api(url.format(token), dict(
+                action='reject',
+                ))
+        self.assertEqual(cm.exception.code, 404)
+        # And the rejection message is now in the virgin queue.
+        items = get_queue_messages('virgin')
+        self.assertEqual(len(items), 1)
+        self.assertEqual(str(items[0].msg), '')
+
+    def test_reject_bad_token(self):
+        # Try to accept a request with a bogus token.
+        with self.assertRaises(HTTPError) as cm:
+            call_api('http://localhost:9001/3.0/lists/[e-mail]'
+                     '/requests/bogus',
+                     dict(action='reject'))
+        self.assertEqual(cm.exception.code, 404)