Merge into 3.0 : rest_api_ban_management : Code : GNU Mailman

Status:	Work in progress
Proposed branch:	lp:~jimmy-sigint/mailman/rest_api_ban_management
Merge into:	lp:mailman
Diff against target:	225 lines (+167/-0) 4 files modified src/mailman/interfaces/bans.py (+10/-0) src/mailman/model/bans.py (+5/-0) src/mailman/rest/docs/membership.rst (+45/-0) src/mailman/rest/lists.py (+107/-0)
To merge this branch:	bzr merge lp:~jimmy-sigint/mailman/rest_api_ban_management
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Barry Warsaw		2012-09-21	Needs Information on 2015-05-08
Review via email: mp+125648@code.launchpad.net

Revision history for this message

Barry Warsaw (barry) wrote on 2012-09-22:

#

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

Reply

Revision history for this message

Jimmy Bergman (jimmy-sigint) wrote on 2012-09-24:

#

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

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 foo@example.com http://.../bans -> adds foo@example.com to the global
> bans
> DELETE http://.../bans/foo@example.com -> deletes foo@example.com 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 foo@example.com http://.../lists/list.example.com/bans -> adds
> foo@example.com to list@example.com's bans
> DELETE http://.../lists/list.example.com/bans/foo@example.com -> deletes
> foo@example.com from list@example.com's bans

Sounds great (although with the same question re DELETE of all bans) although
in the current patch I also have
GET http://.../lists/list.example.com/bans/foo@example.com -> check if foo@example.com 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

Reply

Revision history for this message

Barry Warsaw (barry) wrote on 2012-10-13:

#

Download full text (4.3 KiB)

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...

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 foo@example.com http://.../bans -> adds foo@example.com to the global
>> bans
>> DELETE http://.../bans/foo@example.com -> deletes foo@example.com 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 foo@example.com http://.../lists/list.example.com/bans -> adds
>> foo@example.com to list@example.com's bans
>> DELETE http://.../lists/list.example.com/bans/foo@example.com -> deletes
>> foo@example.com from list@example.com'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://.../lists/list.example.com/bans/foo@example.com -> check if
>foo@example.com 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 work
the same for global ban queries.

Now the question is, is it okay to do two queries to see if an address has
either a list-specific ban or a global ban?  That certainly would be the
easiest to implement (at the cost of some efficiency, which we already are
leaning toward not caring about for DELETE).

>> 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?

Let's support DELETE on a single ban only.

>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)?

+1

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

You have enough time.  Consensus seems to be to delay the release of the core
to allow for Postorius to get closer to complete.  I'm okay with that, with a
more liberal view of what's a bug in the core. :)

Go for it!

Reply

Revision history for this message

Barry Warsaw (barry) wrote on 2012-10-16:

#

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.)

Reply

Revision history for this message

Jimmy Bergman (jimmy-sigint) wrote on 2012-10-17:

#

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

Reply

Revision history for this message

Barry Warsaw (barry) wrote on 2012-10-17:

#

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.

Reply

Revision history for this message

Barry Warsaw (barry) wrote on 2015-05-08:

#

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

Reply

Revision history for this message

Barry Warsaw (barry) wrote on 2015-05-08:

#

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

Reply

GNU Mailman

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

Commit message

Description of the change

Unmerged revisions

Preview Diff

Subscribers

 === 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/bee@example.com'
++    ...           '/banlist', { 'address': 'someonebanned@example.com' })
++    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/bee@example.com/banlist')
++    entry 0:
++        banned_email: someonebanned@example.com
++    ...
++
++Or checking if a single address is banned:
++
++    >>> dump_json('http://localhost:9001/3.0/lists/bee@example.com/banlist/someonebanned@example.com')
++    banned_email: someonebanned@example.com
++    http_etag: ...
++
++Unbanning addresses is also possible using the REST API.
++
++    >>> dump_json('http://localhost:9001/3.0/lists/bee@example.com/banlist'
++    ...           '/someonebanned@example.com', 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/bee@example.com/banlist/someonebanned@example.com')
++    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/something@example.org
++    """
++    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/someone@example.com"""
++        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):