4 files changed, 132 insertions, 46 deletions

diff --git a/src/mailman/docs/NEWS.rst b/src/mailman/docs/NEWS.rst
index f535b72f8..9cdc8145d 100644
--- a/src/mailman/docs/NEWS.rst
+++ b/src/mailman/docs/NEWS.rst
@@ -131,6 +131,8 @@ REST
    values accessible through the list's configuraiton resource.  POSTing to
    the resource with either ``send=True``, ``bump=True``, or both invokes the
    given action.
+ * Global and list-centric bans can now be managed through the REST API.
+   Given by Aurélien Bompard.
 
 Other
 -----
diff --git a/src/mailman/rest/bans.py b/src/mailman/rest/bans.py
index b6c25b749..ac0a1ca7f 100644
--- a/src/mailman/rest/bans.py
+++ b/src/mailman/rest/bans.py
@@ -1,4 +1,4 @@
-# Copyright (C) 2015 by the Free Software Foundation, Inc.
+# Copyright (C) 2016 by the Free Software Foundation, Inc.
 #
 # This file is part of GNU Mailman.
 #
@@ -26,16 +26,15 @@ __all__ = [
 from mailman.interfaces.bans import IBanManager
 from mailman.rest.helpers import (
     CollectionMixin, bad_request, child, created, etag, no_content, not_found,
-    okay, path_to)
+    okay)
 from mailman.rest.validator import Validator
 
 
-
 class _BannedBase:
     """Common base class."""
 
-    def __init__(self, mailing_list):
-        self._mlist = mailing_list
+    def __init__(self, mlist):
+        self._mlist = mlist
         self.ban_manager = IBanManager(self._mlist)
 
     def _location(self, email):
@@ -43,42 +42,36 @@ class _BannedBase:
             base_location = ''
         else:
             base_location = 'lists/{}/'.format(self._mlist.list_id)
-        return path_to(
-            '{}bans/{}'.format(base_location, email), self.api_version)
+        return self.api.path_to('{}bans/{}'.format(base_location, email))
 
 
 class BannedEmail(_BannedBase):
     """A banned email."""
 
-    def __init__(self, mailing_list, email):
-        super().__init__(mailing_list)
+    def __init__(self, mlist, email):
+        super().__init__(mlist)
         self._email = email
 
     def on_get(self, request, response):
         """Get a banned email."""
-        if self._email is None:
-            bad_request(response, 'Invalid email')
-        elif not self.ban_manager.is_banned(self._email):
-            not_found(
-                response, 'Email {} is not banned'.format(self._email))
-        else:
+        if self.ban_manager.is_banned(self._email):
             resource = dict(
                 email=self._email,
-                list_id=self._mlist.list_id if self._mlist else None,
                 self_link=self._location(self._email),
                 )
+            if self._mlist is not None:
+                resource['list_id'] = self._mlist.list_id
             okay(response, etag(resource))
+        else:
+            not_found(response, 'Email is not banned: {}'.format(self._email))
 
     def on_delete(self, request, response):
         """Remove an email from the ban list."""
-        if self._email is None:
-            bad_request(response, 'Invalid email')
-        elif not self.ban_manager.is_banned(self._email):
-            bad_request(
-                response, 'Email {} is not banned'.format(self._email))
-        else:
+        if self.ban_manager.is_banned(self._email):
             self.ban_manager.unban(self._email)
             no_content(response)
+        else:
+            not_found(response, 'Email is not banned: {}'.format(self._email))
 
 
 class BannedEmails(_BannedBase, CollectionMixin):
@@ -86,11 +79,13 @@ class BannedEmails(_BannedBase, CollectionMixin):
 
     def _resource_as_dict(self, ban):
         """See `CollectionMixin`."""
