[15.0][FIX] password_security: Reset Password issue#492
Conversation
astirpe
force-pushed
the
15_fix_password_security2
branch
from
a97b93b to
b320d4d
Compare
| qcontext["error"] = e.args[0] | ||
| response = request.render("auth_signup.reset_password", qcontext) | ||
| response.headers["X-Frame-Options"] = "DENY" | ||
| return response |
There was a problem hiding this comment.
What if the call to _validate_pass_reset moved into res.users action_reset_password instead? That would remove the need for this copy pasta.
There was a problem hiding this comment.
It might be necessary to check to see if the request.uid is the admin user, to allow for bulk password resets that ignore this setting.
There was a problem hiding this comment.
In commit 8a5ead9 I moved _validate_pass_reset into action_reset_password. It seems working fine to me. Would you also give it a look?
astirpe
force-pushed
the
15_fix_password_security2
branch
2 times, most recently
from
b0b5320 to
8a5ead9
Compare
| if self.env.context.get("install_mode"): | ||
| return | ||
| if not self.env.context.get("create_user"): | ||
| users = self.filtered(lambda user: user.active) |
There was a problem hiding this comment.
This active user check seems redundant with
but maybe it is handling a case where a module could create an inactive user?
Good catch on the install_mode check. Maybe also add something like
if self.env.uid == SUPERUSER_ID:
as I just got Passwords can only be reset every 24 hour(s). Please contact an administrator for assistance. while logged in as admin. 😆
Edit: Steps to reproduce as admin on a brand new instance with --init=password_security:
- Home Menu > Settings
- Users & Companies > Users
- Check Administrator
- Action > Send Password Reset Instructions
- Error
There was a problem hiding this comment.
The check on active is redundant but I made it on purpose, so that in case an user is inactive, this standard error will be always raised, instead of the eventual _validate_pass_reset.
I just added the SUPERUSER_ID check.
astirpe
force-pushed
the
15_fix_password_security2
branch
from
783bd4d to
9e194e9
Compare
astirpe
force-pushed
the
15_fix_password_security2
branch
2 times, most recently
from
05a45f4 to
d3c90a9
Compare
astirpe
force-pushed
the
15_fix_password_security2
branch
from
d3c90a9 to
7d2a93a
Compare
astirpe
force-pushed
the
15_fix_password_security2
branch
2 times, most recently
from
5711947 to
b339d39
Compare
astirpe
force-pushed
the
15_fix_password_security2
branch
from
b339d39 to
a71e6f9
Compare
|
/ocabot merge patch |
|
On my way to merge this fine PR! |
|
Congratulations, your PR was merged at dda6ac1. Thanks a lot for contributing to OCA. ❤️ |
Syncing from upstream OCA/server-auth (16.0)
4 participants
Fixes the Reset Password issue described in #54.
The fix is backported from #482.
Replaces mocked tests for password reset, as they were not capable to test the code properly.