From f66d3138b01f6d5df2a9ca1884ccddd579a3280a Mon Sep 17 00:00:00 2001 From: Max Erenberg <> Date: Sat, 23 Oct 2021 17:46:49 -0400 Subject: [PATCH] use inetOrgPerson instead of account --- ceo/cli/members.py | 33 ++++++++++++++++--- ceo/tui/members/AddUserView.py | 16 +++++++-- ceo/utils.py | 8 +++-- ceo_common/interfaces/IUser.py | 4 ++- ceod/api/members.py | 5 +++ ceod/model/LDAPService.py | 14 ++++++-- ceod/model/User.py | 9 +++++ .../members/AddMemberTransaction.py | 8 ++++- tests/ceo/cli/test_members.py | 16 ++++++--- tests/ceo/cli/test_positions.py | 32 ++++++++++++++---- tests/ceod/api/test_db_mysql.py | 6 ++-- tests/ceod/api/test_db_psql.py | 6 ++-- tests/ceod/api/test_members.py | 12 +++++-- tests/ceod/api/test_positions.py | 3 +- tests/ceod/model/test_user.py | 12 +++++++ tests/conftest.py | 2 ++ 16 files changed, 153 insertions(+), 33 deletions(-) diff --git a/ceo/cli/members.py b/ceo/cli/members.py index 0411463..89bc724 100644 --- a/ceo/cli/members.py +++ b/ceo/cli/members.py @@ -20,7 +20,9 @@ def members(): @members.command(short_help='Add a new member or club rep') @click.argument('username') -@click.option('--cn', help='Full name', prompt='Full name') +@click.option('--cn', help='Full name', required=False) +@click.option('--given-name', help='First name', required=False) +@click.option('--sn', help='Last name', required=False) @click.option('--program', required=False, help='Academic program') @click.option('--terms', 'num_terms', type=click.IntRange(1, 100), help='Number of terms to add', default=1) @@ -30,19 +32,40 @@ def members(): help=('Forwarding address to set in ~/.forward. ' 'Default is UW address. ' 'Set to the empty string to disable forwarding.')) -def add(username, cn, program, num_terms, clubrep, forwarding_address): +def add(username, cn, given_name, sn, program, num_terms, clubrep, forwarding_address): cfg = component.getUtility(IConfig) uw_domain = cfg.get('uw_domain') - terms = get_terms_for_new_user(num_terms) - - # TODO: get email address from UWLDAP + # Try to get info from UWLDAP + resp = http_get('/api/uwldap/' + username) + if resp.ok: + result = handle_sync_response(resp) + if cn is None and result.get('cn'): + cn = result['cn'] + if given_name is None and result.get('given_name'): + given_name = result['given_name'] + if sn is None and result.get('sn'): + sn = result['sn'] + if program is None and result.get('program'): + program = result['program'] + if forwarding_address is None: + forwarding_address = result['mail_local_addresses'][0] + if cn is None: + cn = click.prompt('Full name') + if given_name is None: + given_name = click.prompt('First name') + if sn is None: + sn = click.prompt('Last name') if forwarding_address is None: forwarding_address = username + '@' + uw_domain + terms = get_terms_for_new_user(num_terms) + body = { 'uid': username, 'cn': cn, + 'given_name': given_name, + 'sn': sn, } if program is not None: body['program'] = program diff --git a/ceo/tui/members/AddUserView.py b/ceo/tui/members/AddUserView.py index 10e81ea..0ba7090 100644 --- a/ceo/tui/members/AddUserView.py +++ b/ceo/tui/members/AddUserView.py @@ -25,6 +25,10 @@ class AddUserView(CeoFrame): layout.add_widget(self._username) self._full_name = Text("Full name:", "cn") layout.add_widget(self._full_name) + self._first_name = Text("First name:", "given_name") + layout.add_widget(self._first_name) + self._last_name = Text("Last name:", "sn") + layout.add_widget(self._last_name) self._program = Text("Program:", "program") layout.add_widget(self._program) self._forwarding_address = Text("Forwarding address:", "forwarding_address") @@ -69,8 +73,14 @@ class AddUserView(CeoFrame): return data = resp.json() self._status_label.text = '' - self._full_name.value = data['cn'] - self._program.value = data.get('program', '') + if data.get('cn'): + self._full_name.value = data['cn'] + if data.get('given_name'): + self._first_name.value = data['given_name'] + if data.get('sn'): + self._last_name.value = data['sn'] + if data.get('program'): + self._program.value = data.get('program', '') if data.get('mail_local_addresses'): self._forwarding_address.value = data['mail_local_addresses'][0] finally: @@ -80,6 +90,8 @@ class AddUserView(CeoFrame): body = { 'uid': self._username.value, 'cn': self._full_name.value, + 'given_name': self._first_name.value, + 'sn': self._last_name.value, } if self._program.value: body['program'] = self._program.value diff --git a/ceo/utils.py b/ceo/utils.py index 44f96dd..b6490a2 100644 --- a/ceo/utils.py +++ b/ceo/utils.py @@ -92,9 +92,13 @@ def user_dict_kv(d: Dict) -> List[Tuple[str]]: """Pretty-format a serialized User as (key, value) pairs.""" pairs = [ ('uid', d['uid']), - ('cn', d['cn']), - ('program', d.get('program', 'Unknown')), + ('full name', d['cn']), ] + if 'given_name' in d: + pairs.append(('first name', d['given_name'])) + if 'sn' in d: + pairs.append(('last name', d['sn'])) + pairs.append(('program', d.get('program', 'Unknown'))) if 'uid_number' in d: pairs.append(('UID number', d['uid_number'])) if 'gid_number' in d: diff --git a/ceo_common/interfaces/IUser.py b/ceo_common/interfaces/IUser.py index 2ebdcb9..5c0c5a2 100644 --- a/ceo_common/interfaces/IUser.py +++ b/ceo_common/interfaces/IUser.py @@ -8,7 +8,9 @@ class IUser(Interface): # LDAP attributes uid = Attribute('user identifier') - cn = Attribute('common name') + cn = Attribute('full name') + given_name = Attribute('first name') + sn = Attribute('last name') login_shell = Attribute('login shell') uid_number = Attribute('uid number') gid_number = Attribute('gid number') diff --git a/ceod/api/members.py b/ceod/api/members.py index 1502d88..0025287 100644 --- a/ceod/api/members.py +++ b/ceod/api/members.py @@ -24,10 +24,15 @@ def create_user(): non_member_terms = body.get('non_member_terms') if (terms and non_member_terms) or not (terms or non_member_terms): raise BadRequest('Must specify either terms or non-member terms') + for attr in ['uid', 'cn', 'given_name', 'sn']: + if not body.get(attr): + raise BadRequest(f"Attribute '{attr}' is missing or empty") txn = AddMemberTransaction( uid=body['uid'], cn=body['cn'], + given_name=body['given_name'], + sn=body['sn'], program=body.get('program'), terms=terms, non_member_terms=non_member_terms, diff --git a/ceod/model/LDAPService.py b/ceod/model/LDAPService.py index d94e234..5708ce1 100644 --- a/ceod/model/LDAPService.py +++ b/ceod/model/LDAPService.py @@ -164,13 +164,19 @@ class LDAPService: conn.delete(dn) def add_user(self, user: IUser): - object_classes = ['account', 'posixAccount', 'shadowAccount'] if user.is_club(): min_id, max_id = self.club_min_id, self.club_max_id - object_classes.append('club') + object_classes = [ + 'top', 'account', 'posixAccount', 'shadowAccount', 'club', + ] else: + assert user.given_name and user.sn, \ + 'First name and last name must be specified for new members' min_id, max_id = self.member_min_id, self.member_max_id - object_classes.append('member') + object_classes = [ + 'top', 'person', 'organizationalPerson', 'inetOrgPerson', + 'posixAccount', 'shadowAccount', 'member', + ] if user.mail_local_addresses: object_classes.append('inetLocalMailRecipient') conn = self._get_ldap_conn() @@ -200,6 +206,8 @@ class LDAPService: if user.is_club_rep: entry.isClubRep = True if not user.is_club(): + entry.givenName = user.given_name + entry.sn = user.sn entry.userPassword = '{SASL}%s@%s' % (user.uid, self.ldap_sasl_realm) try: diff --git a/ceod/model/User.py b/ceod/model/User.py index d5db444..dcef636 100644 --- a/ceod/model/User.py +++ b/ceod/model/User.py @@ -18,6 +18,8 @@ class User: self, uid: str, cn: str, + given_name: Union[str, None] = None, + sn: Union[str, None] = None, program: Union[str, None] = None, terms: Union[List[str], None] = None, non_member_terms: Union[List[str], None] = None, @@ -37,6 +39,8 @@ class User: self.uid = uid self.cn = cn + self.given_name = given_name + self.sn = sn self.program = program self.terms = terms or [] self.non_member_terms = non_member_terms or [] @@ -79,6 +83,9 @@ class User: 'is_club_rep': self.is_club_rep, 'program': self.program or 'Unknown', } + if self.sn and self.given_name: + data['sn'] = self.sn + data['given_name'] = self.given_name if self.terms: data['terms'] = self.terms if self.non_member_terms: @@ -133,6 +140,8 @@ class User: return User( uid=attrs['uid'][0], cn=attrs['cn'][0], + given_name=attrs.get('givenName', [None])[0], + sn=attrs.get('sn', [None])[0], program=attrs.get('program', [None])[0], terms=attrs.get('term'), non_member_terms=attrs.get('nonMemberTerm'), diff --git a/ceod/transactions/members/AddMemberTransaction.py b/ceod/transactions/members/AddMemberTransaction.py index f472807..b2143d2 100644 --- a/ceod/transactions/members/AddMemberTransaction.py +++ b/ceod/transactions/members/AddMemberTransaction.py @@ -31,7 +31,9 @@ class AddMemberTransaction(AbstractTransaction): self, uid: str, cn: str, - program: Union[str, None], + given_name: str, + sn: str, + program: Union[str, None] = None, terms: Union[List[str], None] = None, non_member_terms: Union[List[str], None] = None, forwarding_addresses: Union[List[str], None] = None, @@ -40,6 +42,8 @@ class AddMemberTransaction(AbstractTransaction): cfg = component.getUtility(IConfig) self.uid = uid self.cn = cn + self.given_name = given_name + self.sn = sn self.program = program self.terms = terms self.non_member_terms = non_member_terms @@ -53,6 +57,8 @@ class AddMemberTransaction(AbstractTransaction): user = User( uid=self.uid, cn=self.cn, + given_name=self.given_name, + sn=self.sn, program=self.program, terms=self.terms, non_member_terms=self.non_member_terms, diff --git a/tests/ceo/cli/test_members.py b/tests/ceo/cli/test_members.py index 1b65fa4..fcb153e 100644 --- a/tests/ceo/cli/test_members.py +++ b/tests/ceo/cli/test_members.py @@ -13,7 +13,9 @@ def test_members_get(cli_setup, ldap_user): result = runner.invoke(cli, ['members', 'get', ldap_user.uid]) expected = ( f"uid: {ldap_user.uid}\n" - f"cn: {ldap_user.cn}\n" + f"full name: {ldap_user.cn}\n" + f"first name: {ldap_user.given_name}\n" + f"last name: {ldap_user.sn}\n" f"program: {ldap_user.program}\n" f"UID number: {ldap_user.uid_number}\n" f"GID number: {ldap_user.gid_number}\n" @@ -31,13 +33,15 @@ def test_members_get(cli_setup, ldap_user): def test_members_add(cli_setup): runner = CliRunner() result = runner.invoke(cli, [ - 'members', 'add', 'test_1', '--cn', 'Test One', '--program', 'Math', - '--terms', '1', + 'members', 'add', 'test_1', '--cn', 'Test One', '--given-name', 'Test', + '--sn', 'One', '--program', 'Math', '--terms', '1', ], input='y\n') expected_pat = re.compile(( "^The following user will be created:\n" "uid: test_1\n" - "cn: Test One\n" + "full name: Test One\n" + "first name: Test\n" + "last name: One\n" "program: Math\n" "forwarding addresses: test_1@uwaterloo.internal\n" "member terms: [sfw]\\d{4}\n" @@ -52,7 +56,9 @@ def test_members_add(cli_setup): "Announce new user to mailing list... Done\n" "Transaction successfully completed.\n" "uid: test_1\n" - "cn: Test One\n" + "full name: Test One\n" + "first name: Test\n" + "last name: One\n" "program: Math\n" "UID number: \\d{5}\n" "GID number: \\d{5}\n" diff --git a/tests/ceo/cli/test_positions.py b/tests/ceo/cli/test_positions.py index c7ae111..299f547 100644 --- a/tests/ceo/cli/test_positions.py +++ b/tests/ceo/cli/test_positions.py @@ -1,14 +1,31 @@ from click.testing import CliRunner from ceo.cli import cli +from ceod.model import User, Group -def test_positions(cli_setup): +def test_positions(cli_setup, g_admin_ctx): runner = CliRunner() # Setup test data - for i in range(5): - runner.invoke(cli, ['members', 'add', f'test_{i}', '--cn', f'Test {i}', '--program', 'Math', '--terms', '1'], input='y\n') - runner.invoke(cli, ['groups', 'add', 'exec', '--description', 'Test Group'], input='y\n') + 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=['f2021'], + ) + 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', @@ -50,6 +67,7 @@ webmaster: test_4 '''[1:] # Cleanup test data - for i in range(5): - runner.invoke(cli, ['members', 'delete', f'test_{i}'], input='y\n') - runner.invoke(cli, ['groups', 'delete', 'exec'], input='y\n') + with g_admin_ctx(): + for user in users: + user.remove_from_ldap() + group.remove_from_ldap() diff --git a/tests/ceod/api/test_db_mysql.py b/tests/ceod/api/test_db_mysql.py index ff7c26c..2ef9c08 100644 --- a/tests/ceod/api/test_db_mysql.py +++ b/tests/ceod/api/test_db_mysql.py @@ -9,7 +9,8 @@ def test_api_create_mysql_db(cfg, client, g_admin_ctx, ldap_user, krb_user): uid = ldap_user.uid with g_admin_ctx(): - user = User(uid='someone_else', cn='Some Name', terms=['s2021']) + user = User(uid='someone_else', cn='Some Name', given_name='Some', + sn='Name', terms=['s2021']) user.add_to_ldap() # user should be able to create db for themselves @@ -75,7 +76,8 @@ def test_api_passwd_reset_mysql(cfg, client, g_admin_ctx, ldap_user, krb_user): uid = ldap_user.uid with g_admin_ctx(): - user = User(uid='someone_else', cn='Some Name', terms=['s2021']) + user = User(uid='someone_else', cn='Some Name', given_name='Some', + sn='Name', terms=['s2021']) user.add_to_ldap() status, data = client.post(f"/api/db/mysql/{uid}", json={}) diff --git a/tests/ceod/api/test_db_psql.py b/tests/ceod/api/test_db_psql.py index 8070fcf..4c60e12 100644 --- a/tests/ceod/api/test_db_psql.py +++ b/tests/ceod/api/test_db_psql.py @@ -8,7 +8,8 @@ def test_api_create_psql_db(cfg, client, g_admin_ctx, ldap_user, krb_user): uid = ldap_user.uid with g_admin_ctx(): - user = User(uid='someone_else', cn='Some Name', terms=['s2021']) + user = User(uid='someone_else', cn='Some Name', given_name='Some', + sn='Name', terms=['s2021']) user.add_to_ldap() # user should be able to create db for themselves @@ -77,7 +78,8 @@ def test_api_passwd_reset_psql(cfg, client, g_admin_ctx, ldap_user, krb_user): uid = ldap_user.uid with g_admin_ctx(): - user = User(uid='someone_else', cn='Some Name', terms=['s2021']) + user = User(uid='someone_else', cn='Some Name', given_name='Some', + sn='Name', terms=['s2021']) user.add_to_ldap() status, data = client.post(f"/api/db/postgresql/{uid}", json={}) diff --git a/tests/ceod/api/test_members.py b/tests/ceod/api/test_members.py index 1188eef..21c360f 100644 --- a/tests/ceod/api/test_members.py +++ b/tests/ceod/api/test_members.py @@ -17,6 +17,8 @@ def create_user_resp(client, mocks_for_create_user, mock_mail_server): status, data = client.post('/api/members', json={ 'uid': 'test_1', 'cn': 'Test One', + 'given_name': 'Test', + 'sn': 'One', 'program': 'Math', 'terms': ['s2021'], 'forwarding_addresses': ['test_1@uwaterloo.internal'], @@ -50,6 +52,8 @@ def test_api_create_user(cfg, create_user_resp, mock_mail_server): {"status": "in progress", "operation": "announce_new_user"}, {"status": "completed", "result": { "cn": "Test One", + "given_name": "Test", + "sn": "One", "uid": "test_1", "uid_number": min_uid, "gid_number": min_uid, @@ -77,6 +81,8 @@ def test_api_next_uid(cfg, client, create_user_result): _, data = client.post('/api/members', json={ 'uid': 'test_2', 'cn': 'Test Two', + 'given_name': 'Test', + 'sn': 'Two', 'program': 'Math', 'terms': ['s2021'], }) @@ -205,7 +211,8 @@ def test_api_reset_password(client, create_user_result): def test_authz_check(client, create_user_result): # non-staff members may not create users status, data = client.post('/api/members', json={ - 'uid': 'test_1', 'cn': 'Test One', 'terms': ['s2021'], + 'uid': 'test_1', 'cn': 'Test One', 'given_name': 'Test', + 'sn': 'One', 'terms': ['s2021'], }, principal='regular1') assert status == 403 @@ -219,6 +226,7 @@ def test_authz_check(client, create_user_result): # If we're syscom but we don't pass credentials, the request should fail _, data = client.post('/api/members', json={ - 'uid': 'test_1', 'cn': 'Test One', 'terms': ['s2021'], + 'uid': 'test_1', 'cn': 'Test One', 'given_name': 'Test', + 'sn': 'One', 'terms': ['s2021'], }, principal='ctdalek', delegate=False) assert data[-1]['status'] == 'aborted' diff --git a/tests/ceod/api/test_positions.py b/tests/ceod/api/test_positions.py index bf6e63e..a7efa06 100644 --- a/tests/ceod/api/test_positions.py +++ b/tests/ceod/api/test_positions.py @@ -21,7 +21,8 @@ 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']: - user = User(uid=uid, cn='Some Name', terms=['s2021']) + user = User(uid=uid, cn='Some Name', given_name='Some', sn='Name', + terms=['s2021']) user.add_to_ldap() users.append(user) exec_group = Group(cn='exec', gid_number=10013) diff --git a/tests/ceod/model/test_user.py b/tests/ceod/model/test_user.py index 81ee28f..0cdcac9 100644 --- a/tests/ceod/model/test_user.py +++ b/tests/ceod/model/test_user.py @@ -37,12 +37,16 @@ def test_get_display_info_for_users(cfg, ldap_srv): user1 = User( uid='test_1', cn='Test One', + given_name='Test', + sn='One', program='Math', terms=['s2021'], ) user2 = User( uid='test_2', cn='Test Two', + given_name='Test', + sn='Two', program='Science', non_member_terms=['s2021'], ) @@ -145,6 +149,8 @@ def test_user_to_dict(cfg): user = User( uid='test_jsmith', cn='John Smith', + given_name='John', + sn='Smith', program='Math', terms=['s2021'], uid_number=21000, @@ -154,6 +160,8 @@ def test_user_to_dict(cfg): expected = { 'uid': user.uid, 'cn': user.cn, + 'given_name': user.given_name, + 'sn': user.sn, 'program': user.program, 'terms': user.terms, 'uid_number': user.uid_number, @@ -179,6 +187,8 @@ def test_user_is_club_rep(ldap_user, ldap_srv): user = User( uid='test_jsmith', cn='John Smith', + given_name='John', + sn='Smith', program='Math', terms=['s2021'], ) @@ -186,6 +196,8 @@ def test_user_is_club_rep(ldap_user, ldap_srv): club_rep = User( uid='test_jdoe', cn='John Doe', + given_name='John', + sn='Doe', program='Math', non_member_terms=['s2021'], ) diff --git a/tests/conftest.py b/tests/conftest.py index a61c0dc..c3ae319 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -292,6 +292,8 @@ def simple_user(): return User( uid='test_jdoe', cn='John Doe', + given_name='John', + sn='Doe', program='Math', terms=['s2021'], )