Positions API (#7)

Co-authored-by: Max Erenberg <merenber@csclub.uwaterloo.ca>
Co-authored-by: Rio Liu <r345liu@csclub.uwaterloo.ca>
Co-authored-by: Rio6 <rio.liu@r26.me>
Reviewed-on: #7
Co-authored-by: Rio <r345liu@localhost>
Co-committed-by: Rio <r345liu@localhost>

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):

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."""

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.

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."""

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

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')
 

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)

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

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}'
 

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)

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)

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

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']

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()

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

tests/ceod/model/test_user.py

@@ -116,17 +116,15 @@ 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']
-    assert ldap_srv.get_user(user.uid).positions == user.positions
+    old_positions = user.positions[:]
 
-    user.remove_position('cro')
-    assert user.positions == ['treasurer']
+    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.set_positions(old_positions)
+
 
 def test_user_change_password(krb_user):
     user = krb_user

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

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