[17.0][MIG] password_security + use ir.config_parameters by alexis-via · Pull Request #731 · OCA/server-auth

alexis-via · 2024-11-21T22:33:53Z

Replaces migration PR #610

Migration to v17 + move config parameters from res.company to ir.config_parameter. That way, a user that has access to several companies cannot switch to the company with weakest password security params before changing its password, which was a kind of security hole.

By the way, the password security param of auth_password_policy was already stored in ir.config_parameter (cf https://github.com/odoo/odoo/blob/17.0/addons/auth_password_policy/models/res_config_settings.py#L8), so it's coherent to do the same here.

* [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 (OCA#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/

Currently translated at 100.0% (50 of 50 strings) Translation: server-auth-16.0/server-auth-16.0-password_security Translate-URL: https://translation.odoo-community.org/projects/server-auth-16-0/server-auth-16-0-password_security/es/

Currently translated at 100.0% (50 of 50 strings) Translation: server-auth-16.0/server-auth-16.0-password_security Translate-URL: https://translation.odoo-community.org/projects/server-auth-16-0/server-auth-16-0-password_security/it/

amh-mw

Though I agree with the intent of the improvement, this is the second migration pull request in a row for password_security where improvements are being done in the migration pull request. I think it will be more coherent to migrate between versions as usual and then review the improvement in its own pull request.

alexis-via · 2024-11-21T22:45:13Z

The move to ir.config_parameter is in a separate commit.

etobella · 2024-11-22T08:41:41Z

@amh-mw the usual flow in this kind of changes is as @alexis-via suggested (at least is what I found in a lot of cases in the OCA)

alexis-via · 2024-12-16T08:21:43Z

Let's merge this. Then I'll help to backport it.

amh-mw · 2024-12-16T12:57:25Z

+        f"{openupgrade.get_legacy_name('password_upper')}, "
+        f"{openupgrade.get_legacy_name('password_numeric')}, "
+        f"{openupgrade.get_legacy_name('password_special')} "
+        "FROM res_company ORDER BY id LIMIT 1"


This could degrade security for installations with more than one company. Perhaps remove LIMIT 1 and use MAX(field) or MIN(field) as appropriate?

The security problem is not really in this migration script but in the design of this module which had per-company settings and nobody dared to change this for years !!!
I was very surprised to be the first developer to criticize the design error and propose to fix it.
Anyway, admins will be to checks the new settings after update. I feel that mixing settings from different companies is kind of strange, so my idea was to simplify take the settings from the first company. I just updated to code to take the settings from the first active company.

amh-mw · 2024-12-16T13:05:31Z

+
+@openupgrade.migrate()
+def migrate(env, version):
+    openupgrade.rename_fields(


Why are company fields being renamed if ir.config_parameter is replacing them? Shouldn't pre-migration copy the fields into ir.config_parameter, then post-migration drop the fields?

This is done this way in order to avoid that odoo removes the fields.

amh-mw · 2024-12-16T13:13:28Z

+            "upper": int(params.get_param("password_security.upper", default=1)),
+            "numeric": int(params.get_param("password_security.numeric", default=1)),
+            "special": int(params.get_param("password_security.special", default=1)),
+        }


These defaults don't match previous versions. Change all default back to zero and get rid of this method entirely?

The only default which isn't matching with version 16.0 is minimum_hours. There is was 24 as default on the fields of res.company.

The defaults in the res.config.settings are different than the actual ones.

amh-mw · 2024-12-16T13:14:17Z

+    # So the solution to avoid this problem and have a non-null default value:
+    # 1) define the ir.config_parameter fields on res.config.settings with default=0
+    # 2) initialize the ir.config_parameter with a default value in the init script
+    # So the default value of the fields below are written in post_install.py


This explanation is unnecessary if all defaults are returned to zero.

This is a very very important comment, because it explains a design decision which is not obvious at all. We should keep it.

antoniocanovas

Functional review OK !!

etobella · 2025-01-31T08:45:35Z

@alexis-via Can you attend the comments please?

IsabelAForgeFlow · 2025-03-10T09:00:31Z

LGTM Functional + code

IsabelAForgeFlow

Functional review

antonag32 · 2025-03-13T21:57:22Z

I was going to do a forward port of this fix: #713

But since the module has not been migrated yet it might be a good opportunity to include the commit in this PR from the get go.

Replace fields on res.company by ir.config_parameter Remove dead test for v16 migration script

bguillot · 2025-04-01T09:25:13Z

/ocabot merge nobump

OCA-git-bot · 2025-04-01T09:25:21Z

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 17.0-ocabot-merge-pr-731-by-bguillot-bump-nobump, awaiting test results.

OCA-git-bot · 2025-04-01T09:33:16Z

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

lasley and others added 30 commits November 21, 2024 23:37

[MIG] Make modules uninstallable

dda2403

[MIG] Rename manifest files

71ec569

[MIG] password_security: Migrate to v10

607720a

* 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

[FIX] password_security: Validate admin pass

9f262d3

* Add current time as password_write_date for admin user in demo, disabling the reset prompt - fixes OCA#652

[FIX] password_security: Fix history rule

4ff8f5a

* Switch security to be on correct model to fix OCA#674

Improve UX password_match_message()

a642ffe

[FIX] password_security: Fix password stored and token to reset passw…

99750f8

…ord invalid (OCA#859) * [FIX] password_security: Fix password stored * [REF] password_security: use a unified check_password private method to validate rules and history password

[FIX] password_security: Force password reset

c36d672

* 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

[MIG]password security

5583abc

[IMP]change request

88188de

[IMP]Underscore is a special character #1077

f6f8554

[FIX] password_security: Default last write date #1084

28f3f3d

[FIX] UTF-8 coding is not necessary

a06e409

[MIG] auth_totp_password_security: Migration to 11.0

b253dd6

[I18N] password_security: Translate to Spanish all translatable terms

096c20b

This translates to Spanish all missing translations, 31 in total.

[FIX] password_security: Error 500 when login with bad password (OCA#27)

2f657de

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.

[UPD] Update password_security.pot

ff30b4d

[MIG] password_security: Migration to 12.0

089f28f

[FIX] password_security: correct readme fragments

459cbb9

[UPD] README.rst

fd855d8

[FIX] 12.0 password_security - Reset Password

81a5636

[FIX] 12.0 password_security - Reset Password

eb7f5a1

Avoided requiring the module twice in JS.

[UPD] README.rst

443ee31

Add zxcvbn to password security

3b0f4c8

Force add zxcvbn js lib

0c411de

[UPD] README.rst

04cbae2

Translated using Weblate (Croatian)

bec6383

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/

Ivorra78 and others added 4 commits November 21, 2024 23:37

Translated using Weblate (Spanish)

9566360

Currently translated at 100.0% (50 of 50 strings) Translation: server-auth-16.0/server-auth-16.0-password_security Translate-URL: https://translation.odoo-community.org/projects/server-auth-16-0/server-auth-16-0-password_security/es/

Translated using Weblate (Italian)

a9239b2

Currently translated at 100.0% (50 of 50 strings) Translation: server-auth-16.0/server-auth-16.0-password_security Translate-URL: https://translation.odoo-community.org/projects/server-auth-16-0/server-auth-16-0-password_security/it/

[IMP] password_security: pre-commit stuff

a858641

[MIG] password_security: Migration to 17.0

19be1fc

alexis-via force-pushed the 17.0-mig-password_security branch from f9cf363 to ea020fc Compare November 21, 2024 22:39

amh-mw suggested changes Nov 21, 2024

View reviewed changes

alexis-via mentioned this pull request Nov 22, 2024

[17.0][MIG] password_security: Migration to 17.0 #610

Closed

etobella approved these changes Nov 22, 2024

View reviewed changes

etobella requested a review from amh-mw December 16, 2024 08:26

amh-mw suggested changes Dec 16, 2024

View reviewed changes

antoniocanovas approved these changes Jan 31, 2025

View reviewed changes

IsabelAForgeFlow approved these changes Mar 10, 2025

View reviewed changes

amh-mw approved these changes Mar 10, 2025

View reviewed changes

OCA-git-bot added the approved label Mar 10, 2025

IsabelAForgeFlow mentioned this pull request Mar 10, 2025

[18.0][MIG] password_security #773

Merged

2 tasks

[IMP] password_security: use ir.config_parameter

3de6b17

Replace fields on res.company by ir.config_parameter Remove dead test for v16 migration script

alexis-via force-pushed the 17.0-mig-password_security branch from ea020fc to 3de6b17 Compare April 1, 2025 09:15

OCA-git-bot added the bot is merging ⏳ label Apr 1, 2025

OCA-git-bot merged commit f34b37b into OCA:17.0 Apr 1, 2025
7 checks passed

OCA-git-bot added merged 🎉 and removed bot is merging ⏳ labels Apr 1, 2025

Uh oh!

Conversation

alexis-via commented Nov 21, 2024

Uh oh!

amh-mw left a comment

Choose a reason for hiding this comment

Uh oh!

alexis-via commented Nov 21, 2024

Uh oh!

etobella commented Nov 22, 2024

Uh oh!

alexis-via commented Dec 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antoniocanovas left a comment

Choose a reason for hiding this comment

Uh oh!

etobella commented Jan 31, 2025

Uh oh!

IsabelAForgeFlow commented Mar 10, 2025

Uh oh!

IsabelAForgeFlow left a comment

Choose a reason for hiding this comment

Uh oh!

antonag32 commented Mar 13, 2025

Uh oh!

bguillot commented Apr 1, 2025

Uh oh!

OCA-git-bot commented Apr 1, 2025

Uh oh!

Uh oh!

OCA-git-bot commented Apr 1, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

20 participants