update_password should check password validity #1630
Conversation
|
@tschorr thanks for creating this Pull Request and helping to improve Plone! TL;DR: Finish pushing changes, pass all other checks, then paste a comment: To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically. Happy hacking! |
✅ Deploy Preview for plone-restapi canceled.
|
|
@pbauer this PR will allow to use |
|
@jenkins-plone-org please run jobs |
|
Jenkins test failures look unrelated, but I might be missing something. |
| return self._error( | ||
| 400, | ||
| "Invalid password", | ||
| _(err), |
There was a problem hiding this comment.
It seems that sometimes testPasswordValidity returns an already translated string, sometimes it returns a msgid. Then you need to do a test to see what is returned. This should be fixed in Plone.
There was a problem hiding this comment.
uhnn but you are not passing the confirm parameter. Shouldn't you pass it here?
There was a problem hiding this comment.
Another weird thing is we have an update_password in add.py. Shouldn't this be on update.py?
There was a problem hiding this comment.
It seems that sometimes
testPasswordValidityreturns an already translated string, sometimes it returns a msgid. Then you need to do a test to see what is returned. This should be fixed in Plone.
I would expect _(...) to do the right thing in any case? +1 for fixing this in the RegistrationTool.
uhnn but you are not passing the
confirmparameter. Shouldn't you pass it here?
There's a sensible default and we do not have a password confirmation in this case.
Another weird thing is we have an update_password in add.py. Shouldn't this be on update.py?
Maybe, but imho that's for another ticket.
There was a problem hiding this comment.
I would expect _(...) to do the right thing in any case?
It would not be as expected. RegistrationTool's _() is from the plone domain. The _() here is from the plone.restapi domain. So if testPasswordValidity returned a msgid, you would be putting a msgid from the plone domain inside a msgid from the plone.restapi domain. I don't know if that would work. Even if it didn't give an error, the domains would be different and the translation would not occur. As long as you passed the msgid received from testPasswordValidity directly to the _error method, the translation would take place. But since you are not passing the confirm, this possibility does not exist. So it's fine to stay as it is.
+1 for fixing this in the RegistrationTool
Could you please open an issue about this in Plone?
There's a sensible default and we do not have a password confirmation in this case.
In the Volto form you mentioned, we have the passwordRepeat. Is it not passed to plone.restapi? Does Volto validate if the password confirmation is ok?
Maybe, but imho that's for another ticket.
Could you please open an issue about this here.
There was a problem hiding this comment.
@wesleybl I suggest you open the tickets yourself.
tschorr
changed the title
|
@jenkins-plone-org please run jobs |
p.restapiis usingPasswordResetTool.resetPasswordfor updating passwords, however this method doesn't check for password validity (https://github.com/plone/Products.CMFPlone/blame/master/Products/CMFPlone/PasswordResetTool.py#L103).This results in Volto bypassing the default password policy. Although the minimum password length has been updated to 8 characters (plone/Products.PlonePAS#69), Volto still mentions a minimum of 5 characters (https://github.com/plone/volto/blob/master/src/components/theme/PasswordReset/PasswordReset.jsx#L54) and Plone lets you get away with this. I actually could use a 3 letter password.