[17.0][MIG] password_security: Migration to 17.0 #610
Conversation
* [ADD] res_users_password_security: New module * Create new module to lock down user passwords * [REF] res_users_password_security: PR Review fixes * Also add beta pass history rule * [ADD] res_users_password_security: Pass history and min time * Add pass history memory and threshold * Add minimum time for pass resets through web reset * Begin controller tests * Fix copyright, wrong year for new file * Add tests for password_security_home * Left to do web_auth_reset_password * Fix minimum reset threshold and finish tests * Bug fixes per review * [REF] password_security: PR review improvements * Change tech name to password_security * Use new except format * Limit 1 & new api * Cascade deletion for pass history * [REF] password_security: Fix travis + style * Fix travis errors * self to cls * Better variable names in tests * [FIX] password_security: Fix travis errors
* Bump versions * Installable to True * Add Usage section to ReadMe w/ Runbot link * `_crypt_context` now directly exposes the `CryptContext` * Change all instances of openerp to odoo
* Add current time as password_write_date for admin user in demo, disabling the reset prompt - fixes OCA#652
* Switch security to be on correct model to fix OCA#674
…ord invalid (#859) * [FIX] password_security: Fix password stored * [REF] password_security: use a unified check_password private method to validate rules and history password
* Add logic to overloaded web_login action to log out users with expired passwords, preventing the password reset from being ignored * Add unit test for new logic
This translates to Spanish all missing translations, 31 in total.
Since some implementation details are changed, I had to change some tests that were actually testing the implementation instead of the desired result of the method.
In a normal Odoo deployment, somebody in group *Administration / Access Rights* should be able to create users; but if this addon is installed, it gets this error:
The requested operation cannot be completed due to security restrictions. Please contact your system administrator.
(Document type: Res Users Password History, Operation: create)
This is now tested and fixed.
[The `website` addon returns an aditional redirection][1] that makes these tests fail if ran after installing `website`. The tests were checking the returned value in a funky way anyways. Now, instead of checking the final returned value, we check directly the parameters sent to the redirection method. [1]: https://github.com/odoo/odoo/blob/3b85900fafc9469dca6e7c01fca6dac4f55d20f5/addons/website/controllers/main.py#L85-L89
Avoided requiring the module twice in JS.
Currently translated at 57.9% (22 of 38 strings) Translation: server-auth-12.0/server-auth-12.0-password_security Translate-URL: https://translation.odoo-community.org/projects/server-auth-12-0/server-auth-12-0-password_security/hr/
|
Hi, Would it be possible to merge this pull request as it has been approved and there is no conflicts with the base branch? Thank you. |
|
Hi, looks like it's ready to merge, isn't it @Abranes @salvorapi ? |
|
We're also waiting to have this merged 🙌 |
|
/ocabot merge nobump |
|
What a great day to merge this nice PR. Let's do it! |
|
@etobella your merge command was aborted due to failed check(s), which you can inspect on this commit of 17.0-ocabot-merge-pr-610-by-etobella-bump-nobump. After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red. |
|
@beagle-cloud @DynAppsNV can you rebase the Pull Request? |
|
@etobella your merge command was aborted due to failed check(s), which you can inspect on this commit of 17.0-ocabot-merge-pr-610-by-etobella-bump-nobump. After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red. |
|
Thanks for taking the time to port this module to Odoo v17. I take the opportunity of this v17 migration to give my opinion on this module : storing the password security parameter on res.company is a non-sense : if a user has access to several companies, he could select the company in which the security parameters are weaker to change his password ! The parameters really need to be moved to ir.config_parameter. In the module "auth_password_policy" of the official addons on which this module depend, the field minlength is an ir.config_parameter and I think it's a good design, cf https://github.com/odoo/odoo/blob/17.0/addons/auth_password_policy/models/res_config_settings.py#L8 What do you think ? Do you agree with my opinion ? |
|
/ocabot migration password_security |
|
This PR looks fantastic, let's merge it! |
|
@dreispt your merge command was aborted due to failed check(s), which you can inspect on this commit of 17.0-ocabot-merge-pr-610-by-dreispt-bump-nobump. After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red. |
| @api.model | ||
| def get_password_policy(self): | ||
| data = super().get_password_policy() | ||
| company_id = self.env.user.company_id |
There was a problem hiding this comment.
We should not use "user.company_id" since v13 !
|
password_security % grep -ri "user.company_id" *|wc -l I'm preparing a PR to try to cleanup this mess and move params from company to ir.config_parameter. |
| @@ -0,0 +1,33 @@ | |||
| # License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl). | |||
There was a problem hiding this comment.
THis file should be removed. I'll do it in my PR
|
Here is my PR to improve this PR: DynAppsNV#1 |
|
As my PR to improve this PR was ignored, I made a new OCA PR that include the migration to v17 and the migration to ir.config_parameters with migration scripts from the 1st company to ir.config_parameters |
|
Runboat build is pending since long, is there any idea when this will be ready? |
No description provided.