Skip to content

GitLab

  • Projects
  • Groups
  • Snippets
  • Help
    • Loading...
  • Help
    • Help
    • Support
    • Community forum
    • Submit feedback
    • Contribute to GitLab
  • Sign in / Register
erp5 erp5
  • Project overview
    • Project overview
    • Details
    • Activity
    • Releases
  • Repository
    • Repository
    • Files
    • Commits
    • Branches
    • Tags
    • Contributors
    • Graph
    • Compare
  • Labels
    • Labels
  • Merge requests 139
    • Merge requests 139
  • CI/CD
    • CI/CD
    • Pipelines
    • Jobs
    • Schedules
  • Operations
    • Operations
    • Environments
  • Analytics
    • Analytics
    • CI/CD
    • Repository
    • Value Stream
  • Wiki
    • Wiki
  • Snippets
    • Snippets
  • Members
    • Members
  • Activity
  • Graph
  • Jobs
  • Commits
Collapse sidebar
  • nexedi
  • erp5erp5
  • Merge requests
  • !1561

Merged
Created Feb 22, 2022 by Jérome Perrin@jeromeOwner

ERP5Security,erp5: simplify EncryptedPasswordMixin.setPassword

  • Overview 2
  • Commits 2
  • Pipelines 2
  • Changes 9

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.
Assignee
Assign to
Reviewer
Request review from
None
Milestone
None
Assign milestone
Time tracking
Source branch: fix/set_password_none
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7