diff --git a/ceo_common/errors.py b/ceo_common/errors.py index 7d517d3..539c181 100644 --- a/ceo_common/errors.py +++ b/ceo_common/errors.py @@ -1,11 +1,11 @@ class UserNotFoundError(Exception): - def __init__(self): - super().__init__('user not found') + def __init__(self, username): + super().__init__(f"user '{username}' not found") class GroupNotFoundError(Exception): - def __init__(self): - super().__init__('group not found') + def __init__(self, group_name): + super().__init__(f"group '{group_name}' not found") class BadRequest(Exception): diff --git a/ceo_common/interfaces/IGroup.py b/ceo_common/interfaces/IGroup.py index 7049f14..09857cc 100644 --- a/ceo_common/interfaces/IGroup.py +++ b/ceo_common/interfaces/IGroup.py @@ -1,3 +1,5 @@ +from typing import List + from zope.interface import Interface, Attribute @@ -21,5 +23,8 @@ class IGroup(Interface): def remove_member(username: str): """Remove the member from this group in LDAP.""" + def set_members(usernames: List[str]): + """Set all of the members of this group in LDAP.""" + def to_dict(): """Serialize this group as JSON.""" diff --git a/ceo_common/interfaces/ILDAPService.py b/ceo_common/interfaces/ILDAPService.py index da10a12..89424f4 100644 --- a/ceo_common/interfaces/ILDAPService.py +++ b/ceo_common/interfaces/ILDAPService.py @@ -24,6 +24,9 @@ class ILDAPService(Interface): Useful for displaying a list of users in a compact way. """ + def get_users_with_positions(self) -> List[IUser]: + """Retrieve users who have a non-empty position attribute.""" + def add_user(user: IUser): """ Add the user to the database. diff --git a/ceo_common/interfaces/IUser.py b/ceo_common/interfaces/IUser.py index f6a288f..60fb091 100644 --- a/ceo_common/interfaces/IUser.py +++ b/ceo_common/interfaces/IUser.py @@ -53,11 +53,8 @@ class IUser(Interface): def add_non_member_terms(terms: List[str]): """Add non-member terms for this user.""" - def add_position(position: str): - """Add a position to this user.""" - - def remove_position(position: str): - """Remove a position from this user.""" + def set_positions(self, positions: List[str]): + """Set the positions for this user.""" def change_password(password: str): """Replace this user's password.""" diff --git a/ceo_common/model/Config.py b/ceo_common/model/Config.py index f2ff94e..dfe7bbc 100644 --- a/ceo_common/model/Config.py +++ b/ceo_common/model/Config.py @@ -27,6 +27,6 @@ class Config: return True if val.lower() in ['false', 'no']: return False - if section.startswith('auxiliary '): - return val.split(',') + if section.startswith('auxiliary ') or section == 'positions': + return [item.strip() for item in val.split(',')] return val diff --git a/ceod/api/app_factory.py b/ceod/api/app_factory.py index b8de6c4..f033a2b 100644 --- a/ceod/api/app_factory.py +++ b/ceod/api/app_factory.py @@ -40,6 +40,9 @@ def create_app(flask_config={}): from ceod.api import groups app.register_blueprint(groups.bp, url_prefix='/api/groups') + from ceod.api import positions + app.register_blueprint(positions.bp, url_prefix='/api/positions') + from ceod.api import uwldap app.register_blueprint(uwldap.bp, url_prefix='/api/uwldap') diff --git a/ceod/api/positions.py b/ceod/api/positions.py new file mode 100644 index 0000000..9d9f4c4 --- /dev/null +++ b/ceod/api/positions.py @@ -0,0 +1,45 @@ +from flask import Blueprint, request +from zope import component + +from .utils import authz_restrict_to_syscom, create_streaming_response +from ceo_common.interfaces import ILDAPService, IConfig +from ceod.transactions.members import UpdateMemberPositionsTransaction + +bp = Blueprint('positions', __name__) + + +@bp.route('/', methods=['GET'], strict_slashes=False) +def get_positions(): + ldap_srv = component.getUtility(ILDAPService) + + positions = {} + for user in ldap_srv.get_users_with_positions(): + for position in user.positions: + positions[position] = user.uid + + return positions + + +@bp.route('/', methods=['POST'], strict_slashes=False) +@authz_restrict_to_syscom +def update_positions(): + cfg = component.getUtility(IConfig) + body = request.get_json(force=True) + + required = cfg.get('positions_required') + available = cfg.get('positions_available') + + for position in body.keys(): + if position not in available: + return { + 'error': f'unknown position: {position}' + }, 400 + + for position in required: + if position not in body: + return { + 'error': f'missing required position: {position}' + }, 400 + + txn = UpdateMemberPositionsTransaction(body) + return create_streaming_response(txn) diff --git a/ceod/model/Group.py b/ceod/model/Group.py index cf99e57..3493ce3 100644 --- a/ceod/model/Group.py +++ b/ceod/model/Group.py @@ -105,3 +105,11 @@ class Group: logger.warning(err) raise UserNotInGroupError() self.members.remove(username) + + def set_members(self, usernames: List[str]): + DNs = [ + self.ldap_srv.uid_to_dn(username) for username in usernames + ] + with self.ldap_srv.entry_ctx_for_group(self) as entry: + entry.uniqueMember = DNs + self.members = usernames diff --git a/ceod/model/LDAPService.py b/ceod/model/LDAPService.py index 8c91fc2..218f971 100644 --- a/ceod/model/LDAPService.py +++ b/ceod/model/LDAPService.py @@ -50,7 +50,7 @@ class LDAPService: base, '(objectClass=*)', search_scope=ldap3.BASE, attributes=ldap3.ALL_ATTRIBUTES) except ldap3.core.exceptions.LDAPNoSuchObjectResult: - raise UserNotFoundError() + raise UserNotFoundError(username) return conn.entries[0] def _get_readable_entry_for_group(self, conn: ldap3.Connection, cn: str) -> ldap3.Entry: @@ -60,7 +60,7 @@ class LDAPService: base, '(objectClass=*)', search_scope=ldap3.BASE, attributes=ldap3.ALL_ATTRIBUTES) except ldap3.core.exceptions.LDAPNoSuchObjectResult: - raise GroupNotFoundError() + raise GroupNotFoundError(cn) return conn.entries[0] def _get_writable_entry_for_user(self, user: IUser) -> ldap3.WritableEntry: @@ -96,11 +96,16 @@ class LDAPService: { 'uid': entry.uid.value, 'cn': entry.cn.value, - 'program': entry.program.value, + 'program': entry.program.value or 'Unknown', } for entry in conn.entries ] + def get_users_with_positions(self) -> List[IUser]: + conn = self._get_ldap_conn() + conn.search(self.ldap_users_base, '(position=*)', attributes=ldap3.ALL_ATTRIBUTES) + return [User.deserialize_from_ldap(entry) for entry in conn.entries] + def uid_to_dn(self, uid: str): return f'uid={uid},{self.ldap_users_base}' diff --git a/ceod/model/User.py b/ceod/model/User.py index 7b2c798..5bad209 100644 --- a/ceod/model/User.py +++ b/ceod/model/User.py @@ -67,9 +67,8 @@ class User: 'login_shell': self.login_shell, 'home_directory': self.home_directory, 'is_club': self.is_club(), + 'program': self.program or 'Unknown', } - if self.program: - data['program'] = self.program if self.terms: data['terms'] = self.terms if self.non_member_terms: @@ -158,15 +157,10 @@ class User: entry.nonMemberTerm.add(terms) self.non_member_terms.extend(terms) - def add_position(self, position: str): + def set_positions(self, positions: List[str]): with self.ldap_srv.entry_ctx_for_user(self) as entry: - entry.position.add(position) - self.positions.append(position) - - def remove_position(self, position: str): - with self.ldap_srv.entry_ctx_for_user(self) as entry: - entry.position.delete(position) - self.positions.remove(position) + entry.position = positions + self.positions = positions def get_forwarding_addresses(self) -> List[str]: return self.file_srv.get_forwarding_addresses(self) diff --git a/ceod/transactions/members/UpdateMemberPositionsTransaction.py b/ceod/transactions/members/UpdateMemberPositionsTransaction.py new file mode 100644 index 0000000..32ac77e --- /dev/null +++ b/ceod/transactions/members/UpdateMemberPositionsTransaction.py @@ -0,0 +1,109 @@ +from collections import defaultdict +from typing import Dict + +from zope import component + +from ..AbstractTransaction import AbstractTransaction +from ceo_common.interfaces import ILDAPService, IConfig, IUser +from ceo_common.errors import UserAlreadySubscribedError, UserNotSubscribedError +from ceo_common.logger_factory import logger_factory + +logger = logger_factory(__name__) + + +class UpdateMemberPositionsTransaction(AbstractTransaction): + """Transaction to update the CSC's executive positions.""" + + operations = [ + 'update_positions_ldap', + 'update_exec_group_ldap', + 'subscribe_to_mailing_lists', + ] + + def __init__(self, positions_reversed: Dict[str, str]): + # positions_reversed is position -> username + super().__init__() + self.ldap_srv = component.getUtility(ILDAPService) + + # Reverse the dict so it's easier to use (username -> positions) + self.positions = defaultdict(list) + for position, username in positions_reversed.items(): + self.positions[username].append(position) + + # a cached Dict of the Users who need to be modified (username -> User) + self.users: Dict[str, IUser] = {} + + # for rollback purposes + self.old_positions = {} # username -> positions + self.old_execs = [] + + def child_execute_iter(self): + cfg = component.getUtility(IConfig) + mailing_lists = cfg.get('auxiliary mailing lists_exec') + + # position -> username + new_positions_reversed = {} # For returning result + + # retrieve User objects and cache them + for username in self.positions: + user = self.ldap_srv.get_user(username) + self.users[user.uid] = user + + # Remove positions for old users + for user in self.ldap_srv.get_users_with_positions(): + if user.uid not in self.positions: + self.positions[user.uid] = [] + self.users[user.uid] = user + + # Update positions in LDAP + for username, new_positions in self.positions.items(): + user = self.users[username] + old_positions = user.positions[:] + + user.set_positions(new_positions) + + self.old_positions[username] = old_positions + for position in new_positions: + new_positions_reversed[position] = username + yield 'update_positions_ldap' + + # update exec group in LDAP + exec_group = self.ldap_srv.get_group('exec') + self.old_execs = exec_group.members[:] + new_execs = [ + username for username, new_positions in self.positions.items() + if len(new_positions) > 0 + ] + exec_group.set_members(new_execs) + yield 'update_exec_group_ldap' + + # Update mailing list subscriptions + subscription_failed = False + for username, new_positions in self.positions.items(): + user = self.users[username] + for mailing_list in mailing_lists: + try: + if len(new_positions) > 0: + user.subscribe_to_mailing_list(mailing_list) + else: + user.unsubscribe_from_mailing_list(mailing_list) + except (UserAlreadySubscribedError, UserNotSubscribedError): + pass + except Exception: + logger.warning(f'Failed to update mailing list for {user.uid}') + subscription_failed = True + if subscription_failed: + yield 'failed_to_subscribe_to_mailing_lists' + else: + yield 'subscribe_to_mailing_lists' + + self.finish(new_positions_reversed) + + def rollback(self): + if 'update_exec_group_ldap' in self.finished_operations: + exec_group = self.ldap_srv.get_group('exec') + exec_group.set_members(self.old_execs) + + for username, positions in self.old_positions.items(): + user = self.users[username] + user.set_positions(positions) diff --git a/ceod/transactions/members/__init__.py b/ceod/transactions/members/__init__.py index 8234f15..1443f1f 100644 --- a/ceod/transactions/members/__init__.py +++ b/ceod/transactions/members/__init__.py @@ -2,3 +2,4 @@ from .AddMemberTransaction import AddMemberTransaction from .ModifyMemberTransaction import ModifyMemberTransaction from .RenewMemberTransaction import RenewMemberTransaction from .DeleteMemberTransaction import DeleteMemberTransaction +from .UpdateMemberPositionsTransaction import UpdateMemberPositionsTransaction diff --git a/tests/MockMailmanServer.py b/tests/MockMailmanServer.py index b349f23..561d1ff 100644 --- a/tests/MockMailmanServer.py +++ b/tests/MockMailmanServer.py @@ -35,6 +35,10 @@ class MockMailmanServer: def stop(self): self.loop.call_soon_threadsafe(self.loop.stop) + def clear(self): + for key in self.subscriptions: + self.subscriptions[key].clear() + async def subscribe(self, request): body = await request.post() subscriber = body['subscriber'] diff --git a/tests/ceod/api/test_positions.py b/tests/ceod/api/test_positions.py new file mode 100644 index 0000000..bf6e63e --- /dev/null +++ b/tests/ceod/api/test_positions.py @@ -0,0 +1,93 @@ +from ceod.model import User, Group + + +def test_get_positions(client, ldap_user, g_admin_ctx): + with g_admin_ctx(): + ldap_user.set_positions(['president', 'treasurer']) + status, data = client.get('/api/positions') + assert status == 200 + expected = { + 'president': ldap_user.uid, + 'treasurer': ldap_user.uid, + } + assert data == expected + + +def test_set_positions(cfg, client, g_admin_ctx, mock_mailman_server): + mock_mailman_server.clear() + mailing_lists = cfg.get('auxiliary mailing lists_exec') + base_domain = cfg.get('base_domain') + + users = [] + with g_admin_ctx(): + for uid in ['test_1', 'test_2', 'test_3', 'test_4']: + user = User(uid=uid, cn='Some Name', terms=['s2021']) + user.add_to_ldap() + users.append(user) + exec_group = Group(cn='exec', gid_number=10013) + exec_group.add_to_ldap() + + try: + # missing required position + status, _ = client.post('/api/positions', json={ + 'vice-president': 'test_1', + }) + assert status == 400 + + # non-existent position + status, _ = client.post('/api/positions', json={ + 'president': 'test_1', + 'vice-president': 'test_2', + 'sysadmin': 'test_3', + 'no-such-position': 'test_3', + }) + assert status == 400 + + status, data = client.post('/api/positions', json={ + 'president': 'test_1', + 'vice-president': 'test_2', + 'sysadmin': 'test_3', + }) + assert status == 200 + expected = [ + {"status": "in progress", "operation": "update_positions_ldap"}, + {"status": "in progress", "operation": "update_exec_group_ldap"}, + {"status": "in progress", "operation": "subscribe_to_mailing_lists"}, + {"status": "completed", "result": { + "president": "test_1", + "vice-president": "test_2", + "sysadmin": "test_3", + }}, + ] + assert data == expected + # make sure execs were added to exec group + status, data = client.get('/api/groups/exec') + assert status == 200 + expected = ['test_1', 'test_2', 'test_3'] + assert sorted([item['uid'] for item in data['members']]) == expected + # make sure execs were subscribed to mailing lists + addresses = [f'{uid}@{base_domain}' for uid in expected] + for mailing_list in mailing_lists: + assert sorted(mock_mailman_server.subscriptions[mailing_list]) == addresses + + _, data = client.post('/api/positions', json={ + 'president': 'test_1', + 'vice-president': 'test_2', + 'sysadmin': 'test_2', + 'treasurer': 'test_4', + }) + assert data[-1]['status'] == 'completed' + # make sure old exec was removed from group + expected = ['test_1', 'test_2', 'test_4'] + _, data = client.get('/api/groups/exec') + assert sorted([item['uid'] for item in data['members']]) == expected + # make sure old exec was removed from mailing lists + addresses = [f'{uid}@{base_domain}' for uid in expected] + for mailing_list in mailing_lists: + assert sorted(mock_mailman_server.subscriptions[mailing_list]) == addresses + finally: + with g_admin_ctx(): + for user in users: + user.remove_from_ldap() + exec_group.remove_from_ldap() + mock_mailman_server.clear() diff --git a/tests/ceod/model/test_group.py b/tests/ceod/model/test_group.py index 2dbb3e5..6c4d2f2 100644 --- a/tests/ceod/model/test_group.py +++ b/tests/ceod/model/test_group.py @@ -37,6 +37,10 @@ def test_group_members(ldap_group, ldap_srv): with pytest.raises(UserNotInGroupError): group.remove_member('member1') + group.set_members(['member3']) + assert group.members == ['member3'] + assert ldap_srv.get_group(group.cn).members == group.members + def test_group_to_dict(ldap_group, ldap_user, g_admin_ctx): group = ldap_group diff --git a/tests/ceod/model/test_user.py b/tests/ceod/model/test_user.py index d57c714..fc2c2c2 100644 --- a/tests/ceod/model/test_user.py +++ b/tests/ceod/model/test_user.py @@ -116,16 +116,14 @@ def test_user_terms(ldap_user, ldap_srv): def test_user_positions(ldap_user, ldap_srv): user = ldap_user - user.add_position('treasurer') - assert user.positions == ['treasurer'] - assert ldap_srv.get_user(user.uid).positions == user.positions - user.add_position('cro') - assert user.positions == ['treasurer', 'cro'] + old_positions = user.positions[:] + + new_positions = ['treasurer', 'cro'] + user.set_positions(new_positions) + assert user.positions == new_positions assert ldap_srv.get_user(user.uid).positions == user.positions - user.remove_position('cro') - assert user.positions == ['treasurer'] - assert ldap_srv.get_user(user.uid).positions == user.positions + user.set_positions(old_positions) def test_user_change_password(krb_user): diff --git a/tests/ceod_dev.ini b/tests/ceod_dev.ini index 6e7fd38..f23b130 100644 --- a/tests/ceod_dev.ini +++ b/tests/ceod_dev.ini @@ -52,3 +52,8 @@ office = cdrom,audio,video,www [auxiliary mailing lists] syscom = syscom,syscom-alerts exec = exec + +[positions] +required = president,vice-president,sysadmin +available = president,vice-president,treasurer,secretary, + sysadmin,cro,librarian,imapd,webmaster,offsck diff --git a/tests/ceod_test_local.ini b/tests/ceod_test_local.ini index f14419e..04fdbd9 100644 --- a/tests/ceod_test_local.ini +++ b/tests/ceod_test_local.ini @@ -49,3 +49,8 @@ syscom = office,staff [auxiliary mailing lists] syscom = syscom,syscom-alerts exec = exec + +[positions] +required = president,vice-president,sysadmin +available = president,vice-president,treasurer,secretary, + sysadmin,cro,librarian,imapd,webmaster,offsck