-        return dict(
+        resource = dict(
             email=ban.email,
-            list_id=ban.list_id,
             self_link=self._location(ban.email),
             )
+        if ban.list_id is not None:
+            resource['list_id'] = ban.list_id
+        return resource
 
     def _get_collection(self, request):
         """See `CollectionMixin`."""
diff --git a/src/mailman/rest/docs/membership.rst b/src/mailman/rest/docs/membership.rst
index 4c46e8e29..61ac2c33e 100644
--- a/src/mailman/rest/docs/membership.rst
+++ b/src/mailman/rest/docs/membership.rst
@@ -970,61 +970,55 @@ The moderation action for a member can be changed by PATCH'ing the
 Handling the list of banned addresses
 =====================================
 
-To ban an address from subscribing you can POST to the /bans child
+To ban an address from subscribing you can POST to the ``/bans`` child
 of any list using the REST API.
-::
 
     >>> dump_json('http://localhost:9001/3.0/lists/ant.example.com/bans',
     ...           {'email': 'banned@example.com'})
     content-length: 0
     ...
-    location: http://localhost:9001/3.0/lists/ant.example.com/bans/banned@example.com
+    location: .../3.0/lists/ant.example.com/bans/banned@example.com
     ...
     status: 201
 
 This address is now banned, and you can get the list of banned addresses by
-issuing a GET request on the /bans child::
+issuing a GET request on the ``/bans`` child.
 
     >>> dump_json('http://localhost:9001/3.0/lists/ant.example.com/bans')
     entry 0:
         email: banned@example.com
         http_etag: "..."
         list_id: ant.example.com
-        self_link: http://localhost:9001/3.0/lists/ant.example.com/bans/banned@example.com
+        self_link: .../3.0/lists/ant.example.com/bans/banned@example.com
     ...
 
-Or checking if a single address is banned:
+You can always GET a single banned address.
 
-    >>> dump_json('http://localhost:9001/3.0/lists/ant.example.com/bans/banned@example.com')
+    >>> dump_json('http://localhost:9001/3.0/lists/ant.example.com'
+    ...           '/bans/banned@example.com')
     email: banned@example.com
     http_etag: "..."
     list_id: ant.example.com
-    self_link: http://localhost:9001/3.0/lists/ant.example.com/bans/banned@example.com
-    >>> dump_json('http://localhost:9001/3.0/lists/ant.example.com/bans/someone-else@example.com')
-    Traceback (most recent call last):
-    ...
-    urllib.error.HTTPError: HTTP Error 404: ...
+    self_link: .../3.0/lists/ant.example.com/bans/banned@example.com
 
-Unbanning addresses is also possible by issuing a DELETE request::
+Unbanning addresses is also possible by issuing a DELETE request.
 
-    >>> dump_json('http://localhost:9001/3.0/lists/ant.example.com/bans/banned@example.com',
+    >>> dump_json('http://localhost:9001/3.0/lists/ant.example.com'
+    ...           '/bans/banned@example.com',
     ...           method='DELETE')
     content-length: 0
     ...
     status: 204
 
-After unbanning, the address is not shown in the ban list anymore::
+After unbanning, the address is not shown in the ban list anymore.
 
-    >>> dump_json('http://localhost:9001/3.0/lists/ant.example.com/bans/banned@example.com')
-    Traceback (most recent call last):
-    ...
-    urllib.error.HTTPError: HTTP Error 404: ...
     >>> dump_json('http://localhost:9001/3.0/lists/ant.example.com/bans')
     http_etag: "..."
     start: 0
     total_size: 0
 
-To ban an address from subscribing to every list, you can use the global /bans endpoint::
+Global bans prevent an address from subscribing to any mailing list, and they
+can be added via the top-level ``bans`` resource.
 
     >>> dump_json('http://localhost:9001/3.0/bans',
     ...           {'email': 'banned@example.com'})
@@ -1033,23 +1027,30 @@ To ban an address from subscribing to every list, you can use the global /bans e
     location: http://localhost:9001/3.0/bans/banned@example.com
     ...
     status: 201
+
+Note that entries in the global bans do not have a ``list_id`` field.
+::
+
     >>> dump_json('http://localhost:9001/3.0/bans')
     entry 0:
         email: banned@example.com
         http_etag: "..."
-        list_id: None
         self_link: http://localhost:9001/3.0/bans/banned@example.com
     ...
+
     >>> dump_json('http://localhost:9001/3.0/bans/banned@example.com')
     email: banned@example.com
     http_etag: "..."
-    list_id: None
     self_link: http://localhost:9001/3.0/bans/banned@example.com
+
+As with list-centric bans, you can delete a global ban.
+
     >>> dump_json('http://localhost:9001/3.0/bans/banned@example.com',
     ...           method='DELETE')
     content-length: 0
     ...
     status: 204
+
     >>> dump_json('http://localhost:9001/3.0/bans/banned@example.com')
     Traceback (most recent call last):
     ...
diff --git a/src/mailman/rest/tests/test_bans.py b/src/mailman/rest/tests/test_bans.py
new file mode 100644
index 000000000..ce6d8c843
--- /dev/null
+++ b/src/mailman/rest/tests/test_bans.py
@@ -0,0 +1,88 @@
+# Copyright (C) 2016 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/>.
+
+"""Test address bans."""
+
+__all__ = [
+    'TestBans',
+    ]
+
+
+import unittest
+
+from mailman.app.lifecycle import create_list
+from mailman.database.transaction import transaction
+from mailman.interfaces.bans import IBanManager
+from mailman.testing.layers import RESTLayer
+from mailman.testing.helpers import call_api
+from urllib.error import HTTPError
+
+
+class TestBans(unittest.TestCase):
+    layer = RESTLayer
+
+    def setUp(self):
+        with transaction():
+            self._mlist = create_list('ant@example.com')
+
+    def test_get_missing_banned_address(self):
+        with self.assertRaises(HTTPError) as cm:
+            call_api('http://localhost:9001/3.0/lists/ant.example.com'
+                     '/bans/notbanned@example.com')
+        self.assertEqual(cm.exception.code, 404)
+        self.assertEqual(cm.exception.reason,
+                         b'Email is not banned: notbanned@example.com')
+
+    def test_delete_missing_banned_address(self):
+        with self.assertRaises(HTTPError) as cm:
+            call_api('http://localhost:9001/3.0/lists/ant.example.com'
+                     '/bans/notbanned@example.com',
+                     method='DELETE')
+        self.assertEqual(cm.exception.code, 404)
+        self.assertEqual(cm.exception.reason,
+                         b'Email is not banned: notbanned@example.com')
+
+    def test_not_found_after_unbanning(self):
+        manager = IBanManager(self._mlist)
+        with transaction():
+            manager.ban('banned@example.com')
+        url = ('http://localhost:9001/3.0/lists/ant.example.com'
+               '/bans/banned@example.com')
+        response, content = call_api(url)
+        self.assertEqual(response['email'], 'banned@example.com')
+        response, content = call_api(url, method='DELETE')
+        self.assertEqual(content.status, 204)
+        with self.assertRaises(HTTPError) as cm:
+            call_api(url)
+        self.assertEqual(cm.exception.code, 404)
+        self.assertEqual(cm.exception.reason,
+                         b'Email is not banned: banned@example.com')
+
+    def test_not_found_after_unbanning_global(self):
+        manager = IBanManager(None)
+        with transaction():
+            manager.ban('banned@example.com')
+        url = ('http://localhost:9001/3.0/bans/banned@example.com')
+        response, content = call_api(url)
+        self.assertEqual(response['email'], 'banned@example.com')
+        response, content = call_api(url, method='DELETE')
+        self.assertEqual(content.status, 204)
+        with self.assertRaises(HTTPError) as cm:
+            call_api(url)
+        self.assertEqual(cm.exception.code, 404)
+        self.assertEqual(cm.exception.reason,
+                         b'Email is not banned: banned@example.com')