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

Proposed by Jimmy Bergman on 2012-09-21
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
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

Description of the change

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.

To post a comment you must log in.
Barry Warsaw (barry) wrote :

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) wrote :

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

Read more...

Barry Warsaw (barry) wrote :

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) wrote :

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) wrote :

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) wrote :

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) wrote :

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/mailman/interfaces/bans.py'
2--- src/mailman/interfaces/bans.py 2012-01-01 19:14:46 +0000
3+++ src/mailman/interfaces/bans.py 2012-09-21 09:06:20 +0000
4@@ -108,3 +108,13 @@
5 or not.
6 :rtype: bool
7 """
8+
9+ def ban_list(mailing_list):
10+ """Retrieves a list of bans for a specific mailing list.
11+
12+ :param mailing_list: The fqdn name of the mailing list to retrieve
13+ the ban list for.
14+ :type mailing_list: string
15+ :return: A list of banned emails.
16+ :rtype: list
17+ """
18
19=== modified file 'src/mailman/model/bans.py'
20--- src/mailman/model/bans.py 2012-04-26 02:08:22 +0000
21+++ src/mailman/model/bans.py 2012-09-21 09:06:20 +0000
22@@ -109,3 +109,8 @@
23 re.match(ban.email, email, re.IGNORECASE) is not None):
24 return True
25 return False
26+
27+ @dbconnection
28+ def ban_list(self, store, mailing_list):
29+ """See `IBanManager`."""
30+ return [ ban.email for ban in store.find(Ban, mailing_list=mailing_list) ]
31
32=== modified file 'src/mailman/rest/docs/membership.rst'
33--- src/mailman/rest/docs/membership.rst 2012-09-05 01:31:50 +0000
34+++ src/mailman/rest/docs/membership.rst 2012-09-21 09:06:20 +0000
35@@ -597,6 +597,51 @@
36 >>> set(member.mailing_list for member in elly.memberships.members)
37 set([])
38
39+Handling the list of banned addresses
40+=====================================
41+
42+To ban an address from subscribing you can POST to the /banlist child
43+of any list using the REST API.
44+::
45+
46+ # Ensure our previous reads don't keep the database lock.
47+ >>> transaction.abort()
48+ >>> dump_json('http://localhost:9001/3.0/lists/bee@example.com'
49+ ... '/banlist', { 'address': 'someonebanned@example.com' })
50+ content-length: 0
51+ ...
52+ status: 201
53+
54+This address is now banned, and you can get the list of banned addresses using
55+the REST API:
56+
57+ # Ensure our previous reads don't keep the database lock.
58+ >>> transaction.abort()
59+ >>> dump_json('http://localhost:9001/3.0/lists/bee@example.com/banlist')
60+ entry 0:
61+ banned_email: someonebanned@example.com
62+ ...
63+
64+Or checking if a single address is banned:
65+
66+ >>> dump_json('http://localhost:9001/3.0/lists/bee@example.com/banlist/someonebanned@example.com')
67+ banned_email: someonebanned@example.com
68+ http_etag: ...
69+
70+Unbanning addresses is also possible using the REST API.
71+
72+ >>> dump_json('http://localhost:9001/3.0/lists/bee@example.com/banlist'
73+ ... '/someonebanned@example.com', method='DELETE')
74+ content-length: 0
75+ ...
76+ status: 200
77+
78+After unbanning it shouldn't be available in the ban list:
79+
80+ >>> dump_json('http://localhost:9001/3.0/lists/bee@example.com/banlist/someonebanned@example.com')
81+ Traceback (most recent call last):
82+ ...
83+ HTTPError: HTTP Error 404: 404 Not Found
84
85 Digest delivery
86 ===============
87
88=== modified file 'src/mailman/rest/lists.py'
89--- src/mailman/rest/lists.py 2012-09-05 01:31:50 +0000
90+++ src/mailman/rest/lists.py 2012-09-21 09:06:20 +0000
91@@ -23,6 +23,8 @@
92 __all__ = [
93 'AList',
94 'AllLists',
95+ 'BannedAddress',
96+ 'BannedAddresses',
97 'ListConfiguration',
98 'ListsForDomain',
99 ]
100@@ -33,6 +35,7 @@
101 from zope.component import getUtility
102
103 from mailman.app.lifecycle import create_list, remove_list
104+from mailman.interfaces.bans import IBanManager
105 from mailman.interfaces.domain import BadDomainSpecificationError
106 from mailman.interfaces.listmanager import (
107 IListManager, ListAlreadyExistsError)
108@@ -82,6 +85,30 @@
109
110
111 @restish_matcher
112+def banlist_matcher(request, segments):
113+ """A matcher of all banned addresses for a mailing list.
114+
115+ e.g. /banlist
116+ """
117+ if len(segments) != 1 or segments[0] != 'banlist':
118+ return None
119+ else:
120+ return (), {}, ()
121+
122+
123+@restish_matcher
124+def banned_matcher(request, segments):
125+ """A matcher of a single banned addresses for a mailing list.
126+
127+ e.g. /banlist/something@example.org
128+ """
129+ if len(segments) != 2 or segments[0] != 'banlist':
130+ return None
131+ else:
132+ return (), dict(address=segments[1]), ()
133+
134+
135+@restish_matcher
136 def config_matcher(request, segments):
137 """A matcher for a mailing list's configuration resource.
138
139@@ -173,6 +200,86 @@
140 return http.not_found()
141 return HeldMessages(self._mlist)
142
143+ @resource.child(banlist_matcher)
144+ def banlist(self, request, segments, attribute=None):
145+ """Return a list of banned addresses."""
146+ return BannedAddresses(self._mlist)
147+
148+ @resource.child(banned_matcher)
149+ def banned(self, request, segments, address):
150+ """Return a list of banned addresses."""
151+ return BannedAddress(self._mlist, address)
152+
153+
154+
155
156+class BannedAddress(resource.Resource, CollectionMixin):
157+ """Class to represent a banned address."""
158+
159+ def __init__(self, mlist, address):
160+ self._mlist = mlist
161+ self._address = address
162+ self.ban_manager = getUtility(IBanManager)
163+
164+ @resource.GET()
165+ def container(self, request):
166+ """/banlist/someone@example.com"""
167+ if self._mlist is None or self._address is None or not self.ban_manager.is_banned(self._address, self._mlist.fqdn_listname):
168+ return http.not_found()
169+ resource = dict(banned_email=self._address)
170+ return http.ok([], etag(resource))
171+
172+ @resource.DELETE()
173+ def remove(self, request):
174+ """Remove an address from the ban list."""
175+ if self._mlist is None or self._address is None:
176+ return http.not_found()
177+ else:
178+ if not self.ban_manager.is_banned(self._address, self._mlist.fqdn_listname):
179+ return http.bad_request([], b'Address is not in ban list')
180+
181+ self.ban_manager.unban(self._address, self._mlist.fqdn_listname)
182+ return http.ok([], '')
183+
184+class BannedAddresses(resource.Resource, CollectionMixin):
185+ """Class to represent the list of all banned addresses."""
186+
187+ def __init__(self, mlist):
188+ self._mlist = mlist
189+ self.ban_manager = getUtility(IBanManager)
190+
191+ def _resource_as_dict(self, item):
192+ """See `CollectionMixin`."""
193+ return dict(
194+ banned_email=item,
195+ )
196+
197+ def _get_collection(self, request):
198+ """See `CollectionMixin`."""
199+ return self.ban_manager.ban_list(self._mlist.fqdn_listname)
200+
201+ @resource.GET()
202+ def container(self, request):
203+ """/banlist"""
204+ resource = self._make_collection(request)
205+ return http.ok([], etag(resource))
206+
207+ @resource.POST()
208+ def create(self, request):
209+ """Ban some address from subscribing."""
210+ try:
211+ validator = Validator(address=unicode)
212+ converted = validator(request)
213+
214+ address = converted['address']
215+ if self.ban_manager.is_banned(address, self._mlist.fqdn_listname):
216+ return http.bad_request([], b'Address is already in ban list')
217+
218+ self.ban_manager.ban(address, self._mlist.fqdn_listname)
219+ except ValueError as error:
220+ return http.bad_request([], str(error))
221+ location = path_to('lists/{0}/banlist'.format(self._mlist.fqdn_listname))
222+ return http.created(location, [], None)
223+
224
225
226
227 class AllLists(_ListBase):