fix username valid / invalid filtering (#71)

* only ascii lowercase + numbers valid for new users
* add tests for valid/invalid usernames

dependencies-from-Dockerfile.log

@@ -1,30 +1,30 @@
-aio-pika==9.3.0
+aio-pika==9.3.1
 aiormq==6.7.7
 cachetools==5.3.2
 certifi==2023.11.17
 cffi==1.16.0
 charset-normalizer==3.3.2
-cryptography==41.0.5
+cryptography==41.0.7
 dnspython==2.4.2
-idna==3.4
+idna==3.6
 ldap3==2.9.1
 motor==3.3.2
 multidict==6.0.4
 pamqp==3.2.1
-pyasn1==0.5.0
+pyasn1==0.5.1
 pycparser==2.21
 PyJWT==2.8.0
-pymongo==4.6.0
+pymongo==4.6.1
 pypng==0.20220715.0
 qrcode==7.4.2
 requests==2.31.0
 requests-futures==1.0.1
-tornado==6.3.3
-typing_extensions==4.8.0
+tornado==6.4
+typing_extensions==4.9.0
 Unidecode==1.3.7
 urllib3==2.1.0
 -e /app
-wipac-dev-tools==1.7.1
-wipac-keycloak-rest-services==1.4.49
+wipac-dev-tools==1.8.2
+wipac-keycloak-rest-services==1.4.51
 wipac-rest-tools==1.6.0
-yarl==1.9.2
+yarl==1.9.4

tests/test_api_users.py

@@ -13,10 +13,6 @@
 import user_mgmt.users
 
 
-def test_invalid_usernames():
-    assert user_mgmt.users.Username._username_valid('foo-bar')
-    assert not user_mgmt.users.Username._username_valid('foo-bār')
-
 
 def test_invalid_orcid():
     assert user_mgmt.users.is_orcid('0001-0002-0003-0004')
@@ -179,39 +175,77 @@ async def test_username_select(server, reg_token_client):
     with pytest.raises(Exception):
         await client.request('POST', '/api/username', args)
 
+
+invalid_usernames = [
+    'foo',  # too short
+    'fooooooooooooooooooooooooo',  # too long
+    'foò',  # unicode
+    'fo=o',  # invalid char
+    'fo o',  # space
+    'f\'oo',  # quote
+    'f-oo',  # dash
+    'f.oo',  # dot
+    'f_oo',  # underscore
+    'Foo',  # uppercase
+    'foO',  # uppercase
+]
+# put is less strict
+valid_usernames_put = [
+    'foo',  # too short
+    'fooooooooooooooooooooooooo',  # too long
+    'f-oo',  # dash
+    'f.oo',  # dot
+    'f_oo',  # underscore
+# can't test these last two without experimental Keycloak support
+#    'Foo',  # uppercase
+#    'foO',  # uppercase
+]
+invalid_usernames_put = [
+    'foò',  # unicode
+    'fo=o',  # invalid char
+    'fo o',  # space
+    'f\'oo',  # quote
+]
+
+@pytest.mark.parametrize('username', valid_usernames_put)
 @pytest.mark.asyncio
-async def test_username_invalid(server, reg_token_client, monkeypatch):
+async def test_user_put_valid(username, server):
     rest, krs_client, *_ = server
-    client = await reg_token_client()
+    client = await rest(username)
 
-    # short
-    args = {
-        'first_name': 'Foo',
-        'last_name': 'Bar',
-        'username': 'foo'
-    }
-    with pytest.raises(Exception):
-        await client.request('POST', '/api/username', args)
+    await client.request('PUT', f'/api/users/{username}', {'author_name': 'F. Bar', 'author_email': 'foo@bar'})
+
+
+@pytest.mark.parametrize('username', invalid_usernames_put)
+@pytest.mark.asyncio
+async def test_user_put_invalid(username, server):
+    rest, krs_client, *_ = server
+    client = await rest(username)
 
-    # long
-    args = {
-        'first_name': 'Foo',
-        'last_name': 'Bar',
-        'username': 'fooooooooooooooooooooooooo'
-    }
     with pytest.raises(Exception):
-        await client.request('POST', '/api/username', args)
+        await client.request('PUT', f'/api/users/{username}', {'author_name': 'F. Bar', 'author_email': 'foo@bar'})
+
+
+@pytest.mark.parametrize('username', invalid_usernames)
+def test_invalid_usernames(username):
+    assert not user_mgmt.users.Username._username_valid(username)
+
+@pytest.mark.parametrize('username', invalid_usernames)
+@pytest.mark.asyncio
+async def test_username_invalid(username, server, reg_token_client, monkeypatch):
+    rest, krs_client, *_ = server
+    client = await reg_token_client()
 
-    # non-ascii
     args = {
         'first_name': 'Foo',
         'last_name': 'Bar',
-        'username': 'foò'
+        'username': username
     }
     with pytest.raises(Exception):
         await client.request('POST', '/api/username', args)
 
-    # bad word
+@pytest.mark.asyncio
+async def test_username_invalid_bad_word(server, reg_token_client, monkeypatch):
     monkeypatch.setattr(user_mgmt.users, 'BAD_WORDS', ['bad'])
 
     args = {

user_mgmt/server.py

@@ -96,7 +96,7 @@ def create_server():
     server.add_route(r'/api/experiments/(?P<experiment>[\w\-]+)/institutions', MultiInstitutions, kwargs)
     server.add_route(r'/api/experiments/(?P<experiment>[\w\-]+)/institutions/(?P<institution>[\w\-]+)', Institution, kwargs)
     server.add_route(r'/api/experiments/(?P<experiment>[\w\-]+)/institutions/(?P<institution>[\w\-]+)/users', InstitutionMultiUsers, kwargs)
-    server.add_route(r'/api/experiments/(?P<experiment>[\w\-]+)/institutions/(?P<institution>[\w\-]+)/users/(?P<username>[\w\-]+)', InstitutionUser, kwargs)
+    server.add_route(r'/api/experiments/(?P<experiment>[\w\-]+)/institutions/(?P<institution>[\w\-]+)/users/(?P<username>[\w\-\._]+)', InstitutionUser, kwargs)
 
     server.add_route('/api/inst_approvals', InstApprovals, kwargs)
     server.add_route(r'/api/inst_approvals/(?P<approval_id>\w+)/actions/approve', InstApprovalsActionApprove, kwargs)
@@ -112,7 +112,7 @@ def create_server():
     server.add_route(r'/api/group_approvals/(?P<approval_id>\w+)/actions/deny', GroupApprovalsActionDeny, kwargs)
 
     server.add_route(r'/api/users', MultiUser, kwargs)
-    server.add_route(r'/api/users/(?P<username>[\w\-]+)', User, kwargs)
+    server.add_route(r'/api/users/(?P<username>[\w\-\._]+)', User, kwargs)
     server.add_route('/api/username', Username, kwargs)
     server.add_route(r'/api/experiments/(?P<experiment>[\w\-]+)/associates', AssociateUsers, kwargs)
 

user_mgmt/static/routes/register.js

@@ -186,7 +186,7 @@ export default {
       <textinput name="Last Name" inputName="last_name" v-model.trim="lastName"
        required=true :valid="validLastName" :allValid="valid"></textinput>
       <textinput name="Username" inputName="username" v-model.trim="username"
-       required=true :valid="validUsername" :allValid="valid" helptext="Note: must be between 5-16 characters"></textinput>
+       required=true :valid="validUsername" :allValid="valid" helptext="Note: must be between 5-16 characters, ascii lowercase and numbers"></textinput>
       <textinput name="External Email Address" inputName="email" v-model.trim="email"
        required=true :valid="validEmail" :allValid="valid"></textinput>
       <div v-if="errMessage" class="error_box" v-html="errMessage"></div>

user_mgmt/users.py

@@ -2,8 +2,9 @@
 Handle user profile updates.
 """
 import itertools
-import os
 import logging
+import os
+import string
 
 from tornado.web import HTTPError
 from rest_tools.server import catch_error, authenticated
@@ -73,7 +74,7 @@ class Username(MyHandler):
     @staticmethod
     def _gen_username(first_name, last_name, number):
         """Make ascii username from first and last name."""
-        ret = unidecode.unidecode(first_name[0] + last_name).replace("'", '').replace(' ', '').lower()
+        ret = unidecode.unidecode(first_name[0] + last_name).replace("'", '').replace(' ', '').replace('.','').lower()
         if len(ret) < 5:
             ret = f'{ret:0<5s}'
         if len(ret) > 8:
@@ -84,9 +85,21 @@ def _gen_username(first_name, last_name, number):
 
     @staticmethod
     def _username_valid(username):
-        """Check if a username is valid - length, bad words."""
-        ascii_username = unidecode.unidecode(username).replace("'", '').replace(' ', '').lower()
-        if ascii_username != username:
+        """
+        Check if a username is valid.
+
+        Valid:
+        * ascii string between 5-15 chars
+        * lowercase letters, numbers
+
+        Invalid:
+        * unicode
+        * punctuation
+        * special chars
+        * BAD_WORDS filter
+        """
+        valid_chars = string.ascii_lowercase + string.digits
+        if any(c not in valid_chars for c in username):
             return False
         if len(username) < 5:
             return False
@@ -172,7 +185,7 @@ async def check_auth(self, username):
                     members = await self.group_cache.get_members(group_path)
                     if username in members:
                         return
-            logging.warning('failed inst admin check for admin %r and username %r', self.current_user, username)
+            logging.warning('failed inst admin check for admin %r and username %r', self.auth_data['username'], username)
             raise HTTPError(403, reason='invalid authorization')
 
     async def check_auth_read_only(self, username):
@@ -200,7 +213,7 @@ async def check_auth_read_only(self, username):
                 members = await self.group_cache.get_members(group_path)
                 if username in members:
                     return
-            logging.warning('failed group or inst admin check for admin %r and username %r', self.current_user, username)
+            logging.warning('failed group or inst admin check for admin %r and username %r', self.auth_data['username'], username)
             raise HTTPError(403, reason='invalid authorization')