Name validations enhancements

server/mergin/auth/forms.py

@@ -20,13 +20,22 @@
 
 
 def username_validation(form, field):
-    from ..sync.utils import is_name_allowed
+    from ..sync.utils import (
+        has_valid_characters,
+        has_valid_first_character,
+        is_valid_filename,
+        is_reserved_word,
+    )
 
-    if field.data and (not is_name_allowed(field.data) or "@" in field.data):
-        raise ValidationError(
-            f"Please don't start username with . and "
-            f"use only alphanumeric or these -._! characters in {field.name}."
-        )
+    errors = [
+        has_valid_characters(field.data),
+        has_valid_first_character(field.data),
+        is_reserved_word(field.data),
+        is_valid_filename(field.data),
+    ]
+    for error in errors:
+        if error:
+            raise ValidationError(error)
 
 
 class PasswordValidator:

server/mergin/sync/forms.py

@@ -6,6 +6,29 @@
 from wtforms.validators import DataRequired
 from flask_wtf import FlaskForm
 
+from mergin.sync.utils import (
+    is_reserved_word,
+    has_valid_characters,
+    is_valid_filename,
+    has_valid_first_character,
+)
+
+
+def project_name_validation(name: str) -> str | None:
+    """Check whether project name is valid"""
+    if not name.strip():
+        return "Project name cannot be empty"
+    errors = [
+        has_valid_characters(name),
+        has_valid_first_character(name),
+        is_reserved_word(name),
+        is_valid_filename(name),
+    ]
+    for error in errors:
+        if error:
+            return error
+    return
+
 
 class IntegerListField(Field):
     def _value(self):

server/mergin/sync/public_api_controller.py

@@ -34,6 +34,8 @@
 from sqlalchemy.orm import load_only
 from werkzeug.exceptions import HTTPException
 
+from mergin.sync.forms import project_name_validation
+
 from .interfaces import WorkspaceRole
 from ..app import db
 from ..auth import auth_required
@@ -83,7 +85,6 @@
     is_valid_uuid,
     gpkg_wkb_to_wkt,
     is_versioned_file,
-    is_name_allowed,
     get_project_path,
     get_device_id,
 )
@@ -179,10 +180,11 @@ def add_project(namespace):  # noqa: E501
     """
     request.json["name"] = request.json["name"].strip()
 
-    if not is_name_allowed(request.json["name"]):
+    validation_error = project_name_validation(request.json["name"])
+    if validation_error:
         abort(
             400,
-            "Please don't start project name with . and use only alphanumeric or these -._! characters in project name.",
+            validation_error,
         )
 
     if request.is_json:
@@ -1194,6 +1196,8 @@ def clone_project(namespace, project_name):  # noqa: E501
     dest_ns = request.json.get("namespace", cp_workspace_name).strip()
     dest_project = request.json.get("project", cloned_project.name).strip()
     ws = current_app.ws_handler.get_by_name(dest_ns)
+    if not dest_project:
+        abort(400, "Project name cannot be empty")
     if not ws:
         if dest_ns == current_user.username:
             abort(
@@ -1204,12 +1208,9 @@ def clone_project(namespace, project_name):  # noqa: E501
             abort(404, "Workspace does not exist")
     if not ws.user_has_permissions(current_user, "admin"):
         abort(403, "You do not have permissions for this workspace")
-
-    if not is_name_allowed(dest_project):
-        abort(
-            400,
-            "Please don't start project name with . and use only alphanumeric or these -._! characters in project name.",
-        )
+    validation = project_name_validation(dest_project)
+    if validation:
+        abort(400, validation)
 
     _project = Project.query.filter_by(name=dest_project, workspace_id=ws.id).first()
     if _project:

server/mergin/sync/public_api_v2.yaml

@@ -66,8 +66,6 @@ paths:
                     example: InvalidProjectName
                   detail:
                     type: string
-                    enum:
-                      - "Entered project name is invalid"
                     example: "Entered project name is invalid"
         "401":
           $ref: "#/components/responses/Unauthorized"
@@ -267,7 +265,7 @@ components:
       example: writer
     Role:
       allOf:
-        - $ref: '#/components/schemas/ProjectRole'
+        - $ref: "#/components/schemas/ProjectRole"
           nullable: false
           description: combination of workspace role and project role
     ProjectMember:
@@ -284,8 +282,8 @@ components:
           format: email
           example: john.doe@example.com
         workspace_role:
-          $ref: '#/components/schemas/WorkspaceRole'
+          $ref: "#/components/schemas/WorkspaceRole"
         project_role:
-          $ref: '#/components/schemas/ProjectRole'
+          $ref: "#/components/schemas/ProjectRole"
         role:
-          $ref: '#/components/schemas/Role'
+          $ref: "#/components/schemas/Role"

server/mergin/sync/public_api_v2_controller.py

@@ -7,6 +7,8 @@
 from flask import abort, jsonify
 from flask_login import current_user
 
+from mergin.sync.forms import project_name_validation
+
 from .schemas import ProjectMemberSchema
 from .workspace import WorkspaceRole
 from ..app import db
@@ -15,7 +17,6 @@
 from .models import Project, ProjectRole, ProjectMember
 from .permissions import ProjectPermissions, require_project_by_uuid
 from .private_api_controller import project_access_granted
-from .utils import is_name_allowed
 
 
 @auth_required
@@ -45,13 +46,11 @@ def delete_project_now(id):
 def update_project(id):
     """Rename project"""
     project = require_project_by_uuid(id, ProjectPermissions.Update)
-    new_name = request.json["name"]
-
-    if not is_name_allowed(new_name):
+    new_name = request.json["name"].strip()
+    validation_error = project_name_validation(new_name)
+    if validation_error:
         return (
-            jsonify(
-                code="InvalidProjectName", detail="Entered project name is invalid"
-            ),
+            jsonify(code="InvalidProjectName", detail=validation_error),
             400,
         )
     new_name_exists = Project.query.filter_by(

server/mergin/sync/utils.py

@@ -15,6 +15,7 @@
 from flask import Request
 from typing import Optional
 from sqlalchemy import text
+from pathvalidate import validate_filename, ValidationError
 
 
 def generate_checksum(file, chunk_size=4096):
@@ -261,22 +262,36 @@ def convert_byte(size_bytes, unit):
     return size_bytes
 
 
-def is_name_allowed(string):
-    """Check if string is just has whitelisted character
+def is_reserved_word(name: str) -> str | None:
+    """Check if name is reserved in system"""
+    reserved = r"^support$|^helpdesk$|^merginmaps$|^lutraconsulting$|^mergin$|^lutra$|^input$|^admin$|^sales$"
+    if re.match(reserved, name) is not None:
+        return "The provided value is invalid."
+    return None
 
