use list to represent multiple users for a given position

ceo/cli/positions.py

@@ -16,13 +16,22 @@ def positions():
 def get():
     resp = http_get('/api/positions')
     result = handle_sync_response(resp)
-    print_colon_kv(result.items())
+    print_colon_kv([
+        (position, ', '.join(usernames))
+        for position, usernames in result.items()
+    ])
 
 
 @positions.command(short_help='Update positions')
 def set(**kwargs):
-    body = {k.replace('_', '-'): v for k, v in kwargs.items()}
-    print_body = {k: v or '' for k, v in body.items()}
+    body = {
+        k.replace('_', '-'): v.replace(' ', '').split(',') if v else None
+        for k, v in kwargs.items()
+    }
+    print_body = {
+        k: ', '.join(v) if v else ''
+        for k, v in body.items()
+    }
     click.echo('The positions will be updated:')
     print_colon_kv(print_body.items())
     click.confirm('Do you want to continue?', abort=True)

ceo/tui/controllers/GetPositionsController.py

@@ -19,8 +19,8 @@ class GetPositionsController(Controller):
             positions = tui_utils.handle_sync_response(resp, self)
         except Controller.RequestFailed:
             return
-        for pos, username in positions.items():
-            self.model.positions[pos] = username
+        for pos, usernames in positions.items():
+            self.model.positions[pos] = ','.join(usernames)
 
         def target():
             self.view.flash_text.set_text('')

ceo/tui/controllers/SetPositionsController.py

@@ -17,7 +17,7 @@ class SetPositionsController(Controller):
         body = {}
         for pos, field in self.view.position_fields.items():
             if field.edit_text != '':
-                body[pos] = field.edit_text
+                body[pos] = field.edit_text.replace(' ', '').split(',')
         model = TransactionModel(
             UpdateMemberPositionsTransaction.operations,
             'POST', '/api/positions', json=body
@@ -37,8 +37,8 @@ class SetPositionsController(Controller):
             positions = tui_utils.handle_sync_response(resp, self)
         except Controller.RequestFailed:
             return
-        for pos, username in positions.items():
-            self.model.positions[pos] = username
+        for pos, usernames in positions.items():
+            self.model.positions[pos] = ','.join(usernames)
 
         def target():
             self.view.flash_text.set_text('')

ceo/utils.py

@@ -87,6 +87,9 @@ def space_colon_kv(pairs: List[Tuple[str, str]]) -> List[str]:
     maxlen = max(len(key) for key, val in pairs)
     for key, val in pairs:
         if key != '':
+            if not val:
+                lines.append(key + ':')
+                continue
             prefix = key + ': '
         else:
             # assume this is a continuation from the previous line

ceod/api/positions.py

@@ -2,6 +2,7 @@ from flask import Blueprint, request
 from zope import component
 
 from .utils import authz_restrict_to_syscom, create_streaming_response
+from ceo_common.errors import BadRequest
 from ceo_common.interfaces import ILDAPService, IConfig
 from ceod.transactions.members import UpdateMemberPositionsTransaction
 
@@ -15,10 +16,9 @@ def get_positions():
     positions = {}
     for user in ldap_srv.get_users_with_positions():
         for position in user.positions:
-            if position in positions:
-                positions[position] += f", {user.uid}"
-            else:
-                positions[position] = user.uid
+            if position not in positions:
+                positions[position] = []
+            positions[position].append(user.uid)
 
     return positions
 
@@ -34,22 +34,29 @@ def update_positions():
 
     # remove falsy values and parse multiple users in each position
     # Example: "user1,user2, user3" -> ["user1","user2","user3"]
-    body = {
-        positions: username.replace(' ', '').split(',') for positions, username in body.items()
-        if username
-    }
-
-    for position in body.keys():
+    position_to_usernames = {}
+    for position, usernames in body.items():
+        if not usernames:
+            continue
+        if type(usernames) is list:
+            position_to_usernames[position] = usernames
+        elif type(usernames) is str:
+            position_to_usernames[position] = usernames.replace(' ', '').split(',')
+        else:
+            raise BadRequest('usernames must be a list or comma-separated string')
+
+    # check for duplicates (i.e. one username specified twice in the same list)
+    for usernames in position_to_usernames.values():
+        if len(usernames) != len(set(usernames)):
+            raise BadRequest('username may only be specified at most once for a position')
+
+    for position in position_to_usernames.keys():
         if position not in available:
-            return {
-                'error': f'unknown position: {position}'
-            }, 400
+            raise BadRequest(f'unknown position: {position}')
 
     for position in required:
-        if position not in body:
-            return {
-                'error': f'missing required position: {position}'
-            }, 400
+        if position not in position_to_usernames:
+            raise BadRequest(f'missing required position: {position}')
 
-    txn = UpdateMemberPositionsTransaction(body)
+    txn = UpdateMemberPositionsTransaction(position_to_usernames)
     return create_streaming_response(txn)

ceod/transactions/members/UpdateMemberPositionsTransaction.py

@@ -1,8 +1,7 @@
 from collections import defaultdict
-from typing import Dict
+from typing import Dict, List
 
 from zope import component
-from typing import Union
 
 from ..AbstractTransaction import AbstractTransaction
 from ceo_common.interfaces import ILDAPService, IConfig, IUser
@@ -21,21 +20,19 @@ class UpdateMemberPositionsTransaction(AbstractTransaction):
         'subscribe_to_mailing_lists',
     ]
 
