[15.0][mig] password_security by dsolanki-initos · Pull Request #456 · OCA/server-auth

dsolanki-initos · 2023-01-02T10:03:58Z

No description provided.

amh-mw

I was just starting to migrate this module myself, but noticed that you have already made some good progress, so let me see if I can help you out instead. I am still investigating the "object unbound" test failures.

amh-mw · 2023-01-02T18:38:56Z

password_security/tests/test_password_security_home.py

+            with mock.patch("%s.request" % IMPORT) as request:
+                with mock.patch("%s.ensure_db" % IMPORT) as ensure:
+                    with mock.patch("%s.http" % IMPORT) as http:
+                        http.redirect_with_hash.return_value = MockResponse()


Mock request.redirect here instead?

amh-mw · 2023-01-02T18:46:44Z

password_security/models/res_users.py

+            ".{%d,}$" % int(company_id.password_length),
+        ]
+        if not re.search("".join(password_regex), password):
+            raise UserError(self.password_match_message())


Throw ValidationError here instead to match auth_saml test expectations?

Alternately, #457 should work around this without having to change any exception classes.

@amh-mw, I've done changes as per your feedback.

amh-mw

It looks like odoo.http.request is a proxy for werkzeug.local.LocalStack and the "object unbound" errors are due to the stack being accessed when it does not have a current request. There are some examples of how to work around this, i.e. odoo.addons.website.tools.MockRequest and the setUp function of odoo/addons/base/tests/test_xmlrpc.py, but nothing readily usable. I've worked up a small patch ¹ that pushes mock requests onto the stack for the duration of the tests.

https://gist.github.com/amh-mw/a7cee5ede165fc4b54a3ca504cbbebe9 ↩

dsolanki-initos · 2023-01-05T05:13:02Z

It looks like odoo.http.request is a proxy for werkzeug.local.LocalStack and the "object unbound" errors are due to the stack being accessed when it does not have a current request. There are some examples of how to work around this, i.e. odoo.addons.website.tools.MockRequest and the setUp function of odoo/addons/base/tests/test_xmlrpc.py, but nothing readily usable. I've worked up a small patch 1 that pushes mock requests onto the stack for the duration of the tests.

Footnotes

https://gist.github.com/amh-mw/a7cee5ede165fc4b54a3ca504cbbebe9 leftwards_arrow_with_hook

@amh-mw, Thank you for the suggestion regarding the test case failure, I'll test your patch locally and give you update regarding it.

dsolanki-initos · 2023-01-06T09:29:11Z

@amh-mw, The test case failure due to "object unbound" is fixed with your suggested patch. Also, fixed other errors Please review it.

Thank you for the support

amh-mw

Rather than review the full hundred files, I have individually reviewed the last four commits. I think you need to git fetch upstream, then git rebase -i upstream/15.0, deleting merge commit 285b284 and squashing 46c4666 into the migration commit, then force push back up to get a clean commit history. Otherwise LGTM.

dsolanki-initos · 2023-01-09T06:25:50Z

I'll fix the commits as per your feedback.

