Merge pull request #597 from Ouranosinc/lowercase-all-usernames

CHANGES.rst

@@ -9,7 +9,21 @@ Changes
 `Unreleased <https://github.com/Ouranosinc/Magpie/tree/master>`_ (latest)
 ------------------------------------------------------------------------------------
 
-* Nothing new for the moment.
+Bug Fixes
+~~~~~~~~~
+
+* Ensure that ``user_name`` values for all `User` are lowercase and do not contain whitespace.
+
+  Ziggurat foundations assumes that a `User` will not have a ``user_name`` that differs from another only in terms of
+  case. The simplest way to enforce this is to ensure that all ``user_name`` values are lowercase.
+  Previously, this was not enforced so we could create two `User` which could not be differentiated properly.
+
+  This change includes a database migration that will convert all ``user_name`` that contain uppercase characters
+  to lowercase. This may cause a database conflict if there are two ``user_name`` values that differ only in terms of
+  case. For example "Test" and "test". If this occurs, please manually update those ``user_name`` values to no longer
+  conflict and try the migration again.
+
+  This also prevents new users from being created that contain whitespace.
 
 .. _changes_3.37.1:
 

magpie/alembic/versions/2023-11-10_91af68bcdc67_case_insensitive_user_name_constraint.py

@@ -0,0 +1,53 @@
+"""
+Case Insensitive User_Name Constraint
+
+Revision ID: 91af68bcdc67
+Revises: 5e5acc33adce
+Create Date: 2023-11-10 15:58:23.068465
+"""
+import sqlalchemy as sa
+from alembic import op
+from sqlalchemy.orm.session import sessionmaker
+
+# Revision identifiers, used by Alembic.
+# pylint: disable=C0103,invalid-name  # revision control variables not uppercase
+revision = "91af68bcdc67"
+down_revision = "5e5acc33adce"
+branch_labels = None
+depends_on = None
+
+Session = sessionmaker()
+
+users = sa.table(
+    "users",
+    sa.column("id", sa.Integer),
+    sa.column("user_name", sa.String),
+)
+
+
+def upgrade():
+    # If users with conflicting user_names already exist, the following will fail. The conflicting user_names
+    # must be updated manually and then this migration script can be re-run.
+    try:
+        op.create_index(
+            "ix_users_user_name_unique_case_insensitive",
+            "users",
+            [sa.text("lower(user_name)")],
+            unique=True,
+        )
+    except sa.exc.IntegrityError as e:
+        raise Exception("{}\nPlease manually update conflicting user_names and try again".format(e)) from e
+    session = Session(bind=op.get_bind())
+    for user in session.execute(sa.select([users])):
+        lower_user_name = user.user_name.lower()
+        if user.user_name != lower_user_name:
+            print(
+                "Converting user_name from {} to {} (users.id == {})".format(user.user_name, lower_user_name, user.id)
+            )
+            session.execute(users.update().where(users.c.id == user.id).values(user_name=lower_user_name))
+    session.commit()
+
+
+def downgrade():
+    op.drop_index("ix_users_user_name_unique_case_insensitive", "users")
+    # Previous case information is lost and cannot be restored

magpie/api/management/user/user_utils.py

@@ -1,3 +1,4 @@
+import re
 from inspect import cleandoc
 from secrets import compare_digest  # noqa 'python2-secrets'
 from typing import TYPE_CHECKING
@@ -62,6 +63,8 @@
         UserServicesType
     )
 
