-
-
Notifications
You must be signed in to change notification settings - Fork 75
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uhnn but you are not passing the confirm
parameter. Shouldn't you pass it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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
confirm
parameter. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wesleybl I suggest you open the tickets yourself.
@jenkins-plone-org please run jobs |
p.restapi
is usingPasswordResetTool.resetPassword
for 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.