Validate usernames across tui & for create_user on cli/api (#115)

Current changes should address issues raised by @merenber in #114 excluding #114 (comment) (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: #115
Reviewed-by: Max Erenberg <merenber@csclub.uwaterloo.ca>
Co-authored-by: o32patel <ohm.patel@uwaterloo.ca>
Co-committed-by: o32patel <ohm.patel@uwaterloo.ca>

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:

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
 

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

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)

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:

tests/ceo/utils.py

@@ -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

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)

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']