3 files changed, 304 insertions, 64 deletions

diff --git a/src/mailman/rest/docs/listconf.rst b/src/mailman/rest/docs/listconf.rst
index bcf4f856e..319028be5 100644
--- a/src/mailman/rest/docs/listconf.rst
+++ b/src/mailman/rest/docs/listconf.rst
@@ -186,10 +186,56 @@ These values are changed permanently.
 Sub-resources
 =============
 
-Many of the mailing list configuration variables are actually available as
-sub-resources on the mailing list.  This is because they are collections,
-sequences, and other complex configuration types.  Their values can be
-retrieved and set through the sub-resource.
+Mailing list configuration variables are actually available as sub-resources
+on the mailing list.  Their values can be retrieved and set through the
+sub-resource.
+
+
+Simple resources
+----------------
+
+You can view the current value of the sub-resource.
+
+    >>> dump_json('http://localhost:9001/3.0/lists/ant.example.com'
+    ...           '/config/display_name')
+    display_name: My List
+    http_etag: ...
+
+The resource can be changed by PUTting to it.  Note that the value still
+requires a dictionary, and that dictionary must have a single key matching the
+name of the resource.
+::
+
+    >>> dump_json('http://localhost:9001/3.0/lists/ant.example.com'
+    ...           '/config/display_name',
+    ...           dict(display_name='Your List'),
+    ...           'PUT')
+    content-length: 0
+    date: ...
+    server: ...
+    status: 204
+
+    >>> dump_json('http://localhost:9001/3.0/lists/ant.example.com'
+    ...           '/config/display_name')
+    display_name: Your List
+    http_etag: ...
+
+PATCH works the same way, with the same effect, so you can choose to use
+either method.
+
+    >>> dump_json('http://localhost:9001/3.0/lists/ant.example.com'
+    ...           '/config/display_name',
+    ...           dict(display_name='Their List'),
+    ...           'PATCH')
+    content-length: 0
+    date: ...
+    server: ...
+    status: 204
+
+    >>> dump_json('http://localhost:9001/3.0/lists/ant.example.com'
+    ...           '/config/display_name')
+    display_name: Their List
+    http_etag: ...
 
 
 Acceptable aliases
diff --git a/src/mailman/rest/listconf.py b/src/mailman/rest/listconf.py
index cea8adb03..632c574cb 100644
--- a/src/mailman/rest/listconf.py
+++ b/src/mailman/rest/listconf.py
@@ -32,7 +32,7 @@ from mailman.interfaces.autorespond import ResponseAction
 from mailman.interfaces.mailinglist import (
     IAcceptableAliasSet, ReplyToMunging, SubscriptionPolicy)
 from mailman.rest.helpers import (
-    GetterSetter, bad_request, etag, no_content, okay)
+    GetterSetter, bad_request, etag, no_content, not_found, okay)
 from mailman.rest.validator import (
     PatchValidator, Validator, enum_validator, list_of_strings_validator)
 
@@ -44,7 +44,7 @@ class AcceptableAliases(GetterSetter):
     def get(self, mlist, attribute):
         """Return the mailing list's acceptable aliases."""
         assert attribute == 'acceptable_aliases', (
-            'Unexpected attribute: {}'.format(attribute))
+            'Unexpected attribute: {}'.format(attribute))   # pragma: no cover
         aliases = IAcceptableAliasSet(mlist)
         return sorted(aliases.aliases)
 
@@ -56,7 +56,7 @@ class AcceptableAliases(GetterSetter):
         ignored.
         """
         assert attribute == 'acceptable_aliases', (
-            'Unexpected attribute: {}'.format(attribute))
+            'Unexpected attribute: {}'.format(attribute))   # pragma: no cover
         alias_set = IAcceptableAliasSet(mlist)
         alias_set.clear()
         for alias in value:
@@ -112,9 +112,9 @@ ATTRIBUTES = dict(
     default_nonmember_action=GetterSetter(enum_validator(Action)),
     description=GetterSetter(str),
     digest_last_sent_at=GetterSetter(None),
-    digest_send_periodic=GetterSetter(as_boolean),
+    #digest_send_periodic=GetterSetter(as_boolean),
     digest_size_threshold=GetterSetter(float),
-    digest_volume_frequency=GetterSetter(int),
+    #digest_volume_frequency=GetterSetter(int),
     filter_content=GetterSetter(as_boolean),
     first_strip_reply_to=GetterSetter(as_boolean),
     fqdn_listname=GetterSetter(None),
@@ -163,15 +163,18 @@ class ListConfiguration:
         """Get a mailing list configuration."""
         resource = {}
         if self._attribute is None:
-            # Return all readable attributes.
+            # This is a requst for all the mailing list's configuration
+            # variables.  Return all readable attributes.
             for attribute in ATTRIBUTES:
                 value = ATTRIBUTES[attribute].get(self._mlist, attribute)
                 resource[attribute] = value
         elif self._attribute not in ATTRIBUTES:
-            bad_request(
-                response, b'Unknown attribute: {}'.format(self._attribute))
+            # This is a request for a specific, nonexistent attribute.
+            not_found(
+                response, 'Unknown attribute: {}'.format(self._attribute))
             return
         else:
+            # This is a request for a specific attribute.
             attribute = self._attribute
             value = ATTRIBUTES[attribute].get(self._mlist, attribute)
             resource[attribute] = value
@@ -181,20 +184,31 @@ class ListConfiguration:
         """Set a mailing list configuration."""
         attribute = self._attribute
         if attribute is None:
+            # This is a request to update all the list's writable
+            # configuration variables.  All must be provided in the request.
             validator = Validator(**VALIDATORS)
             try:
                 validator.update(self._mlist, request)
             except ValueError as error:
+                # Unlike the case where we're PUTting to a specific
+                # configuration sub-resource, if we're PUTting to the list's
+                # entire configuration, but the request has a bogus attribute,
+                # the entire request is considered bad.  We can also get here
+                # if one of the attributes is read-only.  The error will
+                # contain sufficient details, so just return it as the reason.
                 bad_request(response, str(error))
                 return
         elif attribute not in ATTRIBUTES:
-            bad_request(response, b'Unknown attribute: {}'.format(attribute))
+            # Here we're PUTting to a specific resource, but that attribute is
+            # bogus so the URL is considered pointing to a missing resource.
+            not_found(response, 'Unknown attribute: {}'.format(attribute))
             return
         elif ATTRIBUTES[attribute].decoder is None:
             bad_request(
-                response, b'Read-only attribute: {}'.format(attribute))
+                response, 'Read-only attribute: {}'.format(attribute))
             return
         else:
+            # We're PUTting to a specific configuration sub-resource.
             validator = Validator(**{attribute: VALIDATORS[attribute]})
             try:
                 validator.update(self._mlist, request)
@@ -205,15 +219,42 @@ class ListConfiguration:
 
     def on_patch(self, request, response):
         """Patch the configuration (i.e. partial update)."""
+        if self._attribute is None:
+            # We're PATCHing one more attributes on the list's configuration
+            # resource, so all the writable attributes are valid candidates
+            # for updating.
+            converters = ATTRIBUTES
+        else:
+            # We're PATCHing a specific list configuration attribute
+            # sub-resource.  Because the request data must be a dictionary, we
+            # restrict it to containing only a single key, which must match
+            # the attribute name.  First, check for any extra attributes in
+            # the request.
+            keys = [key for key, value in request.params.items()]
+            if len(keys) > 1:
+                bad_request(response, 'Expected 1 attribute, got {}'.format(
+                    len(keys)))
+                return
+            converter = ATTRIBUTES.get(self._attribute)
+            if converter is None:
+                # This is the case where the URL points to a nonexisting list
+                # configuration attribute sub-resource.
+                not_found(response, 'Unknown attribute: {}'.format(
+                    self._attribute))
+                return
+            converters = {self._attribute: converter}
         try:
-            validator = PatchValidator(request, ATTRIBUTES)
+            validator = PatchValidator(request, converters)
         except UnknownPATCHRequestError as error:
-            bad_request(
-                response, b'Unknown attribute: {}'.format(error.attribute))
+            # This is the case where the URL points to the list's entire
+            # configuration resource, but the request dictionary contains a
+            # nonexistent attribute.
+            not_found(
+                response, 'Unknown attribute: {}'.format(error.attribute))
             return
         except ReadOnlyPATCHRequestError as error:
             bad_request(
-                response, b'Read-only attribute: {}'.format(error.attribute))
+                response, 'Read-only attribute: {}'.format(error.attribute))
             return
         try:
             validator.update(self._mlist, request)
diff --git a/src/mailman/rest/tests/test_listconf.py b/src/mailman/rest/tests/test_listconf.py
index 37eec5f26..92e616ef7 100644
--- a/src/mailman/rest/tests/test_listconf.py
+++ b/src/mailman/rest/tests/test_listconf.py
@@ -30,6 +30,51 @@ from mailman.interfaces.mailinglist import (
     IAcceptableAliasSet, SubscriptionPolicy)
 from mailman.testing.helpers import call_api
 from mailman.testing.layers import RESTLayer
+from urllib.error import HTTPError
+
+
+# The representation of the listconf resource as a dictionary.  This is
+# required when PUTting to the list's configuration resource.
+RESOURCE = dict(
+    acceptable_aliases=[
+        'ant@example.com',
+        'bee@example.com',
+        'cat@example.com',
+        ],
+    admin_immed_notify=False,
+    admin_notify_mchanges=True,
+    administrivia=False,
+    advertised=False,
+    anonymous_list=True,
+    archive_policy='never',
+    autorespond_owner='respond_and_discard',
+    autorespond_postings='respond_and_continue',
+    autorespond_requests='respond_and_discard',
+    autoresponse_grace_period='45d',
+    autoresponse_owner_text='the owner',
+    autoresponse_postings_text='the mailing list',
+    autoresponse_request_text='the robot',
+    display_name='Fnords',
+    description='This is my mailing list',
+    include_rfc2369_headers=False,
+    allow_list_posts=False,
+    #digest_send_periodic='yes',
+    digest_size_threshold=10.5,
+    #digest_volume_frequency=1,
+    posting_pipeline='virgin',
+    filter_content=True,
+    first_strip_reply_to=True,
+    convert_html_to_plaintext=True,
+    collapse_alternatives=False,
+    reply_goes_to_list='point_to_list',
+    reply_to_address='bee@example.com',
+    send_welcome_message=False,
+    subject_prefix='[ant]',
+    subscription_policy='confirm_then_moderate',
+    welcome_message_uri='mailman:///welcome.txt',
+    default_member_action='hold',
+    default_nonmember_action='discard',
+    )
 
 
 
@@ -40,73 +85,181 @@ class TestConfiguration(unittest.TestCase):
 
     def setUp(self):
         with transaction():
-            self._mlist = create_list('test@example.com')
+            self._mlist = create_list('ant@example.com')
+
+    def test_get_missing_attribute(self):
+        with self.assertRaises(HTTPError) as cm:
+            call_api(
+                'http://localhost:9001/3.0/lists/ant.example.com/config/bogus')
+        self.assertEqual(cm.exception.code, 404)
+        self.assertEqual(cm.exception.reason, b'Unknown attribute: bogus')
 
     def test_put_configuration(self):
-        aliases = [
-            'ant@example.com',
-            'bee@example.com',
-            'cat@example.com',
-            ]
         # When using PUT, all writable attributes must be included.
         resource, response = call_api(
-            'http://localhost:9001/3.0/lists/test@example.com/config',
-            dict(
-                acceptable_aliases=aliases,
-                admin_immed_notify=False,
-                admin_notify_mchanges=True,
-                administrivia=False,
-                advertised=False,
-                anonymous_list=True,
-                archive_policy='never',
-                autorespond_owner='respond_and_discard',
-                autorespond_postings='respond_and_continue',
-                autorespond_requests='respond_and_discard',
-                autoresponse_grace_period='45d',
-                autoresponse_owner_text='the owner',
-                autoresponse_postings_text='the mailing list',
-                autoresponse_request_text='the robot',
-                display_name='Fnords',
-                description='This is my mailing list',
-                include_rfc2369_headers=False,
-                allow_list_posts=False,
-                digest_send_periodic='yes',
-                digest_size_threshold=10.5,
-                digest_volume_frequency=1,
-                posting_pipeline='virgin',
-                filter_content=True,
-                first_strip_reply_to=True,
-                convert_html_to_plaintext=True,
-                collapse_alternatives=False,
-                reply_goes_to_list='point_to_list',
-                reply_to_address='bee@example.com',
-                send_welcome_message=False,
-                subject_prefix='[ant]',
-                subscription_policy='confirm_then_moderate',
-                welcome_message_uri='mailman:///welcome.txt',
-                default_member_action='hold',
-                default_nonmember_action='discard',
-                ),
+            'http://localhost:9001/3.0/lists/ant.example.com/config',
+            RESOURCE,
             'PUT')
         self.assertEqual(response.status, 204)
         self.assertEqual(self._mlist.display_name, 'Fnords')
         # All three acceptable aliases were set.
         self.assertEqual(set(IAcceptableAliasSet(self._mlist).aliases),
-                         set(aliases))
+                         set(RESOURCE['acceptable_aliases']))
+
+    def test_put_attribute(self):
+        resource, response = call_api(
+            'http://localhost:9001/3.0/lists/ant.example.com'
+            '/config/reply_to_address')
+        self.assertEqual(resource['reply_to_address'], '')
+        resource, response = call_api(
+            'http://localhost:9001/3.0/lists/ant.example.com'
+            '/config/reply_to_address',
+            dict(reply_to_address='bar@ant.example.com'),
+            'PUT')
+        self.assertEqual(response.status, 204)
+        resource, response = call_api(
+            'http://localhost:9001/3.0/lists/ant.example.com'
+            '/config/reply_to_address')
+        self.assertEqual(resource['reply_to_address'], 'bar@ant.example.com')
+
+    def test_put_extra_attribute(self):
+        bogus_resource = RESOURCE.copy()
+        bogus_resource['bogus'] = 'yes'
+        with self.assertRaises(HTTPError) as cm:
+            call_api(
+                'http://localhost:9001/3.0/lists/ant.example.com/config',
+                bogus_resource,
+                'PUT')
+        self.assertEqual(cm.exception.code, 400)
+        self.assertEqual(cm.exception.reason, b'Unexpected parameters: bogus')
+
+    def test_put_attribute_mismatch(self):
+        resource, response = call_api(
+            'http://localhost:9001/3.0/lists/ant.example.com'
+            '/config/reply_to_address')
+        self.assertEqual(resource['reply_to_address'], '')
+        with self.assertRaises(HTTPError) as cm:
+            resource, response = call_api(
+                'http://localhost:9001/3.0/lists/ant.example.com'
+                '/config/reply_to_address',
+                dict(display_name='bar@ant.example.com'),
+                'PUT')
+        self.assertEqual(cm.exception.code, 400)
+        self.assertEqual(cm.exception.reason,
+                         b'Unexpected parameters: display_name')
+
+    def test_put_attribute_double(self):
+        with self.assertRaises(HTTPError) as cm:
+            resource, response = call_api(
+                'http://localhost:9001/3.0/lists/ant.example.com'
+                '/config/reply_to_address',
+                dict(display_name='bar@ant.example.com',
+                     reply_to_address='foo@example.com'),
+                'PUT')
+        self.assertEqual(cm.exception.code, 400)
+        self.assertEqual(cm.exception.reason,
+                         b'Unexpected parameters: display_name')
+
+    def test_put_read_only_attribute(self):
+        with self.assertRaises(HTTPError) as cm:
+            call_api('http://localhost:9001/3.0/lists/ant.example.com'
+                     '/config/mail_host',
+                     dict(mail_host='foo.example.com'),
+                    'PUT')
+        self.assertEqual(cm.exception.code, 400)
+        self.assertEqual(cm.exception.reason,
+                         b'Read-only attribute: mail_host')
+
+    def test_put_missing_attribute(self):
+        with self.assertRaises(HTTPError) as cm:
+            call_api(
+                'http://localhost:9001/3.0/lists/ant.example.com/config/bogus',
+                dict(bogus='no matter'),
+                'PUT')
+        self.assertEqual(cm.exception.code, 404)
+        self.assertEqual(cm.exception.reason, b'Unknown attribute: bogus')
 
     def test_patch_subscription_policy(self):
         # The new subscription_policy value can be patched.
         #
         # To start with, the subscription policy is confirm by default.
         resource, response = call_api(
-            'http://localhost:9001/3.0/lists/test@example.com/config')
+            'http://localhost:9001/3.0/lists/ant.example.com/config')
         self.assertEqual(resource['subscription_policy'], 'confirm')
         # Let's patch it to do some moderation.
         resource, response = call_api(
-            'http://localhost:9001/3.0/lists/test@example.com/config', dict(
+            'http://localhost:9001/3.0/lists/ant.example.com/config', dict(
                 subscription_policy='confirm_then_moderate'),
             method='PATCH')
         self.assertEqual(response.status, 204)
         # And now we verify that it has the requested setting.
         self.assertEqual(self._mlist.subscription_policy,
                          SubscriptionPolicy.confirm_then_moderate)
+
+    ## def test_digest_volume_frequency(self):
+    ##     resource, response = call_api(
+    ##         'http://localhost:9001/3.0/lists/test.example.com/config',
+    ##         dict(digest_volume_frequency=DigestFrequency.never),
+    ##         'PATCH')
+    ##     self.assertEqual(self._mlist.digest_volume_frequency,
+    ##                      DigestFrequency.never)
+
+    def test_patch_attribute_double(self):
+        with self.assertRaises(HTTPError) as cm:
+            resource, response = call_api(
+                'http://localhost:9001/3.0/lists/ant.example.com'
+                '/config/reply_to_address',
+                dict(display_name='bar@ant.example.com',
+                     reply_to_address='foo'),
+                'PATCH')
+        self.assertEqual(cm.exception.code, 400)
+        self.assertEqual(cm.exception.reason, b'Expected 1 attribute, got 2')
+
+    def test_unknown_patch_attribute(self):
+        with self.assertRaises(HTTPError) as cm:
+            call_api('http://localhost:9001/3.0/lists/ant.example.com/config',
+                    dict(bogus=1),
+                    'PATCH')
+        self.assertEqual(cm.exception.code, 404)
+        self.assertEqual(cm.exception.reason, b'Unknown attribute: bogus')
+
+    def test_read_only_patch_attribute(self):
+        with self.assertRaises(HTTPError) as cm:
+            call_api('http://localhost:9001/3.0/lists/ant.example.com'
+                     '/config/mail_host',
+                     dict(mail_host='foo.example.com'),
+                    'PATCH')
+        self.assertEqual(cm.exception.code, 400)
+        self.assertEqual(cm.exception.reason,
+                         b'Read-only attribute: mail_host')
+
+    def test_patch_missing_attribute(self):
+        with self.assertRaises(HTTPError) as cm:
+            call_api(
+                'http://localhost:9001/3.0/lists/ant.example.com/config/bogus',
+                dict(bogus='no matter'),
+                'PATCH')
+        self.assertEqual(cm.exception.code, 404)
+        self.assertEqual(cm.exception.reason, b'Unknown attribute: bogus')
+
+    def test_patch_bad_value(self):
+        with self.assertRaises(HTTPError) as cm:
+            call_api(
+                'http://localhost:9001/3.0/lists/ant.example.com/config'
+                '/archive_policy',
+                dict(archive_policy='not a valid archive policy'),
+                'PATCH')
+        self.assertEqual(cm.exception.code, 400)
+        self.assertEqual(cm.exception.reason,
+                         b'Cannot convert parameters: archive_policy')
+
+    def test_bad_pipeline_name(self):
+        with self.assertRaises(HTTPError) as cm:
+            call_api(
+                'http://localhost:9001/3.0/lists/ant.example.com/config'
+                '/posting_pipeline',
+                dict(posting_pipeline='not a valid pipeline'),
+                'PATCH')
+        self.assertEqual(cm.exception.code, 400)
+        self.assertEqual(cm.exception.reason,
+                         b'Cannot convert parameters: posting_pipeline')