(#101) Validate usernames across cli & tui #115
|
@ -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:
|
||||
|
|
|
@ -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
|
||||
|
||||
|
|
13
ceo/utils.py
13
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
|
||||
|
|
|
@ -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()
|
||||
|
||||
o32patel marked this conversation as resolved
|
||||
|
||||
@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)
|
||||
|
|
|
@ -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:
|
||||
|
|
|
@ -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
|
|
@ -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)
|
|
@ -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):
|
||||
o32patel
commented
test_api_create_user_without_valid_username should probably be expanded on with the _ fix mentioned here: #115 (comment) test_api_create_user_without_valid_username should probably be expanded on with the _ fix mentioned here: https://git.csclub.uwaterloo.ca/public/pyceo/pulls/115#issuecomment-14170
|
||||
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']
|
||||
|
|
Loading…
Reference in New Issue
This can be simplified with Python's dataclass decorator: