base/ERP5Security: better handling of empty passwords 👷

bt5/erp5_base/DocumentTemplateItem/portal_components/document.erp5.Login.py

@@ -33,7 +33,7 @@ from Products.ERP5Type import Permissions, PropertySheet, interfaces
 from Products.ERP5Type.XMLObject import XMLObject
 
 
-class Login(XMLObject, LoginAccountProviderMixin, EncryptedPasswordMixin):
+class Login(LoginAccountProviderMixin, EncryptedPasswordMixin, XMLObject):
   meta_type = 'ERP5 Login'
   portal_type = 'Login'
   add_permission = Permissions.AddPortalContent

bt5/erp5_base/DocumentTemplateItem/portal_components/document.erp5.Person.py

@@ -60,7 +60,7 @@ class UserExistsError(
     super(UserExistsError, self).__init__('user id %s already exists' % (user_id, ))
 
 
-class Person(Node, LoginAccountProviderMixin, EncryptedPasswordMixin, ERP5UserMixin):
+class Person(LoginAccountProviderMixin, EncryptedPasswordMixin, ERP5UserMixin, Node):
   """
       An Person object holds the information about
       an person (ex. you, me, someone in the company,

bt5/erp5_base/InterfaceTemplateItem/portal_components/interface.erp5.IEncryptedPassword.py

@@ -39,12 +39,12 @@ class IEncryptedPassword(Interface):
 
   def checkPassword(value):
     """
-    Check the password, usefull when changing password
+    Check the password `value` match the current password, usefull when changing password.
     """
 
   def checkPasswordValueAcceptable(value):
     """
-    Check if the password value is acceptable - i.e. follows site rules.
+    Check if the password `value` is acceptable in regard to password policy.
     """
 
   def setEncodedPassword(value, format='default'): # pylint: disable=redefined-builtin
@@ -63,7 +63,10 @@ class IEncryptedPassword(Interface):
 
   def checkUserCanChangePassword():
     """
-    check user have permission to change his password. Raise in case he cannot.
+    Check if the current logged in user have permission to change their password.
+
+    Raise Products.CMFCore.exceptions.AccessControl_Unauthorized in case they don't
+    have the permission
     """
 
   def setPassword(value) :
@@ -83,7 +86,7 @@ class IEncryptedPassword(Interface):
       Default: None
     format (string)
       String defining the format in which the password is expected.
-      If passowrd is not available in that format, KeyError will be
+      If password is not available in that format, KeyError will be
       raised.
       Default: 'default'
     """

bt5/erp5_base/MixinTemplateItem/portal_components/mixin.erp5.EncryptedPasswordMixin.py

@@ -39,7 +39,7 @@ from Products.ERP5Type.Globals import PersistentMapping
 from Products.CMFCore.utils import _checkPermission
 from Products.CMFCore.exceptions import AccessControl_Unauthorized
 
-class EncryptedPasswordMixin:
+class EncryptedPasswordMixin(object):
 
   # Declarative security
   security = ClassSecurityInfo()
@@ -68,7 +68,7 @@ class EncryptedPasswordMixin:
     usage.
     """
     if not self.getPortalObject().portal_preferences.isAuthenticationPolicyEnabled():
-      # not a policy so basically all passwords are accceptable
+      # not policy enabled, so basically all passwords are accceptable
       return True
     if not self.isPasswordValid(value):
       raise ValueError("Password does not comply with password policy")
@@ -87,7 +87,7 @@ class EncryptedPasswordMixin:
       password = self.password = PersistentMapping()
     self.password[format] = value
 
-  security.declarePublic('setEncodedPassword')
+  security.declareProtected(Permissions.SetOwnPassword, 'setEncodedPassword')
   def setEncodedPassword(
       self,
       value,
@@ -95,27 +95,18 @@ class EncryptedPasswordMixin:
   ):
     """
     """
-    self.checkUserCanChangePassword()
     self._setEncodedPassword(value, format=format)
     self.reindexObject()
 
   def _forceSetPassword(self, value):
     self.password = PersistentMapping()
-    self._setEncodedPassword(pw_encrypt(value))
+    if value:
+      self._setEncodedPassword(pw_encrypt(value))
 
   def _setPassword(self, value):
-    self.checkUserCanChangePassword()
     self.checkPasswordValueAcceptable(value)
     self._forceSetPassword(value)
 
-  security.declarePublic('setPassword')
-  def setPassword(self, value) :
-    """
-    """
-    if value is not None:
-      self._setPassword(value)
-      self.reindexObject()
-
   security.declareProtected(Permissions.AccessContentsInformation, 'getPassword')
   def getPassword(self, *args, **kw):
     """
@@ -140,4 +131,17 @@ class EncryptedPasswordMixin:
           password = default_password
     return password
 
+  security.declareProtected(Permissions.ModifyPortalContent, 'edit')
+  def edit(self, *args, **kw):
+    """edit, with support for empty password for the user interface.
+
+    In the user interface, we can have a my_password field, that will not
+    be pre-filled with the current password, but will be empty. To accomodate
+    this case, we don't edit the password if it is empty.
+    """
+    if kw.get('password') is None:
+      kw.pop('password', None)
+    return super(EncryptedPasswordMixin, self).edit(*args, **kw)
+
+
 InitializeClass(EncryptedPasswordMixin)

bt5/erp5_core_test/TestTemplateItem/portal_components/test.erp5.testPerson.py

@@ -239,27 +239,36 @@ class TestPerson(ERP5TypeTestCase):
     self.assertTrue(person.getUserId())
     self.assertFalse(self.portal.person_module._p_changed)
 
-  def testSetPasswordSecurity(self):
-    p = self._makeOne(id='person')
-    p.manage_permission(Permissions.SetOwnPassword, [], 0)
+  def testSetPasswordSecurityOnPerson(self):
+    self._testSetPasswordSecurity(
+        self._makeOne())
+
+  def testSetPasswordSecurityOnERP5Login(self):
+    self._testSetPasswordSecurity(
+        self._makeOne().newContent(portal_type='ERP5 Login'))
+
+  def _testSetPasswordSecurity(self, login):
+    login.manage_permission(Permissions.SetOwnPassword, [], 0)
     with self.assertRaises(Unauthorized):
-      guarded_getattr(p, 'setPassword')('secret')
+      guarded_getattr(login, 'setPassword')('secret')
     with self.assertRaises(Unauthorized):
-      guarded_getattr(p, 'edit')(password='secret')
+      guarded_getattr(login, 'edit')(password='secret')
 
-    # setPassword(None) has no effect, because in the user interface we always
-    # show an empty field for password. Note that it also does not require any
-    # specific permission.
-    p.setPassword(None)
-    self.assertFalse(p.getPassword())
+    # edit(password=None) has no effect. It's a special case, because in the user interface
+    # we show an empty field for password.
+    login.edit(password=None)
+    self.assertFalse(login.getPassword())
     # Make sure that edit method cannot call __setPasswordByForce and nothing
     # changes.
-    p.edit(password_by_force='waaa')
-    self.assertFalse(p.getPassword())
-
-    p.manage_permission(Permissions.SetOwnPassword, ['Anonymous'], 0)
-    p.setPassword('secret')
-    self.assertTrue(p.getPassword())
+    login.edit(password_by_force='waaa')
+    self.assertFalse(login.getPassword())
+
+    login.manage_permission(Permissions.SetOwnPassword, ['Anonymous'], 0)
+    login.setPassword('secret')
+    password = login.getPassword()
+    self.assertTrue(password)
+    login.edit(password=None)
+    self.assertEqual(login.getPassword(), password)
 
   def testSetUserIdSecurity(self):
     # Changing an already set user id needs "manage users" permissions,

product/ERP5/bootstrap/erp5_core/ToolComponentTemplateItem/portal_components/tool.erp5.PasswordTool.py

@@ -301,9 +301,7 @@ class PasswordTool(BaseTool):
     )
     login_dict, = user_dict['login_list']
     login = portal.unrestrictedTraverse(login_dict['path'])
