From 319987d635fb65c78dc4bbcf437539c9d5c64dfa Mon Sep 17 00:00:00 2001 From: Max Erenberg Date: Wed, 18 Aug 2021 01:59:24 +0000 Subject: [PATCH 01/12] add Kerberos delegation --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index b286421..fa9bdbc 100644 --- a/README.md +++ b/README.md @@ -38,7 +38,7 @@ TODO - Andrew #### Dependencies Next, install and activate a virtualenv: ```sh -sudo apt install libkrb5-dev libsasl2-dev python3-dev +sudo apt install libkrb5-dev python3-dev python3 -m venv venv . venv/bin/activate pip install -r requirements.txt -- 2.39.2 From 467c28d193cf8701869e81c85fc72af04b9ad5b4 Mon Sep 17 00:00:00 2001 From: Rio Liu Date: Tue, 17 Aug 2021 00:43:33 -0400 Subject: [PATCH 02/12] positions api --- ceo_common/interfaces/ILDAPService.py | 3 ++ ceod/api/app_factory.py | 3 +- ceod/api/positions.py | 41 +++++++++++++++++++ ceod/model/LDAPService.py | 5 +++ ceod/model/User.py | 5 +++ .../UpdateMemberPositionsTransaction.py | 29 +++++++++++++ ceod/transactions/members/__init__.py | 1 + 7 files changed, 86 insertions(+), 1 deletion(-) create mode 100644 ceod/api/positions.py create mode 100644 ceod/transactions/members/UpdateMemberPositionsTransaction.py diff --git a/ceo_common/interfaces/ILDAPService.py b/ceo_common/interfaces/ILDAPService.py index da10a12..ee8e795 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 has their position set""" + def add_user(user: IUser): """ Add the user to the database. diff --git a/ceod/api/app_factory.py b/ceod/api/app_factory.py index a7bc66d..04a1195 100644 --- a/ceod/api/app_factory.py +++ b/ceod/api/app_factory.py @@ -31,8 +31,9 @@ def create_app(flask_config={}): # Only ceod_admin_host should serve the /api/members endpoints because # it needs to run kadmin if hostname == cfg.get('ceod_admin_host'): - from ceod.api import members + from ceod.api import members, positions app.register_blueprint(members.bp, url_prefix='/api/members') + app.register_blueprint(positions.bp, url_prefix='/api/positions') # Only offer mailman API if this host is running Mailman if hostname == cfg.get('ceod_mailman_host'): diff --git a/ceod/api/positions.py b/ceod/api/positions.py new file mode 100644 index 0000000..d3608dd --- /dev/null +++ b/ceod/api/positions.py @@ -0,0 +1,41 @@ +import json +from flask import Blueprint, request +from zope import component + +from ceod.transactions.members import UpdateMemberPositionsTransaction +from .utils import authz_restrict_to_syscom, requires_authentication_no_realm, create_streaming_response +from ceo_common.interfaces import ILDAPService + +bp = Blueprint('positions', __name__) + +@bp.route('/', methods=['GET']) +@requires_authentication_no_realm +def get_positions(auth_user: str): + ldap_srv = component.getUtility(ILDAPService) + + users = ldap_srv.get_users_with_positions() + + positions = {} + for user in users: + for position in user.positions: + positions[position] = user.uid + + return json.dumps(positions) + +@bp.route('/', methods=['POST']) +@authz_restrict_to_syscom +def update_positions(): + body = request.get_json(force=True) + # TODO verify json + + # Reverse the dict so it's easier to use + member_positions = {} + for position, user in body.items(): + if user not in member_positions: + member_positions[user] = [] + member_positions[user].append(position) + + txn = UpdateMemberPositionsTransaction(member_positions) + return create_streaming_response(txn) + + # TODO un/subscribe to mailing list diff --git a/ceod/model/LDAPService.py b/ceod/model/LDAPService.py index 8c91fc2..51b9ee4 100644 --- a/ceod/model/LDAPService.py +++ b/ceod/model/LDAPService.py @@ -101,6 +101,11 @@ class LDAPService: for entry in conn.entries ] + def get_users_with_positions(self) -> List[IUser]: + conn = self._get_ldap_conn(False) + 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..9833e81 100644 --- a/ceod/model/User.py +++ b/ceod/model/User.py @@ -168,6 +168,11 @@ class User: entry.position.delete(position) self.positions.remove(position) + def set_positions(self, positions: List[str]): + with self.ldap_srv.entry_ctx_for_user(self) as entry: + 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..6b67e9a --- /dev/null +++ b/ceod/transactions/members/UpdateMemberPositionsTransaction.py @@ -0,0 +1,29 @@ +from typing import List, Dict + +from zope import component + +from ..AbstractTransaction import AbstractTransaction +from ceo_common.interfaces import ILDAPService + +class UpdateMemberPositionsTransaction(AbstractTransaction): + """ + Transaction to update member's positions, and remove positions for anyone that's not in the list + """ + + # Positions is a dict where keys are member names and values are the list of positions they have + def __init__(self, positions: Dict[str, List[str]]): + super().__init__() + self.positions = positions + self.ldap_srv = component.getUtility(ILDAPService) + self.old_positions = {} + + def child_execute_iter(self): + for username, positions in self.positions.items(): + user = self.ldap_srv.get_user(username) + user.set_positions(positions) + yield f'update_positions_{username}' + self.finish(None) + + def rollback(self): + # TODO + pass 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 -- 2.39.2 From aafe6439bfc04a19850b01e0358208800f69c4d0 Mon Sep 17 00:00:00 2001 From: Rio6 Date: Tue, 17 Aug 2021 15:09:00 -0400 Subject: [PATCH 03/12] fix ldap authentication and add rollback to UpdateMemberPositionsTransaction --- ceod/model/LDAPService.py | 10 ++-------- .../members/UpdateMemberPositionsTransaction.py | 17 +++++++++++++---- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/ceod/model/LDAPService.py b/ceod/model/LDAPService.py index 51b9ee4..2487ac3 100644 --- a/ceod/model/LDAPService.py +++ b/ceod/model/LDAPService.py @@ -102,7 +102,7 @@ class LDAPService: ] def get_users_with_positions(self) -> List[IUser]: - conn = self._get_ldap_conn(False) + 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] @@ -274,10 +274,4 @@ class LDAPService: ) ] if dry_run: - return users_to_change - - for uid, old_program, new_program in users_to_change: - changes = {'program': [(ldap3.MODIFY_REPLACE, [new_program])]} - conn.modify(self.uid_to_dn(uid), changes) - - return users_to_change + return user diff --git a/ceod/transactions/members/UpdateMemberPositionsTransaction.py b/ceod/transactions/members/UpdateMemberPositionsTransaction.py index 6b67e9a..5816adf 100644 --- a/ceod/transactions/members/UpdateMemberPositionsTransaction.py +++ b/ceod/transactions/members/UpdateMemberPositionsTransaction.py @@ -15,15 +15,24 @@ class UpdateMemberPositionsTransaction(AbstractTransaction): super().__init__() self.positions = positions self.ldap_srv = component.getUtility(ILDAPService) - self.old_positions = {} + self.old_positions = {} # For rollback def child_execute_iter(self): + new_positions = {} + for username, positions in self.positions.items(): user = self.ldap_srv.get_user(username) + self.old_positions[username] = user.positions[:] user.set_positions(positions) + + for position in user.positions: + new_positions[position] = user.uid + yield f'update_positions_{username}' - self.finish(None) + + self.finish(new_positions) def rollback(self): - # TODO - pass + for username, positions in self.old_positions.items(): + user = self.ldap_srv.get_user(username) + user.set_positions(positions) -- 2.39.2 From 9e119a5f73530ab492fa09823f0ded48e4b838c4 Mon Sep 17 00:00:00 2001 From: Rio6 Date: Tue, 17 Aug 2021 15:38:12 -0400 Subject: [PATCH 04/12] add code to remove positions --- .../members/UpdateMemberPositionsTransaction.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/ceod/transactions/members/UpdateMemberPositionsTransaction.py b/ceod/transactions/members/UpdateMemberPositionsTransaction.py index 5816adf..84f3a61 100644 --- a/ceod/transactions/members/UpdateMemberPositionsTransaction.py +++ b/ceod/transactions/members/UpdateMemberPositionsTransaction.py @@ -20,6 +20,12 @@ class UpdateMemberPositionsTransaction(AbstractTransaction): def child_execute_iter(self): new_positions = {} + # 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] = [] + + # Update positions via ldap for username, positions in self.positions.items(): user = self.ldap_srv.get_user(username) self.old_positions[username] = user.positions[:] -- 2.39.2 From 6e30437b6154a5b71f82411c911e9f37e3b899d6 Mon Sep 17 00:00:00 2001 From: Rio6 Date: Wed, 18 Aug 2021 16:23:55 -0400 Subject: [PATCH 05/12] implement mailing subscription for positions api --- ceod/api/positions.py | 2 -- .../UpdateMemberPositionsTransaction.py | 36 ++++++++++++++----- 2 files changed, 27 insertions(+), 11 deletions(-) diff --git a/ceod/api/positions.py b/ceod/api/positions.py index d3608dd..ce2b894 100644 --- a/ceod/api/positions.py +++ b/ceod/api/positions.py @@ -37,5 +37,3 @@ def update_positions(): txn = UpdateMemberPositionsTransaction(member_positions) return create_streaming_response(txn) - - # TODO un/subscribe to mailing list diff --git a/ceod/transactions/members/UpdateMemberPositionsTransaction.py b/ceod/transactions/members/UpdateMemberPositionsTransaction.py index 84f3a61..d570328 100644 --- a/ceod/transactions/members/UpdateMemberPositionsTransaction.py +++ b/ceod/transactions/members/UpdateMemberPositionsTransaction.py @@ -3,22 +3,30 @@ from typing import List, Dict from zope import component from ..AbstractTransaction import AbstractTransaction -from ceo_common.interfaces import ILDAPService +from ceo_common.interfaces import ILDAPService, IConfig +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 member's positions, and remove positions for anyone that's not in the list + Transaction to update member's positions, and remove positions for anyone that's not in the list, + then subscribe new execs to mailing list and unsubscribe old execs from it. """ # Positions is a dict where keys are member names and values are the list of positions they have def __init__(self, positions: Dict[str, List[str]]): super().__init__() self.positions = positions - self.ldap_srv = component.getUtility(ILDAPService) self.old_positions = {} # For rollback + self.ldap_srv = component.getUtility(ILDAPService) def child_execute_iter(self): - new_positions = {} + cfg = component.getUtility(IConfig) + mailing_lists = cfg.get('auxiliary mailing lists_exec') + + subscribe_status: Dict[IUser, bool] = {} # Remove positions for old users for user in self.ldap_srv.get_users_with_positions(): @@ -30,13 +38,23 @@ class UpdateMemberPositionsTransaction(AbstractTransaction): user = self.ldap_srv.get_user(username) self.old_positions[username] = user.positions[:] user.set_positions(positions) - - for position in user.positions: - new_positions[position] = user.uid - + subscribe_status[user] = len(positions) > 0 yield f'update_positions_{username}' - self.finish(new_positions) + # Update mailing list subscription + for user, subscribe in subscribe_status.items(): + for mailing_list in mailing_lists: + try: + if subscribe: + user.subscribe_to_mailing_list(mailing_list) + else: + user.unsubscribe_from_mailing_list(mailing_list) + except (UserAlreadySubscribedError, UserNotSubscribedError): + pass + except Exception as e: + logger.warning(f'Failed to update mailing list for {user.uid}') + + self.finish(None) def rollback(self): for username, positions in self.old_positions.items(): -- 2.39.2 From e7772d2564e1793ce98d9466564bf76a05068905 Mon Sep 17 00:00:00 2001 From: Rio6 Date: Wed, 18 Aug 2021 16:41:11 -0400 Subject: [PATCH 06/12] make POST on positions api return result again --- ceod/api/positions.py | 9 +-------- .../UpdateMemberPositionsTransaction.py | 18 +++++++++++++++--- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/ceod/api/positions.py b/ceod/api/positions.py index ce2b894..812ec80 100644 --- a/ceod/api/positions.py +++ b/ceod/api/positions.py @@ -28,12 +28,5 @@ def update_positions(): body = request.get_json(force=True) # TODO verify json - # Reverse the dict so it's easier to use - member_positions = {} - for position, user in body.items(): - if user not in member_positions: - member_positions[user] = [] - member_positions[user].append(position) - - txn = UpdateMemberPositionsTransaction(member_positions) + txn = UpdateMemberPositionsTransaction(body) return create_streaming_response(txn) diff --git a/ceod/transactions/members/UpdateMemberPositionsTransaction.py b/ceod/transactions/members/UpdateMemberPositionsTransaction.py index d570328..8b8581a 100644 --- a/ceod/transactions/members/UpdateMemberPositionsTransaction.py +++ b/ceod/transactions/members/UpdateMemberPositionsTransaction.py @@ -16,17 +16,24 @@ class UpdateMemberPositionsTransaction(AbstractTransaction): """ # Positions is a dict where keys are member names and values are the list of positions they have - def __init__(self, positions: Dict[str, List[str]]): + def __init__(self, positions_reversed: Dict[str, List[str]]): super().__init__() - self.positions = positions self.old_positions = {} # For rollback self.ldap_srv = component.getUtility(ILDAPService) + # Reverse the dict so it's easier to use + self.positions = {} + for position, user in positions_reversed.items(): + if user not in self.positions: + self.positions[user] = [] + self.positions[user].append(position) + def child_execute_iter(self): cfg = component.getUtility(IConfig) mailing_lists = cfg.get('auxiliary mailing lists_exec') subscribe_status: Dict[IUser, bool] = {} + new_positions_reversed: Dict[str, str] = {} # For returning result # Remove positions for old users for user in self.ldap_srv.get_users_with_positions(): @@ -38,7 +45,12 @@ class UpdateMemberPositionsTransaction(AbstractTransaction): user = self.ldap_srv.get_user(username) self.old_positions[username] = user.positions[:] user.set_positions(positions) + subscribe_status[user] = len(positions) > 0 + + for position in user.positions: + new_positions_reversed[position] = user.uid + yield f'update_positions_{username}' # Update mailing list subscription @@ -54,7 +66,7 @@ class UpdateMemberPositionsTransaction(AbstractTransaction): except Exception as e: logger.warning(f'Failed to update mailing list for {user.uid}') - self.finish(None) + self.finish(new_positions_reversed) def rollback(self): for username, positions in self.old_positions.items(): -- 2.39.2 From 08c4bf2e3655ee44d46b531702153425991a5415 Mon Sep 17 00:00:00 2001 From: Rio6 Date: Wed, 18 Aug 2021 16:52:47 -0400 Subject: [PATCH 07/12] add input validation to positions api --- ceod/api/positions.py | 23 ++++++++++++++++++----- tests/ceod_dev.ini | 4 ++++ 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/ceod/api/positions.py b/ceod/api/positions.py index 812ec80..46fdf8e 100644 --- a/ceod/api/positions.py +++ b/ceod/api/positions.py @@ -4,7 +4,7 @@ from zope import component from ceod.transactions.members import UpdateMemberPositionsTransaction from .utils import authz_restrict_to_syscom, requires_authentication_no_realm, create_streaming_response -from ceo_common.interfaces import ILDAPService +from ceo_common.interfaces import ILDAPService, IConfig bp = Blueprint('positions', __name__) @@ -13,10 +13,8 @@ bp = Blueprint('positions', __name__) def get_positions(auth_user: str): ldap_srv = component.getUtility(ILDAPService) - users = ldap_srv.get_users_with_positions() - positions = {} - for user in users: + for user in ldap_srv.get_users_with_positions(): for position in user.positions: positions[position] = user.uid @@ -25,8 +23,23 @@ def get_positions(auth_user: str): @bp.route('/', methods=['POST']) @authz_restrict_to_syscom def update_positions(): + cfg = component.getUtility(IConfig) body = request.get_json(force=True) - # TODO verify json + + required = cfg.get('auxiliary positions_required') + available = cfg.get('auxiliary positions_available') + + for position in body.keys(): + if position not in available: + return { + 'error': f'unknown position: {position}' + }, 404 + + 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/tests/ceod_dev.ini b/tests/ceod_dev.ini index 6e7fd38..8527421 100644 --- a/tests/ceod_dev.ini +++ b/tests/ceod_dev.ini @@ -52,3 +52,7 @@ office = cdrom,audio,video,www [auxiliary mailing lists] syscom = syscom,syscom-alerts exec = exec + +[auxiliary positions] +required = president,vice-president,sysadmin +available = president,vice-president,treasurer,secretary,sysadmin,cro,librarian,imapd,webmaster,offsck -- 2.39.2 From 0ed876c0101a8f113d932e3e4f3dcd1600ba3e58 Mon Sep 17 00:00:00 2001 From: Rio6 Date: Fri, 20 Aug 2021 23:43:35 -0400 Subject: [PATCH 08/12] fix rebase --- README.md | 2 +- ceod/model/LDAPService.py | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index fa9bdbc..b286421 100644 --- a/README.md +++ b/README.md @@ -38,7 +38,7 @@ TODO - Andrew #### Dependencies Next, install and activate a virtualenv: ```sh -sudo apt install libkrb5-dev python3-dev +sudo apt install libkrb5-dev libsasl2-dev python3-dev python3 -m venv venv . venv/bin/activate pip install -r requirements.txt diff --git a/ceod/model/LDAPService.py b/ceod/model/LDAPService.py index 2487ac3..53c418d 100644 --- a/ceod/model/LDAPService.py +++ b/ceod/model/LDAPService.py @@ -274,4 +274,10 @@ class LDAPService: ) ] if dry_run: - return user + return users_to_change + + for uid, old_program, new_program in users_to_change: + changes = {'program': [(ldap3.MODIFY_REPLACE, [new_program])]} + conn.modify(self.uid_to_dn(uid), changes) + + return users_to_change -- 2.39.2 From 2c150193fcb08d136d3cd881e48d79ab548b1207 Mon Sep 17 00:00:00 2001 From: Max Erenberg Date: Sat, 21 Aug 2021 20:27:45 +0000 Subject: [PATCH 09/12] cache users in transaction --- ceo_common/errors.py | 8 +- ceod/api/positions.py | 17 +++-- ceod/model/LDAPService.py | 4 +- .../UpdateMemberPositionsTransaction.py | 74 +++++++++++-------- tests/ceod_dev.ini | 2 +- 5 files changed, 60 insertions(+), 45 deletions(-) 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/ceod/api/positions.py b/ceod/api/positions.py index 46fdf8e..d8871fc 100644 --- a/ceod/api/positions.py +++ b/ceod/api/positions.py @@ -2,15 +2,15 @@ import json from flask import Blueprint, request from zope import component -from ceod.transactions.members import UpdateMemberPositionsTransaction -from .utils import authz_restrict_to_syscom, requires_authentication_no_realm, create_streaming_response +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']) -@requires_authentication_no_realm -def get_positions(auth_user: str): + +@bp.route('/', methods=['GET'], strict_slashes=False) +def get_positions(): ldap_srv = component.getUtility(ILDAPService) positions = {} @@ -20,14 +20,15 @@ def get_positions(auth_user: str): return json.dumps(positions) -@bp.route('/', methods=['POST']) + +@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('auxiliary positions_required') - available = cfg.get('auxiliary positions_available') + required = cfg.get('positions_required') + available = cfg.get('positions_available') for position in body.keys(): if position not in available: diff --git a/ceod/model/LDAPService.py b/ceod/model/LDAPService.py index 53c418d..9ddd1e6 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: diff --git a/ceod/transactions/members/UpdateMemberPositionsTransaction.py b/ceod/transactions/members/UpdateMemberPositionsTransaction.py index 8b8581a..9b61dcf 100644 --- a/ceod/transactions/members/UpdateMemberPositionsTransaction.py +++ b/ceod/transactions/members/UpdateMemberPositionsTransaction.py @@ -1,60 +1,73 @@ -from typing import List, Dict +from collections import defaultdict +from typing import Dict from zope import component from ..AbstractTransaction import AbstractTransaction -from ceo_common.interfaces import ILDAPService, IConfig +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 member's positions, and remove positions for anyone that's not in the list, - then subscribe new execs to mailing list and unsubscribe old execs from it. - """ - # Positions is a dict where keys are member names and values are the list of positions they have - def __init__(self, positions_reversed: Dict[str, List[str]]): +class UpdateMemberPositionsTransaction(AbstractTransaction): + """Transaction to update the CSC's executive positions.""" + + operations = [ + 'update_positions_ldap', + 'subscribe_to_mailing_lists', + ] + + def __init__(self, positions_reversed: Dict[str, str]): + # positions_reversed is position -> username super().__init__() - self.old_positions = {} # For rollback + # self.old_positions is username -> positions + self.old_positions = {} # For rollback self.ldap_srv = component.getUtility(ILDAPService) - # Reverse the dict so it's easier to use - self.positions = {} - for position, user in positions_reversed.items(): - if user not in self.positions: - self.positions[user] = [] - self.positions[user].append(position) + # 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] = {} def child_execute_iter(self): cfg = component.getUtility(IConfig) mailing_lists = cfg.get('auxiliary mailing lists_exec') - subscribe_status: Dict[IUser, bool] = {} - new_positions_reversed: Dict[str, str] = {} # For returning result + subscribe_status: Dict[str, bool] = {} + # 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 via ldap - for username, positions in self.positions.items(): - user = self.ldap_srv.get_user(username) - self.old_positions[username] = user.positions[:] - user.set_positions(positions) + for username, user in self.users.items(): + new_positions = self.positions[username] + old_positions = user.positions[:] - subscribe_status[user] = len(positions) > 0 + user.set_positions(new_positions) - for position in user.positions: - new_positions_reversed[position] = user.uid - - yield f'update_positions_{username}' + self.old_positions[username] = old_positions + for position in new_positions: + new_positions_reversed[position] = username + subscribe_status[username] = len(new_positions) > 0 + yield 'update_positions_ldap' # Update mailing list subscription - for user, subscribe in subscribe_status.items(): + for username, subscribe in subscribe_status.items(): for mailing_list in mailing_lists: try: if subscribe: @@ -63,12 +76,13 @@ class UpdateMemberPositionsTransaction(AbstractTransaction): user.unsubscribe_from_mailing_list(mailing_list) except (UserAlreadySubscribedError, UserNotSubscribedError): pass - except Exception as e: + except Exception: logger.warning(f'Failed to update mailing list for {user.uid}') + yield 'subscribe_to_mailing_lists' self.finish(new_positions_reversed) def rollback(self): for username, positions in self.old_positions.items(): - user = self.ldap_srv.get_user(username) + user = self.users[username] user.set_positions(positions) diff --git a/tests/ceod_dev.ini b/tests/ceod_dev.ini index 8527421..b57dfd0 100644 --- a/tests/ceod_dev.ini +++ b/tests/ceod_dev.ini @@ -53,6 +53,6 @@ office = cdrom,audio,video,www syscom = syscom,syscom-alerts exec = exec -[auxiliary positions] +[positions] required = president,vice-president,sysadmin available = president,vice-president,treasurer,secretary,sysadmin,cro,librarian,imapd,webmaster,offsck -- 2.39.2 From 5e8415ad5d1188cfa745713e2e672679c5775374 Mon Sep 17 00:00:00 2001 From: Max Erenberg Date: Sun, 22 Aug 2021 01:27:39 +0000 Subject: [PATCH 10/12] add new execs to exec group --- ceo_common/interfaces/IGroup.py | 5 +++ ceo_common/interfaces/ILDAPService.py | 2 +- ceo_common/model/Config.py | 2 +- ceod/api/app_factory.py | 6 ++-- ceod/model/Group.py | 7 ++++ ceod/model/LDAPService.py | 2 +- .../UpdateMemberPositionsTransaction.py | 36 +++++++++++++------ 7 files changed, 45 insertions(+), 15 deletions(-) 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 ee8e795..89424f4 100644 --- a/ceo_common/interfaces/ILDAPService.py +++ b/ceo_common/interfaces/ILDAPService.py @@ -25,7 +25,7 @@ class ILDAPService(Interface): """ def get_users_with_positions(self) -> List[IUser]: - """Retrieve users who has their position set""" + """Retrieve users who have a non-empty position attribute.""" def add_user(user: IUser): """ diff --git a/ceo_common/model/Config.py b/ceo_common/model/Config.py index f2ff94e..b78b58f 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 '): + if section.startswith('auxiliary ') or section == 'positions': return val.split(',') return val diff --git a/ceod/api/app_factory.py b/ceod/api/app_factory.py index 8530e5c..f033a2b 100644 --- a/ceod/api/app_factory.py +++ b/ceod/api/app_factory.py @@ -29,9 +29,8 @@ def create_app(flask_config={}): # Only ceod_admin_host should serve the /api/members endpoints because # it needs to run kadmin if hostname == cfg.get('ceod_admin_host'): - from ceod.api import members, positions + from ceod.api import members app.register_blueprint(members.bp, url_prefix='/api/members') - app.register_blueprint(positions.bp, url_prefix='/api/positions') # Only offer mailman API if this host is running Mailman if hostname == cfg.get('ceod_mailman_host'): @@ -41,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/model/Group.py b/ceod/model/Group.py index cf99e57..695a8f0 100644 --- a/ceod/model/Group.py +++ b/ceod/model/Group.py @@ -105,3 +105,10 @@ 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 diff --git a/ceod/model/LDAPService.py b/ceod/model/LDAPService.py index 9ddd1e6..218f971 100644 --- a/ceod/model/LDAPService.py +++ b/ceod/model/LDAPService.py @@ -96,7 +96,7 @@ 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 ] diff --git a/ceod/transactions/members/UpdateMemberPositionsTransaction.py b/ceod/transactions/members/UpdateMemberPositionsTransaction.py index 9b61dcf..6cbd1b4 100644 --- a/ceod/transactions/members/UpdateMemberPositionsTransaction.py +++ b/ceod/transactions/members/UpdateMemberPositionsTransaction.py @@ -16,14 +16,13 @@ class UpdateMemberPositionsTransaction(AbstractTransaction): 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.old_positions is username -> positions - self.old_positions = {} # For rollback self.ldap_srv = component.getUtility(ILDAPService) # Reverse the dict so it's easier to use (username -> positions) @@ -34,11 +33,14 @@ class UpdateMemberPositionsTransaction(AbstractTransaction): # 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') - subscribe_status: Dict[str, bool] = {} # position -> username new_positions_reversed = {} # For returning result @@ -53,9 +55,9 @@ class UpdateMemberPositionsTransaction(AbstractTransaction): self.positions[user.uid] = [] self.users[user.uid] = user - # Update positions via ldap - for username, user in self.users.items(): - new_positions = self.positions[username] + # 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) @@ -63,14 +65,24 @@ class UpdateMemberPositionsTransaction(AbstractTransaction): self.old_positions[username] = old_positions for position in new_positions: new_positions_reversed[position] = username - subscribe_status[username] = len(new_positions) > 0 yield 'update_positions_ldap' - # Update mailing list subscription - for username, subscribe in subscribe_status.items(): + # 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 + for username, new_positions in self.positions.items(): + user = self.users[username] for mailing_list in mailing_lists: try: - if subscribe: + if len(new_positions) > 0: user.subscribe_to_mailing_list(mailing_list) else: user.unsubscribe_from_mailing_list(mailing_list) @@ -83,6 +95,10 @@ class UpdateMemberPositionsTransaction(AbstractTransaction): 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) -- 2.39.2 From f497612399297670688d03a692acbc9b2828c3a6 Mon Sep 17 00:00:00 2001 From: Max Erenberg Date: Sun, 22 Aug 2021 04:13:38 +0000 Subject: [PATCH 11/12] add unit tests --- ceo_common/interfaces/IUser.py | 7 +-- ceo_common/model/Config.py | 2 +- ceod/api/positions.py | 5 +- ceod/model/Group.py | 1 + ceod/model/User.py | 13 +---- tests/MockMailmanServer.py | 4 ++ tests/ceod/api/test_positions.py | 93 ++++++++++++++++++++++++++++++++ tests/ceod/model/test_group.py | 4 ++ tests/ceod/model/test_user.py | 14 +++-- tests/ceod_dev.ini | 3 +- tests/ceod_test_local.ini | 5 ++ 11 files changed, 121 insertions(+), 30 deletions(-) create mode 100644 tests/ceod/api/test_positions.py 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 b78b58f..dfe7bbc 100644 --- a/ceo_common/model/Config.py +++ b/ceo_common/model/Config.py @@ -28,5 +28,5 @@ class Config: if val.lower() in ['false', 'no']: return False if section.startswith('auxiliary ') or section == 'positions': - return val.split(',') + return [item.strip() for item in val.split(',')] return val diff --git a/ceod/api/positions.py b/ceod/api/positions.py index d8871fc..9d9f4c4 100644 --- a/ceod/api/positions.py +++ b/ceod/api/positions.py @@ -1,4 +1,3 @@ -import json from flask import Blueprint, request from zope import component @@ -18,7 +17,7 @@ def get_positions(): for position in user.positions: positions[position] = user.uid - return json.dumps(positions) + return positions @bp.route('/', methods=['POST'], strict_slashes=False) @@ -34,7 +33,7 @@ def update_positions(): if position not in available: return { 'error': f'unknown position: {position}' - }, 404 + }, 400 for position in required: if position not in body: diff --git a/ceod/model/Group.py b/ceod/model/Group.py index 695a8f0..3493ce3 100644 --- a/ceod/model/Group.py +++ b/ceod/model/Group.py @@ -112,3 +112,4 @@ class Group: ] with self.ldap_srv.entry_ctx_for_group(self) as entry: entry.uniqueMember = DNs + self.members = usernames diff --git a/ceod/model/User.py b/ceod/model/User.py index 9833e81..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,16 +157,6 @@ class User: entry.nonMemberTerm.add(terms) self.non_member_terms.extend(terms) - def add_position(self, position: 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) - def set_positions(self, positions: List[str]): with self.ldap_srv.entry_ctx_for_user(self) as entry: entry.position = positions 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 b57dfd0..f23b130 100644 --- a/tests/ceod_dev.ini +++ b/tests/ceod_dev.ini @@ -55,4 +55,5 @@ exec = exec [positions] required = president,vice-president,sysadmin -available = president,vice-president,treasurer,secretary,sysadmin,cro,librarian,imapd,webmaster,offsck +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 -- 2.39.2 From ca3ada5886925a6916b3e2f6a8d1b17b7aab8d60 Mon Sep 17 00:00:00 2001 From: Max Erenberg Date: Sun, 22 Aug 2021 06:07:56 +0000 Subject: [PATCH 12/12] add failed_to_subscribe_to_mailing_lists --- .../members/UpdateMemberPositionsTransaction.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/ceod/transactions/members/UpdateMemberPositionsTransaction.py b/ceod/transactions/members/UpdateMemberPositionsTransaction.py index 6cbd1b4..32ac77e 100644 --- a/ceod/transactions/members/UpdateMemberPositionsTransaction.py +++ b/ceod/transactions/members/UpdateMemberPositionsTransaction.py @@ -78,6 +78,7 @@ class UpdateMemberPositionsTransaction(AbstractTransaction): 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: @@ -90,7 +91,11 @@ class UpdateMemberPositionsTransaction(AbstractTransaction): pass except Exception: logger.warning(f'Failed to update mailing list for {user.uid}') - yield 'subscribe_to_mailing_lists' + subscription_failed = True + if subscription_failed: + yield 'failed_to_subscribe_to_mailing_lists' + else: + yield 'subscribe_to_mailing_lists' self.finish(new_positions_reversed) -- 2.39.2