From 95d083fca1093f4f4029f95272e248c063d0b4fc Mon Sep 17 00:00:00 2001 From: Max Erenberg Date: Sat, 21 Aug 2021 06:27:33 +0000 Subject: [PATCH 01/14] use our own SPNEGO implementation --- .drone.yml | 9 +-- .drone/auth1-setup.sh | 13 +++-- .drone/phosphoric-acid-setup.sh | 15 ++--- ceo_common/model/HTTPClient.py | 14 ++--- ceod/api/app_factory.py | 8 +-- ceod/api/error_handlers.py | 3 + ceod/api/spnego.py | 55 +++++++++++++++++++ ceod/api/utils.py | 2 +- requirements.txt | 1 - tests/ceo_common/model/test_remote_mailman.py | 2 +- tests/conftest.py | 15 ++++- tests/conftest_ceod_api.py | 4 +- 12 files changed, 104 insertions(+), 37 deletions(-) create mode 100644 ceod/api/spnego.py diff --git a/.drone.yml b/.drone.yml index f56d99f..835c5bd 100644 --- a/.drone.yml +++ b/.drone.yml @@ -1,19 +1,20 @@ kind: pipeline type: docker -name: phosphoric-acid +name: default steps: -- name: run tests + # use the step name to mock out the gethostname() call in our tests +- name: phosphoric-acid image: python:3.7-buster # unfortunately we have to do everything in one step because there's no # way to share system packages between steps commands: - # install dependencies + # install dependencies - apt update && apt install -y libkrb5-dev libsasl2-dev python3-dev - python3 -m venv venv - . venv/bin/activate - - pip install -r requirements.txt - pip install -r dev-requirements.txt + - pip install -r requirements.txt - cd ceo_common/krb5 && python krb5_build.py && cd ../.. # lint diff --git a/.drone/auth1-setup.sh b/.drone/auth1-setup.sh index 962ce0d..5314f8b 100755 --- a/.drone/auth1-setup.sh +++ b/.drone/auth1-setup.sh @@ -2,17 +2,18 @@ set -ex -# sanity check -test $(hostname) = auth1 - # don't resolve container names to *real* CSC machines sed -E '/^(domain|search)[[:space:]]+csclub.uwaterloo.ca/d' /etc/resolv.conf > /tmp/resolv.conf cat /tmp/resolv.conf > /etc/resolv.conf rm /tmp/resolv.conf +get_ip_addr() { + getent hosts $1 | cut -d' ' -f1 +} + add_fqdn_to_hosts() { - hostname=$1 - ip_addr=$(getent hosts $hostname | cut -d' ' -f1) + ip_addr=$1 + hostname=$2 sed -E "/${ip_addr}.*\\b${hostname}\\b/d" /etc/hosts > /tmp/hosts cat /tmp/hosts > /etc/hosts rm /tmp/hosts @@ -20,7 +21,7 @@ add_fqdn_to_hosts() { } # set FQDN in /etc/hosts -add_fqdn_to_hosts auth1 +add_fqdn_to_hosts $(get_ip_addr $(hostname)) auth1 export DEBIAN_FRONTEND=noninteractive apt update diff --git a/.drone/phosphoric-acid-setup.sh b/.drone/phosphoric-acid-setup.sh index c48b2eb..f1d0969 100755 --- a/.drone/phosphoric-acid-setup.sh +++ b/.drone/phosphoric-acid-setup.sh @@ -2,17 +2,18 @@ set -ex -# sanity check -test $(hostname) = phosphoric-acid - # don't resolve container names to *real* CSC machines sed -E '/^(domain|search)[[:space:]]+csclub.uwaterloo.ca/d' /etc/resolv.conf > /tmp/resolv.conf cat /tmp/resolv.conf > /etc/resolv.conf rm /tmp/resolv.conf +get_ip_addr() { + getent hosts $1 | cut -d' ' -f1 +} + add_fqdn_to_hosts() { - hostname=$1 - ip_addr=$(getent hosts $hostname | cut -d' ' -f1) + ip_addr=$1 + hostname=$2 sed -E "/${ip_addr}.*\\b${hostname}\\b/d" /etc/hosts > /tmp/hosts cat /tmp/hosts > /etc/hosts rm /tmp/hosts @@ -20,8 +21,8 @@ add_fqdn_to_hosts() { } # set FQDN in /etc/hosts -add_fqdn_to_hosts phosphoric-acid -add_fqdn_to_hosts auth1 +add_fqdn_to_hosts $(get_ip_addr $(hostname)) phosphoric-acid +add_fqdn_to_hosts $(get_ip_addr auth1) auth1 export DEBIAN_FRONTEND=noninteractive apt update diff --git a/ceo_common/model/HTTPClient.py b/ceo_common/model/HTTPClient.py index 98ac8e8..81d4bc5 100644 --- a/ceo_common/model/HTTPClient.py +++ b/ceo_common/model/HTTPClient.py @@ -24,18 +24,16 @@ class HTTPClient: def request(self, host: str, api_path: str, method='GET', **kwargs): principal = g.sasl_user - if '@' not in principal: - principal = principal + '@' + self.krb_realm gssapi_name = gssapi.Name(principal) creds = gssapi.Credentials(name=gssapi_name, usage='initiate') - auth = HTTPSPNEGOAuth( - opportunistic_auth=True, - target_name='ceod', - creds=creds, - ) - # always use the FQDN, for HTTPS purposes + # always use the FQDN if '.' not in host: host = host + '.' + self.base_domain + auth = HTTPSPNEGOAuth( + opportunistic_auth=True, + target_name=gssapi.Name('ceod/' + host), + creds=creds, + ) return requests.request( method, f'{self.scheme}://{host}:{self.ceod_port}{api_path}', diff --git a/ceod/api/app_factory.py b/ceod/api/app_factory.py index a7bc66d..b8de6c4 100644 --- a/ceod/api/app_factory.py +++ b/ceod/api/app_factory.py @@ -3,7 +3,6 @@ import os import socket from flask import Flask -from flask_kerberos import init_kerberos from zope import component from .error_handlers import register_error_handlers @@ -11,6 +10,7 @@ from .krb5_cred_handlers import before_request, teardown_request from ceo_common.interfaces import IConfig, IKerberosService, ILDAPService, IFileService, \ IMailmanService, IMailService, IUWLDAPService, IHTTPClient from ceo_common.model import Config, HTTPClient, RemoteMailmanService +from ceod.api.spnego import init_spnego from ceod.model import KerberosService, LDAPService, FileService, \ MailmanService, MailService, UWLDAPService @@ -23,9 +23,7 @@ def create_app(flask_config={}): register_services(app) cfg = component.getUtility(IConfig) - fqdn = socket.getfqdn() - os.environ['KRB5_KTNAME'] = '/etc/krb5.keytab' - init_kerberos(app, service='ceod', hostname=fqdn) + init_spnego('ceod') hostname = socket.gethostname() # Only ceod_admin_host should serve the /api/members endpoints because @@ -68,8 +66,6 @@ def register_services(app): component.provideUtility(cfg, IConfig) # KerberosService - if 'KRB5_KTNAME' not in os.environ: - os.environ['KRB5_KTNAME'] = '/etc/krb5.keytab' hostname = socket.gethostname() fqdn = socket.getfqdn() # Only ceod_admin_host has the ceod/admin key in its keytab diff --git a/ceod/api/error_handlers.py b/ceod/api/error_handlers.py index 54d44f2..4183d80 100644 --- a/ceod/api/error_handlers.py +++ b/ceod/api/error_handlers.py @@ -1,6 +1,7 @@ import traceback from flask.app import Flask +import ldap3 from werkzeug.exceptions import HTTPException from ceo_common.errors import UserNotFoundError, GroupNotFoundError @@ -21,6 +22,8 @@ def generic_error_handler(err: Exception): status_code = err.code elif isinstance(err, UserNotFoundError) or isinstance(err, GroupNotFoundError): status_code = 404 + elif isinstance(err, ldap3.core.exceptions.LDAPStrongerAuthRequiredResult): + status_code = 403 else: status_code = 500 logger.error(traceback.format_exc()) diff --git a/ceod/api/spnego.py b/ceod/api/spnego.py new file mode 100644 index 0000000..bc1878f --- /dev/null +++ b/ceod/api/spnego.py @@ -0,0 +1,55 @@ +from base64 import b64decode, b64encode +import functools +import socket +from typing import Union + +from flask import request, Response, make_response +import gssapi + +_server_name = None + + +def init_spnego(service_name: str, fqdn: Union[str, None] = None): + """Set the server principal which will be used for SPNEGO.""" + global _server_name + if fqdn is None: + fqdn = socket.getfqdn() + _server_name = gssapi.Name('ceod/' + fqdn) + + # make sure that we're actually capable of acquiring credentials + gssapi.Credentials(usage='accept', name=_server_name) + + +def requires_authentication(f): + """ + Requires that all requests to f have a GSSAPI initiator token. + The initiator principal will be passed to the first argument of f + in the form user@REALM. + """ + @functools.wraps(f) + def wrapper(*args, **kwargs): + if 'authorization' not in request.headers: + return Response('Unauthorized', 401, {'WWW-Authenticate': 'Negotiate'}) + header = request.headers['authorization'] + client_token = b64decode(header.split()[1]) + creds = gssapi.Credentials(usage='accept', name=_server_name) + ctx = gssapi.SecurityContext(creds=creds, usage='accept') + server_token = ctx.step(client_token) + + # OK so we're going to cheat a bit here by assuming that Kerberos is the + # mechanism being used (which we know will be true). We know that Kerberos + # only requires one round-trip for the service handshake, so we don't need + # to store state between requests. Just to be sure, we assert that this is + # indeed the case. + # (This isn't compliant with the GSSAPI spec, but why write more code than + # necessary?) + assert ctx.complete, 'only one round trip expected' + + resp = make_response(f(str(ctx.initiator_name), *args, **kwargs)) + # RFC 2744, section 5.1: + # "If no token need be sent, gss_accept_sec_context will indicate this + # by setting the length field of the output_token argument to zero." + if server_token is not None: + resp.headers['WWW-Authenticate'] = 'Negotiate ' + b64encode(server_token).decode() + return resp + return wrapper diff --git a/ceod/api/utils.py b/ceod/api/utils.py index c0068eb..42f900e 100644 --- a/ceod/api/utils.py +++ b/ceod/api/utils.py @@ -7,8 +7,8 @@ import traceback from typing import Callable, List from flask import current_app, stream_with_context -from flask_kerberos import requires_authentication +from .spnego import requires_authentication from ceo_common.logger_factory import logger_factory from ceod.transactions import AbstractTransaction diff --git a/requirements.txt b/requirements.txt index 402fafc..7023ac8 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,5 +1,4 @@ Flask==2.0.1 -Flask-Kerberos==1.0.4 gssapi==1.6.14 Jinja2==3.0.1 ldap3==2.9.1 diff --git a/tests/ceo_common/model/test_remote_mailman.py b/tests/ceo_common/model/test_remote_mailman.py index cc2fada..44b61fb 100644 --- a/tests/ceo_common/model/test_remote_mailman.py +++ b/tests/ceo_common/model/test_remote_mailman.py @@ -24,7 +24,7 @@ def test_remote_mailman(cfg, http_client, app, mock_mailman_server, g_syscom): try: http_client.get(hostname, '/ping') except requests.exceptions.ConnectionError: - time.sleep(0.5) + time.sleep(1) continue break diff --git a/tests/conftest.py b/tests/conftest.py index 3ff3c54..6422f46 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -7,7 +7,7 @@ import shutil import subprocess from subprocess import DEVNULL import tempfile -from unittest.mock import patch +from unittest.mock import patch, Mock import flask import ldap3 @@ -27,8 +27,19 @@ from .MockMailmanServer import MockMailmanServer from .conftest_ceod_api import client # noqa: F401 +@pytest.fixture(scope='session', autouse=True) +def _drone_hostname_mock(): + # Drone doesn't appear to set the hostname of the container. + # Mock it instead. + if 'DRONE_STEP_NAME' in os.environ: + hostname = os.environ['DRONE_STEP_NAME'] + fqdn = hostname + '.csclub.internal' + socket.gethostname = Mock(return_value=hostname) + socket.getfqdn = Mock(return_value=fqdn) + + @pytest.fixture(scope='session') -def cfg(): +def cfg(_drone_hostname_mock): with importlib.resources.path('tests', 'ceod_test_local.ini') as p: config_file = p.__fspath__() _cfg = Config(config_file) diff --git a/tests/conftest_ceod_api.py b/tests/conftest_ceod_api.py index b2c94fc..4dbeb95 100644 --- a/tests/conftest_ceod_api.py +++ b/tests/conftest_ceod_api.py @@ -36,6 +36,8 @@ class CeodTestClient: self.principal_ccaches = {} # this is where we'll store the credentials for each principal self.cache_dir = cache_dir + # for SPNEGO + self.target_name = gssapi.Name('ceod/' + socket.getfqdn()) @contextlib.contextmanager def krb5ccname_env(self, principal): @@ -55,7 +57,7 @@ class CeodTestClient: creds = gssapi.Credentials(name=name, usage='initiate') auth = HTTPSPNEGOAuth( opportunistic_auth=True, - target_name='ceod', + target_name=self.target_name, creds=creds, ) return auth From 38f354c1066d992819682278e5cf688778468a74 Mon Sep 17 00:00:00 2001 From: Max Erenberg Date: Sat, 21 Aug 2021 06:54:59 +0000 Subject: [PATCH 02/14] add sasl-host to slapd.conf --- .drone/slapd.conf | 1 + 1 file changed, 1 insertion(+) diff --git a/.drone/slapd.conf b/.drone/slapd.conf index fea3ef3..c324588 100644 --- a/.drone/slapd.conf +++ b/.drone/slapd.conf @@ -30,6 +30,7 @@ localssf 128 # map kerberos users to ldap users sasl-realm CSCLUB.INTERNAL +sasl-host auth1.csclub.internal authz-regexp "uid=([^/=]*),cn=CSCLUB.INTERNAL,cn=GSSAPI,cn=auth" "uid=$1,ou=people,dc=csclub,dc=internal" authz-regexp "uid=ceod/admin,cn=CSCLUB.INTERNAL,cn=GSSAPI,cn=auth" From bb82945b417767086e56f4d92183baa9722c880d Mon Sep 17 00:00:00 2001 From: Max Erenberg Date: Sat, 21 Aug 2021 07:13:36 +0000 Subject: [PATCH 03/14] remove hostname from /etc/hosts in auth1 --- .drone/auth1-setup.sh | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/.drone/auth1-setup.sh b/.drone/auth1-setup.sh index 5314f8b..bad9a17 100755 --- a/.drone/auth1-setup.sh +++ b/.drone/auth1-setup.sh @@ -23,6 +23,13 @@ add_fqdn_to_hosts() { # set FQDN in /etc/hosts add_fqdn_to_hosts $(get_ip_addr $(hostname)) auth1 +# I'm not sure why, but we also need to remove the hosts entry for the +# container's real hostname, otherwise slapd only looks for the principal +# ldap/ (this is with the sasl-host option) +sed -E "/\\b$(hostname)\\b/d" /etc/hosts > /tmp/hosts +cat /tmp/hosts > /etc/hosts +rm /tmp/hosts + export DEBIAN_FRONTEND=noninteractive apt update apt install -y psmisc From 862dfc01b26d2a75819c436a9ed494d4706a8905 Mon Sep 17 00:00:00 2001 From: Max Erenberg Date: Sat, 21 Aug 2021 07:20:40 +0000 Subject: [PATCH 04/14] add trigger branches to drone.yml --- .drone.yml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.drone.yml b/.drone.yml index 835c5bd..8ab7961 100644 --- a/.drone.yml +++ b/.drone.yml @@ -30,3 +30,8 @@ services: commands: - .drone/auth1-setup.sh - sleep infinity + +trigger: + branch: + - master + - v1 From 7142659a8c7d716849c410a7acd36767d611897f Mon Sep 17 00:00:00 2001 From: Max Erenberg Date: Sun, 22 Aug 2021 04:36:19 +0000 Subject: [PATCH 05/14] force delete Kerberos test principals --- ceod/model/KerberosService.py | 20 ++++++++++++-------- tests/conftest.py | 14 ++++++++++++++ 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/ceod/model/KerberosService.py b/ceod/model/KerberosService.py index 7f3f470..3f67fcb 100644 --- a/ceod/model/KerberosService.py +++ b/ceod/model/KerberosService.py @@ -1,6 +1,7 @@ import os import shutil import subprocess +from typing import List from zope import component from zope.interface import implementer @@ -50,31 +51,34 @@ class KerberosService: if princ is not None: lib.krb5_free_principal(k_ctx, princ) + def _run(self, args: List[str]): + subprocess.run(args, check=True) + def addprinc(self, principal: str, password: str): - subprocess.run([ + self._run([ 'kadmin', '-k', '-p', self.admin_principal, 'addprinc', '-pw', password, '-policy', 'default', '+needchange', '+requires_preauth', principal - ], check=True) + ]) def delprinc(self, principal: str): - subprocess.run([ + self._run([ 'kadmin', '-k', '-p', self.admin_principal, 'delprinc', '-force', principal - ], check=True) + ]) def change_password(self, principal: str, password: str): - subprocess.run([ + self._run([ 'kadmin', '-k', '-p', self.admin_principal, 'cpw', '-pw', password, principal - ], check=True) - subprocess.run([ + ]) + self._run([ 'kadmin', '-k', '-p', self.admin_principal, 'modprinc', '+needchange', principal - ], check=True) + ]) diff --git a/tests/conftest.py b/tests/conftest.py index 6422f46..5b252a1 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -47,6 +47,17 @@ def cfg(_drone_hostname_mock): return _cfg +def delete_test_princs(krb_srv): + proc = subprocess.run([ + 'kadmin', '-k', '-p', krb_srv.admin_principal, 'listprincs', 'test_*', + ], text=True, capture_output=True, check=True) + princs = [line.strip() for line in proc.stdout.splitlines()] + # remove the password prompt + princs = princs[1:] + for princ in princs: + krb_srv.delprinc(princ) + + @pytest.fixture(scope='session') def krb_srv(cfg): # TODO: create temporary Kerberos database using kdb5_util. @@ -60,7 +71,10 @@ def krb_srv(cfg): cache_dir = cfg.get('ceod_krb5_cache_dir') krb = KerberosService(principal) component.provideUtility(krb, IKerberosService) + + delete_test_princs(krb) yield krb + delete_test_princs(krb) shutil.rmtree(cache_dir) From 0783588323aebdf28b581b83952e49169adee4f5 Mon Sep 17 00:00:00 2001 From: Max Erenberg Date: Sun, 22 Aug 2021 05:44:41 +0000 Subject: [PATCH 06/14] announce new user to ceo mailing list --- README.md | 27 +++++++++------- ceo_common/interfaces/IMailService.py | 9 +++++- ceod/model/MailService.py | 31 ++++++++++++++++++- ceod/model/templates/announce_new_user.j2 | 10 ++++++ ceod/transactions/AbstractTransaction.py | 4 +-- .../members/AddMemberTransaction.py | 12 +++++-- tests/ceod/api/test_members.py | 1 + 7 files changed, 77 insertions(+), 17 deletions(-) create mode 100644 ceod/model/templates/announce_new_user.j2 diff --git a/README.md b/README.md index b286421..64fd585 100644 --- a/README.md +++ b/README.md @@ -35,6 +35,22 @@ called `ceod/admin` (remember to addprinc **and** ktadd). #### Database TODO - Andrew +#### Mailman +You should create the following mailing lists from the mail container: +```sh +/opt/mailman3/bin/mailman create syscom@csclub.internal +/opt/mailman3/bin/mailman create syscom-alerts@csclub.internal +/opt/mailman3/bin/mailman create exec@csclub.internal +/opt/mailman3/bin/mailman create ceo@csclub.internal +``` +See https://git.uwaterloo.ca/csc/syscom-dev-environment/-/tree/master/mail +for instructions on how to access the Mailman UI from your browser. + +If you want to actually see the archived messages, you'll +need to tweak the settings for each list from the UI so that non-member +messages get accepted (by default they get held). + + #### Dependencies Next, install and activate a virtualenv: ```sh @@ -108,14 +124,3 @@ curl --negotiate -u : --service-name ceod \ -d '{"uid":"test_1","cn":"Test One","program":"Math","terms":["s2021"]}' \ -X POST http://phosphoric-acid:9987/api/members ``` - -## Miscellaneous -### Mailman -You may wish to add more mailing lists to Mailman; by default, only the -csc-general list exists (from the dev environment playbooks). Just -attach to the mail container and run the following: -```sh -/opt/mailman3/bin/mailman create new_list_name@csclub.internal -``` -See https://git.uwaterloo.ca/csc/syscom-dev-environment/-/tree/master/mail -for instructions on how to access the Mailman UI from your browser. diff --git a/ceo_common/interfaces/IMailService.py b/ceo_common/interfaces/IMailService.py index 6be18b2..baccd1f 100644 --- a/ceo_common/interfaces/IMailService.py +++ b/ceo_common/interfaces/IMailService.py @@ -1,4 +1,4 @@ -from typing import Dict +from typing import Dict, List from zope.interface import Interface @@ -13,3 +13,10 @@ class IMailService(Interface): def send_welcome_message_to(user: IUser): """Send a welcome message to the new member.""" + + def announce_new_user(user: IUser, operations: List[str]): + """ + Announce to the ceo mailing list that the new user was created. + `operations` is a list of the operations which were performed + during the transaction. + """ diff --git a/ceod/model/MailService.py b/ceod/model/MailService.py index fefcffe..b764809 100644 --- a/ceod/model/MailService.py +++ b/ceod/model/MailService.py @@ -2,8 +2,9 @@ import datetime from email.message import EmailMessage import re import smtplib -from typing import Dict +from typing import Dict, List +from flask import g import jinja2 from zope import component from zope.interface import implementer @@ -61,3 +62,31 @@ class MailService: {'Subject': 'Welcome to the Computer Science Club'}, body, ) + + def announce_new_user(self, user: IUser, operations: List[str]): + # The person who added the new user + # TODO: store the auth_user from SPNEGO into flask.g + auth_user = g.sasl_user + if '@' in auth_user: + auth_user = auth_user[:auth_user.index('@')] + + if user.is_club(): + prog = 'addclubrep' + desc = 'Club Rep' + else: + prog = 'addmember' + desc = 'Member' + operations_str = '\n'.join(operations) + template = self.jinja_env.get_template('announce_new_user.j2') + body = template.render( + user=user, auth_user=auth_user, prog=prog, + operations_str=operations_str) + self.send( + f'{prog} ', + f'Membership and Accounts ', + { + 'Subject': f'New {desc}: {user.uid}', + 'Cc': f'{auth_user}@{self.base_domain}', + }, + body, + ) diff --git a/ceod/model/templates/announce_new_user.j2 b/ceod/model/templates/announce_new_user.j2 new file mode 100644 index 0000000..004a664 --- /dev/null +++ b/ceod/model/templates/announce_new_user.j2 @@ -0,0 +1,10 @@ +Name: {{ user.cn }} +Account: {{ user.uid }} +Program: {{ user.program }} +Added by: {{ auth_user }} + +The following operations were performed: +{{ operations_str }} + +Your Friend, +{{ prog }} diff --git a/ceod/transactions/AbstractTransaction.py b/ceod/transactions/AbstractTransaction.py index 04fc83c..933d401 100644 --- a/ceod/transactions/AbstractTransaction.py +++ b/ceod/transactions/AbstractTransaction.py @@ -8,7 +8,7 @@ class AbstractTransaction(ABC): operations = [] def __init__(self): - self.finished_operations = set() + self.finished_operations = [] # child classes should set this to a JSON-serializable object # once they are finished self.result = None @@ -33,7 +33,7 @@ class AbstractTransaction(ABC): one is completed. """ for operation in self.child_execute_iter(): - self.finished_operations.add(operation) + self.finished_operations.append(operation) yield operation def execute(self): diff --git a/ceod/transactions/members/AddMemberTransaction.py b/ceod/transactions/members/AddMemberTransaction.py index 04249b7..dc9b1fc 100644 --- a/ceod/transactions/members/AddMemberTransaction.py +++ b/ceod/transactions/members/AddMemberTransaction.py @@ -23,6 +23,7 @@ class AddMemberTransaction(AbstractTransaction): 'set_forwarding_addresses', 'subscribe_to_mailing_list', 'send_welcome_message', + 'announce_new_user', ] def __init__( @@ -86,14 +87,21 @@ class AddMemberTransaction(AbstractTransaction): 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) + yield 'failed_to_send_welcome_message: ' + str(err) try: 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) + yield 'failed_to_subscribe_to_mailing_list: ' + str(err) + + try: + self.mail_srv.announce_new_user(user, self.finished_operations) + yield 'announce_new_user' + except Exception as err: + logger.warning('announce_new_user failed:\n' + traceback.format_exc()) + yield 'failed_to_announce_new_user: ' + str(err) user_json = user.to_dict(True) # insert the password into the JSON so that the client can see it diff --git a/tests/ceod/api/test_members.py b/tests/ceod/api/test_members.py index aeb1205..d82b2af 100644 --- a/tests/ceod/api/test_members.py +++ b/tests/ceod/api/test_members.py @@ -45,6 +45,7 @@ def test_api_create_user(cfg, create_user_resp): {"status": "in progress", "operation": "set_forwarding_addresses"}, {"status": "in progress", "operation": "send_welcome_message"}, {"status": "in progress", "operation": "subscribe_to_mailing_list"}, + {"status": "in progress", "operation": "announce_new_user"}, {"status": "completed", "result": { "cn": "Test One", "uid": "test_1", From 0974a7471b6f3ae8b015e39ce455ef7351b21d3f Mon Sep 17 00:00:00 2001 From: Max Erenberg Date: Sun, 22 Aug 2021 06:06:11 +0000 Subject: [PATCH 07/14] ignore UserAlreadySubscribedError --- ceod/transactions/members/AddMemberTransaction.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ceod/transactions/members/AddMemberTransaction.py b/ceod/transactions/members/AddMemberTransaction.py index dc9b1fc..2479864 100644 --- a/ceod/transactions/members/AddMemberTransaction.py +++ b/ceod/transactions/members/AddMemberTransaction.py @@ -4,6 +4,7 @@ from typing import Union, List from zope import component from ..AbstractTransaction import AbstractTransaction +from ceo_common.errors import UserAlreadySubscribedError from ceo_common.interfaces import IConfig, IMailService from ceo_common.logger_factory import logger_factory from ceod.model import User, Group @@ -92,6 +93,8 @@ class AddMemberTransaction(AbstractTransaction): try: user.subscribe_to_mailing_list(self.new_member_list) yield 'subscribe_to_mailing_list' + except UserAlreadySubscribedError: + pass except Exception as err: logger.warning('subscribe_to_mailing_list failed:\n' + traceback.format_exc()) yield 'failed_to_subscribe_to_mailing_list: ' + str(err) From ad937eebebf6e25a7958083934fa263f075c9206 Mon Sep 17 00:00:00 2001 From: Rio Date: Sun, 22 Aug 2021 17:57:36 -0400 Subject: [PATCH 08/14] Positions API (#7) Co-authored-by: Max Erenberg Co-authored-by: Rio Liu Co-authored-by: Rio6 Reviewed-on: https://git.csclub.uwaterloo.ca/public/pyceo/pulls/7 Co-authored-by: Rio Co-committed-by: Rio --- ceo_common/errors.py | 8 +- ceo_common/interfaces/IGroup.py | 5 + ceo_common/interfaces/ILDAPService.py | 3 + ceo_common/interfaces/IUser.py | 7 +- ceo_common/model/Config.py | 4 +- ceod/api/app_factory.py | 3 + ceod/api/positions.py | 45 ++++++++ ceod/model/Group.py | 8 ++ ceod/model/LDAPService.py | 11 +- ceod/model/User.py | 14 +-- .../UpdateMemberPositionsTransaction.py | 109 ++++++++++++++++++ ceod/transactions/members/__init__.py | 1 + tests/MockMailmanServer.py | 4 + tests/ceod/api/test_positions.py | 93 +++++++++++++++ tests/ceod/model/test_group.py | 4 + tests/ceod/model/test_user.py | 14 +-- tests/ceod_dev.ini | 5 + tests/ceod_test_local.ini | 5 + 18 files changed, 311 insertions(+), 32 deletions(-) create mode 100644 ceod/api/positions.py create mode 100644 ceod/transactions/members/UpdateMemberPositionsTransaction.py create mode 100644 tests/ceod/api/test_positions.py diff --git a/ceo_common/errors.py b/ceo_common/errors.py index 7d517d3..539c181 100644 --- a/ceo_common/errors.py +++ b/ceo_common/errors.py @@ -1,11 +1,11 @@ class UserNotFoundError(Exception): - def __init__(self): - super().__init__('user not found') + def __init__(self, username): + super().__init__(f"user '{username}' not found") class GroupNotFoundError(Exception): - def __init__(self): - super().__init__('group not found') + def __init__(self, group_name): + super().__init__(f"group '{group_name}' not found") class BadRequest(Exception): diff --git a/ceo_common/interfaces/IGroup.py b/ceo_common/interfaces/IGroup.py index 7049f14..09857cc 100644 --- a/ceo_common/interfaces/IGroup.py +++ b/ceo_common/interfaces/IGroup.py @@ -1,3 +1,5 @@ +from typing import List + from zope.interface import Interface, Attribute @@ -21,5 +23,8 @@ class IGroup(Interface): def remove_member(username: str): """Remove the member from this group in LDAP.""" + def set_members(usernames: List[str]): + """Set all of the members of this group in LDAP.""" + def to_dict(): """Serialize this group as JSON.""" diff --git a/ceo_common/interfaces/ILDAPService.py b/ceo_common/interfaces/ILDAPService.py index da10a12..89424f4 100644 --- a/ceo_common/interfaces/ILDAPService.py +++ b/ceo_common/interfaces/ILDAPService.py @@ -24,6 +24,9 @@ class ILDAPService(Interface): Useful for displaying a list of users in a compact way. """ + def get_users_with_positions(self) -> List[IUser]: + """Retrieve users who have a non-empty position attribute.""" + def add_user(user: IUser): """ Add the user to the database. diff --git a/ceo_common/interfaces/IUser.py b/ceo_common/interfaces/IUser.py index f6a288f..60fb091 100644 --- a/ceo_common/interfaces/IUser.py +++ b/ceo_common/interfaces/IUser.py @@ -53,11 +53,8 @@ class IUser(Interface): def add_non_member_terms(terms: List[str]): """Add non-member terms for this user.""" - def add_position(position: str): - """Add a position to this user.""" - - def remove_position(position: str): - """Remove a position from this user.""" + def set_positions(self, positions: List[str]): + """Set the positions for this user.""" def change_password(password: str): """Replace this user's password.""" diff --git a/ceo_common/model/Config.py b/ceo_common/model/Config.py index f2ff94e..dfe7bbc 100644 --- a/ceo_common/model/Config.py +++ b/ceo_common/model/Config.py @@ -27,6 +27,6 @@ class Config: return True if val.lower() in ['false', 'no']: return False - if section.startswith('auxiliary '): - return val.split(',') + if section.startswith('auxiliary ') or section == 'positions': + return [item.strip() for item in val.split(',')] return val diff --git a/ceod/api/app_factory.py b/ceod/api/app_factory.py index b8de6c4..f033a2b 100644 --- a/ceod/api/app_factory.py +++ b/ceod/api/app_factory.py @@ -40,6 +40,9 @@ def create_app(flask_config={}): from ceod.api import groups app.register_blueprint(groups.bp, url_prefix='/api/groups') + from ceod.api import positions + app.register_blueprint(positions.bp, url_prefix='/api/positions') + from ceod.api import uwldap app.register_blueprint(uwldap.bp, url_prefix='/api/uwldap') diff --git a/ceod/api/positions.py b/ceod/api/positions.py new file mode 100644 index 0000000..9d9f4c4 --- /dev/null +++ b/ceod/api/positions.py @@ -0,0 +1,45 @@ +from flask import Blueprint, request +from zope import component + +from .utils import authz_restrict_to_syscom, create_streaming_response +from ceo_common.interfaces import ILDAPService, IConfig +from ceod.transactions.members import UpdateMemberPositionsTransaction + +bp = Blueprint('positions', __name__) + + +@bp.route('/', methods=['GET'], strict_slashes=False) +def get_positions(): + ldap_srv = component.getUtility(ILDAPService) + + positions = {} + for user in ldap_srv.get_users_with_positions(): + for position in user.positions: + positions[position] = user.uid + + return positions + + +@bp.route('/', methods=['POST'], strict_slashes=False) +@authz_restrict_to_syscom +def update_positions(): + cfg = component.getUtility(IConfig) + body = request.get_json(force=True) + + required = cfg.get('positions_required') + available = cfg.get('positions_available') + + for position in body.keys(): + if position not in available: + return { + 'error': f'unknown position: {position}' + }, 400 + + for position in required: + if position not in body: + return { + 'error': f'missing required position: {position}' + }, 400 + + txn = UpdateMemberPositionsTransaction(body) + return create_streaming_response(txn) diff --git a/ceod/model/Group.py b/ceod/model/Group.py index cf99e57..3493ce3 100644 --- a/ceod/model/Group.py +++ b/ceod/model/Group.py @@ -105,3 +105,11 @@ class Group: logger.warning(err) raise UserNotInGroupError() self.members.remove(username) + + def set_members(self, usernames: List[str]): + DNs = [ + self.ldap_srv.uid_to_dn(username) for username in usernames + ] + with self.ldap_srv.entry_ctx_for_group(self) as entry: + entry.uniqueMember = DNs + self.members = usernames diff --git a/ceod/model/LDAPService.py b/ceod/model/LDAPService.py index 8c91fc2..218f971 100644 --- a/ceod/model/LDAPService.py +++ b/ceod/model/LDAPService.py @@ -50,7 +50,7 @@ class LDAPService: base, '(objectClass=*)', search_scope=ldap3.BASE, attributes=ldap3.ALL_ATTRIBUTES) except ldap3.core.exceptions.LDAPNoSuchObjectResult: - raise UserNotFoundError() + raise UserNotFoundError(username) return conn.entries[0] def _get_readable_entry_for_group(self, conn: ldap3.Connection, cn: str) -> ldap3.Entry: @@ -60,7 +60,7 @@ class LDAPService: base, '(objectClass=*)', search_scope=ldap3.BASE, attributes=ldap3.ALL_ATTRIBUTES) except ldap3.core.exceptions.LDAPNoSuchObjectResult: - raise GroupNotFoundError() + raise GroupNotFoundError(cn) return conn.entries[0] def _get_writable_entry_for_user(self, user: IUser) -> ldap3.WritableEntry: @@ -96,11 +96,16 @@ class LDAPService: { 'uid': entry.uid.value, 'cn': entry.cn.value, - 'program': entry.program.value, + 'program': entry.program.value or 'Unknown', } for entry in conn.entries ] + def get_users_with_positions(self) -> List[IUser]: + conn = self._get_ldap_conn() + conn.search(self.ldap_users_base, '(position=*)', attributes=ldap3.ALL_ATTRIBUTES) + return [User.deserialize_from_ldap(entry) for entry in conn.entries] + def uid_to_dn(self, uid: str): return f'uid={uid},{self.ldap_users_base}' diff --git a/ceod/model/User.py b/ceod/model/User.py index 7b2c798..5bad209 100644 --- a/ceod/model/User.py +++ b/ceod/model/User.py @@ -67,9 +67,8 @@ class User: 'login_shell': self.login_shell, 'home_directory': self.home_directory, 'is_club': self.is_club(), + 'program': self.program or 'Unknown', } - if self.program: - data['program'] = self.program if self.terms: data['terms'] = self.terms if self.non_member_terms: @@ -158,15 +157,10 @@ class User: entry.nonMemberTerm.add(terms) self.non_member_terms.extend(terms) - def add_position(self, position: str): + def set_positions(self, positions: List[str]): with self.ldap_srv.entry_ctx_for_user(self) as entry: - entry.position.add(position) - self.positions.append(position) - - def remove_position(self, position: str): - with self.ldap_srv.entry_ctx_for_user(self) as entry: - entry.position.delete(position) - self.positions.remove(position) + entry.position = positions + self.positions = positions def get_forwarding_addresses(self) -> List[str]: return self.file_srv.get_forwarding_addresses(self) diff --git a/ceod/transactions/members/UpdateMemberPositionsTransaction.py b/ceod/transactions/members/UpdateMemberPositionsTransaction.py new file mode 100644 index 0000000..32ac77e --- /dev/null +++ b/ceod/transactions/members/UpdateMemberPositionsTransaction.py @@ -0,0 +1,109 @@ +from collections import defaultdict +from typing import Dict + +from zope import component + +from ..AbstractTransaction import AbstractTransaction +from ceo_common.interfaces import ILDAPService, IConfig, IUser +from ceo_common.errors import UserAlreadySubscribedError, UserNotSubscribedError +from ceo_common.logger_factory import logger_factory + +logger = logger_factory(__name__) + + +class UpdateMemberPositionsTransaction(AbstractTransaction): + """Transaction to update the CSC's executive positions.""" + + operations = [ + 'update_positions_ldap', + 'update_exec_group_ldap', + 'subscribe_to_mailing_lists', + ] + + def __init__(self, positions_reversed: Dict[str, str]): + # positions_reversed is position -> username + super().__init__() + self.ldap_srv = component.getUtility(ILDAPService) + + # Reverse the dict so it's easier to use (username -> positions) + self.positions = defaultdict(list) + for position, username in positions_reversed.items(): + self.positions[username].append(position) + + # a cached Dict of the Users who need to be modified (username -> User) + self.users: Dict[str, IUser] = {} + + # for rollback purposes + self.old_positions = {} # username -> positions + self.old_execs = [] + + def child_execute_iter(self): + cfg = component.getUtility(IConfig) + mailing_lists = cfg.get('auxiliary mailing lists_exec') + + # position -> username + new_positions_reversed = {} # For returning result + + # retrieve User objects and cache them + for username in self.positions: + user = self.ldap_srv.get_user(username) + self.users[user.uid] = user + + # Remove positions for old users + for user in self.ldap_srv.get_users_with_positions(): + if user.uid not in self.positions: + self.positions[user.uid] = [] + self.users[user.uid] = user + + # Update positions in LDAP + for username, new_positions in self.positions.items(): + user = self.users[username] + old_positions = user.positions[:] + + user.set_positions(new_positions) + + self.old_positions[username] = old_positions + for position in new_positions: + new_positions_reversed[position] = username + yield 'update_positions_ldap' + + # update exec group in LDAP + exec_group = self.ldap_srv.get_group('exec') + self.old_execs = exec_group.members[:] + new_execs = [ + username for username, new_positions in self.positions.items() + if len(new_positions) > 0 + ] + exec_group.set_members(new_execs) + yield 'update_exec_group_ldap' + + # Update mailing list subscriptions + subscription_failed = False + for username, new_positions in self.positions.items(): + user = self.users[username] + for mailing_list in mailing_lists: + try: + if len(new_positions) > 0: + user.subscribe_to_mailing_list(mailing_list) + else: + user.unsubscribe_from_mailing_list(mailing_list) + except (UserAlreadySubscribedError, UserNotSubscribedError): + pass + except Exception: + logger.warning(f'Failed to update mailing list for {user.uid}') + subscription_failed = True + if subscription_failed: + yield 'failed_to_subscribe_to_mailing_lists' + else: + yield 'subscribe_to_mailing_lists' + + self.finish(new_positions_reversed) + + def rollback(self): + if 'update_exec_group_ldap' in self.finished_operations: + exec_group = self.ldap_srv.get_group('exec') + exec_group.set_members(self.old_execs) + + for username, positions in self.old_positions.items(): + user = self.users[username] + user.set_positions(positions) diff --git a/ceod/transactions/members/__init__.py b/ceod/transactions/members/__init__.py index 8234f15..1443f1f 100644 --- a/ceod/transactions/members/__init__.py +++ b/ceod/transactions/members/__init__.py @@ -2,3 +2,4 @@ from .AddMemberTransaction import AddMemberTransaction from .ModifyMemberTransaction import ModifyMemberTransaction from .RenewMemberTransaction import RenewMemberTransaction from .DeleteMemberTransaction import DeleteMemberTransaction +from .UpdateMemberPositionsTransaction import UpdateMemberPositionsTransaction diff --git a/tests/MockMailmanServer.py b/tests/MockMailmanServer.py index b349f23..561d1ff 100644 --- a/tests/MockMailmanServer.py +++ b/tests/MockMailmanServer.py @@ -35,6 +35,10 @@ class MockMailmanServer: def stop(self): self.loop.call_soon_threadsafe(self.loop.stop) + def clear(self): + for key in self.subscriptions: + self.subscriptions[key].clear() + async def subscribe(self, request): body = await request.post() subscriber = body['subscriber'] diff --git a/tests/ceod/api/test_positions.py b/tests/ceod/api/test_positions.py new file mode 100644 index 0000000..bf6e63e --- /dev/null +++ b/tests/ceod/api/test_positions.py @@ -0,0 +1,93 @@ +from ceod.model import User, Group + + +def test_get_positions(client, ldap_user, g_admin_ctx): + with g_admin_ctx(): + ldap_user.set_positions(['president', 'treasurer']) + status, data = client.get('/api/positions') + assert status == 200 + expected = { + 'president': ldap_user.uid, + 'treasurer': ldap_user.uid, + } + assert data == expected + + +def test_set_positions(cfg, client, g_admin_ctx, mock_mailman_server): + mock_mailman_server.clear() + mailing_lists = cfg.get('auxiliary mailing lists_exec') + base_domain = cfg.get('base_domain') + + users = [] + with g_admin_ctx(): + for uid in ['test_1', 'test_2', 'test_3', 'test_4']: + user = User(uid=uid, cn='Some Name', terms=['s2021']) + user.add_to_ldap() + users.append(user) + exec_group = Group(cn='exec', gid_number=10013) + exec_group.add_to_ldap() + + try: + # missing required position + status, _ = client.post('/api/positions', json={ + 'vice-president': 'test_1', + }) + assert status == 400 + + # non-existent position + status, _ = client.post('/api/positions', json={ + 'president': 'test_1', + 'vice-president': 'test_2', + 'sysadmin': 'test_3', + 'no-such-position': 'test_3', + }) + assert status == 400 + + status, data = client.post('/api/positions', json={ + 'president': 'test_1', + 'vice-president': 'test_2', + 'sysadmin': 'test_3', + }) + assert status == 200 + expected = [ + {"status": "in progress", "operation": "update_positions_ldap"}, + {"status": "in progress", "operation": "update_exec_group_ldap"}, + {"status": "in progress", "operation": "subscribe_to_mailing_lists"}, + {"status": "completed", "result": { + "president": "test_1", + "vice-president": "test_2", + "sysadmin": "test_3", + }}, + ] + assert data == expected + # make sure execs were added to exec group + status, data = client.get('/api/groups/exec') + assert status == 200 + expected = ['test_1', 'test_2', 'test_3'] + assert sorted([item['uid'] for item in data['members']]) == expected + # make sure execs were subscribed to mailing lists + addresses = [f'{uid}@{base_domain}' for uid in expected] + for mailing_list in mailing_lists: + assert sorted(mock_mailman_server.subscriptions[mailing_list]) == addresses + + _, data = client.post('/api/positions', json={ + 'president': 'test_1', + 'vice-president': 'test_2', + 'sysadmin': 'test_2', + 'treasurer': 'test_4', + }) + assert data[-1]['status'] == 'completed' + # make sure old exec was removed from group + expected = ['test_1', 'test_2', 'test_4'] + _, data = client.get('/api/groups/exec') + assert sorted([item['uid'] for item in data['members']]) == expected + # make sure old exec was removed from mailing lists + addresses = [f'{uid}@{base_domain}' for uid in expected] + for mailing_list in mailing_lists: + assert sorted(mock_mailman_server.subscriptions[mailing_list]) == addresses + finally: + with g_admin_ctx(): + for user in users: + user.remove_from_ldap() + exec_group.remove_from_ldap() + mock_mailman_server.clear() diff --git a/tests/ceod/model/test_group.py b/tests/ceod/model/test_group.py index 2dbb3e5..6c4d2f2 100644 --- a/tests/ceod/model/test_group.py +++ b/tests/ceod/model/test_group.py @@ -37,6 +37,10 @@ def test_group_members(ldap_group, ldap_srv): with pytest.raises(UserNotInGroupError): group.remove_member('member1') + group.set_members(['member3']) + assert group.members == ['member3'] + assert ldap_srv.get_group(group.cn).members == group.members + def test_group_to_dict(ldap_group, ldap_user, g_admin_ctx): group = ldap_group diff --git a/tests/ceod/model/test_user.py b/tests/ceod/model/test_user.py index d57c714..fc2c2c2 100644 --- a/tests/ceod/model/test_user.py +++ b/tests/ceod/model/test_user.py @@ -116,16 +116,14 @@ def test_user_terms(ldap_user, ldap_srv): def test_user_positions(ldap_user, ldap_srv): user = ldap_user - user.add_position('treasurer') - assert user.positions == ['treasurer'] - assert ldap_srv.get_user(user.uid).positions == user.positions - user.add_position('cro') - assert user.positions == ['treasurer', 'cro'] + old_positions = user.positions[:] + + new_positions = ['treasurer', 'cro'] + user.set_positions(new_positions) + assert user.positions == new_positions assert ldap_srv.get_user(user.uid).positions == user.positions - user.remove_position('cro') - assert user.positions == ['treasurer'] - assert ldap_srv.get_user(user.uid).positions == user.positions + user.set_positions(old_positions) def test_user_change_password(krb_user): diff --git a/tests/ceod_dev.ini b/tests/ceod_dev.ini index 6e7fd38..f23b130 100644 --- a/tests/ceod_dev.ini +++ b/tests/ceod_dev.ini @@ -52,3 +52,8 @@ office = cdrom,audio,video,www [auxiliary mailing lists] syscom = syscom,syscom-alerts exec = exec + +[positions] +required = president,vice-president,sysadmin +available = president,vice-president,treasurer,secretary, + sysadmin,cro,librarian,imapd,webmaster,offsck diff --git a/tests/ceod_test_local.ini b/tests/ceod_test_local.ini index f14419e..04fdbd9 100644 --- a/tests/ceod_test_local.ini +++ b/tests/ceod_test_local.ini @@ -49,3 +49,8 @@ syscom = office,staff [auxiliary mailing lists] syscom = syscom,syscom-alerts exec = exec + +[positions] +required = president,vice-president,sysadmin +available = president,vice-president,treasurer,secretary, + sysadmin,cro,librarian,imapd,webmaster,offsck From 6917247fdd93c5f07dfcce996fbc0a31453d5f12 Mon Sep 17 00:00:00 2001 From: Max Erenberg Date: Mon, 23 Aug 2021 13:59:01 +0000 Subject: [PATCH 09/14] add members CLI --- ceo/__main__.py | 4 + ceo/cli.py | 43 ++++ ceo/krb_check.py | 28 +++ ceo/members.py | 206 ++++++++++++++++++ ceo/operation_strings.py | 19 ++ ceo/utils.py | 149 +++++++++++++ ceo_common/interfaces/IHTTPClient.py | 19 +- ceo_common/model/HTTPClient.py | 49 +++-- ceo_common/model/RemoteMailmanService.py | 9 +- ceo_common/utils.py | 40 ++++ ceod/api/utils.py | 14 +- .../members/AddMemberTransaction.py | 4 +- requirements.txt | 1 + tests/ceo_dev.ini | 9 + tests/ceod/api/test_members.py | 5 +- tests/conftest.py | 2 - tests/conftest_ceod_api.py | 24 +- 17 files changed, 585 insertions(+), 40 deletions(-) create mode 100644 ceo/__main__.py create mode 100644 ceo/cli.py create mode 100644 ceo/krb_check.py create mode 100644 ceo/members.py create mode 100644 ceo/operation_strings.py create mode 100644 ceo/utils.py create mode 100644 ceo_common/utils.py create mode 100644 tests/ceo_dev.ini diff --git a/ceo/__main__.py b/ceo/__main__.py new file mode 100644 index 0000000..aa3acee --- /dev/null +++ b/ceo/__main__.py @@ -0,0 +1,4 @@ +from .cli import cli + +if __name__ == '__main__': + cli(obj={}) diff --git a/ceo/cli.py b/ceo/cli.py new file mode 100644 index 0000000..e590b2f --- /dev/null +++ b/ceo/cli.py @@ -0,0 +1,43 @@ +import importlib.resources +import os +import socket + +import click +from zope import component + +from .krb_check import krb_check +from .members import members +from ceo_common.interfaces import IConfig, IHTTPClient +from ceo_common.model import Config, HTTPClient + + +@click.group() +@click.pass_context +def cli(ctx): + # ensure ctx exists and is a dict + ctx.ensure_object(dict) + + princ = krb_check() + user = princ[:princ.index('@')] + ctx.obj['user'] = user + + register_services() + + +cli.add_command(members) + + +def register_services(): + # Config + # This is a hack to determine if we're in the dev env or not + if socket.getfqdn().endswith('.csclub.internal'): + with importlib.resources.path('tests', 'ceo_dev.ini') as p: + config_file = p.__fspath__() + else: + config_file = os.environ.get('CEO_CONFIG', '/etc/csc/ceo.ini') + cfg = Config(config_file) + component.provideUtility(cfg, IConfig) + + # HTTPService + http_client = HTTPClient() + component.provideUtility(http_client, IHTTPClient) diff --git a/ceo/krb_check.py b/ceo/krb_check.py new file mode 100644 index 0000000..f9f9ce8 --- /dev/null +++ b/ceo/krb_check.py @@ -0,0 +1,28 @@ +import subprocess + +import gssapi + + +def krb_check(): + """ + Spawns a `kinit` process if no credentials are available or the + credentials have expired. + Returns the principal string 'user@REALM'. + """ + try: + creds = gssapi.Credentials(usage='initiate') + except gssapi.raw.misc.GSSError: + kinit() + creds = gssapi.Credentials(usage='initiate') + + try: + result = creds.inquire() + except gssapi.raw.exceptions.ExpiredCredentialsError: + kinit() + result = creds.inquire() + + return str(result.name) + + +def kinit(): + subprocess.run(['kinit'], check=True) diff --git a/ceo/members.py b/ceo/members.py new file mode 100644 index 0000000..b16caaa --- /dev/null +++ b/ceo/members.py @@ -0,0 +1,206 @@ +import socket +import sys +from typing import Dict + +import click +from zope import component + +from .utils import http_post, http_get, http_patch, http_delete, \ + handle_stream_response, handle_sync_response, print_colon_kv, \ + get_failed_operations +from ceo_common.interfaces import IConfig +from ceo_common.utils import get_current_term, add_term, get_max_term +from ceod.transactions.members import ( + AddMemberTransaction, + DeleteMemberTransaction, +) + + +@click.group() +def members(): + pass + + +@members.command(short_help='Add a new member or club rep') +@click.argument('username') +@click.option('--cn', help='Full name', prompt='Full name') +@click.option('--program', required=False, help='Academic program') +@click.option('--terms', 'num_terms', type=click.IntRange(1, 100), + help='Number of terms to add', prompt='Number of terms') +@click.option('--clubrep', is_flag=True, default=False, + help='Add non-member terms instead of member terms') +@click.option('--forwarding-address', required=False, + help=('Forwarding address to set in ~/.forward. ' + 'Default is UW address. ' + 'Set to the empty string to disable forwarding.')) +def add(username, cn, program, num_terms, clubrep, forwarding_address): + cfg = component.getUtility(IConfig) + uw_domain = cfg.get('uw_domain') + + current_term = get_current_term() + terms = [current_term] + for _ in range(1, num_terms): + term = add_term(terms[-1]) + terms.append(term) + if forwarding_address is None: + forwarding_address = username + '@' + uw_domain + + click.echo("The following user will be created:") + lines = [ + ('uid', username), + ('cn', cn), + ] + if program is not None: + lines.append(('program', program)) + if clubrep: + lines.append(('non-member terms', ','.join(terms))) + else: + lines.append(('member terms', ','.join(terms))) + if forwarding_address != '': + lines.append(('forwarding address', forwarding_address)) + print_colon_kv(lines) + + click.confirm('Do you want to continue?', abort=True) + + body = { + 'uid': username, + 'cn': cn, + } + if program is not None: + body['program'] = program + if clubrep: + body['terms'] = terms + else: + body['non_member_terms'] = terms + if forwarding_address != '': + body['forwarding_addresses'] = [forwarding_address] + resp = http_post('/api/members', json=body) + data = handle_stream_response(resp, AddMemberTransaction.operations) + result = data[-1]['result'] + print_user_lines(result) + + failed_operations = get_failed_operations(data) + if 'send_welcome_message' in failed_operations: + click.echo(click.style( + 'Warning: welcome message was not sent. You now need to manually ' + 'send the user their password.', fg='yellow')) + + +def print_user_lines(result: Dict): + """Pretty-print a user JSON response.""" + lines = [ + ('uid', result['uid']), + ('cn', result['cn']), + ('program', result.get('program', 'Unknown')), + ('UID number', result['uid_number']), + ('GID number', result['gid_number']), + ('login shell', result['login_shell']), + ('home directory', result['home_directory']), + ('is a club', result['is_club']), + ] + if 'forwarding_addresses' in result: + lines.append(('forwarding addresses', ','.join(result['forwarding_addresses']))) + if 'terms' in result: + lines.append(('terms', ','.join(result['terms']))) + if 'non_member_terms' in result: + lines.append(('non-member terms', ','.join(result['non_member_terms']))) + if 'password' in result: + lines.append(('password', result['password'])) + print_colon_kv(lines) + + +@members.command(short_help='Get info about a user') +@click.argument('username') +def get(username): + resp = http_get('/api/members/' + username) + result = handle_sync_response(resp) + print_user_lines(result) + + +@members.command(short_help="Replace a user's login shell or forwarding addresses") +@click.argument('username') +@click.option('--login-shell', required=False, help='Login shell') +@click.option('--forwarding-addresses', required=False, + help='Comma-separated list of forwarding addresses') +def modify(username, login_shell, forwarding_addresses): + if login_shell is None and forwarding_addresses is None: + click.echo('Nothing to do.') + sys.exit() + operations = [] + body = {} + if login_shell is not None: + body['login_shell'] = login_shell + operations.append('replace_login_shell') + click.echo('Login shell will be set to: ' + login_shell) + if forwarding_addresses is not None: + forwarding_addresses = forwarding_addresses.split(',') + body['forwarding_addresses'] = forwarding_addresses + operations.append('replace_forwarding_addresses') + prefix = '~/.forward will be set to: ' + click.echo(prefix + forwarding_addresses[0]) + for address in forwarding_addresses[1:]: + click.echo((' ' * len(prefix)) + address) + + click.confirm('Do you want to continue?', abort=True) + + resp = http_patch('/api/members/' + username, json=body) + handle_stream_response(resp, operations) + + +@members.command(short_help="Renew a member or club rep's membership") +@click.argument('username') +@click.option('--terms', 'num_terms', type=click.IntRange(1, 100), + help='Number of terms to add', prompt='Number of terms') +@click.option('--clubrep', is_flag=True, default=False, + help='Add non-member terms instead of member terms') +def renew(username, num_terms, clubrep): + resp = http_get('/api/members/' + username) + result = handle_sync_response(resp) + max_term = None + if clubrep and 'non_member_terms' in result: + max_term = get_max_term(result['non_member_terms']) + elif not clubrep and 'terms' in result: + max_term = get_max_term(result['terms']) + if max_term is not None: + max_term = get_max_term([max_term, get_current_term()]) + else: + max_term = get_current_term() + + terms = [add_term(max_term)] + for _ in range(1, num_terms): + term = add_term(terms[-1]) + terms.append(term) + + if clubrep: + body = {'non_member_terms': terms} + click.echo('The following non-member terms will be added: ' + ','.join(terms)) + else: + body = {'terms': terms} + click.echo('The following member terms will be added: ' + ','.join(terms)) + + click.confirm('Do you want to continue?', abort=True) + + resp = http_post(f'/api/members/{username}/renew', json=body) + handle_sync_response(resp) + click.echo('Done.') + + +@members.command(short_help="Reset a user's password") +@click.argument('username') +def pwreset(username): + click.confirm(f"Are you sure you want to reset {username}'s password?", abort=True) + resp = http_post(f'/api/members/{username}/pwreset') + result = handle_sync_response(resp) + click.echo('New password: ' + result['password']) + + +@members.command(short_help="Delete a user") +@click.argument('username') +def delete(username): + # a hack to determine if we're in the dev environment + if not socket.getfqdn().endswith('.csclub.internal'): + click.echo('This command may only be called during development.') + sys.exit(1) + click.confirm(f"Are you sure you want to delete {username}?", abort=True) + resp = http_delete(f'/api/members/{username}') + handle_stream_response(resp, DeleteMemberTransaction.operations) diff --git a/ceo/operation_strings.py b/ceo/operation_strings.py new file mode 100644 index 0000000..48e7d30 --- /dev/null +++ b/ceo/operation_strings.py @@ -0,0 +1,19 @@ +# These descriptions are printed to the console while a transaction +# is performed, in real time. +descriptions = { + 'add_user_to_ldap': 'Add user to LDAP', + 'add_group_to_ldap': 'Add group to LDAP', + 'add_user_to_kerberos': 'Add user to Kerberos', + 'create_home_dir': 'Create home directory', + 'set_forwarding_addresses': 'Set forwarding addresses', + 'send_welcome_message': 'Send welcome message', + 'subscribe_to_mailing_list': 'Subscribe to mailing list', + 'announce_new_user': 'Announce new user to mailing list', + 'replace_login_shell': 'Replace login shell', + 'replace_forwarding_addresses': 'Replace forwarding addresses', + 'remove_user_from_ldap': 'Remove user from LDAP', + 'remove_group_from_ldap': 'Remove group from LDAP', + 'remove_user_from_kerberos': 'Remove user from Kerberos', + 'delete_home_dir': 'Delete home directory', + 'unsubscribe_from_mailing_list': 'Unsubscribe from mailing list', +} diff --git a/ceo/utils.py b/ceo/utils.py new file mode 100644 index 0000000..b84386c --- /dev/null +++ b/ceo/utils.py @@ -0,0 +1,149 @@ +import json +import sys +from typing import List, Tuple, Dict + +import click +import requests +from zope import component + +from .operation_strings import descriptions as op_desc +from ceo_common.interfaces import IHTTPClient, IConfig + + +def http_request(method: str, path: str, **kwargs) -> requests.Response: + client = component.getUtility(IHTTPClient) + cfg = component.getUtility(IConfig) + if path.startswith('/api/db'): + host = cfg.get('ceod_db_host') + need_cred = False + else: + host = cfg.get('ceod_admin_host') + # The forwarded TGT is only needed for endpoints which write to LDAP + need_cred = method != 'GET' + return client.request( + host, path, method, principal=None, need_cred=need_cred, + stream=True, **kwargs) + + +def http_get(path: str, **kwargs) -> requests.Response: + return http_request('GET', path, **kwargs) + + +def http_post(path: str, **kwargs) -> requests.Response: + return http_request('POST', path, **kwargs) + + +def http_patch(path: str, **kwargs) -> requests.Response: + return http_request('PATCH', path, **kwargs) + + +def http_delete(path: str, **kwargs) -> requests.Response: + return http_request('DELETE', path, **kwargs) + + +def handle_stream_response(resp: requests.Response, operations: List[str]) -> List[Dict]: + """ + Print output to the console while operations are being streamed + from the server over HTTP. + Returns the parsed JSON data streamed from the server. + """ + if resp.status_code != 200: + click.echo('An error occurred:') + click.echo(resp.text) + sys.exit(1) + click.echo(op_desc[operations[0]] + '... ', nl=False) + idx = 0 + data = [] + for line in resp.iter_lines(decode_unicode=True, chunk_size=8): + d = json.loads(line) + data.append(d) + if d['status'] == 'aborted': + click.echo(click.style('ABORTED', fg='red')) + click.echo('The transaction was rolled back.') + click.echo('The error was: ' + d['error']) + click.echo('Please check the ceod logs.') + sys.exit(1) + elif d['status'] == 'completed': + click.echo('Transaction successfully completed.') + return data + + operation = d['operation'] + oper_failed = False + err_msg = None + prefix = 'failed_to_' + if operation.startswith(prefix): + operation = operation[len(prefix):] + oper_failed = True + # sometimes the operation looks like + # "failed_to_do_something: error message" + if ':' in operation: + operation, err_msg = operation.split(': ', 1) + + while idx < len(operations) and operations[idx] != operation: + click.echo('Skipped') + idx += 1 + if idx == len(operations): + break + click.echo(op_desc[operations[idx]] + '... ', nl=False) + if idx == len(operations): + click.echo('Unrecognized operation: ' + operation) + continue + if oper_failed: + click.echo(click.style('Failed', fg='red')) + if err_msg is not None: + click.echo(' Error message: ' + err_msg) + else: + click.echo(click.style('Done', fg='green')) + idx += 1 + if idx < len(operations): + click.echo(op_desc[operations[idx]] + '... ', nl=False) + + raise Exception('server response ended abruptly') + + +def handle_sync_response(resp: requests.Response): + """ + Exit the program if the request was not successful. + Returns the parsed JSON response. + """ + if resp.status_code // 100 != 2: + click.echo('An error occurred:') + click.echo(resp.text) + sys.exit(1) + return resp.json() + + +def print_colon_kv(pairs: List[Tuple[str, str]]): + """ + Pretty-print a list of key-value pairs such that the key and value + columns align. + Example: + key1: value1 + key1000: value2 + """ + maxlen = max(len(key) for key, val in pairs) + for key, val in pairs: + click.echo(key + ': ', nl=False) + extra_space = ' ' * (maxlen - len(key)) + click.echo(extra_space, nl=False) + click.echo(val) + + +def get_failed_operations(data: List[Dict]) -> List[str]: + """ + Get a list of the failed operations using the JSON objects + streamed from the server. + """ + prefix = 'failed_to_' + failed = [] + for d in data: + if 'operation' not in d: + continue + operation = d['operation'] + if not operation.startswith(prefix): + continue + operation = operation[len(prefix):] + if ':' in operation: + operation = operation[:operation.index(':')] + failed.append(operation) + return failed diff --git a/ceo_common/interfaces/IHTTPClient.py b/ceo_common/interfaces/IHTTPClient.py index 0bec342..61c9599 100644 --- a/ceo_common/interfaces/IHTTPClient.py +++ b/ceo_common/interfaces/IHTTPClient.py @@ -1,14 +1,27 @@ +from typing import Union + from zope.interface import Interface class IHTTPClient(Interface): """A helper class for HTTP requests to ceod.""" - def get(host: str, api_path: str, **kwargs): + def request(host: str, api_path: str, method: str, principal: str, + need_cred: bool, **kwargs): + """Make an HTTP request.""" + + def get(host: str, api_path: str, principal: Union[str, None] = None, + need_cred: bool = True, **kwargs): """Make a GET request.""" - def post(host: str, api_path: str, **kwargs): + def post(host: str, api_path: str, principal: Union[str, None] = None, + need_cred: bool = True, **kwargs): """Make a POST request.""" - def delete(host: str, api_path: str, **kwargs): + def patch(host: str, api_path: str, principal: Union[str, None] = None, + need_cred: bool = True, **kwargs): + """Make a PATCH request.""" + + def delete(host: str, api_path: str, principal: Union[str, None] = None, + need_cred: bool = True, **kwargs): """Make a DELETE request.""" diff --git a/ceo_common/model/HTTPClient.py b/ceo_common/model/HTTPClient.py index 81d4bc5..4da38fc 100644 --- a/ceo_common/model/HTTPClient.py +++ b/ceo_common/model/HTTPClient.py @@ -1,10 +1,13 @@ -from flask import g +from base64 import b64encode +from typing import Union + import gssapi import requests from requests_gssapi import HTTPSPNEGOAuth from zope import component from zope.interface import implementer +from ceo_common.krb5.utils import get_fwd_tgt from ceo_common.interfaces import IConfig, IHTTPClient @@ -20,32 +23,48 @@ class HTTPClient: self.ceod_port = cfg.get('ceod_port') self.base_domain = cfg.get('base_domain') - self.krb_realm = cfg.get('ldap_sasl_realm') - - def request(self, host: str, api_path: str, method='GET', **kwargs): - principal = g.sasl_user - gssapi_name = gssapi.Name(principal) - creds = gssapi.Credentials(name=gssapi_name, usage='initiate') + def request(self, host: str, api_path: str, method: str, principal: str, + need_cred: bool, **kwargs): # always use the FQDN if '.' not in host: host = host + '.' + self.base_domain + + # SPNEGO + if principal is not None: + gssapi_name = gssapi.Name(principal) + creds = gssapi.Credentials(name=gssapi_name, usage='initiate') + else: + creds = None auth = HTTPSPNEGOAuth( opportunistic_auth=True, target_name=gssapi.Name('ceod/' + host), creds=creds, ) + + # Forwarded TGT (X-KRB5-CRED) + headers = {} + if need_cred: + b = get_fwd_tgt('ceod/' + host) + headers['X-KRB5-CRED'] = b64encode(b).decode() + return requests.request( method, f'{self.scheme}://{host}:{self.ceod_port}{api_path}', - auth=auth, - **kwargs, + auth=auth, headers=headers, **kwargs, ) - def get(self, host: str, api_path: str, **kwargs): - return self.request(host, api_path, 'GET', **kwargs) + def get(self, host: str, api_path: str, principal: Union[str, None] = None, + need_cred: bool = False, **kwargs): + return self.request(host, api_path, 'GET', principal, need_cred, **kwargs) - def post(self, host: str, api_path: str, **kwargs): - return self.request(host, api_path, 'POST', **kwargs) + def post(self, host: str, api_path: str, principal: Union[str, None] = None, + need_cred: bool = False, **kwargs): + return self.request(host, api_path, 'POST', principal, need_cred, **kwargs) - def delete(self, host: str, api_path: str, **kwargs): - return self.request(host, api_path, 'DELETE', **kwargs) + def patch(self, host: str, api_path: str, principal: Union[str, None] = None, + need_cred: bool = False, **kwargs): + return self.request(host, api_path, 'PATCH', principal, need_cred, **kwargs) + + def delete(self, host: str, api_path: str, principal: Union[str, None] = None, + need_cred: bool = False, **kwargs): + return self.request(host, api_path, 'DELETE', principal, need_cred, **kwargs) diff --git a/ceo_common/model/RemoteMailmanService.py b/ceo_common/model/RemoteMailmanService.py index 03b07ee..d9e70e0 100644 --- a/ceo_common/model/RemoteMailmanService.py +++ b/ceo_common/model/RemoteMailmanService.py @@ -1,3 +1,4 @@ +from flask import g from zope import component from zope.interface import implementer @@ -14,7 +15,9 @@ class RemoteMailmanService: self.http_client = component.getUtility(IHTTPClient) def subscribe(self, address: str, mailing_list: str): - resp = self.http_client.post(self.mailman_host, f'/api/mailman/{mailing_list}/{address}') + resp = self.http_client.post( + self.mailman_host, f'/api/mailman/{mailing_list}/{address}', + principal=g.sasl_user) if not resp.ok: if resp.status_code == 409: raise UserAlreadySubscribedError() @@ -23,7 +26,9 @@ class RemoteMailmanService: raise Exception(resp.json()) def unsubscribe(self, address: str, mailing_list: str): - resp = self.http_client.delete(self.mailman_host, f'/api/mailman/{mailing_list}/{address}') + resp = self.http_client.delete( + self.mailman_host, f'/api/mailman/{mailing_list}/{address}', + principal=g.sasl_user) if not resp.ok: if resp.status_code == 404: raise UserNotSubscribedError() diff --git a/ceo_common/utils.py b/ceo_common/utils.py new file mode 100644 index 0000000..a7d858b --- /dev/null +++ b/ceo_common/utils.py @@ -0,0 +1,40 @@ +import datetime +from typing import List + + +def get_current_term() -> str: + """ + Get the current term as formatted in the CSC LDAP (e.g. 's2021'). + """ + dt = datetime.datetime.now() + c = 'w' + if 5 <= dt.month <= 8: + c = 's' + elif 9 <= dt.month: + c = 'f' + return c + str(dt.year) + + +def add_term(term: str) -> str: + """ + Add one term to the given term and return the string. + Example: add_term('s2021') -> 'f2021' + """ + c = term[0] + s_year = term[1:] + if c == 'w': + return 's' + s_year + elif c == 's': + return 'f' + s_year + year = int(s_year) + return 'w' + str(year + 1) + + +def get_max_term(terms: List[str]) -> str: + """Get the maximum (latest) term.""" + max_year = max(term[1:] for term in terms) + if 'f' + max_year in terms: + return 'f' + max_year + elif 's' + max_year in terms: + return 's' + max_year + return 'w' + max_year diff --git a/ceod/api/utils.py b/ceod/api/utils.py index 42f900e..3db9056 100644 --- a/ceod/api/utils.py +++ b/ceod/api/utils.py @@ -84,9 +84,10 @@ def create_streaming_response(txn: AbstractTransaction): indicating the progress of the transaction. """ def generate(): + generator = txn.execute_iter() try: - for operation in txn.execute_iter(): - operation = yield json.dumps({ + for operation in generator: + yield json.dumps({ 'status': 'in progress', 'operation': operation, }) + '\n' @@ -94,6 +95,15 @@ def create_streaming_response(txn: AbstractTransaction): 'status': 'completed', 'result': txn.result, }) + '\n' + except GeneratorExit: + # Keep on going. Even if the client closes the connection, we don't + # want to give up half way through. + try: + for operation in generator: + pass + except Exception: + logger.warning('Transaction failed:\n' + traceback.format_exc()) + txn.rollback() except Exception as err: logger.warning('Transaction failed:\n' + traceback.format_exc()) txn.rollback() diff --git a/ceod/transactions/members/AddMemberTransaction.py b/ceod/transactions/members/AddMemberTransaction.py index 2479864..38cf539 100644 --- a/ceod/transactions/members/AddMemberTransaction.py +++ b/ceod/transactions/members/AddMemberTransaction.py @@ -22,8 +22,8 @@ class AddMemberTransaction(AbstractTransaction): 'add_user_to_kerberos', 'create_home_dir', 'set_forwarding_addresses', - 'subscribe_to_mailing_list', 'send_welcome_message', + 'subscribe_to_mailing_list', 'announce_new_user', ] @@ -78,7 +78,7 @@ class AddMemberTransaction(AbstractTransaction): if self.forwarding_addresses: user.set_forwarding_addresses(self.forwarding_addresses) - yield 'set_forwarding_addresses' + yield 'set_forwarding_addresses' # The following operations can't/shouldn't be rolled back because the # user has already seen the email diff --git a/requirements.txt b/requirements.txt index 7023ac8..d416050 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,3 +1,4 @@ +click==8.0.1 Flask==2.0.1 gssapi==1.6.14 Jinja2==3.0.1 diff --git a/tests/ceo_dev.ini b/tests/ceo_dev.ini new file mode 100644 index 0000000..e74895e --- /dev/null +++ b/tests/ceo_dev.ini @@ -0,0 +1,9 @@ +[DEFAULT] +base_domain = csclub.internal +uw_domain = uwaterloo.internal + +[ceod] +# this is the host with the ceod/admin Kerberos key +admin_host = phosphoric-acid +use_https = false +port = 9987 diff --git a/tests/ceod/api/test_members.py b/tests/ceod/api/test_members.py index d82b2af..79e4c6f 100644 --- a/tests/ceod/api/test_members.py +++ b/tests/ceod/api/test_members.py @@ -18,6 +18,7 @@ def create_user_resp(client, mocks_for_create_user): 'cn': 'Test One', 'program': 'Math', 'terms': ['s2021'], + 'forwarding_addresses': ['test_1@uwaterloo.internal'], }) assert status == 200 assert data[-1]['status'] == 'completed' @@ -56,7 +57,7 @@ def test_api_create_user(cfg, create_user_resp): "is_club": False, "program": "Math", "terms": ["s2021"], - "forwarding_addresses": [], + "forwarding_addresses": ['test_1@uwaterloo.internal'], "password": "krb5" }}, ] @@ -209,5 +210,5 @@ def test_authz_check(client, create_user_result): # If we're syscom but we don't pass credentials, the request should fail _, data = client.post('/api/members', json={ 'uid': 'test_1', 'cn': 'Test One', 'terms': ['s2021'], - }, principal='ctdalek', no_creds=True) + }, principal='ctdalek', need_cred=False) assert data[-1]['status'] == 'aborted' diff --git a/tests/conftest.py b/tests/conftest.py index 5b252a1..3388fe6 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -52,8 +52,6 @@ def delete_test_princs(krb_srv): 'kadmin', '-k', '-p', krb_srv.admin_principal, 'listprincs', 'test_*', ], text=True, capture_output=True, check=True) princs = [line.strip() for line in proc.stdout.splitlines()] - # remove the password prompt - princs = princs[1:] for princ in princs: krb_srv.delprinc(princ) diff --git a/tests/conftest_ceod_api.py b/tests/conftest_ceod_api.py index 4dbeb95..19f16ba 100644 --- a/tests/conftest_ceod_api.py +++ b/tests/conftest_ceod_api.py @@ -72,7 +72,7 @@ class CeodTestClient: text=True, input='krb5', check=True, stdout=subprocess.DEVNULL, env={'KRB5CCNAME': self.principal_ccaches[principal]}) - def get_headers(self, principal: str, no_creds: bool): + def get_headers(self, principal: str, need_cred: bool): if principal not in self.principal_ccaches: _, filename = tempfile.mkstemp(dir=self.cache_dir) self.principal_ccaches[principal] = filename @@ -82,7 +82,7 @@ class CeodTestClient: # the header using req.prepare(). req = Request('GET', self.base_url, auth=self.get_auth(principal)) headers = list(req.prepare().headers.items()) - if not no_creds: + if need_cred: # Get the X-KRB5-CRED header (forwarded TGT). cred = b64encode(get_fwd_tgt( 'ceod/' + socket.getfqdn(), self.principal_ccaches[principal] @@ -90,14 +90,14 @@ class CeodTestClient: headers.append(('X-KRB5-CRED', cred)) return headers - def request(self, method: str, path: str, principal: str, no_creds: bool, **kwargs): + def request(self, method: str, path: str, principal: str, need_cred: bool, **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 - headers = self.get_headers(principal, no_creds) + headers = self.get_headers(principal, need_cred) resp = self.client.open(path, method=method, headers=headers, **kwargs) status = int(resp.status.split(' ', 1)[0]) if resp.headers['content-type'] == 'application/json': @@ -106,14 +106,14 @@ class CeodTestClient: data = [json.loads(line) for line in resp.data.splitlines()] return status, data - def get(self, path, principal=None, no_creds=False, **kwargs): - return self.request('GET', path, principal, no_creds, **kwargs) + def get(self, path, principal=None, need_cred=True, **kwargs): + return self.request('GET', path, principal, need_cred, **kwargs) - def post(self, path, principal=None, no_creds=False, **kwargs): - return self.request('POST', path, principal, no_creds, **kwargs) + def post(self, path, principal=None, need_cred=True, **kwargs): + return self.request('POST', path, principal, need_cred, **kwargs) - def patch(self, path, principal=None, no_creds=False, **kwargs): - return self.request('PATCH', path, principal, no_creds, **kwargs) + def patch(self, path, principal=None, need_cred=True, **kwargs): + return self.request('PATCH', path, principal, need_cred, **kwargs) - def delete(self, path, principal=None, no_creds=False, **kwargs): - return self.request('DELETE', path, principal, no_creds, **kwargs) + def delete(self, path, principal=None, need_cred=True, **kwargs): + return self.request('DELETE', path, principal, need_cred, **kwargs) From 08a3faaefc881c7491e0cff3997f175a4dc4813e Mon Sep 17 00:00:00 2001 From: Max Erenberg Date: Mon, 23 Aug 2021 23:01:24 +0000 Subject: [PATCH 10/14] add unit tests for members CLI --- ceo/cli/__init__.py | 1 + ceo/{cli.py => cli/entrypoint.py} | 5 +- ceo/{ => cli}/members.py | 66 +++++---- ceo/cli/utils.py | 107 ++++++++++++++ ceo/krb_check.py | 20 ++- ceo/utils.py | 96 +------------ ceo_common/model/Term.py | 67 +++++++++ ceo_common/model/__init__.py | 1 + ceo_common/utils.py | 40 ------ tests/ceo/__init__.py | 0 tests/ceo/cli/__init__.py | 0 tests/ceo/cli/test_members.py | 136 ++++++++++++++++++ tests/ceo_common/model/test_remote_mailman.py | 48 ++----- tests/ceod_test_local.ini | 2 + tests/conftest.py | 104 ++++++++------ tests/conftest_ceo.py | 18 +++ tests/conftest_ceod_api.py | 63 ++------ tests/utils.py | 34 +++++ 18 files changed, 502 insertions(+), 306 deletions(-) create mode 100644 ceo/cli/__init__.py rename ceo/{cli.py => cli/entrypoint.py} (90%) rename ceo/{ => cli}/members.py (80%) create mode 100644 ceo/cli/utils.py create mode 100644 ceo_common/model/Term.py delete mode 100644 ceo_common/utils.py create mode 100644 tests/ceo/__init__.py create mode 100644 tests/ceo/cli/__init__.py create mode 100644 tests/ceo/cli/test_members.py create mode 100644 tests/conftest_ceo.py create mode 100644 tests/utils.py diff --git a/ceo/cli/__init__.py b/ceo/cli/__init__.py new file mode 100644 index 0000000..d3a9b7d --- /dev/null +++ b/ceo/cli/__init__.py @@ -0,0 +1 @@ +from .entrypoint import cli diff --git a/ceo/cli.py b/ceo/cli/entrypoint.py similarity index 90% rename from ceo/cli.py rename to ceo/cli/entrypoint.py index e590b2f..9389c56 100644 --- a/ceo/cli.py +++ b/ceo/cli/entrypoint.py @@ -5,7 +5,7 @@ import socket import click from zope import component -from .krb_check import krb_check +from ..krb_check import krb_check from .members import members from ceo_common.interfaces import IConfig, IHTTPClient from ceo_common.model import Config, HTTPClient @@ -21,7 +21,8 @@ def cli(ctx): user = princ[:princ.index('@')] ctx.obj['user'] = user - register_services() + if os.environ.get('PYTEST') != '1': + register_services() cli.add_command(members) diff --git a/ceo/members.py b/ceo/cli/members.py similarity index 80% rename from ceo/members.py rename to ceo/cli/members.py index b16caaa..67cd049 100644 --- a/ceo/members.py +++ b/ceo/cli/members.py @@ -5,11 +5,10 @@ from typing import Dict import click from zope import component -from .utils import http_post, http_get, http_patch, http_delete, \ - handle_stream_response, handle_sync_response, print_colon_kv, \ - get_failed_operations +from ..utils import http_post, http_get, http_patch, http_delete, get_failed_operations +from .utils import handle_stream_response, handle_sync_response, print_colon_kv from ceo_common.interfaces import IConfig -from ceo_common.utils import get_current_term, add_term, get_max_term +from ceo_common.model import Term from ceod.transactions.members import ( AddMemberTransaction, DeleteMemberTransaction, @@ -37,11 +36,10 @@ def add(username, cn, program, num_terms, clubrep, forwarding_address): cfg = component.getUtility(IConfig) uw_domain = cfg.get('uw_domain') - current_term = get_current_term() - terms = [current_term] - for _ in range(1, num_terms): - term = add_term(terms[-1]) - terms.append(term) + current_term = Term.current() + terms = [current_term + i for i in range(num_terms)] + terms = list(map(str, terms)) + if forwarding_address is None: forwarding_address = username + '@' + uw_domain @@ -69,13 +67,18 @@ def add(username, cn, program, num_terms, clubrep, forwarding_address): if program is not None: body['program'] = program if clubrep: - body['terms'] = terms - else: body['non_member_terms'] = terms + else: + body['terms'] = terms if forwarding_address != '': body['forwarding_addresses'] = [forwarding_address] + operations = AddMemberTransaction.operations + if forwarding_address == '': + # don't bother displaying this because it won't be run + operations.remove('set_forwarding_addresses') + resp = http_post('/api/members', json=body) - data = handle_stream_response(resp, AddMemberTransaction.operations) + data = handle_stream_response(resp, operations) result = data[-1]['result'] print_user_lines(result) @@ -121,7 +124,10 @@ def get(username): @click.argument('username') @click.option('--login-shell', required=False, help='Login shell') @click.option('--forwarding-addresses', required=False, - help='Comma-separated list of forwarding addresses') + help=( + 'Comma-separated list of forwarding addresses. ' + 'Set to the empty string to disable forwarding.' + )) def modify(username, login_shell, forwarding_addresses): if login_shell is None and forwarding_addresses is None: click.echo('Nothing to do.') @@ -133,13 +139,19 @@ def modify(username, login_shell, forwarding_addresses): operations.append('replace_login_shell') click.echo('Login shell will be set to: ' + login_shell) if forwarding_addresses is not None: - forwarding_addresses = forwarding_addresses.split(',') + if forwarding_addresses == '': + forwarding_addresses = [] + else: + forwarding_addresses = forwarding_addresses.split(',') body['forwarding_addresses'] = forwarding_addresses operations.append('replace_forwarding_addresses') prefix = '~/.forward will be set to: ' - click.echo(prefix + forwarding_addresses[0]) - for address in forwarding_addresses[1:]: - click.echo((' ' * len(prefix)) + address) + if len(forwarding_addresses) > 0: + click.echo(prefix + forwarding_addresses[0]) + for address in forwarding_addresses[1:]: + click.echo((' ' * len(prefix)) + address) + else: + click.echo(prefix) click.confirm('Do you want to continue?', abort=True) @@ -157,19 +169,19 @@ def renew(username, num_terms, clubrep): resp = http_get('/api/members/' + username) result = handle_sync_response(resp) max_term = None + current_term = Term.current() if clubrep and 'non_member_terms' in result: - max_term = get_max_term(result['non_member_terms']) + max_term = max(Term(s) for s in result['non_member_terms']) elif not clubrep and 'terms' in result: - max_term = get_max_term(result['terms']) - if max_term is not None: - max_term = get_max_term([max_term, get_current_term()]) - else: - max_term = get_current_term() + max_term = max(Term(s) for s in result['terms']) - terms = [add_term(max_term)] - for _ in range(1, num_terms): - term = add_term(terms[-1]) - terms.append(term) + if max_term is not None and max_term >= current_term: + next_term = max_term + 1 + else: + next_term = Term.current() + + terms = [next_term + i for i in range(num_terms)] + terms = list(map(str, terms)) if clubrep: body = {'non_member_terms': terms} diff --git a/ceo/cli/utils.py b/ceo/cli/utils.py new file mode 100644 index 0000000..cc25e84 --- /dev/null +++ b/ceo/cli/utils.py @@ -0,0 +1,107 @@ +import json +import sys +from typing import List, Tuple, Dict + +import click +import requests + +from ..operation_strings import descriptions as op_desc + + +class Abort(click.ClickException): + """Abort silently.""" + + def __init__(self, exit_code=1): + super().__init__('') + self.exit_code = exit_code + + def show(self): + pass + + +def print_colon_kv(pairs: List[Tuple[str, str]]): + """ + Pretty-print a list of key-value pairs such that the key and value + columns align. + Example: + key1: value1 + key1000: value2 + """ + maxlen = max(len(key) for key, val in pairs) + for key, val in pairs: + click.echo(key + ': ', nl=False) + extra_space = ' ' * (maxlen - len(key)) + click.echo(extra_space, nl=False) + click.echo(val) + + +def handle_stream_response(resp: requests.Response, operations: List[str]) -> List[Dict]: + """ + Print output to the console while operations are being streamed + from the server over HTTP. + Returns the parsed JSON data streamed from the server. + """ + if resp.status_code != 200: + click.echo('An error occurred:') + click.echo(resp.text.rstrip()) + raise Abort() + click.echo(op_desc[operations[0]] + '... ', nl=False) + idx = 0 + data = [] + for line in resp.iter_lines(decode_unicode=True, chunk_size=8): + d = json.loads(line) + data.append(d) + if d['status'] == 'aborted': + click.echo(click.style('ABORTED', fg='red')) + click.echo('The transaction was rolled back.') + click.echo('The error was: ' + d['error']) + click.echo('Please check the ceod logs.') + sys.exit(1) + elif d['status'] == 'completed': + click.echo('Transaction successfully completed.') + return data + + operation = d['operation'] + oper_failed = False + err_msg = None + prefix = 'failed_to_' + if operation.startswith(prefix): + operation = operation[len(prefix):] + oper_failed = True + # sometimes the operation looks like + # "failed_to_do_something: error message" + if ':' in operation: + operation, err_msg = operation.split(': ', 1) + + while idx < len(operations) and operations[idx] != operation: + click.echo('Skipped') + idx += 1 + if idx == len(operations): + break + click.echo(op_desc[operations[idx]] + '... ', nl=False) + if idx == len(operations): + click.echo('Unrecognized operation: ' + operation) + continue + if oper_failed: + click.echo(click.style('Failed', fg='red')) + if err_msg is not None: + click.echo(' Error message: ' + err_msg) + else: + click.echo(click.style('Done', fg='green')) + idx += 1 + if idx < len(operations): + click.echo(op_desc[operations[idx]] + '... ', nl=False) + + raise Exception('server response ended abruptly') + + +def handle_sync_response(resp: requests.Response): + """ + Exit the program if the request was not successful. + Returns the parsed JSON response. + """ + if resp.status_code != 200: + click.echo('An error occurred:') + click.echo(resp.text.rstrip()) + raise Abort() + return resp.json() diff --git a/ceo/krb_check.py b/ceo/krb_check.py index f9f9ce8..7824da7 100644 --- a/ceo/krb_check.py +++ b/ceo/krb_check.py @@ -9,19 +9,15 @@ def krb_check(): credentials have expired. Returns the principal string 'user@REALM'. """ - try: - creds = gssapi.Credentials(usage='initiate') - except gssapi.raw.misc.GSSError: - kinit() - creds = gssapi.Credentials(usage='initiate') + for _ in range(2): + try: + creds = gssapi.Credentials(usage='initiate') + result = creds.inquire() + return str(result.name) + except (gssapi.raw.misc.GSSError, gssapi.raw.exceptions.ExpiredCredentialsError): + kinit() - try: - result = creds.inquire() - except gssapi.raw.exceptions.ExpiredCredentialsError: - kinit() - result = creds.inquire() - - return str(result.name) + raise Exception('could not acquire GSSAPI credentials') def kinit(): diff --git a/ceo/utils.py b/ceo/utils.py index b84386c..fc84265 100644 --- a/ceo/utils.py +++ b/ceo/utils.py @@ -1,12 +1,8 @@ -import json -import sys -from typing import List, Tuple, Dict +from typing import List, Dict -import click import requests from zope import component -from .operation_strings import descriptions as op_desc from ceo_common.interfaces import IHTTPClient, IConfig @@ -41,94 +37,6 @@ def http_delete(path: str, **kwargs) -> requests.Response: return http_request('DELETE', path, **kwargs) -def handle_stream_response(resp: requests.Response, operations: List[str]) -> List[Dict]: - """ - Print output to the console while operations are being streamed - from the server over HTTP. - Returns the parsed JSON data streamed from the server. - """ - if resp.status_code != 200: - click.echo('An error occurred:') - click.echo(resp.text) - sys.exit(1) - click.echo(op_desc[operations[0]] + '... ', nl=False) - idx = 0 - data = [] - for line in resp.iter_lines(decode_unicode=True, chunk_size=8): - d = json.loads(line) - data.append(d) - if d['status'] == 'aborted': - click.echo(click.style('ABORTED', fg='red')) - click.echo('The transaction was rolled back.') - click.echo('The error was: ' + d['error']) - click.echo('Please check the ceod logs.') - sys.exit(1) - elif d['status'] == 'completed': - click.echo('Transaction successfully completed.') - return data - - operation = d['operation'] - oper_failed = False - err_msg = None - prefix = 'failed_to_' - if operation.startswith(prefix): - operation = operation[len(prefix):] - oper_failed = True - # sometimes the operation looks like - # "failed_to_do_something: error message" - if ':' in operation: - operation, err_msg = operation.split(': ', 1) - - while idx < len(operations) and operations[idx] != operation: - click.echo('Skipped') - idx += 1 - if idx == len(operations): - break - click.echo(op_desc[operations[idx]] + '... ', nl=False) - if idx == len(operations): - click.echo('Unrecognized operation: ' + operation) - continue - if oper_failed: - click.echo(click.style('Failed', fg='red')) - if err_msg is not None: - click.echo(' Error message: ' + err_msg) - else: - click.echo(click.style('Done', fg='green')) - idx += 1 - if idx < len(operations): - click.echo(op_desc[operations[idx]] + '... ', nl=False) - - raise Exception('server response ended abruptly') - - -def handle_sync_response(resp: requests.Response): - """ - Exit the program if the request was not successful. - Returns the parsed JSON response. - """ - if resp.status_code // 100 != 2: - click.echo('An error occurred:') - click.echo(resp.text) - sys.exit(1) - return resp.json() - - -def print_colon_kv(pairs: List[Tuple[str, str]]): - """ - Pretty-print a list of key-value pairs such that the key and value - columns align. - Example: - key1: value1 - key1000: value2 - """ - maxlen = max(len(key) for key, val in pairs) - for key, val in pairs: - click.echo(key + ': ', nl=False) - extra_space = ' ' * (maxlen - len(key)) - click.echo(extra_space, nl=False) - click.echo(val) - - def get_failed_operations(data: List[Dict]) -> List[str]: """ Get a list of the failed operations using the JSON objects @@ -144,6 +52,8 @@ def get_failed_operations(data: List[Dict]) -> List[str]: continue operation = operation[len(prefix):] if ':' in operation: + # sometimes the operation looks like + # "failed_to_do_something: error message" operation = operation[:operation.index(':')] failed.append(operation) return failed diff --git a/ceo_common/model/Term.py b/ceo_common/model/Term.py new file mode 100644 index 0000000..84dc1ad --- /dev/null +++ b/ceo_common/model/Term.py @@ -0,0 +1,67 @@ +import datetime + + +class Term: + """A representation of a term in the CSC LDAP, e.g. 's2021'.""" + + seasons = ['w', 's', 'f'] + + def __init__(self, s_term: str): + assert len(s_term) == 5 and s_term[0] in self.seasons and \ + s_term[1:].isdigit() + self.s_term = s_term + + def __repr__(self): + return self.s_term + + @staticmethod + def current(): + """Get a Term object for the current date.""" + dt = datetime.datetime.now() + c = 'w' + if 5 <= dt.month <= 8: + c = 's' + elif 9 <= dt.month: + c = 'f' + s_term = c + str(dt.year) + return Term(s_term) + + def __add__(self, other): + assert type(other) is int and other >= 0 + c = self.s_term[0] + season_idx = self.seasons.index(c) + year = int(self.s_term[1:]) + year += other // 3 + season_idx += other % 3 + if season_idx >= 3: + year += 1 + season_idx -= 3 + s_term = self.seasons[season_idx] + str(year) + return Term(s_term) + + def __eq__(self, other): + return isinstance(other, Term) and self.s_term == other.s_term + + def __lt__(self, other): + if not isinstance(other, Term): + return NotImplemented + c1, c2 = self.s_term[0], other.s_term[0] + year1, year2 = int(self.s_term[1:]), int(other.s_term[1:]) + return year1 < year2 or ( + year1 == year2 and self.seasons.index(c1) < self.seasons.index(c2) + ) + + def __gt__(self, other): + if not isinstance(other, Term): + return NotImplemented + c1, c2 = self.s_term[0], other.s_term[0] + year1, year2 = int(self.s_term[1:]), int(other.s_term[1:]) + return year1 > year2 or ( + year1 == year2 and self.seasons.index(c1) > self.seasons.index(c2) + ) + + def __ge__(self, other): + return self > other or self == other + + def __le__(self, other): + return self < other or self == other diff --git a/ceo_common/model/__init__.py b/ceo_common/model/__init__.py index 382fcef..14967e6 100644 --- a/ceo_common/model/__init__.py +++ b/ceo_common/model/__init__.py @@ -1,3 +1,4 @@ from .Config import Config from .HTTPClient import HTTPClient from .RemoteMailmanService import RemoteMailmanService +from .Term import Term diff --git a/ceo_common/utils.py b/ceo_common/utils.py deleted file mode 100644 index a7d858b..0000000 --- a/ceo_common/utils.py +++ /dev/null @@ -1,40 +0,0 @@ -import datetime -from typing import List - - -def get_current_term() -> str: - """ - Get the current term as formatted in the CSC LDAP (e.g. 's2021'). - """ - dt = datetime.datetime.now() - c = 'w' - if 5 <= dt.month <= 8: - c = 's' - elif 9 <= dt.month: - c = 'f' - return c + str(dt.year) - - -def add_term(term: str) -> str: - """ - Add one term to the given term and return the string. - Example: add_term('s2021') -> 'f2021' - """ - c = term[0] - s_year = term[1:] - if c == 'w': - return 's' + s_year - elif c == 's': - return 'f' + s_year - year = int(s_year) - return 'w' + str(year + 1) - - -def get_max_term(terms: List[str]) -> str: - """Get the maximum (latest) term.""" - max_year = max(term[1:] for term in terms) - if 'f' + max_year in terms: - return 'f' + max_year - elif 's' + max_year in terms: - return 's' + max_year - return 'w' + max_year diff --git a/tests/ceo/__init__.py b/tests/ceo/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/ceo/cli/__init__.py b/tests/ceo/cli/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/ceo/cli/test_members.py b/tests/ceo/cli/test_members.py new file mode 100644 index 0000000..73e852f --- /dev/null +++ b/tests/ceo/cli/test_members.py @@ -0,0 +1,136 @@ +import os +import re +import shutil + +from click.testing import CliRunner + +from ceo.cli import cli +from ceo_common.model import Term + + +def test_members_get(cli_setup, ldap_user): + runner = CliRunner() + result = runner.invoke(cli, ['members', 'get', ldap_user.uid]) + expected = ( + f"uid: {ldap_user.uid}\n" + f"cn: {ldap_user.cn}\n" + f"program: {ldap_user.program}\n" + f"UID number: {ldap_user.uid_number}\n" + f"GID number: {ldap_user.gid_number}\n" + f"login shell: {ldap_user.login_shell}\n" + f"home directory: {ldap_user.home_directory}\n" + f"is a club: {ldap_user.is_club()}\n" + "forwarding addresses: \n" + f"terms: {','.join(ldap_user.terms)}\n" + ) + assert result.exit_code == 0 + assert result.output == expected + + +def test_members_add(cli_setup): + runner = CliRunner() + result = runner.invoke(cli, [ + 'members', 'add', 'test_1', '--cn', 'Test One', '--program', 'Math', + '--terms', '1', + ], input='y\n') + expected_pat = re.compile(( + "^The following user will be created:\n" + "uid: test_1\n" + "cn: Test One\n" + "program: Math\n" + "member terms: [sfw]\\d{4}\n" + "forwarding address: test_1@uwaterloo.internal\n" + "Do you want to continue\\? \\[y/N\\]: y\n" + "Add user to LDAP... Done\n" + "Add group to LDAP... Done\n" + "Add user to Kerberos... Done\n" + "Create home directory... Done\n" + "Set forwarding addresses... Done\n" + "Send welcome message... Done\n" + "Subscribe to mailing list... Done\n" + "Announce new user to mailing list... Done\n" + "Transaction successfully completed.\n" + "uid: test_1\n" + "cn: Test One\n" + "program: Math\n" + "UID number: \\d{5}\n" + "GID number: \\d{5}\n" + "login shell: /bin/bash\n" + "home directory: [a-z0-9/_-]+/test_1\n" + "is a club: False\n" + "forwarding addresses: test_1@uwaterloo.internal\n" + "terms: [sfw]\\d{4}\n" + "password: \\S+\n$" + ), re.MULTILINE) + assert result.exit_code == 0 + assert expected_pat.match(result.output) is not None + + result = runner.invoke(cli, ['members', 'delete', 'test_1'], input='y\n') + assert result.exit_code == 0 + + +def test_members_modify(cli_setup, ldap_user): + # The homedir needs to exist so the API can write to ~/.forward + os.makedirs(ldap_user.home_directory) + try: + runner = CliRunner() + result = runner.invoke(cli, [ + 'members', 'modify', ldap_user.uid, '--login-shell', '/bin/sh', + '--forwarding-addresses', 'jdoe@test1.internal,jdoe@test2.internal', + ], input='y\n') + expected = ( + "Login shell will be set to: /bin/sh\n" + "~/.forward will be set to: jdoe@test1.internal\n" + " jdoe@test2.internal\n" + "Do you want to continue? [y/N]: y\n" + "Replace login shell... Done\n" + "Replace forwarding addresses... Done\n" + "Transaction successfully completed.\n" + ) + assert result.exit_code == 0 + assert result.output == expected + finally: + shutil.rmtree(ldap_user.home_directory) + + +def test_members_renew(cli_setup, ldap_user, g_admin_ctx): + # set the user's last term to something really old + with g_admin_ctx(), ldap_user.ldap_srv.entry_ctx_for_user(ldap_user) as entry: + entry.term = ['s1999', 'f1999'] + current_term = Term.current() + + runner = CliRunner() + result = runner.invoke(cli, [ + 'members', 'renew', ldap_user.uid, '--terms', '1', + ], input='y\n') + expected = ( + f"The following member terms will be added: {current_term}\n" + "Do you want to continue? [y/N]: y\n" + "Done.\n" + ) + assert result.exit_code == 0 + assert result.output == expected + + runner = CliRunner() + result = runner.invoke(cli, [ + 'members', 'renew', ldap_user.uid, '--terms', '2', + ], input='y\n') + expected = ( + f"The following member terms will be added: {current_term+1},{current_term+2}\n" + "Do you want to continue? [y/N]: y\n" + "Done.\n" + ) + assert result.exit_code == 0 + assert result.output == expected + + +def test_members_pwreset(cli_setup, ldap_user, krb_user): + runner = CliRunner() + result = runner.invoke( + cli, ['members', 'pwreset', ldap_user.uid], input='y\n') + expected_pat = re.compile(( + f"^Are you sure you want to reset {ldap_user.uid}'s password\\? \\[y/N\\]: y\n" + "New password: \\S+\n$" + ), re.MULTILINE) + assert result.exit_code == 0 + assert expected_pat.match(result.output) is not None diff --git a/tests/ceo_common/model/test_remote_mailman.py b/tests/ceo_common/model/test_remote_mailman.py index 44b61fb..8518e5b 100644 --- a/tests/ceo_common/model/test_remote_mailman.py +++ b/tests/ceo_common/model/test_remote_mailman.py @@ -1,42 +1,12 @@ -from multiprocessing import Process -import socket -import sys -import time - -import requests - from ceo_common.model import RemoteMailmanService -def test_remote_mailman(cfg, http_client, app, mock_mailman_server, g_syscom): - port = cfg.get('ceod_port') - hostname = socket.gethostname() - - def server_start(): - sys.stdout = open('/dev/null', 'w') - sys.stderr = sys.stdout - app.run(debug=False, host='0.0.0.0', port=port) - - proc = Process(target=server_start) - proc.start() - - for _ in range(5): - try: - http_client.get(hostname, '/ping') - except requests.exceptions.ConnectionError: - time.sleep(1) - continue - break - - try: - mailman_srv = RemoteMailmanService() - assert mock_mailman_server.subscriptions['csc-general'] == [] - # RemoteMailmanService -> app -> MailmanService -> MockMailmanServer - address = 'test_1@csclub.internal' - mailman_srv.subscribe(address, 'csc-general') - assert mock_mailman_server.subscriptions['csc-general'] == [address] - mailman_srv.unsubscribe(address, 'csc-general') - assert mock_mailman_server.subscriptions['csc-general'] == [] - finally: - proc.terminate() - proc.join() +def test_remote_mailman(app_process, mock_mailman_server, g_syscom): + mailman_srv = RemoteMailmanService() + assert mock_mailman_server.subscriptions['csc-general'] == [] + # RemoteMailmanService -> app -> MailmanService -> MockMailmanServer + address = 'test_1@csclub.internal' + mailman_srv.subscribe(address, 'csc-general') + assert mock_mailman_server.subscriptions['csc-general'] == [address] + mailman_srv.unsubscribe(address, 'csc-general') + assert mock_mailman_server.subscriptions['csc-general'] == [] diff --git a/tests/ceod_test_local.ini b/tests/ceod_test_local.ini index 04fdbd9..6a8f7bd 100644 --- a/tests/ceod_test_local.ini +++ b/tests/ceod_test_local.ini @@ -1,5 +1,7 @@ [DEFAULT] base_domain = csclub.internal +# merge ceod.ini and ceo.ini values together to make testing easier +uw_domain = uwaterloo.internal [ceod] admin_host = phosphoric-acid diff --git a/tests/conftest.py b/tests/conftest.py index 3388fe6..d51f230 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,20 +1,23 @@ import contextlib import grp import importlib.resources +from multiprocessing import Process import os import pwd import shutil import subprocess -from subprocess import DEVNULL -import tempfile +import sys +import time from unittest.mock import patch, Mock import flask import ldap3 import pytest +import requests import socket from zope import component +from .utils import krb5ccname_ctx from ceo_common.interfaces import IConfig, IKerberosService, ILDAPService, \ IFileService, IMailmanService, IHTTPClient, IUWLDAPService, IMailService from ceo_common.model import Config, HTTPClient @@ -25,6 +28,7 @@ import ceod.utils as utils from .MockSMTPServer import MockSMTPServer from .MockMailmanServer import MockMailmanServer from .conftest_ceod_api import client # noqa: F401 +from .conftest_ceo import cli_setup # noqa: F401 @pytest.fixture(scope='session', autouse=True) @@ -47,6 +51,18 @@ def cfg(_drone_hostname_mock): return _cfg +@pytest.fixture(scope='session', autouse=True) +def _delete_ccaches(): + # I've noticed when pytest finishes, the temporary files + # created by tempfile.NamedTemporaryFile() aren't destroyed. + # So, we clean them up here. + from .utils import _ccaches + yield + # forcefully decrement the reference counts, which will trigger + # the destructors + _ccaches.clear() + + def delete_test_princs(krb_srv): proc = subprocess.run([ 'kadmin', '-k', '-p', krb_srv.admin_principal, 'listprincs', 'test_*', @@ -86,20 +102,8 @@ def delete_subtree(conn: ldap3.Connection, base_dn: str): pass -@pytest.fixture(scope='session') -def ceod_admin_creds(cfg, krb_srv): - """ - Acquire credentials for ceod/admin and store them - in the default ccache. - """ - subprocess.run( - ['kinit', '-k', cfg.get('ldap_admin_principal')], - check=True, - ) - - @pytest.fixture -def g_admin_ctx(cfg, ceod_admin_creds, app): +def g_admin_ctx(app): """ Store the principal for ceod/admin in flask.g. This context manager should be used any time LDAP is modified via the @@ -109,62 +113,45 @@ def g_admin_ctx(cfg, ceod_admin_creds, app): """ @contextlib.contextmanager def wrapper(): - admin_principal = cfg.get('ldap_admin_principal') - with app.app_context(): + with krb5ccname_ctx('ceod/admin'), app.app_context(): try: - flask.g.sasl_user = admin_principal + flask.g.sasl_user = 'ceod/admin' yield finally: flask.g.pop('sasl_user') return wrapper -@pytest.fixture(scope='session') -def syscom_creds(): - """ - Acquire credentials for a syscom member and store them in a ccache. - Yields the name of the ccache file. - """ - with tempfile.NamedTemporaryFile() as f: - subprocess.run( - ['kinit', '-c', f.name, 'ctdalek'], - check=True, text=True, input='krb5', stdout=DEVNULL, - ) - yield f.name - - @pytest.fixture -def g_syscom(syscom_creds, app): +def g_syscom(app): """ Store the principal for the syscom member in flask.g, and point KRB5CCNAME to the file where the TGT is stored. Use this fixture if you need syscom credentials for an HTTP request to a different process. """ - filename = syscom_creds - with app.app_context(): - old_krb5ccname = os.environ['KRB5CCNAME'] - os.environ['KRB5CCNAME'] = 'FILE:' + filename + with krb5ccname_ctx('ctdalek'), app.app_context(): try: flask.g.sasl_user = 'ctdalek' - yield filename + yield finally: - os.environ['KRB5CCNAME'] = old_krb5ccname flask.g.pop('sasl_user') @pytest.fixture(scope='session') -def ldap_conn(cfg, ceod_admin_creds) -> ldap3.Connection: +def ldap_conn(cfg) -> ldap3.Connection: # Assume that the same server URL is being used for the CSC # and UWLDAP during the tests. cfg = component.getUtility(IConfig) server_url = cfg.get('ldap_server_url') # sanity check assert server_url == cfg.get('uwldap_server_url') - return ldap3.Connection( - server_url, auto_bind=True, raise_exceptions=True, - authentication=ldap3.SASL, sasl_mechanism=ldap3.KERBEROS, - user=cfg.get('ldap_admin_principal')) + with krb5ccname_ctx('ceod/admin'): + conn = ldap3.Connection( + server_url, auto_bind=True, raise_exceptions=True, + authentication=ldap3.SASL, sasl_mechanism=ldap3.KERBEROS, + user='ceod/admin') + return conn @pytest.fixture(scope='session') @@ -375,3 +362,32 @@ def uwldap_user(cfg, uwldap_srv, ldap_conn): ) yield user conn.delete(dn) + + +@pytest.fixture(scope='module') +def app_process(cfg, app, http_client): + port = cfg.get('ceod_port') + hostname = socket.gethostname() + + def server_start(): + sys.stdout = open('/dev/null', 'w') + sys.stderr = sys.stdout + app.run(debug=False, host='0.0.0.0', port=port) + + proc = Process(target=server_start) + proc.start() + + try: + with krb5ccname_ctx('ctdalek'): + for i in range(5): + try: + http_client.get(hostname, '/ping') + except requests.exceptions.ConnectionError: + time.sleep(1) + continue + break + assert i != 5, 'Timed out' + yield + finally: + proc.terminate() + proc.join() diff --git a/tests/conftest_ceo.py b/tests/conftest_ceo.py new file mode 100644 index 0000000..be1f63d --- /dev/null +++ b/tests/conftest_ceo.py @@ -0,0 +1,18 @@ +import os + +import pytest + +from .utils import krb5ccname_ctx + + +@pytest.fixture(scope='module') +def cli_setup(app_process): + # This tells the CLI entrypoint not to register additional zope services. + os.environ['PYTEST'] = '1' + + # Running the client and the server in the same process would be very + # messy because they would be sharing the same environment variables, + # Kerberos cache, and registered utilities (via zope). So we're just + # going to start the app in a child process intead. + with krb5ccname_ctx('ctdalek'): + yield diff --git a/tests/conftest_ceod_api.py b/tests/conftest_ceod_api.py index 19f16ba..1962d37 100644 --- a/tests/conftest_ceod_api.py +++ b/tests/conftest_ceod_api.py @@ -1,10 +1,6 @@ from base64 import b64encode -import contextlib -import os import json import socket -import subprocess -import tempfile from flask import g from flask.testing import FlaskClient @@ -14,6 +10,7 @@ from requests import Request from requests_gssapi import HTTPSPNEGOAuth from ceo_common.krb5.utils import get_fwd_tgt +from .utils import krb5ccname_ctx __all__ = ['client'] @@ -21,40 +18,23 @@ __all__ = ['client'] @pytest.fixture(scope='session') def client(app): app_client = app.test_client() - with tempfile.TemporaryDirectory() as cache_dir: - yield CeodTestClient(app_client, cache_dir) + yield CeodTestClient(app_client) class CeodTestClient: - def __init__(self, app_client: FlaskClient, cache_dir: str): + def __init__(self, app_client: FlaskClient): self.client = app_client self.syscom_principal = 'ctdalek' # this is only used for the HTTPSNEGOAuth self.base_url = f'http://{socket.getfqdn()}' - # for each principal for which we acquired a TGT, map their - # username to a file (ccache) storing their TGT - self.principal_ccaches = {} - # this is where we'll store the credentials for each principal - self.cache_dir = cache_dir # for SPNEGO self.target_name = gssapi.Name('ceod/' + socket.getfqdn()) - @contextlib.contextmanager - def krb5ccname_env(self, principal): - """Temporarily change KRB5CCNAME to the ccache of the principal.""" - old_krb5ccname = os.environ['KRB5CCNAME'] - os.environ['KRB5CCNAME'] = self.principal_ccaches[principal] - try: - yield - finally: - os.environ['KRB5CCNAME'] = old_krb5ccname - def get_auth(self, principal): """Acquire a HTTPSPNEGOAuth instance for the principal.""" name = gssapi.Name(principal) # the 'store' arg doesn't seem to work for DIR ccaches - with self.krb5ccname_env(principal): - creds = gssapi.Credentials(name=name, usage='initiate') + creds = gssapi.Credentials(name=name, usage='initiate') auth = HTTPSPNEGOAuth( opportunistic_auth=True, target_name=self.target_name, @@ -62,32 +42,17 @@ class CeodTestClient: ) return auth - def kinit(self, principal): - """Acquire an initial TGT for the principal.""" - # For some reason, kinit with the '-c' option deletes the other - # credentials in the cache collection, so we need to override the - # env variable - subprocess.run( - ['kinit', principal], - text=True, input='krb5', check=True, stdout=subprocess.DEVNULL, - env={'KRB5CCNAME': self.principal_ccaches[principal]}) - def get_headers(self, principal: str, need_cred: bool): - if principal not in self.principal_ccaches: - _, filename = tempfile.mkstemp(dir=self.cache_dir) - self.principal_ccaches[principal] = filename - self.kinit(principal) - # Get the Authorization header (SPNEGO). - # The method doesn't matter here because we just need to extract - # the header using req.prepare(). - req = Request('GET', self.base_url, auth=self.get_auth(principal)) - headers = list(req.prepare().headers.items()) - if need_cred: - # Get the X-KRB5-CRED header (forwarded TGT). - cred = b64encode(get_fwd_tgt( - 'ceod/' + socket.getfqdn(), self.principal_ccaches[principal] - )).decode() - headers.append(('X-KRB5-CRED', cred)) + with krb5ccname_ctx(principal): + # Get the Authorization header (SPNEGO). + # The method doesn't matter here because we just need to extract + # the header using req.prepare(). + req = Request('GET', self.base_url, auth=self.get_auth(principal)) + headers = list(req.prepare().headers.items()) + if need_cred: + # Get the X-KRB5-CRED header (forwarded TGT). + cred = b64encode(get_fwd_tgt('ceod/' + socket.getfqdn())).decode() + headers.append(('X-KRB5-CRED', cred)) return headers def request(self, method: str, path: str, principal: str, need_cred: bool, **kwargs): diff --git a/tests/utils.py b/tests/utils.py new file mode 100644 index 0000000..324aedd --- /dev/null +++ b/tests/utils.py @@ -0,0 +1,34 @@ +import contextlib +import os +import subprocess +from subprocess import DEVNULL +import tempfile + + +# map principals to files storing credentials +_ccaches = {} + + +@contextlib.contextmanager +def krb5ccname_ctx(principal: str): + """ + Temporarily set KRB5CCNAME to a ccache storing credentials + for the specified user. + """ + old_krb5ccname = os.environ['KRB5CCNAME'] + try: + if principal not in _ccaches: + f = tempfile.NamedTemporaryFile() + os.environ['KRB5CCNAME'] = 'FILE:' + f.name + args = ['kinit', principal] + if principal == 'ceod/admin': + args = ['kinit', '-k', principal] + subprocess.run( + args, stdout=DEVNULL, text=True, input='krb5', + check=True) + _ccaches[principal] = f + else: + os.environ['KRB5CCNAME'] = 'FILE:' + _ccaches[principal].name + yield + finally: + os.environ['KRB5CCNAME'] = old_krb5ccname From e851c77e74eea6d1d56353d5e1cb20d0ddb01a42 Mon Sep 17 00:00:00 2001 From: Max Erenberg Date: Mon, 23 Aug 2021 23:36:49 +0000 Subject: [PATCH 11/14] include password in welcome email --- ceo_common/interfaces/IMailService.py | 7 +++++-- ceod/model/MailService.py | 4 ++-- ceod/model/templates/welcome_message.j2 | 9 +++++++-- ceod/transactions/members/AddMemberTransaction.py | 2 +- tests/ceod/model/test_mail.py | 2 +- 5 files changed, 16 insertions(+), 8 deletions(-) diff --git a/ceo_common/interfaces/IMailService.py b/ceo_common/interfaces/IMailService.py index baccd1f..b2149d5 100644 --- a/ceo_common/interfaces/IMailService.py +++ b/ceo_common/interfaces/IMailService.py @@ -11,8 +11,11 @@ class IMailService(Interface): def send(_from: str, to: str, headers: Dict[str, str], content: str): """Send a message with the given headers and content.""" - def send_welcome_message_to(user: IUser): - """Send a welcome message to the new member.""" + def send_welcome_message_to(user: IUser, password: str): + """ + Send a welcome message to the new member, including their temporary + password. + """ def announce_new_user(user: IUser, operations: List[str]): """ diff --git a/ceod/model/MailService.py b/ceod/model/MailService.py index b764809..be61e2f 100644 --- a/ceod/model/MailService.py +++ b/ceod/model/MailService.py @@ -51,11 +51,11 @@ class MailService: client.send_message(msg) client.quit() - def send_welcome_message_to(self, user: IUser): + def send_welcome_message_to(self, user: IUser, password: str): template = self.jinja_env.get_template('welcome_message.j2') # TODO: store surname and givenName in LDAP first_name = user.cn.split(' ', 1)[0] - body = template.render(name=first_name, user=user.uid) + body = template.render(name=first_name, user=user.uid, password=password) self.send( f'Computer Science Club ', f'{user.cn} <{user.uid}@{self.base_domain}>', diff --git a/ceod/model/templates/welcome_message.j2 b/ceod/model/templates/welcome_message.j2 index b396494..c17aaab 100644 --- a/ceod/model/templates/welcome_message.j2 +++ b/ceod/model/templates/welcome_message.j2 @@ -2,7 +2,6 @@ Hello {{ name }}: Welcome to the Computer Science Club! We are pleased that you have chosen to join us. We welcome you to come out to our events, or just hang out in our office (MC 3036/3037). You have been automatically subscribed to our mailing list, csc-general, which we use to keep you informed of upcoming events. - Typical events include: * Talks: these mostly technical talks are given by members, faculty and distinguished guests. Past topics include randomized algorithms, video encoding, computer security and adaptable user interfaces. People of all skill levels are welcome, and snacks are often served after talks. * Code parties: late-night hackathons perfect for contributing to open source, working on personal projects, or making progress on a CS assignment you've been putting off. Refreshments provided, and both music and geek classic movies have been played in the past. @@ -19,7 +18,13 @@ You can hear about upcoming events in a number of ways: Even when events aren't being held, you are welcome to hang out in the club office (MC 3036/3037, across the hall from MathSoc). It's often open late into the evening, and sells pop and snacks at reasonable prices. If you're so inclined, you are also welcome in our IRC channel, #csc on FreeNode. -You now have a CSC user account with username "{{ user }}" and the password you supplied when you joined. You can use this account to log into almost any CSC system, including our office terminals and servers. A complete list is available at: +You now have a CSC user account with username "{{ user }}". Your temporary password is: + +{{ password }} + +You will be prompted to change your password when you login to any CSC machine for the first time. + +You can use this account to log into almost any CSC system, including our office terminals and servers. A complete list is available at: http://wiki.csclub.uwaterloo.ca/Machine_List diff --git a/ceod/transactions/members/AddMemberTransaction.py b/ceod/transactions/members/AddMemberTransaction.py index 38cf539..f472807 100644 --- a/ceod/transactions/members/AddMemberTransaction.py +++ b/ceod/transactions/members/AddMemberTransaction.py @@ -84,7 +84,7 @@ class AddMemberTransaction(AbstractTransaction): # user has already seen the email try: - self.mail_srv.send_welcome_message_to(user) + self.mail_srv.send_welcome_message_to(user, password) yield 'send_welcome_message' except Exception as err: logger.warning('send_welcome_message failed:\n' + traceback.format_exc()) diff --git a/tests/ceod/model/test_mail.py b/tests/ceod/model/test_mail.py index 75ea840..5ce0d0f 100644 --- a/tests/ceod/model/test_mail.py +++ b/tests/ceod/model/test_mail.py @@ -1,7 +1,7 @@ def test_welcome_message(cfg, mock_mail_server, mail_srv, simple_user): base_domain = cfg.get('base_domain') mock_mail_server.messages.clear() - mail_srv.send_welcome_message_to(simple_user) + mail_srv.send_welcome_message_to(simple_user, 'password') msg = mock_mail_server.messages[0] assert msg['from'] == f'exec@{base_domain}' assert msg['to'] == f'{simple_user.uid}@{base_domain}' From 45192d75bfdb75b74adc426a5374f00519f695fa Mon Sep 17 00:00:00 2001 From: Max Erenberg Date: Mon, 23 Aug 2021 23:40:52 +0000 Subject: [PATCH 12/14] update social media links in welcome message --- ceod/model/templates/welcome_message.j2 | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ceod/model/templates/welcome_message.j2 b/ceod/model/templates/welcome_message.j2 index c17aaab..27b7273 100644 --- a/ceod/model/templates/welcome_message.j2 +++ b/ceod/model/templates/welcome_message.j2 @@ -10,12 +10,13 @@ Typical events include: You can hear about upcoming events in a number of ways: * Check our website from time to time: http://csclub.uwaterloo.ca/ * Subscribe to our events calendar feed: http://csclub.uwaterloo.ca/events.ics -* Like the CSC on Facebook: https://www.facebook.com/watcsc +* Like the CSC on Facebook: https://www.facebook.com/uw.computerscienceclub +* Join the CSC Discord server: https://discord.gg/uwcsclub * Read your email: announcements are sent via the csc-general mailing list * Keep an eye out in the MC: posters for upcoming events appear in stairwells and hallways -Even when events aren't being held, you are welcome to hang out in the club office (MC 3036/3037, across the hall from MathSoc). It's often open late into the evening, and sells pop and snacks at reasonable prices. If you're so inclined, you are also welcome in our IRC channel, #csc on FreeNode. +Even when events aren't being held, you are welcome to hang out in the club office (MC 3036/3037, across the hall from MathSoc). It's often open late into the evening, and sells pop and snacks at reasonable prices. If you're so inclined, you are also welcome in our IRC channel, #csc on libera.chat. You now have a CSC user account with username "{{ user }}". Your temporary password is: From 831ebf17aa915818540259afe6a12253d4132568 Mon Sep 17 00:00:00 2001 From: Max Erenberg Date: Tue, 24 Aug 2021 05:48:55 +0000 Subject: [PATCH 13/14] add groups CLI --- ceo/cli/entrypoint.py | 2 + ceo/cli/groups.py | 148 +++++++++++++++++++++++++++++++++ ceo/cli/members.py | 9 +- ceo/cli/utils.py | 16 +++- ceo/operation_strings.py | 8 ++ tests/ceo/cli/test_groups.py | 154 +++++++++++++++++++++++++++++++++++ 6 files changed, 330 insertions(+), 7 deletions(-) create mode 100644 ceo/cli/groups.py create mode 100644 tests/ceo/cli/test_groups.py diff --git a/ceo/cli/entrypoint.py b/ceo/cli/entrypoint.py index 9389c56..2b90c52 100644 --- a/ceo/cli/entrypoint.py +++ b/ceo/cli/entrypoint.py @@ -7,6 +7,7 @@ from zope import component from ..krb_check import krb_check from .members import members +from .groups import groups from ceo_common.interfaces import IConfig, IHTTPClient from ceo_common.model import Config, HTTPClient @@ -26,6 +27,7 @@ def cli(ctx): cli.add_command(members) +cli.add_command(groups) def register_services(): diff --git a/ceo/cli/groups.py b/ceo/cli/groups.py new file mode 100644 index 0000000..6973683 --- /dev/null +++ b/ceo/cli/groups.py @@ -0,0 +1,148 @@ +from typing import Dict + +import click +from zope import component + +from ..utils import http_post, http_get, http_delete +from .utils import handle_stream_response, handle_sync_response, print_colon_kv, \ + check_if_in_development +from ceo_common.interfaces import IConfig +from ceod.transactions.groups import ( + AddGroupTransaction, + AddMemberToGroupTransaction, + RemoveMemberFromGroupTransaction, + DeleteGroupTransaction, +) + + +@click.group() +def groups(): + pass + + +@groups.command(short_help='Add a new group') +@click.argument('group_name') +@click.option('-d', '--description', help='Group description', prompt=True) +def add(group_name, description): + click.echo('The following group will be created:') + lines = [ + ('cn', group_name), + ('description', description), + ] + print_colon_kv(lines) + + click.confirm('Do you want to continue?', abort=True) + + body = { + 'cn': group_name, + 'description': description, + } + operations = AddGroupTransaction.operations + resp = http_post('/api/groups', json=body) + data = handle_stream_response(resp, operations) + result = data[-1]['result'] + print_group_lines(result) + + +def print_group_lines(result: Dict): + """Pretty-print a group JSON response.""" + lines = [ + ('cn', result['cn']), + ('description', result.get('description', 'Unknown')), + ('gid_number', str(result['gid_number'])), + ] + for i, member in enumerate(result['members']): + if i == 0: + prefix = 'members' + else: + prefix = '' + lines.append((prefix, member['cn'] + ' (' + member['uid'] + ')')) + print_colon_kv(lines) + + +@groups.command(short_help='Get info about a group') +@click.argument('group_name') +def get(group_name): + resp = http_get('/api/groups/' + group_name) + result = handle_sync_response(resp) + print_group_lines(result) + + +@groups.command(short_help='Add a member to a group') +@click.argument('group_name') +@click.argument('username') +@click.option('--no-subscribe', is_flag=True, default=False, + help='Do not subscribe the member to any auxiliary mailing lists.') +def addmember(group_name, username, no_subscribe): + click.confirm(f'Are you sure you want to add {username} to {group_name}?', + abort=True) + base_domain = component.getUtility(IConfig).get('base_domain') + url = f'/api/groups/{group_name}/members/{username}' + operations = AddMemberToGroupTransaction.operations + + if no_subscribe: + url += '?subscribe_to_lists=false' + operations.remove('subscribe_user_to_auxiliary_mailing_lists') + resp = http_post(url) + data = handle_stream_response(resp, operations) + result = data[-1]['result'] + lines = [] + for i, group in enumerate(result['added_to_groups']): + if i == 0: + prefix = 'Added to groups' + else: + prefix = '' + lines.append((prefix, group)) + for i, mailing_list in enumerate(result.get('subscribed_to_lists', [])): + if i == 0: + prefix = 'Subscribed to lists' + else: + prefix = '' + if '@' not in mailing_list: + mailing_list += '@' + base_domain + lines.append((prefix, mailing_list)) + print_colon_kv(lines) + + +@groups.command(short_help='Remove a member from a group') +@click.argument('group_name') +@click.argument('username') +@click.option('--no-unsubscribe', is_flag=True, default=False, + help='Do not unsubscribe the member from any auxiliary mailing lists.') +def removemember(group_name, username, no_unsubscribe): + click.confirm(f'Are you sure you want to remove {username} from {group_name}?', + abort=True) + base_domain = component.getUtility(IConfig).get('base_domain') + url = f'/api/groups/{group_name}/members/{username}' + operations = RemoveMemberFromGroupTransaction.operations + if no_unsubscribe: + url += '?unsubscribe_from_lists=false' + operations.remove('unsubscribe_user_from_auxiliary_mailing_lists') + resp = http_delete(url) + data = handle_stream_response(resp, operations) + result = data[-1]['result'] + lines = [] + for i, group in enumerate(result['removed_from_groups']): + if i == 0: + prefix = 'Removed from groups' + else: + prefix = '' + lines.append((prefix, group)) + for i, mailing_list in enumerate(result.get('unsubscribed_from_lists', [])): + if i == 0: + prefix = 'Unsubscribed from lists' + else: + prefix = '' + if '@' not in mailing_list: + mailing_list += '@' + base_domain + lines.append((prefix, mailing_list)) + print_colon_kv(lines) + + +@groups.command(short_help='Delete a group') +@click.argument('group_name') +def delete(group_name): + check_if_in_development() + click.confirm(f"Are you sure you want to delete {group_name}?", abort=True) + resp = http_delete(f'/api/groups/{group_name}') + handle_stream_response(resp, DeleteGroupTransaction.operations) diff --git a/ceo/cli/members.py b/ceo/cli/members.py index 67cd049..37a82be 100644 --- a/ceo/cli/members.py +++ b/ceo/cli/members.py @@ -1,4 +1,3 @@ -import socket import sys from typing import Dict @@ -6,7 +5,8 @@ import click from zope import component from ..utils import http_post, http_get, http_patch, http_delete, get_failed_operations -from .utils import handle_stream_response, handle_sync_response, print_colon_kv +from .utils import handle_stream_response, handle_sync_response, print_colon_kv, \ + check_if_in_development from ceo_common.interfaces import IConfig from ceo_common.model import Term from ceod.transactions.members import ( @@ -209,10 +209,7 @@ def pwreset(username): @members.command(short_help="Delete a user") @click.argument('username') def delete(username): - # a hack to determine if we're in the dev environment - if not socket.getfqdn().endswith('.csclub.internal'): - click.echo('This command may only be called during development.') - sys.exit(1) + check_if_in_development() click.confirm(f"Are you sure you want to delete {username}?", abort=True) resp = http_delete(f'/api/members/{username}') handle_stream_response(resp, DeleteMemberTransaction.operations) diff --git a/ceo/cli/utils.py b/ceo/cli/utils.py index cc25e84..98aa23f 100644 --- a/ceo/cli/utils.py +++ b/ceo/cli/utils.py @@ -1,4 +1,5 @@ import json +import socket import sys from typing import List, Tuple, Dict @@ -29,7 +30,11 @@ def print_colon_kv(pairs: List[Tuple[str, str]]): """ maxlen = max(len(key) for key, val in pairs) for key, val in pairs: - click.echo(key + ': ', nl=False) + if key != '': + click.echo(key + ': ', nl=False) + else: + # assume this is a continuation from the previous line + click.echo(' ', nl=False) extra_space = ' ' * (maxlen - len(key)) click.echo(extra_space, nl=False) click.echo(val) @@ -58,6 +63,8 @@ def handle_stream_response(resp: requests.Response, operations: List[str]) -> Li click.echo('Please check the ceod logs.') sys.exit(1) elif d['status'] == 'completed': + if idx < len(operations): + click.echo('Skipped') click.echo('Transaction successfully completed.') return data @@ -105,3 +112,10 @@ def handle_sync_response(resp: requests.Response): click.echo(resp.text.rstrip()) raise Abort() return resp.json() + + +def check_if_in_development() -> bool: + """Aborts if we are not currently in the dev environment.""" + if not socket.getfqdn().endswith('.csclub.internal'): + click.echo('This command may only be called during development.') + raise Abort() diff --git a/ceo/operation_strings.py b/ceo/operation_strings.py index 48e7d30..8ff44f7 100644 --- a/ceo/operation_strings.py +++ b/ceo/operation_strings.py @@ -16,4 +16,12 @@ descriptions = { 'remove_user_from_kerberos': 'Remove user from Kerberos', 'delete_home_dir': 'Delete home directory', 'unsubscribe_from_mailing_list': 'Unsubscribe from mailing list', + 'add_sudo_role': 'Add sudo role to LDAP', + 'add_user_to_group': 'Add user to group', + 'add_user_to_auxiliary_groups': 'Add user to auxiliary groups', + 'subscribe_user_to_auxiliary_mailing_lists': 'Subscribe user to auxiliary mailing lists', + 'remove_user_from_group': 'Remove user from group', + 'remove_user_from_auxiliary_groups': 'Remove user from auxiliary groups', + 'unsubscribe_user_from_auxiliary_mailing_lists': 'Unsubscribe user from auxiliary mailing lists', + 'remove_sudo_role': 'Remove sudo role from LDAP', } diff --git a/tests/ceo/cli/test_groups.py b/tests/ceo/cli/test_groups.py new file mode 100644 index 0000000..c81802e --- /dev/null +++ b/tests/ceo/cli/test_groups.py @@ -0,0 +1,154 @@ +import re + +from click.testing import CliRunner + +from ceo.cli import cli + + +def test_groups(cli_setup, ldap_user): + runner = CliRunner() + result = runner.invoke(cli, [ + 'groups', 'add', 'test_group_1', '-d', 'Test Group One', + ], input='y\n') + expected_pat = re.compile(( + "^The following group will be created:\n" + "cn: test_group_1\n" + "description: Test Group One\n" + "Do you want to continue\\? \\[y/N\\]: y\n" + "Add user to LDAP... Done\n" + "Add group to LDAP... Done\n" + "Add sudo role to LDAP... Done\n" + "Create home directory... Done\n" + "Transaction successfully completed.\n" + "cn: test_group_1\n" + "description: Test Group One\n" + "gid_number: \\d{5}\n$" + ), re.MULTILINE) + assert result.exit_code == 0 + assert expected_pat.match(result.output) is not None + + runner = CliRunner() + result = runner.invoke(cli, ['groups', 'get', 'test_group_1']) + expected_pat = re.compile(( + "^cn: test_group_1\n" + "description: Test Group One\n" + "gid_number: \\d{5}\n$" + ), re.MULTILINE) + assert result.exit_code == 0 + assert expected_pat.match(result.output) is not None + + runner = CliRunner() + result = runner.invoke(cli, [ + 'groups', 'addmember', 'test_group_1', ldap_user.uid, + ], input='y\n') + expected = ( + f"Are you sure you want to add {ldap_user.uid} to test_group_1? [y/N]: y\n" + "Add user to group... Done\n" + "Add user to auxiliary groups... Skipped\n" + "Transaction successfully completed.\n" + "Added to groups: test_group_1\n" + ) + assert result.exit_code == 0 + assert result.output == expected + + runner = CliRunner() + result = runner.invoke(cli, ['groups', 'get', 'test_group_1']) + expected_pat = re.compile(f"members:\\s+{ldap_user.cn} \\({ldap_user.uid}\\)") + assert result.exit_code == 0 + assert expected_pat.search(result.output) is not None + + runner = CliRunner() + result = runner.invoke(cli, [ + 'groups', 'removemember', 'test_group_1', ldap_user.uid, + ], input='y\n') + expected = ( + f"Are you sure you want to remove {ldap_user.uid} from test_group_1? [y/N]: y\n" + "Remove user from group... Done\n" + "Remove user from auxiliary groups... Skipped\n" + "Transaction successfully completed.\n" + "Removed from groups: test_group_1\n" + ) + assert result.exit_code == 0 + assert result.output == expected + + runner = CliRunner() + result = runner.invoke(cli, ['groups', 'delete', 'test_group_1'], input='y\n') + assert result.exit_code == 0 + + +def create_group(group_name, desc): + runner = CliRunner() + result = runner.invoke(cli, [ + 'groups', 'add', group_name, '-d', desc, + ], input='y\n') + assert result.exit_code == 0 + + +def delete_group(group_name): + runner = CliRunner() + result = runner.invoke(cli, ['groups', 'delete', group_name], input='y\n') + assert result.exit_code == 0 + + +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('staff', 'Staff') + + runner = CliRunner() + result = runner.invoke(cli, [ + 'groups', 'addmember', 'syscom', ldap_user.uid, + ], input='y\n') + expected = ( + f"Are you sure you want to add {ldap_user.uid} to syscom? [y/N]: y\n" + "Add user to group... Done\n" + "Add user to auxiliary groups... Done\n" + "Subscribe user to auxiliary mailing lists... Done\n" + "Transaction successfully completed.\n" + "Added to groups: syscom\n" + " office\n" + " staff\n" + "Subscribed to lists: syscom@csclub.internal\n" + " syscom-alerts@csclub.internal\n" + ) + assert result.exit_code == 0 + assert result.output == expected + + runner = CliRunner() + result = runner.invoke(cli, [ + 'groups', 'removemember', 'syscom', ldap_user.uid, + ], input='y\n') + expected = ( + f"Are you sure you want to remove {ldap_user.uid} from syscom? [y/N]: y\n" + "Remove user from group... Done\n" + "Remove user from auxiliary groups... Done\n" + "Unsubscribe user from auxiliary mailing lists... Done\n" + "Transaction successfully completed.\n" + "Removed from groups: syscom\n" + " office\n" + " staff\n" + "Unsubscribed from lists: syscom@csclub.internal\n" + " syscom-alerts@csclub.internal\n" + ) + assert result.exit_code == 0 + assert result.output == expected + + runner = CliRunner() + result = runner.invoke(cli, [ + 'groups', 'addmember', 'syscom', ldap_user.uid, '--no-subscribe', + ], input='y\n') + assert result.exit_code == 0 + assert 'Subscribed to lists' not in result.output + + runner = CliRunner() + result = runner.invoke(cli, [ + 'groups', 'removemember', 'syscom', ldap_user.uid, '--no-unsubscribe', + ], input='y\n') + assert result.exit_code == 0 + assert 'Unsubscribed from lists' not in result.output + + delete_group('syscom') + delete_group('office') + delete_group('staff') From 51737585bd00098194bc3dfab8e6f308708fe5db Mon Sep 17 00:00:00 2001 From: Max Erenberg Date: Tue, 24 Aug 2021 19:37:05 +0000 Subject: [PATCH 14/14] add updateprograms CLI --- ceo/cli/entrypoint.py | 2 ++ ceo/cli/groups.py | 2 +- ceo/cli/members.py | 7 +++-- ceo/cli/updateprograms.py | 35 +++++++++++++++++++++++ ceod/api/members.py | 10 ++++--- tests/ceo/cli/test_members.py | 19 ++++++------- tests/ceo/cli/test_updatemembers.py | 43 +++++++++++++++++++++++++++++ 7 files changed, 101 insertions(+), 17 deletions(-) create mode 100644 ceo/cli/updateprograms.py create mode 100644 tests/ceo/cli/test_updatemembers.py diff --git a/ceo/cli/entrypoint.py b/ceo/cli/entrypoint.py index 2b90c52..04f5306 100644 --- a/ceo/cli/entrypoint.py +++ b/ceo/cli/entrypoint.py @@ -8,6 +8,7 @@ from zope import component from ..krb_check import krb_check from .members import members from .groups import groups +from .updateprograms import updateprograms from ceo_common.interfaces import IConfig, IHTTPClient from ceo_common.model import Config, HTTPClient @@ -28,6 +29,7 @@ def cli(ctx): cli.add_command(members) cli.add_command(groups) +cli.add_command(updateprograms) def register_services(): diff --git a/ceo/cli/groups.py b/ceo/cli/groups.py index 6973683..61c54f0 100644 --- a/ceo/cli/groups.py +++ b/ceo/cli/groups.py @@ -15,7 +15,7 @@ from ceod.transactions.groups import ( ) -@click.group() +@click.group(short_help='Perform operations on CSC groups/clubs') def groups(): pass diff --git a/ceo/cli/members.py b/ceo/cli/members.py index 37a82be..dd6a3e8 100644 --- a/ceo/cli/members.py +++ b/ceo/cli/members.py @@ -15,7 +15,7 @@ from ceod.transactions.members import ( ) -@click.group() +@click.group(short_help='Perform operations on CSC members and club reps') def members(): pass @@ -102,7 +102,10 @@ def print_user_lines(result: Dict): ('is a club', result['is_club']), ] if 'forwarding_addresses' in result: - lines.append(('forwarding addresses', ','.join(result['forwarding_addresses']))) + if len(result['forwarding_addresses']) != 0: + lines.append(('forwarding addresses', result['forwarding_addresses'][0])) + for address in result['forwarding_addresses'][1:]: + lines.append(('', address)) if 'terms' in result: lines.append(('terms', ','.join(result['terms']))) if 'non_member_terms' in result: diff --git a/ceo/cli/updateprograms.py b/ceo/cli/updateprograms.py new file mode 100644 index 0000000..c817dd6 --- /dev/null +++ b/ceo/cli/updateprograms.py @@ -0,0 +1,35 @@ +import click + +from ..utils import http_post +from .utils import handle_sync_response, print_colon_kv + + +@click.command(short_help="Sync the 'program' attribute with UWLDAP") +@click.option('--dry-run', is_flag=True, default=False) +@click.option('--members', required=False) +def updateprograms(dry_run, members): + body = {} + if dry_run: + body['dry_run'] = True + if members is not None: + body['members'] = ','.split(members) + + if not dry_run: + click.confirm('Are you sure that you want to sync programs with UWLDAP?', abort=True) + + resp = http_post('/api/uwldap/updateprograms', json=body) + result = handle_sync_response(resp) + if len(result) == 0: + click.echo('All programs are up-to-date.') + return + if dry_run: + click.echo('Members whose program would be changed:') + else: + click.echo('Members whose program was changed:') + lines = [] + for uid, csc_program, uw_program in result: + csc_program = csc_program or 'Unknown' + csc_program = click.style(csc_program, fg='yellow') + uw_program = click.style(uw_program, fg='green') + lines.append((uid, csc_program + ' -> ' + uw_program)) + print_colon_kv(lines) diff --git a/ceod/api/members.py b/ceod/api/members.py index 4788765..215d822 100644 --- a/ceod/api/members.py +++ b/ceod/api/members.py @@ -35,10 +35,12 @@ def create_user(): @requires_authentication_no_realm 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 + if user_is_in_group(auth_user, 'syscom'): + # Only syscom members may see the user's forwarding addresses, + # since this requires reading a file in the user's home directory. + # To avoid situations where an unprivileged user symlinks their + # ~/.forward file to /etc/shadow or something, we don't allow + # non-syscom members to use this option either. get_forwarding_addresses = True ldap_srv = component.getUtility(ILDAPService) user = ldap_srv.get_user(username) diff --git a/tests/ceo/cli/test_members.py b/tests/ceo/cli/test_members.py index 73e852f..a4fce2a 100644 --- a/tests/ceo/cli/test_members.py +++ b/tests/ceo/cli/test_members.py @@ -12,16 +12,15 @@ def test_members_get(cli_setup, ldap_user): runner = CliRunner() result = runner.invoke(cli, ['members', 'get', ldap_user.uid]) expected = ( - f"uid: {ldap_user.uid}\n" - f"cn: {ldap_user.cn}\n" - f"program: {ldap_user.program}\n" - f"UID number: {ldap_user.uid_number}\n" - f"GID number: {ldap_user.gid_number}\n" - f"login shell: {ldap_user.login_shell}\n" - f"home directory: {ldap_user.home_directory}\n" - f"is a club: {ldap_user.is_club()}\n" - "forwarding addresses: \n" - f"terms: {','.join(ldap_user.terms)}\n" + f"uid: {ldap_user.uid}\n" + f"cn: {ldap_user.cn}\n" + f"program: {ldap_user.program}\n" + f"UID number: {ldap_user.uid_number}\n" + f"GID number: {ldap_user.gid_number}\n" + f"login shell: {ldap_user.login_shell}\n" + f"home directory: {ldap_user.home_directory}\n" + f"is a club: {ldap_user.is_club()}\n" + f"terms: {','.join(ldap_user.terms)}\n" ) assert result.exit_code == 0 assert result.output == expected diff --git a/tests/ceo/cli/test_updatemembers.py b/tests/ceo/cli/test_updatemembers.py new file mode 100644 index 0000000..2f8f799 --- /dev/null +++ b/tests/ceo/cli/test_updatemembers.py @@ -0,0 +1,43 @@ +from click.testing import CliRunner +import ldap3 + +from ceo.cli import cli + + +def test_updatemembers(cli_setup, cfg, ldap_conn, ldap_user, uwldap_user): + # sanity check + assert ldap_user.uid == uwldap_user.uid + # modify the user's program in UWLDAP + conn = ldap_conn + base_dn = cfg.get('uwldap_base') + dn = f'uid={uwldap_user.uid},{base_dn}' + changes = {'ou': [(ldap3.MODIFY_REPLACE, ['New Program'])]} + conn.modify(dn, changes) + + runner = CliRunner() + result = runner.invoke(cli, ['updateprograms', '--dry-run']) + expected = ( + "Members whose program would be changed:\n" + f"{ldap_user.uid}: {ldap_user.program} -> New Program\n" + ) + assert result.exit_code == 0 + assert result.output == expected + + runner = CliRunner() + result = runner.invoke(cli, ['updateprograms'], input='y\n') + expected = ( + "Are you sure that you want to sync programs with UWLDAP? [y/N]: y\n" + "Members whose program was changed:\n" + f"{ldap_user.uid}: {ldap_user.program} -> New Program\n" + ) + assert result.exit_code == 0 + assert result.output == expected + + runner = CliRunner() + result = runner.invoke(cli, ['updateprograms'], input='y\n') + expected = ( + "Are you sure that you want to sync programs with UWLDAP? [y/N]: y\n" + "All programs are up-to-date.\n" + ) + assert result.exit_code == 0 + assert result.output == expected