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

[15.0][FIX] password_security: Reset Password issue #492

Merged
merged 1 commit into from
Jul 2, 2023
Merged

[15.0][FIX] password_security: Reset Password issue #492

merged 1 commit into from
Jul 2, 2023

Conversation

astirpe
Copy link
Member

@astirpe astirpe commented Mar 3, 2023 •
edited
Loading

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.

@astirpe astirpe force-pushed the 15_fix_password_security2 branch from a97b93b to b320d4d Compare March 3, 2023 09:53
@astirpe astirpe marked this pull request as ready for review March 3, 2023 09:58
@astirpe astirpe mentioned this pull request Mar 3, 2023
Merged
7 tasks
amh-mw
amh-mw suggested changes Mar 3, 2023
View reviewed changes
password_security/controllers/main.py Outdated
qcontext["error"] = e.args[0]
response = request.render("auth_signup.reset_password", qcontext)
response.headers["X-Frame-Options"] = "DENY"
return response
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@astirpe astirpe Mar 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

password_security/models/res_users.py Outdated Show resolved Hide resolved
@astirpe astirpe force-pushed the 15_fix_password_security2 branch 2 times, most recently from b0b5320 to 8a5ead9 Compare March 6, 2023 09:53
amh-mw
amh-mw suggested changes Mar 6, 2023
View reviewed changes
password_security/models/res_users.py Outdated
if self.env.context.get("install_mode"):
return
if not self.env.context.get("create_user"):
users = self.filtered(lambda user: user.active)
Copy link

@amh-mw amh-mw Mar 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This active user check seems redundant with

https://github.com/odoo/odoo/blob/991d1c430f2c9bb0d2b2d7ac8f75338eafd2aced/addons/auth_signup/models/res_users.py#L166-L167

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:

  1. Home Menu > Settings
  2. Users & Companies > Users
  3. Check Administrator
  4. Action > Send Password Reset Instructions
  5. Error

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 astirpe force-pushed the 15_fix_password_security2 branch from 783bd4d to 9e194e9 Compare March 6, 2023 14:34
amh-mw
amh-mw suggested changes Mar 6, 2023
View reviewed changes
password_security/tests/test_reset_password.py Outdated Show resolved Hide resolved
password_security/models/res_users.py Outdated Show resolved Hide resolved
@astirpe astirpe force-pushed the 15_fix_password_security2 branch 2 times, most recently from 05a45f4 to d3c90a9 Compare March 7, 2023 07:31
amh-mw
amh-mw suggested changes Mar 7, 2023
View reviewed changes
Copy link

@amh-mw amh-mw left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if there's anything to be done about the codecov/project red build. Yes, the lines of code covered have decreased, but that's because the new code is fewer lines of code...

password_security/models/res_users.py Outdated Show resolved Hide resolved
@astirpe astirpe force-pushed the 15_fix_password_security2 branch from d3c90a9 to 7d2a93a Compare March 7, 2023 14:21
@astirpe astirpe marked this pull request as draft March 7, 2023 14:25
@astirpe astirpe force-pushed the 15_fix_password_security2 branch 2 times, most recently from 5711947 to b339d39 Compare March 7, 2023 15:22
@astirpe astirpe force-pushed the 15_fix_password_security2 branch from b339d39 to a71e6f9 Compare March 7, 2023 15:25
@astirpe astirpe marked this pull request as ready for review March 7, 2023 15:28
amh-mw
amh-mw approved these changes Mar 7, 2023
View reviewed changes
Copy link

@amh-mw amh-mw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM @OCA/tools-maintainers

@thomaspaulb
Copy link
Contributor

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 15.0-ocabot-merge-pr-492-by-thomaspaulb-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit b9f1ece into OCA:15.0 Jul 2, 2023
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at dda6ac1. Thanks a lot for contributing to OCA. ❤️

SiesslPhillip pushed a commit to grueneerde/OCA-server-auth that referenced this pull request Nov 20, 2024
@bt-admin
Merge pull request OCA#492 from OCA/16.0
f811a10 
Syncing from upstream OCA/server-auth (16.0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amh-mw amh-mw amh-mw approved these changes

@thomaspaulb thomaspaulb thomaspaulb approved these changes

Assignees
No one assigned
Labels
approved merged 🎉
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@astirpe @thomaspaulb @OCA-git-bot @amh-mw