Repository navigation
[RFC] new api.groups decorator for function #944
Description
Activity
I think this is a good idea - security by obscurity (hiding the buttons) doesn't work.
Question is where to put this. I made an OCA Python module that has an
api.foreachimplementation in it, but there wasn't much movement on the getting it into OCA frontI still think it's a good idea to have an OCA API library. @lasley you only need to create the repo following the method put in the OCA instance task ("Create a project") and make the PR.
About this feature, 👍 for adding it, but not with the decorators
add_groupsand so on. This should be handled with the inheritance: if you add any new group in the decorator on an overwritten method, then that group is added to the list. About the removal of groups, then you should use other techniques like in core (for example, another method with less groups that calls the original method).Hi ! Thanks for your answer. A OCA API library LGTM. Great idea !
but not with the decorators add_groups and so on
You're right, it could be simplified indeed.
About the removal of groups, then you should use other techniques like in core (for example, another method with less groups that calls the original method).
Not sure it will cover all the needs, but we'll talk about it in a specific PR against the new repo.
@lasley you only need to create the repo following the method put in the OCA instance task ("Create a project") and make the PR.
@lasley , please ping me when the repo is created, I'll began a POC then and try to finish the work in the next Open (/ Closed) Days.
I still think it's a good idea to have an OCA API library. @lasley you only need to create the repo following the method put in the OCA instance task ("Create a project") and make the PR.
Good point - at the time I didn't have permissions to do this. I'm not finding this process in the OCA instance though- I'll dig a bit more & report back with the new repo.
Reacted by Jairo LlopisHmmm ok yeah so I can't find a process for repo creation, so I was just going to go ahead and create the repo - but it seems I don't have permissions. @pedrobaeza would you mind creating a blank OCA/python-oca repo so that I can submit a PR to it?
👍 For having a decorator that does this. I had the idea to create a
@privilegeddecorator that would do the same thing and that could be imported from a new tools module in server-tools.But having the decorator a part of a pip library might be as well. I only wonder about the name @api.groups. Would that not be confusing as this is then not coming from the odoo api python module?
Hi @NL66278,
well, about the name space, I think that collision will not be possible, because the name will be @oca.groups and not @api.groups. (or something similar)
@legalsylvain I was reacting to the first example use. Having
@oca.groupswould be great.@legalsylvain It's a good idea!
I'd rather keep' oca' as a namespace package available to all oca packages. We could name this new package 'oca-decorator'. This package would provide 'oca.decorator' (or oca.xyz)
from oca.decorator import groups @groups(..) def function_name():
About
groupsI'd prefer a more explicit name for exampleallowed_groups.lmi
@lasley this is the task where the process for a new repository/PSC is specified: https://odoo-community.org/web#id=264&view_type=form&model=project.task&action=468&active_id=2
Omg thanks Pedro! For some reason I searched everything from Project to Repo, but didn't think to include "PSC". Derp!
I'll still need some additional rights in the OCA GitHub though - I don't have the new button:
Try again now that I have changed your role.
Reacted by Dave LasleyReacted by Dave Lasley7 remaining items
I have a question about the structure of this code. (maybe my question is irrelevant, sorry)
Basically, I see two ways to write this code.
A) via a classical odoo module (in an existing or new repository). so, to use it, we have to write some
from odoo.addons.my_oca_lib_in_module import my_decoratorB) via a python lib.
It seems that you all are talking about the solution "B", and of course, it is the cleaner solution.
but in the other hand, I fear that such technology will block some users that have no technical skill to use modules that depend on the new oca-python lib. I think for a exemple to people that are installing module via the UI, or other via odoo.sh (and tomorrow with the OCA appstore, maybe !). I'm not sure that all the hosting system will allow to install custom python lib easily.
it would be a pity if some users will not have the possibility to install OCA modules, because there are depending on an extra lib that is not installable for some people.There are only one other exemple in the OCA for the time being. The openupgradelib. the code was initially in the OpenUpgrade project, and was moved into an external python-lib. For me it is not a blocking point for the openupgradelib, because making an upgrade is a technical thing.
What do you think ?
Do we know how Odoo.sh handles pip dependencies?
IMO this is on them to fix the
external_depencieskey in the manifest - we have a lot of modules that use it. I don't think modules using this new library would be any different than, say,barcodes_generator_abstractrequiringbarcodenot to say Odoo.sh (or Odoo.hs for french people) is the next Runbot. Bet taken.
Reacted by Laurent Mignon (ACSONE)😆 Yeah we know where my bet lies on that one
we have a lot of modules that use it (by @lasley)
Indeed, but it is quite different. Of course, if you want a system that handle barcode generation, you should install barcode lib. the same for asterisk, etc...
but this is very limited, and maybe some users in the world are not installing such modules because they can't or they don't know how to do. but there is no alternative. to use barcode generation you need barcode library.Here, it's different. I just updated OCA code source analysis. today there are 4182 modules. (a module present in two milestone is counted twice). In the other hand, In all that 4182 modules, there are only 274 python dependencies. So 94% of the OCA modules are not depending on external python libs and can be installed very easily. details :
If tomorrow, there is a good extra oca python lib with usefull decorators (like the one we try to describe in this thread that try to fix a security issue), most of the OCA modules will depends on this lib, step by step.
The objective of the OCA is to promote OCA members work. So if such decision (to create a lib to install) is reducing the possibility of people to use OCA modules, I think that we should not go in this direction for the time being. We could create first a little oca-decorator repository, with classical modules. And if in 1 / 2 years, it is mature, and if deployment and hosting system are more mature too (*), it will be very easy to put all the code in a external lib.
openupgrade code has been during years in openupgrade repository and has been set in a external lib recently by @StefanRijnhart. I think that this changes was not a big deal.
(*) : hosting and deployment system have changed a lot during the last years. (geting code by bzr, getting code by github, using buildout technology, and more recently pypi deployment)
I'd like to hear what @OCA/board are thinking about that point. it is not only a technical decision, it is also a strategic decision
Now that https://github.com/OCA/oca-decorators is available, this RFC can move there.
@legalsylvain - I originally submitted a module for
api.foreachbefore creating this as a library. Take a look at this PR, and you will see why we abandoned this strategy.Usage of the api was damned near impossible when you start accounting for an import path that is 60-70 chars, and a try/except that has to be put around each of the imports. This will make adoption of the module basically zero - the import is harder than the record loop.
Interestingly enough our try/except strategy fails miserably with decorators, but that's a completely different topic. Again, I wish Odoo would fix their
external_dependencieskey.@dreispt - is there a good way to move this pre-existing discussion to another repo? There's a lot of points exposed here that I'm not sure would be fluidly carried over.
Usage of the api was damned near impossible when you start accounting for an import path that is 60-70 chars, and a try/except that has to be put around each of the imports
I don't understand what is the try / except need in that solution.
To use a oca@foreach decorator define in a oca_api lib or module. :
Lib solution impacts
deploy :- pip install or via buildout. Saas limitation.
manifest file :
'external_dependencies': {'python': ['oca_api']}Python file :
from oca_api import ocaModule solution impacts
deploy :- pip install or download and install via UI or via buildout or just adding a module in addons file. no saas limitation.
manifest file :
'depends': ['oca_api'],Python file :
from odoo.addons.oca_api import ocaOr did I missed something ? If yes please correct me.
Kind regards.
I don't understand what is the try / except need in that solution.
The try/except is what we have to do when importing anything that is not core Odoo, so your examples would be the following instead:
try: from oca_api import oca except ImportError:... try: from odoo.addons.oca_api import oca except ImportError:...But yeah with your module naming, it isn't nearly as bad as mine was.
I think a lot of this has to do with the fact that the module is not installing into a DB, and is not encapsulated to the environment as most modules would be. This means it is providing functionality to environments that have not explicitly installed the module, and could be considered an issue (security or otherwise).
@lmignon just exposed similar argument to above in OCA/webhook#3 (comment). I think these are parallel conversations - basically boiling into how we provide feature-sets like this.
I personally feel that @lmignon is right here & we should be providing this as a lib. IIRC his advice was the main reason why I moved the
foreachdecorator out of a module as well.Hi @lasley.
Ow thanks ! I thought that making dependencies to a module was enough to assume that the module is present, but rgreping all OCA code, i saw a lot of samples of what you talk. (a lot for report_xls, and connector).
I don't have the skills to understand security issues hre, due to a module. For me, if the module is not installed, the code of the module of oca_api should not be called. but maybe I'm wrong.
Regards.
I thought that making dependencies to a module was enough to assume that the module is present,
Take a look at odoo/odoo#14850 for more info
For me, if the module is not installed, the code of the module of oca_api should not be called.
Therein lies the problem. This is not the case - Odoo imports all modules within the
addonspath into memory. Due to this, imports on the module will succeed in any module - whether or not the dependent is actually installed.This is core in Odoo, and realistically there is no circumvention. This is also why I do not co-host my customers in the same way that Odoo SA does. I'm curious if they've managed to solve this with Odoo.sh, but my guess is that they have a ticking timebomb on their hands in their SaaS
Thanks for all this clarifications. It's a pity that odoo/odoo#14850 is not accepted.
I still think that making a lot of OCA modules depending on an external lib will surely limit use of these modules. But well, a module doesn't seems to be a solution indeed.
Regards and thanks a lot for your insight.I close this issue. (moved into OCA/oca-decorators#7)
regards and thanks for all your comments.- added a commit that references this issue
on Nov 20, 2024
Hi all,
I think about a new decorator like
to decorate function that are call by function in a xml file like
<button name='function_name' />.The decorator will check if current user is a member of one of the defined groups, if not an access error is raised. Then, in the render view, the button is hidden if the user is not a member of the defined groups automatically
That new decorator could fix security breaches that allow user to call a function of a hidden button, by xml RPC.
In a second time we could imagine other decorators @api.add_groups or @api.remove_groups to add or remove access to a function, in a custom module that inherit a model.
Thanks for your comment.