From de232964138758cc2e21ee433ebac9e3e0a17b67 Mon Sep 17 00:00:00 2001 From: o32patel Date: Mon, 22 Jan 2024 13:15:40 -0500 Subject: [PATCH] Validate usernames across tui & for create_user on cli/api (#115) Current changes should address issues raised by @merenber in #114 excluding https://git.csclub.uwaterloo.ca/public/pyceo/pulls/114#issuecomment-14153 (both CLI and TUI validation) * Unit test for invalid name was added but needs to be modified as regex should be changed to disallow underscores eventually. Reviewed-on: https://git.csclub.uwaterloo.ca/public/pyceo/pulls/115 Reviewed-by: Max Erenberg Co-authored-by: o32patel Co-committed-by: o32patel --- ceo/cli/members.py | 7 +++++++ ceo/tui/controllers/Controller.py | 6 +++--- ceo/utils.py | 13 +----------- ceo_common/utils.py | 19 +++++++++++++++++ ceod/api/members.py | 6 ++++++ tests/ceo/utils.py | 19 ----------------- tests/ceo_common/test_utils.py | 19 +++++++++++++++++ tests/ceod/api/test_members.py | 34 +++++++++++++++++++++++++++++++ 8 files changed, 89 insertions(+), 34 deletions(-) delete mode 100644 tests/ceo/utils.py create mode 100644 tests/ceo_common/test_utils.py diff --git a/ceo/cli/members.py b/ceo/cli/members.py index d895a69..fd1a515 100644 --- a/ceo/cli/members.py +++ b/ceo/cli/members.py @@ -3,6 +3,8 @@ from typing import Dict import click from zope import component +from ceo_common.utils import validate_username + from ..term_utils import get_terms_for_renewal_for_user from ..utils import http_post, http_get, http_patch, http_delete, \ @@ -37,6 +39,11 @@ def add(username, cn, given_name, sn, program, num_terms, clubrep, forwarding_ad cfg = component.getUtility(IConfig) uw_domain = cfg.get('uw_domain') + # Verify that the username is valid before requesting data from UWLDAP + username_validator = validate_username(username) + if not username_validator.is_valid: + return click.echo("The provided username is invalid") + # Try to get info from UWLDAP resp = http_get('/api/uwldap/' + username) if resp.ok: diff --git a/ceo/tui/controllers/Controller.py b/ceo/tui/controllers/Controller.py index c9f389b..f1e1040 100644 --- a/ceo/tui/controllers/Controller.py +++ b/ceo/tui/controllers/Controller.py @@ -1,7 +1,7 @@ from abc import ABC import ceo.tui.utils as utils -from ...utils import validate_username +from ceo_common.utils import validate_username # NOTE: one controller can control multiple views, @@ -54,8 +54,8 @@ class Controller(ABC): username = self.view.username_edit.edit_text # TODO: share validation logic between CLI and TUI verification_res = validate_username(username) - if verification_res: - self.view.popup(verification_res) + if not verification_res.is_valid: + self.view.popup(verification_res.error_message) raise Controller.InvalidInput() return username diff --git a/ceo/utils.py b/ceo/utils.py index 3106513..b053f7d 100644 --- a/ceo/utils.py +++ b/ceo/utils.py @@ -1,10 +1,9 @@ import functools import json import os -from typing import List, Dict, Tuple, Callable, Union +from typing import List, Dict, Tuple, Callable import requests -import re from zope import component from .StreamResponseHandler import StreamResponseHandler @@ -12,8 +11,6 @@ from ceo_common.interfaces import IHTTPClient, IConfig from ceo_common.model import Term from ceod.transactions.members import AddMemberTransaction -VALID_USERNAME_RE = re.compile(r"^[a-z0-9]+[a-z0-9-]+$") - def http_request(method: str, path: str, **kwargs) -> requests.Response: client = component.getUtility(IHTTPClient) @@ -269,11 +266,3 @@ From other CSC machines you can connect using return True except PermissionError: return False - - -def validate_username(username: str) -> Union[str, None]: - if not username: - return 'Username must not be empty' - if not VALID_USERNAME_RE.fullmatch(username): - return 'Username is invalid' - return None diff --git a/ceo_common/utils.py b/ceo_common/utils.py index 0e4dfc1..f126e56 100644 --- a/ceo_common/utils.py +++ b/ceo_common/utils.py @@ -1,4 +1,9 @@ import datetime +import re +from dataclasses import dataclass + +# TODO: disallow underscores. Will break many tests with usernames that include _ +VALID_USERNAME_RE = re.compile(r"^[a-z][a-z0-9-_]+$") class fuzzy_result: @@ -51,3 +56,17 @@ def get_current_datetime() -> datetime.datetime: # We place this in a separate function so that we can mock it out # in our unit tests. return datetime.datetime.now() + + +@dataclass +class UsernameValidationResult: + is_valid: bool + error_message: str = '' + + +def validate_username(username: str) -> UsernameValidationResult: + if not username: + return UsernameValidationResult(False, 'Username must not be empty') + if not VALID_USERNAME_RE.fullmatch(username): + return UsernameValidationResult(False, 'Username is invalid') + return UsernameValidationResult(True) diff --git a/ceod/api/members.py b/ceod/api/members.py index 0d22ab6..29ba20d 100644 --- a/ceod/api/members.py +++ b/ceod/api/members.py @@ -9,6 +9,7 @@ from ceo_common.errors import BadRequest, UserAlreadySubscribedError, UserNotSub from ceo_common.interfaces import ILDAPService, IConfig, IMailService from ceo_common.logger_factory import logger_factory from ceo_common.model.Term import get_terms_for_new_user, get_terms_for_renewal +from ceo_common.utils import validate_username from ceod.transactions.members import ( AddMemberTransaction, ModifyMemberTransaction, @@ -30,6 +31,7 @@ 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') if type(terms) is int: @@ -42,6 +44,10 @@ def create_user(): if type(body['forwarding_addresses']) is not list: raise BadRequest('forwarding_addresses must be a list of email addresses') + uid_validator = validate_username(body['uid']) + if not uid_validator.is_valid: + raise BadRequest("Attribute 'uid' is missing or invalid") + if terms: logger.info(f"Creating member {body['uid']} for terms {terms}") else: diff --git a/tests/ceo/utils.py b/tests/ceo/utils.py deleted file mode 100644 index de503dd..0000000 --- a/tests/ceo/utils.py +++ /dev/null @@ -1,19 +0,0 @@ -import ceo.utils as utils - - -def test_validate_username(): - assert utils.validate_username('') == 'Username must not be empty' - assert utils.validate_username('-failure') == 'Username is invalid' - assert utils.validate_username('35 - joe') == 'Username is invalid' - assert utils.validate_username('35 -joe') == 'Username is invalid' - assert utils.validate_username('35- joe') == 'Username is invalid' - assert utils.validate_username('35$joe') == 'Username is invalid' - assert utils.validate_username(' 35joe') == 'Username is invalid' - assert utils.validate_username('35 joe') == 'Username is invalid' - assert utils.validate_username('35joe ') == 'Username is invalid' - assert utils.validate_username('joe!') == 'Username is invalid' - assert utils.validate_username('e45jong') is None - assert utils.validate_username('joe-35') is None - assert utils.validate_username('joe35-') is None - assert utils.validate_username('35joe-') is None - assert utils.validate_username('35-joe') is None diff --git a/tests/ceo_common/test_utils.py b/tests/ceo_common/test_utils.py new file mode 100644 index 0000000..2c3b685 --- /dev/null +++ b/tests/ceo_common/test_utils.py @@ -0,0 +1,19 @@ +import ceo_common.utils as utils + + +def test_validate_username(): + assert utils.validate_username('') == utils.UsernameValidationResult(False, 'Username must not be empty') + assert utils.validate_username('-failure') == utils.UsernameValidationResult(False, 'Username is invalid') + assert utils.validate_username('35 - joe') == utils.UsernameValidationResult(False, 'Username is invalid') + assert utils.validate_username('35 -joe') == utils.UsernameValidationResult(False, 'Username is invalid') + assert utils.validate_username('35- joe') == utils.UsernameValidationResult(False, 'Username is invalid') + assert utils.validate_username('35joe-') == utils.UsernameValidationResult(False, 'Username is invalid') + assert utils.validate_username('35$joe') == utils.UsernameValidationResult(False, 'Username is invalid') + assert utils.validate_username('35-joe') == utils.UsernameValidationResult(False, 'Username is invalid') + assert utils.validate_username(' 35joe') == utils.UsernameValidationResult(False, 'Username is invalid') + assert utils.validate_username('35 joe') == utils.UsernameValidationResult(False, 'Username is invalid') + assert utils.validate_username('35joe ') == utils.UsernameValidationResult(False, 'Username is invalid') + assert utils.validate_username('joe!') == utils.UsernameValidationResult(False, 'Username is invalid') + assert utils.validate_username('e45jong') == utils.UsernameValidationResult(True) + assert utils.validate_username('joe-35') == utils.UsernameValidationResult(True) + assert utils.validate_username('joe35-') == utils.UsernameValidationResult(True) diff --git a/tests/ceod/api/test_members.py b/tests/ceod/api/test_members.py index 4411e66..a8bb176 100644 --- a/tests/ceod/api/test_members.py +++ b/tests/ceod/api/test_members.py @@ -135,6 +135,40 @@ def test_api_create_user_without_forwarding_addresses(cfg, client): assert data['error'] == "BadRequest: Attribute 'forwarding_addresses' is missing or empty" +def test_api_create_user_without_valid_username(cfg, client): + status, data = client.post('/api/members', json={ + 'uid': '4_test', + 'cn': 'Test Four', + 'given_name': 'Test', + 'sn': 'Four', + 'program': 'Math', + 'terms': ['w2024'], + 'forwarding_addresses': ['test4@uwaterloo.internal'], + }) + try: + assert status == 400 + assert data['error'] == "BadRequest: Attribute 'uid' is missing or invalid" + finally: + client.delete('/api/members/4_test') + + +def test_api_create_user_with_valid_username(cfg, client): + status, data = client.post('/api/members', json={ + 'uid': 'test-4', + 'cn': 'Test Four', + 'given_name': 'Test', + 'sn': 'Four', + 'program': 'Math', + 'terms': ['w2024'], + 'forwarding_addresses': ['test4@uwaterloo.internal'], + }) + try: + assert status == 200 + assert data[-1]['status'] == 'completed' + finally: + client.delete('/api/members/test-4') + + def test_api_get_user(cfg, client, create_user_result): old_data = create_user_result.copy() uid = old_data['uid']