[FIX] base_tier_validation: allow sudo writes

[yajo]

Fix https://github.com/OCA/server-ux/issues/875 by always allowing superuser to write.

@moduon MT-5997

[OCA-git-bot]

Hi @LoisRForgeFlow, some modules you are maintaining are being modified, check this out!

[yajo]

What is the process that needs to bypass the validation?

You can see the full report in https://github.com/OCA/server-ux/issues/875. The process is the standard notifications creation cron, along with the need to auto-generate a portal token for some records. It's an Odoo standard process (you might see other modules in the traceback, but they don't affect this issue).

For instance, it will allow procurements to extend POs under validation even when the procurement comes from a regular user. See https://github.com/odoo/odoo/blob/9fd48fcaa04e884871789abb7f19dacef49a2a8c/addons/purchase_stock/models/stock_rule.py#L139

AFAICS, the purchase_tier_validation module validates only purchase.order, not purchase.order.line, so IIUC this problem you say wouldn't happen.

Do you see other cases where this would be dangerous?

FWIW, reading this module code, it seems to me like it could be implemented as an extension to the rules system, where write permissions are extended by the validation state of a record. A sensible refactor would be to override check_access_rule() instead of overriding write(). And just like the rules system, IMHO it makes sense that sudo() overpasses them.

[fcvalgar]

I'm testing in an EE client environment with the following modules installed and have not seen any problems with any of the flows used by these modules.

[LoisRForgeFlow]

@yajo

You can see the full report in https://github.com/OCA/server-ux/issues/875.

Thanks, it was in the description, too quick reading... :man_facepalming:

AFAICS, the purchase_tier_validation module validates only purchase.order, not purchase.order.line, so IIUC this problem you say wouldn't happen.

EDIT: I did a wrong test, it appear to go throught, which was unexpected for me. I'll review.

EDIT2: So I tested v15 and the issue also happens, a procurement can extend a PO under validation, this was for sure not the intended behavior. So we have one more issue to fix...

In any case, from my point of view since there are sudos in several places in odoo core that you cannot control, I'm not 100% sure that the patch you propose is the path to go.

[yajo]

Yeah, it's a tricky case.

I could follow this other approach from https://github.com/OCA/server-ux/issues/875#issue-2282619751:

1. Add portal to the dependencies of base_tier_validation
2. Override _portal_ensure_token().
3. Call super with_context(skip_validation_check=True)

However I feel like it's the wrong approach.

I mean: by default, when Odoo does some action with sudo, it usually means that it should happen always, no matter if you can or not write to the record. Thus, there are only a few sudos around, like the case at hand.

So I think it makes sense to assume that the tier validation modules should cover those cases explicitly if needed. I pushed a change to avoid skipping validations when skip_validation_check context is explicitly set to False.

But you're the maintainer. If you prefer the other solution, just tell me and I'll adapt the PR. It will involve depending on portal, though.

[rafaelbn]

Hello @yajo ,

Then, after talking with @LoisRForgeFlow , the FIX https://github.com/OCA/delivery-carrier/pull/815 is enough ?

😄 ❤️ Thank all for you ideas and opinions with a great and sane contributions!

[yajo]

the FIX OCA/delivery-carrier#815 is enough ?

Yes, that fixes the problem without needing to merge this PR.

However, this PR still makes sense IMHO. I let you close it or merge it @LoisRForgeFlow 😃

[LoisRForgeFlow]

However, this PR still makes sense IMHO. I let you close it or merge it @LoisRForgeFlow 😃

Yes, I mean conceptually it makes sense. However, we uncovered the issue with PO lines that should be addressed and we also know that the procurements use sudo. I'll need to find some time to tinker a bit.

[rafaelbn]

@yajo Review this please! 🙏🏼 I guess we can close it as you say in https://github.com/OCA/server-ux/issues/875 that it is solved in https://github.com/OCA/delivery-carrier/pull/799 so this PR is already solved in other module? Thank you! 😄 ❤️

[yajo]

Nevertheless, this PR is probably the good thing to do, but I let @LoisRForgeFlow choose if merge or close.

For our particular issue, it's not needed because it was fixed in OCA/delivery-carrier#799.

[github-actions[bot]]

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.