Skip to content
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.

Sign up for GitHub

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

Jump to bottom

update_password should check password validity #1630

Merged
merged 4 commits into from
Apr 27, 2023
Merged

update_password should check password validity #1630

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
85e0a27
test password validity
tschorr Apr 26, 2023
ab97e43
update changelog
tschorr Apr 26, 2023
203dd2b
change status code to match the one used by PlonePAS.MembershipTool.s…
tschorr Apr 26, 2023
d08753a
add a test
tschorr Apr 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions news/1630.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Respect Password Policy
[tschorr]
9 changes: 8 additions & 1 deletion src/plone/restapi/services/users/add.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,14 +266,14 @@ def update_password(self, data):
pas = getToolByName(self.context, "acl_users")
mt = getToolByName(self.context, "portal_membership")
pwt = getToolByName(self.context, "portal_password_reset")
registration_tool = getToolByName(self.context, "portal_registration")

if target_user is None:
self.request.response.setStatus(404)
return

# Send password reset mail
if list(data) == []:
registration_tool = getToolByName(self.context, "portal_registration")
registration_tool.mailPassword(username, self.request)
return

Expand All @@ -299,6 +299,13 @@ def update_password(self, data):
# Reset the password with a reset token
if reset_token:
try:
err = registration_tool.testPasswordValidity(new_password)
if err is not None:
return self._error(
400,
"Invalid password",
_(err),
Copy link
Member

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.

Copy link
Member

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?

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

)
pwt.resetPassword(username, reset_token, new_password)
except InvalidRequestError:
return self._error(
Expand Down