[17.0][IMP] ai_oca_bridge_extra_parameters: adding env on eval context#56
Conversation
|
Hi @arielbarreiros96, |
arielbarreiros96
left a comment
arielbarreiros96
left a comment
There was a problem hiding this comment.
Hi @adasatorres, thanks for the contribution, if you need to use the env, you should be able to do if using object.env, if for some reason the object not set, you can always set the parameter_type to 'self', forcing the object for that case to be the bridge itself, and from there access the env. Hope this helps
|
Hi, @etobella. I understand the general concerns around exposing env within safe_eval, particularly from a security and execution-context control perspective (discussed in #7) . However, I would like to raise this specific case, as it has some particular characteristics. There are scenarios in which, because certain payload fields arrive as None, no object is generated. In these cases, having access to env is necessary to build system-level context information. From a security standpoint, the use of env should not pose any additional risk, as the cursor is protected and its usage is restricted within safe_eval. Furthermore, env is already indirectly available through the object, so explicitly including it does not expand the functional scope or current capabilities. Lastly, in terms of memory consumption, the impact would be equivalent, since env is a shared object and no new instances would be created. For these reasons, I believe that enabling env in this context would allow these specific cases to be addressed without introducing additional risks, while preserving the current guarantees around security and performance. |
etobella
left a comment
etobella
left a comment
There was a problem hiding this comment.
In this case, make it in an independent module.
For me it has security concerns and we should add it to everyone using the module.
Sorry if I’m not understanding the issue, but is there any difference between accessing env directly and accessing it through object.env? |
|
Currently, if the evaluated object (object or record) corresponds to an Odoo recordset, it is already possible to access the environment using object.env. Allowing direct access to env within the evaluation context simply simplifies the creation of formulas, enabling expressions such as env['res.partner'].search([...]) instead of object.env['res.partner'].search([...]. On the other hand, the execution environment remains protected through forbidden patterns, which prevent the execution of critical methods, imports, and dynamic code, as well as through the restriction of accessible variables in the context. This approach ensures that access continues to be secure and controlled. In summary, the user will continue to be limited by the permissions granted by Odoo and will not be able to access capabilities beyond what is already available. The proposal to expose env does not modify the functional scope, only the syntax allowed for evaluations, while maintaining all existing security and performance guarantees. I do not see the need to separate this into another addon. In any case, if this does not move forward, I will look for an alternative that does not involve modifying it. I will leave the PR open as I am interested in hearing the opinion of the rest of the community. Have a great day. |
| "KeyError", | ||
| "IndexError", | ||
| "ZeroDivisionError", | ||
| "env", |
There was a problem hiding this comment.
Not actually, you enabled it here. It was removed for a reason....
There was a problem hiding this comment.
There I'm allowing env itself, but object.env was usable before. I have several extra parameters using object.env and they work fine.
There was a problem hiding this comment.
Yes, yes, sorry, see my latest comment. I made a technical review of the module and you comments are true
etobella
left a comment
etobella
left a comment
There was a problem hiding this comment.
After a technical review of the module, it is true that object.env was allowed... for this reason it should have sense to use env....
I will not block it as it was approved before and the changes are consistent with the current situation
Make sense, from this perspective it makes the access to env more straight forward. It's a small change so I'm approving |
|
/ocabot merge patch |
|
What a great day to merge this nice PR. Let's do it! |
|
Congratulations, your PR was merged at fbb3ed8. Thanks a lot for contributing to OCA. ❤️ |
4 participants
This PR adds the Odoo environment to the evaluation context of formulas and extraparameter expressions.