* [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

Currently translated at 100.0% (56 of 56 strings) Translation: server-auth-14.0/server-auth-14.0-password_security Translate-URL: https://translation.odoo-community.org/projects/server-auth-14-0/server-auth-14-0-password_security/fi/

Currently translated at 26.7% (15 of 56 strings) Translation: server-auth-14.0/server-auth-14.0-password_security Translate-URL: https://translation.odoo-community.org/projects/server-auth-14-0/server-auth-14-0-password_security/sv/

Currently translated at 39.2% (22 of 56 strings) Translation: server-auth-14.0/server-auth-14.0-password_security Translate-URL: https://translation.odoo-community.org/projects/server-auth-14-0/server-auth-14-0-password_security/sv/

amh-mw

@OCA/core-maintainers LGTM. Merge?

amh-mw · 2023-01-11T18:42:09Z

@OCA/tools-maintainers Ready for review and merge?

thomaspaulb

Only minor things LGTM

thomaspaulb · 2023-01-11T18:52:53Z

password_security/models/res_users.py

        if company_id.password_special:
            message.append(
-                _("\n* Special character (at least % characters)")
+                _("\n* Special character (at least %s characters)")


If you change it here, you should also update it in the .pot file.

thomaspaulb · 2023-01-11T18:53:30Z

password_security/models/res_users.py

                    _("Cannot use the most recent %d passwords")
                    % rec_id.company_id.password_history
                )
+        return


Does this really do anything?

It does nothing, I'll remove it.

OCA-git-bot · 2023-01-11T19:00:02Z

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

simahawk · 2023-01-13T15:09:08Z

/ocabot migration password_security

ivantodorovich

LG

simahawk · 2023-02-03T08:15:34Z

/ocabot merge nobump

OCA-git-bot · 2023-02-03T08:47:17Z

On my way to merge this fine PR!
Prepared branch 15.0-ocabot-merge-pr-456-by-simahawk-bump-nobump, awaiting test results.

OCA-git-bot · 2023-02-03T08:52:15Z

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

Syncing from upstream OCA/server-auth (16.0)

amh-mw suggested changes Jan 2, 2023

View reviewed changes

dsolanki-initos force-pushed the 15.0-mig-password_security branch from b8608ea to 544587a Compare January 4, 2023 11:01

amh-mw suggested changes Jan 4, 2023

View reviewed changes

dsolanki-initos force-pushed the 15.0-mig-password_security branch from b03721a to 46c4666 Compare January 6, 2023 09:20

amh-mw suggested changes Jan 6, 2023

View reviewed changes

lasley and others added 22 commits January 9, 2023 17:17

[MIG] Make modules uninstallable

915bdcf

[MIG] Rename manifest files

6fae00b

[MIG] password_security: Migrate to v10

364f247

* 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

7ed919c

* 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

d066ef3

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

Improve UX password_match_message()

7c8dc25

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

7116324

…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

8f74185

* 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

a95a179

[IMP]change request

5772c13

[IMP]Underscore is a special character #1077

25c1c8e

[FIX] password_security: Default last write date #1084

eabbf62

[FIX] UTF-8 coding is not necessary

271655f

[MIG] auth_totp_password_security: Migration to 11.0

96fde0b

[I18N] password_security: Translate to Spanish all translatable terms

b41b989

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

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

5333ce4

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

daba3b5

[MIG] password_security: Migration to 12.0

bcfec00

[FIX] password_security: correct readme fragments

aa5a029

jarmokortetjarvi and others added 5 commits January 9, 2023 17:17

[IMP] update dotfiles [ci skip]

35796e3

[IMP] password_security: black, isort, prettier

a11bb6c

dsolanki-initos force-pushed the 15.0-mig-password_security branch from 46c4666 to 873b81b Compare January 9, 2023 11:57

amh-mw approved these changes Jan 9, 2023

View reviewed changes

thomaspaulb approved these changes Jan 11, 2023

View reviewed changes

OCA-git-bot added approved ready to merge labels Jan 11, 2023

jans23 mentioned this pull request Jan 12, 2023

[MIG] password_security: Migration to 15.0 #356

Closed

[MIG] password_security: Migration to 15.0

ee0a84e

dsolanki-initos force-pushed the 15.0-mig-password_security branch from 873b81b to ee0a84e Compare January 12, 2023 05:33

simahawk changed the title ~~15.0 [mig] password_security~~ [15.0][mig] password_security Jan 13, 2023

OCA-git-bot added this to the 15.0 milestone Jan 13, 2023

OCA-git-bot mentioned this pull request Jan 13, 2023

Migration to version 15.0 #299

Closed

18 tasks

This was referenced Jan 18, 2023

[15.0][IMP] password_security: Match defaults to NIST 800-63B. #464

Closed

[15.0][MIG] base_export_async: Migration to 15.0 OCA/queue#498

Merged

ivantodorovich approved these changes Jan 26, 2023

View reviewed changes

OCA-git-bot added the bot is merging ⏳ label Feb 3, 2023

OCA-git-bot merged commit de53842 into OCA:15.0 Feb 3, 2023

OCA-git-bot added merged 🎉 and removed bot is merging ⏳ labels Feb 3, 2023

SiesslPhillip pushed a commit to grueneerde/OCA-server-auth that referenced this pull request Nov 20, 2024

Merge pull request OCA#456 from OCA/16.0

e8f0735

Syncing from upstream OCA/server-auth (16.0)

Uh oh!

Conversation

dsolanki-initos commented Jan 2, 2023

Uh oh!

amh-mw left a comment

Choose a reason for hiding this comment

Uh oh!

amh-mw Jan 2, 2023

Choose a reason for hiding this comment

Uh oh!

amh-mw Jan 2, 2023

Choose a reason for hiding this comment

Uh oh!

amh-mw Jan 2, 2023

Choose a reason for hiding this comment

Uh oh!

dsolanki-initos Jan 5, 2023

Choose a reason for hiding this comment

Uh oh!

amh-mw left a comment

Choose a reason for hiding this comment

Footnotes

Uh oh!

dsolanki-initos commented Jan 5, 2023

Footnotes

Uh oh!

dsolanki-initos commented Jan 6, 2023

Uh oh!

amh-mw left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsolanki-initos commented Jan 9, 2023

Uh oh!

amh-mw left a comment

Choose a reason for hiding this comment

Uh oh!

amh-mw commented Jan 11, 2023

Uh oh!

thomaspaulb left a comment

Choose a reason for hiding this comment

Uh oh!

thomaspaulb Jan 11, 2023

Choose a reason for hiding this comment

Uh oh!

thomaspaulb Jan 11, 2023

Choose a reason for hiding this comment

Uh oh!

dsolanki-initos Jan 12, 2023

Choose a reason for hiding this comment

Uh oh!

OCA-git-bot commented Jan 11, 2023

Uh oh!

simahawk commented Jan 13, 2023

Uh oh!

ivantodorovich left a comment

Choose a reason for hiding this comment

Uh oh!

simahawk commented Feb 3, 2023

Uh oh!

OCA-git-bot commented Feb 3, 2023

Uh oh!

OCA-git-bot commented Feb 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

20 participants

amh-mw left a comment •

edited

Loading