[16.0][MIG/IMP] password_security

[astirpe]

Together with the module migration, I'm proposing a revised version of the module:

• Removed old widget, replaced by "Password Meter" widget by standard Odoo
• Unify field password_length with standard field minlength, to avoid incompatibility with standard module auth_password_policy_portal
• Rewrite tests for controller, needed because of new Odoo 16 changes
• Includes [15.0][FIX] password_security: Reset Password issue #492
• Includes [15.0][FIX] password_security: correctly look at previous password history #494
• Remove password estimate functionality (see [16.0][MIG/IMP] password_security #482 (comment))
• Fixes warning in tests password_security module throws warning during tests #141

[amh-mw]

@astirpe Nice job; tremendous amount of work. It feels like it should be six pull requests. If you would be willing to break it up, I'd be willing to review all the parts. I think it's a bit dangerous to do it all in one go, especially with the unit tests being rewritten. How would you feel about fixing each of the items you marked for back-porting in 15.0 as separate pull requests and getting those merged first, so that your migration commit isn't quite so expansive?

[astirpe]

@amh-mw, yes that's a good idea!

I backported two of the fixes: #491 (constraint of password_estimate range) and #492 (Reset Password issue)

The fix for #89 is the one already discussed in #448 and that's still an open PR.

The fix for #141 is just a consequence of the complete rewrite of the mocked tests.

For the rest, I don't think the other points should be backported to V15.
Once the above PRs for V15 will be merged, I will include the related commits in this PR. Is that ok for you?

[amh-mw]

Once the above PRs for V15 will be merged, I will include the related commits in this PR. Is that ok for you?

I think it would make more sense to rebase against 15.0 afterwards, so that you just have the one migration commit.

[amh-mw]

Just poking around a bit while waiting for the maintainers to merge the 15.0 pull requests. I'll give it a more thorough review after.

[amh-mw]

Do you need to provide a migration for this field? Maybe something like:

ICP = self.env['ir.config_parameter']
ICP.set_param('auth_password_policy.minlength', max(
    ICP.get_param('auth_password_policy.minlength'),
    *self.env['res.company'].search([]).mapped('password_length'))
))

[astirpe]

Yes, seems a good idea 👍 Thanks!

[astirpe]

Implemented. Migration tests could be executed locally by adding password_length = fields.Integer(default=12) to res.company

[amh-mw]

Seems weird to continue to hide minlength now that it has been adopted over password_length.

[astirpe]

It's just because I wanted to embed the minlength inside the group of fields "Password Policy"

server-auth/password_security/views/res_config_settings_views.xml

Lines 53 to 57 in 6eab9a5

	<div class="mt16">
	<span>
	Minimum number of characters
	<field name="minlength" class="oe_inline" />
	</span>

This way it looks&feels better, otherwise the minlength will stay detached from the rest of the fields.

[hwangh95]

How about instead of removing the entire box that contains the label, we reuse it? There might have been some customization within the settings box that gets removed otherwise.

Something like this?

<!-- Remove label for uniform styling -->
<xpath expr="//label[@for='minlength']" position="replace"/>


<!-- Add an id for easier lookup -->
<xpath expr="//field[@name='minlength']/ancestor::div[hasclass('o_setting_box')]" position="attributes">
    <attribute name="id">password_policy</attribute>
</xpath>

<!-- Move the settings box to desired location -->
<xpath expr="//div[@id='enable_password_reset']" position="after">
    <xpath expr="//div[@id='password_policy']" position="move"/>
</xpath>


<!-- Ensure our settings will come before any former customization -->
<xpath expr="//div[@id='password_policy']//div[hasclass('o_setting_right_pane')]/*" position="before">
    <!-- Add all the settings here -->
    <span id="minlength">
         Minimum number of characters
    </span>
</xpath>


<!-- Add oe_inline attribute to minlength -->
<xpath expr="//field[@name='minlength']" position="attributes">
  <attribute name="class">oe_inline</attribute>
</xpath>

<!-- Move the minlength field to desired location -->
<xpath expr="//span[@id='minlength']" position="inside">
    <xpath expr="//field[@name='minlength']" position="move"/>
</xpath>

It might seem like a lot of work, but I personally feel like we should try to minimize making nodes disappear

[astirpe]

Yes thanks! Done in b72dde1, would you check?

[hwangh95]

Looks mostly good to me! There's the comment that still says

I don't know if you wanted the password_security settings to come before or after the previous nodes.

[astirpe]

No idea why OCB test now fails. Maybe a temporary glitch?

[Abranes]

@astirpe Could you rebase from 16.0 to resolve the conflicts?

[astirpe]

Still test in OCB failing, I cannot even reproduce the failure with OCB locally.

[cmarrero]

After upgrade and enter Settings gives the following error with the refactor:

