From 12a83ce4c01843eafd27f9781ebe63f0511c9230 Mon Sep 17 00:00:00 2001 From: Max Erenberg Date: Thu, 19 Aug 2021 05:11:22 +0000 Subject: [PATCH 1/4] remove create_sync_response --- ceod/api/members.py | 21 ++++++++++++--------- ceod/api/utils.py | 20 -------------------- tests/ceod/api/test_members.py | 2 ++ 3 files changed, 14 insertions(+), 29 deletions(-) diff --git a/ceod/api/members.py b/ceod/api/members.py index 7a9453d..ba504f8 100644 --- a/ceod/api/members.py +++ b/ceod/api/members.py @@ -3,13 +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 -from ceo_common.errors import UserNotFoundError + create_streaming_response, development_only +from ceo_common.errors import UserNotFoundError, BadRequest from ceo_common.interfaces import ILDAPService from ceod.transactions.members import ( AddMemberTransaction, ModifyMemberTransaction, - RenewMemberTransaction, DeleteMemberTransaction, ) import ceod.utils as utils @@ -68,12 +67,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 53282ea..97bc484 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/ceod/api/test_members.py b/tests/ceod/api/test_members.py index 863abdb..d7f72be 100644 --- a/tests/ceod/api/test_members.py +++ b/tests/ceod/api/test_members.py @@ -171,11 +171,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}') From 2273ffa241daf4eb1075699ddea72d6f376c51bd Mon Sep 17 00:00:00 2001 From: Max Erenberg Date: Thu, 19 Aug 2021 06:21:30 +0000 Subject: [PATCH 2/4] add test for krb5 --- tests/ceo_common/__init__.py | 0 tests/ceo_common/krb5/__init__.py | 0 tests/ceo_common/krb5/test_krb5.py | 43 ++++++++++++++++++++++++++++++ 3 files changed, 43 insertions(+) create mode 100644 tests/ceo_common/__init__.py create mode 100644 tests/ceo_common/krb5/__init__.py create mode 100644 tests/ceo_common/krb5/test_krb5.py 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() From cc0bc4a638e9959b30b8150b067178c043021183 Mon Sep 17 00:00:00 2001 From: Max Erenberg Date: Thu, 19 Aug 2021 16:14:41 +0000 Subject: [PATCH 3/4] add tests for Mailman API --- ceod/api/krb5_cred_handlers.py | 2 + tests/ceod/api/test_mailman.py | 14 +++++++ tests/ceod/model/test_group.py | 4 +- tests/ceod/model/test_user.py | 10 ++--- tests/ceod/model/test_uwldap.py | 2 +- tests/ceod_test_local.ini | 2 +- tests/conftest.py | 74 ++++++++++++++++++++------------- tests/conftest_ceod_api.py | 5 +++ 8 files changed, 75 insertions(+), 38 deletions(-) create mode 100644 tests/ceod/api/test_mailman.py 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/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/model/test_group.py b/tests/ceod/model/test_group.py index c246646..2ae2d45 100644 --- a/tests/ceod/model/test_group.py +++ b/tests/ceod/model/test_group.py @@ -3,7 +3,7 @@ import pytest from ceo_common.errors import GroupNotFoundError -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() @@ -15,7 +15,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') diff --git a/tests/ceod/model/test_user.py b/tests/ceod/model/test_user.py index 2d8e92b..cf98924 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() @@ -74,7 +74,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']) @@ -86,7 +86,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') @@ -108,7 +108,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 bf233bd..f86bdea 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 a75d93e..86f1fe3 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,3 +1,4 @@ +import contextlib import importlib.resources import os import shutil @@ -70,6 +71,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 @@ -85,7 +107,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') @@ -98,12 +120,22 @@ def ldap_srv(cfg, krb_srv, ldap_conn): conn.add(base_dn, 'organizationalUnit') _ldap_srv = LDAPService() component.provideUtility(_ldap_srv, ILDAPService) + yield _ldap_srv delete_subtree(conn, users_base) delete_subtree(conn, groups_base) +@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() @@ -145,7 +177,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) @@ -176,35 +207,16 @@ 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.app_context(): - try: - flask.g.sasl_user = admin_principal - yield - finally: - flask.g.pop('sasl_user') - - @pytest.fixture def simple_user(): return User( @@ -225,10 +237,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 @@ -247,10 +261,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( From ecf089c261cfaa8a44815d094341a82d6586a408 Mon Sep 17 00:00:00 2001 From: Max Erenberg Date: Thu, 19 Aug 2021 12:58:59 -0400 Subject: [PATCH 4/4] Implement Groups API (#6) This PR implements the /api/groups endpoints. Closes https://git.csclub.uwaterloo.ca/public/pyceo/issues/2. Reviewed-on: https://git.csclub.uwaterloo.ca/public/pyceo/pulls/6 Co-authored-by: Max Erenberg Co-committed-by: Max Erenberg --- ceo_common/errors.py | 10 ++ ceo_common/interfaces/IGroup.py | 2 + ceo_common/interfaces/ILDAPService.py | 8 +- ceo_common/krb5/krb5_build.py | 3 +- ceo_common/model/HTTPClient.py | 5 +- ceod/api/app_factory.py | 5 +- ceod/api/error_handlers.py | 9 +- ceod/api/groups.py | 68 ++++++++ ceod/api/members.py | 14 +- ceod/api/utils.py | 7 +- ceod/model/Group.py | 51 +++++- ceod/model/LDAPService.py | 33 +++- .../groups/AddGroupTransaction.py | 72 ++++++++ .../groups/AddMemberToGroupTransaction.py | 89 ++++++++++ .../groups/DeleteGroupTransaction.py | 44 +++++ .../RemoveMemberFromGroupTransaction.py | 89 ++++++++++ ceod/transactions/groups/__init__.py | 4 + .../members/AddMemberTransaction.py | 30 ++-- .../members/DeleteMemberTransaction.py | 10 +- tests/ceod/api/test_groups.py | 164 ++++++++++++++++++ tests/ceod/api/test_members.py | 21 +-- tests/ceod/model/test_group.py | 37 +++- tests/ceod/model/test_user.py | 27 +++ tests/ceod_dev.ini | 4 +- tests/ceod_test_local.ini | 7 + tests/conftest.py | 33 +++- 26 files changed, 762 insertions(+), 84 deletions(-) create mode 100644 ceod/api/groups.py create mode 100644 ceod/transactions/groups/AddGroupTransaction.py create mode 100644 ceod/transactions/groups/AddMemberToGroupTransaction.py create mode 100644 ceod/transactions/groups/DeleteGroupTransaction.py create mode 100644 ceod/transactions/groups/RemoveMemberFromGroupTransaction.py create mode 100644 ceod/transactions/groups/__init__.py create mode 100644 tests/ceod/api/test_groups.py diff --git a/ceo_common/errors.py b/ceo_common/errors.py index 632cc5b..7d517d3 100644 --- a/ceo_common/errors.py +++ b/ceo_common/errors.py @@ -26,6 +26,16 @@ class GroupAlreadyExistsError(Exception): super().__init__('group already exists') +class UserAlreadyInGroupError(Exception): + def __init__(self): + super().__init__('user is already in group') + + +class UserNotInGroupError(Exception): + def __init__(self): + super().__init__('user is not in group') + + class UserAlreadySubscribedError(Exception): def __init__(self): super().__init__('user is already subscribed') diff --git a/ceo_common/interfaces/IGroup.py b/ceo_common/interfaces/IGroup.py index bd6bce4..7049f14 100644 --- a/ceo_common/interfaces/IGroup.py +++ b/ceo_common/interfaces/IGroup.py @@ -6,9 +6,11 @@ class IGroup(Interface): cn = Attribute('common name') gid_number = Attribute('gid number') + description = Attribute('optional description') members = Attribute('usernames of group members') ldap3_entry = Attribute('cached ldap3.Entry instance for this group') + user_cn = Attribute('cached CN of the user associated with this group') def add_to_ldap(): """Add a new record to LDAP for this group.""" diff --git a/ceo_common/interfaces/ILDAPService.py b/ceo_common/interfaces/ILDAPService.py index bb3bb74..da10a12 100644 --- a/ceo_common/interfaces/ILDAPService.py +++ b/ceo_common/interfaces/ILDAPService.py @@ -1,4 +1,4 @@ -from typing import List, Union +from typing import List, Dict, Union from zope.interface import Interface @@ -18,6 +18,12 @@ class ILDAPService(Interface): def get_user(username: str) -> IUser: """Retrieve the user with the given username.""" + def get_display_info_for_users(usernames: List[str]) -> List[Dict[str, str]]: + """ + Retrieve a subset of the LDAP attributes for the given users. + Useful for displaying a list of users in a compact way. + """ + def add_user(user: IUser): """ Add the user to the database. diff --git a/ceo_common/krb5/krb5_build.py b/ceo_common/krb5/krb5_build.py index 4afc474..3c67721 100644 --- a/ceo_common/krb5/krb5_build.py +++ b/ceo_common/krb5/krb5_build.py @@ -89,7 +89,8 @@ ffibuilder.set_source( """ #include """, - libraries=['krb5'] + libraries=['krb5'], + extra_link_args=['-fsanitize=address', '-static-libasan'], ) if __name__ == '__main__': diff --git a/ceo_common/model/HTTPClient.py b/ceo_common/model/HTTPClient.py index 441a1ac..98ac8e8 100644 --- a/ceo_common/model/HTTPClient.py +++ b/ceo_common/model/HTTPClient.py @@ -1,14 +1,11 @@ -import socket - from flask import g import gssapi -from gssapi.raw.exceptions import ExpiredCredentialsError import requests from requests_gssapi import HTTPSPNEGOAuth from zope import component from zope.interface import implementer -from ceo_common.interfaces import IConfig, IKerberosService, IHTTPClient +from ceo_common.interfaces import IConfig, IHTTPClient @implementer(IHTTPClient) diff --git a/ceod/api/app_factory.py b/ceod/api/app_factory.py index 063e704..a7bc66d 100644 --- a/ceod/api/app_factory.py +++ b/ceod/api/app_factory.py @@ -34,11 +34,14 @@ def create_app(flask_config={}): from ceod.api import members app.register_blueprint(members.bp, url_prefix='/api/members') + # Only offer mailman API if this host is running Mailman if hostname == cfg.get('ceod_mailman_host'): - # Only offer mailman API if this host is running Mailman from ceod.api import mailman app.register_blueprint(mailman.bp, url_prefix='/api/mailman') + from ceod.api import groups + app.register_blueprint(groups.bp, url_prefix='/api/groups') + from ceod.api import uwldap app.register_blueprint(uwldap.bp, url_prefix='/api/uwldap') diff --git a/ceod/api/error_handlers.py b/ceod/api/error_handlers.py index 69cfb09..092c3bf 100644 --- a/ceod/api/error_handlers.py +++ b/ceod/api/error_handlers.py @@ -3,6 +3,7 @@ import traceback from flask.app import Flask from werkzeug.exceptions import HTTPException +from ceo_common.errors import UserNotFoundError, GroupNotFoundError from ceo_common.logger_factory import logger_factory __all__ = ['register_error_handlers'] @@ -19,5 +20,9 @@ def generic_error_handler(err: Exception): # pass through HTTP errors if isinstance(err, HTTPException): return err - logger.error(traceback.format_exc()) - return {'error': type(err).__name__ + ': ' + str(err)}, 500 + if isinstance(err, UserNotFoundError) or isinstance(err, GroupNotFoundError): + status_code = 404 + else: + status_code = 500 + logger.error(traceback.format_exc()) + return {'error': type(err).__name__ + ': ' + str(err)}, status_code diff --git a/ceod/api/groups.py b/ceod/api/groups.py new file mode 100644 index 0000000..fbeed39 --- /dev/null +++ b/ceod/api/groups.py @@ -0,0 +1,68 @@ +from flask import Blueprint, request +from zope import component + +from .utils import authz_restrict_to_syscom, is_truthy, \ + create_streaming_response, development_only +from ceo_common.interfaces import ILDAPService +from ceod.transactions.groups import ( + AddGroupTransaction, + AddMemberToGroupTransaction, + RemoveMemberFromGroupTransaction, + DeleteGroupTransaction, +) + +bp = Blueprint('groups', __name__) + + +@bp.route('/', methods=['POST'], strict_slashes=False) +@authz_restrict_to_syscom +def create_group(): + body = request.get_json(force=True) + txn = AddGroupTransaction( + cn=body['cn'], + description=body['description'], + ) + return create_streaming_response(txn) + + +@bp.route('/') +def get_group(group_name): + ldap_srv = component.getUtility(ILDAPService) + group = ldap_srv.get_group(group_name) + return group.to_dict() + + +@bp.route('//members/', methods=['POST']) +@authz_restrict_to_syscom +def add_member_to_group(group_name, username): + subscribe_to_lists = is_truthy( + request.args.get('subscribe_to_lists', 'true') + ) + txn = AddMemberToGroupTransaction( + username=username, + group_name=group_name, + subscribe_to_lists=subscribe_to_lists, + ) + return create_streaming_response(txn) + + +@bp.route('//members/', methods=['DELETE']) +@authz_restrict_to_syscom +def remove_member_from_group(group_name, username): + unsubscribe_from_lists = is_truthy( + request.args.get('unsubscribe_from_lists', 'true') + ) + txn = RemoveMemberFromGroupTransaction( + username=username, + group_name=group_name, + unsubscribe_from_lists=unsubscribe_from_lists, + ) + return create_streaming_response(txn) + + +@bp.route('/', methods=['DELETE']) +@authz_restrict_to_syscom +@development_only +def delete_group(group_name): + txn = DeleteGroupTransaction(group_name) + return create_streaming_response(txn) diff --git a/ceod/api/members.py b/ceod/api/members.py index ba504f8..4788765 100644 --- a/ceod/api/members.py +++ b/ceod/api/members.py @@ -4,7 +4,7 @@ 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, development_only -from ceo_common.errors import UserNotFoundError, BadRequest +from ceo_common.errors import BadRequest from ceo_common.interfaces import ILDAPService from ceod.transactions.members import ( AddMemberTransaction, @@ -36,15 +36,13 @@ def create_user(): def get_user(auth_user: str, username: str): get_forwarding_addresses = False if auth_user == username or user_is_in_group(auth_user, 'syscom'): + # Only syscom members, or the user themselves, may see the user's + # forwarding addresses, since this requires reading a file in the + # user's home directory get_forwarding_addresses = True ldap_srv = component.getUtility(ILDAPService) - try: - user = ldap_srv.get_user(username) - return user.to_dict(get_forwarding_addresses) - except UserNotFoundError: - return { - 'error': 'user not found' - }, 404 + user = ldap_srv.get_user(username) + return user.to_dict(get_forwarding_addresses) @bp.route('/', methods=['PATCH']) diff --git a/ceod/api/utils.py b/ceod/api/utils.py index 97bc484..c0068eb 100644 --- a/ceod/api/utils.py +++ b/ceod/api/utils.py @@ -109,9 +109,14 @@ def create_streaming_response(txn: AbstractTransaction): def development_only(f: Callable) -> Callable: @functools.wraps(f) def wrapper(*args, **kwargs): - if current_app.config.get('ENV') == 'development': + if current_app.config.get('ENV') == 'development' or \ + current_app.config.get('TESTING'): return f(*args, **kwargs) return { 'error': 'This endpoint may only be called in development' }, 403 return wrapper + + +def is_truthy(s: str) -> bool: + return s.lower() in ['yes', 'true'] diff --git a/ceod/model/Group.py b/ceod/model/Group.py index 5a21f37..cf99e57 100644 --- a/ceod/model/Group.py +++ b/ceod/model/Group.py @@ -6,7 +6,12 @@ from zope import component from zope.interface import implementer from .utils import dn_to_uid +from ceo_common.errors import UserAlreadyInGroupError, UserNotInGroupError, \ + UserNotFoundError from ceo_common.interfaces import ILDAPService, IGroup +from ceo_common.logger_factory import logger_factory + +logger = logger_factory(__name__) @implementer(IGroup) @@ -15,14 +20,18 @@ class Group: self, cn: str, gid_number: int, + description: Union[str, None] = None, members: Union[List[str], None] = None, ldap3_entry: Union[ldap3.Entry, None] = None, + user_cn: Union[str, None] = None, ): self.cn = cn self.gid_number = gid_number + self.description = description # this is a list of the usernames of the members in this group self.members = members or [] self.ldap3_entry = ldap3_entry + self.user_cn = user_cn self.ldap_srv = component.getUtility(ILDAPService) @@ -30,11 +39,32 @@ class Group: return json.dumps(self.to_dict(), indent=2) def to_dict(self): - return { + data = { 'cn': self.cn, 'gid_number': self.gid_number, - 'members': self.members, } + description = None + if self.description: + description = self.description + elif self.user_cn: + # for clubs, the human-readable description is stored in the + # 'cn' attribute of the associated user + description = self.user_cn + else: + try: + # TODO: only fetch the CN to save bandwidth + user = self.ldap_srv.get_user(self.cn) + description = user.cn + self.user_cn = user.cn + except UserNotFoundError: + # some groups, like syscom, don't have an associated user + pass + if description: + data['description'] = description + # to_dict() is usually called for display purposes, so get some more + # information to display + data['members'] = self.ldap_srv.get_display_info_for_users(self.members) + return data def add_to_ldap(self): self.ldap_srv.add_group(self) @@ -49,6 +79,7 @@ class Group: return Group( cn=attrs['cn'][0], gid_number=attrs['gidNumber'][0], + description=attrs.get('description', [None])[0], members=[ dn_to_uid(dn) for dn in attrs.get('uniqueMember', []) ], @@ -57,12 +88,20 @@ class Group: def add_member(self, username: str): dn = self.ldap_srv.uid_to_dn(username) - with self.ldap_srv.entry_ctx_for_group(self) as entry: - entry.uniqueMember.add(dn) + try: + with self.ldap_srv.entry_ctx_for_group(self) as entry: + entry.uniqueMember.add(dn) + except ldap3.core.exceptions.LDAPAttributeOrValueExistsResult as err: + logger.warning(err) + raise UserAlreadyInGroupError() self.members.append(username) def remove_member(self, username: str): dn = self.ldap_srv.uid_to_dn(username) - with self.ldap_srv.entry_ctx_for_group(self) as entry: - entry.uniqueMember.delete(dn) + try: + with self.ldap_srv.entry_ctx_for_group(self) as entry: + entry.uniqueMember.delete(dn) + except ldap3.core.exceptions.LDAPCursorError as err: + logger.warning(err) + raise UserNotInGroupError() self.members.remove(username) diff --git a/ceod/model/LDAPService.py b/ceod/model/LDAPService.py index a9ce90e..8c91fc2 100644 --- a/ceod/model/LDAPService.py +++ b/ceod/model/LDAPService.py @@ -1,7 +1,7 @@ import contextlib import grp import pwd -from typing import Union, List +from typing import Union, Dict, List from flask import g import ldap3 @@ -31,13 +31,16 @@ class LDAPService: self.club_max_id = cfg.get('clubs_max_id') def _get_ldap_conn(self) -> ldap3.Connection: + if 'ldap_conn' in g: + return g.ldap_conn kwargs = {'auto_bind': True, 'raise_exceptions': True} if 'sasl_user' in g: kwargs['authentication'] = ldap3.SASL kwargs['sasl_mechanism'] = ldap3.KERBEROS kwargs['user'] = g.sasl_user - # TODO: cache the connection for a single request conn = ldap3.Connection(self.ldap_server_url, **kwargs) + # cache the connection for a single request + g.ldap_conn = conn return conn def _get_readable_entry_for_user(self, conn: ldap3.Connection, username: str) -> ldap3.Entry: @@ -82,6 +85,22 @@ class LDAPService: entry = self._get_readable_entry_for_group(conn, cn) return Group.deserialize_from_ldap(entry) + def get_display_info_for_users(self, usernames: List[str]) -> List[Dict[str, str]]: + if not usernames: + return [] + conn = self._get_ldap_conn() + filter = '(|' + ''.join([f'(uid={uid})' for uid in usernames]) + ')' + attributes = ['uid', 'cn', 'program'] + conn.search(self.ldap_users_base, filter, attributes=attributes) + return [ + { + 'uid': entry.uid.value, + 'cn': entry.cn.value, + 'program': entry.program.value, + } + for entry in conn.entries + ] + def uid_to_dn(self, uid: str): return f'uid={uid},{self.ldap_users_base}' @@ -200,6 +219,8 @@ class LDAPService: entry.gidNumber = group.gid_number if group.members: entry.uniqueMember = [self.uid_to_dn(uid) for uid in group.members] + if group.description: + entry.description = group.description try: writer.commit() @@ -223,14 +244,12 @@ class LDAPService: uwldap_batch_size: int = 10, ): if members: - filter_str = '(|' + ''.join([ - f'(uid={uid})' for uid in members - ]) + ')' + filter = '(|' + ''.join([f'(uid={uid})' for uid in members]) + ')' else: - filter_str = '(objectClass=*)' + filter = '(objectClass=*)' conn = self._get_ldap_conn() conn.search( - self.ldap_users_base, filter_str, attributes=['uid', 'program']) + self.ldap_users_base, filter, attributes=['uid', 'program']) uids = [entry.uid.value for entry in conn.entries] csc_programs = [entry.program.value for entry in conn.entries] diff --git a/ceod/transactions/groups/AddGroupTransaction.py b/ceod/transactions/groups/AddGroupTransaction.py new file mode 100644 index 0000000..eee418e --- /dev/null +++ b/ceod/transactions/groups/AddGroupTransaction.py @@ -0,0 +1,72 @@ +from zope import component + +from ..AbstractTransaction import AbstractTransaction +from ceo_common.interfaces import ILDAPService +from ceod.model import Group, User + + +class AddGroupTransaction(AbstractTransaction): + """Transaction to create a new group.""" + + operations = [ + 'add_user_to_ldap', + 'add_group_to_ldap', + 'add_sudo_role', + 'create_home_dir', + ] + + def __init__( + self, + cn: str, + description: str, + ): + super().__init__() + self.cn = cn + self.description = description + self.user = None + self.group = None + + def child_execute_iter(self): + ldap_srv = component.getUtility(ILDAPService) + + # The user's UID is the group's CN. + # The user's CN is the group's description. + user = User( + uid=self.cn, + cn=self.description, + is_club=True, + ) + self.user = user + user.add_to_ldap() + yield 'add_user_to_ldap' + + # For now, we only store the description in the user CN, and not in + # the group record. We can change this later if desired. + group = Group( + cn=self.cn, + gid_number=user.gid_number, + user_cn=self.description, + ) + self.group = group + group.add_to_ldap() + yield 'add_group_to_ldap' + + ldap_srv.add_sudo_role(self.cn) + yield 'add_sudo_role' + + user.create_home_dir() + yield 'create_home_dir' + + self.finish(group.to_dict()) + + def rollback(self): + ldap_srv = component.getUtility(ILDAPService) + + if 'create_home_dir' in self.finished_operations: + self.user.delete_home_dir() + if 'add_sudo_role' in self.finished_operations: + ldap_srv.remove_sudo_role(self.cn) + if 'add_group_to_ldap' in self.finished_operations: + self.group.remove_from_ldap() + if 'add_user_to_ldap' in self.finished_operations: + self.user.remove_from_ldap() diff --git a/ceod/transactions/groups/AddMemberToGroupTransaction.py b/ceod/transactions/groups/AddMemberToGroupTransaction.py new file mode 100644 index 0000000..d8a739b --- /dev/null +++ b/ceod/transactions/groups/AddMemberToGroupTransaction.py @@ -0,0 +1,89 @@ +import traceback + +from zope import component + +from ..AbstractTransaction import AbstractTransaction +from ceo_common.errors import UserAlreadyInGroupError, UserAlreadySubscribedError +from ceo_common.interfaces import ILDAPService, IConfig +from ceo_common.logger_factory import logger_factory + +logger = logger_factory(__name__) + + +class AddMemberToGroupTransaction(AbstractTransaction): + """A transaction to add a member to a group.""" + + operations = [ + 'add_user_to_group', + 'add_user_to_auxiliary_groups', + 'subscribe_user_to_auxiliary_mailing_lists', + ] + + def __init__(self, username: str, group_name: str, subscribe_to_lists: bool): + super().__init__() + self.username = username + self.group_name = group_name + self.subscribe_to_lists = subscribe_to_lists + # a list of auxiliary Groups to which the user will be added + self.aux_groups = [] + # a list of mailing list names to which the user will be subscribed + self.aux_lists = [] + self.user = None + self.group = None + + def child_execute_iter(self): + cfg = component.getUtility(IConfig) + ldap_srv = component.getUtility(ILDAPService) + user = ldap_srv.get_user(self.username) + self.user = user + group = ldap_srv.get_group(self.group_name) + self.group = group + + group.add_member(self.username) + yield 'add_user_to_group' + + try: + aux_groups = cfg.get('auxiliary groups_' + self.group_name) + for aux_group_name in aux_groups: + try: + aux_group = ldap_srv.get_group(aux_group_name) + aux_group.add_member(self.username) + self.aux_groups.append(aux_group) + except UserAlreadyInGroupError: + pass + if self.aux_groups: + yield 'add_user_to_auxiliary_groups' + except KeyError: + pass + + if self.subscribe_to_lists: + try: + aux_lists = cfg.get('auxiliary mailing lists_' + self.group_name) + for aux_list in aux_lists: + try: + user.subscribe_to_mailing_list(aux_list) + self.aux_lists.append(aux_list) + except UserAlreadySubscribedError: + pass + if self.aux_lists: + yield 'subscribe_user_to_auxiliary_mailing_lists' + except KeyError: + pass + except Exception: + logger.error(traceback.format_exc()) + yield 'failed_to_subscribe_user_to_auxiliary_mailing_lists' + + result = { + 'added_to_groups': [self.group_name] + [ + group.cn for group in self.aux_groups + ], + } + if self.aux_lists: + result['subscribed_to_lists'] = self.aux_lists + self.finish(result) + + def rollback(self): + for aux_group in self.aux_groups: + aux_group.remove_member(self.username) + if 'add_user_to_group' in self.finished_operations: + self.group.remove_member(self.username) diff --git a/ceod/transactions/groups/DeleteGroupTransaction.py b/ceod/transactions/groups/DeleteGroupTransaction.py new file mode 100644 index 0000000..aed2f85 --- /dev/null +++ b/ceod/transactions/groups/DeleteGroupTransaction.py @@ -0,0 +1,44 @@ +from zope import component + +from ..AbstractTransaction import AbstractTransaction +from ceo_common.interfaces import ILDAPService +from ceo_common.logger_factory import logger_factory + +logger = logger_factory(__name__) + + +class DeleteGroupTransaction(AbstractTransaction): + """ + A transaction to permanently delete a group. + This should only be called in development mode. + """ + + operations = [ + 'remove_sudo_role', + 'delete_home_dir', + 'remove_user_from_ldap', + 'remove_group_from_ldap', + ] + + def __init__(self, group_name): + super().__init__() + self.group_name = group_name + + def child_execute_iter(self): + ldap_srv = component.getUtility(ILDAPService) + user = ldap_srv.get_user(self.group_name) + group = ldap_srv.get_group(self.group_name) + + ldap_srv.remove_sudo_role(group.cn) + yield 'remove_sudo_role' + + user.delete_home_dir() + yield 'delete_home_dir' + + user.remove_from_ldap() + yield 'remove_user_from_ldap' + + group.remove_from_ldap() + yield 'remove_group_from_ldap' + + self.finish('OK') diff --git a/ceod/transactions/groups/RemoveMemberFromGroupTransaction.py b/ceod/transactions/groups/RemoveMemberFromGroupTransaction.py new file mode 100644 index 0000000..d5e2a77 --- /dev/null +++ b/ceod/transactions/groups/RemoveMemberFromGroupTransaction.py @@ -0,0 +1,89 @@ +import traceback + +from zope import component + +from ..AbstractTransaction import AbstractTransaction +from ceo_common.errors import UserNotInGroupError, UserNotSubscribedError +from ceo_common.interfaces import ILDAPService, IConfig +from ceo_common.logger_factory import logger_factory + +logger = logger_factory(__name__) + + +class RemoveMemberFromGroupTransaction(AbstractTransaction): + """A transaction to remove a member from a group.""" + + operations = [ + 'remove_user_from_group', + 'remove_user_from_auxiliary_groups', + 'unsubscribe_user_from_auxiliary_mailing_lists', + ] + + def __init__(self, username: str, group_name: str, unsubscribe_from_lists: bool = True): + super().__init__() + self.username = username + self.group_name = group_name + self.unsubscribe_from_lists = unsubscribe_from_lists + # a list of auxiliary Groups from which the user will be removed + self.aux_groups = [] + # a list of mailing list names from which the user will be unsubscribed + self.aux_lists = [] + self.user = None + self.group = None + + def child_execute_iter(self): + cfg = component.getUtility(IConfig) + ldap_srv = component.getUtility(ILDAPService) + user = ldap_srv.get_user(self.username) + self.user = user + group = ldap_srv.get_group(self.group_name) + self.group = group + + group.remove_member(self.username) + yield 'remove_user_from_group' + + try: + aux_groups = cfg.get('auxiliary groups_' + self.group_name) + for aux_group_name in aux_groups: + try: + aux_group = ldap_srv.get_group(aux_group_name) + aux_group.remove_member(self.username) + self.aux_groups.append(aux_group) + except UserNotInGroupError: + pass + if self.aux_groups: + yield 'remove_user_from_auxiliary_groups' + except KeyError: + pass + + if self.unsubscribe_from_lists: + try: + aux_lists = cfg.get('auxiliary mailing lists_' + self.group_name) + for aux_list in aux_lists: + try: + user.unsubscribe_from_mailing_list(aux_list) + self.aux_lists.append(aux_list) + except UserNotSubscribedError: + pass + if self.aux_lists: + yield 'unsubscribe_user_from_auxiliary_mailing_lists' + except KeyError: + pass + except Exception: + logger.error(traceback.format_exc()) + yield 'failed_to_unsubscribe_user_from_auxiliary_mailing_lists' + + result = { + 'removed_from_groups': [self.group_name] + [ + group.cn for group in self.aux_groups + ], + } + if self.aux_lists: + result['unsubscribed_from_lists'] = self.aux_lists + self.finish(result) + + def rollback(self): + for aux_group in self.aux_groups: + aux_group.add_member(self.username) + if 'remove_user_from_group' in self.finished_operations: + self.group.add_member(self.username) diff --git a/ceod/transactions/groups/__init__.py b/ceod/transactions/groups/__init__.py new file mode 100644 index 0000000..2d7b11b --- /dev/null +++ b/ceod/transactions/groups/__init__.py @@ -0,0 +1,4 @@ +from .AddGroupTransaction import AddGroupTransaction +from .AddMemberToGroupTransaction import AddMemberToGroupTransaction +from .RemoveMemberFromGroupTransaction import RemoveMemberFromGroupTransaction +from .DeleteGroupTransaction import DeleteGroupTransaction diff --git a/ceod/transactions/members/AddMemberTransaction.py b/ceod/transactions/members/AddMemberTransaction.py index c3367d3..04249b7 100644 --- a/ceod/transactions/members/AddMemberTransaction.py +++ b/ceod/transactions/members/AddMemberTransaction.py @@ -42,70 +42,70 @@ class AddMemberTransaction(AbstractTransaction): self.terms = terms self.non_member_terms = non_member_terms self.forwarding_addresses = forwarding_addresses - self.member = None + self.user = None self.group = None self.new_member_list = cfg.get('mailman3_new_member_list') self.mail_srv = component.getUtility(IMailService) def child_execute_iter(self): - member = User( + user = User( uid=self.uid, cn=self.cn, program=self.program, terms=self.terms, non_member_terms=self.non_member_terms, ) - self.member = member - member.add_to_ldap() + self.user = user + user.add_to_ldap() yield 'add_user_to_ldap' group = Group( - cn=member.uid, - gid_number=member.gid_number, + cn=user.uid, + gid_number=user.gid_number, ) self.group = group group.add_to_ldap() yield 'add_group_to_ldap' password = utils.gen_password() - member.add_to_kerberos(password) + user.add_to_kerberos(password) yield 'add_user_to_kerberos' - member.create_home_dir() + user.create_home_dir() yield 'create_home_dir' if self.forwarding_addresses: - member.set_forwarding_addresses(self.forwarding_addresses) + user.set_forwarding_addresses(self.forwarding_addresses) yield 'set_forwarding_addresses' # The following operations can't/shouldn't be rolled back because the # user has already seen the email try: - self.mail_srv.send_welcome_message_to(member) + self.mail_srv.send_welcome_message_to(user) yield 'send_welcome_message' except Exception as err: logger.warning('send_welcome_message failed:\n' + traceback.format_exc()) yield 'failed_to_send_welcome_message\n' + str(err) try: - member.subscribe_to_mailing_list(self.new_member_list) + user.subscribe_to_mailing_list(self.new_member_list) yield 'subscribe_to_mailing_list' except Exception as err: logger.warning('subscribe_to_mailing_list failed:\n' + traceback.format_exc()) yield 'failed_to_subscribe_to_mailing_list\n' + str(err) - user_json = member.to_dict(True) + user_json = user.to_dict(True) # insert the password into the JSON so that the client can see it user_json['password'] = password self.finish(user_json) def rollback(self): if 'create_home_dir' in self.finished_operations: - self.member.delete_home_dir() + self.user.delete_home_dir() if 'add_user_to_kerberos' in self.finished_operations: - self.member.remove_from_kerberos() + self.user.remove_from_kerberos() if 'add_group_to_ldap' in self.finished_operations: self.group.remove_from_ldap() if 'add_user_to_ldap' in self.finished_operations: - self.member.remove_from_ldap() + self.user.remove_from_ldap() diff --git a/ceod/transactions/members/DeleteMemberTransaction.py b/ceod/transactions/members/DeleteMemberTransaction.py index e78aabb..840525d 100644 --- a/ceod/transactions/members/DeleteMemberTransaction.py +++ b/ceod/transactions/members/DeleteMemberTransaction.py @@ -1,14 +1,19 @@ +import traceback + from zope import component from ..AbstractTransaction import AbstractTransaction from ceo_common.errors import UserNotSubscribedError from ceo_common.interfaces import ILDAPService, IConfig +from ceo_common.logger_factory import logger_factory + +logger = logger_factory(__name__) class DeleteMemberTransaction(AbstractTransaction): """ Transaction to permanently delete a member and their resources. - This should only be used during testing. + This should only be used during development. """ operations = [ @@ -43,5 +48,8 @@ class DeleteMemberTransaction(AbstractTransaction): yield 'unsubscribe_from_mailing_list' except UserNotSubscribedError: pass + except Exception: + logger.error(traceback.format_exc()) + yield 'failed_to_unsubscribe_from_mailing_list' self.finish('OK') diff --git a/tests/ceod/api/test_groups.py b/tests/ceod/api/test_groups.py new file mode 100644 index 0000000..e13b172 --- /dev/null +++ b/tests/ceod/api/test_groups.py @@ -0,0 +1,164 @@ +import ldap3 +import pytest + +from ceod.model import Group + + +def test_api_group_not_found(client): + status, data = client.get('/api/groups/no_such_group') + assert status == 404 + + +@pytest.fixture(scope='module') +def create_group_resp(client): + status, data = client.post('/api/groups', json={ + 'cn': 'test_group1', + 'description': 'Test Group One', + }) + assert status == 200 + assert data[-1]['status'] == 'completed' + yield status, data + status, data = client.delete('/api/groups/test_group1') + assert status == 200 + assert data[-1]['status'] == 'completed' + + +@pytest.fixture(scope='module') +def create_group_result(create_group_resp): + # convenience method + _, data = create_group_resp + return data[-1]['result'] + + +def test_api_create_group(cfg, create_group_resp, ldap_conn): + _, data = create_group_resp + min_uid = cfg.get('clubs_min_id') + users_base = cfg.get('ldap_users_base') + sudo_base = cfg.get('ldap_sudo_base') + + expected = [ + {"status": "in progress", "operation": "add_user_to_ldap"}, + {"status": "in progress", "operation": "add_group_to_ldap"}, + {"status": "in progress", "operation": "add_sudo_role"}, + {"status": "in progress", "operation": "create_home_dir"}, + {"status": "completed", "result": { + "cn": "test_group1", + "gid_number": min_uid, + "description": "Test Group One", + "members": [], + }}, + ] + assert data == expected + + # verify that a user was also created + ldap_conn.search( + f'uid=test_group1,{users_base}', '(objectClass=*)', + search_scope=ldap3.BASE) + assert len(ldap_conn.entries) == 1 + + # verify that a sudo role was also created + ldap_conn.search( + f'cn=%test_group1,{sudo_base}', '(objectClass=*)', + search_scope=ldap3.BASE) + assert len(ldap_conn.entries) == 1 + + +def test_api_get_group(cfg, client, create_group_result): + old_data = create_group_result + cn = old_data['cn'] + + status, data = client.get(f'/api/groups/{cn}') + assert status == 200 + assert data == old_data + + +def test_api_add_member_to_group(client, create_group_result, ldap_user): + uid = ldap_user.uid + cn = create_group_result['cn'] + + status, data = client.post(f'/api/groups/{cn}/members/{uid}') + assert status == 200 + expected = [ + {"status": "in progress", "operation": "add_user_to_group"}, + {"status": "completed", "result": {"added_to_groups": [cn]}}, + ] + assert data == expected + + _, data = client.get(f'/api/groups/{cn}') + expected = { + "cn": cn, + "gid_number": create_group_result['gid_number'], + "description": create_group_result['description'], + "members": [ + { + "cn": ldap_user.cn, + "program": ldap_user.program, + "uid": ldap_user.uid, + } + ], + } + assert data == expected + + status, data = client.delete(f'/api/groups/{cn}/members/{uid}') + assert status == 200 + expected = [ + {"status": "in progress", "operation": "remove_user_from_group"}, + {"status": "completed", "result": {"removed_from_groups": [cn]}}, + ] + assert data == expected + + _, data = client.get(f'/api/groups/{cn}') + assert data['members'] == [] + + +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 + # MockMailmanServer.py. + aux_groups = cfg.get('auxiliary groups_syscom') + aux_lists = cfg.get('auxiliary mailing lists_syscom') + min_uid = cfg.get('clubs_min_id') + # add one to account for the 'Test Group One' group, above + min_uid += 1 + + group_names = ['syscom'] + aux_groups + groups = [] + 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}') + expected = [ + {"status": "in progress", "operation": "add_user_to_group"}, + {"status": "in progress", "operation": "add_user_to_auxiliary_groups"}, + {"status": "in progress", "operation": "subscribe_user_to_auxiliary_mailing_lists"}, + {"status": "completed", "result": { + "added_to_groups": group_names, + "subscribed_to_lists": aux_lists, + }}, + ] + assert data == expected + + _, data = client.delete(f'/api/groups/syscom/members/{uid}') + expected = [ + {"status": "in progress", "operation": "remove_user_from_group"}, + {"status": "in progress", "operation": "remove_user_from_auxiliary_groups"}, + {"status": "in progress", "operation": "unsubscribe_user_from_auxiliary_mailing_lists"}, + {"status": "completed", "result": { + "removed_from_groups": group_names, + "unsubscribed_from_lists": aux_lists, + }}, + ] + assert data == expected + + with g_admin_ctx(): + for group in groups: + group.remove_from_ldap() diff --git a/tests/ceod/api/test_members.py b/tests/ceod/api/test_members.py index d7f72be..dd6bb05 100644 --- a/tests/ceod/api/test_members.py +++ b/tests/ceod/api/test_members.py @@ -1,5 +1,3 @@ -import pwd -import grp from unittest.mock import patch import ldap3 @@ -11,24 +9,9 @@ import ceod.utils as utils def test_api_user_not_found(client): status, data = client.get('/api/members/no_such_user') assert status == 404 - assert data['error'] == 'user not found' -@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') +@pytest.fixture(scope='module') def create_user_resp(client, mocks_for_create_user): status, data = client.post('/api/members', json={ 'uid': 'test_1', @@ -44,7 +27,7 @@ def create_user_resp(client, mocks_for_create_user): assert data[-1]['status'] == 'completed' -@pytest.fixture(scope='session') +@pytest.fixture(scope='module') def create_user_result(create_user_resp): # convenience method _, data = create_user_resp diff --git a/tests/ceod/model/test_group.py b/tests/ceod/model/test_group.py index 2ae2d45..2dbb3e5 100644 --- a/tests/ceod/model/test_group.py +++ b/tests/ceod/model/test_group.py @@ -1,6 +1,7 @@ import pytest -from ceo_common.errors import GroupNotFoundError +from ceo_common.errors import GroupNotFoundError, UserAlreadyInGroupError, \ + UserNotInGroupError def test_group_add_to_ldap(simple_group, ldap_srv): @@ -22,6 +23,9 @@ def test_group_members(ldap_group, ldap_srv): assert group.members == ['member1'] assert ldap_srv.get_group(group.cn).members == group.members + with pytest.raises(UserAlreadyInGroupError): + group.add_member('member1') + group.add_member('member2') assert group.members == ['member1', 'member2'] assert ldap_srv.get_group(group.cn).members == group.members @@ -30,13 +34,28 @@ def test_group_members(ldap_group, ldap_srv): assert group.members == ['member2'] assert ldap_srv.get_group(group.cn).members == group.members + with pytest.raises(UserNotInGroupError): + group.remove_member('member1') -def test_group_to_dict(simple_group): - group = simple_group - expected = { - 'cn': group.cn, - 'gid_number': group.gid_number, - 'members': group.members, - } - assert group.to_dict() == expected +def test_group_to_dict(ldap_group, ldap_user, g_admin_ctx): + group = ldap_group + + 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 diff --git a/tests/ceod/model/test_user.py b/tests/ceod/model/test_user.py index cf98924..d57c714 100644 --- a/tests/ceod/model/test_user.py +++ b/tests/ceod/model/test_user.py @@ -33,6 +33,33 @@ def test_club_add_to_ldap(cfg, ldap_srv, simple_club): club.remove_from_ldap() +def test_get_display_info_for_users(cfg, ldap_srv): + user1 = User( + uid='test_1', + cn='Test One', + program='Math', + terms=['s2021'], + ) + user2 = User( + uid='test_2', + cn='Test Two', + program='Science', + non_member_terms=['s2021'], + ) + user1.add_to_ldap() + user2.add_to_ldap() + try: + expected = [ + {'uid': user1.uid, 'cn': user1.cn, 'program': user1.program}, + {'uid': user2.uid, 'cn': user2.cn, 'program': user2.program}, + ] + retrieved = ldap_srv.get_display_info_for_users([user1.uid, user2.uid]) + assert expected == retrieved + finally: + user1.remove_from_ldap() + user2.remove_from_ldap() + + def getprinc(username, admin_principal, should_exist=True): proc = subprocess.run([ 'kadmin', '-k', '-p', admin_principal, diff --git a/tests/ceod_dev.ini b/tests/ceod_dev.ini index 974e0b8..6e7fd38 100644 --- a/tests/ceod_dev.ini +++ b/tests/ceod_dev.ini @@ -50,5 +50,5 @@ syscom = office,staff,adm,src office = cdrom,audio,video,www [auxiliary mailing lists] -syscom = syscom,syscom-alerts,syscom-moderators,mirror -exec = exec,exec-moderators +syscom = syscom,syscom-alerts +exec = exec diff --git a/tests/ceod_test_local.ini b/tests/ceod_test_local.ini index f86bdea..d5dd24b 100644 --- a/tests/ceod_test_local.ini +++ b/tests/ceod_test_local.ini @@ -42,3 +42,10 @@ api_base_url = http://localhost:8002 api_username = restadmin api_password = mailman3 new_member_list = csc-general + +[auxiliary groups] +syscom = office,staff + +[auxiliary mailing lists] +syscom = syscom,syscom-alerts +exec = exec diff --git a/tests/conftest.py b/tests/conftest.py index 86f1fe3..8944d88 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,9 +1,12 @@ import contextlib +import grp import importlib.resources import os +import pwd import shutil import subprocess import tempfile +from unittest.mock import patch import flask import ldap3 @@ -18,9 +21,10 @@ from ceod.api import create_app from ceod.model import KerberosService, LDAPService, FileService, User, \ MailmanService, Group, UWLDAPService, UWLDAPRecord, MailService from ceod.model.utils import strings_to_bytes +import ceod.utils as utils from .MockSMTPServer import MockSMTPServer from .MockMailmanServer import MockMailmanServer -from .conftest_ceod_api import * +from .conftest_ceod_api import client @pytest.fixture(scope='session') @@ -111,20 +115,20 @@ 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') + sudo_base = cfg.get('ldap_sudo_base') - delete_subtree(conn, users_base) - delete_subtree(conn, groups_base) - - for base_dn in [users_base, groups_base]: + for base_dn in [users_base, groups_base, sudo_base]: + delete_subtree(conn, base_dn) ou = base_dn.split(',', 1)[0].split('=')[1] conn.add(base_dn, 'organizationalUnit') + _ldap_srv = LDAPService() component.provideUtility(_ldap_srv, ILDAPService) yield _ldap_srv - delete_subtree(conn, users_base) - delete_subtree(conn, groups_base) + for base_dn in [users_base, groups_base, sudo_base]: + delete_subtree(conn, base_dn) @pytest.fixture @@ -217,6 +221,20 @@ def app( return app +@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 def simple_user(): return User( @@ -257,6 +275,7 @@ def simple_group(): return Group( cn='group1', gid_number=21000, + user_cn='Group One', )