From 783163c4e7eda6d5983bcca512db645c64dad349 Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Sat, 21 Mar 2015 21:32:12 -0400 Subject: * Refactor add_member() so that it uses a RequestRecord namedtuple. * RequestRecord contains no password key so these are not part of the held requests database any more. * Pending record contains `email` now instead of `address`. --- src/mailman/app/docs/moderator.rst | 30 +++--- src/mailman/app/membership.py | 103 +++++++----------- src/mailman/app/moderator.py | 57 +++++----- src/mailman/app/subscriptions.py | 19 ++-- src/mailman/app/tests/test_bounces.py | 22 ++-- src/mailman/app/tests/test_membership.py | 157 ++++++++++++++++------------ src/mailman/app/tests/test_moderation.py | 30 +++++- src/mailman/app/tests/test_notifications.py | 37 ++++--- src/mailman/chains/docs/moderation.rst | 7 +- src/mailman/commands/cli_members.py | 12 +-- src/mailman/commands/docs/members.rst | 37 ++++--- src/mailman/interfaces/subscriptions.py | 16 +++ src/mailman/interfaces/usermanager.py | 16 +++ src/mailman/model/usermanager.py | 21 ++++ src/mailman/mta/tests/test_delivery.py | 8 +- src/mailman/rest/docs/moderation.rst | 24 ++--- src/mailman/rest/tests/test_moderation.py | 11 +- src/mailman/runners/docs/outgoing.rst | 16 +-- src/mailman/templates/en/unsubauth.txt | 2 +- 19 files changed, 367 insertions(+), 258 deletions(-) diff --git a/src/mailman/app/docs/moderator.rst b/src/mailman/app/docs/moderator.rst index 490c9630a..82d29074a 100644 --- a/src/mailman/app/docs/moderator.rst +++ b/src/mailman/app/docs/moderator.rst @@ -238,14 +238,16 @@ Holding subscription requests For closed lists, subscription requests will also be held for moderator approval. In this case, several pieces of information related to the subscription must be provided, including the subscriber's address and real -name, their password (possibly hashed), what kind of delivery option they are -choosing and their preferred language. +name, what kind of delivery option they are choosing and their preferred +language. >>> from mailman.app.moderator import hold_subscription >>> from mailman.interfaces.member import DeliveryMode + >>> from mailman.interfaces.subscriptions import RequestRecord >>> req_id = hold_subscription( - ... mlist, 'fred@example.org', 'Fred Person', - ... '{NONE}abcxyz', DeliveryMode.regular, 'en') + ... mlist, + ... RequestRecord('fred@example.org', 'Fred Person', + ... DeliveryMode.regular, 'en')) Disposing of membership change requests @@ -269,8 +271,9 @@ The held subscription can also be discarded. Gwen tries to subscribe to the mailing list, but... >>> req_id = hold_subscription( - ... mlist, 'gwen@example.org', 'Gwen Person', - ... '{NONE}zyxcba', DeliveryMode.regular, 'en') + ... mlist, + ... RequestRecord('gwen@example.org', 'Gwen Person', + ... DeliveryMode.regular, 'en')) ...her request is rejected... @@ -305,8 +308,9 @@ mailing list. >>> mlist.send_welcome_message = False >>> req_id = hold_subscription( - ... mlist, 'herb@example.org', 'Herb Person', - ... 'abcxyz', DeliveryMode.regular, 'en') + ... mlist, + ... RequestRecord('herb@example.org', 'Herb Person', + ... DeliveryMode.regular, 'en')) The moderators accept the subscription request. @@ -399,8 +403,9 @@ list is configured to send them. Iris tries to subscribe to the mailing list. - >>> req_id = hold_subscription(mlist, 'iris@example.org', 'Iris Person', - ... 'password', DeliveryMode.regular, 'en') + >>> req_id = hold_subscription(mlist, + ... RequestRecord('iris@example.org', 'Iris Person', + ... DeliveryMode.regular, 'en')) There's now a message in the virgin queue, destined for the list owner. @@ -491,8 +496,9 @@ can get a welcome message. >>> mlist.admin_notify_mchanges = False >>> mlist.send_welcome_message = True - >>> req_id = hold_subscription(mlist, 'kate@example.org', 'Kate Person', - ... 'password', DeliveryMode.regular, 'en') + >>> req_id = hold_subscription(mlist, + ... RequestRecord('kate@example.org', 'Kate Person', + ... DeliveryMode.regular, 'en')) >>> handle_subscription(mlist, req_id, Action.accept) >>> messages = get_queue_messages('virgin') >>> len(messages) diff --git a/src/mailman/app/membership.py b/src/mailman/app/membership.py index 996493dc4..757a87393 100644 --- a/src/mailman/app/membership.py +++ b/src/mailman/app/membership.py @@ -27,7 +27,6 @@ __all__ = [ from email.utils import formataddr from mailman.app.notifications import ( send_goodbye_message, send_welcome_message) -from mailman.config import config from mailman.core.i18n import _ from mailman.email.message import OwnerNotification from mailman.interfaces.bans import IBanManager @@ -40,8 +39,7 @@ from zope.component import getUtility -def add_member(mlist, email, display_name, password, delivery_mode, language, - role=MemberRole.member): +def add_member(mlist, record, role=MemberRole.member): """Add a member right now. The member's subscription must be approved by whatever policy the list @@ -49,16 +47,8 @@ def add_member(mlist, email, display_name, password, delivery_mode, language, :param mlist: The mailing list to add the member to. :type mlist: `IMailingList` - :param email: The email address to subscribe. - :type email: str - :param display_name: The subscriber's full name. - :type display_name: str - :param password: The subscriber's plain text password. - :type password: str - :param delivery_mode: The delivery mode the subscriber has chosen. - :type delivery_mode: DeliveryMode - :param language: The language that the subscriber is going to use. - :type language: str + :param record: a subscription request record. + :type record: RequestRecord :param role: The membership role for this subscription. :type role: `MemberRole` :return: The just created member. @@ -69,62 +59,41 @@ def add_member(mlist, email, display_name, password, delivery_mode, language, :raises MembershipIsBannedError: if the membership is not allowed. """ # Check to see if the email address is banned. - if IBanManager(mlist).is_banned(email): - raise MembershipIsBannedError(mlist, email) - # See if there's already a user linked with the given address. + if IBanManager(mlist).is_banned(record.email): + raise MembershipIsBannedError(mlist, record.email) + # Make sure there is a user linked with the given address. user_manager = getUtility(IUserManager) - user = user_manager.get_user(email) - if user is None: - # A user linked to this address does not yet exist. Is the address - # itself known but just not linked to a user? - address = user_manager.get_address(email) - if address is None: - # Nope, we don't even know about this address, so create both the - # user and address now. - user = user_manager.create_user(email, display_name) - # Do it this way so we don't have to flush the previous change. - address = list(user.addresses)[0] - else: - # The address object exists, but it's not linked to a user. - # Create the user and link it now. - user = user_manager.create_user() - user.display_name = ( - display_name if display_name else address.display_name) - user.link(address) - # Encrypt the password using the currently selected hash scheme. - user.password = config.password_context.encrypt(password) - user.preferences.preferred_language = language + user = user_manager.make_user(record.email, record.display_name) + # Encrypt the password using the currently selected hash scheme. + user.preferences.preferred_language = record.language + # Subscribe the address, not the user. + # We're looking for two versions of the email address, the case + # preserved version and the case insensitive version. We'll + # subscribe the version with matching case if it exists, otherwise + # we'll use one of the matching case-insensitively ones. It's + # undefined which one we pick. + case_preserved = None + case_insensitive = None + for address in user.addresses: + if address.original_email == record.email: + case_preserved = address + if address.email == record.email.lower(): + case_insensitive = address + assert case_preserved is not None or case_insensitive is not None, ( + 'Could not find a linked address for: {}'.format(record.email)) + address = (case_preserved if case_preserved is not None + else case_insensitive) + # Create the member and set the appropriate preferences. It's + # possible we're subscribing the lower cased version of the address; + # if that's already subscribed re-issue the exception with the correct + # email address (i.e. the one passed in here). + try: member = mlist.subscribe(address, role) - member.preferences.delivery_mode = delivery_mode - else: - # The user exists and is linked to the case-insensitive address. - # We're looking for two versions of the email address, the case - # preserved version and the case insensitive version. We'll - # subscribe the version with matching case if it exists, otherwise - # we'll use one of the matching case-insensitively ones. It's - # undefined which one we pick. - case_preserved = None - case_insensitive = None - for address in user.addresses: - if address.original_email == email: - case_preserved = address - if address.email == email.lower(): - case_insensitive = address - assert case_preserved is not None or case_insensitive is not None, ( - 'Could not find a linked address for: {}'.format(email)) - address = (case_preserved if case_preserved is not None - else case_insensitive) - # Create the member and set the appropriate preferences. It's - # possible we're subscribing the lower cased version of the address; - # if that's already subscribed re-issue the exception with the correct - # email address (i.e. the one passed in here). - try: - member = mlist.subscribe(address, role) - except AlreadySubscribedError as error: - raise AlreadySubscribedError( - error.fqdn_listname, email, error.role) - member.preferences.preferred_language = language - member.preferences.delivery_mode = delivery_mode + except AlreadySubscribedError as error: + raise AlreadySubscribedError( + error.fqdn_listname, record.email, error.role) + member.preferences.preferred_language = record.language + member.preferences.delivery_mode = record.delivery_mode return member diff --git a/src/mailman/app/moderator.py b/src/mailman/app/moderator.py index b55388e6a..0e4d59479 100644 --- a/src/mailman/app/moderator.py +++ b/src/mailman/app/moderator.py @@ -44,6 +44,7 @@ from mailman.interfaces.member import ( AlreadySubscribedError, DeliveryMode, NotAMemberError) from mailman.interfaces.messages import IMessageStore from mailman.interfaces.requests import IListRequests, RequestType +from mailman.interfaces.subscriptions import RequestRecord from mailman.utilities.datetime import now from mailman.utilities.i18n import make from zope.component import getUtility @@ -192,26 +193,26 @@ def handle_message(mlist, id, action, -def hold_subscription(mlist, address, display_name, password, mode, language): +def hold_subscription(mlist, record): data = dict(when=now().isoformat(), - address=address, - display_name=display_name, - password=password, - delivery_mode=mode.name, - language=language) - # Now hold this request. We'll use the address as the key. + email=record.email, + display_name=record.display_name, + delivery_mode=record.delivery_mode.name, + language=record.language) + # Now hold this request. We'll use the email address as the key. requestsdb = IListRequests(mlist) request_id = requestsdb.hold_request( - RequestType.subscription, address, data) + RequestType.subscription, record.email, data) vlog.info('%s: held subscription request from %s', - mlist.fqdn_listname, address) + mlist.fqdn_listname, record.email) # Possibly notify the administrator in default list language if mlist.admin_immed_notify: + email = record.email # XXX: seems unnecessary subject = _( - 'New subscription request to $mlist.display_name from $address') + 'New subscription request to $mlist.display_name from $email') text = make('subauth.txt', mailing_list=mlist, - username=address, + username=record.email, listname=mlist.fqdn_listname, admindb_url=mlist.script_url('admindb'), ) @@ -236,19 +237,19 @@ def handle_subscription(mlist, id, action, comment=None): elif action is Action.reject: key, data = requestdb.get_request(id) _refuse(mlist, _('Subscription request'), - data['address'], + data['email'], comment or _('[No reason given]'), lang=getUtility(ILanguageManager)[data['language']]) elif action is Action.accept: key, data = requestdb.get_request(id) delivery_mode = DeliveryMode[data['delivery_mode']] - address = data['address'] + email = data['email'] display_name = data['display_name'] language = getUtility(ILanguageManager)[data['language']] - password = data['password'] try: - add_member(mlist, address, display_name, password, - delivery_mode, language) + add_member( + mlist, + RequestRecord(email, display_name, delivery_mode, language)) except AlreadySubscribedError: # The address got subscribed in some other way after the original # request was made and accepted. @@ -256,9 +257,9 @@ def handle_subscription(mlist, id, action, comment=None): else: if mlist.admin_notify_mchanges: send_admin_subscription_notice( - mlist, address, display_name, language) + mlist, email, display_name, language) slog.info('%s: new %s, %s %s', mlist.fqdn_listname, - delivery_mode, formataddr((display_name, address)), + delivery_mode, formataddr((display_name, email)), 'via admin approval') else: raise AssertionError('Unexpected action: {0}'.format(action)) @@ -267,20 +268,20 @@ def handle_subscription(mlist, id, action, comment=None): -def hold_unsubscription(mlist, address): - data = dict(address=address) +def hold_unsubscription(mlist, email): + data = dict(email=email) requestsdb = IListRequests(mlist) request_id = requestsdb.hold_request( - RequestType.unsubscription, address, data) + RequestType.unsubscription, email, data) vlog.info('%s: held unsubscription request from %s', - mlist.fqdn_listname, address) + mlist.fqdn_listname, email) # Possibly notify the administrator of the hold if mlist.admin_immed_notify: subject = _( - 'New unsubscription request from $mlist.display_name by $address') + 'New unsubscription request from $mlist.display_name by $email') text = make('unsubauth.txt', mailing_list=mlist, - address=address, + email=email, listname=mlist.fqdn_listname, admindb_url=mlist.script_url('admindb'), ) @@ -297,7 +298,7 @@ def hold_unsubscription(mlist, address): def handle_unsubscription(mlist, id, action, comment=None): requestdb = IListRequests(mlist) key, data = requestdb.get_request(id) - address = data['address'] + email = data['email'] if action is Action.defer: # Nothing to do. return @@ -306,16 +307,16 @@ def handle_unsubscription(mlist, id, action, comment=None): pass elif action is Action.reject: key, data = requestdb.get_request(id) - _refuse(mlist, _('Unsubscription request'), address, + _refuse(mlist, _('Unsubscription request'), email, comment or _('[No reason given]')) elif action is Action.accept: key, data = requestdb.get_request(id) try: - delete_member(mlist, address) + delete_member(mlist, email) except NotAMemberError: # User has already been unsubscribed. pass - slog.info('%s: deleted %s', mlist.fqdn_listname, address) + slog.info('%s: deleted %s', mlist.fqdn_listname, email) else: raise AssertionError('Unexpected action: {0}'.format(action)) # Delete the request from the database. diff --git a/src/mailman/app/subscriptions.py b/src/mailman/app/subscriptions.py index bcb9e6585..cc363b30d 100644 --- a/src/mailman/app/subscriptions.py +++ b/src/mailman/app/subscriptions.py @@ -24,21 +24,19 @@ __all__ = [ from operator import attrgetter -from passlib.utils import generate_password as generate from sqlalchemy import and_, or_ from uuid import UUID from zope.component import getUtility from zope.interface import implementer from mailman.app.membership import add_member, delete_member -from mailman.config import config from mailman.core.constants import system_preferences from mailman.database.transaction import dbconnection from mailman.interfaces.listmanager import ( IListManager, ListDeletingEvent, NoSuchListError) from mailman.interfaces.member import DeliveryMode, MemberRole from mailman.interfaces.subscriptions import ( - ISubscriptionService, MissingUserError) + ISubscriptionService, MissingUserError, RequestRecord) from mailman.interfaces.usermanager import IUserManager from mailman.model.member import Member @@ -148,16 +146,11 @@ class SubscriptionService: if isinstance(subscriber, str): if display_name is None: display_name, at, domain = subscriber.partition('@') - # Because we want to keep the REST API simple, there is no - # password or language given to us. We'll use the system's - # default language for the user's default language. We'll set the - # password to a system default. This will have to get reset since - # it can't be retrieved. Note that none of these are used unless - # the address is completely new to us. - password = generate(int(config.passwords.password_length)) - return add_member(mlist, subscriber, display_name, password, - delivery_mode, - system_preferences.preferred_language, role) + return add_member( + mlist, + RequestRecord(subscriber, display_name, delivery_mode, + system_preferences.preferred_language), + role) else: # We have to assume it's a UUID. assert isinstance(subscriber, UUID), 'Not a UUID' diff --git a/src/mailman/app/tests/test_bounces.py b/src/mailman/app/tests/test_bounces.py index a84ef24c2..626d4841f 100644 --- a/src/mailman/app/tests/test_bounces.py +++ b/src/mailman/app/tests/test_bounces.py @@ -42,6 +42,7 @@ from mailman.interfaces.bounce import UnrecognizedBounceDisposition from mailman.interfaces.languages import ILanguageManager from mailman.interfaces.member import DeliveryMode, MemberRole from mailman.interfaces.pending import IPendings +from mailman.interfaces.subscriptions import RequestRecord from mailman.interfaces.usermanager import IUserManager from mailman.testing.helpers import ( LogFileMark, get_queue_messages, specialized_message_from_string as mfs) @@ -193,9 +194,10 @@ class TestSendProbe(unittest.TestCase): def setUp(self): self._mlist = create_list('test@example.com') self._mlist.send_welcome_message = False - self._member = add_member(self._mlist, 'anne@example.com', - 'Anne Person', 'xxx', - DeliveryMode.regular, 'en') + self._member = add_member( + self._mlist, + RequestRecord('anne@example.com', 'Anne Person', + DeliveryMode.regular, 'en')) self._msg = mfs("""\ From: bouncer@example.com To: anne@example.com @@ -285,9 +287,10 @@ class TestSendProbeNonEnglish(unittest.TestCase): def setUp(self): self._mlist = create_list('test@example.com') - self._member = add_member(self._mlist, 'anne@example.com', - 'Anne Person', 'xxx', - DeliveryMode.regular, 'en') + self._member = add_member( + self._mlist, + RequestRecord('anne@example.com', 'Anne Person', + DeliveryMode.regular, 'en')) self._msg = mfs("""\ From: bouncer@example.com To: anne@example.com @@ -351,9 +354,10 @@ class TestProbe(unittest.TestCase): def setUp(self): self._mlist = create_list('test@example.com') self._mlist.send_welcome_message = False - self._member = add_member(self._mlist, 'anne@example.com', - 'Anne Person', 'xxx', - DeliveryMode.regular, 'en') + self._member = add_member( + self._mlist, + RequestRecord('anne@example.com', 'Anne Person', + DeliveryMode.regular, 'en')) self._msg = mfs("""\ From: bouncer@example.com To: anne@example.com diff --git a/src/mailman/app/tests/test_membership.py b/src/mailman/app/tests/test_membership.py index cdf0641ea..481de2bb8 100644 --- a/src/mailman/app/tests/test_membership.py +++ b/src/mailman/app/tests/test_membership.py @@ -19,7 +19,6 @@ __all__ = [ 'TestAddMember', - 'TestAddMemberPassword', 'TestDeleteMember', ] @@ -33,6 +32,7 @@ from mailman.interfaces.bans import IBanManager from mailman.interfaces.member import ( AlreadySubscribedError, DeliveryMode, MemberRole, MembershipIsBannedError, NotAMemberError) +from mailman.interfaces.subscriptions import RequestRecord from mailman.interfaces.usermanager import IUserManager from mailman.testing.layers import ConfigLayer from zope.component import getUtility @@ -48,9 +48,11 @@ class TestAddMember(unittest.TestCase): def test_add_member_new_user(self): # Test subscribing a user to a mailing list when the email address has # not yet been associated with a user. - member = add_member(self._mlist, 'aperson@example.com', - 'Anne Person', '123', DeliveryMode.regular, - system_preferences.preferred_language) + member = add_member( + self._mlist, + RequestRecord('aperson@example.com', 'Anne Person', + DeliveryMode.regular, + system_preferences.preferred_language)) self.assertEqual(member.address.email, 'aperson@example.com') self.assertEqual(member.list_id, 'test.example.com') self.assertEqual(member.role, MemberRole.member) @@ -60,9 +62,11 @@ class TestAddMember(unittest.TestCase): # already been associated with a user. user_manager = getUtility(IUserManager) user_manager.create_user('aperson@example.com', 'Anne Person') - member = add_member(self._mlist, 'aperson@example.com', - 'Anne Person', '123', DeliveryMode.regular, - system_preferences.preferred_language) + member = add_member( + self._mlist, + RequestRecord('aperson@example.com', 'Anne Person', + DeliveryMode.regular, + system_preferences.preferred_language)) self.assertEqual(member.address.email, 'aperson@example.com') self.assertEqual(member.list_id, 'test.example.com') @@ -71,9 +75,11 @@ class TestAddMember(unittest.TestCase): # subscribe to the mailing list. IBanManager(self._mlist).ban('anne@example.com') with self.assertRaises(MembershipIsBannedError) as cm: - add_member(self._mlist, 'anne@example.com', 'Anne Person', - '123', DeliveryMode.regular, - system_preferences.preferred_language) + add_member( + self._mlist, + RequestRecord('anne@example.com', 'Anne Person', + DeliveryMode.regular, + system_preferences.preferred_language)) self.assertEqual( str(cm.exception), 'anne@example.com is not allowed to subscribe to test@example.com') @@ -84,17 +90,21 @@ class TestAddMember(unittest.TestCase): IBanManager(None).ban('anne@example.com') self.assertRaises( MembershipIsBannedError, - add_member, self._mlist, 'anne@example.com', 'Anne Person', - '123', DeliveryMode.regular, system_preferences.preferred_language) + add_member, self._mlist, + RequestRecord('anne@example.com', 'Anne Person', + DeliveryMode.regular, + system_preferences.preferred_language)) def test_add_member_banned_from_different_list(self): # Test that members who are banned by on a different list can still be # subscribed to other mlists. sample_list = create_list('sample@example.com') IBanManager(sample_list).ban('anne@example.com') - member = add_member(self._mlist, 'anne@example.com', - 'Anne Person', '123', DeliveryMode.regular, - system_preferences.preferred_language) + member = add_member( + self._mlist, + RequestRecord('anne@example.com', 'Anne Person', + DeliveryMode.regular, + system_preferences.preferred_language)) self.assertEqual(member.address.email, 'anne@example.com') def test_add_member_banned_by_pattern(self): @@ -102,33 +112,41 @@ class TestAddMember(unittest.TestCase): IBanManager(self._mlist).ban('^.*@example.com') self.assertRaises( MembershipIsBannedError, - add_member, self._mlist, 'anne@example.com', 'Anne Person', - '123', DeliveryMode.regular, system_preferences.preferred_language) + add_member, self._mlist, + RequestRecord('anne@example.com', 'Anne Person', + DeliveryMode.regular, + system_preferences.preferred_language)) def test_add_member_globally_banned_by_pattern(self): # Addresses matching global regexp ban patterns cannot subscribe. IBanManager(None).ban('^.*@example.com') self.assertRaises( MembershipIsBannedError, - add_member, self._mlist, 'anne@example.com', 'Anne Person', - '123', DeliveryMode.regular, system_preferences.preferred_language) + add_member, self._mlist, + RequestRecord('anne@example.com', 'Anne Person', + DeliveryMode.regular, + system_preferences.preferred_language)) def test_add_member_banned_from_different_list_by_pattern(self): # Addresses matching regexp ban patterns on one list can still # subscribe to other mailing lists. sample_list = create_list('sample@example.com') IBanManager(sample_list).ban('^.*@example.com') - member = add_member(self._mlist, 'anne@example.com', - 'Anne Person', '123', DeliveryMode.regular, - system_preferences.preferred_language) + member = add_member( + self._mlist, + RequestRecord('anne@example.com', 'Anne Person', + DeliveryMode.regular, + system_preferences.preferred_language)) self.assertEqual(member.address.email, 'anne@example.com') def test_add_member_moderator(self): # Test adding a moderator to a mailing list. - member = add_member(self._mlist, 'aperson@example.com', - 'Anne Person', '123', DeliveryMode.regular, - system_preferences.preferred_language, - MemberRole.moderator) + member = add_member( + self._mlist, + RequestRecord('aperson@example.com', 'Anne Person', + DeliveryMode.regular, + system_preferences.preferred_language), + MemberRole.moderator) self.assertEqual(member.address.email, 'aperson@example.com') self.assertEqual(member.list_id, 'test.example.com') self.assertEqual(member.role, MemberRole.moderator) @@ -136,29 +154,37 @@ class TestAddMember(unittest.TestCase): def test_add_member_twice(self): # Adding a member with the same role twice causes an # AlreadySubscribedError to be raised. - add_member(self._mlist, 'aperson@example.com', - 'Anne Person', '123', DeliveryMode.regular, - system_preferences.preferred_language, - MemberRole.member) + add_member( + self._mlist, + RequestRecord('aperson@example.com', 'Anne Person', + DeliveryMode.regular, + system_preferences.preferred_language), + MemberRole.member) with self.assertRaises(AlreadySubscribedError) as cm: - add_member(self._mlist, 'aperson@example.com', - 'Anne Person', '123', DeliveryMode.regular, - system_preferences.preferred_language, - MemberRole.member) + add_member( + self._mlist, + RequestRecord('aperson@example.com', 'Anne Person', + DeliveryMode.regular, + system_preferences.preferred_language), + MemberRole.member) self.assertEqual(cm.exception.fqdn_listname, 'test@example.com') self.assertEqual(cm.exception.email, 'aperson@example.com') self.assertEqual(cm.exception.role, MemberRole.member) def test_add_member_with_different_roles(self): # Adding a member twice with different roles is okay. - member_1 = add_member(self._mlist, 'aperson@example.com', - 'Anne Person', '123', DeliveryMode.regular, - system_preferences.preferred_language, - MemberRole.member) - member_2 = add_member(self._mlist, 'aperson@example.com', - 'Anne Person', '123', DeliveryMode.regular, - system_preferences.preferred_language, - MemberRole.owner) + member_1 = add_member( + self._mlist, + RequestRecord('aperson@example.com', 'Anne Person', + DeliveryMode.regular, + system_preferences.preferred_language), + MemberRole.member) + member_2 = add_member( + self._mlist, + RequestRecord('aperson@example.com', 'Anne Person', + DeliveryMode.regular, + system_preferences.preferred_language), + MemberRole.owner) self.assertEqual(member_1.list_id, member_2.list_id) self.assertEqual(member_1.address, member_2.address) self.assertEqual(member_1.user, member_2.user) @@ -171,43 +197,38 @@ class TestAddMember(unittest.TestCase): # test subscribes the lower case address and ensures the original # mixed case address can't be subscribed. email = 'APerson@example.com' - add_member(self._mlist, email.lower(), 'Ann Person', '123', - DeliveryMode.regular, system_preferences.preferred_language) + add_member( + self._mlist, + RequestRecord(email.lower(), 'Ann Person', + DeliveryMode.regular, + system_preferences.preferred_language)) with self.assertRaises(AlreadySubscribedError) as cm: - add_member(self._mlist, email, 'Ann Person', '123', - DeliveryMode.regular, - system_preferences.preferred_language) + add_member( + self._mlist, + RequestRecord(email, 'Ann Person', + DeliveryMode.regular, + system_preferences.preferred_language)) self.assertEqual(cm.exception.email, email) - + def test_add_member_with_lower_case_email(self): # LP: #1425359 - Mailman is case-perserving, case-insensitive. This # test subscribes the mixed case address and ensures the lower cased # address can't be added. email = 'APerson@example.com' - add_member(self._mlist, email, 'Ann Person', '123', - DeliveryMode.regular, system_preferences.preferred_language) + add_member( + self._mlist, + RequestRecord(email, 'Ann Person', + DeliveryMode.regular, + system_preferences.preferred_language)) with self.assertRaises(AlreadySubscribedError) as cm: - add_member(self._mlist, email.lower(), 'Ann Person', '123', - DeliveryMode.regular, - system_preferences.preferred_language) + add_member( + self._mlist, + RequestRecord(email.lower(), 'Ann Person', + DeliveryMode.regular, + system_preferences.preferred_language)) self.assertEqual(cm.exception.email, email.lower()) - -class TestAddMemberPassword(unittest.TestCase): - layer = ConfigLayer - - def setUp(self): - self._mlist = create_list('test@example.com') - - def test_add_member_password(self): - # Test that the password stored with the new user is encrypted. - member = add_member(self._mlist, 'anne@example.com', - 'Anne Person', 'abc', DeliveryMode.regular, - system_preferences.preferred_language) - self.assertEqual(member.user.password, '{plaintext}abc') - - class TestDeleteMember(unittest.TestCase): layer = ConfigLayer diff --git a/src/mailman/app/tests/test_moderation.py b/src/mailman/app/tests/test_moderation.py index 72a25253e..a371d8520 100644 --- a/src/mailman/app/tests/test_moderation.py +++ b/src/mailman/app/tests/test_moderation.py @@ -19,16 +19,21 @@ __all__ = [ 'TestModeration', + 'TestUnsubscription', ] import unittest from mailman.app.lifecycle import create_list -from mailman.app.moderator import handle_message, hold_message +from mailman.app.moderator import ( + handle_message, handle_subscription, handle_unsubscription, hold_message, + hold_subscription, hold_unsubscription) from mailman.interfaces.action import Action +from mailman.interfaces.member import DeliveryMode from mailman.interfaces.messages import IMessageStore from mailman.interfaces.requests import IListRequests +from mailman.interfaces.subscriptions import RequestRecord from mailman.runners.incoming import IncomingRunner from mailman.runners.outgoing import OutgoingRunner from mailman.runners.pipeline import PipelineRunner @@ -148,3 +153,26 @@ Message-ID: 'Forward of moderated message') self.assertEqual(messages[0].msgdata['recipients'], ['zack@example.com']) + + + +class TestUnsubscription(unittest.TestCase): + """Test unsubscription requests.""" + + layer = SMTPLayer + + def setUp(self): + self._mlist = create_list('test@example.com') + self._request_db = IListRequests(self._mlist) + + def test_unsubscribe_defer(self): + # When unsubscriptions must be approved by the moderator, but the + # moderator defers this decision. + token = hold_subscription( + self._mlist, + RequestRecord('anne@example.org', 'Anne Person', + DeliveryMode.regular, 'en')) + handle_subscription(self._mlist, token, Action.accept) + # Now hold and handle an unsubscription request. + token = hold_unsubscription(self._mlist, 'anne@example.org') + handle_unsubscription(self._mlist, token, Action.defer) diff --git a/src/mailman/app/tests/test_notifications.py b/src/mailman/app/tests/test_notifications.py index e46a50ebd..2bc0ed902 100644 --- a/src/mailman/app/tests/test_notifications.py +++ b/src/mailman/app/tests/test_notifications.py @@ -32,6 +32,7 @@ from mailman.app.membership import add_member from mailman.config import config from mailman.interfaces.languages import ILanguageManager from mailman.interfaces.member import DeliveryMode, MemberRole +from mailman.interfaces.subscriptions import RequestRecord from mailman.testing.helpers import get_queue_messages from mailman.testing.layers import ConfigLayer from zope.component import getUtility @@ -78,8 +79,10 @@ Welcome to the $list_name mailing list. shutil.rmtree(self.var_dir) def test_welcome_message(self): - add_member(self._mlist, 'anne@example.com', 'Anne Person', - 'password', DeliveryMode.regular, 'en') + add_member( + self._mlist, + RequestRecord('anne@example.com', 'Anne Person', + DeliveryMode.regular, 'en')) # Now there's one message in the virgin queue. messages = get_queue_messages('virgin') self.assertEqual(len(messages), 1) @@ -104,8 +107,10 @@ Welcome to the Test List mailing list. # Add the xx language and subscribe Anne using it. manager = getUtility(ILanguageManager) manager.add('xx', 'us-ascii', 'Xlandia') - add_member(self._mlist, 'anne@example.com', 'Anne Person', - 'password', DeliveryMode.regular, 'xx') + add_member( + self._mlist, + RequestRecord('anne@example.com', 'Anne Person', + DeliveryMode.regular, 'xx')) # Now there's one message in the virgin queue. messages = get_queue_messages('virgin') self.assertEqual(len(messages), 1) @@ -118,27 +123,33 @@ Welcome to the Test List mailing list. def test_no_welcome_message_to_owners(self): # Welcome messages go only to mailing list members, not to owners. - add_member(self._mlist, 'anne@example.com', 'Anne Person', - 'password', DeliveryMode.regular, 'xx', - MemberRole.owner) + add_member( + self._mlist, + RequestRecord('anne@example.com', 'Anne Person', + DeliveryMode.regular, 'xx'), + MemberRole.owner) # There is no welcome message in the virgin queue. messages = get_queue_messages('virgin') self.assertEqual(len(messages), 0) def test_no_welcome_message_to_nonmembers(self): # Welcome messages go only to mailing list members, not to nonmembers. - add_member(self._mlist, 'anne@example.com', 'Anne Person', - 'password', DeliveryMode.regular, 'xx', - MemberRole.nonmember) + add_member( + self._mlist, + RequestRecord('anne@example.com', 'Anne Person', + DeliveryMode.regular, 'xx'), + MemberRole.nonmember) # There is no welcome message in the virgin queue. messages = get_queue_messages('virgin') self.assertEqual(len(messages), 0) def test_no_welcome_message_to_moderators(self): # Welcome messages go only to mailing list members, not to moderators. - add_member(self._mlist, 'anne@example.com', 'Anne Person', - 'password', DeliveryMode.regular, 'xx', - MemberRole.moderator) + add_member( + self._mlist, + RequestRecord('anne@example.com', 'Anne Person', + DeliveryMode.regular, 'xx'), + MemberRole.moderator) # There is no welcome message in the virgin queue. messages = get_queue_messages('virgin') self.assertEqual(len(messages), 0) diff --git a/src/mailman/chains/docs/moderation.rst b/src/mailman/chains/docs/moderation.rst index 1fe7e40cb..ed7ee5a7e 100644 --- a/src/mailman/chains/docs/moderation.rst +++ b/src/mailman/chains/docs/moderation.rst @@ -31,11 +31,14 @@ Posts by list members are moderated if the member's moderation action is not deferred. The default setting for the moderation action of new members is determined by the mailing list's settings. By default, a mailing list is not set to moderate new member postings. +:: >>> from mailman.app.membership import add_member >>> from mailman.interfaces.member import DeliveryMode - >>> member = add_member(mlist, 'anne@example.com', 'Anne', 'aaa', - ... DeliveryMode.regular, 'en') + >>> from mailman.interfaces.subscriptions import RequestRecord + + >>> member = add_member(mlist, RequestRecord('anne@example.com', 'Anne', + ... DeliveryMode.regular, 'en')) >>> member on test@example.com as MemberRole.member> >>> print(member.moderation_action) diff --git a/src/mailman/commands/cli_members.py b/src/mailman/commands/cli_members.py index e4cad5966..ccacbeeb8 100644 --- a/src/mailman/commands/cli_members.py +++ b/src/mailman/commands/cli_members.py @@ -27,15 +27,14 @@ import codecs from email.utils import formataddr, parseaddr from mailman.app.membership import add_member -from mailman.config import config from mailman.core.i18n import _ from mailman.database.transaction import transactional from mailman.interfaces.command import ICLISubCommand from mailman.interfaces.listmanager import IListManager from mailman.interfaces.member import ( AlreadySubscribedError, DeliveryMode, DeliveryStatus) +from mailman.interfaces.subscriptions import RequestRecord from operator import attrgetter -from passlib.utils import generate_password as generate from zope.component import getUtility from zope.interface import implementer @@ -193,12 +192,11 @@ class Members: continue # Parse the line and ensure that the values are unicodes. display_name, email = parseaddr(line) - # Give the user a default, user-friendly password. - password = generate(int(config.passwords.password_length)) try: - add_member(mlist, email, display_name, password, - DeliveryMode.regular, - mlist.preferred_language.code) + add_member(mlist, + RequestRecord(email, display_name, + DeliveryMode.regular, + mlist.preferred_language.code)) except AlreadySubscribedError: # It's okay if the address is already subscribed, just # print a warning and continue. diff --git a/src/mailman/commands/docs/members.rst b/src/mailman/commands/docs/members.rst index c90418181..3e5b2d09c 100644 --- a/src/mailman/commands/docs/members.rst +++ b/src/mailman/commands/docs/members.rst @@ -36,12 +36,16 @@ Once the mailing list add some members, they will be displayed. >>> from mailman.interfaces.member import DeliveryMode >>> from mailman.app.membership import add_member - >>> add_member(mlist1, 'anne@example.com', 'Anne Person', 'xxx', - ... DeliveryMode.regular, mlist1.preferred_language.code) + >>> from mailman.interfaces.subscriptions import RequestRecord + + >>> add_member(mlist1, RequestRecord('anne@example.com', 'Anne Person', + ... DeliveryMode.regular, + ... mlist1.preferred_language.code)) on test1@example.com as MemberRole.member> - >>> add_member(mlist1, 'bart@example.com', 'Bart Person', 'xxx', - ... DeliveryMode.regular, mlist1.preferred_language.code) + >>> add_member(mlist1, RequestRecord('bart@example.com', 'Bart Person', + ... DeliveryMode.regular, + ... mlist1.preferred_language.code)) on test1@example.com as MemberRole.member> @@ -52,8 +56,9 @@ Once the mailing list add some members, they will be displayed. Members are displayed in alphabetical order based on their address. :: - >>> add_member(mlist1, 'anne@aaaxample.com', 'Anne Person', 'xxx', - ... DeliveryMode.regular, mlist1.preferred_language.code) + >>> add_member(mlist1, RequestRecord('anne@aaaxample.com', 'Anne Person', + ... DeliveryMode.regular, + ... mlist1.preferred_language.code)) on test1@example.com as MemberRole.member> @@ -136,21 +141,29 @@ status is enabled... :: >>> from mailman.interfaces.member import DeliveryStatus + >>> member = mlist1.members.get_member('anne@aaaxample.com') >>> member.preferences.delivery_status = DeliveryStatus.by_moderator >>> member = mlist1.members.get_member('bart@example.com') >>> member.preferences.delivery_status = DeliveryStatus.by_user + >>> member = add_member( - ... mlist1, 'cris@example.com', 'Cris Person', 'xxx', - ... DeliveryMode.regular, mlist1.preferred_language.code) + ... mlist1, + ... RequestRecord('cris@example.com', 'Cris Person', + ... DeliveryMode.regular, + ... mlist1.preferred_language.code)) >>> member.preferences.delivery_status = DeliveryStatus.unknown >>> member = add_member( - ... mlist1, 'dave@example.com', 'Dave Person', 'xxx', - ... DeliveryMode.regular, mlist1.preferred_language.code) + ... mlist1, + ... RequestRecord('dave@example.com', 'Dave Person', + ... DeliveryMode.regular, + ... mlist1.preferred_language.code)) >>> member.preferences.delivery_status = DeliveryStatus.enabled >>> member = add_member( - ... mlist1, 'elly@example.com', 'Elly Person', 'xxx', - ... DeliveryMode.regular, mlist1.preferred_language.code) + ... mlist1, + ... RequestRecord('elly@example.com', 'Elly Person', + ... DeliveryMode.regular, + ... mlist1.preferred_language.code)) >>> member.preferences.delivery_status = DeliveryStatus.by_bounces >>> args.nomail = 'enabled' diff --git a/src/mailman/interfaces/subscriptions.py b/src/mailman/interfaces/subscriptions.py index c72a902cb..677f591ef 100644 --- a/src/mailman/interfaces/subscriptions.py +++ b/src/mailman/interfaces/subscriptions.py @@ -19,9 +19,12 @@ __all__ = [ 'ISubscriptionService', + 'RequestRecord', ] +from collections import namedtuple + from mailman.interfaces.errors import MailmanError from mailman.interfaces.member import DeliveryMode, MemberRole from zope.interface import Interface @@ -39,6 +42,19 @@ class MissingUserError(MailmanError): return self.user_id + +_RequestRecord = namedtuple( + 'RequestRecord', + 'email display_name delivery_mode, language') +def RequestRecord(email, display_name='', + delivery_mode=DeliveryMode.regular, + language=None): + if language is None: + from mailman.core.constants import system_preferences + language = system_preferences.preferred_language + return _RequestRecord(email, display_name, delivery_mode, language) + + class ISubscriptionService(Interface): """General Subscription services.""" diff --git a/src/mailman/interfaces/usermanager.py b/src/mailman/interfaces/usermanager.py index 798d1d127..5f3a324cc 100644 --- a/src/mailman/interfaces/usermanager.py +++ b/src/mailman/interfaces/usermanager.py @@ -43,6 +43,22 @@ class IUserManager(Interface): registered. """ + def make_user(email, display_name=None): + """Create a new user linked to an address object. + + If ``email`` is already associated with an existing `IAddress` + object, use that, otherwise create a new `IAddress`. If the + address object already points to an `IUser` return it. If a new + `IUser` is created, link the address to the user. + + :param email: The email address. + :type email: str + :param display_name: The display name. + :type display_name: str + :return: the IUser object that exists or was created. + :rtype: IUser + """ + def delete_user(user): """Delete the given user. diff --git a/src/mailman/model/usermanager.py b/src/mailman/model/usermanager.py index aefd955ed..ae499452b 100644 --- a/src/mailman/model/usermanager.py +++ b/src/mailman/model/usermanager.py @@ -46,6 +46,27 @@ class UserManager: user.link(address) return user + def make_user(self, email, display_name=None): + """See `IUserManager`.""" + # See if there's already a user linked with the given address. + user = self.get_user(email) + if user is None: + # A user linked to this address does not yet exist. Is the + # address itself known but just not linked to a user? + address = self.get_address(email) + if address is None: + # Nope, we don't even know about this address, so create both + # the user and address now. + return self.create_user(email, display_name) + # The address exists, but it's not yet linked to a user. Create + # the empty user object and link them together. + user = self.create_user() + user.display_name = ( + display_name if display_name else address.display_name) + user.link(address) + return user + return user + @dbconnection def delete_user(self, store, user): """See `IUserManager`.""" diff --git a/src/mailman/mta/tests/test_delivery.py b/src/mailman/mta/tests/test_delivery.py index 77d31d3a3..18eef8845 100644 --- a/src/mailman/mta/tests/test_delivery.py +++ b/src/mailman/mta/tests/test_delivery.py @@ -32,6 +32,7 @@ from mailman.app.membership import add_member from mailman.config import config from mailman.interfaces.mailinglist import Personalization from mailman.interfaces.member import DeliveryMode +from mailman.interfaces.subscriptions import RequestRecord from mailman.mta.deliver import Deliver from mailman.testing.helpers import ( specialized_message_from_string as mfs) @@ -63,9 +64,10 @@ class TestIndividualDelivery(unittest.TestCase): self._mlist = create_list('test@example.com') self._mlist.personalize = Personalization.individual # Make Anne a member of this mailing list. - self._anne = add_member(self._mlist, - 'anne@example.org', 'Anne Person', - 'xyz', DeliveryMode.regular, 'en') + self._anne = add_member( + self._mlist, + RequestRecord('anne@example.org', 'Anne Person', + DeliveryMode.regular, 'en')) # Clear out any results from the previous test. del _deliveries[:] self._msg = mfs("""\ diff --git a/src/mailman/rest/docs/moderation.rst b/src/mailman/rest/docs/moderation.rst index 6aec921f0..aa41ff350 100644 --- a/src/mailman/rest/docs/moderation.rst +++ b/src/mailman/rest/docs/moderation.rst @@ -216,24 +216,26 @@ requests. 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, 'anne@example.com', 'Anne Person', - ... 'password', DeliveryMode.regular, 'en') + ... 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: - address: anne@example.com delivery_mode: regular display_name: Anne Person + email: anne@example.com http_etag: "..." language: en - password: password request_id: ... type: subscription when: 2005-08-01T07:49:23 @@ -249,8 +251,8 @@ Bart tries to leave a mailing list, but he may not be allowed to. >>> from mailman.app.membership import add_member >>> from mailman.app.moderator import hold_unsubscription - >>> bart = add_member(ant, 'bart@example.com', 'Bart Person', - ... 'password', DeliveryMode.regular, 'en') + >>> bart = add_member(ant, RequestRecord('bart@example.com', 'Bart Person', + ... DeliveryMode.regular, 'en')) >>> unsub_req_id = hold_unsubscription(ant, 'bart@example.com') >>> transaction.commit() @@ -258,17 +260,16 @@ The unsubscription request is also available from the mailing list. >>> dump_json('http://localhost:9001/3.0/lists/ant@example.com/requests') entry 0: - address: anne@example.com delivery_mode: regular display_name: Anne Person + email: anne@example.com http_etag: "..." language: en - password: password request_id: ... type: subscription when: 2005-08-01T07:49:23 entry 1: - address: bart@example.com + email: bart@example.com http_etag: "..." request_id: ... type: unsubscription @@ -285,12 +286,11 @@ 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)) - address: anne@example.com delivery_mode: regular display_name: Anne Person + email: anne@example.com http_etag: "..." language: en - password: password request_id: ... type: subscription when: 2005-08-01T07:49:23 @@ -299,7 +299,7 @@ Bart's unsubscription request looks like this. >>> dump_json('http://localhost:9001/3.0/lists/ant@example.com/' ... 'requests/{}'.format(unsub_req_id)) - address: bart@example.com + email: bart@example.com http_etag: "..." request_id: ... type: unsubscription diff --git a/src/mailman/rest/tests/test_moderation.py b/src/mailman/rest/tests/test_moderation.py index 262a7ec60..c77ae2aca 100644 --- a/src/mailman/rest/tests/test_moderation.py +++ b/src/mailman/rest/tests/test_moderation.py @@ -29,6 +29,7 @@ from mailman.app.moderator import hold_message, hold_subscription from mailman.config import config from mailman.database.transaction import transaction from mailman.interfaces.member import DeliveryMode +from mailman.interfaces.subscriptions import RequestRecord from mailman.testing.helpers import ( call_api, specialized_message_from_string as mfs) from mailman.testing.layers import RESTLayer @@ -76,8 +77,9 @@ Something else. # in the database. held_id = hold_message(self._mlist, self._msg) subscribe_id = hold_subscription( - self._mlist, 'bperson@example.net', 'Bart Person', 'xyz', - DeliveryMode.regular, 'en') + 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: @@ -114,8 +116,9 @@ Something else. def test_bad_subscription_action(self): # POSTing to a held message with a bad action. held_id = hold_subscription( - self._mlist, 'cperson@example.net', 'Cris Person', 'xyz', - DeliveryMode.regular, 'en') + 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}' with self.assertRaises(HTTPError) as cm: diff --git a/src/mailman/runners/docs/outgoing.rst b/src/mailman/runners/docs/outgoing.rst index 7c3d1a989..7f7216687 100644 --- a/src/mailman/runners/docs/outgoing.rst +++ b/src/mailman/runners/docs/outgoing.rst @@ -17,16 +17,20 @@ move messages to the 'retry queue' for handling delivery failures. >>> from mailman.app.membership import add_member >>> from mailman.interfaces.member import DeliveryMode - >>> add_member(mlist, 'aperson@example.com', 'Anne Person', - ... 'password', DeliveryMode.regular, 'en') + >>> from mailman.interfaces.subscriptions import RequestRecord + + >>> add_member(mlist, RequestRecord('aperson@example.com', 'Anne Person', + ... DeliveryMode.regular, 'en')) on test@example.com as MemberRole.member> - >>> add_member(mlist, 'bperson@example.com', 'Bart Person', - ... 'password', DeliveryMode.regular, 'en') + + >>> add_member(mlist, RequestRecord('bperson@example.com', 'Bart Person', + ... DeliveryMode.regular, 'en')) on test@example.com as MemberRole.member> - >>> add_member(mlist, 'cperson@example.com', 'Cris Person', - ... 'password', DeliveryMode.regular, 'en') + + >>> add_member(mlist, RequestRecord('cperson@example.com', 'Cris Person', + ... DeliveryMode.regular, 'en')) on test@example.com as MemberRole.member> diff --git a/src/mailman/templates/en/unsubauth.txt b/src/mailman/templates/en/unsubauth.txt index 5975a2ce8..e6a6a82f2 100644 --- a/src/mailman/templates/en/unsubauth.txt +++ b/src/mailman/templates/en/unsubauth.txt @@ -1,7 +1,7 @@ Your authorization is required for a mailing list unsubscription request approval: - By: $address + By: $email From: $listname At your convenience, visit: -- cgit v1.2.3-70-g09d2