RPC_ERROR
Odoo Server Error
Traceback (most recent call last):
File "/Users/cmarrero/PycharmProjects/odoo/odoo/http.py", line 1584, in _serve_db
return service_model.retrying(self._serve_ir_http, self.env)
File "/Users/cmarrero/PycharmProjects/odoo/odoo/service/model.py", line 133, in retrying
result = func()
File "/Users/cmarrero/PycharmProjects/odoo/odoo/http.py", line 1611, in _serve_ir_http
response = self.dispatcher.dispatch(rule.endpoint, args)
File "/Users/cmarrero/PycharmProjects/odoo/odoo/http.py", line 1815, in dispatch
result = self.request.registry['ir.http']._dispatch(endpoint)
File "/Users/cmarrero/PycharmProjects/odoo/addons/website/models/ir_http.py", line 235, in _dispatch
response = super()._dispatch(endpoint)
File "/Users/cmarrero/PycharmProjects/odoo/odoo/addons/base/models/ir_http.py", line 154, in _dispatch
result = endpoint(**request.params)
File "/Users/cmarrero/PycharmProjects/odoo/odoo/http.py", line 697, in route_wrapper
result = endpoint(self, *args, **params_ok)
File "/Users/cmarrero/PycharmProjects/odoo/addons/web/controllers/dataset.py", line 42, in call_kw
return self._call_kw(model, method, args, kwargs)
File "/Users/cmarrero/PycharmProjects/odoo/addons/web/controllers/dataset.py", line 33, in _call_kw
return call_kw(request.env[model], method, args, kwargs)
File "/Users/cmarrero/PycharmProjects/odoo/odoo/api.py", line 457, in call_kw
result = _call_kw_model(method, model, args, kwargs)
File "/Users/cmarrero/PycharmProjects/odoo/odoo/api.py", line 430, in _call_kw_model
result = method(recs, *args, **kwargs)
File "/Users/cmarrero/PycharmProjects/odoo/odoo/addons/base/models/ir_ui_view.py", line 2491, in get_views
result['views'] = {
File "/Users/cmarrero/PycharmProjects/odoo/odoo/addons/base/models/ir_ui_view.py", line 2492, in
v_type: self.get_view(
File "/Users/cmarrero/PycharmProjects/odoo/odoo/addons/base/models/ir_ui_view.py", line 2676, in get_view
result = dict(self._get_view_cache(view_id, view_type, **options))
File "", line 2, in _get_view_cache
File "/Users/cmarrero/PycharmProjects/odoo/odoo/tools/cache.py", line 90, in lookup
value = d[key] = self.method(*args, **kwargs)
File "/Users/cmarrero/PycharmProjects/odoo/odoo/addons/base/models/ir_ui_view.py", line 2636, in _get_view_cache
arch, view = self._get_view(view_id, view_type, **options)
File "/Users/cmarrero/PycharmProjects/odoo/addons/web/models/models.py", line 245, in _get_view
arch, view = super()._get_view(view_id, view_type, **options)
File "/Users/cmarrero/PycharmProjects/odoo/odoo/addons/base/models/ir_ui_view.py", line 2583, in _get_view
arch = view._get_combined_arch()
File "/Users/cmarrero/PycharmProjects/odoo/odoo/addons/base/models/ir_ui_view.py", line 999, in _get_combined_arch
arch = root.with_prefetch(tree_views._prefetch_ids)._combine(hierarchy)
File "/Users/cmarrero/PycharmProjects/odoo/odoo/addons/base/models/ir_ui_view.py", line 940, in _combine
combined_arch = view.apply_inheritance_specs(combined_arch, arch)
File "/Users/cmarrero/PycharmProjects/odoo/odoo/addons/base/models/ir_ui_view.py", line 877, in apply_inheritance_specs
self._raise_view_error(str(e), specs_tree)
File "/Users/cmarrero/PycharmProjects/odoo/odoo/addons/base/models/ir_ui_view.py", line 754, in _raise_view_error
raise err from from_exception
ValueError: Element '' cannot be located in parent view

The above server error caused the following client error:
RPC_ERROR: Odoo Server Error
at makeErrorFromResponse (http://localhost:8069/web/assets/10737-0b43f28/web.assets_backend.min.js:985:163)
at XMLHttpRequest. (http://localhost:8069/web/assets/10737-0b43f28/web.assets_backend.min.js:993:13)

[astirpe]

@cmarrero if you are getting the error in a production server I would suggest you to restore the old code (before the config settings refactoring).

Instead if you are on a test server you can try this:

• Login as admin with enabled developer mode
• Go to Settings > Technical > User interface > Views
• Search view named "res.config.settings.form.password_security" and delete it
• Go to Apps
• Search module Password Security and click on Upgrade button
• Verify that the view named "res.config.settings.form.password_security" is re-created
• Check whether the error does not occur again

[astirpe]

I just found out that when a new user is trying to signup by entering a very large password, there is a critical performance issue.
It's caused by the adoption of zxcvbn library, in particular the execution of this line https://github.com/dwolfhub/zxcvbn-python/blob/master/zxcvbn/__init__.py#L25 is very slow. That could potentially cause a DDOS attack in my opinion.

Probably that's also the reason of #487 but I didn't investigate in deep that issue.

So I'm going to remove the password estimate check functionality from this module and also remove the dependency on zxcvbn, because the password estimation is already provided by "Password Meter" widget by standard Odoo.

I hope is ok with you all.

[hwangh95]

Are there any updates on this PR? Or is it good to go?

[astirpe]

Would be good for me if this PR gets merged soon

[hwangh95]

Small UI change

[zuher83]

Ready to migrate ?

[antonag32]

Hello, is there anything left to do before merging? I could help with any tasks left.

[thomaspaulb]

/ocabot migration password_security

/ocabot merge nobump

[OCA-git-bot]

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-482-by-thomaspaulb-bump-nobump, awaiting test results.

[OCA-git-bot]

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