Commit 81a6500d authored by Jérome Perrin's avatar Jérome Perrin

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
parent 8127666c
Pipeline #16668 failed with stage
in 0 seconds
...@@ -232,6 +232,10 @@ class ERP5LoginUserManager(BasePlugin): ...@@ -232,6 +232,10 @@ class ERP5LoginUserManager(BasePlugin):
special_user_name_set.add(user_login) special_user_name_set.add(user_login)
else: else:
login_list.append(user_login) 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 = {} login_dict = {}
if exact_match: if exact_match:
requested = set(login_list).__contains__ requested = set(login_list).__contains__
......
...@@ -274,13 +274,13 @@ class TestUserManagement(UserManagementTestCase): ...@@ -274,13 +274,13 @@ class TestUserManagement(UserManagementTestCase):
self._assertUserExists(login, password) self._assertUserExists(login, password)
self._assertUserDoesNotExists('case_test_User', password) self._assertUserDoesNotExists('case_test_User', password)
def test_PersonLoginIsNotStripped(self): def test_PersonLoginIsStripped(self):
"""Make sure 'foo ', ' foo' and ' foo ' do not match user 'foo'. """ """Make sure 'foo ', ' foo' and ' foo ' match user 'foo'. """
_, login, password = self._makePerson() _, login, password = self._makePerson()
self._assertUserExists(login, password) self._assertUserExists(login, password)
self._assertUserDoesNotExists(login + ' ', password) self._assertUserExists(login + ' ', password)
self._assertUserDoesNotExists(' ' + login, password) self._assertUserExists(' ' + login, password)
self._assertUserDoesNotExists(' ' + login + ' ', password) self._assertUserExists(' ' + login + ' ', password)
def test_PersonLoginCannotBeComposed(self): def test_PersonLoginCannotBeComposed(self):
"""Make sure ZSQLCatalog keywords cannot be used at login time""" """Make sure ZSQLCatalog keywords cannot be used at login time"""
...@@ -491,6 +491,18 @@ class DuplicatePrevention(UserManagementTestCase): ...@@ -491,6 +491,18 @@ class DuplicatePrevention(UserManagementTestCase):
self.assertRaises(ValidationFailed, login2_value.validate) self.assertRaises(ValidationFailed, login2_value.validate)
self.assertRaises(ValidationFailed, self.portal.portal_workflow.doActionFor, login2_value, 'validate_action') 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): def _duplicateLoginReference(self, commit):
_, login1, _ = self._makePerson(tic=False) _, login1, _ = self._makePerson(tic=False)
user_id2, login2, _ = self._makePerson(tic=False) user_id2, login2, _ = self._makePerson(tic=False)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment