diff --git a/ceo_common/interfaces/ILDAPService.py b/ceo_common/interfaces/ILDAPService.py index 09738fb..84ee9ad 100644 --- a/ceo_common/interfaces/ILDAPService.py +++ b/ceo_common/interfaces/ILDAPService.py @@ -33,6 +33,11 @@ class ILDAPService(Interface): A new UID and GID will be generated and returned in the new user. """ + def get_groups_for_user(username: str) -> List[str]: + """ + Get a list of the groups to which the user belongs. + """ + def remove_user(user: IUser): """Remove this user from the database.""" diff --git a/ceod/api/members.py b/ceod/api/members.py index 167434f..8394ee7 100644 --- a/ceod/api/members.py +++ b/ceod/api/members.py @@ -1,9 +1,9 @@ -from flask import Blueprint, g, request +from flask import Blueprint, request from flask.json import jsonify from zope import component from .utils import authz_restrict_to_staff, authz_restrict_to_syscom, \ - user_is_in_group, requires_authentication_no_realm, \ + user_is_in_group, requires_authentication_no_realm, requires_admin_creds, \ create_streaming_response, development_only, is_truthy from ceo_common.errors import BadRequest, UserAlreadySubscribedError, UserNotSubscribedError from ceo_common.interfaces import ILDAPService, IConfig, IMailService @@ -20,8 +20,12 @@ logger = logger_factory(__name__) @bp.route('/', methods=['POST'], strict_slashes=False) +@requires_admin_creds @authz_restrict_to_staff def create_user(): + # We need to use the admin creds here because office members may not + # directly create new LDAP records. + body = request.get_json(force=True) terms = body.get('terms') non_member_terms = body.get('non_member_terms') @@ -31,10 +35,6 @@ def create_user(): if not body.get(attr): raise BadRequest(f"Attribute '{attr}' is missing or empty") - # We need to use the admin creds here because office members may not - # directly create new LDAP records. - g.need_admin_creds = True - if terms: logger.info(f"Creating member {body['uid']} for terms {terms}") else: @@ -86,19 +86,19 @@ def patch_user(auth_user: str, username: str): @bp.route('//renew', methods=['POST']) +@requires_admin_creds @authz_restrict_to_staff def renew_user(username: str): + # We need to use the admin creds here because office members should + # not be able to directly modify the shadowExpire field; this could + # prevent syscom members from logging into the machines. + body = request.get_json(force=True) terms = body.get('terms') 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') - # We need to use the admin creds here because office members should - # not be able to directly modify the shadowExpire field; this could - # prevent syscom members from logging into the machines. - g.need_admin_creds = True - ldap_srv = component.getUtility(ILDAPService) cfg = component.getUtility(IConfig) user = ldap_srv.get_user(username) diff --git a/ceod/api/utils.py b/ceod/api/utils.py index 01ba4fa..394cacf 100644 --- a/ceod/api/utils.py +++ b/ceod/api/utils.py @@ -1,12 +1,9 @@ import functools -import grp import json -import os -import pwd import traceback from typing import Callable, List -from flask import current_app, stream_with_context +from flask import current_app, g, stream_with_context from zope import component from .spnego import requires_authentication @@ -40,9 +37,24 @@ def requires_authentication_no_realm(f: Callable) -> Callable: return wrapper -def user_is_in_group(user: str, group: str) -> bool: - """Returns True if `user` is in `group`, False otherwise.""" - return user in grp.getgrnam(group).gr_mem +def requires_admin_creds(f: Callable) -> Callable: + """ + Forces the next LDAP connection to use the admin Kerberos credentials. + This must be used BEFORE any of the authz decorators, since those + may require an LDAP connection, which will get cached for later use. + """ + @functools.wraps(f) + def wrapper(*args, **kwargs): + g.need_admin_creds = True + return f(*args, **kwargs) + return wrapper + + +def user_is_in_group(username: str, group_name: str) -> bool: + """Returns True if `username` is in `group_name`, False otherwise.""" + ldap_srv = component.getUtility(ILDAPService) + group = ldap_srv.get_group(group_name) + return username in group.members def authz_restrict_to_groups(f: Callable, allowed_groups: List[str]) -> Callable: @@ -51,8 +63,6 @@ def authz_restrict_to_groups(f: Callable, allowed_groups: List[str]) -> Callable specified groups. """ - allowed_group_ids = [grp.getgrnam(g).gr_gid for g in allowed_groups] - @requires_authentication_no_realm @functools.wraps(f) def wrapper(_username: str, *args, **kwargs): @@ -62,15 +72,14 @@ def authz_restrict_to_groups(f: Callable, allowed_groups: List[str]) -> Callable if username.startswith('ceod/'): # ceod services are always allowed to make internal calls return f(*args, **kwargs) - for gid in os.getgrouplist(username, pwd.getpwnam(username).pw_gid): - if gid in allowed_group_ids: + ldap_srv = component.getUtility(ILDAPService) + for group_name in ldap_srv.get_groups_for_user(username): + if group_name in allowed_groups: return f(*args, **kwargs) logger.debug( f"User '{username}' denied since they are not in one of {allowed_groups}" ) - return { - 'error': f'You must be in one of {allowed_groups}' - }, 403 + return {'error': f'You must be in one of {allowed_groups}'}, 403 return wrapper diff --git a/ceod/model/LDAPService.py b/ceod/model/LDAPService.py index 74c79f3..a569991 100644 --- a/ceod/model/LDAPService.py +++ b/ceod/model/LDAPService.py @@ -98,6 +98,13 @@ class LDAPService: entry = self._get_readable_entry_for_group(conn, cn) return Group.deserialize_from_ldap(entry) + def get_groups_for_user(self, username: str) -> List[str]: + conn = self._get_ldap_conn() + conn.search(self.ldap_groups_base, + f'(uniqueMember={self.uid_to_dn(username)})', + attributes=['cn']) + return [entry.cn.value for entry in conn.entries] + def get_display_info_for_users(self, usernames: List[str]) -> List[Dict[str, str]]: if not usernames: return [] diff --git a/tests/ceo/cli/test_groups.py b/tests/ceo/cli/test_groups.py index 462fc99..50c4931 100644 --- a/tests/ceo/cli/test_groups.py +++ b/tests/ceo/cli/test_groups.py @@ -117,8 +117,7 @@ def test_groups_multiple_members(cli_setup, new_user_gen): def test_groups_with_auxiliary_groups_and_mailing_lists(cli_setup, ldap_user): runner = CliRunner() # make sure auxiliary groups + mailing lists exist in ceod_test_local.ini - create_group('syscom', 'Systems Committee') - create_group('office', 'Office') + create_group('adm', 'Administrators') create_group('staff', 'Staff') runner = CliRunner() @@ -131,7 +130,7 @@ def test_groups_with_auxiliary_groups_and_mailing_lists(cli_setup, ldap_user): "Add user to auxiliary groups... Done\n" "Subscribe user to auxiliary mailing lists... Done\n" "Transaction successfully completed.\n" - f"Added {ldap_user.uid} to syscom, office, staff\n" + f"Added {ldap_user.uid} to syscom, adm, staff\n" f"Subscribed {ldap_user.uid} to syscom@csclub.internal, syscom-alerts@csclub.internal\n" ) assert result.exit_code == 0 @@ -147,7 +146,7 @@ def test_groups_with_auxiliary_groups_and_mailing_lists(cli_setup, ldap_user): "Remove user from auxiliary groups... Done\n" "Unsubscribe user from auxiliary mailing lists... Done\n" "Transaction successfully completed.\n" - f"Removed {ldap_user.uid} from syscom, office, staff\n" + f"Removed {ldap_user.uid} from syscom, adm, staff\n" f"Unsubscribed {ldap_user.uid} from syscom@csclub.internal, syscom-alerts@csclub.internal\n" ) assert result.exit_code == 0 @@ -167,6 +166,5 @@ def test_groups_with_auxiliary_groups_and_mailing_lists(cli_setup, ldap_user): assert result.exit_code == 0 assert 'Unsubscribed' not in result.output - delete_group('syscom') - delete_group('office') + delete_group('adm') delete_group('staff') diff --git a/tests/ceo/cli/test_members.py b/tests/ceo/cli/test_members.py index 09b5acb..cf13a38 100644 --- a/tests/ceo/cli/test_members.py +++ b/tests/ceo/cli/test_members.py @@ -4,6 +4,7 @@ import shutil from datetime import datetime, timedelta from click.testing import CliRunner +import ldap3 from ceo.cli import cli from ceo_common.model import Term @@ -146,7 +147,7 @@ def test_members_pwreset(cli_setup, ldap_user, krb_user): assert expected_pat.match(result.output) is not None -def test_members_expire(cli_setup, app_process, ldap_user, syscom_group): +def test_members_expire(cli_setup, app_process, ldap_user): runner = CliRunner() try: @@ -172,12 +173,17 @@ def test_members_expire(cli_setup, app_process, ldap_user, syscom_group): restore_datetime_in_app_process(app_process) -def test_members_remindexpire(cli_setup, app_process, ldap_user): +def test_members_remindexpire(cfg, cli_setup, app_process, ldap_conn, ldap_user): runner = CliRunner() term = Term(ldap_user.terms[0]) + test_date = (term + 1).to_datetime() + # Add a term to ctdalek so that he doesn't show up in the results + base_dn = cfg.get('ldap_users_base') + ldap_conn.modify( + f'uid=ctdalek,{base_dn}', + {'term': [(ldap3.MODIFY_ADD, [str(term + 1)])]}) try: - test_date = (term + 1).to_datetime() set_datetime_in_app_process(app_process, test_date) result = runner.invoke(cli, ['members', 'remindexpire', '--dry-run']) assert result.exit_code == 0 @@ -200,3 +206,7 @@ def test_members_remindexpire(cli_setup, app_process, ldap_user): assert result.output == "No members are pending expiration.\n" finally: restore_datetime_in_app_process(app_process) + + ldap_conn.modify( + f'uid=ctdalek,{base_dn}', + {'term': [(ldap3.MODIFY_DELETE, [str(term + 1)])]}) diff --git a/tests/ceod/api/test_groups.py b/tests/ceod/api/test_groups.py index e13b172..67388ea 100644 --- a/tests/ceod/api/test_groups.py +++ b/tests/ceod/api/test_groups.py @@ -125,7 +125,8 @@ def test_api_group_auxiliary(cfg, client, ldap_user, g_admin_ctx): group_names = ['syscom'] + aux_groups groups = [] with g_admin_ctx(): - for group_name in group_names: + # the syscom group should already exist, since we need it for auth + for group_name in aux_groups: group = Group( cn=group_name, gid_number=min_uid, @@ -161,4 +162,5 @@ def test_api_group_auxiliary(cfg, client, ldap_user, g_admin_ctx): with g_admin_ctx(): for group in groups: - group.remove_from_ldap() + if group.cn != 'syscom': + group.remove_from_ldap() diff --git a/tests/ceod/api/test_members.py b/tests/ceod/api/test_members.py index 7818353..8417cc6 100644 --- a/tests/ceod/api/test_members.py +++ b/tests/ceod/api/test_members.py @@ -241,7 +241,7 @@ def test_authz_check(client, create_user_result): @pytest.mark.parametrize('term_attr', ['terms', 'non_member_terms']) -def test_expire(client, new_user, term_attr, syscom_group, ldap_conn): +def test_expire(client, new_user, term_attr, ldap_conn): assert new_user.shadowExpire is None current_term = Term.current() start_of_current_term = current_term.to_datetime() @@ -281,6 +281,7 @@ def test_expire(client, new_user, term_attr, syscom_group, ldap_conn): status, data = client.post('/api/members/expire?dry_run=yes') assert status == 200 + print(data) assert (data == [uid]) == should_expire _, user = client.get(f'/api/members/{uid}') @@ -306,15 +307,13 @@ def test_expire(client, new_user, term_attr, syscom_group, ldap_conn): @pytest.mark.parametrize('in_syscom', [True, False]) -def test_expire_syscom_member(client, new_user, syscom_group, g_admin_ctx, ldap_conn, in_syscom): +def test_expire_syscom_member(client, new_user, ldap_conn, in_syscom): uid = new_user.uid start_of_current_term = Term.current().to_datetime() if in_syscom: group_dn = new_user.ldap_srv.group_cn_to_dn('syscom') user_dn = new_user.ldap_srv.uid_to_dn(uid) - changes = { - 'uniqueMember': [(ldap3.MODIFY_ADD, [user_dn])] - } + changes = {'uniqueMember': [(ldap3.MODIFY_ADD, [user_dn])]} ldap_conn.modify(group_dn, changes) with patch.object(ceo_common.utils, 'get_current_datetime') as datetime_mock: datetime_mock.return_value = start_of_current_term + datetime.timedelta(days=160) @@ -324,9 +323,12 @@ def test_expire_syscom_member(client, new_user, syscom_group, g_admin_ctx, ldap_ assert data == [] else: assert data == [uid] + if in_syscom: + changes = {'uniqueMember': [(ldap3.MODIFY_DELETE, [user_dn])]} + ldap_conn.modify(group_dn, changes) -def test_office_member(cfg, client): +def test_office_member(cfg, client, office_user): admin_principal = cfg.get('ldap_admin_principal') ccache_file = cfg.get('ldap_admin_principal_ccache') if os.path.isfile(ccache_file): @@ -367,11 +369,17 @@ def test_office_member(cfg, client): assert status == 200 -def test_membership_renewal_reminder(client, mock_mail_server): +def test_membership_renewal_reminder(cfg, client, mock_mail_server, ldap_conn): uids = ['test3', 'test4'] # fast-forward by one term so that we don't clash with the other users # created by other tests term = Term.current() + 1 + # Add some terms to ctdalek so that he doesn't show up in the API calls below + base_dn = cfg.get('ldap_users_base') + ldap_conn.modify( + f'uid=ctdalek,{base_dn}', + {'term': [(ldap3.MODIFY_ADD, [str(term), str(term + 1), str(term + 2)])]}) + with patch.object(ceo_common.utils, 'get_current_datetime') as datetime_mock: datetime_mock.return_value = term.to_datetime() for uid in uids: @@ -433,3 +441,7 @@ def test_membership_renewal_reminder(client, mock_mail_server): status, _ = client.delete(f'/api/members/{uid}') assert status == 200 mock_mail_server.messages.clear() + + ldap_conn.modify( + f'uid=ctdalek,{base_dn}', + {'term': [(ldap3.MODIFY_DELETE, [str(term), str(term + 1), str(term + 2)])]}) diff --git a/tests/ceod_test_local.ini b/tests/ceod_test_local.ini index 8e78cef..8adc851 100644 --- a/tests/ceod_test_local.ini +++ b/tests/ceod_test_local.ini @@ -27,7 +27,8 @@ server_url = ldap://auth1.csclub.internal base = ou=TestUWLDAP,dc=csclub,dc=internal [members] -min_id = 20001 +# 20000 is ctdalek, 20001 is office1 +min_id = 20002 max_id = 29999 home = /tmp/test_users skel = /users/skel @@ -49,7 +50,7 @@ api_password = mailman3 new_member_list = csc-general [auxiliary groups] -syscom = office,staff +syscom = adm,staff [auxiliary mailing lists] syscom = syscom,syscom-alerts diff --git a/tests/conftest.py b/tests/conftest.py index 224b742..e3c3c3a 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -159,6 +159,70 @@ def ldap_conn(cfg) -> ldap3.Connection: return conn +@pytest.fixture +def office_user(cfg, ldap_conn): + users_base = cfg.get('ldap_users_base') + home_parent = cfg.get('members_home') + ldap_conn.add( + f'uid=office1,{users_base}', + ['top', 'account', 'posixAccount', 'shadowAccount', 'member'], + { + 'uid': 'office1', + 'uidNumber': '20001', + 'gidNumber': '20001', + 'homeDirectory': f'{home_parent}/office1', + 'loginShell': '/bin/bash', + 'cn': 'Office One', + 'givenName': 'Office', + 'sn': 'One', + 'program': 'Math', + 'term': [str(Term.current())], + } + ) + yield + ldap_conn.delete(f'uid=office1,{users_base}') + + +def add_stock_groups_and_users(cfg: IConfig, conn: ldap3.Connection): + users_base = cfg.get('ldap_users_base') + groups_base = cfg.get('ldap_groups_base') + home_parent = cfg.get('members_home') + conn.add( + f'cn=syscom,{groups_base}', + ['top', 'group', 'posixGroup'], + { + 'cn': 'syscom', + 'gidNumber': '10001', + 'uniqueMember': [f'uid=ctdalek,{users_base}'] + } + ) + conn.add( + f'uid=ctdalek,{users_base}', + ['top', 'account', 'posixAccount', 'shadowAccount', 'member'], + { + 'uid': 'ctdalek', + 'uidNumber': '20000', + 'gidNumber': '20000', + 'homeDirectory': f'{home_parent}/ctdalek', + 'loginShell': '/bin/bash', + 'cn': 'Calum T. Dalek', + 'givenName': 'Calum', + 'sn': 'Dalek', + 'program': 'Math', + 'term': [str(Term.current())], + } + ) + conn.add( + f'cn=office,{groups_base}', + ['top', 'group', 'posixGroup'], + { + 'cn': 'office', + 'gidNumber': '10003', + 'uniqueMember': [f'uid=office1,{users_base}'] + } + ) + + @pytest.fixture(scope='session') def ldap_srv_session(cfg, krb_srv, ldap_conn): conn = ldap_conn @@ -169,6 +233,8 @@ def ldap_srv_session(cfg, krb_srv, ldap_conn): for base_dn in [users_base, groups_base, sudo_base]: delete_subtree(conn, base_dn) conn.add(base_dn, 'organizationalUnit') + # Needed for API authentication + add_stock_groups_and_users(cfg, conn) _ldap_srv = LDAPService() component.getGlobalSiteManager().registerUtility(_ldap_srv, ILDAPService) @@ -496,20 +562,6 @@ def ldap_group(simple_group, g_admin_ctx): simple_group.remove_from_ldap() -@pytest.fixture -def syscom_group(g_admin_ctx): - group = Group( - cn='syscom', - gid_number=10001, - user_cn='Systems Committee' - ) - with g_admin_ctx(): - group.add_to_ldap() - yield group - with g_admin_ctx(): - group.remove_from_ldap() - - @pytest.fixture def uwldap_user(cfg, uwldap_srv, ldap_conn): conn = ldap_conn