-    :param string: string to be checked.
-    :type string: str
 
-    :return: boolean of has just whitelisted character
-    :rtype: bool
-    """
-    return (
-        re.match(
-            r".*[\@\#\$\%\^\&\*\(\)\{\}\[\]\?\'\"`,;\:\+\=\~\\\/\|\<\>].*|^[\s^\.].*$|^CON$|^PRN$|^AUX$|^NUL$|^COM\d$|^LPT\d|^support$|^helpdesk$|^merginmaps$|^lutraconsulting$|^mergin$|^lutra$|^input$|^admin$|^sales$|^$",
-            string,
-        )
-        is None
-    )
+def has_valid_characters(name: str) -> str | None:
+    """Check if name contains only valid characters"""
+    if re.match(r"^[\w\s\-\.]+$", name) is None:
+        return "Please use only alphanumeric or the following -_. characters."
+    return None
+
+
+def has_valid_first_character(name: str) -> str | None:
+    """Check if name contains only valid characters in first position"""
+    if re.match(r"^[\s^\.].*$", name) is not None:
+        return f"Value can not start with space or dot."
+    return None
+
+
+def is_valid_filename(name: str) -> str | None:
+    """Check if name contains only valid characters for filename"""
+    error = None
+    try:
+        validate_filename(name)
+    except ValidationError:
+        error = "The provided value is invalid."
+    return error
 
 
 def workspace_names(workspaces):

server/mergin/tests/test_project_controller.py

@@ -386,6 +386,7 @@ def test_get_projects_by_names(client):
     ({"name": "foo/bar", "template": test_project}, 400),  # invalid project name
     ({"name": "ba%r", "template": test_project}, 400),  # invalid project name
     ({"name": "bar*", "template": test_project}, 400),  # invalid project name
+    ({"name": "  ", "template": test_project}, 400),  # empty
     ({"name": "support", "template": test_project}, 400),  # forbidden project name
     ({"name": test_project}, 409),
 ]

server/mergin/tests/test_public_api_v2.py

@@ -64,7 +64,7 @@ def test_rename_project(client):
     assert response.status_code == 400
     assert response.json["code"] == "InvalidProjectName"
     response = client.patch(
-        f"v2/projects/{project.id}", json={"name": " new_project_name"}
+        f"v2/projects/{project.id}", json={"name": ".new_project_name"}
     )
     assert response.status_code == 400
     assert response.json["code"] == "InvalidProjectName"

server/mergin/tests/test_utils.py

@@ -11,7 +11,14 @@
 from unittest.mock import MagicMock
 
 from ..app import db
-from ..sync.utils import parse_gpkgb_header_size, gpkg_wkb_to_wkt, is_name_allowed
+from ..sync.utils import (
+    parse_gpkgb_header_size,
+    gpkg_wkb_to_wkt,
+    is_reserved_word,
+    has_valid_characters,
+    has_valid_first_character,
+    is_valid_filename,
+)
 from ..auth.models import LoginHistory, User
 from . import json_headers
 from .utils import login
@@ -137,13 +144,15 @@ def test_is_name_allowed():
         ("Pro123ject", True),
         ("123PROJECT", True),
         ("PROJECT", True),
-        ("project ", True),
+        # Not valid filename
+        ("project ", False),
         ("pro ject", True),
         ("proj-ect", True),
         ("-project", True),
         ("proj_ect", True),
         ("proj.ect", True),
-        ("proj!ect", True),
+        # We are repmoving ! from valids
+        ("proj!ect", False),
         (" project", False),
         (".project", False),
         ("proj~ect", False),
@@ -182,14 +191,15 @@ def test_is_name_allowed():
         ("NUL", False),
         ("NULL", True),
         ("PRN", False),
-        ("LPT0", False),
+        # is not reserved word
+        ("LPT0", True),
         ("lpt0", True),
         ("LPT1", False),
-        ("lpt1", True),
+        ("lpt1", False),
         ("COM1", False),
-        ("com1", True),
+        ("com1", False),
         ("AUX", False),
-        ("AuX", True),
+        ("AuX", False),
         ("projAUXect", True),
         ("CONproject", True),
         ("projectCON", True),
@@ -204,7 +214,19 @@ def test_is_name_allowed():
         ("sales", False),
         ("", False),
         ("    ", False),
+        ("😄guy", False),
+        ("会社", True),
     ]
 
     for t in test_cases:
-        assert is_name_allowed(t[0]) == t[1]
+        name = t[0]
+        expected = t[1]
+        assert (
+            not (
+                has_valid_characters(name)
+                and has_valid_first_character(name)
+                and is_valid_filename(name)
+                and is_reserved_word(name)
+            )
+            == expected
+        )