+USERNAME_REGEX = re.compile(r"^[a-z0-9]+(?:[_\-\.][a-z0-9]+)*$")
+
 
 def create_user(user_name,              # type: Str
                 password,               # type: Optional[Str]
@@ -869,7 +872,7 @@ def check_user_info(user_name=None, email=None, password=None, group_name=None,
         ax.verify_param(user_name, not_none=True, not_empty=True, param_name="user_name",
                         http_error=HTTPBadRequest,
                         msg_on_fail=s.Users_CheckInfo_UserNameValue_BadRequestResponseSchema.description)
-        ax.verify_param(user_name, matches=True, param_name="user_name", param_compare=ax.PARAM_REGEX,
+        ax.verify_param(user_name, matches=True, param_name="user_name", param_compare=USERNAME_REGEX,
                         http_error=HTTPBadRequest,
                         msg_on_fail=s.Users_CheckInfo_UserNameValue_BadRequestResponseSchema.description)
         extra_regex = get_constant("MAGPIE_USER_NAME_EXTRA_REGEX", raise_missing=False, raise_not_set=False)

tests/interfaces.py

@@ -4549,6 +4549,40 @@ def test_PostUsers_Conflict_Name(self):
                                   headers=self.json_headers, cookies=self.cookies, expect_errors=True)
         utils.check_response_basic_info(resp, 409, expected_method="POST")
 
+    @runner.MAGPIE_TEST_USERS
+    def test_PostUsers_Invalid_Name_Uppercase(self):
+        """
+        Do not allow creation of users that have uppercase characters.
+
+        .. versionadded:: 3.37.2
+        """
+        utils.warn_version(self, "do not allow uppercase characters or spaces in user_names", "3.37.2", skip=True)
+        data = {
+            "user_name": self.test_user_name.upper(),
+            "password": self.test_user_name,
+            "email": "{}@mail.com".format(self.test_user_name),
+        }
+        resp = utils.test_request(self, "POST", "/users", data=data,
+                                  headers=self.json_headers, cookies=self.cookies, expect_errors=True)
+        utils.check_response_basic_info(resp, 400, expected_method="POST")
+
+    @runner.MAGPIE_TEST_USERS
+    def test_PostUsers_Invalid_Name_Whitespace(self):
+        """
+        Do not allow creation of users that have whitespace.
+
+        .. versionadded:: 3.37.2
+        """
+        utils.warn_version(self, "do not allow uppercase characters or spaces in user_names", "3.37.2", skip=True)
+        data = {
+            "user_name": "a {}".format(self.test_user_name),
+            "password": self.test_user_name,
+            "email": "{}@mail.com".format(self.test_user_name),
+        }
+        resp = utils.test_request(self, "POST", "/users", data=data,
+                                  headers=self.json_headers, cookies=self.cookies, expect_errors=True)
+        utils.check_response_basic_info(resp, 400, expected_method="POST")
+
     @runner.MAGPIE_TEST_USERS
     def test_PostUsers_Conflict_Email(self):
         utils.warn_version(self, "user creation with duplicate email conflict", "3.11.0", skip=True)
@@ -4708,6 +4742,36 @@ def test_UpdateUser_username_ReservedKeyword_LoggedUser(self):
                                   headers=self.json_headers, cookies=self.cookies)
         utils.check_response_basic_info(resp, 403, expected_method=self.update_method)
 
+    @runner.MAGPIE_TEST_USERS
+    def test_UpdateUser_username_Invalid_Uppercase(self):
+        """
+        Do not allow user_name that contains uppercase characters.
+
+        .. versionadded:: 3.37.2
+        """
+        utils.warn_version(self, "do not allow uppercase characters or spaces in user_names", "3.37.2", skip=True)
+        utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP"))
+        data = {"user_name": self.test_user_name.upper()}
+        path = "/users/{usr}".format(usr=self.test_user_name)
+        resp = utils.test_request(self, self.update_method, path, data=data, expect_errors=True,
+                                  headers=self.json_headers, cookies=self.cookies)
+        utils.check_response_basic_info(resp, 400, expected_method=self.update_method)
+
+    @runner.MAGPIE_TEST_USERS
+    def test_UpdateUser_username_Invalid_Whitespace(self):
+        """
+        Do not allow user_name that contains whitespace.
+
+        .. versionadded:: 3.37.2
+        """
+        utils.warn_version(self, "do not allow uppercase characters or spaces in user_names", "3.37.2", skip=True)
+        utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP"))
+        data = {"user_name": "a {}".format(self.test_user_name)}
+        path = "/users/{usr}".format(usr=self.test_user_name)
+        resp = utils.test_request(self, self.update_method, path, data=data, expect_errors=True,
+                                  headers=self.json_headers, cookies=self.cookies)
+        utils.check_response_basic_info(resp, 400, expected_method=self.update_method)
+
     @runner.MAGPIE_TEST_USERS
     def test_UpdateUser_email(self):
         utils.TestSetup.create_TestUser(self, override_group_name=get_constant("MAGPIE_ANONYMOUS_GROUP"))
@@ -7537,27 +7601,6 @@ def test_AddUser_FormSubmit_WithExtraUsernameRegex_Invalid(self):
             msg = s.Users_CheckInfo_UserNameValueExtraRegex_BadRequestResponseSchema.description
             utils.check_val_is_in(msg, html.unescape(body))
 
-    @runner.MAGPIE_TEST_USERS
-    def test_AddUser_FormSubmit_WithExtraUsernameRegex_CaseInvalid(self):
-        """
-        Check that the extra_user_name_regex setting is used to validate a new user name when the user name is
-        invalid according to that regex because the case is incorrect but is valid according to the ax.PARAM_REGEX.
-
-        .. versionchanged:: 3.37.1
-        """
-        utils.warn_version(self, "case sensitive user name extra regex", "3.37.1", skip=True)
-        with utils.mocked_get_settings(settings={"magpie.user_name_extra_regex": "^[a-z]+$"}):
-            data = {"user_name": "UpperCaseUserName", "group_name": get_constant("MAGPIE_USERS_GROUP"),
-                    "email": "{}@mail.com".format(self.test_user_name),
-                    "password": self.test_user_name, "confirm": self.test_user_name}
-            path = "/ui/users/add"
-            form = "add_user_form"
-            resp = utils.TestSetup.check_FormSubmit(self, form_match=form, form_submit="create", form_data=data,
-                                                    path=path)
-            body = utils.check_ui_response_basic_info(resp)
-            msg = s.Users_CheckInfo_UserNameValueExtraRegex_BadRequestResponseSchema.description
-            utils.check_val_is_in(msg, html.unescape(body))
-
     @runner.MAGPIE_TEST_USERS
     def test_AddUser_FormSubmit_WithExtraUsernameRegex_ValidGoodUsername(self):
         """
@@ -7608,6 +7651,42 @@ def test_AddUser_FormSubmit_WithExtraUsernameRegex_InvalidBadUsername(self):
             msg = s.Users_CheckInfo_UserNameValueExtraRegex_BadRequestResponseSchema.description
             utils.check_val_not_in(msg, html.unescape(body))
 
+    @runner.MAGPIE_TEST_USERS
+    def test_AddUser_FormSubmit_InvalidUsername_Uppercase(self):
+        """
+        Check that the form submit to create a user fails if the user name contains uppercase characters.
+
+        .. versionadded:: 3.37.2
+        """
+        utils.warn_version(self, "do not allow case-insensitive matching usernames", "3.37.2", skip=True)
+        data = {"user_name": self.test_user_name.upper(), "group_name": get_constant("MAGPIE_USERS_GROUP"),
+                "email": "{}@mail.com".format(self.test_user_name), "password": self.test_user_name,
+                "confirm": self.test_user_name}
+        path = "/ui/users/add"
+        form = "add_user_form"
+        resp = utils.TestSetup.check_FormSubmit(self, form_match=form, form_submit="create", form_data=data,
+                                                path=path)
+        body = utils.check_ui_response_basic_info(resp)
+        utils.check_val_is_in("Invalid 'user_name' value specified.", html.unescape(body))
+
+    @runner.MAGPIE_TEST_USERS
+    def test_AddUser_FormSubmit_InvalidUsername_Whitespace(self):
+        """
+        Check that the form submit to create a user fails if the user name contains whitespace characters.
+
+        .. versionadded:: 3.37.2
+        """
+        utils.warn_version(self, "do not allow case-insensitive matching usernames", "3.37.2", skip=True)
+        data = {"user_name": "a {}".format(self.test_user_name), "group_name": get_constant("MAGPIE_USERS_GROUP"),
+                "email": "{}@mail.com".format(self.test_user_name), "password": self.test_user_name,
+                "confirm": self.test_user_name}
+        path = "/ui/users/add"
+        form = "add_user_form"
+        resp = utils.TestSetup.check_FormSubmit(self, form_match=form, form_submit="create", form_data=data,
+                                                path=path)
+        body = utils.check_ui_response_basic_info(resp)
+        utils.check_val_is_in("Invalid 'user_name' value specified.", html.unescape(body))
+
     @runner.MAGPIE_TEST_STATUS
     def test_AddGroup_PageStatus(self):
         path = "/ui/groups/add"