From 569553ab883624c9c0dc2217d4e16b6841fb0f23 Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Wed, 15 Apr 2015 16:24:15 -0400 Subject: The SubscriptionWorkflow and Registar classes now have both a token and a "token owner". The latter describes who owns the token --i.e. which phase of the workflow is being waited on. It can either be no one, the subscriber, or the moderator. Tokens and token owners are properly initialized and reset when the workflow is completed, so we always know which step of the process is being waited on. Also, remove ISubscriptionService.join() since this will now be handled by the IRegistrar adapter. --- src/mailman/app/registrar.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'src/mailman/app/registrar.py') diff --git a/src/mailman/app/registrar.py b/src/mailman/app/registrar.py index ae4322d22..a3699de91 100644 --- a/src/mailman/app/registrar.py +++ b/src/mailman/app/registrar.py @@ -63,7 +63,7 @@ class Registrar: pre_confirmed=pre_confirmed, pre_approved=pre_approved) list(workflow) - return workflow.token + return workflow.token, workflow.token_owner def confirm(self, token): """See `IRegistrar`.""" @@ -71,7 +71,7 @@ class Registrar: workflow.token = token workflow.restore() list(workflow) - return workflow.token + return workflow.token, workflow.token_owner def discard(self, token): """See `IRegistrar`.""" -- cgit v1.2.3-70-g09d2 From 08f457799cd36349a4fd22642f4c05b4eabb306d Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Wed, 15 Apr 2015 22:51:39 -0400 Subject: Plumb the subscription policy through the REST API. --- TODO.rst | 2 +- src/mailman/app/registrar.py | 4 +- src/mailman/app/subscriptions.py | 15 ++- src/mailman/app/tests/test_registrar.py | 47 ++++++--- src/mailman/app/tests/test_subscriptions.py | 32 +++++- src/mailman/commands/eml_confirm.py | 11 ++- src/mailman/commands/tests/test_confirm.py | 3 +- src/mailman/interfaces/registrar.py | 5 +- src/mailman/model/docs/registration.rst | 18 +++- src/mailman/rest/docs/membership.rst | 136 +++++-------------------- src/mailman/rest/helpers.py | 6 ++ src/mailman/rest/members.py | 148 +++++++++++++++++++++++----- src/mailman/rest/tests/test_membership.py | 12 +++ src/mailman/rest/tests/test_users.py | 3 +- src/mailman/runners/docs/command.rst | 2 +- src/mailman/runners/tests/test_confirm.py | 3 +- src/mailman/runners/tests/test_join.py | 10 +- 17 files changed, 287 insertions(+), 170 deletions(-) (limited to 'src/mailman/app/registrar.py') diff --git a/TODO.rst b/TODO.rst index 362499258..131c52faf 100644 --- a/TODO.rst +++ b/TODO.rst @@ -1,5 +1,5 @@ * TO DO: - - plumb subscription policy through rest api + - rename ISubscriptionService to IMemberSearch or somesuch - get rid of hold_subscription - subsume handle_subscription - workflow for unsubscription diff --git a/src/mailman/app/registrar.py b/src/mailman/app/registrar.py index a3699de91..240742bc0 100644 --- a/src/mailman/app/registrar.py +++ b/src/mailman/app/registrar.py @@ -63,7 +63,7 @@ class Registrar: pre_confirmed=pre_confirmed, pre_approved=pre_approved) list(workflow) - return workflow.token, workflow.token_owner + return workflow.token, workflow.token_owner, workflow.member def confirm(self, token): """See `IRegistrar`.""" @@ -71,7 +71,7 @@ class Registrar: workflow.token = token workflow.restore() list(workflow) - return workflow.token, workflow.token_owner + return workflow.token, workflow.token_owner, workflow.member def discard(self, token): """See `IRegistrar`.""" diff --git a/src/mailman/app/subscriptions.py b/src/mailman/app/subscriptions.py index aa9d600bf..1c6b96016 100644 --- a/src/mailman/app/subscriptions.py +++ b/src/mailman/app/subscriptions.py @@ -223,10 +223,16 @@ class SubscriptionWorkflow(Workflow): if self.mlist.subscription_policy is SubscriptionPolicy.moderate: self.push('moderation_checks') return - # If the subscription has been pre-confirmed, then we can skip to the - # moderation checks. + # If the subscription has been pre-confirmed, then we can skip the + # confirmation check can be skipped. If moderator approval is + # required we need to check that, otherwise we can go straight to + # subscription. if self.pre_confirmed: - self.push('moderation_checks') + next_step = ('moderation_checks' + if self.mlist.subscription_policy is + SubscriptionPolicy.confirm_then_moderate + else 'do_subscription') + self.push(next_step) return # The user must confirm their subscription. self.push('send_confirmation') @@ -235,7 +241,8 @@ class SubscriptionWorkflow(Workflow): # Does the moderator need to approve the subscription request? assert self.mlist.subscription_policy in ( SubscriptionPolicy.moderate, - SubscriptionPolicy.confirm_then_moderate) + SubscriptionPolicy.confirm_then_moderate, + ), self.mlist.subscription_policy if self.pre_approved: self.push('do_subscription') else: diff --git a/src/mailman/app/tests/test_registrar.py b/src/mailman/app/tests/test_registrar.py index 1431b5e6f..a2ab2c686 100644 --- a/src/mailman/app/tests/test_registrar.py +++ b/src/mailman/app/tests/test_registrar.py @@ -48,13 +48,14 @@ class TestRegistrar(unittest.TestCase): self._anne = getUtility(IUserManager).create_address( 'anne@example.com') - def test_unique_token(self): + def test_initial_conditions(self): # Registering a subscription request provides a unique token associated # with a pendable, and the owner of the token. self.assertEqual(self._pendings.count, 0) - token, token_owner = self._registrar.register(self._anne) + token, token_owner, member = self._registrar.register(self._anne) self.assertIsNotNone(token) self.assertEqual(token_owner, TokenOwner.subscriber) + self.assertIsNone(member) self.assertEqual(self._pendings.count, 1) record = self._pendings.confirm(token, expunge=False) self.assertEqual(record['list_id'], self._mlist.list_id) @@ -66,10 +67,11 @@ class TestRegistrar(unittest.TestCase): # there's nothing more to do. self._mlist.subscription_policy = SubscriptionPolicy.open self._anne.verified_on = now() - token, token_owner = self._registrar.register(self._anne) + token, token_owner, rmember = self._registrar.register(self._anne) self.assertIsNone(token) self.assertEqual(token_owner, TokenOwner.no_one) member = self._mlist.regular_members.get_member('anne@example.com') + self.assertEqual(rmember, member) self.assertEqual(member.address, self._anne) # There's nothing to confirm. record = self._pendings.confirm(token, expunge=False) @@ -86,16 +88,18 @@ class TestRegistrar(unittest.TestCase): # to approve. Running the workflow gives us a token. Confirming the # token subscribes the user. self._mlist.subscription_policy = SubscriptionPolicy.open - token, token_owner = self._registrar.register(self._anne) + token, token_owner, rmember = self._registrar.register(self._anne) self.assertIsNotNone(token) self.assertEqual(token_owner, TokenOwner.subscriber) + self.assertIsNone(rmember) member = self._mlist.regular_members.get_member('anne@example.com') self.assertIsNone(member) # Now confirm the subscription. - token, token_owner = self._registrar.confirm(token) + token, token_owner, rmember = self._registrar.confirm(token) self.assertIsNone(token) self.assertEqual(token_owner, TokenOwner.no_one) member = self._mlist.regular_members.get_member('anne@example.com') + self.assertEqual(rmember, member) self.assertEqual(member.address, self._anne) def test_confirm_because_confirm(self): @@ -105,16 +109,18 @@ class TestRegistrar(unittest.TestCase): # user. self._mlist.subscription_policy = SubscriptionPolicy.confirm self._anne.verified_on = now() - token, token_owner = self._registrar.register(self._anne) + token, token_owner, rmember = self._registrar.register(self._anne) self.assertIsNotNone(token) self.assertEqual(token_owner, TokenOwner.subscriber) + self.assertIsNone(rmember) member = self._mlist.regular_members.get_member('anne@example.com') self.assertIsNone(member) # Now confirm the subscription. - token, token_owner = self._registrar.confirm(token) + token, token_owner, rmember = self._registrar.confirm(token) self.assertIsNone(token) self.assertEqual(token_owner, TokenOwner.no_one) member = self._mlist.regular_members.get_member('anne@example.com') + self.assertEqual(rmember, member) self.assertEqual(member.address, self._anne) def test_confirm_because_moderation(self): @@ -123,16 +129,18 @@ class TestRegistrar(unittest.TestCase): # token subscribes the user. self._mlist.subscription_policy = SubscriptionPolicy.moderate self._anne.verified_on = now() - token, token_owner = self._registrar.register(self._anne) + token, token_owner, rmember = self._registrar.register(self._anne) self.assertIsNotNone(token) self.assertEqual(token_owner, TokenOwner.moderator) + self.assertIsNone(rmember) member = self._mlist.regular_members.get_member('anne@example.com') self.assertIsNone(member) # Now confirm the subscription. - token, token_owner = self._registrar.confirm(token) + token, token_owner, rmember = self._registrar.confirm(token) self.assertIsNone(token) self.assertEqual(token_owner, TokenOwner.no_one) member = self._mlist.regular_members.get_member('anne@example.com') + self.assertEqual(rmember, member) self.assertEqual(member.address, self._anne) def test_confirm_because_confirm_then_moderation(self): @@ -145,27 +153,30 @@ class TestRegistrar(unittest.TestCase): SubscriptionPolicy.confirm_then_moderate self._anne.verified_on = now() # Runs until subscription confirmation. - token, token_owner = self._registrar.register(self._anne) + token, token_owner, rmember = self._registrar.register(self._anne) self.assertIsNotNone(token) self.assertEqual(token_owner, TokenOwner.subscriber) + self.assertIsNone(rmember) member = self._mlist.regular_members.get_member('anne@example.com') self.assertIsNone(member) # Now confirm the subscription, and wait for the moderator to approve # the subscription. She is still not subscribed. - new_token, token_owner = self._registrar.confirm(token) + new_token, token_owner, rmember = self._registrar.confirm(token) # The new token, used for the moderator to approve the message, is not # the same as the old token. self.assertNotEqual(new_token, token) self.assertIsNotNone(new_token) self.assertEqual(token_owner, TokenOwner.moderator) + self.assertIsNone(rmember) member = self._mlist.regular_members.get_member('anne@example.com') self.assertIsNone(member) # Confirm once more, this time as the moderator approving the # subscription. Now she's a member. - token, token_owner = self._registrar.confirm(new_token) + token, token_owner, rmember = self._registrar.confirm(new_token) self.assertIsNone(token) self.assertEqual(token_owner, TokenOwner.no_one) member = self._mlist.regular_members.get_member('anne@example.com') + self.assertEqual(rmember, member) self.assertEqual(member.address, self._anne) def test_confirm_then_moderate_with_different_tokens(self): @@ -177,18 +188,20 @@ class TestRegistrar(unittest.TestCase): SubscriptionPolicy.confirm_then_moderate self._anne.verified_on = now() # Runs until subscription confirmation. - token, token_owner = self._registrar.register(self._anne) + token, token_owner, rmember = self._registrar.register(self._anne) self.assertIsNotNone(token) self.assertEqual(token_owner, TokenOwner.subscriber) + self.assertIsNone(rmember) member = self._mlist.regular_members.get_member('anne@example.com') self.assertIsNone(member) # Now confirm the subscription, and wait for the moderator to approve # the subscription. She is still not subscribed. - new_token, token_owner = self._registrar.confirm(token) + new_token, token_owner, rmember = self._registrar.confirm(token) # The status is not true because the user has not yet been subscribed # to the mailing list. self.assertIsNotNone(new_token) self.assertEqual(token_owner, TokenOwner.moderator) + self.assertIsNone(rmember) member = self._mlist.regular_members.get_member('anne@example.com') self.assertIsNone(member) # The new token is different than the old token. @@ -197,11 +210,12 @@ class TestRegistrar(unittest.TestCase): self.assertRaises(LookupError, self._registrar.confirm, token) # Confirm once more, this time with the new token, as the moderator # approving the subscription. Now she's a member. - done_token, token_owner = self._registrar.confirm(new_token) + done_token, token_owner, rmember = self._registrar.confirm(new_token) # The token is None, signifying that the member has been subscribed. self.assertIsNone(done_token) self.assertEqual(token_owner, TokenOwner.no_one) member = self._mlist.regular_members.get_member('anne@example.com') + self.assertEqual(rmember, member) self.assertEqual(member.address, self._anne) def test_discard_waiting_for_confirmation(self): @@ -210,9 +224,10 @@ class TestRegistrar(unittest.TestCase): self._mlist.subscription_policy = SubscriptionPolicy.confirm self._anne.verified_on = now() # Runs until subscription confirmation. - token, token_owner = self._registrar.register(self._anne) + token, token_owner, rmember = self._registrar.register(self._anne) self.assertIsNotNone(token) self.assertEqual(token_owner, TokenOwner.subscriber) + self.assertIsNone(rmember) member = self._mlist.regular_members.get_member('anne@example.com') self.assertIsNone(member) # Now discard the subscription request. diff --git a/src/mailman/app/tests/test_subscriptions.py b/src/mailman/app/tests/test_subscriptions.py index fa4fec78f..42984c77c 100644 --- a/src/mailman/app/tests/test_subscriptions.py +++ b/src/mailman/app/tests/test_subscriptions.py @@ -203,13 +203,29 @@ class TestSubscriptionWorkflow(unittest.TestCase): def test_confirmation_checks_confirm_pre_confirmed(self): # The subscription policy requires user confirmation, but their - # subscription is pre-confirmed. + # subscription is pre-confirmed. Since moderation is not required, + # the user will be immediately subscribed. self._mlist.subscription_policy = SubscriptionPolicy.confirm anne = self._user_manager.create_address(self._anne) workflow = SubscriptionWorkflow(self._mlist, anne, pre_verified=True, pre_confirmed=True) workflow.run_thru('confirmation_checks') + with patch.object(workflow, '_step_do_subscription') as step: + next(workflow) + step.assert_called_once_with() + + def test_confirmation_checks_confirm_then_moderate_pre_confirmed(self): + # The subscription policy requires user confirmation, but their + # subscription is pre-confirmed. Since moderation is required, that + # check will be performed. + self._mlist.subscription_policy = \ + SubscriptionPolicy.confirm_then_moderate + anne = self._user_manager.create_address(self._anne) + workflow = SubscriptionWorkflow(self._mlist, anne, + pre_verified=True, + pre_confirmed=True) + workflow.run_thru('confirmation_checks') with patch.object(workflow, '_step_moderation_checks') as step: next(workflow) step.assert_called_once_with() @@ -598,3 +614,17 @@ approval: # No further token is needed. self.assertIsNone(final_workflow.token) self.assertEqual(final_workflow.token_owner, TokenOwner.no_one) + + def test_confirmation_needed_and_pre_confirmed(self): + # The subscription policy is 'confirm' but the subscription is + # pre-confirmed so the moderation checks can be skipped. + self._mlist.subscription_policy = SubscriptionPolicy.confirm + anne = self._user_manager.create_address(self._anne) + workflow = SubscriptionWorkflow( + self._mlist, anne, + pre_verified=True, pre_confirmed=True, pre_approved=True) + list(workflow) + # Anne was subscribed. + self.assertIsNone(workflow.token) + self.assertEqual(workflow.token_owner, TokenOwner.no_one) + self.assertEqual(workflow.member.address, anne) diff --git a/src/mailman/commands/eml_confirm.py b/src/mailman/commands/eml_confirm.py index 077fab9a6..ddf0db0e2 100644 --- a/src/mailman/commands/eml_confirm.py +++ b/src/mailman/commands/eml_confirm.py @@ -25,6 +25,7 @@ __all__ = [ from mailman.core.i18n import _ from mailman.interfaces.command import ContinueProcessing, IEmailCommand from mailman.interfaces.registrar import IRegistrar +from mailman.interfaces.subscriptions import TokenOwner from zope.interface import implementer @@ -53,7 +54,15 @@ class Confirm: tokens.add(token) results.confirms = tokens try: - succeeded = (IRegistrar(mlist).confirm(token) is None) + token, token_owner, member = IRegistrar(mlist).confirm(token) + if token is None: + assert token_owner is TokenOwner.no_one, token_owner + assert member is not None, member + succeeded = True + else: + assert token_owner is not TokenOwner.no_one, token_owner + assert member is None, member + succeeded = False except LookupError: # The token must not exist in the database. succeeded = False diff --git a/src/mailman/commands/tests/test_confirm.py b/src/mailman/commands/tests/test_confirm.py index 98a26bf7d..e980141b0 100644 --- a/src/mailman/commands/tests/test_confirm.py +++ b/src/mailman/commands/tests/test_confirm.py @@ -46,7 +46,8 @@ class TestConfirm(unittest.TestCase): self._mlist = create_list('test@example.com') anne = getUtility(IUserManager).create_address( 'anne@example.com', 'Anne Person') - self._token = IRegistrar(self._mlist).register(anne) + self._token, token_owner, member = IRegistrar(self._mlist).register( + anne) self._command = Confirm() # Clear the virgin queue. get_queue_messages('virgin') diff --git a/src/mailman/interfaces/registrar.py b/src/mailman/interfaces/registrar.py index 019ce9d2a..6f049b9d7 100644 --- a/src/mailman/interfaces/registrar.py +++ b/src/mailman/interfaces/registrar.py @@ -77,8 +77,9 @@ class IRegistrar(Interface): :type email: ``IUser`` or ``IAddress`` :return: None if the workflow completes with the member being subscribed. If the workflow is paused for user confirmation or - moderator approval, a 2-tuple is returned where the first element - is a ``TokenOwner`` and the second element is the token hash. + moderator approval, a 3-tuple is returned where the first element + is a ``TokenOwner`` the second element is the token hash, and the + third element is the subscribed member. :rtype: None or 2-tuple of (TokenOwner, str) :raises MembershipIsBannedError: when the address being subscribed appears in the global or list-centric bans. diff --git a/src/mailman/model/docs/registration.rst b/src/mailman/model/docs/registration.rst index fc7ad6f1a..4b1e13520 100644 --- a/src/mailman/model/docs/registration.rst +++ b/src/mailman/model/docs/registration.rst @@ -35,11 +35,13 @@ which represents this work flow. Anne attempts to join the mailing list. - >>> token = registrar.register(anne) + >>> token, token_owner, member = registrar.register(anne) Because her email address has not yet been verified, she has not yet become a member of the mailing list. + >>> print(member) + None >>> print(mlist.members.get_member('anne@example.com')) None @@ -47,7 +49,10 @@ Once she verifies her email address, she will become a member of the mailing list. In this case, verifying implies that she also confirms her wish to join the mailing list. - >>> registrar.confirm(token) + >>> token, token_owner, member = registrar.confirm(token) + >>> member + on ant@example.com + as MemberRole.member> >>> mlist.members.get_member('anne@example.com') on ant@example.com as MemberRole.member> @@ -78,13 +83,18 @@ Now when Bart registers as a user for the mailing list, a token will still be generated, but this is only used by the moderator. At first, Bart is not subscribed to the mailing list. - >>> token = registrar.register(bart) + >>> token, token_owner, member = registrar.register(bart) + >>> print(member) + None >>> print(mlist.members.get_member('bart@example.com')) None When the moderator confirms Bart's subscription, he joins the mailing list. - >>> registrar.confirm(token) + >>> token, token_owner, member = registrar.confirm(token) + >>> member + on ant@example.com + as MemberRole.member> >>> mlist.members.get_member('bart@example.com') on ant@example.com as MemberRole.member> diff --git a/src/mailman/rest/docs/membership.rst b/src/mailman/rest/docs/membership.rst index 3cb83a0c8..f3a09cfe9 100644 --- a/src/mailman/rest/docs/membership.rst +++ b/src/mailman/rest/docs/membership.rst @@ -598,14 +598,17 @@ A user can be subscribed to a mailing list via the REST API, either by a specific address, or more generally by their preferred address. A subscribed user is called a member. -Elly wants to subscribes to the `ant` mailing list. Since Elly's email -address is not yet known to Mailman, a user is created for her. By default, -get gets a regular delivery. +The list owner wants to subscribe Elly to the `ant` mailing list. Since +Elly's email address is not yet known to Mailman, a user is created for her. +By default, get gets a regular delivery. >>> dump_json('http://localhost:9001/3.0/members', { ... 'list_id': 'ant.example.com', ... 'subscriber': 'eperson@example.com', ... 'display_name': 'Elly Person', + ... 'pre_verified': True, + ... 'pre_confirmed': True, + ... 'pre_approved': True, ... }) content-length: 0 date: ... @@ -659,6 +662,9 @@ list with her preferred address. >>> dump_json('http://localhost:9001/3.0/members', { ... 'list_id': 'ant.example.com', ... 'subscriber': user_id, + ... 'pre_verified': True, + ... 'pre_confirmed': True, + ... 'pre_approved': True, ... }) content-length: 0 date: ... @@ -729,94 +735,6 @@ Elly is no longer a member of the mailing list. set() -Digest delivery -=============== - -Fred joins the `ant` mailing list but wants MIME digest delivery. -:: - - >>> transaction.abort() - >>> dump_json('http://localhost:9001/3.0/members', { - ... 'list_id': 'ant.example.com', - ... 'subscriber': 'fperson@example.com', - ... 'display_name': 'Fred Person', - ... 'delivery_mode': 'mime_digests', - ... }) - content-length: 0 - date: ... - location: http://localhost:9001/3.0/members/10 - server: ... - status: 201 - - >>> fred = user_manager.get_user('fperson@example.com') - >>> memberships = list(fred.memberships.members) - >>> len(memberships) - 1 - -Fred is getting MIME deliveries. - - >>> memberships[0] - - on ant@example.com as MemberRole.member> - >>> print(memberships[0].delivery_mode) - DeliveryMode.mime_digests - - >>> dump_json('http://localhost:9001/3.0/members/10') - address: http://localhost:9001/3.0/addresses/fperson@example.com - delivery_mode: mime_digests - email: fperson@example.com - http_etag: "..." - list_id: ant.example.com - member_id: 10 - role: member - self_link: http://localhost:9001/3.0/members/10 - user: http://localhost:9001/3.0/users/7 - -Fred wants to change his delivery from MIME digest back to regular delivery. -This can be done by PATCH'ing his member with the `delivery_mode` parameter. -:: - - >>> transaction.abort() - >>> dump_json('http://localhost:9001/3.0/members/10', { - ... 'delivery_mode': 'regular', - ... }, method='PATCH') - content-length: 0 - date: ... - server: ... - status: 204 - - >>> dump_json('http://localhost:9001/3.0/members/10') - address: http://localhost:9001/3.0/addresses/fperson@example.com - delivery_mode: regular - email: fperson@example.com - http_etag: "..." - list_id: ant.example.com - member_id: 10 - role: member - self_link: http://localhost:9001/3.0/members/10 - user: http://localhost:9001/3.0/users/7 - -If a PATCH request changes no attributes, nothing happens. -:: - - >>> dump_json('http://localhost:9001/3.0/members/10', {}, method='PATCH') - content-length: 0 - date: ... - server: ... - status: 204 - - >>> dump_json('http://localhost:9001/3.0/members/10') - address: http://localhost:9001/3.0/addresses/fperson@example.com - delivery_mode: regular - email: fperson@example.com - http_etag: "..." - list_id: ant.example.com - member_id: 10 - role: member - self_link: http://localhost:9001/3.0/members/10 - user: http://localhost:9001/3.0/users/7 - - Changing delivery address ========================= @@ -849,30 +767,30 @@ addresses. >>> dump_json('http://localhost:9001/3.0/members') entry 0: ... - entry 5: + entry 4: address: http://localhost:9001/3.0/addresses/herb@example.com delivery_mode: regular email: herb@example.com http_etag: "..." list_id: ant.example.com - member_id: 11 + member_id: 10 role: member - self_link: http://localhost:9001/3.0/members/11 - user: http://localhost:9001/3.0/users/8 + self_link: http://localhost:9001/3.0/members/10 + user: http://localhost:9001/3.0/users/7 ... - entry 10: + entry 9: address: http://localhost:9001/3.0/addresses/herb@example.com delivery_mode: regular email: herb@example.com http_etag: "..." list_id: bee.example.com - member_id: 12 + member_id: 11 role: member - self_link: http://localhost:9001/3.0/members/12 - user: http://localhost:9001/3.0/users/8 + self_link: http://localhost:9001/3.0/members/11 + user: http://localhost:9001/3.0/users/7 http_etag: "..." start: 0 - total_size: 11 + total_size: 10 In order to change all of his subscriptions to use a different email address, Herb must iterate through his memberships explicitly. @@ -883,13 +801,13 @@ Herb must iterate through his memberships explicitly. >>> memberships = [entry['self_link'] for entry in content['entries']] >>> for url in sorted(memberships): ... print(url) + http://localhost:9001/3.0/members/10 http://localhost:9001/3.0/members/11 - http://localhost:9001/3.0/members/12 For each membership resource, the subscription address is changed by PATCH'ing the `address` attribute. - >>> dump_json('http://localhost:9001/3.0/members/11', { + >>> dump_json('http://localhost:9001/3.0/members/10', { ... 'address': 'hperson@example.com', ... }, method='PATCH') content-length: 0 @@ -897,7 +815,7 @@ the `address` attribute. server: ... status: 204 - >>> dump_json('http://localhost:9001/3.0/members/12', { + >>> dump_json('http://localhost:9001/3.0/members/11', { ... 'address': 'hperson@example.com', ... }, method='PATCH') content-length: 0 @@ -924,20 +842,20 @@ his membership ids have not changed. email: hperson@example.com http_etag: "..." list_id: ant.example.com - member_id: 11 + member_id: 10 role: member - self_link: http://localhost:9001/3.0/members/11 - user: http://localhost:9001/3.0/users/8 + self_link: http://localhost:9001/3.0/members/10 + user: http://localhost:9001/3.0/users/7 entry 1: address: http://localhost:9001/3.0/addresses/hperson@example.com delivery_mode: regular email: hperson@example.com http_etag: "..." list_id: bee.example.com - member_id: 12 + member_id: 11 role: member - self_link: http://localhost:9001/3.0/members/12 - user: http://localhost:9001/3.0/users/8 + self_link: http://localhost:9001/3.0/members/11 + user: http://localhost:9001/3.0/users/7 http_etag: "..." start: 0 total_size: 2 diff --git a/src/mailman/rest/helpers.py b/src/mailman/rest/helpers.py index edd57b76b..c737fcbc7 100644 --- a/src/mailman/rest/helpers.py +++ b/src/mailman/rest/helpers.py @@ -300,6 +300,12 @@ def not_found(response, body=b'404 Not Found'): response.body = body +def accepted(response, body=None): + response.status = falcon.HTTP_202 + if body is not None: + response.body = body + + def bad_request(response, body='400 Bad Request'): response.status = falcon.HTTP_400 if body is not None: diff --git a/src/mailman/rest/members.py b/src/mailman/rest/members.py index a0b5d4f4e..d6a57a673 100644 --- a/src/mailman/rest/members.py +++ b/src/mailman/rest/members.py @@ -25,18 +25,20 @@ __all__ = [ ] -from mailman.app.membership import delete_member -from mailman.interfaces.address import InvalidEmailAddressError -from mailman.interfaces.listmanager import IListManager, NoSuchListError +from mailman.app.membership import add_member, delete_member +from mailman.interfaces.address import IAddress, InvalidEmailAddressError +from mailman.interfaces.listmanager import IListManager from mailman.interfaces.member import ( AlreadySubscribedError, DeliveryMode, MemberRole, MembershipError, - NotAMemberError) -from mailman.interfaces.subscriptions import ISubscriptionService -from mailman.interfaces.user import UnverifiedAddressError + MembershipIsBannedError, NotAMemberError) +from mailman.interfaces.registrar import IRegistrar +from mailman.interfaces.subscriptions import ( + ISubscriptionService, RequestRecord, TokenOwner) +from mailman.interfaces.user import IUser, UnverifiedAddressError from mailman.interfaces.usermanager import IUserManager from mailman.rest.helpers import ( - CollectionMixin, NotFound, bad_request, child, conflict, created, etag, - no_content, not_found, okay, paginate, path_to) + CollectionMixin, NotFound, accepted, bad_request, child, conflict, + created, etag, no_content, not_found, okay, paginate, path_to) from mailman.rest.preferences import Preferences, ReadOnlyPreferences from mailman.rest.validator import ( Validator, enum_validator, subscriber_validator) @@ -202,7 +204,6 @@ class AllMembers(_MemberBase): def on_post(self, request, response): """Create a new member.""" - service = getUtility(ISubscriptionService) try: validator = Validator( list_id=str, @@ -210,22 +211,125 @@ class AllMembers(_MemberBase): display_name=str, delivery_mode=enum_validator(DeliveryMode), role=enum_validator(MemberRole), - _optional=('delivery_mode', 'display_name', 'role')) - member = service.join(**validator(request)) - except AlreadySubscribedError: - conflict(response, b'Member already subscribed') - except NoSuchListError: - bad_request(response, b'No such list') - except InvalidEmailAddressError: - bad_request(response, b'Invalid email address') + pre_verified=bool, + pre_confirmed=bool, + pre_approved=bool, + _optional=('delivery_mode', 'display_name', 'role', + 'pre_verified', 'pre_confirmed', 'pre_approved')) + arguments = validator(request) except ValueError as error: bad_request(response, str(error)) + return + # Dig the mailing list out of the arguments. + list_id = arguments.pop('list_id') + mlist = getUtility(IListManager).get_by_list_id(list_id) + if mlist is None: + bad_request(response, b'No such list') + return + # Figure out what kind of subscriber is being registered. Either it's + # a user via their preferred email address or it's an explicit address. + # If it's a UUID, then it must be associated with an existing user. + subscriber = arguments.pop('subscriber') + user_manager = getUtility(IUserManager) + # We use the display name if there is one. + display_name = arguments.pop('display_name', '') + if isinstance(subscriber, UUID): + user = user_manager.get_user_by_id(subscriber) + if user is None: + bad_request(response, b'No such user') + return + subscriber = user else: - # The member_id are UUIDs. We need to use the integer equivalent - # in the URL. - member_id = member.member_id.int - location = path_to('members/{0}'.format(member_id)) - created(response, location) + # This must be an email address. See if there's an existing + # address object associated with this email. + address = user_manager.get_address(subscriber) + if address is None: + # Create a new address, which of course will not be validated. + address = user_manager.create_address( + subscriber, display_name) + subscriber = address + # What role are we subscribing? Regular members go through the + # subscription policy workflow while owners, moderators, and + # nonmembers go through the legacy API for now. + role = arguments.pop('role', MemberRole.member) + if role is MemberRole.member: + # Get the pre_ flags for the subscription workflow. + pre_verified = arguments.pop('pre_verified', False) + pre_confirmed = arguments.pop('pre_confirmed', False) + pre_approved = arguments.pop('pre_approved', False) + # Now we can run the registration process until either the + # subscriber is subscribed, or the workflow is paused for + # verification, confirmation, or approval. + registrar = IRegistrar(mlist) + try: + token, token_owner, member = registrar.register( + subscriber, + pre_verified=pre_verified, + pre_confirmed=pre_confirmed, + pre_approved=pre_approved) + except AlreadySubscribedError: + conflict(response, b'Member already subscribed') + 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 = path_to('members/{0}'.format(member_id)) + created(response, location) + return + # The member could not be directly subscribed because there are + # some out-of-band steps that need to be completed. E.g. the user + # must confirm their subscription or the moderator must approve + # it. In this case, an HTTP 202 Accepted is exactly the code that + # we should use, and we'll return both the confirmation token and + # the "token owner" so the client knows who should confirm it. + assert token is not None, token + assert token_owner is not TokenOwner.no_one, token_owner + assert member is None, member + content = dict(token=token, token_owner=token_owner.name) + accepted(response, etag(content)) + return + # 2015-04-15 BAW: We're subscribing some role other than a regular + # member. Use the legacy API for this for now. + assert role in (MemberRole.owner, + MemberRole.moderator, + MemberRole.nonmember) + # 2015-04-15 BAW: We're limited to using an email address with this + # legacy API, so if the subscriber is a user, the user must have a + # preferred address, which we'll use, even though it will subscribe + # the explicit address. It is an error if the user does not have a + # preferred address. + # + # If the subscriber is an address object, just use that. + if IUser.providedBy(subscriber): + if subscriber.preferred_address is None: + bad_request(response, b'User without preferred address') + return + email = subscriber.preferred_address.email + else: + assert IAddress.providedBy(subscriber) + email = subscriber.email + delivery_mode = arguments.pop('delivery_mode', DeliveryMode.regular) + record = RequestRecord(email, display_name, delivery_mode) + try: + member = add_member(mlist, record, role) + except InvalidEmailAddressError: + bad_request(response, b'Invalid email address') + return + except MembershipIsBannedError: + bad_request(response, b'Membership is banned') + return + # 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 = path_to('members/{0}'.format(member_id)) + created(response, location) + return def on_get(self, request, response): """/members""" diff --git a/src/mailman/rest/tests/test_membership.py b/src/mailman/rest/tests/test_membership.py index a77dea3b5..4542677b6 100644 --- a/src/mailman/rest/tests/test_membership.py +++ b/src/mailman/rest/tests/test_membership.py @@ -100,6 +100,9 @@ class TestMembership(unittest.TestCase): call_api('http://localhost:9001/3.0/members', { 'list_id': 'test.example.com', 'subscriber': 'anne@example.com', + 'pre_verified': True, + 'pre_confirmed': True, + 'pre_approved': True, }) self.assertEqual(cm.exception.code, 409) self.assertEqual(cm.exception.reason, b'Member already subscribed') @@ -115,6 +118,9 @@ class TestMembership(unittest.TestCase): call_api('http://localhost:9001/3.0/members', { 'list_id': 'test.example.com', 'subscriber': 'ANNE@example.com', + 'pre_verified': True, + 'pre_confirmed': True, + 'pre_approved': True, }) self.assertEqual(cm.exception.code, 409) self.assertEqual(cm.exception.reason, b'Member already subscribed') @@ -130,6 +136,9 @@ class TestMembership(unittest.TestCase): call_api('http://localhost:9001/3.0/members', { 'list_id': 'test.example.com', 'subscriber': 'anne@example.com', + 'pre_verified': True, + 'pre_confirmed': True, + 'pre_approved': True, }) self.assertEqual(cm.exception.code, 409) self.assertEqual(cm.exception.reason, b'Member already subscribed') @@ -151,6 +160,9 @@ class TestMembership(unittest.TestCase): 'list_id': 'test.example.com', 'subscriber': 'hugh/person@example.com', 'display_name': 'Hugh Person', + 'pre_verified': True, + 'pre_confirmed': True, + 'pre_approved': True, }) self.assertEqual(content, None) self.assertEqual(response.status, 201) diff --git a/src/mailman/rest/tests/test_users.py b/src/mailman/rest/tests/test_users.py index af2c9f0d1..ac8d018e8 100644 --- a/src/mailman/rest/tests/test_users.py +++ b/src/mailman/rest/tests/test_users.py @@ -376,7 +376,8 @@ class TestLP1074374(unittest.TestCase): call_api('http://localhost:9001/3.0/members', dict( list_id='test.example.com', subscriber='anne@example.com', - role='member')) + role='member', + pre_verified=True, pre_confirmed=True, pre_approved=True)) # This is not the Anne you're looking for. (IOW, the new Anne is a # different user). content, response = call_api( diff --git a/src/mailman/runners/docs/command.rst b/src/mailman/runners/docs/command.rst index 19d772b00..c97e6454c 100644 --- a/src/mailman/runners/docs/command.rst +++ b/src/mailman/runners/docs/command.rst @@ -147,7 +147,7 @@ address, and the other is the results of his email command. ... print('Subject:', subject) ... if 'confirm' in str(subject): ... token = str(subject).split()[1].strip() - ... new_token = registrar.confirm(token) + ... new_token, token_owner, member = registrar.confirm(token) ... assert new_token is None, 'Confirmation failed' Subject: The results of your email commands Subject: confirm ... diff --git a/src/mailman/runners/tests/test_confirm.py b/src/mailman/runners/tests/test_confirm.py index 71ec5988d..b24d14a52 100644 --- a/src/mailman/runners/tests/test_confirm.py +++ b/src/mailman/runners/tests/test_confirm.py @@ -53,7 +53,8 @@ class TestConfirm(unittest.TestCase): self._mlist = create_list('test@example.com') self._mlist.send_welcome_message = False anne = getUtility(IUserManager).create_address('anne@example.org') - self._token = IRegistrar(self._mlist).register(anne) + registrar = IRegistrar(self._mlist) + self._token, token_owner, member = registrar.register(anne) def test_confirm_with_re_prefix(self): subject = 'Re: confirm {0}'.format(self._token) diff --git a/src/mailman/runners/tests/test_join.py b/src/mailman/runners/tests/test_join.py index c3b52cf0c..1067517e2 100644 --- a/src/mailman/runners/tests/test_join.py +++ b/src/mailman/runners/tests/test_join.py @@ -30,7 +30,7 @@ from mailman.app.lifecycle import create_list from mailman.config import config from mailman.interfaces.member import DeliveryMode from mailman.interfaces.registrar import IRegistrar -from mailman.interfaces.subscriptions import ISubscriptionService +from mailman.interfaces.subscriptions import ISubscriptionService, TokenOwner from mailman.interfaces.usermanager import IUserManager from mailman.runners.command import CommandRunner from mailman.testing.helpers import ( @@ -160,14 +160,16 @@ class TestJoinWithDigests(unittest.TestCase): subject_words = str(messages[1].msg['subject']).split() self.assertEqual(subject_words[0], 'confirm') token = subject_words[1] - status = IRegistrar(self._mlist).confirm(token) - self.assertIsNone(status, 'Confirmation failed') + token, token_owner, rmember = IRegistrar(self._mlist).confirm(token) + self.assertIsNone(token) + self.assertEqual(token_owner, TokenOwner.no_one) # Now, make sure that Anne is a member of the list and is receiving # digest deliveries. members = getUtility(ISubscriptionService).find_members( 'anne@example.org') self.assertEqual(len(members), 1) - return members[0] + self.assertEqual(rmember, members[0]) + return rmember def test_join_with_implicit_no_digests(self): # Test the digest=mime argument to the join command. -- cgit v1.2.3-70-g09d2