ERP5Security,erp5: simplify EncryptedPasswordMixin.setPassword

For historical reasons, EncryptedPasswordMixin.setPassword was public
and did its own security checks, this was the case since 7d0882ef (
setPassword have to do explicit security checks…, 2007-11-12), this was
because we wanted to support cases where user can edit the login ("Edit
portal content" permission), but not changed the password ("Set own
password" permission).

Also, we wanted to support the case where login is edited through a view
form, in that case we have a my_password field that is empty and we
don't want to set the password to None in that case.

For these two reasons the API to set password was very complex and
behaving differently from other accessors: usually setSomething(None)
just set something to None, ie. "unset" something, but for passwords it
was not the case. Also we had to introduce _forceSetPassword method,
which sets the password without security checks, so that it can be
called from unrestricted code for cases where user does not have the
permission to reset password (like in the reset password scenario).

Since d1312cdb ( make edit check the security remove all useless
security declaration on private method, 2008-05-23), edit supports
restricted properties, so we can simplify all this and make setPassword
a more standard accessor, ie:
 - setPassword has a security declaration, so if it is called from
  restricted python the security will apply at `__getattr__` time.
  `edit` method will also check security
 - setPassword(None) reset the password.
 - The logic to not change the password when editing in view mode is now
  `edit` responsability. ie. `login.setPassword(None)` resets, but
  `login.edit(password=None)` does not reset.

This also correct some usage of the lower level API (`pw_encrypt` and
`pw_validate`) which were never supposed to use `None`:
 - `pw_validate` was called with None when a user without password was
  trying to login, causing a TypeError that was cached by PAS and logged
  with level debug (and refusing login). Now the error is no longer raised.
 - `pw_encrypt` was called with None (but apparently only in the tests,
  when doing `user.newContent(portal_type='ERP5 Login', password=None)`)
  and this was creating a login with password `'None'` with AccessControl 2.
  With AccessControl 4 this was an Error.

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(EncryptedPasswordMixin, XMLObject, LoginAccountProviderMixin):
   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(EncryptedPasswordMixin, Node, LoginAccountProviderMixin, ERP5UserMixin):
   """
       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

@@ -54,12 +54,16 @@ class IEncryptedPassword(Interface):
 
     Raise Products.CMFCore.exceptions.AccessControl_Unauthorized in case they don't
     have the permission.
+
+    This method is deprecated and is not used by IEncryptedPassword internally.
     """
 
-  def setPassword(value) :
+  def setPassword(value):
     """
-    Set the password, only if the password is not empty and if
-    checkUserCanChangePassword don't raise any error
+    Set the password to `value` (a string holding the password in clear text).
+
+    Passing an empty value (such as None or empty string) will erase previously defined
+    password, which usually prevent login with this password.
     """
 
   def setEncodedPassword(value, format='default'): # pylint: disable=redefined-builtin
@@ -67,13 +71,13 @@ class IEncryptedPassword(Interface):
     Set an already encoded password.
     """
 
-  def _forceSetPassword(value):
+  def edit(self, **kw):
     """
-    Because both _setPassword and setPassword are considered as
-    public method (they are callable from user directly or through edit method)
-    _forceSetPassword is needed to reset password without security check by
-    Password Tool. This method is not callable through edit method as it not
-    begins with _set*
+    Edit the password and other properties of the documents through user interface.
+
+    This method is responsible for supporting the case where a IEncryptedPassword is
+    edited with a my_password field that is empty by default and not resetting the password
+    when edited with password=None.
     """
 
   def getPassword(*args, **kw):

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()
@@ -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,20 @@ class EncryptedPasswordMixin:
   ):
     """
     """
-    self.checkUserCanChangePassword()
     self._setEncodedPassword(value, format=format)
     self.reindexObject()
 
   def _forceSetPassword(self, value):
+    # this method is kept for backward compatibility, as there might be interaction
+    # workflows on this method.
     self.password = PersistentMapping()
+    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 +133,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

@@ -32,6 +32,7 @@ import mock
 
 from AccessControl.SecurityManagement import newSecurityManager
 from AccessControl import Unauthorized
+from AccessControl.ZopeGuards import guarded_getattr
 
 from Products.ERP5Type.tests.ERP5TypeTestCase import ERP5TypeTestCase
 from Products.ERP5Type import Permissions
@@ -238,25 +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)
-    self.assertRaises(Unauthorized, p.setPassword, 'secret')
-    self.assertRaises(Unauthorized, p.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())
+  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(login, 'setPassword')('secret')
+    with self.assertRaises(Unauthorized):
+      guarded_getattr(login, 'edit')(password='secret')
+
+    # 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,

bt5/erp5_credential/DocumentTemplateItem/portal_components/document.erp5.CredentialRequest.py

@@ -32,7 +32,7 @@ from erp5.component.document.Ticket import Ticket
 from erp5.component.mixin.EncryptedPasswordMixin import EncryptedPasswordMixin
 
 
-class CredentialRequest(Ticket, EncryptedPasswordMixin):
+class CredentialRequest(EncryptedPasswordMixin, Ticket):
   """
   """
 
@@ -54,11 +54,6 @@ class CredentialRequest(Ticket, EncryptedPasswordMixin):
                     , PropertySheet.Url
                     )
 
-  def checkUserCanChangePassword(self):
-    # every body can change a password of a credential request as annonymous
-    # should be able to do it
-    pass
-
   def checkPasswordValueAcceptable(self, value):
     # all passwords are acceptable on Credential Request
     pass

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/ERP5LoginUserManager.py

@@ -127,10 +127,10 @@ class ERP5LoginUserManager(BasePlugin):
     is_authentication_policy_enabled = self.getPortalObject().portal_preferences.isAuthenticationPolicyEnabled()
     if check_password:
       password = credentials.get('password')
-      if not password or not pw_validate(
-        login_value.getPassword(),
-        password,
-      ):
+      login_password = login_value.getPassword()
+      if (not password
+          or login_password is None
+          or not pw_validate(login_password, password)):
         if is_authentication_policy_enabled:
           login_value.notifyLoginFailure()
         return

product/ERP5Security/tests/testERP5Security.py

@@ -332,14 +332,59 @@ 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_PersonWithLoginWithoutPasswordAreNotUsers(self):
+    """Tests a person with a login but no password set is not a valid user."""
+    # similar to _makePerson, but not passing password= to newContent
+    login = 'login_%s' % self._login_generator()
+    new_person = self.portal.person_module.newContent(portal_type='Person')
+    new_person.newContent(portal_type='Assignment').open()
+    new_person.newContent(
+        portal_type='ERP5 Login',
+        reference=login,
+    ).validate()
+    self.tic()
+    self._assertUserDoesNotExists(login, '')
+    self._assertUserDoesNotExists(login, 'None')
 
   def test_PersonWithEmptyLoginAreNotUsers(self):
     """Tests a person with empty login & password is not a valid user."""