[18.0][FIX] account_payment_order: Manager can create partner bank accounts

[monen17]

I have found the following unexpected behavior while writing the tests of OCA/l10n-italy#4920:

1. Write a test class inheriting from AccountTestInvoicingCommon, note that the self.env.user is an account manager.
2. In a test, create a res.partner.bank record

Expected:
The record is created

Actual:

odoo.exceptions.AccessError: You are not allowed to create 'Bank Accounts' (res.partner.bank) records.

This operation is allowed for the following groups:
	- Extra Rights/Accounting / Payments

Contact your administrator to request access if necessary.

Additional context
The root cause is that in

bank-payment/account_payment_order/security/ir.model.access.csv

Line 7 in 311be5c

base.access_res_partner_bank_group_partner_manager,Full access on res.partner.bank to Account Payment group,base.model_res_partner_bank,group_account_payment,1,1,1,1

an access right in the core is edited so that only users of the group Extra Rights/Accounting / Payments can create partner bank accounts.

In the tests of 16.0 this was not a problem because the account manager created by AccountTestInvoicingCommon was inheriting the groups of OdooBot in https://github.com/odoo/odoo/blob/5de47fb3bd4619dab2c84fff8f08d69dfee7b308/addons/account/tests/common.py#L49.
This is no more the case in 18.0, where the account manager only has the account manager groups (see https://github.com/odoo/odoo/blob/0745730d44ad2a0e198a6f37b4ee7cb8e88f30ea/addons/account/tests/common.py#L248).

I understand there are security implications for this change, but I think it makes sense that an account manager can create bank accounts of the partners.

Please let me know if I'm wrong or if there are better solutions! Thanks

[pedrobaeza]

This was done on purpose for security, removing the permissions and putting them in a special group called "Accounting / Payments".

[monen17]

This was done on purpose for security, removing the permissions and putting them in a special group called "Accounting / Payments".

Thanks for having a look!
The special group is still there, do you think there is a problem with the proposed solution?
Do you have an alternative solution for the described issue?
I might move this edit to a noupdate="1" data node so that the user can remove the implied link, would that be better?

This issue affects any test class inheriting from AccountTestInvoicingCommon and that's a common base class for tests using invoices; it also affects modules that do not depend on account_payment_order as long as it has been installed in the DB before the tests execution

[TheMule71]

This was done on purpose for security, removing the permissions and putting them in a special group called "Accounting / Payments".

Hi, to clarify what's happening here.

Module A changes the behaviour of a standard model by requiring membership to a module specific group for users to create an instance.

Module B - that does NOT depend on A - creates instances of such model in its test.

A new module C that depends on both A and B is created (or migrated). Now tests on C fail in module B, because B does not use the group defined in A when creating instances of the model.

This creates a situation that cannot be solved in C unless it actively undoes what module A does permission-wise, which is not the desired outcome.

Plus B is just one of many other modules that potentially may access the base model in their tests, not to mention it's completely unrelated to module A, so changing it to depend on A and modifing its tests is also a non solution.

[andreampiovesana]

we need to sblok the situation, @pedrobaeza what do you suggest?

[micheledic]

@pedrobaeza ping

[pedrobaeza]

I'm OK with removing this restriction instead of adding a new permission if #1340 is finished and bring to 18.0. The question is that an invoicing manager shouldn't be able to send money without more supervision. That's a very critical security hole. But having the other flag with its isolated permission makes the same effect.

[monen17]

I'm OK with removing this restriction instead of adding a new permission if #1340 is finished and bring to 18.0. The question is that an invoicing manager shouldn't be able to send money without more supervision. That's a very critical security hole. But having the other flag with its isolated permission makes the same effect.

Thanks @pedrobaeza for the explanation and the proposed solution, now I can better understand what this module is doing and why.
Unfortunately the PR you linked has unsolved conversations and seems abandoned: I'm pretty sure I don't know enough about the subject to revive it.

Since the problem only happens during tests execution, I have changed approach and tried to only change what is happening in the affected accounting tests.
The new implementation won't change the module's functionality, so the existing security is preserved, but allows modules that are unaware of this change to work as expected.

[pedrobaeza]

@monen17 meanwhile, Odoo has made a move even in 16, forcing in certain flows to have the flag activated: odoo/odoo@7763099, so maybe is already covered or almost to get to that functional requirement.

[pedrobaeza]

I will review the upstream Odoo changes and make the corresponding changes here for not requiring this extra group.

[monen17]

@monen17 meanwhile, Odoo has made a move even in 16, forcing in certain flows to have the flag activated: odoo/odoo@7763099, so maybe is already covered or almost to get to that functional requirement.

Thanks for the update!
It's hard to keep track of upstream changes, if i understand that correctly, when that change gets merged in 18.0 we'll be able to drop the new security group added by this module.

[pedrobaeza]

Yes, I would say that is already in 18. I'll let you know.

[monen17]

Yes, I would say that is already in 18. I'll let you know.

I have seen #1557 just now; do you still plan to remove this security group? I don't want to rush you, just trying to keep track with what is happening.