-
-
Notifications
You must be signed in to change notification settings - Fork 491
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FIX] base_tier_validation: allow sudo writes #876
base: 16.0
Are you sure you want to change the base?
Conversation
Hi @LoisRForgeFlow, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @yajo
What is the process that needs to bypass the validation?
The problem I see with this change is that it is a bit to wide of a fix. 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
You can see the full report in #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).
AFAICS, the 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 |
Thanks, it was in the description, too quick reading... 🤦♂️
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. |
4e4db90
to
2b0f4ec
Compare
Yeah, it's a tricky case. I could follow this other approach from #875 (comment):
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 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 |
Fix #875 by always allowing superuser to write.
@moduon MT-5997