From e3c50d867a6980d13cdd5207115cb2bc44bc275b Mon Sep 17 00:00:00 2001 From: Max Erenberg Date: Sat, 23 Oct 2021 10:23:43 -0400 Subject: [PATCH] Add isClubRep attribute (#27) Closes #24. Co-authored-by: Max Erenberg <> Reviewed-on: https://git.csclub.uwaterloo.ca/public/pyceo/pulls/27 Co-authored-by: Max Erenberg Co-committed-by: Max Erenberg --- .drone/csc.schema | 6 ++- README.md | 4 +- ceo/utils.py | 2 + ceo_common/interfaces/IUser.py | 1 + ceod/api/members.py | 14 ++++++- ceod/model/LDAPService.py | 2 + ceod/model/User.py | 17 +++++++++ ceod/model/utils.py | 17 ++++++++- .../members/RenewMemberTransaction.py | 3 -- architecture.md => docs/architecture.md | 0 one_time_scripts/is_club_rep.py | 37 +++++++++++++++++++ setup.cfg | 2 +- tests/ceo/cli/test_members.py | 4 +- tests/ceod/api/test_members.py | 3 ++ tests/ceod/model/test_user.py | 26 +++++++++++++ 15 files changed, 128 insertions(+), 10 deletions(-) rename architecture.md => docs/architecture.md (100%) create mode 100644 one_time_scripts/is_club_rep.py diff --git a/.drone/csc.schema b/.drone/csc.schema index e104398..9c00a43 100644 --- a/.drone/csc.schema +++ b/.drone/csc.schema @@ -20,10 +20,14 @@ attributetype ( 1.3.6.1.4.1.27934.1.1.5 NAME 'nonMemberTerm' EQUALITY caseIgnoreIA5Match SYNTAX 1.3.6.1.4.1.1466.115.121.1.26{5} ) +attributetype ( 1.3.6.1.4.1.27934.1.1.6 NAME 'isClubRep' + EQUALITY booleanMatch + SYNTAX 1.3.6.1.4.1.1466.115.121.1.7 ) + objectclass ( 1.3.6.1.4.1.27934.1.2.1 NAME 'member' SUP top AUXILIARY MUST ( cn $ uid ) - MAY ( studentid $ program $ term $ nonMemberTerm $ description $ position ) ) + MAY ( studentid $ program $ term $ nonMemberTerm $ description $ position $ isClubRep ) ) objectclass ( 1.3.6.1.4.1.27934.1.2.2 NAME 'club' SUP top AUXILIARY diff --git a/README.md b/README.md index 4779ae5..8a9998b 100644 --- a/README.md +++ b/README.md @@ -2,9 +2,11 @@ [![Build Status](https://ci.csclub.uwaterloo.ca/api/badges/public/pyceo/status.svg?ref=refs/heads/v1)](https://ci.csclub.uwaterloo.ca/public/pyceo) CEO (**C**SC **E**lectronic **O**ffice) is the tool used by CSC to manage -club accounts and memberships. See [architecture.md](architecture.md) for an +club accounts and memberships. See [docs/architecture.md](docs/architecture.md) for an overview of its architecture. +The API documentation is available as a plain HTML file in [docs/redoc-static.html](docs/redoc-static.html). + ## Development ### Docker If you are not modifying code related to email or Mailman, then you may use diff --git a/ceo/utils.py b/ceo/utils.py index cec156e..44f96dd 100644 --- a/ceo/utils.py +++ b/ceo/utils.py @@ -105,6 +105,8 @@ def user_dict_kv(d: Dict) -> List[Tuple[str]]: pairs.append(('home directory', d['home_directory'])) if 'is_club' in d: pairs.append(('is a club', str(d['is_club']))) + if 'is_club_rep' in d: + pairs.append(('is a club rep', str(d['is_club_rep']))) if 'forwarding_addresses' in d: if len(d['forwarding_addresses']) > 0: pairs.append(('forwarding addresses', d['forwarding_addresses'][0])) diff --git a/ceo_common/interfaces/IUser.py b/ceo_common/interfaces/IUser.py index 60fb091..2ebdcb9 100644 --- a/ceo_common/interfaces/IUser.py +++ b/ceo_common/interfaces/IUser.py @@ -19,6 +19,7 @@ class IUser(Interface): non_member_terms = Attribute('list of terms for which this person was ' 'a club rep') mail_local_addresses = Attribute('email aliases') + is_club_rep = Attribute('whether this user is a club rep or not') # Non-LDAP attributes ldap3_entry = Attribute('cached ldap3.Entry instance for this user') diff --git a/ceod/api/members.py b/ceod/api/members.py index 215d822..1502d88 100644 --- a/ceod/api/members.py +++ b/ceod/api/members.py @@ -20,12 +20,17 @@ bp = Blueprint('members', __name__) @authz_restrict_to_staff def create_user(): body = request.get_json(force=True) + terms = body.get('terms') + non_member_terms = body.get('non_member_terms') + if (terms and non_member_terms) or not (terms or non_member_terms): + raise BadRequest('Must specify either terms or non-member terms') + txn = AddMemberTransaction( uid=body['uid'], cn=body['cn'], program=body.get('program'), - terms=body.get('terms'), - non_member_terms=body.get('non_member_terms'), + terms=terms, + non_member_terms=non_member_terms, forwarding_addresses=body.get('forwarding_addresses'), ) return create_streaming_response(txn) @@ -67,6 +72,11 @@ def patch_user(auth_user: str, username: str): @authz_restrict_to_staff def renew_user(username: str): body = request.get_json(force=True) + terms = body.get('terms') + non_member_terms = body.get('non_member_terms') + if (terms and non_member_terms) or not (terms or non_member_terms): + raise BadRequest('Must specify either terms or non-member terms') + ldap_srv = component.getUtility(ILDAPService) user = ldap_srv.get_user(username) if body.get('terms'): diff --git a/ceod/model/LDAPService.py b/ceod/model/LDAPService.py index 800fbcf..d94e234 100644 --- a/ceod/model/LDAPService.py +++ b/ceod/model/LDAPService.py @@ -197,6 +197,8 @@ class LDAPService: entry.position = user.positions if user.mail_local_addresses: entry.mailLocalAddress = user.mail_local_addresses + if user.is_club_rep: + entry.isClubRep = True if not user.is_club(): entry.userPassword = '{SASL}%s@%s' % (user.uid, self.ldap_sasl_realm) diff --git a/ceod/model/User.py b/ceod/model/User.py index 7b57eeb..d5db444 100644 --- a/ceod/model/User.py +++ b/ceod/model/User.py @@ -6,6 +6,7 @@ import ldap3 from zope import component from zope.interface import implementer +from .utils import should_be_club_rep from .validators import is_valid_shell, is_valid_term from ceo_common.interfaces import ILDAPService, IKerberosService, IFileService, \ IUser, IConfig, IMailmanService @@ -26,6 +27,7 @@ class User: home_directory: Union[str, None] = None, positions: Union[List[str], None] = None, mail_local_addresses: Union[List[str], None] = None, + is_club_rep: Union[bool, None] = None, is_club: bool = False, ldap3_entry: Union[ldap3.Entry, None] = None, ): @@ -52,6 +54,14 @@ class User: self.positions = positions or [] self.mail_local_addresses = mail_local_addresses or [] self._is_club = is_club + if is_club_rep is None: + if is_club: + # not a real user + self.is_club_rep = False + else: + self.is_club_rep = should_be_club_rep(terms, non_member_terms) + else: + self.is_club_rep = is_club_rep self.ldap3_entry = ldap3_entry self.ldap_srv = component.getUtility(ILDAPService) @@ -66,6 +76,7 @@ class User: 'login_shell': self.login_shell, 'home_directory': self.home_directory, 'is_club': self.is_club(), + 'is_club_rep': self.is_club_rep, 'program': self.program or 'Unknown', } if self.terms: @@ -131,6 +142,7 @@ class User: home_directory=attrs['homeDirectory'][0], positions=attrs.get('position'), mail_local_addresses=attrs.get('mailLocalAddress'), + is_club_rep=attrs.get('isClubRep', [False])[0], is_club=('club' in attrs['objectClass']), ldap3_entry=entry, ) @@ -148,7 +160,10 @@ class User: raise Exception('%s is not a valid term' % term) with self.ldap_srv.entry_ctx_for_user(self) as entry: entry.term.add(terms) + if entry.isClubRep.value: + entry.isClubRep.remove() self.terms.extend(terms) + self.is_club_rep = False def add_non_member_terms(self, terms: List[str]): for term in terms: @@ -156,7 +171,9 @@ class User: raise Exception('%s is not a valid term' % term) with self.ldap_srv.entry_ctx_for_user(self) as entry: entry.nonMemberTerm.add(terms) + entry.isClubRep = True self.non_member_terms.extend(terms) + self.is_club_rep = True def set_positions(self, positions: List[str]): with self.ldap_srv.entry_ctx_for_user(self) as entry: diff --git a/ceod/model/utils.py b/ceod/model/utils.py index 80610c3..6b215f7 100644 --- a/ceod/model/utils.py +++ b/ceod/model/utils.py @@ -1,4 +1,6 @@ -from typing import Dict, List +from typing import Dict, List, Union + +from ceo_common.model import Term def bytes_to_strings(data: Dict[str, List[bytes]]) -> Dict[str, List[str]]: @@ -25,3 +27,16 @@ def dn_to_uid(dn: str) -> str: -> 'ctdalek' """ return dn.split(',', 1)[0].split('=')[1] + + +def should_be_club_rep(terms: Union[None, List[str]], + non_member_terms: Union[None, List[str]]) -> bool: + """Returns True iff a user's most recent term was a non-member term.""" + if not non_member_terms: + # no non-member terms => was only ever a member + return False + if not terms: + # no member terms => was only ever a club rep + return True + # decide using the most recent term (member or non-member) + return max(map(Term, non_member_terms)) > max(map(Term, terms)) diff --git a/ceod/transactions/members/RenewMemberTransaction.py b/ceod/transactions/members/RenewMemberTransaction.py index ba37f02..58ca284 100644 --- a/ceod/transactions/members/RenewMemberTransaction.py +++ b/ceod/transactions/members/RenewMemberTransaction.py @@ -3,7 +3,6 @@ from typing import Union, List from zope import component from ..AbstractTransaction import AbstractTransaction -from ceo_common.errors import BadRequest from ceo_common.interfaces import ILDAPService @@ -23,8 +22,6 @@ class RenewMemberTransaction(AbstractTransaction): ): super().__init__() self.username = username - if (terms and non_member_terms) or not (terms or non_member_terms): - raise BadRequest('Must specify either terms or non-member terms') self.terms = terms self.non_member_terms = non_member_terms self.ldap_srv = component.getUtility(ILDAPService) diff --git a/architecture.md b/docs/architecture.md similarity index 100% rename from architecture.md rename to docs/architecture.md diff --git a/one_time_scripts/is_club_rep.py b/one_time_scripts/is_club_rep.py new file mode 100644 index 0000000..8de1ab8 --- /dev/null +++ b/one_time_scripts/is_club_rep.py @@ -0,0 +1,37 @@ +#!/usr/bin/env python3 +""" +This is a script which adds the isClubRep attribute to all LDAP user records +whose most recent nonMemberTerm is later than their most recent (member) term. + +GSSAPI is used for LDAP authentication, so make sure to run `kinit` first. +Also, make sure to run this script from the top-level of the git directory +(see the sys.path hack below). +""" +import os +import sys + +import ldap3 + +sys.path.append(os.getcwd()) +from ceod.model.utils import should_be_club_rep + +# modify as necessary +LDAP_URI = "ldaps://auth1.csclub.uwaterloo.ca" +LDAP_MEMBERS_BASE = "ou=People,dc=csclub,dc=uwaterloo,dc=ca" + +conn = ldap3.Connection( + LDAP_URI, authentication=ldap3.SASL, sasl_mechanism=ldap3.KERBEROS, + auto_bind=True, raise_exceptions=True) +conn.search(LDAP_MEMBERS_BASE, '(objectClass=member)', + attributes=['uid', 'isClubRep', 'term', 'nonMemberTerm']) +total_records_updated = 0 +for entry in conn.entries: + if not should_be_club_rep(entry.term.values, entry.nonMemberTerm.values): + continue + if entry.isClubRep.value: + continue + changes = {'isClubRep': [(ldap3.MODIFY_REPLACE, [True])]} + conn.modify(entry.entry_dn, changes) + print('Modified %s' % entry.uid.value) + total_records_updated += 1 +print('Total records updated: %d' % total_records_updated) diff --git a/setup.cfg b/setup.cfg index 428a656..1833634 100644 --- a/setup.cfg +++ b/setup.cfg @@ -2,4 +2,4 @@ ignore = # line too long E501 -exclude = .git,.vscode,venv,__pycache__,__init__.py,build,dist +exclude = .git,.vscode,venv,__pycache__,__init__.py,build,dist,one_time_scripts diff --git a/tests/ceo/cli/test_members.py b/tests/ceo/cli/test_members.py index 2670e69..1b65fa4 100644 --- a/tests/ceo/cli/test_members.py +++ b/tests/ceo/cli/test_members.py @@ -20,6 +20,7 @@ def test_members_get(cli_setup, ldap_user): 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"is a club rep: {ldap_user.is_club_rep}\n" "forwarding addresses: \n" f"member terms: {','.join(ldap_user.terms)}\n" ) @@ -58,7 +59,8 @@ def test_members_add(cli_setup): "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" + "is a club rep: False\n" + "forwarding addresses: test_1@uwaterloo\\.internal\n" "member terms: [sfw]\\d{4}\n" "password: \\S+\n$" ), re.MULTILINE) diff --git a/tests/ceod/api/test_members.py b/tests/ceod/api/test_members.py index 2933cb4..1188eef 100644 --- a/tests/ceod/api/test_members.py +++ b/tests/ceod/api/test_members.py @@ -56,6 +56,7 @@ def test_api_create_user(cfg, create_user_resp, mock_mail_server): "login_shell": "/bin/bash", "home_directory": "/tmp/test_users/test_1", "is_club": False, + "is_club_rep": False, "program": "Math", "terms": ["s2021"], "forwarding_addresses": ['test_1@uwaterloo.internal'], @@ -175,6 +176,7 @@ def test_api_renew_user(cfg, client, create_user_result, ldap_conn): _, data = client.get(f'/api/members/{uid}') assert data['terms'] == old_terms + new_terms assert data['non_member_terms'] == old_non_member_terms + new_non_member_terms + assert data['is_club_rep'] # cleanup base_dn = cfg.get('ldap_users_base') @@ -182,6 +184,7 @@ def test_api_renew_user(cfg, client, create_user_result, ldap_conn): changes = { 'term': [(ldap3.MODIFY_REPLACE, old_terms)], 'nonMemberTerm': [(ldap3.MODIFY_REPLACE, old_non_member_terms)], + 'isClubRep': [(ldap3.MODIFY_REPLACE, [])], } ldap_conn.modify(dn, changes) diff --git a/tests/ceod/model/test_user.py b/tests/ceod/model/test_user.py index fc2c2c2..81ee28f 100644 --- a/tests/ceod/model/test_user.py +++ b/tests/ceod/model/test_user.py @@ -162,6 +162,7 @@ def test_user_to_dict(cfg): 'login_shell': '/bin/bash', 'home_directory': user.home_directory, 'is_club': False, + 'is_club_rep': False, } assert user.to_dict() == expected @@ -172,3 +173,28 @@ def test_user_to_dict(cfg): user.create_home_dir() expected['forwarding_addresses'] = [] assert user.to_dict(True) == expected + + +def test_user_is_club_rep(ldap_user, ldap_srv): + user = User( + uid='test_jsmith', + cn='John Smith', + program='Math', + terms=['s2021'], + ) + assert not user.is_club_rep + club_rep = User( + uid='test_jdoe', + cn='John Doe', + program='Math', + non_member_terms=['s2021'], + ) + assert club_rep.is_club_rep + + ldap_user.add_terms(['f2021']) + assert not ldap_user.is_club_rep + assert not ldap_srv.get_user(ldap_user.uid).is_club_rep + + ldap_user.add_non_member_terms(['w2022']) + assert ldap_user.is_club_rep + assert ldap_srv.get_user(ldap_user.uid).is_club_rep