[RFC] new api.groups decorator for function

[legalsylvain]

Hi all,

I think about a new decorator like

@api.groups('group_1', 'group_2', '...')
def function_name(self, args):
    ...

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.

[lasley]

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.foreach implementation in it, but there wasn't much movement on the getting it into OCA front

[pedrobaeza]

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.

About this feature, 👍 for adding it, but not with the decorators add_groups and 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).

[legalsylvain]

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.

[lasley]

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.

[lasley]

Hmmm 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?

[NL66278]

👍 For having a decorator that does this. I had the idea to create a @privileged decorator 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?

[legalsylvain]

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)

[NL66278]

@legalsylvain I was reacting to the first example use. Having @oca.groups would be great.

[lmignon]

@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 groups I'd prefer a more explicit name for example allowed_groups.

lmi

[pedrobaeza]

@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

[lasley]

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:

[pedrobaeza]

Try again now that I have changed your role.

[lmignon]

@lasley @pedro Why python-oca? This name is meaningless. How do you plan to name the python package?

[lasley]

@lmignon - I asked for suggestions and this was the best one. The python package is similarly named.

What's you idea? I'm all ears.

[lmignon]

@lasley My main concern is to avoid the creation of a monolytic python package. If the primary goal it to provide new specialized decorators why not oca-decorator. The name should depend of the functional scope covered by the package or whatever you want. At least the name must be meaningfull. IMO 'python-oca' is too wide.

[lasley]

I'm good with oca-decorator, although we'd need to structure the module a bit differently I think then to make the decorators top-level in the namespace. Pretty sure I had it that way originally, but we pushed it back to oca.helpers in LasLabs/python-oca#1

Anyways it's easy enough to make that change, and I think it would be better to discuss it in the context of code. Seeing as I made the repo, will submit the PR and include your suggestion - then we can discuss in context instead of this hijacked PR.

[lmignon]

@lasley the ocanamespace must stay empty. Otherwise it will not be possible to create others python packages under the same namespace.

[lasley]

Got it, so we would basically just want to rename our helpers package to decorators, then call that our schema for the Python stuff? Abstract E.g. oca-package-sub == oca.package.sub

[lmignon]

yep 😏 that's the idea.

[sbidoul]

Too bad ocapi is taken 😸

[lasley]

OCA/oca-decorators#1 submitted

[legalsylvain]

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_decorator

B) 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 ?

[lasley]

Do we know how Odoo.sh handles pip dependencies?

IMO this is on them to fix the external_depencies key 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_abstract requiring barcode

[rvalyi]

not to say Odoo.sh (or Odoo.hs for french people) is the next Runbot. Bet taken.

[lasley]

😆 Yeah we know where my bet lies on that one

[legalsylvain]

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

[dreispt]

Now that https://github.com/OCA/oca-decorators is available, this RFC can move there.

[lasley]

@legalsylvain - I originally submitted a module for api.foreach before 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_dependencies key.

@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.

[legalsylvain]

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 oca

Module 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 oca

Or did I missed something ? If yes please correct me.

Kind regards.

[lasley]

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 foreach decorator out of a module as well.

[legalsylvain]

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.

[lasley]

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 addons path 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

[legalsylvain]

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.

[legalsylvain]

I close this issue. (moved into OCA/oca-decorators#7)
regards and thanks for all your comments.