Merge lp:~jimmy-sigint/mailman/rest_api_ban_management into lp:mailman

[Jimmy Bergman]

This branch adds a REST API resource for managing per list bans, which is useful
in control panels targetting mailman 3.

See the src/mailman/rest/docs/membership.rst patch for usage.

[Barry Warsaw (barry)]

Nice branch, thanks Jimmy. A few comments.

Note that there is an IBanManager utility which you can use to add and remove list-specific or global bans. Probably better to use this utility in the REST API rather than adding a new interface to IMailingList.

Also (perhaps because you didn't know about the IBanManager), your branch doesn't provide a way to set or delete global bans.

What do you think about a resource tree that looks something like this:

GET http://.../bans -> a list of all global bans
DELETE http://.../bans -> deletes all global bans
POST <email address hidden> http://.../bans -> adds <email address hidden> to the global bans
DELETE http://.../<email address hidden> -> deletes <email address hidden> from the global bans

Then for list-specific bans, we would use

GET http://.../lists/list.example.com/bans -> a list of all list-specific bans
DELETE http://.../lists/list.example.com/bans -> delete all global bans
POST <email address hidden> http://.../lists/list.example.com/bans -> adds <email address hidden> to <email address hidden>'s bans
DELETE http://.../<email address hidden> -> deletes <email address hidden> from <email address hidden>'s bans

I don't particularly like DELETE on the /bans resource to delete all the sub-resources, since the implication is that this will remove /bans, not everything underneath it. But it's the best I can come up with for now. Obviously DELETE on /bans doesn't delete the /bans resource.

Would you like to take a shot at making these changes? If so, please push an update and respond to this comment and I'd be happy to take another look. I'd like to get this into 3.0b3.

Thanks!

review: Needs Fixing

[Jimmy Bergman (jimmy-sigint)]

Hi

> Note that there is an IBanManager utility which you can use to add and remove
> list-specific or global bans. Probably better to use this utility in the REST
> API rather than adding a new interface to IMailingList.

You must have misread the patch, I actually use the IBanManager. I extended
it to be able to return a list of banned emails for a list, since
the semantics wouldn't be RESTy if i couldn't list.

> Also (perhaps because you didn't know about the IBanManager), your branch
> doesn't provide a way to set or delete global bans.

It is more the fact that I created the first version of the patch before
the IBanManager came to life, because our control panel software mailman 3
integration has this view that displays per list bans. I could
add global bans as well since it makes sense.

> What do you think about a resource tree that looks something like this:
>
> GET http://.../bans -> a list of all global bans
> DELETE http://.../bans -> deletes all global bans

Is this really required? I guess it might be nice of course, but
it feels like an uncommon operation (and one that can be done
using the other DELETE although less efficiently).

> POST <email address hidden> http://.../bans -> adds <email address hidden> to the global
> bans
> DELETE http://.../<email address hidden> -> deletes <email address hidden> from the
> global bans

> Then for list-specific bans, we would use
>
> GET http://.../lists/list.example.com/bans -> a list of all list-specific bans
> DELETE http://.../lists/list.example.com/bans -> delete all global bans
> POST <email address hidden> http://.../lists/list.example.com/bans -> adds
> <email address hidden> to <email address hidden>'s bans
> DELETE http://.../<email address hidden> -> deletes
> <email address hidden> from <email address hidden>'s bans

Sounds great (although with the same question re DELETE of all bans) although
in the current patch I also have
GET http://.../<email address hidden> -> check if <email address hidden> is banned on list.example.com

which I think should stay (also i named it ../banlist, but /bans sounds better).

> Would you like to take a shot at making these changes? If so, please push an
> update and respond to this comment and I'd be happy to take another look. I'd
> like to get this into 3.0b3.

Sure, I can do the above change once we agree on:
1. Should there be a DELETE for removing all bans, or is it enough with DELETE of a single ban + iterating if you need to clear the ban list?

2. Should I keep my extra GET on the banned email sub-resource (obviously a similar one would also be implemented for the global bans)?

And what is the timeframe for 3.0b3 so that I know when I need to push it?

Best regards,
Jimmy

[Barry Warsaw (barry)]

On Sep 24, 2012, at 06:00 AM, Jimmy Bergman wrote:

>> Note that there is an IBanManager utility which you can use to add and
>> remove list-specific or global bans. Probably better to use this utility
>> in the REST API rather than adding a new interface to IMailingList.
>
>You must have misread the patch, I actually use the IBanManager. I extended
>it to be able to return a list of banned emails for a list, since
>the semantics wouldn't be RESTy if i couldn't list.

You're right, sorry about that.

BTW, I'm probably going to change the way IBanManager works, to be a bit more
like IAcceptableAliasSet. This would mean you'd adapt an IMailingList to an
IBanManager, and then we could drop the mailing_list parameters to the various
calls. The one difference is that we'd allow you to adapt None to IBanManager
to get the global bans.

Don't worry about all that though. Once your branch is ready, I'll make those
changes as I merge in your work.

>> Also (perhaps because you didn't know about the IBanManager), your branch
>> doesn't provide a way to set or delete global bans.
>
>It is more the fact that I created the first version of the patch before
>the IBanManager came to life, because our control panel software mailman 3
>integration has this view that displays per list bans. I could
>add global bans as well since it makes sense.

Cool.

>> What do you think about a resource tree that looks something like this:
>>
>> GET http://.../bans -> a list of all global bans
>> DELETE http://.../bans -> deletes all global bans
>
>Is this really required? I guess it might be nice of course, but
>it feels like an uncommon operation (and one that can be done
>using the other DELETE although less efficiently).

True. I'm also not really happy with DELETE on .../bans because it doesn't
actually delete the resource. Okay, so let's forget DELETE on all. Do we
need GET on all?

>> POST <email address hidden> http://.../bans -> adds <email address hidden> to the global
>> bans
>> DELETE http://.../<email address hidden> -> deletes <email address hidden> from the
>> global bans
>
>
>> Then for list-specific bans, we would use
>>
>> GET http://.../lists/list.example.com/bans -> a list of all list-specific bans
>> DELETE http://.../lists/list.example.com/bans -> delete all global bans
>> POST <email address hidden> http://.../lists/list.example.com/bans -> adds
>> <email address hidden> to <email address hidden>'s bans
>> DELETE http://.../<email address hidden> -> deletes
>> <email address hidden> from <email address hidden>'s bans
>
>Sounds great (although with the same question re DELETE of all bans)

I would definitely want it to be symmetrical. So if we want to have DELETE
all global bans, we'd want the same for list bans, and if not, then neither
ban list would support DELETE. Same with GET.

>although in the current patch I also have GET
>http://.../<email address hidden> -> check if
><email address hidden> is banned on list.example.com
>
>which I think should stay (also i named it ../banlist, but /bans sounds
>better).

Agreed, let's keep GET on a specific address. I suppose it would 404 if the
given address had no ban, and probably 204 if there is a ban. It would wo...

[Barry Warsaw (barry)]

On Oct 13, 2012, at 01:54 PM, Barry Warsaw wrote:

>BTW, I'm probably going to change the way IBanManager works, to be a bit more
>like IAcceptableAliasSet. This would mean you'd adapt an IMailingList to an
>IBanManager, and then we could drop the mailing_list parameters to the
>various calls. The one difference is that we'd allow you to adapt None to
>IBanManager to get the global bans.

I couldn't resist, so I made this change. I also realized that the ban table
was using mailing_list (i.e. the fqdn listname) as the cross-reference into
the mailinglist table. This is wrong because that can change. What can't
change though is the list-id, so I've added a schema migration to make that
change too (though it should all be hidden underneath the interface).

Still, you'll need to adjust your branch for the new API. If you can't do
that, please let me know, and I'll fix that when I merge your branch. (After
your response to my previous comments.)

[Jimmy Bergman (jimmy-sigint)]

Hi

On Wed, Oct 17, 2012 at 12:46 AM, Barry Warsaw <email address hidden> wrote:
> Still, you'll need to adjust your branch for the new API. If you can't do
> that, please let me know, and I'll fix that when I merge your branch. (After
> your response to my previous comments.)

I will do that at the same time as the other changes we discussed.

I'll hopefully be able to do the changes within a week or two.

Best regards,
Jimmy

[Barry Warsaw (barry)]

On Oct 17, 2012, at 08:25 AM, Jimmy Bergman wrote:

>I will do that at the same time as the other changes we discussed.
>
>I'll hopefully be able to do the changes within a week or two.

Sounds great, thanks.

[Barry Warsaw (barry)]

Hi Jimmy. I know it's been a while but do you have any interesting in updating this branch against the current git master branch, now hosted at gitlab?

https://gitlab.com/mailman/mailman

If so, please feel free to do a merge request over there and close this merge proposal. I think this would be useful to have access to the bans via REST.

review: Needs Information

[Barry Warsaw (barry)]

I created an issue over on gitlab: https://gitlab.com/mailman/mailman/issues/2

Unmerged revisions

7174. By Jimmy Bergman on 2012-09-21

Add support for handling per list bans in the REST API.

=== modified file 'src/mailman/interfaces/bans.py'
--- src/mailman/interfaces/bans.py	2012-01-01 19:14:46 +0000
+++ src/mailman/interfaces/bans.py	2012-09-21 09:06:20 +0000
@@ -108,3 +108,13 @@
             or not.
         :rtype: bool
         """
+
+    def ban_list(mailing_list):
+        """Retrieves a list of bans for a specific mailing list.
+
+        :param mailing_list: The fqdn name of the mailing list to retrieve
+            the ban list for.
+        :type mailing_list: string
+        :return: A list of banned emails.
+        :rtype: list
+        """
 
=== modified file 'src/mailman/model/bans.py'
--- src/mailman/model/bans.py	2012-04-26 02:08:22 +0000
+++ src/mailman/model/bans.py	2012-09-21 09:06:20 +0000
@@ -109,3 +109,8 @@
                     re.match(ban.email, email, re.IGNORECASE) is not None):
                     return True
         return False
+
+    @dbconnection
+    def ban_list(self, store, mailing_list):
+        """See `IBanManager`."""
+        return [ ban.email for ban in store.find(Ban, mailing_list=mailing_list) ]
 
=== modified file 'src/mailman/rest/docs/membership.rst'
--- src/mailman/rest/docs/membership.rst	2012-09-05 01:31:50 +0000
+++ src/mailman/rest/docs/membership.rst	2012-09-21 09:06:20 +0000
@@ -597,6 +597,51 @@
     >>> set(member.mailing_list for member in elly.memberships.members)
     set([])
 
+Handling the list of banned addresses
+=====================================
+
+To ban an address from subscribing you can POST to the /banlist child
+of any list using the REST API.
+::
+
+    # Ensure our previous reads don't keep the database lock.
+    >>> transaction.abort()
+    >>> dump_json('http://localhost:9001/3.0/lists/[e-mail]'
+    ...           '/banlist', { 'address': '[e-mail]' })
+    content-length: 0
+    ...
+    status: 201
+
+This address is now banned, and you can get the list of banned addresses using
+the REST API:
+
+    # Ensure our previous reads don't keep the database lock.
+    >>> transaction.abort()
+    >>> dump_json('http://localhost:9001/3.0/lists/[e-mail]/banlist')
+    entry 0:
+        banned_email: [e-mail]
+    ...
+
+Or checking if a single address is banned:
+
+    >>> dump_json('http://localhost:9001/3.0/lists/[e-mail]/banlist/[e-mail]')
+    banned_email: [e-mail]
+    http_etag: ...
+
+Unbanning addresses is also possible using the REST API.
+
+    >>> dump_json('http://localhost:9001/3.0/lists/[e-mail]/banlist'
+    ...           '/[e-mail]', method='DELETE')
+    content-length: 0
+    ...
+    status: 200
+
+After unbanning it shouldn't be available in the ban list:
+
+    >>> dump_json('http://localhost:9001/3.0/lists/[e-mail]/banlist/[e-mail]')
+    Traceback (most recent call last):
+    ...
+    HTTPError: HTTP Error 404: 404 Not Found
 
 Digest delivery
 ===============
 
=== modified file 'src/mailman/rest/lists.py'
--- src/mailman/rest/lists.py	2012-09-05 01:31:50 +0000
+++ src/mailman/rest/lists.py	2012-09-21 09:06:20 +0000
@@ -23,6 +23,8 @@
 __all__ = [
     'AList',
     'AllLists',
+    'BannedAddress',
+    'BannedAddresses',
     'ListConfiguration',
     'ListsForDomain',
     ]
@@ -33,6 +35,7 @@
 from zope.component import getUtility
 
 from mailman.app.lifecycle import create_list, remove_list
+from mailman.interfaces.bans import IBanManager
 from mailman.interfaces.domain import BadDomainSpecificationError
 from mailman.interfaces.listmanager import (
     IListManager, ListAlreadyExistsError)
@@ -82,6 +85,30 @@
 
 
 @restish_matcher
+def banlist_matcher(request, segments):
+    """A matcher of all banned addresses for a mailing list.
+
+    e.g. /banlist
+    """
+    if len(segments) != 1 or segments[0] != 'banlist':
+        return None
+    else:
+        return (), {}, ()
+
+
+@restish_matcher
+def banned_matcher(request, segments):
+    """A matcher of a single banned addresses for a mailing list.
+
+    e.g. /banlist/[e-mail]
+    """
+    if len(segments) != 2 or segments[0] != 'banlist':
+        return None
+    else:
+        return (), dict(address=segments[1]), ()
+
+
+@restish_matcher
 def config_matcher(request, segments):
     """A matcher for a mailing list's configuration resource.
 
@@ -173,6 +200,86 @@
             return http.not_found()
         return HeldMessages(self._mlist)
 
+    @resource.child(banlist_matcher)
+    def banlist(self, request, segments, attribute=None):
+        """Return a list of banned addresses."""
+        return BannedAddresses(self._mlist)
+
+    @resource.child(banned_matcher)
+    def banned(self, request, segments, address):
+        """Return a list of banned addresses."""
+        return BannedAddress(self._mlist, address)
+
+
+
 
+class BannedAddress(resource.Resource, CollectionMixin):
+    """Class to represent a banned address."""
+
+    def __init__(self, mlist, address):
+        self._mlist = mlist 
+        self._address = address
+        self.ban_manager = getUtility(IBanManager)
+
+    @resource.GET()
+    def container(self, request):
+        """/banlist/[e-mail]"""
+        if self._mlist is None or self._address is None or not self.ban_manager.is_banned(self._address, self._mlist.fqdn_listname):
+            return http.not_found()
+        resource = dict(banned_email=self._address)
+        return http.ok([], etag(resource))
+
+    @resource.DELETE()
+    def remove(self, request):
+        """Remove an address from the ban list."""
+        if self._mlist is None or self._address is None:
+            return http.not_found()
+        else:
+            if not self.ban_manager.is_banned(self._address, self._mlist.fqdn_listname):
+                return http.bad_request([], b'Address is not in ban list')
+
+        self.ban_manager.unban(self._address, self._mlist.fqdn_listname)
+        return http.ok([], '')
+
+class BannedAddresses(resource.Resource, CollectionMixin):
+    """Class to represent the list of all banned addresses."""
+
+    def __init__(self, mlist):
+        self._mlist = mlist 
+        self.ban_manager = getUtility(IBanManager)
+
+    def _resource_as_dict(self, item):
+        """See `CollectionMixin`."""
+        return dict(
+            banned_email=item,
+            )
+
+    def _get_collection(self, request):
+        """See `CollectionMixin`."""
+        return self.ban_manager.ban_list(self._mlist.fqdn_listname)
+
+    @resource.GET()
+    def container(self, request):
+        """/banlist"""
+        resource = self._make_collection(request)
+        return http.ok([], etag(resource))
+
+    @resource.POST()
+    def create(self, request):
+        """Ban some address from subscribing."""
+        try:
+            validator = Validator(address=unicode)
+            converted = validator(request)
+
+            address = converted['address']
+            if self.ban_manager.is_banned(address, self._mlist.fqdn_listname):
+                return http.bad_request([], b'Address is already in ban list')
+
+            self.ban_manager.ban(address, self._mlist.fqdn_listname)
+        except ValueError as error:
+            return http.bad_request([], str(error))
+        location = path_to('lists/{0}/banlist'.format(self._mlist.fqdn_listname))
+        return http.created(location, [], None)
+
 
 
 
 class AllLists(_ListBase):