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/tests/test_subscriptions.py | 41 ++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) (limited to 'src/mailman/app/tests/test_subscriptions.py') diff --git a/src/mailman/app/tests/test_subscriptions.py b/src/mailman/app/tests/test_subscriptions.py index a4971d793..77f308b9c 100644 --- a/src/mailman/app/tests/test_subscriptions.py +++ b/src/mailman/app/tests/test_subscriptions.py @@ -34,7 +34,7 @@ from mailman.interfaces.member import ( MemberRole, MembershipIsBannedError, MissingPreferredAddressError) from mailman.interfaces.pending import IPendings from mailman.interfaces.subscriptions import ( - MissingUserError, ISubscriptionService) + ISubscriptionService, MissingUserError, TokenOwner) from mailman.testing.helpers import LogFileMark, get_queue_messages from mailman.testing.layers import ConfigLayer from mailman.interfaces.mailinglist import SubscriptionPolicy @@ -45,6 +45,7 @@ from zope.component import getUtility +@unittest.skip('XXX -- no more .join()') class TestJoin(unittest.TestCase): layer = ConfigLayer @@ -88,6 +89,12 @@ class TestSubscriptionWorkflow(unittest.TestCase): self._anne = 'anne@example.com' self._user_manager = getUtility(IUserManager) + def test_no_token_to_start_with(self): + # The workflow starts with no tokens. + workflow = SubscriptionWorkflow(self._mlist) + self.assertIsNone(workflow.token) + self.assertEqual(workflow.token_owner, TokenOwner.no_one) + def test_user_or_address_required(self): # The `subscriber` attribute must be a user or address. workflow = SubscriptionWorkflow(self._mlist) @@ -311,6 +318,9 @@ class TestSubscriptionWorkflow(unittest.TestCase): # Anne is now a member of the mailing list. member = self._mlist.regular_members.get_member(self._anne) self.assertEqual(member.address, anne) + # No further token is needed. + self.assertIsNone(workflow.token) + self.assertEqual(workflow.token_owner, TokenOwner.no_one) def test_do_subscription_pre_approved(self): # An moderation-requiring subscription policy plus a pre-verified and @@ -326,6 +336,9 @@ class TestSubscriptionWorkflow(unittest.TestCase): # Anne is now a member of the mailing list. member = self._mlist.regular_members.get_member(self._anne) self.assertEqual(member.address, anne) + # No further token is needed. + self.assertIsNone(workflow.token) + self.assertEqual(workflow.token_owner, TokenOwner.no_one) def test_do_subscription_pre_approved_pre_confirmed(self): # An moderation-requiring subscription policy plus a pre-verified and @@ -343,6 +356,9 @@ class TestSubscriptionWorkflow(unittest.TestCase): # Anne is now a member of the mailing list. member = self._mlist.regular_members.get_member(self._anne) self.assertEqual(member.address, anne) + # No further token is needed. + self.assertIsNone(workflow.token) + self.assertEqual(workflow.token_owner, TokenOwner.no_one) def test_do_subscription_cleanups(self): # Once the user is subscribed, the token, and its associated pending @@ -362,6 +378,7 @@ class TestSubscriptionWorkflow(unittest.TestCase): self.assertEqual(member.address, anne) # The workflow is done, so it has no token. self.assertIsNone(workflow.token) + self.assertEqual(workflow.token_owner, TokenOwner.no_one) # The pendable associated with the token has been evicted. self.assertIsNone(getUtility(IPendings).confirm(token, expunge=False)) # There is no saved workflow associated with the token. This shows up @@ -384,6 +401,9 @@ class TestSubscriptionWorkflow(unittest.TestCase): # The user is not currently subscribed to the mailing list. member = self._mlist.regular_members.get_member(self._anne) self.assertIsNone(member) + # The token is owned by the moderator. + self.assertIsNotNone(workflow.token) + self.assertEqual(workflow.token_owner, TokenOwner.moderator) # Create a new workflow with the previous workflow's save token, and # restore its state. This models an approved subscription and should # result in the user getting subscribed. @@ -394,6 +414,9 @@ class TestSubscriptionWorkflow(unittest.TestCase): # Now the user is subscribed to the mailing list. member = self._mlist.regular_members.get_member(self._anne) self.assertEqual(member.address, anne) + # No further token is needed. + self.assertIsNone(approved_workflow.token) + self.assertEqual(approved_workflow.token_owner, TokenOwner.no_one) def test_get_moderator_approval_log_on_hold(self): # When the subscription is held for moderator approval, a message is @@ -530,6 +553,10 @@ approval: workflow = SubscriptionWorkflow(self._mlist, anne) list(workflow) self.assertIsNone(self._mlist.regular_members.get_member(self._anne)) + # The token is owned by the subscriber. + self.assertIsNotNone(workflow.token) + self.assertEqual(workflow.token_owner, TokenOwner.subscriber) + # Confirm. confirm_workflow = SubscriptionWorkflow(self._mlist) confirm_workflow.token = workflow.token confirm_workflow.restore() @@ -537,6 +564,9 @@ approval: self.assertIsNotNone(anne.verified_on) self.assertEqual( self._mlist.regular_members.get_member(self._anne).address, anne) + # No further token is needed. + self.assertIsNone(confirm_workflow.token) + self.assertEqual(confirm_workflow.token_owner, TokenOwner.no_one) def test_prevent_confirmation_replay_attacks(self): # Ensure that if the workflow requires two confirmations, e.g. first @@ -553,11 +583,17 @@ approval: # Anne is not yet a member of the mailing list. member = self._mlist.regular_members.get_member(self._anne) self.assertIsNone(member) + # The token is owned by the subscriber. + self.assertIsNotNone(workflow.token) + self.assertEqual(workflow.token_owner, TokenOwner.subscriber) # The old token will not work for moderator approval. moderator_workflow = SubscriptionWorkflow(self._mlist) moderator_workflow.token = token moderator_workflow.restore() list(moderator_workflow) + # The token is owned by the moderator. + self.assertIsNotNone(moderator_workflow.token) + self.assertEqual(moderator_workflow.token_owner, TokenOwner.moderator) # While we wait for the moderator to approve the subscription, note # that there's a new token for the next steps. self.assertNotEqual(token, moderator_workflow.token) @@ -578,3 +614,6 @@ approval: # And now Anne is a member. member = self._mlist.regular_members.get_member(self._anne) self.assertEqual(member.address.email, self._anne) + # No further token is needed. + self.assertIsNone(final_workflow.token) + self.assertEqual(final_workflow.token_owner, TokenOwner.no_one) -- cgit v1.2.3-70-g09d2 From 6c094ce9d81cd5e12ba13c851cbd1018ca3fb935 Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Wed, 15 Apr 2015 18:42:42 -0400 Subject: Give the SubscriptionWorkflow a member attribute which gets set to the member once their subscription is completed. Remove some dead tests. Update an interface. --- src/mailman/app/subscriptions.py | 3 +- src/mailman/app/tests/test_subscriptions.py | 59 ++++++++++------------------- src/mailman/interfaces/registrar.py | 6 +-- 3 files changed, 25 insertions(+), 43 deletions(-) (limited to 'src/mailman/app/tests/test_subscriptions.py') diff --git a/src/mailman/app/subscriptions.py b/src/mailman/app/subscriptions.py index 669a32432..aa9d600bf 100644 --- a/src/mailman/app/subscriptions.py +++ b/src/mailman/app/subscriptions.py @@ -103,6 +103,7 @@ class SubscriptionWorkflow(Workflow): self.address = None self.user = None self.which = None + self.member = None self._set_token(TokenOwner.no_one) # The subscriber must be either an IUser or IAddress. if IAddress.providedBy(subscriber): @@ -285,7 +286,7 @@ class SubscriptionWorkflow(Workflow): def _step_do_subscription(self): # We can immediately subscribe the user to the mailing list. - self.mlist.subscribe(self.subscriber) + self.member = self.mlist.subscribe(self.subscriber) # This workflow is done so throw away any associated state. getUtility(IWorkflowStateManager).restore(self.name, self.token) diff --git a/src/mailman/app/tests/test_subscriptions.py b/src/mailman/app/tests/test_subscriptions.py index 77f308b9c..fa4fec78f 100644 --- a/src/mailman/app/tests/test_subscriptions.py +++ b/src/mailman/app/tests/test_subscriptions.py @@ -44,40 +44,6 @@ from unittest.mock import patch from zope.component import getUtility - -@unittest.skip('XXX -- no more .join()') -class TestJoin(unittest.TestCase): - layer = ConfigLayer - - def setUp(self): - self._mlist = create_list('test@example.com') - self._service = getUtility(ISubscriptionService) - - def test_join_user_with_bogus_id(self): - # When `subscriber` is a missing user id, an exception is raised. - with self.assertRaises(MissingUserError) as cm: - self._service.join('test.example.com', uuid.UUID(int=99)) - self.assertEqual(cm.exception.user_id, uuid.UUID(int=99)) - - def test_join_user_with_invalid_email_address(self): - # When `subscriber` is a string that is not an email address, an - # exception is raised. - with self.assertRaises(InvalidEmailAddressError) as cm: - self._service.join('test.example.com', 'bogus') - self.assertEqual(cm.exception.email, 'bogus') - - def test_missing_preferred_address(self): - # A user cannot join a mailing list if they have no preferred address. - anne = self._service.join( - 'test.example.com', 'anne@example.com', 'Anne Person') - # Try to join Anne as a user with a different role. Her user has no - # preferred address, so this will fail. - self.assertRaises(MissingPreferredAddressError, - self._service.join, - 'test.example.com', anne.user.user_id, - role=MemberRole.owner) - - class TestSubscriptionWorkflow(unittest.TestCase): layer = ConfigLayer @@ -89,11 +55,12 @@ class TestSubscriptionWorkflow(unittest.TestCase): self._anne = 'anne@example.com' self._user_manager = getUtility(IUserManager) - def test_no_token_to_start_with(self): - # The workflow starts with no tokens. + def test_start_state(self): + # The workflow starts with no tokens or member. workflow = SubscriptionWorkflow(self._mlist) self.assertIsNone(workflow.token) self.assertEqual(workflow.token_owner, TokenOwner.no_one) + self.assertIsNone(workflow.member) def test_user_or_address_required(self): # The `subscriber` attribute must be a user or address. @@ -318,6 +285,7 @@ class TestSubscriptionWorkflow(unittest.TestCase): # Anne is now a member of the mailing list. member = self._mlist.regular_members.get_member(self._anne) self.assertEqual(member.address, anne) + self.assertEqual(workflow.member, member) # No further token is needed. self.assertIsNone(workflow.token) self.assertEqual(workflow.token_owner, TokenOwner.no_one) @@ -336,6 +304,7 @@ class TestSubscriptionWorkflow(unittest.TestCase): # Anne is now a member of the mailing list. member = self._mlist.regular_members.get_member(self._anne) self.assertEqual(member.address, anne) + self.assertEqual(workflow.member, member) # No further token is needed. self.assertIsNone(workflow.token) self.assertEqual(workflow.token_owner, TokenOwner.no_one) @@ -356,6 +325,7 @@ class TestSubscriptionWorkflow(unittest.TestCase): # Anne is now a member of the mailing list. member = self._mlist.regular_members.get_member(self._anne) self.assertEqual(member.address, anne) + self.assertEqual(workflow.member, member) # No further token is needed. self.assertIsNone(workflow.token) self.assertEqual(workflow.token_owner, TokenOwner.no_one) @@ -376,6 +346,7 @@ class TestSubscriptionWorkflow(unittest.TestCase): # Anne is now a member of the mailing list. member = self._mlist.regular_members.get_member(self._anne) self.assertEqual(member.address, anne) + self.assertEqual(workflow.member, member) # The workflow is done, so it has no token. self.assertIsNone(workflow.token) self.assertEqual(workflow.token_owner, TokenOwner.no_one) @@ -401,6 +372,7 @@ class TestSubscriptionWorkflow(unittest.TestCase): # The user is not currently subscribed to the mailing list. member = self._mlist.regular_members.get_member(self._anne) self.assertIsNone(member) + self.assertIsNone(workflow.member) # The token is owned by the moderator. self.assertIsNotNone(workflow.token) self.assertEqual(workflow.token_owner, TokenOwner.moderator) @@ -414,6 +386,7 @@ class TestSubscriptionWorkflow(unittest.TestCase): # Now the user is subscribed to the mailing list. member = self._mlist.regular_members.get_member(self._anne) self.assertEqual(member.address, anne) + self.assertEqual(approved_workflow.member, member) # No further token is needed. self.assertIsNone(approved_workflow.token) self.assertEqual(approved_workflow.token_owner, TokenOwner.no_one) @@ -552,7 +525,10 @@ approval: self.assertIsNone(anne.verified_on) workflow = SubscriptionWorkflow(self._mlist, anne) list(workflow) - self.assertIsNone(self._mlist.regular_members.get_member(self._anne)) + # Anne is not yet a member. + member = self._mlist.regular_members.get_member(self._anne) + self.assertIsNone(member) + self.assertIsNone(workflow.member) # The token is owned by the subscriber. self.assertIsNotNone(workflow.token) self.assertEqual(workflow.token_owner, TokenOwner.subscriber) @@ -562,8 +538,10 @@ approval: confirm_workflow.restore() list(confirm_workflow) self.assertIsNotNone(anne.verified_on) - self.assertEqual( - self._mlist.regular_members.get_member(self._anne).address, anne) + # Anne is now a member. + member = self._mlist.regular_members.get_member(self._anne) + self.assertEqual(member.address, anne) + self.assertEqual(confirm_workflow.member, member) # No further token is needed. self.assertIsNone(confirm_workflow.token) self.assertEqual(confirm_workflow.token_owner, TokenOwner.no_one) @@ -583,6 +561,7 @@ approval: # Anne is not yet a member of the mailing list. member = self._mlist.regular_members.get_member(self._anne) self.assertIsNone(member) + self.assertIsNone(workflow.member) # The token is owned by the subscriber. self.assertIsNotNone(workflow.token) self.assertEqual(workflow.token_owner, TokenOwner.subscriber) @@ -606,6 +585,7 @@ approval: # Anne is still not subscribed. member = self._mlist.regular_members.get_member(self._anne) self.assertIsNone(member) + self.assertIsNone(final_workflow.member) # However, if we use the new token, her subscription request will be # approved by the moderator. final_workflow.token = moderator_workflow.token @@ -614,6 +594,7 @@ approval: # And now Anne is a member. member = self._mlist.regular_members.get_member(self._anne) self.assertEqual(member.address.email, self._anne) + self.assertEqual(final_workflow.member, member) # No further token is needed. self.assertIsNone(final_workflow.token) self.assertEqual(final_workflow.token_owner, TokenOwner.no_one) diff --git a/src/mailman/interfaces/registrar.py b/src/mailman/interfaces/registrar.py index e19a3137c..019ce9d2a 100644 --- a/src/mailman/interfaces/registrar.py +++ b/src/mailman/interfaces/registrar.py @@ -52,9 +52,11 @@ class IRegistrar(Interface): This is a higher level interface to user registration, email address confirmation, etc. than the IUserManager. The latter does no validation, syntax checking, or confirmation, while this interface does. + + To use this, adapt an ``IMailingList`` to this interface. """ - def register(mlist, subscriber=None, *, + def register(subscriber=None, *, pre_verified=False, pre_confirmed=False, pre_approved=False): """Subscribe an address or user according to subscription policies. @@ -71,8 +73,6 @@ class IRegistrar(Interface): approve the subscription. Use the ``confirm(token)`` method to resume the workflow. - :param mlist: The mailing list to subscribe to. - :type mlist: `IMailingList` :param subscriber: The user or address to subscribe. :type email: ``IUser`` or ``IAddress`` :return: None if the workflow completes with the member being -- 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/tests/test_subscriptions.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 From 53d4229bff96677a54fd443322c166c2bdcc3054 Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Thu, 16 Apr 2015 15:28:06 -0400 Subject: Checkpointing. --- src/mailman/app/subscriptions.py | 3 +- src/mailman/app/tests/test_subscriptions.py | 16 ++ src/mailman/interfaces/pending.py | 6 + src/mailman/interfaces/registrar.py | 23 +- src/mailman/model/docs/pending.rst | 19 +- src/mailman/model/pending.py | 5 + src/mailman/rest/docs/moderation.rst | 361 ---------------------------- src/mailman/rest/docs/post-moderation.rst | 192 +++++++++++++++ src/mailman/rest/lists.py | 3 +- src/mailman/rest/moderation.py | 252 ------------------- src/mailman/rest/post_moderation.py | 155 ++++++++++++ src/mailman/rest/tests/test_moderation.py | 219 +++++++++++++---- 12 files changed, 577 insertions(+), 677 deletions(-) delete mode 100644 src/mailman/rest/docs/moderation.rst create mode 100644 src/mailman/rest/docs/post-moderation.rst delete mode 100644 src/mailman/rest/moderation.py create mode 100644 src/mailman/rest/post_moderation.py (limited to 'src/mailman/app/tests/test_subscriptions.py') diff --git a/src/mailman/app/subscriptions.py b/src/mailman/app/subscriptions.py index 1c6b96016..46ce549af 100644 --- a/src/mailman/app/subscriptions.py +++ b/src/mailman/app/subscriptions.py @@ -24,7 +24,6 @@ __all__ = [ ] - import uuid import logging @@ -170,6 +169,8 @@ class SubscriptionWorkflow(Workflow): pendable = Pendable( list_id=self.mlist.list_id, address=self.address.email, + hold_date=now().replace(microsecond=0).isoformat(), + token_owner=token_owner.name, ) self.token = getUtility(IPendings).add(pendable, timedelta(days=3650)) diff --git a/src/mailman/app/tests/test_subscriptions.py b/src/mailman/app/tests/test_subscriptions.py index 732266ac3..6b5d88026 100644 --- a/src/mailman/app/tests/test_subscriptions.py +++ b/src/mailman/app/tests/test_subscriptions.py @@ -57,6 +57,22 @@ class TestSubscriptionWorkflow(unittest.TestCase): self.assertEqual(workflow.token_owner, TokenOwner.no_one) self.assertIsNone(workflow.member) + def test_pended_data(self): + # There is a Pendable associated with the held request, and it has + # some data associated with it. + anne = self._user_manager.create_address(self._anne) + workflow = SubscriptionWorkflow(self._mlist, anne) + try: + workflow.run_thru('send_confirmation') + except StopIteration: + pass + self.assertIsNotNone(workflow.token) + pendable = getUtility(IPendings).confirm(workflow.token, expunge=False) + self.assertEqual(pendable['list_id'], 'test.example.com') + self.assertEqual(pendable['address'], 'anne@example.com') + self.assertEqual(pendable['hold_date'], '2005-08-01T07:49:23') + self.assertEqual(pendable['token_owner'], 'subscriber') + def test_user_or_address_required(self): # The `subscriber` attribute must be a user or address. workflow = SubscriptionWorkflow(self._mlist) diff --git a/src/mailman/interfaces/pending.py b/src/mailman/interfaces/pending.py index 222f0dfbf..c921123de 100644 --- a/src/mailman/interfaces/pending.py +++ b/src/mailman/interfaces/pending.py @@ -95,4 +95,10 @@ class IPendings(Interface): def evict(): """Remove all pended items whose lifetime has expired.""" + def __iter__(): + """An iterator over all pendables. + + Each element is a 2-tuple of the form (token, dict). + """ + count = Attribute('The number of pendables in the pendings database.') diff --git a/src/mailman/interfaces/registrar.py b/src/mailman/interfaces/registrar.py index 6f049b9d7..959e0bf6a 100644 --- a/src/mailman/interfaces/registrar.py +++ b/src/mailman/interfaces/registrar.py @@ -75,12 +75,13 @@ class IRegistrar(Interface): :param subscriber: The user or address to subscribe. :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 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) + :return: A 3-tuple is returned where the first element is the token + hash, the second element is a ``TokenOwner`, and the third element + is the subscribed member. If the subscriber got subscribed + immediately, the token will be None and the member will be + an ``IMember``. If the subscription got held, the token + will be a hash and the member will be None. + :rtype: (str-or-None, ``TokenOwner``, ``IMember``-or-None) :raises MembershipIsBannedError: when the address being subscribed appears in the global or list-centric bans. """ @@ -94,9 +95,13 @@ class IRegistrar(Interface): :param token: A token matching a workflow. :type token: string - :return: The new token for any follow up confirmation, or None if the - user was subscribed. - :rtype: str or None + :return: A 3-tuple is returned where the first element is the token + hash, the second element is a ``TokenOwner`, and the third element + is the subscribed member. If the subscriber got subscribed + immediately, the token will be None and the member will be + an ``IMember``. If the subscription is still being held, the token + will be a hash and the member will be None. + :rtype: (str-or-None, ``TokenOwner``, ``IMember``-or-None) :raises LookupError: when no workflow is associated with the token. """ diff --git a/src/mailman/model/docs/pending.rst b/src/mailman/model/docs/pending.rst index 4a14edb2a..03eee3772 100644 --- a/src/mailman/model/docs/pending.rst +++ b/src/mailman/model/docs/pending.rst @@ -43,10 +43,9 @@ There's exactly one entry in the pendings database now. >>> pendingdb.count 1 -There's not much you can do with tokens except to *confirm* them, which -basically means returning the `IPendable` structure (as a dictionary) from the -database that matches the token. If the token isn't in the database, None is -returned. +You can *confirm* the pending, which means returning the `IPendable` structure +(as a dictionary) from the database that matches the token. If the token +isn't in the database, None is returned. >>> pendable = pendingdb.confirm(b'missing') >>> print(pendable) @@ -83,6 +82,18 @@ expunge it. >>> print(pendingdb.confirm(token_1)) None +You can iterate over all the pendings in the database. + + >>> pendables = list(pendingdb) + >>> def sort_key(item): + ... token, pendable = item + ... return pendable['type'] + >>> sorted_pendables = sorted(pendables, key=sort_key) + >>> for token, pendable in sorted_pendables: + ... print(pendable['type']) + three + two + An event can be given a lifetime when it is pended, otherwise it just uses a default lifetime. diff --git a/src/mailman/model/pending.py b/src/mailman/model/pending.py index bbe95d5f0..04b63f2ca 100644 --- a/src/mailman/model/pending.py +++ b/src/mailman/model/pending.py @@ -166,6 +166,11 @@ class Pendings: store.delete(keyvalue) store.delete(pending) + @dbconnection + def __iter__(self, store): + for pending in store.query(Pended).all(): + yield pending.token, self.confirm(pending.token, expunge=False) + @property @dbconnection def count(self, store): diff --git a/src/mailman/rest/docs/moderation.rst b/src/mailman/rest/docs/moderation.rst deleted file mode 100644 index 0a9fd59a8..000000000 --- a/src/mailman/rest/docs/moderation.rst +++ /dev/null @@ -1,361 +0,0 @@ -========== -Moderation -========== - -There are two kinds of moderation tasks a list administrator may need to -perform. Messages which are held for approval can be accepted, rejected, -discarded, or deferred. Subscription (and sometimes unsubscription) requests -can similarly be accepted, discarded, rejected, or deferred. - - -Message moderation -================== - -Viewing the list of held messages ---------------------------------- - -Held messages can be moderated through the REST API. A mailing list starts -with no held messages. - - >>> ant = create_list('ant@example.com') - >>> transaction.commit() - >>> dump_json('http://localhost:9001/3.0/lists/ant@example.com/held') - http_etag: "..." - start: 0 - total_size: 0 - -When a message gets held for moderator approval, it shows up in this list. -:: - - >>> msg = message_from_string("""\ - ... From: anne@example.com - ... To: ant@example.com - ... Subject: Something - ... Message-ID: - ... - ... Something else. - ... """) - - >>> from mailman.app.moderator import hold_message - >>> request_id = hold_message(ant, msg, {'extra': 7}, 'Because') - >>> transaction.commit() - - >>> dump_json('http://localhost:9001/3.0/lists/ant@example.com/held') - entry 0: - extra: 7 - hold_date: 2005-08-01T07:49:23 - http_etag: "..." - message_id: - msg: From: anne@example.com - To: ant@example.com - Subject: Something - Message-ID: - X-Message-ID-Hash: GCSMSG43GYWWVUMO6F7FBUSSPNXQCJ6M - - Something else. - - reason: Because - request_id: 1 - sender: anne@example.com - subject: Something - http_etag: "..." - start: 0 - total_size: 1 - -You can get an individual held message by providing the *request id* for that -message. This will include the text of the message. -:: - - >>> def url(request_id): - ... return ('http://localhost:9001/3.0/lists/' - ... 'ant@example.com/held/{0}'.format(request_id)) - - >>> dump_json(url(request_id)) - extra: 7 - hold_date: 2005-08-01T07:49:23 - http_etag: "..." - message_id: - msg: From: anne@example.com - To: ant@example.com - Subject: Something - Message-ID: - X-Message-ID-Hash: GCSMSG43GYWWVUMO6F7FBUSSPNXQCJ6M - - Something else. - - reason: Because - request_id: 1 - sender: anne@example.com - subject: Something - - -Disposing of held messages --------------------------- - -Individual messages can be moderated through the API by POSTing back to the -held message's resource. The POST data requires an action of one of the -following: - - * discard - throw the message away. - * reject - bounces the message back to the original author. - * defer - defer any action on the message (continue to hold it) - * accept - accept the message for posting. - -Let's see what happens when the above message is deferred. - - >>> dump_json(url(request_id), { - ... 'action': 'defer', - ... }) - content-length: 0 - date: ... - server: ... - status: 204 - -The message is still in the moderation queue. - - >>> dump_json(url(request_id)) - extra: 7 - hold_date: 2005-08-01T07:49:23 - http_etag: "..." - message_id: - msg: From: anne@example.com - To: ant@example.com - Subject: Something - Message-ID: - X-Message-ID-Hash: GCSMSG43GYWWVUMO6F7FBUSSPNXQCJ6M - - Something else. - - reason: Because - request_id: 1 - sender: anne@example.com - subject: Something - -The held message can be discarded. - - >>> dump_json(url(request_id), { - ... 'action': 'discard', - ... }) - content-length: 0 - date: ... - server: ... - status: 204 - -Messages can also be accepted via the REST API. Let's hold a new message for -moderation. -:: - - >>> del msg['message-id'] - >>> msg['Message-ID'] = '' - >>> request_id = hold_message(ant, msg) - >>> transaction.commit() - - >>> results = call_http(url(request_id)) - >>> print(results['message_id']) - - - >>> dump_json(url(request_id), { - ... 'action': 'accept', - ... }) - content-length: 0 - date: ... - server: ... - status: 204 - - >>> from mailman.testing.helpers import get_queue_messages - >>> messages = get_queue_messages('pipeline') - >>> len(messages) - 1 - >>> print(messages[0].msg['message-id']) - - -Messages can be rejected via the REST API too. These bounce the message back -to the original author. -:: - - >>> del msg['message-id'] - >>> msg['Message-ID'] = '' - >>> request_id = hold_message(ant, msg) - >>> transaction.commit() - - >>> results = call_http(url(request_id)) - >>> print(results['message_id']) - - - >>> dump_json(url(request_id), { - ... 'action': 'reject', - ... }) - content-length: 0 - date: ... - server: ... - status: 204 - - >>> from mailman.testing.helpers import get_queue_messages - >>> messages = get_queue_messages('virgin') - >>> len(messages) - 1 - >>> print(messages[0].msg['subject']) - Request to mailing list "Ant" rejected - - -Subscription moderation -======================= - -Viewing subscription requests ------------------------------ - -Subscription and unsubscription requests can be moderated via the REST API as -well. A mailing list starts with no pending subscription or unsubscription -requests. - - >>> ant.admin_immed_notify = False - >>> dump_json('http://localhost:9001/3.0/lists/ant@example.com/requests') - http_etag: "..." - start: 0 - total_size: 0 - -When Anne tries to subscribe to the Ant list, her subscription is held for -moderator approval. -:: - - >>> from mailman.app.moderator import hold_subscription - >>> from mailman.interfaces.member import DeliveryMode - >>> from mailman.interfaces.subscriptions import RequestRecord - - >>> sub_req_id = hold_subscription( - ... ant, RequestRecord('anne@example.com', 'Anne Person', - ... DeliveryMode.regular, 'en')) - >>> transaction.commit() - -The subscription request is available from the mailing list. - - >>> dump_json('http://localhost:9001/3.0/lists/ant@example.com/requests') - entry 0: - delivery_mode: regular - display_name: Anne Person - email: anne@example.com - http_etag: "..." - language: en - request_id: ... - type: subscription - when: 2005-08-01T07:49:23 - http_etag: "..." - start: 0 - total_size: 1 - - -Viewing unsubscription requests -------------------------------- - -Bart tries to leave a mailing list, but he may not be allowed to. - - >>> from mailman.testing.helpers import subscribe - >>> from mailman.app.moderator import hold_unsubscription - >>> bart = subscribe(ant, 'Bart', email='bart@example.com') - >>> unsub_req_id = hold_unsubscription(ant, 'bart@example.com') - >>> transaction.commit() - -The unsubscription request is also available from the mailing list. - - >>> dump_json('http://localhost:9001/3.0/lists/ant@example.com/requests') - entry 0: - delivery_mode: regular - display_name: Anne Person - email: anne@example.com - http_etag: "..." - language: en - request_id: ... - type: subscription - when: 2005-08-01T07:49:23 - entry 1: - email: bart@example.com - http_etag: "..." - request_id: ... - type: unsubscription - http_etag: "..." - start: 0 - total_size: 2 - - -Viewing individual requests ---------------------------- - -You can view an individual membership change request by providing the -request id. Anne's subscription request looks like this. - - >>> dump_json('http://localhost:9001/3.0/lists/ant@example.com/' - ... 'requests/{}'.format(sub_req_id)) - delivery_mode: regular - display_name: Anne Person - email: anne@example.com - http_etag: "..." - language: en - request_id: ... - type: subscription - when: 2005-08-01T07:49:23 - -Bart's unsubscription request looks like this. - - >>> dump_json('http://localhost:9001/3.0/lists/ant@example.com/' - ... 'requests/{}'.format(unsub_req_id)) - email: bart@example.com - http_etag: "..." - request_id: ... - type: unsubscription - - -Disposing of subscription requests ----------------------------------- - -Similar to held messages, you can dispose of held subscription and -unsubscription requests by POSTing back to the request's resource. The POST -data requires an action of one of the following: - - * discard - throw the request away. - * reject - the request is denied and a notification is sent to the email - address requesting the membership change. - * defer - defer any action on this membership change (continue to hold it). - * accept - accept the membership change. - -Anne's subscription request is accepted. - - >>> dump_json('http://localhost:9001/3.0/lists/' - ... 'ant@example.com/requests/{}'.format(sub_req_id), - ... {'action': 'accept'}) - content-length: 0 - date: ... - server: ... - status: 204 - -Anne is now a member of the mailing list. - - >>> transaction.abort() - >>> ant.members.get_member('anne@example.com') - on ant@example.com - as MemberRole.member> - >>> transaction.abort() - -Bart's unsubscription request is discarded. - - >>> dump_json('http://localhost:9001/3.0/lists/' - ... 'ant@example.com/requests/{}'.format(unsub_req_id), - ... {'action': 'discard'}) - content-length: 0 - date: ... - server: ... - status: 204 - -Bart is still a member of the mailing list. - - >>> transaction.abort() - >>> print(ant.members.get_member('bart@example.com')) - on ant@example.com - as MemberRole.member> - >>> transaction.abort() - -There are no more membership change requests. - - >>> dump_json('http://localhost:9001/3.0/lists/ant@example.com/requests') - http_etag: "..." - start: 0 - total_size: 0 diff --git a/src/mailman/rest/docs/post-moderation.rst b/src/mailman/rest/docs/post-moderation.rst new file mode 100644 index 000000000..f082de157 --- /dev/null +++ b/src/mailman/rest/docs/post-moderation.rst @@ -0,0 +1,192 @@ +=============== +Post Moderation +=============== + +Messages which are held for approval can be accepted, rejected, discarded, or +deferred by the list moderators. + + +Viewing the list of held messages +================================= + +Held messages can be moderated through the REST API. A mailing list starts +with no held messages. + + >>> ant = create_list('ant@example.com') + >>> dump_json('http://localhost:9001/3.0/lists/ant@example.com/held') + http_etag: "..." + start: 0 + total_size: 0 + +When a message gets held for moderator approval, it shows up in this list. +:: + + >>> msg = message_from_string("""\ + ... From: anne@example.com + ... To: ant@example.com + ... Subject: Something + ... Message-ID: + ... + ... Something else. + ... """) + + >>> from mailman.app.moderator import hold_message + >>> request_id = hold_message(ant, msg, {'extra': 7}, 'Because') + >>> transaction.commit() + + >>> dump_json('http://localhost:9001/3.0/lists/ant@example.com/held') + entry 0: + extra: 7 + hold_date: 2005-08-01T07:49:23 + http_etag: "..." + message_id: + msg: From: anne@example.com + To: ant@example.com + Subject: Something + Message-ID: + X-Message-ID-Hash: GCSMSG43GYWWVUMO6F7FBUSSPNXQCJ6M + + Something else. + + reason: Because + request_id: 1 + sender: anne@example.com + subject: Something + http_etag: "..." + start: 0 + total_size: 1 + +You can get an individual held message by providing the *request id* for that +message. This will include the text of the message. +:: + + >>> def url(request_id): + ... return ('http://localhost:9001/3.0/lists/' + ... 'ant@example.com/held/{0}'.format(request_id)) + + >>> dump_json(url(request_id)) + extra: 7 + hold_date: 2005-08-01T07:49:23 + http_etag: "..." + message_id: + msg: From: anne@example.com + To: ant@example.com + Subject: Something + Message-ID: + X-Message-ID-Hash: GCSMSG43GYWWVUMO6F7FBUSSPNXQCJ6M + + Something else. + + reason: Because + request_id: 1 + sender: anne@example.com + subject: Something + + +Disposing of held messages +========================== + +Individual messages can be moderated through the API by POSTing back to the +held message's resource. The POST data requires an action of one of the +following: + + * discard - throw the message away. + * reject - bounces the message back to the original author. + * defer - defer any action on the message (continue to hold it) + * accept - accept the message for posting. + +Let's see what happens when the above message is deferred. + + >>> dump_json(url(request_id), { + ... 'action': 'defer', + ... }) + content-length: 0 + date: ... + server: ... + status: 204 + +The message is still in the moderation queue. + + >>> dump_json(url(request_id)) + extra: 7 + hold_date: 2005-08-01T07:49:23 + http_etag: "..." + message_id: + msg: From: anne@example.com + To: ant@example.com + Subject: Something + Message-ID: + X-Message-ID-Hash: GCSMSG43GYWWVUMO6F7FBUSSPNXQCJ6M + + Something else. + + reason: Because + request_id: 1 + sender: anne@example.com + subject: Something + +The held message can be discarded. + + >>> dump_json(url(request_id), { + ... 'action': 'discard', + ... }) + content-length: 0 + date: ... + server: ... + status: 204 + +Messages can also be accepted via the REST API. Let's hold a new message for +moderation. +:: + + >>> del msg['message-id'] + >>> msg['Message-ID'] = '' + >>> request_id = hold_message(ant, msg) + >>> transaction.commit() + + >>> results = call_http(url(request_id)) + >>> print(results['message_id']) + + + >>> dump_json(url(request_id), { + ... 'action': 'accept', + ... }) + content-length: 0 + date: ... + server: ... + status: 204 + + >>> from mailman.testing.helpers import get_queue_messages + >>> messages = get_queue_messages('pipeline') + >>> len(messages) + 1 + >>> print(messages[0].msg['message-id']) + + +Messages can be rejected via the REST API too. These bounce the message back +to the original author. +:: + + >>> del msg['message-id'] + >>> msg['Message-ID'] = '' + >>> request_id = hold_message(ant, msg) + >>> transaction.commit() + + >>> results = call_http(url(request_id)) + >>> print(results['message_id']) + + + >>> dump_json(url(request_id), { + ... 'action': 'reject', + ... }) + content-length: 0 + date: ... + server: ... + status: 204 + + >>> from mailman.testing.helpers import get_queue_messages + >>> messages = get_queue_messages('virgin') + >>> len(messages) + 1 + >>> print(messages[0].msg['subject']) + Request to mailing list "Ant" rejected diff --git a/src/mailman/rest/lists.py b/src/mailman/rest/lists.py index f6bc27917..0607102cb 100644 --- a/src/mailman/rest/lists.py +++ b/src/mailman/rest/lists.py @@ -42,7 +42,8 @@ from mailman.rest.helpers import ( CollectionMixin, GetterSetter, NotFound, bad_request, child, created, etag, no_content, not_found, okay, paginate, path_to) from mailman.rest.members import AMember, MemberCollection -from mailman.rest.moderation import HeldMessages, SubscriptionRequests +from mailman.rest.post_moderation import HeldMessages +from mailman.rest.sub_moderation import SubscriptionRequests from mailman.rest.validator import Validator from operator import attrgetter from zope.component import getUtility diff --git a/src/mailman/rest/moderation.py b/src/mailman/rest/moderation.py deleted file mode 100644 index 984e2d34a..000000000 --- a/src/mailman/rest/moderation.py +++ /dev/null @@ -1,252 +0,0 @@ -# Copyright (C) 2012-2015 by the Free Software Foundation, Inc. -# -# This file is part of GNU Mailman. -# -# GNU Mailman is free software: you can redistribute it and/or modify it under -# the terms of the GNU General Public License as published by the Free -# Software Foundation, either version 3 of the License, or (at your option) -# any later version. -# -# GNU Mailman is distributed in the hope that it will be useful, but WITHOUT -# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or -# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for -# more details. -# -# You should have received a copy of the GNU General Public License along with -# GNU Mailman. If not, see . - -"""REST API for Message moderation.""" - -__all__ = [ - 'HeldMessage', - 'HeldMessages', - 'MembershipChangeRequest', - 'SubscriptionRequests', - ] - - -from mailman.app.moderator import ( - handle_message, handle_subscription, handle_unsubscription) -from mailman.interfaces.action import Action -from mailman.interfaces.messages import IMessageStore -from mailman.interfaces.requests import IListRequests, RequestType -from mailman.rest.helpers import ( - CollectionMixin, bad_request, child, etag, no_content, not_found, okay) -from mailman.rest.validator import Validator, enum_validator -from zope.component import getUtility - - -HELD_MESSAGE_REQUESTS = (RequestType.held_message,) -MEMBERSHIP_CHANGE_REQUESTS = (RequestType.subscription, - RequestType.unsubscription) - - - -class _ModerationBase: - """Common base class.""" - - def _make_resource(self, request_id, expected_request_types): - requests = IListRequests(self._mlist) - results = requests.get_request(request_id) - if results is None: - return None - key, data = results - resource = dict(key=key, request_id=request_id) - # Flatten the IRequest payload into the JSON representation. - resource.update(data) - # Check for a matching request type, and insert the type name into the - # resource. - request_type = RequestType[resource.pop('_request_type')] - if request_type not in expected_request_types: - return None - resource['type'] = request_type.name - # This key isn't what you think it is. Usually, it's the Pendable - # record's row id, which isn't helpful at all. If it's not there, - # that's fine too. - resource.pop('id', None) - return resource - - - -class _HeldMessageBase(_ModerationBase): - """Held messages are a little different.""" - - def _make_resource(self, request_id): - resource = super(_HeldMessageBase, self)._make_resource( - request_id, HELD_MESSAGE_REQUESTS) - if resource is None: - return None - # Grab the message and insert its text representation into the - # resource. XXX See LP: #967954 - key = resource.pop('key') - msg = getUtility(IMessageStore).get_message_by_id(key) - resource['msg'] = msg.as_string() - # Some of the _mod_* keys we want to rename and place into the JSON - # resource. Others we can drop. Since we're mutating the dictionary, - # we need to make a copy of the keys. When you port this to Python 3, - # you'll need to list()-ify the .keys() dictionary view. - for key in list(resource): - if key in ('_mod_subject', '_mod_hold_date', '_mod_reason', - '_mod_sender', '_mod_message_id'): - resource[key[5:]] = resource.pop(key) - elif key.startswith('_mod_'): - del resource[key] - # Also, held message resources will always be this type, so ignore - # this key value. - del resource['type'] - return resource - - -class HeldMessage(_HeldMessageBase): - """Resource for moderating a held message.""" - - def __init__(self, mlist, request_id): - self._mlist = mlist - self._request_id = request_id - - def on_get(self, request, response): - try: - request_id = int(self._request_id) - except ValueError: - bad_request(response) - return - resource = self._make_resource(request_id) - if resource is None: - not_found(response) - else: - okay(response, etag(resource)) - - def on_post(self, request, response): - try: - validator = Validator(action=enum_validator(Action)) - arguments = validator(request) - except ValueError as error: - bad_request(response, str(error)) - return - requests = IListRequests(self._mlist) - try: - request_id = int(self._request_id) - except ValueError: - bad_request(response) - return - results = requests.get_request(request_id, RequestType.held_message) - if results is None: - not_found(response) - else: - handle_message(self._mlist, request_id, **arguments) - no_content(response) - - - -class HeldMessages(_HeldMessageBase, CollectionMixin): - """Resource for messages held for moderation.""" - - def __init__(self, mlist): - self._mlist = mlist - self._requests = None - - def _resource_as_dict(self, request): - """See `CollectionMixin`.""" - return self._make_resource(request.id) - - def _get_collection(self, request): - requests = IListRequests(self._mlist) - self._requests = requests - return list(requests.of_type(RequestType.held_message)) - - def on_get(self, request, response): - """/lists/listname/held""" - resource = self._make_collection(request) - okay(response, etag(resource)) - - @child(r'^(?P[^/]+)') - def message(self, request, segments, **kw): - return HeldMessage(self._mlist, kw['id']) - - - -class MembershipChangeRequest(_ModerationBase): - """Resource for moderating a membership change.""" - - def __init__(self, mlist, request_id): - self._mlist = mlist - self._request_id = request_id - - def on_get(self, request, response): - try: - request_id = int(self._request_id) - except ValueError: - bad_request(response) - return - resource = self._make_resource(request_id, MEMBERSHIP_CHANGE_REQUESTS) - if resource is None: - not_found(response) - else: - # Remove unnecessary keys. - del resource['key'] - okay(response, etag(resource)) - - def on_post(self, request, response): - try: - validator = Validator(action=enum_validator(Action)) - arguments = validator(request) - except ValueError as error: - bad_request(response, str(error)) - return - requests = IListRequests(self._mlist) - try: - request_id = int(self._request_id) - except ValueError: - bad_request(response) - return - results = requests.get_request(request_id) - if results is None: - not_found(response) - return - key, data = results - try: - request_type = RequestType[data['_request_type']] - except ValueError: - bad_request(response) - return - if request_type is RequestType.subscription: - handle_subscription(self._mlist, request_id, **arguments) - elif request_type is RequestType.unsubscription: - handle_unsubscription(self._mlist, request_id, **arguments) - else: - bad_request(response) - return - no_content(response) - - -class SubscriptionRequests(_ModerationBase, CollectionMixin): - """Resource for membership change requests.""" - - def __init__(self, mlist): - self._mlist = mlist - self._requests = None - - def _resource_as_dict(self, request): - """See `CollectionMixin`.""" - resource = self._make_resource(request.id, MEMBERSHIP_CHANGE_REQUESTS) - # Remove unnecessary keys. - del resource['key'] - return resource - - def _get_collection(self, request): - requests = IListRequests(self._mlist) - self._requests = requests - items = [] - for request_type in MEMBERSHIP_CHANGE_REQUESTS: - for request in requests.of_type(request_type): - items.append(request) - return items - - def on_get(self, request, response): - """/lists/listname/requests""" - resource = self._make_collection(request) - okay(response, etag(resource)) - - @child(r'^(?P[^/]+)') - def subscription(self, request, segments, **kw): - return MembershipChangeRequest(self._mlist, kw['id']) diff --git a/src/mailman/rest/post_moderation.py b/src/mailman/rest/post_moderation.py new file mode 100644 index 000000000..6156fa39f --- /dev/null +++ b/src/mailman/rest/post_moderation.py @@ -0,0 +1,155 @@ +# Copyright (C) 2012-2015 by the Free Software Foundation, Inc. +# +# This file is part of GNU Mailman. +# +# GNU Mailman is free software: you can redistribute it and/or modify it under +# the terms of the GNU General Public License as published by the Free +# Software Foundation, either version 3 of the License, or (at your option) +# any later version. +# +# GNU Mailman is distributed in the hope that it will be useful, but WITHOUT +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +# more details. +# +# You should have received a copy of the GNU General Public License along with +# GNU Mailman. If not, see . + +"""REST API for held message moderation.""" + +__all__ = [ + 'HeldMessage', + 'HeldMessages', + ] + + +from mailman.app.moderator import handle_message +from mailman.interfaces.action import Action +from mailman.interfaces.messages import IMessageStore +from mailman.interfaces.requests import IListRequests, RequestType +from mailman.rest.helpers import ( + CollectionMixin, bad_request, child, etag, no_content, not_found, okay) +from mailman.rest.validator import Validator, enum_validator +from zope.component import getUtility + + + +class _ModerationBase: + """Common base class.""" + + def _make_resource(self, request_id): + requests = IListRequests(self._mlist) + results = requests.get_request(request_id) + if results is None: + return None + key, data = results + resource = dict(key=key, request_id=request_id) + # Flatten the IRequest payload into the JSON representation. + resource.update(data) + # Check for a matching request type, and insert the type name into the + # resource. + request_type = RequestType[resource.pop('_request_type')] + if request_type is not RequestType.held_message: + return None + resource['type'] = RequestType.held_message.name + # This key isn't what you think it is. Usually, it's the Pendable + # record's row id, which isn't helpful at all. If it's not there, + # that's fine too. + resource.pop('id', None) + return resource + + + +class _HeldMessageBase(_ModerationBase): + """Held messages are a little different.""" + + def _make_resource(self, request_id): + resource = super(_HeldMessageBase, self)._make_resource(request_id) + if resource is None: + return None + # Grab the message and insert its text representation into the + # resource. XXX See LP: #967954 + key = resource.pop('key') + msg = getUtility(IMessageStore).get_message_by_id(key) + resource['msg'] = msg.as_string() + # Some of the _mod_* keys we want to rename and place into the JSON + # resource. Others we can drop. Since we're mutating the dictionary, + # we need to make a copy of the keys. When you port this to Python 3, + # you'll need to list()-ify the .keys() dictionary view. + for key in list(resource): + if key in ('_mod_subject', '_mod_hold_date', '_mod_reason', + '_mod_sender', '_mod_message_id'): + resource[key[5:]] = resource.pop(key) + elif key.startswith('_mod_'): + del resource[key] + # Also, held message resources will always be this type, so ignore + # this key value. + del resource['type'] + return resource + + +class HeldMessage(_HeldMessageBase): + """Resource for moderating a held message.""" + + def __init__(self, mlist, request_id): + self._mlist = mlist + self._request_id = request_id + + def on_get(self, request, response): + try: + request_id = int(self._request_id) + except ValueError: + bad_request(response) + return + resource = self._make_resource(request_id) + if resource is None: + not_found(response) + else: + okay(response, etag(resource)) + + def on_post(self, request, response): + try: + validator = Validator(action=enum_validator(Action)) + arguments = validator(request) + except ValueError as error: + bad_request(response, str(error)) + return + requests = IListRequests(self._mlist) + try: + request_id = int(self._request_id) + except ValueError: + bad_request(response) + return + results = requests.get_request(request_id, RequestType.held_message) + if results is None: + not_found(response) + else: + handle_message(self._mlist, request_id, **arguments) + no_content(response) + + + +class HeldMessages(_HeldMessageBase, CollectionMixin): + """Resource for messages held for moderation.""" + + def __init__(self, mlist): + self._mlist = mlist + self._requests = None + + def _resource_as_dict(self, request): + """See `CollectionMixin`.""" + return self._make_resource(request.id) + + def _get_collection(self, request): + requests = IListRequests(self._mlist) + self._requests = requests + return list(requests.of_type(RequestType.held_message)) + + def on_get(self, request, response): + """/lists/listname/held""" + resource = self._make_collection(request) + okay(response, etag(resource)) + + @child(r'^(?P[^/]+)') + def message(self, request, segments, **kw): + return HeldMessage(self._mlist, kw['id']) diff --git a/src/mailman/rest/tests/test_moderation.py b/src/mailman/rest/tests/test_moderation.py index c77ae2aca..ab7383251 100644 --- a/src/mailman/rest/tests/test_moderation.py +++ b/src/mailman/rest/tests/test_moderation.py @@ -18,26 +18,28 @@ """REST moderation tests.""" __all__ = [ - 'TestModeration', + 'TestPostModeration', + 'TestSubscriptionModeration', ] import unittest from mailman.app.lifecycle import create_list -from mailman.app.moderator import hold_message, hold_subscription -from mailman.config import config +from mailman.app.moderator import hold_message from mailman.database.transaction import transaction -from mailman.interfaces.member import DeliveryMode -from mailman.interfaces.subscriptions import RequestRecord +from mailman.interfaces.registrar import IRegistrar +from mailman.interfaces.usermanager import IUserManager from mailman.testing.helpers import ( - call_api, specialized_message_from_string as mfs) + call_api, get_queue_messages, specialized_message_from_string as mfs) from mailman.testing.layers import RESTLayer +from mailman.utilities.datetime import now from urllib.error import HTTPError +from zope.component import getUtility -class TestModeration(unittest.TestCase): +class TestPostModeration(unittest.TestCase): layer = RESTLayer def setUp(self): @@ -71,24 +73,6 @@ Something else. call_api('http://localhost:9001/3.0/lists/ant@example.com/held/99') self.assertEqual(cm.exception.code, 404) - def test_subscription_request_as_held_message(self): - # Provide the request id of a subscription request using the held - # message API returns a not-found even though the request id is - # in the database. - held_id = hold_message(self._mlist, self._msg) - subscribe_id = hold_subscription( - self._mlist, - RequestRecord('bperson@example.net', 'Bart Person', - DeliveryMode.regular, 'en')) - config.db.store.commit() - url = 'http://localhost:9001/3.0/lists/ant@example.com/held/{0}' - with self.assertRaises(HTTPError) as cm: - call_api(url.format(subscribe_id)) - self.assertEqual(cm.exception.code, 404) - # But using the held_id returns a valid response. - response, content = call_api(url.format(held_id)) - self.assertEqual(response['message_id'], '') - def test_bad_held_message_action(self): # POSTing to a held message with a bad action. held_id = hold_message(self._mlist, self._msg) @@ -99,43 +83,180 @@ Something else. self.assertEqual(cm.exception.msg, b'Cannot convert parameters: action') - def test_bad_subscription_request_id(self): - # Bad request when request_id is not an integer. + def test_discard(self): + # Discarding a message removes it from the moderation queue. + with transaction(): + held_id = hold_message(self._mlist, self._msg) + url = 'http://localhost:9001/3.0/lists/ant@example.com/held/{}'.format( + held_id) + content, response = call_api(url, dict(action='discard')) + self.assertEqual(response.status, 204) + # Now it's gone. with self.assertRaises(HTTPError) as cm: - call_api('http://localhost:9001/3.0/lists/ant@example.com/' - 'requests/bogus') - self.assertEqual(cm.exception.code, 400) + call_api(url, dict(action='discard')) + self.assertEqual(cm.exception.code, 404) - def test_missing_subscription_request_id(self): - # Bad request when the request_id is not in the database. + + +class TestSubscriptionModeration(unittest.TestCase): + layer = RESTLayer + + def setUp(self): + with transaction(): + self._mlist = create_list('ant@example.com') + self._registrar = IRegistrar(self._mlist) + manager = getUtility(IUserManager) + self._anne = manager.create_address( + 'anne@example.com', 'Anne Person') + self._bart = manager.make_user( + 'bart@example.com', 'Bart Person') + preferred = list(self._bart.addresses)[0] + preferred.verified_on = now() + self._bart.preferred_address = preferred + + def test_no_such_list(self): + # Try to get the requests of a nonexistent list. + with self.assertRaises(HTTPError) as cm: + call_api('http://localhost:9001/3.0/lists/bee@example.com/' + 'requests') + self.assertEqual(cm.exception.code, 404) + + def test_no_such_subscription_token(self): + # Bad request when the token is not in the database. with self.assertRaises(HTTPError) as cm: call_api('http://localhost:9001/3.0/lists/ant@example.com/' - 'requests/99') + 'requests/missing') self.assertEqual(cm.exception.code, 404) def test_bad_subscription_action(self): # POSTing to a held message with a bad action. - held_id = hold_subscription( - self._mlist, - RequestRecord('cperson@example.net', 'Cris Person', - DeliveryMode.regular, 'en')) - config.db.store.commit() - url = 'http://localhost:9001/3.0/lists/ant@example.com/requests/{0}' + token, token_owner, member = self._registrar.register(self._anne) + # Anne's subscription request got held. + self.assertIsNone(member) + # Let's try to handle her request, but with a bogus action. + url = 'http://localhost:9001/3.0/lists/ant@example.com/requests/{}' with self.assertRaises(HTTPError) as cm: - call_api(url.format(held_id), {'action': 'bogus'}) + call_api(url.format(token), dict( + action='bogus', + )) self.assertEqual(cm.exception.code, 400) self.assertEqual(cm.exception.msg, b'Cannot convert parameters: action') + def test_list_held_requests(self): + # We can view all the held requests. + token_1, token_owner, member = self._registrar.register(self._anne) + # Anne's subscription request got held. + self.assertIsNone(member) + token_2, token_owner, member = self._registrar.register(self._bart) + content, response = call_api( + 'http://localhost:9001/3.0/lists/ant@example.com/requests') + self.assertEqual(response.status, 200) + self.assertEqual(content['total_size'], 2) + import pdb; pdb.set_trace() + + def test_accept(self): + # POST to the request to accept it. + token, token_owner, member = self._registrar.register(self._anne) + # Anne's subscription request got held. + self.assertIsNone(member) + url = 'http://localhost:9001/3.0/lists/ant@example.com/requests/{}' + with self.assertRaises(HTTPError) as cm: + call_api(url.format(token), dict( + action='accept', + )) + self.assertEqual(cm.exception.code, 204) + # Anne is a member. + self.assertEqual( + self._mlist.get_members('anne@example.com').address, + self._anne) + # The request URL no longer exists. + with self.assertRaises(HTTPError) as cm: + call_api(url.format(token), dict( + action='accept', + )) + self.assertEqual(cm.exception.code, 404) + def test_discard(self): - # Discarding a message removes it from the moderation queue. - with transaction(): - held_id = hold_message(self._mlist, self._msg) - url = 'http://localhost:9001/3.0/lists/ant@example.com/held/{}'.format( - held_id) - content, response = call_api(url, dict(action='discard')) - self.assertEqual(response.status, 204) - # Now it's gone. + # POST to the request to discard it. + token, token_owner, member = self._registrar.register(self._anne) + # Anne's subscription request got held. + self.assertIsNone(member) + url = 'http://localhost:9001/3.0/lists/ant@example.com/requests/{}' with self.assertRaises(HTTPError) as cm: - call_api(url, dict(action='discard')) + call_api(url.format(token), dict( + action='discard', + )) + self.assertEqual(cm.exception.code, 204) + # Anne is not a member. + self.assertIsNone(self._mlist.get_members('anne@example.com')) + # The request URL no longer exists. + with self.assertRaises(HTTPError) as cm: + call_api(url.format(token), dict( + action='discard', + )) + self.assertEqual(cm.exception.code, 404) + + def test_defer(self): + # Defer the decision for some other moderator. + token, token_owner, member = self._registrar.register(self._anne) + # Anne's subscription request got held. + self.assertIsNone(member) + url = 'http://localhost:9001/3.0/lists/ant@example.com/requests/{}' + with self.assertRaises(HTTPError) as cm: + call_api(url.format(token), dict( + action='defer', + )) + self.assertEqual(cm.exception.code, 204) + # Anne is not a member. + self.assertIsNone(self._mlist.get_members('anne@example.com')) + # The request URL still exists. + with self.assertRaises(HTTPError) as cm: + call_api(url.format(token), dict( + action='defer', + )) + self.assertEqual(cm.exception.code, 204) + # And now we can accept it. + with self.assertRaises(HTTPError) as cm: + call_api(url.format(token), dict( + action='accept', + )) + self.assertEqual(cm.exception.code, 204) + # Anne is a member. + self.assertEqual( + self._mlist.get_members('anne@example.com').address, + self._anne) + # The request URL no longer exists. + with self.assertRaises(HTTPError) as cm: + call_api(url.format(token), dict( + action='accept', + )) + self.assertEqual(cm.exception.code, 404) + + def test_reject(self): + # POST to the request to reject it. This leaves a bounce message in + # the virgin queue. + token, token_owner, member = self._registrar.register(self._anne) + # Anne's subscription request got held. + self.assertIsNone(member) + # There are currently no messages in the virgin queue. + items = get_queue_messages('virgin') + self.assertEqual(len(items), 0) + url = 'http://localhost:9001/3.0/lists/ant@example.com/requests/{}' + with self.assertRaises(HTTPError) as cm: + call_api(url.format(token), dict( + action='reject', + )) + self.assertEqual(cm.exception.code, 204) + # Anne is not a member. + self.assertIsNone(self._mlist.get_members('anne@example.com')) + # The request URL no longer exists. + with self.assertRaises(HTTPError) as cm: + call_api(url.format(token), dict( + action='reject', + )) self.assertEqual(cm.exception.code, 404) + # And the rejection message is now in the virgin queue. + items = get_queue_messages('virgin') + self.assertEqual(len(items), 1) + self.assertEqual(str(items[0].msg), '') -- cgit v1.2.3-70-g09d2 From 3857294c3dc8fdf294f23179926f062c4e0a6c90 Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Fri, 17 Apr 2015 11:30:23 -0400 Subject: Check pointing new subscription moderation REST API. --- src/mailman/app/subscriptions.py | 5 +- src/mailman/app/tests/test_subscriptions.py | 5 +- src/mailman/rest/docs/post-moderation.rst | 1 + src/mailman/rest/docs/sub-moderation.rst | 8 +- src/mailman/rest/sub_moderation.py | 89 ++++++++++--------- src/mailman/rest/tests/test_moderation.py | 131 ++++++++++++++++++---------- 6 files changed, 147 insertions(+), 92 deletions(-) (limited to 'src/mailman/app/tests/test_subscriptions.py') diff --git a/src/mailman/app/subscriptions.py b/src/mailman/app/subscriptions.py index 46ce549af..8794fb78a 100644 --- a/src/mailman/app/subscriptions.py +++ b/src/mailman/app/subscriptions.py @@ -168,8 +168,9 @@ class SubscriptionWorkflow(Workflow): return pendable = Pendable( list_id=self.mlist.list_id, - address=self.address.email, - hold_date=now().replace(microsecond=0).isoformat(), + email=self.address.email, + display_name=self.address.display_name, + when=now().replace(microsecond=0).isoformat(), token_owner=token_owner.name, ) self.token = getUtility(IPendings).add(pendable, timedelta(days=3650)) diff --git a/src/mailman/app/tests/test_subscriptions.py b/src/mailman/app/tests/test_subscriptions.py index 6b5d88026..2d5a3733b 100644 --- a/src/mailman/app/tests/test_subscriptions.py +++ b/src/mailman/app/tests/test_subscriptions.py @@ -69,8 +69,9 @@ class TestSubscriptionWorkflow(unittest.TestCase): self.assertIsNotNone(workflow.token) pendable = getUtility(IPendings).confirm(workflow.token, expunge=False) self.assertEqual(pendable['list_id'], 'test.example.com') - self.assertEqual(pendable['address'], 'anne@example.com') - self.assertEqual(pendable['hold_date'], '2005-08-01T07:49:23') + self.assertEqual(pendable['email'], 'anne@example.com') + self.assertEqual(pendable['display_name'], '') + self.assertEqual(pendable['when'], '2005-08-01T07:49:23') self.assertEqual(pendable['token_owner'], 'subscriber') def test_user_or_address_required(self): diff --git a/src/mailman/rest/docs/post-moderation.rst b/src/mailman/rest/docs/post-moderation.rst index f082de157..6dd96e71a 100644 --- a/src/mailman/rest/docs/post-moderation.rst +++ b/src/mailman/rest/docs/post-moderation.rst @@ -13,6 +13,7 @@ Held messages can be moderated through the REST API. A mailing list starts with no held messages. >>> ant = create_list('ant@example.com') + >>> transaction.commit() >>> dump_json('http://localhost:9001/3.0/lists/ant@example.com/held') http_etag: "..." start: 0 diff --git a/src/mailman/rest/docs/sub-moderation.rst b/src/mailman/rest/docs/sub-moderation.rst index a658ac410..0468d75b5 100644 --- a/src/mailman/rest/docs/sub-moderation.rst +++ b/src/mailman/rest/docs/sub-moderation.rst @@ -42,15 +42,15 @@ The message is being held for moderator approval. The subscription request can be viewed in the REST API. + >>> transaction.commit() >>> dump_json('http://localhost:9001/3.0/lists/ant@example.com/requests') entry 0: - delivery_mode: regular display_name: Anne Person email: anne@example.com http_etag: "..." - language: en - request_id: ... - type: subscription + list_id: ant.example.com + token: ... + token_owner: moderator when: 2005-08-01T07:49:23 http_etag: "..." start: 0 diff --git a/src/mailman/rest/sub_moderation.py b/src/mailman/rest/sub_moderation.py index 7bc6554a6..12bd8ab26 100644 --- a/src/mailman/rest/sub_moderation.py +++ b/src/mailman/rest/sub_moderation.py @@ -38,8 +38,11 @@ class _ModerationBase: def __init__(self): self._pendings = getUtility(IPendings) - def _make_resource(self, token): + def _resource_as_dict(self, token): pendable = self._pendings.confirm(token, expunge=False) + if pendable is None: + # This token isn't in the database. + raise LookupError resource = dict(token=token) resource.update(pendable) return resource @@ -50,28 +53,20 @@ class IndividualRequest(_ModerationBase): """Resource for moderating a membership change.""" def __init__(self, mlist, token): + super().__init__() self._mlist = mlist self._registrar = IRegistrar(self._mlist) self._token = token def on_get(self, request, response): + # Get the pended record associated with this token, if it exists in + # the pending table. try: - token, token_owner, member = self._registrar.confirm(self._token) + resource = self._resource_as_dict(self._token) except LookupError: not_found(response) return - try: - request_id = int(self._request_id) - except ValueError: - bad_request(response) - return - resource = self._make_resource(request_id, MEMBERSHIP_CHANGE_REQUESTS) - if resource is None: - not_found(response) - else: - # Remove unnecessary keys. - del resource['key'] - okay(response, etag(resource)) + okay(response, etag(resource)) def on_post(self, request, response): try: @@ -80,32 +75,34 @@ class IndividualRequest(_ModerationBase): except ValueError as error: bad_request(response, str(error)) return - requests = IListRequests(self._mlist) - try: - request_id = int(self._request_id) - except ValueError: - bad_request(response) - return - results = requests.get_request(request_id) - if results is None: - not_found(response) - return - key, data = results - try: - request_type = RequestType[data['_request_type']] - except ValueError: - bad_request(response) - return - if request_type is RequestType.subscription: - handle_subscription(self._mlist, request_id, **arguments) - elif request_type is RequestType.unsubscription: - handle_unsubscription(self._mlist, request_id, **arguments) - else: - bad_request(response) - return - no_content(response) + action = arguments['action'] + if action is Action.defer: + # At least see if the token is in the database. + pendable = self._pendings.confirm(self._token, expunge=False) + if pendable is None: + not_found(response) + else: + no_content(response) + elif action is Action.accept: + try: + self._registrar.confirm(self._token) + except LookupError: + not_found(response) + else: + no_content(response) + elif action is Action.discard: + # At least see if the token is in the database. + pendable = self._pendings.confirm(self._token, expunge=True) + if pendable is None: + not_found(response) + else: + no_content(response) + elif action is Action.reject: + # XXX + no_content(response) + class SubscriptionRequests(_ModerationBase, CollectionMixin): """Resource for membership change requests.""" @@ -116,8 +113,20 @@ class SubscriptionRequests(_ModerationBase, CollectionMixin): def _get_collection(self, request): # There's currently no better way to query the pendings database for # all the entries that are associated with subscription holds on this - # mailing list. Brute force for now. - return [token for token, pendable in getUtility(IPendings)] + # mailing list. Brute force iterating over all the pendables. + collection = [] + for token, pendable in getUtility(IPendings): + if 'token_owner' not in pendable: + # This isn't a subscription hold. + continue + list_id = pendable.get('list_id') + if list_id != self._mlist.list_id: + # Either there isn't a list_id field, in which case it can't + # be a subscription hold, or this is a hold for some other + # mailing list. + continue + collection.append(token) + return collection def on_get(self, request, response): """/lists/listname/requests""" diff --git a/src/mailman/rest/tests/test_moderation.py b/src/mailman/rest/tests/test_moderation.py index ab7383251..cc1f67f7e 100644 --- a/src/mailman/rest/tests/test_moderation.py +++ b/src/mailman/rest/tests/test_moderation.py @@ -145,30 +145,51 @@ class TestSubscriptionModeration(unittest.TestCase): def test_list_held_requests(self): # We can view all the held requests. - token_1, token_owner, member = self._registrar.register(self._anne) - # Anne's subscription request got held. - self.assertIsNone(member) - token_2, token_owner, member = self._registrar.register(self._bart) + with transaction(): + token_1, token_owner, member = self._registrar.register(self._anne) + # Anne's subscription request got held. + self.assertIsNotNone(token_1) + self.assertIsNone(member) + token_2, token_owner, member = self._registrar.register(self._bart) + self.assertIsNotNone(token_2) + self.assertIsNone(member) content, response = call_api( 'http://localhost:9001/3.0/lists/ant@example.com/requests') self.assertEqual(response.status, 200) self.assertEqual(content['total_size'], 2) - import pdb; pdb.set_trace() + tokens = set(json['token'] for json in content['entries']) + self.assertEqual(tokens, {token_1, token_2}) + emails = set(json['email'] for json in content['entries']) + self.assertEqual(emails, {'anne@example.com', 'bart@example.com'}) + + def test_individual_request(self): + # We can view an individual request. + with transaction(): + token, token_owner, member = self._registrar.register(self._anne) + # Anne's subscription request got held. + self.assertIsNotNone(token) + self.assertIsNone(member) + url = 'http://localhost:9001/3.0/lists/ant@example.com/requests/{}' + content, response = call_api(url.format(token)) + self.assertEqual(response.status, 200) + self.assertEqual(content['token'], token) + self.assertEqual(content['token_owner'], token_owner.name) + self.assertEqual(content['email'], 'anne@example.com') def test_accept(self): # POST to the request to accept it. - token, token_owner, member = self._registrar.register(self._anne) + with transaction(): + token, token_owner, member = self._registrar.register(self._anne) # Anne's subscription request got held. self.assertIsNone(member) url = 'http://localhost:9001/3.0/lists/ant@example.com/requests/{}' - with self.assertRaises(HTTPError) as cm: - call_api(url.format(token), dict( - action='accept', - )) - self.assertEqual(cm.exception.code, 204) + content, response = call_api(url.format(token), dict( + action='accept', + )) + self.assertEqual(response.status, 204) # Anne is a member. self.assertEqual( - self._mlist.get_members('anne@example.com').address, + self._mlist.members.get_member('anne@example.com').address, self._anne) # The request URL no longer exists. with self.assertRaises(HTTPError) as cm: @@ -177,19 +198,27 @@ class TestSubscriptionModeration(unittest.TestCase): )) self.assertEqual(cm.exception.code, 404) + def test_accept_bad_token(self): + # Try to accept a request with a bogus token. + with self.assertRaises(HTTPError) as cm: + call_api('http://localhost:9001/3.0/lists/ant@example.com' + '/requests/bogus', + dict(action='accept')) + self.assertEqual(cm.exception.code, 404) + def test_discard(self): # POST to the request to discard it. - token, token_owner, member = self._registrar.register(self._anne) + with transaction(): + token, token_owner, member = self._registrar.register(self._anne) # Anne's subscription request got held. self.assertIsNone(member) url = 'http://localhost:9001/3.0/lists/ant@example.com/requests/{}' - with self.assertRaises(HTTPError) as cm: - call_api(url.format(token), dict( - action='discard', - )) - self.assertEqual(cm.exception.code, 204) + content, response = call_api(url.format(token), dict( + action='discard', + )) + self.assertEqual(response.status, 204) # Anne is not a member. - self.assertIsNone(self._mlist.get_members('anne@example.com')) + self.assertIsNone(self._mlist.members.get_member('anne@example.com')) # The request URL no longer exists. with self.assertRaises(HTTPError) as cm: call_api(url.format(token), dict( @@ -199,32 +228,30 @@ class TestSubscriptionModeration(unittest.TestCase): def test_defer(self): # Defer the decision for some other moderator. - token, token_owner, member = self._registrar.register(self._anne) + with transaction(): + token, token_owner, member = self._registrar.register(self._anne) # Anne's subscription request got held. self.assertIsNone(member) url = 'http://localhost:9001/3.0/lists/ant@example.com/requests/{}' - with self.assertRaises(HTTPError) as cm: - call_api(url.format(token), dict( - action='defer', - )) - self.assertEqual(cm.exception.code, 204) + content, response = call_api(url.format(token), dict( + action='defer', + )) + self.assertEqual(response.status, 204) # Anne is not a member. - self.assertIsNone(self._mlist.get_members('anne@example.com')) + self.assertIsNone(self._mlist.members.get_member('anne@example.com')) # The request URL still exists. - with self.assertRaises(HTTPError) as cm: - call_api(url.format(token), dict( - action='defer', - )) - self.assertEqual(cm.exception.code, 204) + content, response = call_api(url.format(token), dict( + action='defer', + )) + self.assertEqual(response.status, 204) # And now we can accept it. - with self.assertRaises(HTTPError) as cm: - call_api(url.format(token), dict( - action='accept', - )) - self.assertEqual(cm.exception.code, 204) + content, response = call_api(url.format(token), dict( + action='accept', + )) + self.assertEqual(response.status, 204) # Anne is a member. self.assertEqual( - self._mlist.get_members('anne@example.com').address, + self._mlist.members.get_member('anne@example.com').address, self._anne) # The request URL no longer exists. with self.assertRaises(HTTPError) as cm: @@ -233,23 +260,31 @@ class TestSubscriptionModeration(unittest.TestCase): )) self.assertEqual(cm.exception.code, 404) + def test_defer_bad_token(self): + # Try to accept a request with a bogus token. + with self.assertRaises(HTTPError) as cm: + call_api('http://localhost:9001/3.0/lists/ant@example.com' + '/requests/bogus', + dict(action='defer')) + self.assertEqual(cm.exception.code, 404) + def test_reject(self): # POST to the request to reject it. This leaves a bounce message in # the virgin queue. - token, token_owner, member = self._registrar.register(self._anne) + with transaction(): + token, token_owner, member = self._registrar.register(self._anne) # Anne's subscription request got held. self.assertIsNone(member) # There are currently no messages in the virgin queue. items = get_queue_messages('virgin') self.assertEqual(len(items), 0) url = 'http://localhost:9001/3.0/lists/ant@example.com/requests/{}' - with self.assertRaises(HTTPError) as cm: - call_api(url.format(token), dict( - action='reject', - )) - self.assertEqual(cm.exception.code, 204) + content, response = call_api(url.format(token), dict( + action='reject', + )) + self.assertEqual(response.status, 204) # Anne is not a member. - self.assertIsNone(self._mlist.get_members('anne@example.com')) + self.assertIsNone(self._mlist.members.get_member('anne@example.com')) # The request URL no longer exists. with self.assertRaises(HTTPError) as cm: call_api(url.format(token), dict( @@ -260,3 +295,11 @@ class TestSubscriptionModeration(unittest.TestCase): items = get_queue_messages('virgin') self.assertEqual(len(items), 1) self.assertEqual(str(items[0].msg), '') + + def test_reject_bad_token(self): + # Try to accept a request with a bogus token. + with self.assertRaises(HTTPError) as cm: + call_api('http://localhost:9001/3.0/lists/ant@example.com' + '/requests/bogus', + dict(action='reject')) + self.assertEqual(cm.exception.code, 404) -- cgit v1.2.3-70-g09d2