-    def __init__(self, positions_reversed: Dict[str, Union[str, list]]):
+    def __init__(self, position_to_usernames: Dict[str, List[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():
-            if isinstance(username, str):
-                self.positions[username].append(position)
-            elif isinstance(username, list):
-                for user in username:
-                    self.positions[user].append(position)
+        for position, usernames in position_to_usernames.items():
+            if isinstance(usernames, list):
+                for username in usernames:
+                    self.positions[username].append(position)
             else:
-                raise TypeError("Username(s) under each position must either be a string or a list")
+                raise TypeError("Username(s) under each position must be a list")
 
         # a cached Dict of the Users who need to be modified (username -> User)
         self.users: Dict[str, IUser] = {}
@@ -49,7 +46,7 @@ class UpdateMemberPositionsTransaction(AbstractTransaction):
         mailing_lists = cfg.get('auxiliary mailing lists_exec')
 
         # position -> username
-        new_positions_reversed = {}  # For returning result
+        new_position_to_usernames = {}  # For returning result
 
         # retrieve User objects and cache them
         for username in self.positions:
@@ -71,7 +68,9 @@ class UpdateMemberPositionsTransaction(AbstractTransaction):
 
             self.old_positions[username] = old_positions
             for position in new_positions:
-                new_positions_reversed[position] = username
+                if position not in new_position_to_usernames:
+                    new_position_to_usernames[position] = []
+                new_position_to_usernames[position].append(username)
         yield 'update_positions_ldap'
 
         # update exec group in LDAP
@@ -104,7 +103,7 @@ class UpdateMemberPositionsTransaction(AbstractTransaction):
         else:
             yield 'subscribe_to_mailing_lists'
 
-        self.finish(new_positions_reversed)
+        self.finish(new_position_to_usernames)
 
     def rollback(self):
         if 'update_exec_group_ldap' in self.finished_operations:

docker-compose.yml

@@ -4,6 +4,8 @@ x-common: &common
   image: python:3.9-bullseye
   volumes:
     - .:$PWD:z
+  security_opt:
+    - label:disable
   environment:
     FLASK_APP: ceod.api
     FLASK_ENV: development

docs/openapi.yaml

@@ -61,9 +61,9 @@ paths:
                 program:
                   $ref: "#/components/schemas/Program"
                 terms:
-                  $ref: "#/components/schemas/Terms"
+                  $ref: "#/components/schemas/TermsOrNumTerms"
                 non_member_terms:
-                  $ref: "#/components/schemas/NonMemberTerms"
+                  $ref: "#/components/schemas/TermsOrNumTerms"
                 forwarding_addresses:
                   $ref: "#/components/schemas/ForwardingAddresses"
       responses:
@@ -161,17 +161,11 @@ paths:
               - type: object
                 properties:
                   terms:
-                    type: array
-                    description: Terms for which this user will be a member
-                    items:
-                      $ref: "#/components/schemas/Term"
+                    $ref: "#/components/schemas/TermsOrNumTerms"
               - type: object
                 properties:
                   non_member_terms:
-                    type: array
-                    description: Terms for which this user will be a club rep
-                    items:
-                      $ref: "#/components/schemas/Term"
+                    $ref: "#/components/schemas/TermsOrNumTerms"
             example: {"terms": ["f2021"]}
       responses:
         "200":
@@ -383,11 +377,14 @@ paths:
               schema:
                 type: object
                 additionalProperties:
-                  type: string
+                  type: array
+                  description: list of usernames
+                  items:
+                    type: string
                 example:
-                  president: user0
-                  vice-president: user1
-                  sysadmin: user2
+                  president: ["user1"]
+                  vice-president: ["user2", "user3"]
+                  sysadmin: ["user4"]
                   treasurer:
     post:
       tags: ['positions']
@@ -404,11 +401,18 @@ paths:
             schema:
               type: object
               additionalProperties:
-                type: string
+                oneOf:
+                  - type: string
+                    description: username or comma-separated list of usernames
+                  - type: array
+                    description: list of usernames
+                    items:
+                      type: string
               example:
-                president: user0
-                vice-president: user1
-                sysadmin: user2
+                president: user1
+                vice-president: user2, user3
+                secretary: ["user4", "user5"]
+                sysadmin: ["user6"]
                 treasurer:
       responses:
         "200":
@@ -422,7 +426,7 @@ paths:
                 {"status": "in progress", "operation": "update_positions_ldap"}
                 {"status": "in progress", "operation": "update_exec_group_ldap"}
                 {"status": "in progress", "operation": "subscribe_to_mailing_list"}
-                {"status": "completed", "result": "OK"}
+                {"status": "completed", "result": {"president": ["user1"],"vice-president": ["user2", "user3"],"secretary": ["user4". "user5"],"sysadmin": ["user6"]}}
         "400":
           description: Failed
           content:
@@ -883,14 +887,15 @@ components:
       example: MAT/Mathematics Computer Science
     Terms:
       type: array
-      description: Terms for which this user was a member
-      items:
-        $ref: "#/components/schemas/Term"
-    NonMemberTerms:
-      type: array
-      description: Terms for which this user was a club rep
+      description: List of terms
       items:
         $ref: "#/components/schemas/Term"
+    TermsOrNumTerms:
+      oneOf:
+        - type: integer
+          description: number of additional terms to add
+          example: 1
+        - $ref: "#/components/schemas/Terms"
     LoginShell:
       type: string
       description: Login shell
@@ -939,7 +944,7 @@ components:
         terms:
           $ref: "#/components/schemas/Terms"
         non_member_terms:
-          $ref: "#/components/schemas/NonMemberTerms"
+          $ref: "#/components/schemas/Terms"
         forwarding_addresses:
           $ref: "#/components/schemas/ForwardingAddresses"
         groups:

requirements.txt

@@ -6,7 +6,7 @@ gunicorn==20.1.0
 Jinja2==3.1.2
 ldap3==2.9.1
 mysql-connector-python==8.0.26
-psycopg2==2.9.1
+psycopg2-binary==2.9.1
 python-augeas==1.1.0
 requests==2.26.0
 requests-gssapi==1.2.3

tests/ceo/cli/test_db_mysql.py

@@ -1,5 +1,6 @@
 import os
 import shutil
+import time
 
 from click.testing import CliRunner
 from mysql.connector import connect
@@ -10,10 +11,15 @@ from ceo.cli import cli
 
 
 def mysql_attempt_connection(host, username, password):
+    # Sometimes, when running the tests locally, I've observed a race condition
+    # where another client can't "see" a database right after we create it.
+    # I only observed this after upgrading the containers to bullseye (MariaDB
+    # version: 10.5.18).
+    time.sleep(0.05)
     with connect(
-            host=host,
-            user=username,
-            password=password,
+        host=host,
+        user=username,
+        password=password,
     ) as con, con.cursor() as cur:
         cur.execute("SHOW DATABASES")
         response = cur.fetchall()
@@ -34,7 +40,7 @@ def test_mysql(cli_setup, cfg, ldap_user):
 
     # create database for user
     result = runner.invoke(cli, ['mysql', 'create', username], input='y\n')
-    print(result.output)
+    #print(result.output)  # noqa: E265
     assert result.exit_code == 0
     assert os.path.isfile(info_file_path)
 

tests/ceo/cli/test_members.py

@@ -26,9 +26,9 @@ def test_members_get(cli_setup, ldap_user):
         f"home directory:       {ldap_user.home_directory}\n"
         f"is a club:            {ldap_user.is_club()}\n"
         f"is a club rep:        {ldap_user.is_club_rep}\n"
-        f"forwarding addresses: \n"
+        f"forwarding addresses:\n"
         f"member terms:         {','.join(ldap_user.terms)}\n"
-        f"groups:               \n"
+        f"groups:\n"
     )
     assert result.exit_code == 0
     assert result.output == expected

tests/ceo/cli/test_positions.py

@@ -44,17 +44,17 @@ vice-president: test_1
 sysadmin:       test_2
 secretary:      test_3
 webmaster:      test_4
-treasurer:      
-cro:            
-librarian:      
-imapd:          
-offsck:         
+treasurer:
+cro:
+librarian:
+imapd:
+offsck:
 Do you want to continue? [y/N]: y
 Update positions in LDAP... Done
 Update executive group in LDAP... Done
 Subscribe to mailing lists... Done
 Transaction successfully completed.
-'''[1:]  # noqa: W291
+'''[1:]
 
     result = runner.invoke(cli, ['positions', 'get'])
     assert result.exit_code == 0
@@ -71,3 +71,71 @@ webmaster:      test_4
         for user in users:
             user.remove_from_ldap()
         group.remove_from_ldap()
+
+
+def test_positions_multiple_users(cli_setup, g_admin_ctx):
+    runner = CliRunner()
+
+    # Setup test data
+    users = []
+    with g_admin_ctx():
+        for i in range(5):
+            user = User(
+                uid=f'test_{i}',
+                cn=f'Test {i}',
+                given_name='Test',
+                sn=str(i),
+                program='Math',
+                terms=['w2023'],
+            )
+            user.add_to_ldap()
+            users.append(user)
+        group = Group(
+            cn='exec',
+            description='Test Group',
+            gid_number=10500,
+        )
+        group.add_to_ldap()
+
+    result = runner.invoke(cli, [
+        'positions', 'set',
+        '--president', 'test_0',
+        '--vice-president', 'test_1,test_2',
+        '--sysadmin', 'test_2',
+        '--secretary', 'test_3, test_4, test_2',
+    ], input='y\n')
+
+    assert result.exit_code == 0
+    assert result.output == '''
+The positions will be updated:
+president:      test_0
+vice-president: test_1, test_2
+sysadmin:       test_2
+secretary:      test_3, test_4, test_2
+treasurer:
+cro:
+librarian:
+imapd:
+webmaster:
+offsck:
+Do you want to continue? [y/N]: y
+Update positions in LDAP... Done
+Update executive group in LDAP... Done
+Subscribe to mailing lists... Done
+Transaction successfully completed.
+'''[1:]
+
+    result = runner.invoke(cli, ['positions', 'get'])
+    assert result.exit_code == 0
+    assert result.output == '''
+president:      test_0
+secretary:      test_2, test_3, test_4
+sysadmin:       test_2
+vice-president: test_1, test_2
+'''[1:]
+
+    # Cleanup test data
+    with g_admin_ctx():
+        for user in users:
+            user.remove_from_ldap()
+        group.remove_from_ldap()

tests/ceod/api/test_positions.py

@@ -7,8 +7,8 @@ def test_get_positions(client, ldap_user, g_admin_ctx):
     status, data = client.get('/api/positions')
     assert status == 200
     expected = {
-        'president': ldap_user.uid,
-        'treasurer': ldap_user.uid,
+        'president': [ldap_user.uid],
+        'treasurer': [ldap_user.uid],
     }
     assert data == expected
 
@@ -20,7 +20,7 @@ def test_set_positions(cfg, client, g_admin_ctx, mock_mailman_server):
 
     users = []
     with g_admin_ctx():
-        for uid in ['test_1', 'test_2', 'test_3', 'test_4']:
+        for uid in ['test1', 'test2', 'test3', 'test4']:
             user = User(uid=uid, cn='Some Name', given_name='Some', sn='Name',
                         terms=['s2021'])
             user.add_to_ldap()
@@ -31,23 +31,23 @@ def test_set_positions(cfg, client, g_admin_ctx, mock_mailman_server):
     try:
         # missing required position
         status, _ = client.post('/api/positions', json={
-            'vice-president': 'test_1',
+            'vice-president': 'test1',
         })
         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',
+            'president': 'test1',
+            'vice-president': 'test2',
+            'sysadmin': 'test3',
+            'no-such-position': 'test3',
         })
         assert status == 400
 
         status, data = client.post('/api/positions', json={
-            'president': 'test_1',
-            'vice-president': 'test_2',
-            'sysadmin': 'test_3',
+            'president': 'test1',
+            'vice-president': 'test2',
+            'sysadmin': 'test3',
         })
         assert status == 200
         expected = [
@@ -55,16 +55,16 @@ def test_set_positions(cfg, client, g_admin_ctx, mock_mailman_server):
             {"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",
+                "president": ["test1"],
+                "vice-president": ["test2"],
+                "sysadmin": ["test3"],
             }},
         ]
         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']
+        expected = ['test1', 'test2', 'test3']
         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]
@@ -72,20 +72,33 @@ def test_set_positions(cfg, client, g_admin_ctx, mock_mailman_server):
             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',
+            'president': 'test1',
+            'vice-president': 'test2',
+            'sysadmin': 'test2',
+            'treasurer': 'test4',
         })
         assert data[-1]['status'] == 'completed'
         # make sure old exec was removed from group
-        expected = ['test_1', 'test_2', 'test_4']
+        expected = ['test1', 'test2', 'test4']
         _, 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
+
+        # multiple users per position
+        status, data = client.post('/api/positions', json={
+            'president': 'test1',
+            'vice-president': ['test2', 'test3'],
+            'sysadmin': 'test2, test3,test4',
+        })
+        assert status == 200
+        assert data[-1] == {'status': 'completed', 'result': {
+            'president': ['test1'],
+            'vice-president': ['test2', 'test3'],
+            'sysadmin': ['test2', 'test3', 'test4'],
+        }}
     finally:
         with g_admin_ctx():
             for user in users: