Merge branch 'v1' into uwldap_tests

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

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(
+    ldap_srv = component.getUtility(ILDAPService)
-        username,
+    user = ldap_srv.get_user(username)
-        terms=body.get('terms'),
+    if body.get('terms'):
-        non_member_terms=body.get('non_member_terms'),
+        user.add_terms(body['terms'])
-    )
+        return {'terms_added': body['terms']}
-    return create_sync_response(txn)
+    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('/<username>/pwreset', methods=['POST'])

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

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

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:
+    with g_admin_ctx():
-        group = Group(
+        for group_name in group_names:
-            cn=group_name,
+            group = Group(
-            gid_number=min_uid,
+                cn=group_name,
-        )
+                gid_number=min_uid,
-        group.add_to_ldap()
+            )
-        groups.append(group)
+            group.add_to_ldap()
-        min_uid += 1
+            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:
+    with g_admin_ctx():
-        group.remove_from_ldap()
+        for group in groups:
+            group.remove_from_ldap()

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

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

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

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 = {
+    with g_admin_ctx():
-        'cn': group.cn,
+        # we need LDAP credentials because to_dict() might make calls
-        'description': group.user_cn,
+        # to LDAP
-        'gid_number': group.gid_number,
+        expected = {
-        'members': [],
+            'cn': group.cn,
-    }
+            'description': group.user_cn,
-    assert group.to_dict() == expected
+            'gid_number': group.gid_number,
+            'members': [],
+        }
+        assert group.to_dict() == expected
 
-    group.add_member(ldap_user.uid)
+        group.add_member(ldap_user.uid)
-    expected['members'].append({
+        expected['members'].append({
-        'uid': ldap_user.uid,
+            'uid': ldap_user.uid,
-        'cn': ldap_user.cn,
+            'cn': ldap_user.cn,
-        'program': ldap_user.program,
+            'program': ldap_user.program,
-    })
+        })
-    assert group.to_dict() == expected
+        assert group.to_dict() == expected

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

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

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

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
+    app = create_app({'TESTING': True})
-    assert os.geteuid() == 0
-    app = create_app({
-        'TESTING': True,
-    })
     return app
 
 
-@pytest.fixture
+@pytest.fixture(scope='session')
-def g_admin(cfg, ceod_admin_creds, app):
+def mocks_for_create_user():
-    """
+    with patch.object(utils, 'gen_password') as gen_password_mock, \
-    Store the creds for ceod/admin in flask.g.
+            patch.object(pwd, 'getpwuid') as getpwuid_mock, \
-    This fixture should be used any time LDAP is modified via the LDAPService.
+            patch.object(grp, 'getgrgid') as getgrgid_mock:
-    """
+        gen_password_mock.return_value = 'krb5'
-    admin_principal = cfg.get('ldap_admin_principal')
+        # Normally, if getpwuid or getgrgid do *not* raise a KeyError,
-    with app.test_request_context():
+        # then LDAPService will skip that UID. Therefore, by raising a
-        try:
+        # KeyError, we are making sure that the UID will *not* be skipped.
-            flask.g.sasl_user = admin_principal
+        getpwuid_mock.side_effect = KeyError()
-            yield
+        getgrgid_mock.side_effect = KeyError()
-        finally:
+        yield
-            flask.g.pop('sasl_user')
 
 
 @pytest.fixture(scope='session')
@@ -243,10 +269,12 @@ def simple_club():
 
 
 @pytest.fixture
-def ldap_user(simple_user, g_admin):
+def ldap_user(simple_user, g_admin_ctx):
-    simple_user.add_to_ldap()
+    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):
+def ldap_group(simple_group, g_admin_ctx):
-    simple_group.add_to_ldap()
+    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

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(