From ff2ac95d5efa2bd4a968de05316ce14e9b469902 Mon Sep 17 00:00:00 2001 From: Max Erenberg Date: Mon, 2 Aug 2021 07:19:29 +0000 Subject: [PATCH] add PATCH /api/members/:username endpoint --- README.md | 7 +-- ceo_common/interfaces/IUser.py | 17 +++++- ceod/api/members.py | 32 +++++++++-- ceod/api/utils.py | 44 ++++++++++---- ceod/model/User.py | 20 ++++++- ceod/model/validators.py | 8 +++ ceod/transactions/AbstractTransaction.py | 3 + .../members/AddMemberTransaction.py | 26 ++++++--- .../members/ModifyMemberTransaction.py | 57 +++++++++++++++++++ ceod/transactions/members/__init__.py | 1 + 10 files changed, 181 insertions(+), 34 deletions(-) create mode 100644 ceod/transactions/members/ModifyMemberTransaction.py diff --git a/README.md b/README.md index 6286bb8..f6533b1 100644 --- a/README.md +++ b/README.md @@ -53,10 +53,5 @@ Here's an example of using curl with SPNEGO: ```sh # Get a Kerberos TGT first kinit -curl --negotiate -u : --service-name ceod http://mail:9987/api/mailman/csc-general/ctdalek -``` - -Not all endpoints are SPNEGO-protected, e.g. to get info for a member: -```sh -curl http://phosphoric-acid:9987/api/members/ctdalek +curl --negotiate -u : --service-name ceod -X POST http://mail:9987/api/mailman/csc-general/ctdalek ``` diff --git a/ceo_common/interfaces/IUser.py b/ceo_common/interfaces/IUser.py index 2f29ecb..696b2c4 100644 --- a/ceo_common/interfaces/IUser.py +++ b/ceo_common/interfaces/IUser.py @@ -63,12 +63,21 @@ class IUser(Interface): def change_password(password: str): """Replace this user's password.""" + def replace_login_shell(login_shell: str): + """Replace this user's login shell.""" + def create_home_dir(): """Create a new home directory for this user.""" def delete_home_dir(): """Delete this user's home dir.""" + def subscribe_to_mailing_list(mailing_list: str): + """Subscribe this user to the mailing list.""" + + def unsubscribe_from_mailing_list(mailing_list: str): + """Unsubscribe this user from the mailing list.""" + def serialize_for_ldap() -> Dict[str, List[bytes]]: """ Serialize this user into a dict to be passed to @@ -82,5 +91,9 @@ class IUser(Interface): :returns: IUser """ - def to_dict() -> Dict: - """Serialize this user into a dict.""" + def to_dict(get_forwarding_addresses: bool = False) -> Dict: + """ + Serialize this user into a dict. + If get_forwarding_addresses is True, the forwarding addresses + for the user will also be returned, if present. + """ diff --git a/ceod/api/members.py b/ceod/api/members.py index 8d47c03..e4d1287 100644 --- a/ceod/api/members.py +++ b/ceod/api/members.py @@ -1,10 +1,14 @@ from flask import Blueprint, request from zope import component -from .utils import authz_restrict_to_staff, create_streaming_response +from .utils import authz_restrict_to_staff, user_is_in_group, \ + requires_authentication_no_realm, create_streaming_response from ceo_common.errors import UserNotFoundError from ceo_common.interfaces import ILDAPService -from ceod.transactions.members import AddMemberTransaction +from ceod.transactions.members import ( + AddMemberTransaction, + ModifyMemberTransaction, +) bp = Blueprint('members', __name__) @@ -25,11 +29,31 @@ def create_user(): @bp.route('/') -def get_user(username: str): +@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'): + get_forwarding_addresses = True ldap_srv = component.getUtility(ILDAPService) try: - return ldap_srv.get_user(username).to_dict() + return ldap_srv.get_user(username).to_dict(get_forwarding_addresses) except UserNotFoundError: return { 'error': 'user not found' }, 404 + + +@bp.route('/', methods=['PATCH']) +@requires_authentication_no_realm +def patch_user(auth_user: str, username: str): + if not (auth_user == username or user_is_in_group(auth_user, 'syscom')): + return { + 'error': "You are not authorized to modify other users' attributes" + }, 403 + body = request.get_json(force=True) + txn = ModifyMemberTransaction( + username, + login_shell=body.get('login_shell'), + forwarding_addresses=body.get('forwarding_addresses'), + ) + return create_streaming_response(txn) diff --git a/ceod/api/utils.py b/ceod/api/utils.py index 4e7de28..e124422 100644 --- a/ceod/api/utils.py +++ b/ceod/api/utils.py @@ -1,7 +1,10 @@ import functools import grp import json +import os +import pwd import socket +import traceback from typing import Callable, List from flask import current_app @@ -49,29 +52,45 @@ def restrict_host(role: str) -> Callable[[Callable], Callable]: return error_decorator +def requires_authentication_no_realm(f: Callable) -> Callable: + """ + Like requires_authentication, but strips the realm out of the principal string. + e.g. user1@CSCLUB.UWATERLOO.CA -> user1 + """ + @requires_authentication + @functools.wraps(f) + def wrapper(principal: str, *args, **kwargs): + user = principal[:principal.index('@')] + logger.debug(f'received request from {user}') + return f(user, *args, **kwargs) + return wrapper + + +def user_is_in_group(user: str, group: str) -> bool: + """Returns True if `user` is in `group`, False otherwise.""" + return user in grp.getgrnam(group).gr_mem + + def authz_restrict_to_groups(f: Callable, allowed_groups: List[str]) -> Callable: """ Restrict an endpoint to users who belong to one or more of the specified groups. """ - # TODO: cache group members, but place a time limit on the cache validity + allowed_group_ids = [grp.getgrnam(g).gr_gid for g in allowed_groups] - @requires_authentication + @requires_authentication_no_realm @functools.wraps(f) - def wrapper(user: str, *args, **kwargs): - """ - :param user: a Kerberos principal (e.g. 'user1@CSCLUB.UWATERLOO.CA') - """ - logger.debug(f'received request from {user}') - username = user[:user.index('@')] + def wrapper(_username: str, *args, **kwargs): + # we need to call the argument _username to avoid name clashes with + # the arguments of f + username = _username if username.startswith('ceod/'): # ceod services are always allowed to make internal calls return f(*args, **kwargs) - for group in allowed_groups: - for group_member in grp.getgrnam(group).gr_mem: - if username == group_member: - return f(*args, **kwargs) + for gid in os.getgrouplist(username, pwd.getpwnam(username).pw_gid): + if gid in allowed_group_ids: + return f(*args, **kwargs) logger.debug( f"User '{username}' denied since they are not in one of {allowed_groups}" ) @@ -113,6 +132,7 @@ def create_streaming_response(txn: AbstractTransaction): 'result': txn.result, }) + '\n' except Exception as err: + logger.warning('Transaction failed:\n' + traceback.format_exc()) txn.rollback() yield json.dumps({ 'status': 'aborted', diff --git a/ceod/model/User.py b/ceod/model/User.py index 2398014..93a4512 100644 --- a/ceod/model/User.py +++ b/ceod/model/User.py @@ -7,8 +7,9 @@ from zope import component from zope.interface import implementer from .utils import strings_to_bytes, bytes_to_strings +from .validators import is_valid_shell from ceo_common.interfaces import ILDAPService, IKerberosService, IFileService, \ - IUser, IConfig + IUser, IConfig, IMailmanService @implementer(IUser) @@ -56,7 +57,7 @@ class User: self.krb_srv = component.getUtility(IKerberosService) self.file_srv = component.getUtility(IFileService) - def to_dict(self) -> Dict: + def to_dict(self, get_forwarding_addresses: bool = False) -> Dict: data = { 'dn': self.dn, 'cn': self.cn, @@ -77,6 +78,8 @@ class User: data['positions'] = self.positions if self.mail_local_addresses: data['mail_local_addresses'] = self.mail_local_addresses + if get_forwarding_addresses: + data['forwarding_addresses'] = self.get_forwarding_addresses() return data def __repr__(self) -> str: @@ -110,6 +113,12 @@ class User: def delete_home_dir(self): self.file_srv.delete_home_dir(self) + def subscribe_to_mailing_list(self, mailing_list: str): + component.getUtility(IMailmanService).subscribe(self.uid, mailing_list) + + def unsubscribe_from_mailing_list(self, mailing_list: str): + component.getUtility(IMailmanService).unsubscribe(self.uid, mailing_list) + def serialize_for_ldap(self) -> Dict: data = { 'cn': [self.cn], @@ -161,6 +170,13 @@ class User: is_club=('club' in data['objectClass']), ) + def replace_login_shell(self, login_shell: str): + new_user = copy.copy(self) + if not is_valid_shell(login_shell): + raise Exception('%s is not a valid shell' % login_shell) + new_user.login_shell = login_shell + self.ldap_srv.modify_user(self, new_user) + def add_terms(self, terms: List[str]): new_user = copy.copy(self) new_user.terms = self.terms.copy() diff --git a/ceod/model/validators.py b/ceod/model/validators.py index 3a53517..a59b602 100644 --- a/ceod/model/validators.py +++ b/ceod/model/validators.py @@ -50,3 +50,11 @@ def check_valid_username(f: Callable) -> Callable: def is_valid_forwarding_address(address: str) -> bool: """Returns True if the address is a valid forwarding address.""" return valid_forwarding_address_re.match(address) is not None + + +def is_valid_shell(shell: str) -> bool: + """Returns True if this shell is in /etc/shells, otherwise False.""" + return shell in [ + line.strip() for line in open('/etc/shells') + if line != '' and not line.isspace() + ] diff --git a/ceod/transactions/AbstractTransaction.py b/ceod/transactions/AbstractTransaction.py index 5c16f27..aae7a42 100644 --- a/ceod/transactions/AbstractTransaction.py +++ b/ceod/transactions/AbstractTransaction.py @@ -14,6 +14,9 @@ class AbstractTransaction(ABC): self.result = None def finish(self, result): + """ + Store a result in this object so that it can be returned to the client. + """ self.result = result @abstractmethod diff --git a/ceod/transactions/members/AddMemberTransaction.py b/ceod/transactions/members/AddMemberTransaction.py index c897b6f..36d769d 100644 --- a/ceod/transactions/members/AddMemberTransaction.py +++ b/ceod/transactions/members/AddMemberTransaction.py @@ -1,13 +1,17 @@ import base64 import os +import traceback from typing import Union, List from zope import component from ..AbstractTransaction import AbstractTransaction -from ceo_common.interfaces import IConfig, IMailService, IMailmanService +from ceo_common.interfaces import IConfig, IMailService +from ceo_common.logger_factory import logger_factory from ceod.model import User, Group +logger = logger_factory(__name__) + def gen_password() -> str: """Generate a temporary password.""" @@ -48,7 +52,6 @@ class AddMemberTransaction(AbstractTransaction): self.group = None self.new_member_list = cfg.get('new_member_list') self.mail_srv = component.getUtility(IMailService) - self.mailman_srv = component.getUtility(IMailmanService) def child_execute_iter(self): member = User( @@ -84,14 +87,21 @@ class AddMemberTransaction(AbstractTransaction): # The following operations can't/shouldn't be rolled back because the # user has already seen the email - self.mail_srv.send_welcome_message_to(member) - yield 'send_welcome_message' + try: + self.mail_srv.send_welcome_message_to(member) + 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) - # This will be done on mail (remote) - self.mailman_srv.subscribe(member.uid, self.new_member_list) - yield 'subscribe_to_mailing_list' + try: + member.subscribe_to_mailing_list(self.new_member_list) + yield 'subscribe_to_mailing_list' + except Exception as err: + logger.warning('subscribe_to_mailing_list failed:\n' + traceback.format_exc()) + yield 'failed_to_subscribe_to_mailing_list\n' + str(err) - user_json = member.to_dict() + user_json = member.to_dict(True) # insert the password into the JSON so that the client can see it user_json['password'] = password self.finish(user_json) diff --git a/ceod/transactions/members/ModifyMemberTransaction.py b/ceod/transactions/members/ModifyMemberTransaction.py new file mode 100644 index 0000000..a4269a5 --- /dev/null +++ b/ceod/transactions/members/ModifyMemberTransaction.py @@ -0,0 +1,57 @@ +from typing import Union, List + +from zope import component + +from ..AbstractTransaction import AbstractTransaction +from ceo_common.interfaces import ILDAPService + + +class ModifyMemberTransaction(AbstractTransaction): + """ + Transaction to modify a member's attributes. + These attributes should be modifiable by the user themselves. + """ + + operations = [ + 'replace_login_shell', + 'replace_forwarding_addresses', + ] + + def __init__( + self, + username: str, + login_shell: Union[str, None], + forwarding_addresses: Union[List[str], None], + ): + super().__init__() + self.username = username + self.login_shell = login_shell + self.forwarding_addresses = forwarding_addresses + self.ldap_srv = component.getUtility(ILDAPService) + + # For the rollback + self.user = None + self.old_login_shell = None + self.old_forwarding_addresses = None + + def child_execute_iter(self): + user = self.ldap_srv.get_user(self.username) + self.user = user + + if self.login_shell: + self.old_login_shell = user.login_shell + user.replace_login_shell(self.login_shell) + yield 'replace_login_shell' + + if self.forwarding_addresses: + self.old_forwarding_addresses = user.get_forwarding_addresses() + user.set_forwarding_addresses(self.forwarding_addresses) + yield 'replace_forwarding_addresses' + + self.finish('OK') + + def rollback(self): + if 'replace_login_shell' in self.finished_operations: + self.user.replace_login_shell(self.old_login_shell) + if 'replace_forwarding_addresses' in self.finished_operations: + self.user.set_forwarding_addresses(self.old_forwarding_addresses) diff --git a/ceod/transactions/members/__init__.py b/ceod/transactions/members/__init__.py index 8211901..da8a9cd 100644 --- a/ceod/transactions/members/__init__.py +++ b/ceod/transactions/members/__init__.py @@ -1 +1,2 @@ from .AddMemberTransaction import AddMemberTransaction +from .ModifyMemberTransaction import ModifyMemberTransaction