From b4110d887ddc5bebfb0d74fbc663e7d5bd2a3128 Mon Sep 17 00:00:00 2001 From: Rio Date: Sat, 11 Dec 2021 16:30:18 -0500 Subject: [PATCH] Expire member cli and api (#33) Closes #23 Co-authored-by: Rio Liu Co-authored-by: Max Erenberg <> Reviewed-on: https://git.csclub.uwaterloo.ca/public/pyceo/pulls/33 Co-authored-by: Rio Co-committed-by: Rio --- ceo/cli/members.py | 15 +++++ ceo_common/interfaces/ILDAPService.py | 6 ++ ceo_common/interfaces/IUser.py | 1 + ceo_common/model/Term.py | 20 ++++-- ceod/api/members.py | 21 +++++- ceod/api/utils.py | 2 +- ceod/model/LDAPService.py | 26 ++++++++ ceod/model/User.py | 13 ++++ tests/ceo/cli/test_members.py | 26 ++++++++ tests/ceod/api/test_members.py | 83 ++++++++++++++++++----- tests/ceod/model/test_user.py | 1 + tests/conftest.py | 95 +++++++++++++++------------ tests/utils.py | 24 +++++++ 13 files changed, 267 insertions(+), 66 deletions(-) diff --git a/ceo/cli/members.py b/ceo/cli/members.py index 89bc724..db64729 100644 --- a/ceo/cli/members.py +++ b/ceo/cli/members.py @@ -187,3 +187,18 @@ def delete(username): 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) + + +@members.command(short_help="Check for and mark expired members") +@click.option('--dry-run', is_flag=True, default=False) +def expire(dry_run): + resp = http_post(f'/api/members/expire?dry_run={dry_run and "yes" or "no"}') + result = handle_sync_response(resp) + + if len(result) > 0: + if dry_run: + click.echo("The following members will be marked as expired:") + else: + click.echo("The following members has been marked as expired:") + for username in result: + click.echo(username) diff --git a/ceo_common/interfaces/ILDAPService.py b/ceo_common/interfaces/ILDAPService.py index 89424f4..870cebd 100644 --- a/ceo_common/interfaces/ILDAPService.py +++ b/ceo_common/interfaces/ILDAPService.py @@ -87,3 +87,9 @@ class ILDAPService(Interface): be returned along with their new programs, in the same format described above. """ + + def get_expiring_users(self) -> List[IUser]: + """ + Retrieves members whose term or nonMemberTerm does not contain the + current or the last term. + """ diff --git a/ceo_common/interfaces/IUser.py b/ceo_common/interfaces/IUser.py index f288a43..7523354 100644 --- a/ceo_common/interfaces/IUser.py +++ b/ceo_common/interfaces/IUser.py @@ -22,6 +22,7 @@ class IUser(Interface): 'a club rep') mail_local_addresses = Attribute('email aliases') is_club_rep = Attribute('whether this user is a club rep or not') + shadowExpire = Attribute('whether the user is marked as expired') # Non-LDAP attributes ldap3_entry = Attribute('cached ldap3.Entry instance for this user') diff --git a/ceo_common/model/Term.py b/ceo_common/model/Term.py index 650f045..3839b63 100644 --- a/ceo_common/model/Term.py +++ b/ceo_common/model/Term.py @@ -16,17 +16,22 @@ class Term: def __repr__(self): return self.s_term + @staticmethod + def from_datetime(dt: datetime.datetime): + """Get a Term object for the given date.""" + idx = (dt.month - 1) // 4 + c = Term.seasons[idx] + s_term = c + str(dt.year) + return Term(s_term) + @staticmethod def current(): """Get a Term object for the current date.""" dt = utils.get_current_datetime() - 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) + return Term.from_datetime(dt) + + def start_month(self): + return self.seasons.index(self.s_term[0]) * 4 + 1 def __add__(self, other): assert type(other) is int @@ -40,6 +45,7 @@ class Term: return Term(s_term) def __sub__(self, other): + assert type(other) is int return self.__add__(-other) def __eq__(self, other): diff --git a/ceod/api/members.py b/ceod/api/members.py index 0025287..a4f135f 100644 --- a/ceod/api/members.py +++ b/ceod/api/members.py @@ -1,9 +1,9 @@ -from flask import Blueprint, request +from flask import Blueprint, request, json from zope import component from .utils import authz_restrict_to_staff, authz_restrict_to_syscom, \ user_is_in_group, requires_authentication_no_realm, \ - create_streaming_response, development_only + create_streaming_response, development_only, is_truthy from ceo_common.errors import BadRequest from ceo_common.interfaces import ILDAPService from ceod.transactions.members import ( @@ -86,9 +86,11 @@ def renew_user(username: str): user = ldap_srv.get_user(username) if body.get('terms'): user.add_terms(body['terms']) + user.set_expired(False) return {'terms_added': body['terms']} elif body.get('non_member_terms'): user.add_non_member_terms(body['non_member_terms']) + user.set_expired(False) return {'non_member_terms_added': body['non_member_terms']} else: raise BadRequest('Must specify either terms or non-member terms') @@ -110,3 +112,18 @@ def reset_user_password(username: str): def delete_user(username: str): txn = DeleteMemberTransaction(username) return create_streaming_response(txn) + + +@bp.route('/expire', methods=['POST']) +@authz_restrict_to_syscom +def expire_users(): + dry_run = is_truthy(request.args.get('dry_run', 'false')) + + ldap_srv = component.getUtility(ILDAPService) + members = ldap_srv.get_expiring_users() + + if not dry_run: + for member in members: + member.set_expired(True) + + return json.jsonify([member.uid for member in members]) diff --git a/ceod/api/utils.py b/ceod/api/utils.py index acd255f..01ba4fa 100644 --- a/ceod/api/utils.py +++ b/ceod/api/utils.py @@ -140,4 +140,4 @@ def development_only(f: Callable) -> Callable: def is_truthy(s: str) -> bool: - return s.lower() in ['yes', 'true'] + return s.lower() in ['yes', 'true', '1'] diff --git a/ceod/model/LDAPService.py b/ceod/model/LDAPService.py index 7c1b96c..6c30f00 100644 --- a/ceod/model/LDAPService.py +++ b/ceod/model/LDAPService.py @@ -13,6 +13,8 @@ from ceo_common.errors import UserNotFoundError, GroupNotFoundError, \ UserAlreadyExistsError, GroupAlreadyExistsError from ceo_common.interfaces import ILDAPService, IConfig, \ IUser, IGroup, IUWLDAPService +from ceo_common.model import Term +import ceo_common.utils as ceo_common_utils from .User import User from .Group import Group @@ -243,6 +245,30 @@ class LDAPService: except ldap3.core.exceptions.LDAPEntryAlreadyExistsResult: raise GroupAlreadyExistsError() + def get_expiring_users(self) -> List[IUser]: + query = [] + + term = Term.current() + query.append(f'term={term}') + query.append(f'nonMemberTerm={term}') + + # Include last term too if the new term just started + dt = ceo_common_utils.get_current_datetime() + if dt.month == term.start_month(): + last_term = term - 1 + query.append(f'term={last_term}') + query.append(f'nonMemberTerm={last_term}') + + query = '(!(|(shadowExpire=1)(' + ')('.join(query) + ')))' + + conn = self._get_ldap_conn() + conn.search( + self.ldap_users_base, + query, + attributes=ldap3.ALL_ATTRIBUTES, + search_scope=ldap3.LEVEL) + return [User.deserialize_from_ldap(entry) for entry in conn.entries] + @contextlib.contextmanager def entry_ctx_for_group(self, group: IGroup): entry = self._get_writable_entry_for_group(group) diff --git a/ceod/model/User.py b/ceod/model/User.py index 6495c39..7118376 100644 --- a/ceod/model/User.py +++ b/ceod/model/User.py @@ -33,6 +33,7 @@ class User: is_club_rep: Union[bool, None] = None, is_club: bool = False, ldap3_entry: Union[ldap3.Entry, None] = None, + shadowExpire: Union[int, None] = None, ): cfg = component.getUtility(IConfig) @@ -66,6 +67,7 @@ class User: else: self.is_club_rep = is_club_rep self.ldap3_entry = ldap3_entry + self.shadowExpire = shadowExpire self.ldap_srv = component.getUtility(ILDAPService) self.krb_srv = component.getUtility(IKerberosService) @@ -82,6 +84,7 @@ class User: 'is_club': self.is_club(), 'is_club_rep': self.is_club_rep, 'program': self.program or 'Unknown', + 'shadowExpire': self.shadowExpire, } if self.sn and self.given_name: data['sn'] = self.sn @@ -155,6 +158,7 @@ class User: mail_local_addresses=attrs.get('mailLocalAddress'), is_club_rep=attrs.get('isClubRep', [False])[0], is_club=('club' in attrs['objectClass']), + shadowExpire=attrs.get('shadowExpire'), ldap3_entry=entry, ) @@ -205,3 +209,12 @@ class User: current_term = Term.current() most_recent_term = max(map(Term, self.terms)) return most_recent_term >= current_term + + def set_expired(self, expired: bool): + with self.ldap_srv.entry_ctx_for_user(self) as entry: + if expired: + entry.shadowExpire = 1 + self.shadowExpire = 1 + else: + entry.shadowExpire.remove() + self.shadowExpire = None diff --git a/tests/ceo/cli/test_members.py b/tests/ceo/cli/test_members.py index fcb153e..c1a1e5d 100644 --- a/tests/ceo/cli/test_members.py +++ b/tests/ceo/cli/test_members.py @@ -1,11 +1,14 @@ import os import re import shutil +from datetime import datetime from click.testing import CliRunner +from unittest.mock import patch from ceo.cli import cli from ceo_common.model import Term +import ceo_common.utils def test_members_get(cli_setup, ldap_user): @@ -142,3 +145,26 @@ def test_members_pwreset(cli_setup, ldap_user, krb_user): ), re.MULTILINE) assert result.exit_code == 0 assert expected_pat.match(result.output) is not None + + +def test_members_expire(cli_setup, ldap_user): + runner = CliRunner() + + with patch.object(ceo_common.utils, 'get_current_datetime') as datetime_mock: + # use a time that we know for sure will expire + datetime_mock.return_value = datetime(4000, 4, 1) + + result = runner.invoke(cli, ['members', 'expire', '--dry-run']) + assert result.exit_code == 0 + assert result.output == f"The following members will be marked as expired:\n{ldap_user.uid}\n" + + result = runner.invoke(cli, ['members', 'expire']) + assert result.exit_code == 0 + assert result.output == f"The following members has been marked as expired:\n{ldap_user.uid}\n" + + runner.invoke(cli, ['members', 'renew', ldap_user.uid, '--terms', '1']) + assert result.exit_code == 0 + + result = runner.invoke(cli, ['members', 'expire', '--dry-run']) + assert result.exit_code == 0 + assert result.output == '' diff --git a/tests/ceod/api/test_members.py b/tests/ceod/api/test_members.py index b936d0f..e62e18e 100644 --- a/tests/ceod/api/test_members.py +++ b/tests/ceod/api/test_members.py @@ -2,8 +2,11 @@ from unittest.mock import patch import ldap3 import pytest +import datetime -import ceod.utils as utils +import ceod.utils +import ceo_common.utils +from ceo_common.model import Term def test_api_user_not_found(client): @@ -12,26 +15,26 @@ def test_api_user_not_found(client): @pytest.fixture(scope='module') -def create_user_resp(client, mocks_for_create_user, mock_mail_server): +def create_user_resp(client, mocks_for_create_user_module, mock_mail_server): mock_mail_server.messages.clear() status, data = client.post('/api/members', json={ - 'uid': 'test_1', + 'uid': 'test1', 'cn': 'Test One', 'given_name': 'Test', 'sn': 'One', 'program': 'Math', 'terms': ['s2021'], - 'forwarding_addresses': ['test_1@uwaterloo.internal'], + 'forwarding_addresses': ['test1@uwaterloo.internal'], }) assert status == 200 assert data[-1]['status'] == 'completed' yield status, data - status, data = client.delete('/api/members/test_1') + status, data = client.delete('/api/members/test1') assert status == 200 assert data[-1]['status'] == 'completed' -@pytest.fixture(scope='module') +@pytest.fixture(scope='function') def create_user_result(create_user_resp): # convenience method _, data = create_user_resp @@ -54,25 +57,26 @@ def test_api_create_user(cfg, create_user_resp, mock_mail_server): "cn": "Test One", "given_name": "Test", "sn": "One", - "uid": "test_1", + "uid": "test1", "uid_number": min_uid, "gid_number": min_uid, "login_shell": "/bin/bash", - "home_directory": "/tmp/test_users/test_1", + "home_directory": "/tmp/test_users/test1", "is_club": False, "is_club_rep": False, "program": "Math", "terms": ["s2021"], - "mail_local_addresses": ["test_1@csclub.internal"], - "forwarding_addresses": ['test_1@uwaterloo.internal'], - "password": "krb5" + "mail_local_addresses": ["test1@csclub.internal"], + "forwarding_addresses": ['test1@uwaterloo.internal'], + "password": "krb5", + "shadowExpire": None, }}, ] assert data == expected # Two messages should have been sent: a welcome message to the new member, # and an announcement to the ceo mailing list assert len(mock_mail_server.messages) == 2 - assert mock_mail_server.messages[0]['to'] == 'test_1@csclub.internal' + assert mock_mail_server.messages[0]['to'] == 'test1@csclub.internal' assert mock_mail_server.messages[1]['to'] == 'ceo@csclub.internal,ctdalek@csclub.internal' mock_mail_server.messages.clear() @@ -198,7 +202,7 @@ def test_api_renew_user(cfg, client, create_user_result, ldap_conn): def test_api_reset_password(client, create_user_result): uid = create_user_result['uid'] - with patch.object(utils, 'gen_password') as gen_password_mock: + with patch.object(ceod.utils, 'gen_password') as gen_password_mock: gen_password_mock.return_value = 'new_password' status, data = client.post(f'/api/members/{uid}/pwreset') assert status == 200 @@ -212,7 +216,7 @@ def test_api_reset_password(client, create_user_result): def test_authz_check(client, create_user_result): # non-staff members may not create users status, data = client.post('/api/members', json={ - 'uid': 'test_1', 'cn': 'Test One', 'given_name': 'Test', + 'uid': 'test1', 'cn': 'Test One', 'given_name': 'Test', 'sn': 'One', 'terms': ['s2021'], }, principal='regular1') assert status == 403 @@ -227,7 +231,56 @@ 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', 'given_name': 'Test', + 'uid': 'test1', 'cn': 'Test One', 'given_name': 'Test', 'sn': 'One', 'terms': ['s2021'], }, principal='ctdalek', delegate=False) assert data[-1]['status'] == 'aborted' + + +@pytest.mark.parametrize('term_attr', ['terms', 'non_member_terms']) +def test_expire(client, new_user_gen, term_attr): + start_of_current_term = Term.current().to_datetime() + # test_date, should_expire + test_cases = [ + # same term, membership is still valid + (start_of_current_term + datetime.timedelta(days=90), False), + # first month of next term, grace period is activated + (start_of_current_term + datetime.timedelta(days=130), False), + # second month of next term, membership is now invalid + (start_of_current_term + datetime.timedelta(days=160), True), + # next next term, membership is definitely invalid + (start_of_current_term + datetime.timedelta(days=250), True), + ] + + for test_date, should_expire in test_cases: + with new_user_gen() as user_obj, \ + patch.object(ceo_common.utils, 'get_current_datetime') as datetime_mock: + user = user_obj.to_dict() + uid = user['uid'] + datetime_mock.return_value = test_date + + assert user['shadowExpire'] is None + + status, data = client.post('/api/members/expire?dry_run=yes') + assert status == 200 + assert (data == [uid]) == should_expire + + _, user = client.get(f'/api/members/{uid}') + assert user['shadowExpire'] is None + + status, data = client.post('/api/members/expire') + assert status == 200 + assert (data == [uid]) == should_expire + + _, user = client.get(f'/api/members/{uid}') + assert (user['shadowExpire'] is not None) == should_expire + + if not should_expire: + continue + + term = Term.from_datetime(test_date) + status, _ = client.post(f'/api/members/{uid}/renew', json={term_attr: [str(term)]}) + assert status == 200 + + _, user = client.get(f'/api/members/{uid}') + assert user['shadowExpire'] is None diff --git a/tests/ceod/model/test_user.py b/tests/ceod/model/test_user.py index 0cdcac9..f5baf6e 100644 --- a/tests/ceod/model/test_user.py +++ b/tests/ceod/model/test_user.py @@ -171,6 +171,7 @@ def test_user_to_dict(cfg): 'home_directory': user.home_directory, 'is_club': False, 'is_club_rep': False, + 'shadowExpire': None, } assert user.to_dict() == expected diff --git a/tests/conftest.py b/tests/conftest.py index 558af1c..f4aa991 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,15 +1,13 @@ import contextlib -import grp import importlib.resources from multiprocessing import Process import os -import pwd import shutil import subprocess from subprocess import DEVNULL import sys import time -from unittest.mock import patch, Mock +from unittest.mock import Mock import flask import gssapi @@ -19,7 +17,12 @@ import requests import socket from zope import component -from .utils import gssapi_token_ctx, ccache_cleanup # noqa: F401 +# noqa: F401 +from .utils import ( # noqa: F401 + gssapi_token_ctx, + ccache_cleanup, + mocks_for_create_user_ctx, +) from ceo_common.interfaces import IConfig, IKerberosService, ILDAPService, \ IFileService, IMailmanService, IHTTPClient, IUWLDAPService, IMailService, \ IDatabaseService, ICloudService @@ -29,7 +32,6 @@ from ceod.db import MySQLService, PostgreSQLService from ceod.model import KerberosService, LDAPService, FileService, User, \ MailmanService, Group, UWLDAPService, UWLDAPRecord, MailService, \ CloudService -import ceod.utils as utils from .MockSMTPServer import MockSMTPServer from .MockMailmanServer import MockMailmanServer from .MockCloudStackServer import MockCloudStackServer @@ -301,17 +303,15 @@ def app( return app -@pytest.fixture(scope='session') +@pytest.fixture def mocks_for_create_user(): - with patch.object(utils, 'gen_password') as gen_password_mock, \ - patch.object(pwd, 'getpwuid') as getpwuid_mock, \ - patch.object(grp, 'getgrgid') as getgrgid_mock: - gen_password_mock.return_value = 'krb5' - # Normally, if getpwuid or getgrgid do *not* raise a KeyError, - # then LDAPService will skip that UID. Therefore, by raising a - # KeyError, we are making sure that the UID will *not* be skipped. - getpwuid_mock.side_effect = KeyError() - getgrgid_mock.side_effect = KeyError() + with mocks_for_create_user_ctx(): + yield + + +@pytest.fixture(scope='module') +def mocks_for_create_user_module(): + with mocks_for_create_user_ctx(): yield @@ -356,32 +356,45 @@ def krb_user(simple_user): simple_user.remove_from_kerberos() -_new_user_id_counter = 10001 -@pytest.fixture # noqa: E302 -def new_user(client, g_admin_ctx, ldap_srv_session): # noqa: F811 - global _new_user_id_counter - uid = 'test' + str(_new_user_id_counter) - _new_user_id_counter += 1 - status, data = client.post('/api/members', json={ - 'uid': uid, - 'cn': 'John Doe', - 'given_name': 'John', - 'sn': 'Doe', - 'program': 'Math', - 'terms': [str(Term.current())], - }) - assert status == 200 - assert data[-1]['status'] == 'completed' - with g_admin_ctx(): - user = ldap_srv_session.get_user(uid) - subprocess.run([ - 'kadmin', '-k', '-p', 'ceod/admin', 'cpw', - '-pw', 'krb5', uid, - ], check=True) - yield user - status, data = client.delete(f'/api/members/{uid}') - assert status == 200 - assert data[-1]['status'] == 'completed' +@pytest.fixture +def new_user_gen( + client, g_admin_ctx, ldap_srv_session, mocks_for_create_user, # noqa: F811 +): + _new_user_id_counter = 11001 + + @contextlib.contextmanager + def wrapper(): + nonlocal _new_user_id_counter + uid = 'test' + str(_new_user_id_counter) + _new_user_id_counter += 1 + status, data = client.post('/api/members', json={ + 'uid': uid, + 'cn': 'John Doe', + 'given_name': 'John', + 'sn': 'Doe', + 'program': 'Math', + 'terms': [str(Term.current())], + }) + assert status == 200 + assert data[-1]['status'] == 'completed' + subprocess.run([ + 'kadmin', '-k', '-p', 'ceod/admin', + 'modprinc', '-needchange', uid, + ], check=True) + with g_admin_ctx(): + user = ldap_srv_session.get_user(uid) + yield user + status, data = client.delete(f'/api/members/{uid}') + assert status == 200 + assert data[-1]['status'] == 'completed' + + return wrapper + + +@pytest.fixture +def new_user(new_user_gen): + with new_user_gen() as user: + yield user @pytest.fixture diff --git a/tests/utils.py b/tests/utils.py index 1feaec1..c0b5f6f 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -1,8 +1,12 @@ +import ceod.utils as ceod_utils import contextlib import os +import grp +import pwd import subprocess from subprocess import DEVNULL import tempfile +from unittest.mock import patch import gssapi import pytest @@ -45,3 +49,23 @@ def ccache_cleanup(): """Make sure the ccache files get deleted at the end of the tests.""" yield _cache.clear() + + +@contextlib.contextmanager +def gen_password_mock_ctx(): + with patch.object(ceod_utils, 'gen_password') as mock: + mock.return_value = 'krb5' + yield + + +@contextlib.contextmanager +def mocks_for_create_user_ctx(): + with gen_password_mock_ctx(), \ + patch.object(pwd, 'getpwuid') as getpwuid_mock, \ + patch.object(grp, 'getgrgid') as getgrgid_mock: + # Normally, if getpwuid or getgrgid do *not* raise a KeyError, + # then LDAPService will skip that UID. Therefore, by raising a + # KeyError, we are making sure that the UID will *not* be skipped. + getpwuid_mock.side_effect = KeyError() + getgrgid_mock.side_effect = KeyError() + yield