diff --git a/ceod/api/krb5_cred_handlers.py b/ceod/api/krb5_cred_handlers.py index 0cf26f7..237e6aa 100644 --- a/ceod/api/krb5_cred_handlers.py +++ b/ceod/api/krb5_cred_handlers.py @@ -25,5 +25,7 @@ def teardown_request(err): try: ctx = g.stored_creds_ctx ctx.__exit__(None, None, None) + g.pop('sasl_user') + g.pop('stored_creds_ctx') except Exception: logger.error(traceback.format_exc()) diff --git a/ceod/api/members.py b/ceod/api/members.py index fb3d44b..4788765 100644 --- a/ceod/api/members.py +++ b/ceod/api/members.py @@ -3,12 +3,12 @@ from zope import component from .utils import authz_restrict_to_staff, authz_restrict_to_syscom, \ user_is_in_group, requires_authentication_no_realm, \ - create_streaming_response, create_sync_response, development_only + create_streaming_response, development_only +from ceo_common.errors import BadRequest from ceo_common.interfaces import ILDAPService from ceod.transactions.members import ( AddMemberTransaction, ModifyMemberTransaction, - RenewMemberTransaction, DeleteMemberTransaction, ) import ceod.utils as utils @@ -65,12 +65,16 @@ def patch_user(auth_user: str, username: str): @authz_restrict_to_staff def renew_user(username: str): body = request.get_json(force=True) - txn = RenewMemberTransaction( - username, - terms=body.get('terms'), - non_member_terms=body.get('non_member_terms'), - ) - return create_sync_response(txn) + ldap_srv = component.getUtility(ILDAPService) + user = ldap_srv.get_user(username) + if body.get('terms'): + user.add_terms(body['terms']) + return {'terms_added': body['terms']} + elif body.get('non_member_terms'): + user.add_non_member_terms(body['non_member_terms']) + return {'non_member_terms_added': body['non_member_terms']} + else: + raise BadRequest('Must specify either terms or non-member terms') @bp.route('//pwreset', methods=['POST']) diff --git a/ceod/api/utils.py b/ceod/api/utils.py index 58f702b..c0068eb 100644 --- a/ceod/api/utils.py +++ b/ceod/api/utils.py @@ -7,7 +7,6 @@ import traceback from typing import Callable, List from flask import current_app, stream_with_context -from flask.json import jsonify from flask_kerberos import requires_authentication from ceo_common.logger_factory import logger_factory @@ -107,25 +106,6 @@ def create_streaming_response(txn: AbstractTransaction): stream_with_context(generate()), mimetype='text/plain') -def create_sync_response(txn: AbstractTransaction): - """ - Runs the transaction synchronously and returns a JSON response. - """ - try: - txn.execute() - # if the result is already an Object, don't wrap it again - if isinstance(txn.result, dict) or isinstance(txn.result, list): - return jsonify(txn.result) - # if the result is a string or number, wrap it in an Object - return {'result': txn.result} - except Exception as err: - logger.warning('Transaction failed:\n' + traceback.format_exc()) - txn.rollback() - return { - 'error': str(err), - }, 500 - - def development_only(f: Callable) -> Callable: @functools.wraps(f) def wrapper(*args, **kwargs): diff --git a/tests/ceo_common/__init__.py b/tests/ceo_common/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/ceo_common/krb5/__init__.py b/tests/ceo_common/krb5/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/ceo_common/krb5/test_krb5.py b/tests/ceo_common/krb5/test_krb5.py new file mode 100644 index 0000000..0f876a3 --- /dev/null +++ b/tests/ceo_common/krb5/test_krb5.py @@ -0,0 +1,43 @@ +import os +import subprocess +from subprocess import DEVNULL +import tempfile + +import ldap3 + +from ceo_common.krb5.utils import get_fwd_tgt, store_fwd_tgt_creds + + +def test_fwd_tgt(cfg): + realm = cfg.get('ldap_sasl_realm') + ldap_server = cfg.get('ldap_server_url') + old_krb5ccname = os.environ['KRB5CCNAME'] + f1 = tempfile.NamedTemporaryFile() + d2 = tempfile.TemporaryDirectory() + + try: + subprocess.run( + ['kinit', '-c', 'FILE:' + f1.name, 'regular1'], + text=True, input='krb5', check=True, stdout=DEVNULL) + subprocess.run( + ['kinit', '-c', 'DIR:' + d2.name, 'ctdalek'], + text=True, input='krb5', check=True, stdout=DEVNULL) + os.environ['KRB5CCNAME'] = 'FILE:' + f1.name + b = get_fwd_tgt('phosphoric-acid') + os.environ['KRB5CCNAME'] = 'DIR:' + d2.name + # make sure that we can import the creds from regular1 into the + # cache collection + with store_fwd_tgt_creds(b) as name: + assert name == 'regular1@' + realm + + kwargs = { + 'server': ldap_server, 'auto_bind': True, + 'authentication': ldap3.SASL, 'sasl_mechanism': ldap3.KERBEROS, + } + conn = ldap3.Connection(**kwargs, user='regular1') + assert conn.extend.standard.who_am_i().startswith('dn:uid=regular1,') + conn.unbind() + finally: + os.environ['KRB5CCNAME'] = old_krb5ccname + f1.close() + d2.cleanup() diff --git a/tests/ceod/api/test_groups.py b/tests/ceod/api/test_groups.py index 055e58a..e13b172 100644 --- a/tests/ceod/api/test_groups.py +++ b/tests/ceod/api/test_groups.py @@ -111,7 +111,7 @@ def test_api_add_member_to_group(client, create_group_result, ldap_user): assert data['members'] == [] -def test_api_group_auxiliary(cfg, client, ldap_user): +def test_api_group_auxiliary(cfg, client, ldap_user, g_admin_ctx): # Make sure that syscom has auxiliary mailing lists and groups # defined in ceod_test_local.ini. # Also make sure that the auxiliary mailing lists are defined in @@ -124,14 +124,15 @@ def test_api_group_auxiliary(cfg, client, ldap_user): group_names = ['syscom'] + aux_groups groups = [] - for group_name in group_names: - group = Group( - cn=group_name, - gid_number=min_uid, - ) - group.add_to_ldap() - groups.append(group) - min_uid += 1 + with g_admin_ctx(): + for group_name in group_names: + group = Group( + cn=group_name, + gid_number=min_uid, + ) + group.add_to_ldap() + groups.append(group) + min_uid += 1 uid = ldap_user.uid _, data = client.post(f'/api/groups/syscom/members/{uid}') @@ -158,5 +159,6 @@ def test_api_group_auxiliary(cfg, client, ldap_user): ] assert data == expected - for group in groups: - group.remove_from_ldap() + with g_admin_ctx(): + for group in groups: + group.remove_from_ldap() diff --git a/tests/ceod/api/test_mailman.py b/tests/ceod/api/test_mailman.py new file mode 100644 index 0000000..d55384f --- /dev/null +++ b/tests/ceod/api/test_mailman.py @@ -0,0 +1,14 @@ +def test_subscriptions(cfg, client, ldap_user, mock_mailman_server): + base_domain = cfg.get('base_domain') + uid = ldap_user.uid + address = f'{uid}@{base_domain}' + + status, data = client.post(f'/api/mailman/csc-general/{uid}') + assert status == 200 + assert data == {'result': 'OK'} + assert address in mock_mailman_server.subscriptions['csc-general'] + + status, data = client.delete(f'/api/mailman/csc-general/{uid}') + assert status == 200 + assert data == {'result': 'OK'} + assert address not in mock_mailman_server.subscriptions['csc-general'] diff --git a/tests/ceod/api/test_members.py b/tests/ceod/api/test_members.py index 52acd51..dd6bb05 100644 --- a/tests/ceod/api/test_members.py +++ b/tests/ceod/api/test_members.py @@ -154,11 +154,13 @@ def test_api_renew_user(cfg, client, create_user_result, ldap_conn): status, data = client.post( f'/api/members/{uid}/renew', json={'terms': new_terms}) assert status == 200 + assert data == {'terms_added': new_terms} new_non_member_terms = ['w2022', 's2022'] status, data = client.post( f'/api/members/{uid}/renew', json={'non_member_terms': new_non_member_terms}) assert status == 200 + assert data == {'non_member_terms_added': new_non_member_terms} # check that the changes were applied _, data = client.get(f'/api/members/{uid}') diff --git a/tests/ceod/api/test_uwldap.py b/tests/ceod/api/test_uwldap.py index d6f02f5..d4994e5 100644 --- a/tests/ceod/api/test_uwldap.py +++ b/tests/ceod/api/test_uwldap.py @@ -17,7 +17,7 @@ def test_get_user(client, uwldap_user): def test_updateprograms( - cfg, ldap_conn, g_admin, client, ldap_user, uwldap_user): + cfg, ldap_conn, client, ldap_user, uwldap_user): # sanity check assert ldap_user.uid == uwldap_user.uid # modify the user's program in UWLDAP diff --git a/tests/ceod/model/test_group.py b/tests/ceod/model/test_group.py index 97200ee..2dbb3e5 100644 --- a/tests/ceod/model/test_group.py +++ b/tests/ceod/model/test_group.py @@ -4,7 +4,7 @@ from ceo_common.errors import GroupNotFoundError, UserAlreadyInGroupError, \ UserNotInGroupError -def test_group_add_to_ldap(simple_group, ldap_srv, g_admin): +def test_group_add_to_ldap(simple_group, ldap_srv): group = simple_group group.add_to_ldap() @@ -16,7 +16,7 @@ def test_group_add_to_ldap(simple_group, ldap_srv, g_admin): ldap_srv.get_group(group.cn) -def test_group_members(ldap_group, ldap_srv, g_admin): +def test_group_members(ldap_group, ldap_srv): group = ldap_group group.add_member('member1') @@ -38,21 +38,24 @@ def test_group_members(ldap_group, ldap_srv, g_admin): group.remove_member('member1') -def test_group_to_dict(ldap_group, ldap_user): +def test_group_to_dict(ldap_group, ldap_user, g_admin_ctx): group = ldap_group - expected = { - 'cn': group.cn, - 'description': group.user_cn, - 'gid_number': group.gid_number, - 'members': [], - } - assert group.to_dict() == expected + with g_admin_ctx(): + # we need LDAP credentials because to_dict() might make calls + # to LDAP + expected = { + 'cn': group.cn, + 'description': group.user_cn, + 'gid_number': group.gid_number, + 'members': [], + } + assert group.to_dict() == expected - group.add_member(ldap_user.uid) - expected['members'].append({ - 'uid': ldap_user.uid, - 'cn': ldap_user.cn, - 'program': ldap_user.program, - }) - assert group.to_dict() == expected + group.add_member(ldap_user.uid) + expected['members'].append({ + 'uid': ldap_user.uid, + 'cn': ldap_user.cn, + 'program': ldap_user.program, + }) + assert group.to_dict() == expected diff --git a/tests/ceod/model/test_user.py b/tests/ceod/model/test_user.py index fc49a8a..d57c714 100644 --- a/tests/ceod/model/test_user.py +++ b/tests/ceod/model/test_user.py @@ -7,7 +7,7 @@ from ceo_common.errors import UserNotFoundError, UserAlreadyExistsError from ceod.model import User -def test_user_add_to_ldap(cfg, ldap_srv, simple_user, g_admin): +def test_user_add_to_ldap(cfg, ldap_srv, simple_user): user = simple_user min_id = cfg.get('members_min_id') user.add_to_ldap() @@ -23,7 +23,7 @@ def test_user_add_to_ldap(cfg, ldap_srv, simple_user, g_admin): ldap_srv.get_user(user.uid) -def test_club_add_to_ldap(cfg, ldap_srv, simple_club, g_admin): +def test_club_add_to_ldap(cfg, ldap_srv, simple_club): club = simple_club min_id = cfg.get('clubs_min_id') club.add_to_ldap() @@ -33,7 +33,7 @@ def test_club_add_to_ldap(cfg, ldap_srv, simple_club, g_admin): club.remove_from_ldap() -def test_get_display_info_for_users(cfg, ldap_srv, g_admin): +def test_get_display_info_for_users(cfg, ldap_srv): user1 = User( uid='test_1', cn='Test One', @@ -101,7 +101,7 @@ def test_user_forwarding_addresses(cfg, ldap_user): assert not os.path.isdir(user.home_directory) -def test_user_terms(ldap_user, ldap_srv, g_admin): +def test_user_terms(ldap_user, ldap_srv): user = ldap_user user.add_terms(['f2021']) @@ -113,7 +113,7 @@ def test_user_terms(ldap_user, ldap_srv, g_admin): assert ldap_srv.get_user(user.uid).non_member_terms == user.non_member_terms -def test_user_positions(ldap_user, ldap_srv, g_admin): +def test_user_positions(ldap_user, ldap_srv): user = ldap_user user.add_position('treasurer') @@ -135,7 +135,7 @@ def test_user_change_password(krb_user): user.change_password('new_password') -def test_login_shell(ldap_user, ldap_srv, g_admin): +def test_login_shell(ldap_user, ldap_srv): user = ldap_user user.replace_login_shell('/bin/sh') diff --git a/tests/ceod/model/test_uwldap.py b/tests/ceod/model/test_uwldap.py index ccb58b5..efd758b 100644 --- a/tests/ceod/model/test_uwldap.py +++ b/tests/ceod/model/test_uwldap.py @@ -10,7 +10,7 @@ def test_uwldap_get(uwldap_srv, uwldap_user): def test_ldap_updateprograms( - cfg, ldap_conn, ldap_srv, uwldap_srv, ldap_user, uwldap_user, g_admin): + cfg, ldap_conn, ldap_srv, uwldap_srv, ldap_user, uwldap_user): # sanity check assert ldap_user.uid == uwldap_user.uid # modify the user's program in UWLDAP diff --git a/tests/ceod_test_local.ini b/tests/ceod_test_local.ini index fcca91d..d5dd24b 100644 --- a/tests/ceod_test_local.ini +++ b/tests/ceod_test_local.ini @@ -4,7 +4,7 @@ base_domain = csclub.internal [ceod] admin_host = phosphoric-acid fs_root_host = phosphoric-acid -mailman_host = mail +mailman_host = phosphoric-acid krb5_cache_dir = /tmp/ceod_test_krb5_cache use_https = false port = 9987 diff --git a/tests/conftest.py b/tests/conftest.py index c1b6177..2c56b67 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,3 +1,4 @@ +import contextlib import grp import importlib.resources import os @@ -74,6 +75,27 @@ def ceod_admin_creds(cfg, krb_srv): ) +@pytest.fixture +def g_admin_ctx(cfg, ceod_admin_creds, app): + """ + Store the principal for ceod/admin in flask.g. + This context manager should be used any time LDAP is modified via the + LDAPService, and we are not in a request context. + This should NOT be active when CeodTestClient is making a request, since + that will override the values in flask.g. + """ + @contextlib.contextmanager + def wrapper(): + admin_principal = cfg.get('ldap_admin_principal') + with app.app_context(): + try: + flask.g.sasl_user = admin_principal + yield + finally: + flask.g.pop('sasl_user') + return wrapper + + @pytest.fixture(scope='session') def ldap_conn(cfg, ceod_admin_creds) -> ldap3.Connection: # Assume that the same server URL is being used for the CSC @@ -89,7 +111,7 @@ def ldap_conn(cfg, ceod_admin_creds) -> ldap3.Connection: @pytest.fixture(scope='session') -def ldap_srv(cfg, krb_srv, ldap_conn): +def ldap_srv_session(cfg, krb_srv, ldap_conn): conn = ldap_conn users_base = cfg.get('ldap_users_base') groups_base = cfg.get('ldap_groups_base') @@ -102,12 +124,22 @@ def ldap_srv(cfg, krb_srv, ldap_conn): _ldap_srv = LDAPService() component.provideUtility(_ldap_srv, ILDAPService) + yield _ldap_srv for base_dn in [users_base, groups_base, sudo_base]: delete_subtree(conn, base_dn) +@pytest.fixture +def ldap_srv(ldap_srv_session, g_admin_ctx): + # This is an ugly hack to get around the fact that function-scoped + # fixtures (g_admin_ctx) can't be used from session-scoped fixtures + # (ldap_srv_session). + with g_admin_ctx(): + yield ldap_srv_session + + @pytest.fixture(scope='session') def file_srv(cfg): _file_srv = FileService() @@ -149,7 +181,6 @@ def mailman_srv(mock_mailman_server, cfg, http_client): def uwldap_srv(cfg, ldap_conn): conn = ldap_conn base_dn = cfg.get('uwldap_base') - ou = base_dn.split(',', 1)[0].split('=')[1] delete_subtree(conn, base_dn) @@ -180,33 +211,28 @@ def mail_srv(cfg, mock_mail_server): def app( cfg, krb_srv, - ldap_srv, + ldap_srv_session, file_srv, mailman_srv, uwldap_srv, mail_srv, ): - # need to be root to read keytab - assert os.geteuid() == 0 - app = create_app({ - 'TESTING': True, - }) + app = create_app({'TESTING': True}) return app -@pytest.fixture -def g_admin(cfg, ceod_admin_creds, app): - """ - Store the creds for ceod/admin in flask.g. - This fixture should be used any time LDAP is modified via the LDAPService. - """ - admin_principal = cfg.get('ldap_admin_principal') - with app.test_request_context(): - try: - flask.g.sasl_user = admin_principal - yield - finally: - flask.g.pop('sasl_user') +@pytest.fixture(scope='session') +def mocks_for_create_user(): + with patch.object(utils, 'gen_password') as gen_password_mock, \ + patch.object(pwd, 'getpwuid') as getpwuid_mock, \ + patch.object(grp, 'getgrgid') as getgrgid_mock: + gen_password_mock.return_value = 'krb5' + # Normally, if getpwuid or getgrgid do *not* raise a KeyError, + # then LDAPService will skip that UID. Therefore, by raising a + # KeyError, we are making sure that the UID will *not* be skipped. + getpwuid_mock.side_effect = KeyError() + getgrgid_mock.side_effect = KeyError() + yield @pytest.fixture(scope='session') @@ -243,10 +269,12 @@ def simple_club(): @pytest.fixture -def ldap_user(simple_user, g_admin): - simple_user.add_to_ldap() +def ldap_user(simple_user, g_admin_ctx): + with g_admin_ctx(): + simple_user.add_to_ldap() yield simple_user - simple_user.remove_from_ldap() + with g_admin_ctx(): + simple_user.remove_from_ldap() @pytest.fixture @@ -266,10 +294,12 @@ def simple_group(): @pytest.fixture -def ldap_group(simple_group, g_admin): - simple_group.add_to_ldap() +def ldap_group(simple_group, g_admin_ctx): + with g_admin_ctx(): + simple_group.add_to_ldap() yield simple_group - simple_group.remove_from_ldap() + with g_admin_ctx(): + simple_group.remove_from_ldap() @pytest.fixture diff --git a/tests/conftest_ceod_api.py b/tests/conftest_ceod_api.py index 70d2fcf..fe76cb2 100644 --- a/tests/conftest_ceod_api.py +++ b/tests/conftest_ceod_api.py @@ -4,6 +4,7 @@ import socket import subprocess import tempfile +from flask import g from flask.testing import FlaskClient import gssapi import pytest @@ -68,6 +69,10 @@ class CeodTestClient: return headers def request(self, method, path, principal, **kwargs): + # Make sure that we're not already in a request context, otherwise + # g will get overridden + with pytest.raises(RuntimeError): + '' in g if principal is None: principal = self.syscom_principal resp = self.client.open(