From baeb83b1e20d029d9df2ca47ca6e6b5e4169f7e1 Mon Sep 17 00:00:00 2001 From: Max Erenberg Date: Tue, 3 Aug 2021 23:19:33 +0000 Subject: [PATCH] use ConfigParser --- ceo_common/errors.py | 27 ++++++++++++-- ceo_common/model/Config.py | 47 +++++++----------------- ceo_common/model/HTTPClient.py | 4 ++ ceo_common/model/RemoteMailmanService.py | 2 +- ceo_common/test/__init__.py | 0 ceo_common/test/ceod_dev.ini | 45 +++++++++++++++++++++++ ceod/api/app_factory.py | 22 +++++++---- ceod/api/error_handlers.py | 23 ++++++++++++ ceod/api/mailman.py | 7 +++- ceod/api/members.py | 3 +- ceod/model/FileService.py | 4 +- ceod/model/LDAPService.py | 22 +++++++---- ceod/model/MailService.py | 4 +- ceod/model/MailmanService.py | 32 ++++++++++------ ceod/model/User.py | 4 +- 15 files changed, 172 insertions(+), 74 deletions(-) create mode 100644 ceo_common/test/__init__.py create mode 100644 ceo_common/test/ceod_dev.ini create mode 100644 ceod/api/error_handlers.py diff --git a/ceo_common/errors.py b/ceo_common/errors.py index e99e1fc..8bba115 100644 --- a/ceo_common/errors.py +++ b/ceo_common/errors.py @@ -1,18 +1,37 @@ class UserNotFoundError(Exception): - pass + def __init__(self): + super().__init__('user not found') class GroupNotFoundError(Exception): - pass + def __init__(self): + super().__init__('group not found') class BadRequest(Exception): pass +class UserAlreadyExistsError(Exception): + def __init__(self): + super().__init__('user already exists') + + +class GroupAlreadyExistsError(Exception): + def __init__(self): + super().__init__('group already exists') + + class UserAlreadySubscribedError(Exception): - pass + def __init__(self): + super().__init__('user is already subscribed') class UserNotSubscribedError(Exception): - pass + def __init__(self): + super().__init__('user is not subscribed') + + +class NoSuchListError(Exception): + def __init__(self): + super().__init__('mailing list does not exist') diff --git a/ceo_common/model/Config.py b/ceo_common/model/Config.py index 0f32c13..da75703 100644 --- a/ceo_common/model/Config.py +++ b/ceo_common/model/Config.py @@ -1,3 +1,7 @@ +from configparser import ConfigParser +import os +from typing import Union + from zope.interface import implementer from ceo_common.interfaces import IConfig @@ -5,39 +9,14 @@ from ceo_common.interfaces import IConfig @implementer(IConfig) class Config: - # TODO: read from a config file - _domain = 'csclub.internal' - _ldap_base = ','.join(['dc=' + dc for dc in _domain.split('.')]) - _config = { - 'base_domain': _domain, - 'ldap_admin_principal': 'ceod/admin', - 'ldap_server_url': 'ldap://ldap-master.' + _domain, - 'ldap_users_base': 'ou=People,' + _ldap_base, - 'ldap_groups_base': 'ou=Group,' + _ldap_base, - 'ldap_sudo_base': 'ou=SUDOers,' + _ldap_base, - 'ldap_sasl_realm': _domain.upper(), - 'uwldap_server_url': 'ldap://uwldap.uwaterloo.ca', - 'uwldap_base': 'dc=uwaterloo,dc=ca', - 'member_min_id': 20001, - 'member_max_id': 29999, - 'club_min_id': 30001, - 'club_max_id': 39999, - 'member_home': '/users', - 'club_home': '/users', - 'member_home_skel': '/users/skel', - 'club_home_skel': '/users/skel', - 'smtp_url': 'smtp://mail.' + _domain, - 'smtp_starttls': False, - 'mailman3_api_base_url': 'http://localhost:8001/3.1', - 'mailman3_api_username': 'restadmin', - 'mailman3_api_password': 'mailman3', - 'new_member_list': 'csc-general', - 'ceod_admin_host': 'phosphoric-acid', - 'fs_root_host': 'phosphoric-acid', - 'mailman_host': 'mail', - 'ceod_use_https': False, - 'ceod_port': 9987, - } + def __init__(self, config_file: Union[str, None] = None): + if config_file is None: + config_file = os.environ.get('CEOD_CONFIG', '/etc/csc/ceod.ini') + self.config = ConfigParser() + self.config.read(config_file) def get(self, key: str) -> str: - return self._config[key] + section, subkey = key.split('_', 1) + if section in self.config: + return self.config[section][subkey] + return self.config['DEFAULT'][key] diff --git a/ceo_common/model/HTTPClient.py b/ceo_common/model/HTTPClient.py index 660bb78..c5b8ebe 100644 --- a/ceo_common/model/HTTPClient.py +++ b/ceo_common/model/HTTPClient.py @@ -20,6 +20,7 @@ class HTTPClient: else: self.scheme = 'http' self.ceod_port = cfg.get('ceod_port') + self.base_domain = cfg.get('base_domain') # Determine which principal to use for SPNEGO # TODO: this code is duplicated in app_factory.py. Figure out @@ -51,6 +52,9 @@ class HTTPClient: target_name='ceod', creds=self.get_creds(), ) + # always use the FQDN, for HTTPS purposes + if '.' not in host: + host = host + '.' + self.base_domain return requests.request( method, f'{self.scheme}://{host}:{self.ceod_port}{api_path}', diff --git a/ceo_common/model/RemoteMailmanService.py b/ceo_common/model/RemoteMailmanService.py index aed7f23..5618a91 100644 --- a/ceo_common/model/RemoteMailmanService.py +++ b/ceo_common/model/RemoteMailmanService.py @@ -8,7 +8,7 @@ from ..interfaces import IMailmanService, IConfig, IHTTPClient class RemoteMailmanService: def __init__(self): cfg = component.getUtility(IConfig) - self.mailman_host = cfg.get('mailman_host') + self.mailman_host = cfg.get('ceod_mailman_host') self.http_client = component.getUtility(IHTTPClient) def subscribe(self, address: str, mailing_list: str): diff --git a/ceo_common/test/__init__.py b/ceo_common/test/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/ceo_common/test/ceod_dev.ini b/ceo_common/test/ceod_dev.ini new file mode 100644 index 0000000..13a3aad --- /dev/null +++ b/ceo_common/test/ceod_dev.ini @@ -0,0 +1,45 @@ +[DEFAULT] +base_domain = csclub.internal + +[ceod] +# this is the host with the ceod/admin Kerberos key +admin_host = phosphoric-acid +# this is the host with NFS no_root_squash +fs_root_host = phosphoric-acid +mailman_host = mail +use_https = false +port = 9987 + +[ldap] +admin_principal = ceod/admin +server_url = ldap://ldap-master.csclub.internal +sasl_realm = CSCLUB.INTERNAL +users_base = ou=People,dc=csclub,dc=internal +groups_base = ou=Group,dc=csclub,dc=internal +sudo_base = ou=SUDOers,dc=csclub,dc=internal + +[uwldap] +server_url = ldap://uwldap.uwaterloo.ca +base = dc=uwaterloo,dc=ca + +[members] +min_id = 20001 +max_id = 29999 +home = /users +skel = /users/skel + +[clubs] +min_id = 30001 +max_id = 39999 +home = /users +skel = /users/skel + +[mail] +smtp_url = smtp://mail.csclub.internal +smtp_starttls = false + +[mailman3] +api_base_url = http://localhost:8001/3.1 +api_username = restadmin +api_password = mailman3 +new_member_list = csc-general diff --git a/ceod/api/app_factory.py b/ceod/api/app_factory.py index 9bbe40c..c94122a 100644 --- a/ceod/api/app_factory.py +++ b/ceod/api/app_factory.py @@ -1,9 +1,12 @@ +import importlib.resources +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 from ceo_common.interfaces import IConfig, IKerberosService, ILDAPService, IFileService, \ IMailmanService, IMailService, IUWLDAPService, IHTTPClient from ceo_common.model import Config, HTTPClient, RemoteMailmanService @@ -15,11 +18,12 @@ def create_app(flask_config={}): app = Flask(__name__) app.config.from_mapping(flask_config) - if app.config.get('TESTING') or app.config.get('ENV') == 'development': - # TODO: create test config class - cfg = Config() + if app.config.get('ENV') == 'development' and 'CEOD_CONFIG' not in os.environ: + with importlib.resources.path('ceo_common.test', 'ceod_dev.ini') as p: + config_file = p.__fspath__() else: - cfg = Config() + config_file = None + cfg = Config(config_file) component.provideUtility(cfg, IConfig) init_kerberos(app, service='ceod') @@ -43,13 +47,15 @@ def create_app(flask_config={}): http_client = HTTPClient() component.provideUtility(http_client, IHTTPClient) - # Only instantiate FileService is this host has NFS no_root_squash - if hostname == cfg.get('fs_root_host'): + # Only instantiate FileService if this host has NFS no_root_squash + # If admin_host and fs_root_host become separate, we will need + # to create a RemoteFileService + if hostname == cfg.get('ceod_fs_root_host'): file_srv = FileService() component.provideUtility(file_srv, IFileService) # Only offer mailman API if this host is running Mailman - if hostname == cfg.get('mailman_host'): + if hostname == cfg.get('ceod_mailman_host'): mailman_srv = MailmanService() component.provideUtility(mailman_srv, IMailmanService) @@ -68,6 +74,8 @@ def create_app(flask_config={}): from ceod.api import uwldap app.register_blueprint(uwldap.bp, url_prefix='/api/uwldap') + register_error_handlers(app) + @app.route('/ping') def ping(): """Health check""" diff --git a/ceod/api/error_handlers.py b/ceod/api/error_handlers.py new file mode 100644 index 0000000..69cfb09 --- /dev/null +++ b/ceod/api/error_handlers.py @@ -0,0 +1,23 @@ +import traceback + +from flask.app import Flask +from werkzeug.exceptions import HTTPException + +from ceo_common.logger_factory import logger_factory + +__all__ = ['register_error_handlers'] +logger = logger_factory(__name__) + + +def register_error_handlers(app: Flask): + """Register error handlers for the application.""" + app.register_error_handler(Exception, generic_error_handler) + + +def generic_error_handler(err: Exception): + """Return JSON for internal server errors.""" + # pass through HTTP errors + if isinstance(err, HTTPException): + return err + logger.error(traceback.format_exc()) + return {'error': type(err).__name__ + ': ' + str(err)}, 500 diff --git a/ceod/api/mailman.py b/ceod/api/mailman.py index 34c899f..3e26c3f 100644 --- a/ceod/api/mailman.py +++ b/ceod/api/mailman.py @@ -2,7 +2,8 @@ from flask import Blueprint from zope import component from .utils import authz_restrict_to_staff -from ceo_common.errors import UserAlreadySubscribedError, UserNotSubscribedError +from ceo_common.errors import UserAlreadySubscribedError, UserNotSubscribedError, \ + NoSuchListError from ceo_common.interfaces import IMailmanService bp = Blueprint('mailman', __name__) @@ -16,6 +17,8 @@ def subscribe(mailing_list, username): mailman_srv.subscribe(username, mailing_list) except UserAlreadySubscribedError as err: return {'error': str(err)}, 409 + except NoSuchListError as err: + return {'error': str(err)}, 404 return {'result': 'OK'} @@ -25,6 +28,6 @@ def unsubscribe(mailing_list, username): mailman_srv = component.getUtility(IMailmanService) try: mailman_srv.unsubscribe(username, mailing_list) - except UserNotSubscribedError as err: + except (UserNotSubscribedError, NoSuchListError) as err: return {'error': str(err)}, 404 return {'result': 'OK'} diff --git a/ceod/api/members.py b/ceod/api/members.py index de44f05..993d3b8 100644 --- a/ceod/api/members.py +++ b/ceod/api/members.py @@ -39,7 +39,8 @@ def get_user(auth_user: str, username: str): get_forwarding_addresses = True ldap_srv = component.getUtility(ILDAPService) try: - return ldap_srv.get_user(username).to_dict(get_forwarding_addresses) + user = ldap_srv.get_user(username) + return user.to_dict(get_forwarding_addresses) except UserNotFoundError: return { 'error': 'user not found' diff --git a/ceod/model/FileService.py b/ceod/model/FileService.py index 84a6827..c5ffb6e 100644 --- a/ceod/model/FileService.py +++ b/ceod/model/FileService.py @@ -13,8 +13,8 @@ from ceo_common.interfaces import IFileService, IConfig, IUser class FileService: def __init__(self): cfg = component.getUtility(IConfig) - self.member_home_skel = cfg.get('member_home_skel') - self.club_home_skel = cfg.get('club_home_skel') + self.member_home_skel = cfg.get('members_skel') + self.club_home_skel = cfg.get('clubs_skel') def create_home_dir(self, user: IUser): if user.is_club(): diff --git a/ceod/model/LDAPService.py b/ceod/model/LDAPService.py index c9c0fe7..d574492 100644 --- a/ceod/model/LDAPService.py +++ b/ceod/model/LDAPService.py @@ -8,7 +8,8 @@ import ldap.modlist from zope import component from zope.interface import implementer -from ceo_common.errors import UserNotFoundError, GroupNotFoundError +from ceo_common.errors import UserNotFoundError, GroupNotFoundError, \ + UserAlreadyExistsError, GroupAlreadyExistsError from ceo_common.interfaces import ILDAPService, IKerberosService, IConfig, \ IUser, IGroup, IUWLDAPService from .User import User @@ -25,10 +26,10 @@ class LDAPService: self.ldap_server_url = cfg.get('ldap_server_url') self.ldap_users_base = cfg.get('ldap_users_base') self.ldap_groups_base = cfg.get('ldap_groups_base') - self.member_min_id = cfg.get('member_min_id') - self.member_max_id = cfg.get('member_max_id') - self.club_min_id = cfg.get('club_min_id') - self.club_max_id = cfg.get('club_max_id') + self.member_min_id = cfg.get('members_min_id') + self.member_max_id = cfg.get('members_max_id') + self.club_min_id = cfg.get('clubs_min_id') + self.club_max_id = cfg.get('clubs_max_id') def _get_ldap_conn(self, gssapi_bind: bool = True) -> ldap.ldapobject.LDAPObject: # TODO: cache the connection @@ -126,8 +127,10 @@ class LDAPService: new_user.gid_number = uid_number modlist = ldap.modlist.addModlist(new_user.serialize_for_ldap()) - conn.add_s(new_user.dn, modlist) - + try: + conn.add_s(new_user.dn, modlist) + except ldap.ALREADY_EXISTS: + raise UserAlreadyExistsError() return new_user def modify_user(self, old_user: IUser, new_user: IUser): @@ -147,7 +150,10 @@ class LDAPService: # make sure that the caller initialized the GID number assert group.gid_number modlist = ldap.modlist.addModlist(group.serialize_for_ldap()) - conn.add_s(group.dn, modlist) + try: + conn.add_s(group.dn, modlist) + except ldap.ALREADY_EXISTS: + raise GroupAlreadyExistsError() return group def modify_group(self, old_group: IGroup, new_group: IGroup): diff --git a/ceod/model/MailService.py b/ceod/model/MailService.py index eba1918..fefcffe 100644 --- a/ceod/model/MailService.py +++ b/ceod/model/MailService.py @@ -17,14 +17,14 @@ smtp_url_re = re.compile(r'^(?Psmtps?)://(?P[\w.-]+)(:(?P\d+ class MailService: def __init__(self): cfg = component.getUtility(IConfig) - smtp_url = cfg.get('smtp_url') + smtp_url = cfg.get('mail_smtp_url') match = smtp_url_re.match(smtp_url) if match is None: raise Exception('Invalid SMTP URL: %s' % smtp_url) self.smtps = match.group('scheme') == 'smtps' self.host = match.group('host') self.port = int(match.group('port') or 25) - self.starttls = cfg.get('smtp_starttls') + self.starttls = cfg.get('mail_smtp_starttls') assert not (self.smtps and self.starttls) self.base_domain = cfg.get('base_domain') self.jinja_env = jinja2.Environment( diff --git a/ceod/model/MailmanService.py b/ceod/model/MailmanService.py index b604f6a..9efdac6 100644 --- a/ceod/model/MailmanService.py +++ b/ceod/model/MailmanService.py @@ -3,7 +3,8 @@ from requests.auth import HTTPBasicAuth from zope import component from zope.interface import implementer -from ceo_common.errors import UserAlreadySubscribedError, UserNotSubscribedError +from ceo_common.errors import UserAlreadySubscribedError, UserNotSubscribedError, \ + NoSuchListError from ceo_common.interfaces import IMailmanService, IConfig @@ -13,8 +14,9 @@ class MailmanService: cfg = component.getUtility(IConfig) self.base_domain = cfg.get('base_domain') self.api_base_url = cfg.get('mailman3_api_base_url') - self.api_username = cfg.get('mailman3_api_username') - self.api_password = cfg.get('mailman3_api_password') + api_username = cfg.get('mailman3_api_username') + api_password = cfg.get('mailman3_api_password') + self.basic_auth = HTTPBasicAuth(api_username, api_password) def subscribe(self, address: str, mailing_list: str): if '@' in mailing_list: @@ -31,11 +33,15 @@ class MailmanService: 'pre_confirmed': 'True', 'pre_approved': 'True', }, - auth=HTTPBasicAuth(self.api_username, self.api_password), + auth=self.basic_auth, ) - if resp.status_code == 409: - raise UserAlreadySubscribedError(resp.json()['description']) - resp.raise_for_status() + if not resp.ok: + desc = resp.json().get('description') + if resp.status_code == 409: + raise UserAlreadySubscribedError() + elif resp.status_code == 400 and desc == 'No such list': + raise NoSuchListError() + raise Exception(desc) def unsubscribe(self, address: str, mailing_list: str): if '@' not in mailing_list: @@ -49,8 +55,12 @@ class MailmanService: 'pre_approved': 'True', 'pre_confirmed': 'True', }, - auth=HTTPBasicAuth(self.api_username, self.api_password), + auth=self.basic_auth, ) - if resp.status_code == 404: - raise UserNotSubscribedError('user is not subscribed') - resp.raise_for_status() + if not resp.ok: + desc = resp.json().get('description') + if resp.status_code == 404: + # Unfortunately, a 404 here could mean either the list doesn't + # exist, or the member isn't subscribed + raise UserNotSubscribedError() + raise Exception(desc) diff --git a/ceod/model/User.py b/ceod/model/User.py index 5fb5ab8..05ffcc0 100644 --- a/ceod/model/User.py +++ b/ceod/model/User.py @@ -41,9 +41,9 @@ class User: self.gid_number = gid_number if home_directory is None: if is_club: - home_parent = cfg.get('member_home') + home_parent = cfg.get('members_home') else: - home_parent = cfg.get('club_home') + home_parent = cfg.get('clubs_home') self.home_directory = os.path.join(home_parent, uid) else: self.home_directory = home_directory