Barry Warsaw pushed to branch master at mailman / Mailman
Commits: dac92997 by Barry Warsaw at 2015-12-30T14:38:10Z Major push for completing issue #121. Now in API 3.1, all UUIDs must be the hex representations of a UUID, not the int representation. Also: * Some general code cleanup. * Fix issue 185 (REST server crash when subscribing a user without a preferred address). - - - - - 35577e20 by Barry Warsaw at 2015-12-31T11:44:27Z Cleanups, corner case fixes, and coverage. - - - - - 18 changed files: - src/mailman/docs/NEWS.rst - src/mailman/rest/addresses.py - src/mailman/rest/docs/addresses.rst - src/mailman/rest/domains.py - src/mailman/rest/helpers.py - src/mailman/rest/lists.py - src/mailman/rest/members.py - src/mailman/rest/preferences.py - src/mailman/rest/root.py - src/mailman/rest/tests/test_addresses.py - src/mailman/rest/tests/test_domains.py - src/mailman/rest/tests/test_lists.py - src/mailman/rest/tests/test_membership.py - src/mailman/rest/tests/test_users.py - src/mailman/rest/tests/test_validator.py - src/mailman/rest/users.py - src/mailman/rest/validator.py - src/mailman/testing/documentation.py Changes: ===================================== src/mailman/docs/NEWS.rst ===================================== --- a/src/mailman/docs/NEWS.rst +++ b/src/mailman/docs/NEWS.rst @@ -54,6 +54,8 @@ Bugs store. Given by Aurélien Bompard, tweaked by Barry Warsaw. (Closes: #167) * Fix a bug in ``SubscriptionService.find_members()`` when searching for a subscribed address that is not linked to a user. Given by Aurélien Bompard. + * Fix a REST server crash when trying to subscribe a user without a preferred + address. (Closes #185) Configuration ------------- @@ -101,7 +103,7 @@ REST * REST API version 3.1 introduced. Mostly backward compatible with version 3.0 except that UUIDs are represented as hex strings instead of 128-bit integers, since the latter are not compatible with all versions of - JavaScript. + JavaScript. (Closes #121) * When creating a user via REST using an address that already exists, but isn't linked, the address is linked to the new user. Given by Aurélien Bompard. ===================================== src/mailman/rest/addresses.py ===================================== --- a/src/mailman/rest/addresses.py +++ b/src/mailman/rest/addresses.py @@ -51,7 +51,7 @@ class _AddressBase(CollectionMixin): email=address.email, original_email=address.original_email, registered_on=address.registered_on, - self_link=self.path_to('addresses/{0}'.format(address.email)), + self_link=self.path_to('addresses/{}'.format(address.email)), ) # Add optional attributes. These can be None or the empty string. if address.display_name: @@ -59,8 +59,9 @@ class _AddressBase(CollectionMixin): if address.verified_on: representation['verified_on'] = address.verified_on if address.user: - representation['user'] = self.path_to( - 'users/{0}'.format(address.user.user_id.int)) + uid = getattr(address.user.user_id, + 'int' if self.api_version == '3.0' else 'hex') + representation['user'] = self.path_to('users/{}'.format(uid)) return representation def _get_collection(self, request): @@ -125,7 +126,7 @@ class AnAddress(_AddressBase): def memberships(self, request, segments): """/addresses/<email>/memberships""" if len(segments) != 0: - return BadRequest(), [] + return NotFound(), [] if self._address is None: return NotFound(), [] return AddressMemberships(self._address) @@ -139,7 +140,7 @@ class AnAddress(_AddressBase): return NotFound(), [] child = Preferences( self._address.preferences, - 'addresses/{0}'.format(self._address.email)) + 'addresses/{}'.format(self._address.email)) return child, [] @child() @@ -177,8 +178,8 @@ class UserAddresses(_AddressBase): """The addresses of a user.""" def __init__(self, user): - self._user = user super().__init__() + self._user = user def _get_collection(self, request): """See `CollectionMixin`.""" @@ -187,19 +188,15 @@ class UserAddresses(_AddressBase): def on_get(self, request, response): """/addresses""" - if self._user is None: - not_found(response) - else: - okay(response, etag(self._make_collection(request))) + assert self._user is not None + okay(response, etag(self._make_collection(request))) def on_post(self, request, response): """POST to /addresses Add a new address to the user record. """ - if self._user is None: - not_found(response) - return + assert self._user is not None user_manager = getUtility(IUserManager) validator = Validator(email=str, display_name=str, ===================================== src/mailman/rest/docs/addresses.rst ===================================== --- a/src/mailman/rest/docs/addresses.rst +++ b/src/mailman/rest/docs/addresses.rst @@ -175,10 +175,10 @@ A link to the user resource is now available as a sub-resource. user: http://localhost:9001/3.0/users/1 To prevent automatic user creation from taking place, add the `auto_create` -parameter to the POST request and set it to a false-equivalent value like 0: +parameter to the POST request and set it to False. >>> dump_json('http://localhost:9001/3.0/addresses/a...@example.com/user', - ... {'display_name': 'Anne User', 'auto_create': 0}) + ... {'display_name': 'Anne User', 'auto_create': False}) Traceback (most recent call last): ... urllib.error.HTTPError: HTTP Error 403: ... ===================================== src/mailman/rest/domains.py ===================================== --- a/src/mailman/rest/domains.py +++ b/src/mailman/rest/domains.py @@ -44,7 +44,7 @@ class _DomainBase(CollectionMixin): base_url=domain.base_url, description=domain.description, mail_host=domain.mail_host, - self_link=self.path_to('domains/{0}'.format(domain.mail_host)), + self_link=self.path_to('domains/{}'.format(domain.mail_host)), url_host=domain.url_host, ) @@ -97,7 +97,7 @@ class ADomain(_DomainBase): return NotFound() return OwnersForDomain(domain) else: - return BadRequest(), [] + return NotFound(), [] class AllDomains(_DomainBase): @@ -122,10 +122,8 @@ class AllDomains(_DomainBase): domain = domain_manager.add(**values) except BadDomainSpecificationError as error: bad_request(response, str(error)) - except ValueError as error: - bad_request(response, str(error)) else: - location = self.path_to('domains/{0}'.format(domain.mail_host)) + location = self.path_to('domains/{}'.format(domain.mail_host)) created(response, location) def on_get(self, request, response): ===================================== src/mailman/rest/helpers.py ===================================== --- a/src/mailman/rest/helpers.py +++ b/src/mailman/rest/helpers.py @@ -111,7 +111,7 @@ def etag(resource): # library requires a bytes. Use the safest possible encoding. hashfood = pformat(resource).encode('raw-unicode-escape') etag = hashlib.sha1(hashfood).hexdigest() - resource['http_etag'] = '"{0}"'.format(etag) + resource['http_etag'] = '"{}"'.format(etag) return json.dumps(resource, cls=ExtendedEncoder, sort_keys=as_boolean(config.devmode.enabled)) @@ -130,7 +130,7 @@ class CollectionMixin: :return: The representation of the resource. :rtype: dict """ - raise NotImplementedError + raise NotImplementedError # pragma: no cover def _resource_as_json(self, resource): """Return the JSON formatted representation of the resource.""" @@ -147,7 +147,7 @@ class CollectionMixin: :return: The collection :rtype: list """ - raise NotImplementedError + raise NotImplementedError # pragma: no cover def _paginate(self, request, collection): """Method to paginate through collection result lists. ===================================== src/mailman/rest/lists.py ===================================== --- a/src/mailman/rest/lists.py +++ b/src/mailman/rest/lists.py @@ -111,7 +111,7 @@ class _ListBase(CollectionMixin): mail_host=mlist.mail_host, member_count=mlist.members.member_count, volume=mlist.volume, - self_link=self.path_to('lists/{0}'.format(mlist.list_id)), + self_link=self.path_to('lists/{}'.format(mlist.list_id)), ) def _get_collection(self, request): @@ -157,7 +157,7 @@ class AList(_ListBase): if len(members) == 0: return NotFound(), [] assert len(members) == 1, 'Too many matches' - return AMember(members[0].member_id) + return AMember(request.context['api_version'], members[0].member_id) @child(roster_matcher) def roster(self, request, segments, role): @@ -217,8 +217,6 @@ class AllLists(_ListBase): except BadDomainSpecificationError as error: reason = 'Domain does not exist: {}'.format(error.domain) bad_request(response, reason.encode('utf-8')) - except ValueError as error: - bad_request(response, str(error)) else: location = self.path_to('lists/{0}'.format(mlist.list_id)) created(response, location) @@ -234,7 +232,7 @@ class MembersOfList(MemberCollection): """The members of a mailing list.""" def __init__(self, mailing_list, role): - super(MembersOfList, self).__init__() + super().__init__() self._mlist = mailing_list self._role = role @@ -268,7 +266,7 @@ class ArchiverGetterSetter(GetterSetter): """Resource for updating archiver statuses.""" def __init__(self, mlist): - super(ArchiverGetterSetter, self).__init__() + super().__init__() self._archiver_set = IListArchiverSet(mlist) def put(self, mlist, attribute, value): ===================================== src/mailman/rest/members.py ===================================== --- a/src/mailman/rest/members.py +++ b/src/mailman/rest/members.py @@ -31,7 +31,7 @@ from mailman.interfaces.address import IAddress, InvalidEmailAddressError from mailman.interfaces.listmanager import IListManager from mailman.interfaces.member import ( AlreadySubscribedError, DeliveryMode, MemberRole, MembershipError, - MembershipIsBannedError, NotAMemberError) + MembershipIsBannedError, MissingPreferredAddressError, NotAMemberError) from mailman.interfaces.registrar import IRegistrar from mailman.interfaces.subscriptions import ( ISubscriptionService, RequestRecord, TokenOwner) @@ -52,16 +52,21 @@ from zope.component import getUtility class _MemberBase(CollectionMixin): """Shared base class for member representations.""" + def _get_uuid(self, member): + return getattr(member.member_id, + 'int' if self.api_version == '3.0' else 'hex') + def _resource_as_dict(self, member): """See `CollectionMixin`.""" enum, dot, role = str(member.role).partition('.') # The member will always have a member id and an address id. It will # only have a user id if the address is linked to a user. # E.g. nonmembers we've only seen via postings to lists they are not - # subscribed to will not have a user id. The user_id and the - # member_id are UUIDs. We need to use the integer equivalent in the - # URL. - member_id = member.member_id.int + # subscribed to will not have a user id. The user_id and the + # member_id are UUIDs. In API 3.0 we use the integer equivalent of + # the UID in the URL, but in API 3.1 we use the hex equivalent. See + # issue #121 for details. + member_id = self._get_uuid(member) response = dict( address=self.path_to('addresses/{}'.format(member.address.email)), delivery_mode=member.delivery_mode, @@ -75,8 +80,9 @@ class _MemberBase(CollectionMixin): # Add the user link if there is one. user = member.user if user is not None: - response['user'] = self.path_to( - 'users/{}'.format(user.user_id.int)) + user_id = getattr(user.user_id, + 'int' if self.api_version == '3.0' else 'hex') + response['user'] = self.path_to('users/{}'.format(user_id)) return response def _get_collection(self, request): @@ -94,7 +100,7 @@ class MemberCollection(_MemberBase): """ def _get_collection(self, request): """See `CollectionMixin`.""" - raise NotImplementedError + raise NotImplementedError # pragma: no cover def on_get(self, request, response): """roster/[members|owners|moderators]""" @@ -106,13 +112,18 @@ class MemberCollection(_MemberBase): class AMember(_MemberBase): """A member.""" - def __init__(self, member_id_string): - # REST gives us the member id as the string of an int; we have to - # convert it to a UUID. + def __init__(self, api_version, member_id_string): + # The member_id_string is the string representation of the member's + # UUID. In API 3.0, the argument is the string representation of the + # int representation of the UUID. In API 3.1 it's the hex. + self.api_version = api_version try: - member_id = UUID(int=int(member_id_string)) + if api_version == '3.0': + member_id = UUID(int=int(member_id_string)) + else: + member_id = UUID(hex=member_id_string) except ValueError: - # The string argument could not be converted to an integer. + # The string argument could not be converted to a UUID. self._member = None else: service = getUtility(ISubscriptionService) @@ -132,9 +143,9 @@ class AMember(_MemberBase): return NotFound(), [] if self._member is None: return NotFound(), [] + member_id = self._get_uuid(self._member) child = Preferences( - self._member.preferences, - 'members/{0}'.format(self._member.member_id.int)) + self._member.preferences, 'members/{}'.format(member_id)) return child, [] @child() @@ -146,7 +157,7 @@ class AMember(_MemberBase): return NotFound(), [] child = ReadOnlyPreferences( self._member, - 'members/{0}/all'.format(self._member.member_id.int)) + 'members/{}/all'.format(self._get_uuid(self._member))) return child, [] def on_delete(self, request, response): @@ -159,11 +170,7 @@ class AMember(_MemberBase): return mlist = getUtility(IListManager).get_by_list_id(self._member.list_id) if self._member.role is MemberRole.member: - try: - delete_member(mlist, self._member.address.email, False, False) - except NotAMemberError: - not_found(response) - return + delete_member(mlist, self._member.address.email, False, False) else: self._member.unsubscribe() no_content(response) @@ -213,7 +220,7 @@ class AllMembers(_MemberBase): try: validator = Validator( list_id=str, - subscriber=subscriber_validator, + subscriber=subscriber_validator(self.api_version), display_name=str, delivery_mode=enum_validator(DeliveryMode), role=enum_validator(MemberRole), @@ -276,14 +283,17 @@ class AllMembers(_MemberBase): except AlreadySubscribedError: conflict(response, b'Member already subscribed') return + except MissingPreferredAddressError: + bad_request(response, b'User has no preferred address') + return if token is None: assert token_owner is TokenOwner.no_one, token_owner # The subscription completed. Let's get the resulting member # and return the location to the new member. Member ids are # UUIDs and need to be converted to URLs because JSON doesn't # directly support UUIDs. - member_id = member.member_id.int - location = self.path_to('members/{0}'.format(member_id)) + member_id = self._get_uuid(member) + location = self.path_to('members/{}'.format(member_id)) created(response, location) return # The member could not be directly subscribed because there are @@ -332,8 +342,8 @@ class AllMembers(_MemberBase): # and return the location to the new member. Member ids are # UUIDs and need to be converted to URLs because JSON doesn't # directly support UUIDs. - member_id = member.member_id.int - location = self.path_to('members/{0}'.format(member_id)) + member_id = self._get_uuid(member) + location = self.path_to('members/{}'.format(member_id)) created(response, location) def on_get(self, request, response): ===================================== src/mailman/rest/preferences.py ===================================== --- a/src/mailman/rest/preferences.py +++ b/src/mailman/rest/preferences.py @@ -65,7 +65,7 @@ class ReadOnlyPreferences: resource['preferred_language'] = preferred_language.code # Add the self link. resource['self_link'] = path_to( - '{0}/preferences'.format(self._base_url), + '{}/preferences'.format(self._base_url), self.api_version) okay(response, etag(resource)) ===================================== src/mailman/rest/root.py ===================================== --- a/src/mailman/rest/root.py +++ b/src/mailman/rest/root.py @@ -178,10 +178,14 @@ class TopLevel: /<api>/addresses/<email> """ if len(segments) == 0: - return AllAddresses() + resource = AllAddresses() + resource.api_version = request.context['api_version'] + return resource else: email = segments.pop(0) - return AnAddress(email), segments + resource = AnAddress(email) + resource.api_version = request.context['api_version'] + return resource, segments @child() def domains(self, request, segments): @@ -214,24 +218,32 @@ class TopLevel: @child() def members(self, request, segments): """/<api>/members""" + api_version = request.context['api_version'] if len(segments) == 0: - return AllMembers() + resource = AllMembers() + resource.api_version = api_version + return resource # Either the next segment is the string "find" or a member id. They # cannot collide. segment = segments.pop(0) if segment == 'find': - return FindMembers(), segments + resource = FindMembers() + resource.api_version = api_version else: - return AMember(segment), segments + resource = AMember(api_version, segment) + return resource, segments @child() def users(self, request, segments): """/<api>/users""" + api_version = request.context['api_version'] if len(segments) == 0: - return AllUsers() + resource = AllUsers() + resource.api_version = api_version + return resource else: user_id = segments.pop(0) - return AUser(user_id), segments + return AUser(api_version, user_id), segments @child() def owners(self, request, segments): @@ -239,7 +251,9 @@ class TopLevel: if len(segments) != 0: return BadRequest(), [] else: - return ServerOwners(), segments + resource = ServerOwners() + resource.api_version = request.context['api_version'] + return resource, segments @child() def templates(self, request, segments): ===================================== src/mailman/rest/tests/test_addresses.py ===================================== --- a/src/mailman/rest/tests/test_addresses.py +++ b/src/mailman/rest/tests/test_addresses.py @@ -18,6 +18,7 @@ """REST address tests.""" __all__ = [ + 'TestAPI31Addresses', 'TestAddresses', ] @@ -27,7 +28,7 @@ import unittest from mailman.app.lifecycle import create_list from mailman.database.transaction import transaction from mailman.interfaces.usermanager import IUserManager -from mailman.testing.helpers import call_api +from mailman.testing.helpers import call_api, subscribe from mailman.testing.layers import RESTLayer from mailman.utilities.datetime import now from urllib.error import HTTPError @@ -62,6 +63,13 @@ class TestAddresses(unittest.TestCase): 'nob...@example.com/memberships') self.assertEqual(cm.exception.code, 404) + def test_membership_of_address_with_no_user(self): + with transaction(): + getUtility(IUserManager).create_address('a...@example.com') + response, content = call_api( + 'http://localhost:9001/3.0/addresses/a...@example.com/memberships') + self.assertEqual(response['total_size'], 0) + def test_verify_a_missing_address(self): # POSTing to the 'verify' sub-resource returns a 404. with self.assertRaises(HTTPError) as cm: @@ -432,3 +440,141 @@ class TestAddresses(unittest.TestCase): 'http://localhost:9001/3.0/addresses/a...@example.com', method='DELETE') self.assertEqual(cm.exception.code, 404) + + def test_bad_memberships_url(self): + with transaction(): + subscribe(self._mlist, 'Anne') + with self.assertRaises(HTTPError) as cm: + call_api('http://localhost:9001/3.0/addresses/' + 'aper...@example.com/memberships/bogus') + self.assertEqual(cm.exception.code, 404) + + def test_bad_preferences_url(self): + with transaction(): + subscribe(self._mlist, 'Anne') + with self.assertRaises(HTTPError) as cm: + call_api('http://localhost:9001/3.0/addresses/' + 'aper...@example.com/preferences/bogus') + self.assertEqual(cm.exception.code, 404) + + def test_bad_preferences_address(self): + with self.assertRaises(HTTPError) as cm: + call_api('http://localhost:9001/3.0/addresses/' + 'nob...@example.com/preferences') + self.assertEqual(cm.exception.code, 404) + + def test_bad_user_address(self): + with self.assertRaises(HTTPError) as cm: + call_api('http://localhost:9001/3.0/addresses/' + 'nob...@example.com/user') + self.assertEqual(cm.exception.code, 404) + + def test_bad_user_addresses_url(self): + with self.assertRaises(HTTPError) as cm: + call_api('http://localhost:9001/3.0/users/' + 'nob...@example.com/addresses') + self.assertEqual(cm.exception.code, 404) + + + +class TestAPI31Addresses(unittest.TestCase): + """UUIDs are represented as hex instead of int in API 3.1 + + See issue #121 for details. This is a deliberately backward + incompatible change. + """ + + layer = RESTLayer + + def test_address_user_id_is_hex(self): + user_manager = getUtility(IUserManager) + with transaction(): + user_manager.create_user('a...@example.com', 'Anne') + response, headers = call_api( + 'http://localhost:9001/3.1/addresses/a...@example.com') + self.assertEqual( + response['user'], + 'http://localhost:9001/3.1/users/00000000000000000000000000000001') + + def test_addresses_user_ids_are_hex(self): + user_manager = getUtility(IUserManager) + with transaction(): + user_manager.create_user('a...@example.com', 'Anne') + user_manager.create_user('b...@example.com', 'Bart') + response, headers = call_api('http://localhost:9001/3.1/addresses') + entries = response['entries'] + self.assertEqual( + entries[0]['user'], + 'http://localhost:9001/3.1/users/00000000000000000000000000000001') + self.assertEqual( + entries[1]['user'], + 'http://localhost:9001/3.1/users/00000000000000000000000000000002') + + def test_user_subresource_post(self): + # If the address is not yet linked to a user, POSTing a user id to the + # 'user' subresource links the address to the given user. In + # API 3.0, the user id must be the hex representation. + user_manager = getUtility(IUserManager) + with transaction(): + anne = user_manager.create_user('anne.per...@example.org', 'Anne') + anne_addr = user_manager.create_address('a...@example.com') + response, headers = call_api( + 'http://localhost:9001/3.1/addresses/a...@example.com/user', { + 'user_id': anne.user_id.hex, + }) + self.assertEqual(headers['status'], '200') + self.assertEqual(anne_addr.user, anne) + self.assertEqual(sorted([a.email for a in anne.addresses]), + ['anne.per...@example.org', 'a...@example.com']) + + def test_user_subresource_cannot_post_int(self): + # If the address is not yet linked to a user, POSTing a user id to the + # 'user' subresource links the address to the given user. In + # API 3.1, the user id must be the hex representation. + user_manager = getUtility(IUserManager) + with transaction(): + anne = user_manager.create_user('anne.per...@example.org', 'Anne') + user_manager.create_address('a...@example.com') + with self.assertRaises(HTTPError) as cm: + call_api( + 'http://localhost:9001/3.1/addresses/a...@example.com/user', { + 'user_id': anne.user_id.int, + }) + self.assertEqual(cm.exception.code, 400) + self.assertEqual(cm.exception.reason, + b'badly formed hexadecimal UUID string') + + def test_user_subresource_put(self): + # By PUTing to the 'user' resource, you can change the user that an + # address is linked to. + user_manager = getUtility(IUserManager) + with transaction(): + anne = user_manager.create_user('a...@example.com', 'Anne') + bart = user_manager.create_user(display_name='Bart') + response, headers = call_api( + 'http://localhost:9001/3.1/addresses/a...@example.com/user', { + 'user_id': bart.user_id.hex, + }, method='PUT') + self.assertEqual(headers['status'], '200') + self.assertEqual(anne.addresses, []) + self.assertEqual([address.email for address in bart.addresses], + ['a...@example.com']) + self.assertEqual(bart, + user_manager.get_address('a...@example.com').user) + + def test_user_subresource_cannot_put_int(self): + # If the address is not yet linked to a user, POSTing a user id to the + # 'user' subresource links the address to the given user. In + # API 3.1, the user id must be the hex representation. + user_manager = getUtility(IUserManager) + with transaction(): + anne = user_manager.create_user('anne.per...@example.org', 'Anne') + user_manager.create_address('a...@example.com') + with self.assertRaises(HTTPError) as cm: + call_api( + 'http://localhost:9001/3.1/addresses/a...@example.com/user', { + 'user_id': anne.user_id.int, + }, method='PUT') + self.assertEqual(cm.exception.code, 400) + self.assertEqual(cm.exception.reason, + b'badly formed hexadecimal UUID string') ===================================== src/mailman/rest/tests/test_domains.py ===================================== --- a/src/mailman/rest/tests/test_domains.py +++ b/src/mailman/rest/tests/test_domains.py @@ -106,6 +106,13 @@ class TestDomains(unittest.TestCase): 'http://localhost:9001/3.0/domains/does-not-exist.com/lists') self.assertEqual(cm.exception.code, 404) + def test_create_existing_domain(self): + with self.assertRaises(HTTPError) as cm: + call_api('http://localhost:9001/3.0/domains', dict( + mail_host='example.com', + )) + self.assertEqual(cm.exception.code, 400) + def test_double_delete(self): # You cannot delete a domain twice. content, response = call_api( @@ -118,7 +125,6 @@ class TestDomains(unittest.TestCase): self.assertEqual(cm.exception.code, 404) - class TestDomainOwners(unittest.TestCase): layer = RESTLayer @@ -129,6 +135,12 @@ class TestDomainOwners(unittest.TestCase): call_api('http://localhost:9001/3.0/domains/example.net/owners') self.assertEqual(cm.exception.code, 404) + def test_bad_domain_owners_url(self): + with self.assertRaises(HTTPError) as cm: + call_api( + 'http://localhost:9001/3.0/domains/example.com/owners/bogus') + self.assertEqual(cm.exception.code, 404) + def test_post_to_missing_domain_owners(self): # Try to add owners to a missing domain. with self.assertRaises(HTTPError) as cm: ===================================== src/mailman/rest/tests/test_lists.py ===================================== --- a/src/mailman/rest/tests/test_lists.py +++ b/src/mailman/rest/tests/test_lists.py @@ -218,6 +218,12 @@ class TestLists(unittest.TestCase): call_api('http://localhost:9001/3.0/lists/bogus.example.com') self.assertEqual(cm.exception.code, 404) + def test_not_found_member_role(self): + with self.assertRaises(HTTPError) as cm: + call_api('http://localhost:9001/3.0/lists/test.example.com' + '/owner/nob...@example.com') + self.assertEqual(cm.exception.code, 404) + class TestListArchivers(unittest.TestCase): ===================================== src/mailman/rest/tests/test_membership.py ===================================== --- a/src/mailman/rest/tests/test_membership.py +++ b/src/mailman/rest/tests/test_membership.py @@ -18,6 +18,7 @@ """REST membership tests.""" __all__ = [ + 'TestAPI31Members', 'TestMembership', 'TestNonmembership', ] @@ -28,10 +29,11 @@ import unittest from mailman.app.lifecycle import create_list from mailman.config import config from mailman.database.transaction import transaction +from mailman.interfaces.member import DeliveryMode, MemberRole from mailman.interfaces.usermanager import IUserManager from mailman.testing.helpers import ( TestableMaster, call_api, get_lmtp_client, make_testable_runner, - wait_for_webservice) + subscribe, wait_for_webservice) from mailman.runners.incoming import IncomingRunner from mailman.testing.layers import ConfigLayer, RESTLayer from mailman.utilities.datetime import now @@ -107,6 +109,21 @@ class TestMembership(unittest.TestCase): self.assertEqual(cm.exception.code, 409) self.assertEqual(cm.exception.reason, b'Member already subscribed') + def test_subscribe_user_without_preferred_address(self): + with transaction(): + getUtility(IUserManager).create_user('a...@example.com') + # Subscribe the user to the mailing list by hex UUID. + with self.assertRaises(HTTPError) as cm: + call_api('http://localhost:9001/3.1/members', { + 'list_id': 'test.example.com', + 'subscriber': '00000000000000000000000000000001', + 'pre_verified': True, + 'pre_confirmed': True, + 'pre_approved': True, + }) + self.assertEqual(cm.exception.code, 400) + self.assertEqual(cm.exception.reason, b'User has no preferred address') + def test_add_member_with_mixed_case_email(self): # LP: #1425359 - Mailman is case-perserving, case-insensitive. This # test subscribes the lower case address and ensures the original mixed @@ -266,6 +283,32 @@ class TestMembership(unittest.TestCase): self.assertEqual(cm.exception.reason, b'Cannot convert parameters: moderation_action') + def test_bad_preferences_url(self): + with transaction(): + subscribe(self._mlist, 'Anne') + with self.assertRaises(HTTPError) as cm: + call_api('http://localhost:9001/3.0/members/1/preferences/bogus') + self.assertEqual(cm.exception.code, 404) + + def test_not_a_member_preferences(self): + with self.assertRaises(HTTPError) as cm: + call_api('http://localhost:9001/3.0/members/1/preferences') + self.assertEqual(cm.exception.code, 404) + + def test_not_a_member_all_preferences(self): + with self.assertRaises(HTTPError) as cm: + call_api('http://localhost:9001/3.0/members/1/all/preferences') + self.assertEqual(cm.exception.code, 404) + + def test_delete_other_role(self): + with transaction(): + subscribe(self._mlist, 'Anne', MemberRole.moderator) + response, headers = call_api( + 'http://localhost:9001/3.0/members/1', + method='DELETE') + self.assertEqual(headers.status, 204) + self.assertEqual(len(list(self._mlist.moderators.members)), 0) + class CustomLayer(ConfigLayer): @@ -371,3 +414,135 @@ Some text. # previously been linked to a user record. self.assertEqual(nonmember['user'], 'http://localhost:9001/3.0/users/1') + + + +class TestAPI31Members(unittest.TestCase): + layer = RESTLayer + + def setUp(self): + with transaction(): + self._mlist = create_list('a...@example.com') + + def test_member_ids_are_hex(self): + with transaction(): + subscribe(self._mlist, 'Anne') + subscribe(self._mlist, 'Bart') + response, headers = call_api('http://localhost:9001/3.1/members') + entries = response['entries'] + self.assertEqual(len(entries), 2) + self.assertEqual( + entries[0]['self_link'], + 'http://localhost:9001/3.1/members/00000000000000000000000000000001') + self.assertEqual( + entries[0]['member_id'], + '00000000000000000000000000000001') + self.assertEqual( + entries[0]['user'], + 'http://localhost:9001/3.1/users/00000000000000000000000000000001') + self.assertEqual( + entries[1]['self_link'], + 'http://localhost:9001/3.1/members/00000000000000000000000000000002') + self.assertEqual( + entries[1]['member_id'], + '00000000000000000000000000000002') + self.assertEqual( + entries[1]['user'], + 'http://localhost:9001/3.1/users/00000000000000000000000000000002') + + def test_get_member_id_by_hex(self): + with transaction(): + subscribe(self._mlist, 'Anne') + response, headers = call_api( + 'http://localhost:9001/3.1/members/00000000000000000000000000000001') + self.assertEqual( + response['member_id'], + '00000000000000000000000000000001') + self.assertEqual( + response['self_link'], + 'http://localhost:9001/3.1/members/00000000000000000000000000000001') + self.assertEqual( + response['user'], + 'http://localhost:9001/3.1/users/00000000000000000000000000000001') + self.assertEqual( + response['address'], + 'http://localhost:9001/3.1/addresses/aper...@example.com') + + def test_cannot_get_member_id_by_int(self): + with transaction(): + subscribe(self._mlist, 'Anne') + with self.assertRaises(HTTPError) as cm: + call_api('http://localhost:9001/3.1/members/1') + self.assertEqual(cm.exception.code, 404) + + def test_preferences(self): + with transaction(): + member = subscribe(self._mlist, 'Anne') + member.preferences.delivery_mode = DeliveryMode.summary_digests + response, headers = call_api( + 'http://localhost:9001/3.1/members' + '/00000000000000000000000000000001/preferences') + self.assertEqual(response['delivery_mode'], 'summary_digests') + + def test_all_preferences(self): + with transaction(): + member = subscribe(self._mlist, 'Anne') + member.preferences.delivery_mode = DeliveryMode.summary_digests + response, headers = call_api( + 'http://localhost:9001/3.1/members' + '/00000000000000000000000000000001/all/preferences') + self.assertEqual(response['delivery_mode'], 'summary_digests') + + def test_create_new_membership_by_hex(self): + with transaction(): + user = getUtility(IUserManager).create_user('a...@example.com') + _set_preferred(user) + # Subscribe the user to the mailing list by hex UUID. + response, headers = call_api( + 'http://localhost:9001/3.1/members', { + 'list_id': 'ant.example.com', + 'subscriber': '00000000000000000000000000000001', + 'pre_verified': True, + 'pre_confirmed': True, + 'pre_approved': True, + }) + self.assertEqual(headers.status, 201) + self.assertEqual( + headers['location'], + 'http://localhost:9001/3.1/members/00000000000000000000000000000001' + ) + + def test_create_new_owner_by_hex(self): + with transaction(): + user = getUtility(IUserManager).create_user('a...@example.com') + _set_preferred(user) + # Subscribe the user to the mailing list by hex UUID. + response, headers = call_api( + 'http://localhost:9001/3.1/members', { + 'list_id': 'ant.example.com', + 'subscriber': '00000000000000000000000000000001', + 'role': 'owner', + }) + self.assertEqual(headers.status, 201) + self.assertEqual( + headers['location'], + 'http://localhost:9001/3.1/members/00000000000000000000000000000001' + ) + + def test_cannot_create_new_membership_by_int(self): + with transaction(): + user = getUtility(IUserManager).create_user('a...@example.com') + _set_preferred(user) + # We can't use the int representation of the UUID with API 3.1. + with self.assertRaises(HTTPError) as cm: + call_api('http://localhost:9001/3.1/members', { + 'list_id': 'ant.example.com', + 'subscriber': '1', + 'pre_verified': True, + 'pre_confirmed': True, + 'pre_approved': True, + }) + # This is a bad request because the `subscriber` value isn't something + # that's known to the system, in API 3.1. It's not technically a 404 + # because that's reserved for URL lookups. + self.assertEqual(cm.exception.code, 400) ===================================== src/mailman/rest/tests/test_users.py ===================================== --- a/src/mailman/rest/tests/test_users.py +++ b/src/mailman/rest/tests/test_users.py @@ -18,6 +18,7 @@ """REST user tests.""" __all__ = [ + 'TestAPI31Users', 'TestLP1074374', 'TestLP1419519', 'TestLogin', @@ -31,6 +32,7 @@ import unittest from mailman.app.lifecycle import create_list from mailman.config import config from mailman.database.transaction import transaction +from mailman.interfaces.member import DeliveryMode from mailman.interfaces.usermanager import IUserManager from mailman.testing.helpers import call_api, configuration from mailman.testing.layers import RESTLayer @@ -292,6 +294,16 @@ class TestUsers(unittest.TestCase): id=anne.preferences.id) self.assertEqual(preferences.count(), 0) + def test_preferences_self_link(self): + with transaction(): + user = getUtility(IUserManager).create_user('a...@example.com') + user.preferences.delivery_mode = DeliveryMode.summary_digests + content, response = call_api( + 'http://localhost:9001/3.0/users/1/preferences') + self.assertEqual( + content['self_link'], + 'http://localhost:9001/3.0/users/1/preferences') + class TestLogin(unittest.TestCase): @@ -502,3 +514,69 @@ class TestLP1419519(unittest.TestCase): config.db.abort() emails = sorted(address.email for address in self.manager.addresses) self.assertEqual(len(emails), 0) + + + +class TestAPI31Users(unittest.TestCase): + """UUIDs are represented as hex in API 3.1.""" + + layer = RESTLayer + + def test_get_user(self): + with transaction(): + getUtility(IUserManager).create_user('a...@example.com') + content, response = call_api( + 'http://localhost:9001/3.1/users/00000000000000000000000000000001') + self.assertEqual( + content['user_id'], '00000000000000000000000000000001') + self.assertEqual( + content['self_link'], + 'http://localhost:9001/3.1/users/00000000000000000000000000000001') + + def test_cannot_get_user_by_int(self): + with transaction(): + getUtility(IUserManager).create_user('a...@example.com') + with self.assertRaises(HTTPError) as cm: + call_api('http://localhost:9001/3.1/users/1') + self.assertEqual(cm.exception.code, 404) + + def test_get_all_users(self): + user_manager = getUtility(IUserManager) + with transaction(): + user_manager.create_user('a...@example.com') + user_manager.create_user('b...@example.com') + content, response = call_api('http://localhost:9001/3.1/users') + entries = content['entries'] + self.assertEqual(len(entries), 2) + self.assertEqual( + entries[0]['user_id'], '00000000000000000000000000000001') + self.assertEqual( + entries[0]['self_link'], + 'http://localhost:9001/3.1/users/00000000000000000000000000000001') + self.assertEqual( + entries[1]['user_id'], '00000000000000000000000000000002') + self.assertEqual( + entries[1]['self_link'], + 'http://localhost:9001/3.1/users/00000000000000000000000000000002') + + def test_create_user(self): + content, response = call_api( + 'http://localhost:9001/3.1/users', { + 'email': 'a...@example.com', + }) + self.assertEqual(response.status, 201) + self.assertEqual( + response['location'], + 'http://localhost:9001/3.1/users/00000000000000000000000000000001') + + def test_preferences_self_link(self): + with transaction(): + user = getUtility(IUserManager).create_user('a...@example.com') + user.preferences.delivery_mode = DeliveryMode.summary_digests + content, response = call_api( + 'http://localhost:9001/3.1/users' + '/00000000000000000000000000000001/preferences') + self.assertEqual( + content['self_link'], + 'http://localhost:9001/3.1/users' + '/00000000000000000000000000000001/preferences') ===================================== src/mailman/rest/tests/test_validator.py ===================================== --- a/src/mailman/rest/tests/test_validator.py +++ b/src/mailman/rest/tests/test_validator.py @@ -50,15 +50,38 @@ class TestValidators(unittest.TestCase): self.assertRaises(ValueError, list_of_strings_validator, 7) self.assertRaises(ValueError, list_of_strings_validator, ['ant', 7]) - def test_subscriber_validator_uuid(self): + def test_subscriber_validator_int_uuid(self): # Convert from an existing user id to a UUID. anne = getUtility(IUserManager).make_user('a...@example.com') - uuid = subscriber_validator(str(anne.user_id.int)) + uuid = subscriber_validator('3.0')(str(anne.user_id.int)) self.assertEqual(anne.user_id, uuid) - def test_subscriber_validator_bad_uuid(self): - self.assertRaises(ValueError, subscriber_validator, 'not-a-thing') + def test_subscriber_validator_hex_uuid(self): + # Convert from an existing user id to a UUID. + anne = getUtility(IUserManager).make_user('a...@example.com') + uuid = subscriber_validator('3.1')(anne.user_id.hex) + self.assertEqual(anne.user_id, uuid) + + def test_subscriber_validator_no_int_uuid(self): + # API 3.1 does not accept ints as subscriber id's. + anne = getUtility(IUserManager).make_user('a...@example.com') + self.assertRaises(ValueError, + subscriber_validator('3.1'), str(anne.user_id.int)) + + def test_subscriber_validator_bad_int_uuid(self): + # In API 3.0, UUIDs are ints. + self.assertRaises(ValueError, + subscriber_validator('3.0'), 'not-a-thing') + + def test_subscriber_validator_bad_int_hex(self): + # In API 3.1, UUIDs are hexes. + self.assertRaises(ValueError, + subscriber_validator('3.1'), 'not-a-thing') + + def test_subscriber_validator_email_address_API30(self): + self.assertEqual(subscriber_validator('3.0')('a...@example.com'), + 'a...@example.com') - def test_subscriber_validator_email_address(self): - self.assertEqual(subscriber_validator('a...@example.com'), + def test_subscriber_validator_email_address_API31(self): + self.assertEqual(subscriber_validator('3.1')('a...@example.com'), 'a...@example.com') ===================================== src/mailman/rest/users.py ===================================== --- a/src/mailman/rest/users.py +++ b/src/mailman/rest/users.py @@ -117,8 +117,9 @@ def create_user(arguments, request, response): password = generate(int(config.passwords.password_length)) user.password = config.password_context.encrypt(password) user.is_server_owner = is_server_owner - location = path_to('users/{}'.format(user.user_id.int), - request.context['api_version']) + api_version = request.context['api_version'] + user_id = getattr(user.user_id, 'int' if api_version == '3.0' else 'hex') + location = path_to('users/{}'.format(user_id), api_version) created(response, location) return user @@ -127,13 +128,17 @@ def create_user(arguments, request, response): class _UserBase(CollectionMixin): """Shared base class for user representations.""" + def _get_uuid(self, user): + return getattr(user.user_id, + 'int' if self.api_version == '3.0' else 'hex') + def _resource_as_dict(self, user): """See `CollectionMixin`.""" # The canonical URL for a user is their unique user id, although we # can always look up a user based on any registered and validated # email address associated with their account. The user id is a UUID, # but we serialize its integer equivalent. - user_id = user.user_id.int + user_id = self._get_uuid(user) resource = dict( created_on=user.created_on, is_server_owner=user.is_server_owner, @@ -177,24 +182,29 @@ class AllUsers(_UserBase): class AUser(_UserBase): """A user.""" - def __init__(self, user_identifier): + def __init__(self, api_version, user_identifier): """Get a user by various type of identifiers. :param user_identifier: The identifier used to retrieve the user. The - identifier may either be an integer user-id, or an email address - controlled by the user. The type of identifier is auto-detected + identifier may either be an email address controlled by the user + or the UUID of the user. The type of identifier is auto-detected by looking for an `@` symbol, in which case it's taken as an email - address, otherwise it's assumed to be an integer. + address, otherwise it's assumed to be a UUID. However, UUIDs in + API 3.0 are integers, while in 3.1 are hex. :type user_identifier: string """ + self.api_version = api_version user_manager = getUtility(IUserManager) if '@' in user_identifier: self._user = user_manager.get_user(user_identifier) else: - # The identifier is the string representation of an integer that - # must be converted to a UUID. + # The identifier is the string representation of a UUID, either an + # int in API 3.0 or a hex in API 3.1. try: - user_id = UUID(int=int(user_identifier)) + if api_version == '3.0': + user_id = UUID(int=int(user_identifier)) + else: + user_id = UUID(hex=user_identifier) except ValueError: self._user = None else: @@ -227,14 +237,14 @@ class AUser(_UserBase): @child() def preferences(self, request, segments): - """/addresses/<email>/preferences""" + """/users/<id>/preferences""" if len(segments) != 0: return BadRequest(), [] if self._user is None: return NotFound(), [] child = Preferences( self._user.preferences, - 'users/{0}'.format(self._user.user_id.int)) + 'users/{}'.format(self._get_uuid(self._user))) return child, [] def on_patch(self, request, response): @@ -309,13 +319,14 @@ class AddressUser(_UserBase): if self._user: conflict(response) return + api_version = request.context['api_version'] # When creating a linked user by POSTing, the user either must already # exist, or it can be automatically created, if the auto_create flag # is given and true (if missing, it defaults to true). However, in # this case we do not accept 'email' as a POST field. fields = CREATION_FIELDS.copy() del fields['email'] - fields['user_id'] = int + fields['user_id'] = (int if api_version == '3.0' else str) fields['auto_create'] = as_boolean fields['_optional'] = fields['_optional'] + ( 'user_id', 'auto_create', 'is_server_owner') @@ -328,7 +339,12 @@ class AddressUser(_UserBase): user_manager = getUtility(IUserManager) if 'user_id' in arguments: raw_uid = arguments['user_id'] - user_id = UUID(int=raw_uid) + kws = {('int' if api_version == '3.0' else 'hex'): raw_uid} + try: + user_id = UUID(**kws) + except ValueError as error: + bad_request(response, str(error)) + return user = user_manager.get_user_by_id(user_id) if user is None: not_found(response, b'No user with ID {}'.format(raw_uid)) @@ -348,11 +364,12 @@ class AddressUser(_UserBase): def on_put(self, request, response): """Set or replace the addresses's user.""" + api_version = request.context['api_version'] if self._user: self._user.unlink(self._address) # Process post data and check for an existing user. fields = CREATION_FIELDS.copy() - fields['user_id'] = int + fields['user_id'] = (int if api_version == '3.0' else str) fields['_optional'] = fields['_optional'] + ( 'user_id', 'email', 'is_server_owner') try: @@ -364,7 +381,12 @@ class AddressUser(_UserBase): user_manager = getUtility(IUserManager) if 'user_id' in arguments: raw_uid = arguments['user_id'] - user_id = UUID(int=raw_uid) + kws = {('int' if api_version == '3.0' else 'hex'): raw_uid} + try: + user_id = UUID(**kws) + except ValueError as error: + bad_request(response, str(error)) + return user = user_manager.get_user_by_id(user_id) if user is None: not_found(response, b'No user with ID {}'.format(raw_uid)) ===================================== src/mailman/rest/validator.py ===================================== --- a/src/mailman/rest/validator.py +++ b/src/mailman/rest/validator.py @@ -55,15 +55,24 @@ class enum_validator: raise ValueError(exception.args[0]) -def subscriber_validator(subscriber): - """Convert an email-or-int to an email-or-UUID.""" - try: - return UUID(int=int(subscriber)) - except ValueError: - # It must be an email address. - if getUtility(IEmailValidator).is_valid(subscriber): - return subscriber - raise ValueError +def subscriber_validator(api_version): + """Convert an email-or-(int|hex) to an email-or-UUID.""" + def _inner(subscriber): + # In API 3.0, the uuid is represented by an int, so if we can int + # convert the value, we know it's a UUID-as-int. In API 3.1 though, + # uuids are represented by the hex version, which of course cannot + # include an @ sign. + try: + if api_version == '3.0': + return UUID(int=int(subscriber)) + else: + return UUID(hex=subscriber) + except ValueError: + # It must be an email address. + if getUtility(IEmailValidator).is_valid(subscriber): + return subscriber + raise ValueError + return _inner def language_validator(code): ===================================== src/mailman/testing/documentation.py ===================================== --- a/src/mailman/testing/documentation.py +++ b/src/mailman/testing/documentation.py @@ -129,13 +129,13 @@ def dump_json(url, data=None, method=None, username=None, password=None): # entry is a dictionary. print('entry %d:' % i) for entry_key in sorted(entry): - print(' {0}: {1}'.format(entry_key, entry[entry_key])) + print(' {}: {}'.format(entry_key, entry[entry_key])) elif isinstance(value, list): printable_value = COMMASPACE.join( - "'{0}'".format(s) for s in sorted(value)) - print('{0}: [{1}]'.format(key, printable_value)) + "'{}'".format(s) for s in sorted(value)) + print('{}: [{}]'.format(key, printable_value)) else: - print('{0}: {1}'.format(key, value)) + print('{}: {}'.format(key, value)) View it on GitLab: https://gitlab.com/mailman/mailman/compare/8e69b848270da6ba4852f8c6dfdeeed8124ab024...35577e20f8133880377f211eded6faafb60cffd3
_______________________________________________ Mailman-checkins mailing list Mailman-checkins@python.org Unsubscribe: https://mail.python.org/mailman/options/mailman-checkins/archive%40jab.org