diff --git a/README.md b/README.md index b5b1a384..b237f352 100644 --- a/README.md +++ b/README.md @@ -131,6 +131,7 @@ This next release focus on two-factor-authentication as a measure to increase se * Add two-factor authentication with email verification #201 * Generate a new session on login and 2FA #220 * Enforce permission on /etc/rdiffweb configuration folder +* Enforce validation on fullname, username and email Breaking changes: diff --git a/rdiffweb/controller/page_admin_users.py b/rdiffweb/controller/page_admin_users.py index b6d4e72d..953ee719 100644 --- a/rdiffweb/controller/page_admin_users.py +++ b/rdiffweb/controller/page_admin_users.py @@ -74,6 +74,8 @@ class UserForm(CherryForm): validators=[ validators.data_required(), validators.length(max=256, message=_('Username too long.')), + validators.length(min=3, message=_('Username too short.')), + validators.regexp(UserObject.PATTERN_USERNAME, message=_('Must not contain any special characters.')), ], ) fullname = StringField( @@ -81,6 +83,7 @@ class UserForm(CherryForm): validators=[ validators.optional(), validators.length(max=256, message=_('Fullname too long.')), + validators.regexp(UserObject.PATTERN_FULLNAME, message=_('Must not contain any special characters.')), ], ) email = EmailField( @@ -88,6 +91,7 @@ class UserForm(CherryForm): validators=[ validators.optional(), validators.length(max=256, message=_('Email too long.')), + validators.regexp(UserObject.PATTERN_EMAIL, message=_('Must be a valid email address.')), ], ) password = PasswordField( @@ -140,17 +144,6 @@ class UserForm(CherryForm): widget=widgets.HiddenInput(), ) - def __init__(self, *args, **kwargs): - super().__init__(*args, **kwargs) - cfg = cherrypy.tree.apps[''].cfg - self.password.validators += [ - validators.length( - min=cfg.password_min_length, - max=cfg.password_max_length, - message=_('Password must have between %(min)d and %(max)d characters.'), - ) - ] - def validate_role(self, field): # Don't allow the user to changes it's "role" state. currentuser = cherrypy.request.currentuser diff --git a/rdiffweb/controller/page_pref_general.py b/rdiffweb/controller/page_pref_general.py index 8988e792..df2b731c 100644 --- a/rdiffweb/controller/page_pref_general.py +++ b/rdiffweb/controller/page_pref_general.py @@ -19,9 +19,6 @@ to change password ans refresh it's repository view. """ -import logging -import re - import cherrypy from wtforms.fields import HiddenField, PasswordField, StringField, SubmitField from wtforms.fields.html5 import EmailField @@ -29,13 +26,9 @@ from rdiffweb.controller import Controller, flash from rdiffweb.controller.form import CherryForm +from rdiffweb.core.model import UserObject from rdiffweb.tools.i18n import gettext_lazy as _ -# Define the logger -_logger = logging.getLogger(__name__) - -PATTERN_EMAIL = re.compile(r'[a-z0-9._%+-]+@[a-z0-9.-]+\.[a-z]{2,4}$') - class UserProfileForm(CherryForm): action = HiddenField(default='set_profile_info') @@ -45,14 +38,15 @@ class UserProfileForm(CherryForm): validators=[ Optional(), Length(max=256, message=_('Fullname too long.')), + Regexp(UserObject.PATTERN_FULLNAME, message=_('Must not contain any special characters.')), ], ) email = EmailField( _('Email'), validators=[ DataRequired(), - Length(max=256, message=_("Invalid email.")), - Regexp(PATTERN_EMAIL, message=_("Invalid email.")), + Length(max=256, message=_("Email too long.")), + Regexp(UserObject.PATTERN_EMAIL, message=_("Must be a valid email address.")), ], ) set_profile_info = SubmitField(_('Save changes')) diff --git a/rdiffweb/controller/tests/test_page_admin_users.py b/rdiffweb/controller/tests/test_page_admin_users.py index c7331a74..c32a88f4 100644 --- a/rdiffweb/controller/tests/test_page_admin_users.py +++ b/rdiffweb/controller/tests/test_page_admin_users.py @@ -18,6 +18,7 @@ from unittest.mock import ANY, MagicMock import cherrypy +from parameterized import parameterized import rdiffweb.test from rdiffweb.core.model import UserObject @@ -170,40 +171,58 @@ def test_add_edit_delete(self): self.assertInBody("User account removed.") self.assertNotInBody("test2") - def test_edit_fullname(self): + @parameterized.expand( + [ + # Invalid + ('evil.com', False), + ('http://test', False), + ('email@test.test', False), + ('/test/', False), + # Valid + ('My fullname', True), + ('Test Test', True), + ('Éric Terrien-Pascal', True), + ("Tel'c", True), + ] + ) + def test_edit_fullname_with_special_character(self, new_fullname, expected_valid): # Given an existing user # When updating the user's fullname self.getPage( "/admin/users/", method='POST', - body={'action': 'edit', 'username': self.USERNAME, 'fullname': 'My fullname'}, + body={'action': 'edit', 'username': self.USERNAME, 'fullname': new_fullname}, ) self.assertStatus(200) - # Then user is updated successfully - self.assertInBody("User information modified successfully.") - # Then database is updated - obj = UserObject.query.filter(UserObject.username == self.USERNAME).first() - self.assertEqual('My fullname', obj.fullname) - - def test_add_edit_delete_user_with_encoding(self): - """ - Check creation of user with non-ascii char. - """ - self._add_user("Éric", "éric@test.com", "pr3j5Dwi", "/home/", UserObject.USER_ROLE) - self.assertInBody("User added successfully.") - self.assertInBody("Éric") - self.assertInBody("éric@test.com") - # Update user - self._edit_user("Éric", "eric.létourno@test.com", "écureuil", "/tmp/", UserObject.ADMIN_ROLE) - self.assertInBody("User information modified successfully.") - self.assertInBody("Éric") - self.assertInBody("eric.létourno@test.com") - self.assertNotInBody("/home/") - self.assertInBody("/tmp/") - - self._delete_user("Éric") - self.assertInBody("User account removed.") - self.assertNotInBody("Éric") + if expected_valid: + self.assertInBody("User information modified successfully.") + self.assertNotInBody("Fullname: Must not contain any special characters.") + else: + self.assertNotInBody("User information modified successfully.") + self.assertInBody("Fullname: Must not contain any special characters.") + + @parameterized.expand( + [ + # Invalid + ('http://username', False), + ('username@test.test', False), + ('/username/', False), + # Valid + ('username.com', True), + ('admin_user', True), + ('test.test', True), + ('test-test', True), + ] + ) + def test_add_user_with_special_character(self, new_username, expected_valid): + self._add_user(new_username, "eric@test.com", "pr3j5Dwi", "/home/", UserObject.USER_ROLE) + self.assertStatus(200) + if expected_valid: + self.assertInBody("User added successfully.") + self.assertNotInBody("Username: Must not contain any special characters.") + else: + self.assertNotInBody("User added successfully.") + self.assertInBody("Username: Must not contain any special characters.") def test_add_user_with_empty_username(self): """ diff --git a/rdiffweb/controller/tests/test_page_prefs_general.py b/rdiffweb/controller/tests/test_page_prefs_general.py index bfe15221..b46765d1 100644 --- a/rdiffweb/controller/tests/test_page_prefs_general.py +++ b/rdiffweb/controller/tests/test_page_prefs_general.py @@ -23,6 +23,7 @@ from unittest.mock import MagicMock import cherrypy +from parameterized import parameterized import rdiffweb.test from rdiffweb.core.model import RepoObject, UserObject @@ -88,16 +89,31 @@ def test_change_username_noop(self): self.assertIsNotNone(user) self.assertEqual("test@test.com", user.email) - def test_change_fullname(self): + @parameterized.expand( + [ + # Invalid + ('@test.com', False), + ('test.com', False), + ('test@te_st.com', False), + ('test@test.com, test2@test.com', False), + # Valid + ('test', True), + ('My Fullname', True), + ] + ) + def test_change_fullname(self, new_fullname, expected_valid): # Given an authenticated user # When update the fullname - self._set_profile_info("test@test.com", "My Fullname") + self._set_profile_info("test@test.com", new_fullname) self.assertStatus(200) - self.assertInBody("Profile updated successfully.") - # Then database is updated with fullname - self.assertInBody("My Fullname") - user = UserObject.query.filter(UserObject.username == self.USERNAME).first() - self.assertEqual("My Fullname", user.fullname) + if expected_valid: + self.assertInBody("Profile updated successfully.") + # Then database is updated with fullname + self.assertInBody(new_fullname) + user = UserObject.query.filter(UserObject.username == self.USERNAME).first() + self.assertEqual(new_fullname, user.fullname) + else: + self.assertNotInBody("Profile updated successfully.") def test_change_fullname_method_get(self): # Given an authenticated user @@ -126,30 +142,31 @@ def test_change_email(self): self.assertStatus(200) self.assertInBody("Profile updated successfully.") - def test_change_email_with_invalid_email(self): - self._set_profile_info("@test.com") - self.assertStatus(200) - self.assertInBody("Invalid email") - - self._set_profile_info("test.com") - self.assertStatus(200) - self.assertInBody("Invalid email") - - self._set_profile_info("test") - self.assertStatus(200) - self.assertInBody("Invalid email") - - self._set_profile_info("test@te_st.com") - self.assertStatus(200) - self.assertInBody("Invalid email") - - self._set_profile_info("test@test.com, test2@test.com") + @parameterized.expand( + [ + # Invalid + ('@test.com', False), + ('test.com', False), + ('test', False), + ('test@te_st.com', False), + ('test@test.com, test2@test.com', False), + # Valid + ('test@test.com', True), + ] + ) + def test_change_email_with_invalid_email(self, new_email, expected_valid): + self._set_profile_info(new_email) self.assertStatus(200) - self.assertInBody("Invalid email") + if expected_valid: + self.assertInBody("Profile updated successfully.") + self.assertNotInBody("Must be a valid email address.") + else: + self.assertNotInBody("Profile updated successfully.") + self.assertInBody("Must be a valid email address.") def test_change_email_with_too_long(self): self._set_profile_info(("test1" * 50) + "@test.com") - self.assertInBody("Invalid email") + self.assertInBody("Email too long.") def test_change_password(self): self.listener.user_password_changed.reset_mock() diff --git a/rdiffweb/core/model/_user.py b/rdiffweb/core/model/_user.py index 0e846979..441e10b3 100644 --- a/rdiffweb/core/model/_user.py +++ b/rdiffweb/core/model/_user.py @@ -55,6 +55,7 @@ class UserObject(Base): __tablename__ = 'users' __table_args__ = {'sqlite_autoincrement': True} + # Value for role. ADMIN_ROLE = 0 MAINTAINER_ROLE = 5 USER_ROLE = 10 @@ -63,9 +64,15 @@ class UserObject(Base): 'maintainer': MAINTAINER_ROLE, 'user': USER_ROLE, } + # Value for mfa field DISABLED_MFA = 0 ENABLED_MFA = 1 + # Regex pattern to be used for validation. + PATTERN_EMAIL = r"[a-z0-9._%+-]+@[a-z0-9.-]+\.[a-z]{2,4}$" + PATTERN_FULLNAME = r"""[^!"#$%&()*+,./:;<=>?@[\]_{|}~]+$""" + PATTERN_USERNAME = r"[a-zA-Z0-9_.\-]+$" + userid = Column('UserID', Integer, primary_key=True) _username = Column('Username', String, nullable=False, unique=True) hash_password = Column('Password', String, nullable=False, default="")