-    login.checkPasswordValueAcceptable(password) # this will raise if password does not match policy
-    login._forceSetPassword(password)
-    login.reindexObject()
+    login.setPassword(password) # this will raise if password does not match policy
     return redirect(REQUEST, site_url,
                     translateString("Password changed."))
 

product/ERP5Security/tests/testERP5Security.py

@@ -332,14 +332,45 @@ class TestUserManagement(UserManagementTestCase):
     _, _, password = self._makePerson(login=login)
     self._assertUserExists(login, password)
 
-  def test_PersonWithLoginWithEmptyPasswordAreNotUsers(self):
+  def test_PersonWithLoginWithNonePasswordAreNotUsers(self):
+    """Tests a person with a login but None as a password is not a valid user."""
+    # check password set to None at creation
+    _, login, _ = self._makePerson(password=None)
+    self._assertUserDoesNotExists(login, None)
+    self._assertUserDoesNotExists(login, 'None')
+    self._assertUserDoesNotExists(login, '')
+
+    # check password set to None after being set
+    user_data, = self.portal.acl_users.searchUsers(login=login, exact_match=True)
+    erp5_login = self.portal.restrictedTraverse(user_data['login_list'][0]['path'])
+    erp5_login.setPassword('secret')
+    self.tic()
+    self._assertUserExists(login, 'secret')
+    erp5_login.setPassword(None)
+    self.tic()
+    self._assertUserDoesNotExists(login, 'secret')
+    self._assertUserDoesNotExists(login, None)
+    self._assertUserDoesNotExists(login, 'None')
+    self._assertUserDoesNotExists(login, '')
+
+  def test_PersonWithLoginWithEmptyStringPasswordAreNotUsers(self):
     """Tests a person with a login but no password is not a valid user."""
-    password = None
-    _, login, _ = self._makePerson(password=password)
-    self._assertUserDoesNotExists(login, password)
-    password = ''
-    _, login, self._makePerson(password=password)
-    self._assertUserDoesNotExists(login, password)
+    _, login, _ = self._makePerson(password='')
+    self._assertUserDoesNotExists(login, '')
+    self._assertUserDoesNotExists(login, 'None')
+
+    # check password set to '' after being set
+    user_data, = self.portal.acl_users.searchUsers(login=login, exact_match=True)
+    erp5_login = self.portal.restrictedTraverse(user_data['login_list'][0]['path'])
+    erp5_login.setPassword('secret')
+    self.tic()
+    self._assertUserExists(login, 'secret')
+    erp5_login.setPassword('')
+    self.tic()
+    self._assertUserDoesNotExists(login, 'secret')
+    self._assertUserDoesNotExists(login, None)
+    self._assertUserDoesNotExists(login, 'None')
+    self._assertUserDoesNotExists(login, '')
 
   def test_PersonWithEmptyLoginAreNotUsers(self):
     """Tests a person with empty login & password is not a valid user."""