Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enforce valid email address for User email #308

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions server/mergin/auth/controller.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,6 @@
ApiLoginForm,
)
from .. import db
from ..app import DEPRECATION_API_MSG
from ..utils import format_time_delta


# public endpoints
Expand Down
6 changes: 6 additions & 0 deletions server/mergin/auth/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

from .models import User
from ..app import UpdateForm, CustomStringField
from .utils import is_email_address_valid


def username_validation(form, field):
Expand Down Expand Up @@ -64,6 +65,11 @@ class RegisterUserForm(FlaskForm):
def validate(self):
if not super().validate():
return False
if not is_email_address_valid(self.email.data):
self.email.errors.append(
"Email address contains non-ASCII characters. Only ASCII emails are allowed."
)
return False

user = User.query.filter(
(func.lower(User.username) == func.lower(self.username.data.strip()))
Expand Down
6 changes: 5 additions & 1 deletion server/mergin/auth/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from typing import List, Optional
import bcrypt
from flask import current_app, request
from sqlalchemy import or_, func
from sqlalchemy import or_, func, CheckConstraint
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need CheckConstraint?


from .. import db
from ..sync.utils import get_user_agent, get_ip, get_device_id
Expand Down Expand Up @@ -35,6 +35,10 @@ class User(db.Model):
__table_args__ = (
db.Index("ix_user_username", func.lower(username), unique=True),
db.Index("ix_user_email", func.lower(email), unique=True),
CheckConstraint(
"email ~ '^[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+.[A-Za-z]{2,}$'",
name="email_format",
),
)

def __init__(self, username, email, passwd, is_admin=False):
Expand Down
11 changes: 11 additions & 0 deletions server/mergin/auth/utils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# Copyright (C) Lutra Consulting Limited
#
# SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-MerginMaps-Commercial

import re


def is_email_address_valid(email: str) -> bool:
"""Check if email address contains only ASCII characters and basic email address requirements"""
email_ascii = r"^[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+.[A-Za-z]{2,}$"
return re.match(email_ascii, email) is not None
24 changes: 24 additions & 0 deletions server/mergin/tests/test_auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from flask import url_for
from itsdangerous import URLSafeTimedSerializer
from sqlalchemy import desc
from sqlalchemy.exc import IntegrityError
from unittest.mock import patch

from ..auth.models import User, UserProfile, LoginHistory
Expand Down Expand Up @@ -123,6 +124,12 @@ def test_logout(client):
400,
), # tests with upper case, but email already exists
("XmerginX", " mergin@mergin.com ", "#pwd123", 400), # invalid password
(
"mergin4",
"invalid\360@email.com",
"#pwd1234",
400,
), # non-ascii character in the email
]


Expand Down Expand Up @@ -762,3 +769,20 @@ def test_update_project_v2(client):
login_as_admin(client)
resp = client.patch(f"v2/projects/{project.id}", json=data)
assert resp.status_code == 204


user_data = [
("user1", True), # no problem
("user\360", False), # non-ascii character
("user\\", False), # disallowed character
]


@pytest.mark.parametrize("username,success", user_data)
def test_user_email_db_constraint(client, username, success):
if success:
add_user(username=username)
else:
with pytest.raises(IntegrityError):
add_user(username=username)
db.session.rollback()
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
"""Email address format check

Revision ID: 1a6175c78a10
Revises: 1ab5b02ce532
Create Date: 2024-10-17 15:13:11.360991

"""

from alembic import op


# revision identifiers, used by Alembic.
revision = "1a6175c78a10"
down_revision = "1ab5b02ce532"
branch_labels = None
depends_on = None


def upgrade():
op.create_check_constraint(
constraint_name="email_format",
table_name="user",
condition="email ~ '^[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+.[A-Za-z]{2,}$'",
)


def downgrade():
op.drop_constraint("email_format", "user", type_="check")
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
"""Migrage project version author name to user.id

Revision ID: 1ab5b02ce532
Revises: 57d0de13ce4a
Revises: 1c23e3be03a3
Create Date: 2024-09-06 14:01:40.668483

"""
Expand Down
Loading