From 81a6500dbbd47928cb3596cf127dd27b7f6a345a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9rome=20Perrin?= <jerome@nexedi.com> Date: Fri, 30 Apr 2021 03:01:07 +0200 Subject: [PATCH] ERP5Security: ignore leading/trailing spaces in usernames We received a few support requests from users who are able to change their password but not to log in afterwards. These users probably copy and pasted their user name with an extra leading or trailing space. In the reset password dialog, these spaces are stripped, because Formulator by default strips the input (unless "whitespace preserve" is on, but we usually don't set it except in text areas). Historically we have been completely avoiding the extra spaces and made the login/user_id case sensitive, because login and user id were the same thing and there have been issues when looking up user id in mariadb because of mariadb collations, so we took the easy way of saying "logins are case sensitives and spaces also mater", but with separate login / user id, this can be revisited, because the login is only used to check the password and find an user ID. Stripping spaces from logins is a common thing these days (google, twitter, facebook strip logins) which simplifies user experience and reduces support. The risk of conflicts seems very low, if users are created with ERP5 Forms Formulator already had stripped the login anyway. After this change in case of two user names ('alice' and ' alice ') conflict, none of them would be able to login. We keep compatibility with users with trailing spaces, so if there is only a user named ' alice ', without other users that would conflict (for example 'alice' or ' alice'), this user remain able to login anyway. This last part is probably not so important in reality, it is for compatibility with testPasswordTool.TestPasswordTool.test_login_with_trailing_space --- product/ERP5Security/ERP5LoginUserManager.py | 4 ++++ .../ERP5Security/tests/testERP5Security.py | 22 ++++++++++++++----- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/product/ERP5Security/ERP5LoginUserManager.py b/product/ERP5Security/ERP5LoginUserManager.py index c044cf1a59..53c4d83009 100644 --- a/product/ERP5Security/ERP5LoginUserManager.py +++ b/product/ERP5Security/ERP5LoginUserManager.py @@ -232,6 +232,10 @@ class ERP5LoginUserManager(BasePlugin): special_user_name_set.add(user_login) else: login_list.append(user_login) + # Ignore leading or trailing space in login name + user_login_stripped = user_login.strip() + if user_login_stripped != user_login: + login_list.append(user_login_stripped) login_dict = {} if exact_match: requested = set(login_list).__contains__ diff --git a/product/ERP5Security/tests/testERP5Security.py b/product/ERP5Security/tests/testERP5Security.py index 67dbb432e8..4f5bc92456 100644 --- a/product/ERP5Security/tests/testERP5Security.py +++ b/product/ERP5Security/tests/testERP5Security.py @@ -274,13 +274,13 @@ class TestUserManagement(UserManagementTestCase): self._assertUserExists(login, password) self._assertUserDoesNotExists('case_test_User', password) - def test_PersonLoginIsNotStripped(self): - """Make sure 'foo ', ' foo' and ' foo ' do not match user 'foo'. """ + def test_PersonLoginIsStripped(self): + """Make sure 'foo ', ' foo' and ' foo ' match user 'foo'. """ _, login, password = self._makePerson() self._assertUserExists(login, password) - self._assertUserDoesNotExists(login + ' ', password) - self._assertUserDoesNotExists(' ' + login, password) - self._assertUserDoesNotExists(' ' + login + ' ', password) + self._assertUserExists(login + ' ', password) + self._assertUserExists(' ' + login, password) + self._assertUserExists(' ' + login + ' ', password) def test_PersonLoginCannotBeComposed(self): """Make sure ZSQLCatalog keywords cannot be used at login time""" @@ -491,6 +491,18 @@ class DuplicatePrevention(UserManagementTestCase): self.assertRaises(ValidationFailed, login2_value.validate) self.assertRaises(ValidationFailed, self.portal.portal_workflow.doActionFor, login2_value, 'validate_action') + def test_duplicateLoginReference_stripped(self): + _, login1, _ = self._makePerson() + _, login2, _ = self._makePerson() + pas_user2, = self.portal.acl_users.searchUsers(login=login2, exact_match=True) + pas_login2, = pas_user2['login_list'] + login2_value = self.portal.restrictedTraverse(pas_login2['path']) + login2_value.invalidate() + login2_value.setReference(login1 + ' ') + self.commit() + self.assertRaises(ValidationFailed, login2_value.validate) + self.assertRaises(ValidationFailed, self.portal.portal_workflow.doActionFor, login2_value, 'validate_action') + def _duplicateLoginReference(self, commit): _, login1, _ = self._makePerson(tic=False) user_id2, login2, _ = self._makePerson(tic=False) -- 2.30.9