[12.0][add] New decorator @api.allowed_groups

[legalsylvain]

Hi all,

The recent PR made by @StefanRijnhart here odoo/odoo#66505 made me think of an old idea that we talked about here #944 and here OCA/oca-decorators#7.

I so made a little PoC, and it works quite well.

@OCA Members : What do you think ? CC : @lasley, @pedrobaeza, @NL66278, @lmignon, @sbidoul

@odoo Team : Any chance to see this feature integrated in the Odoo core directly ? I don't know who ping in the team. @Yenthe666 any idea ?

Note
I finally made my PoC using a module design and not integrating the code in the current oca lib https://github.com/OCA/oca-decorators because I was more comfortable with this type of design, and that the reasons for using a library seems obsolete in recent versions of Odoo.

Description

This module is a technical module for developpers.

It adds a new decorator, named @api.allowed_groups.

• When the function is executed, it checks if the user belong to one of the given groups.

• It also adds automatically group(s) in the according form views to hide
  buttons if the user doesn't belong to one of the given groups.

Interests

1. it makes the application more secure, the developer only has to write the accreditation level in one place, instead of writing it twice (once in the XML view, and the other time in the python code)

In Odoo, there are many places where an action is hidden in the Form view, but the function can be called in particular by XML-RPC calls, that makes a lot a security issue, included
in recent version.

Ref : odoo/odoo#66505

2. It makes the code more easy to maintain, because there is only one place
   where the groups are defined instead two places without this module (XML and Python code)

Usage

Once installed, the following code:

<button type="object" name="my_action" groups="purchase.group_purchase_manager"/>

def my_action(self):
    if not self.env.user.has_group("purchase.group_purchase_manager"):
        raise AccessError(_("Some Error"))
    pass

can be replaced by:

<button type="object" name="my_action"/>

@api.allowed_groups("purchase.group_purchase_manager")
def my_action(self):
    pass

Multi-group membership checks

It is possible to list many groups. In that case, the action will be allowed
if the user belong to at least one group.

@api.allowed_groups("purchase.group_purchase_manager", "sale.group_sale_manager")
def my_action(self):
    pass

Conflict between view and decorators definition

The groups defined in the decorators take precedence over the groups that would be defined in the existing views.

Inheritance mechanisms

it is possible to change accreditation level in custom module.

For exemple, if a module A defines a function like this:

    @api.allowed_groups("purchase.group_purchase_manager")
    def my_action(self):
        pass

In a custom module B, that depends on module A :

1. You can overwrite accreditation level by writing

    # Now checks if user is member of 'purchase.group_purchase_user'
    @api.allowed_groups("purchase.group_purchase_user")
    def my_action(self):
        return super().my_action()

2. You can remove checkes, by writing

    # No longer performs checks
    @api.allowed_groups()
    def my_action(self):
        return super().my_action()

3. Or you can only overload the function, without changing the accreditation level by writing

    # the user must always be a member of 'purchase.group_purchase_manager'
    def my_action(self):
        # Custom code ...
        return super().my_action()

[Yenthe666]

Hmm it is an interesting idea. Did you check performance wise what the cost is?
If you'd like to discuss it with Odoo I bet we need @mart-e, @Feyensv and possible @odony their feedback/thoughts :)

[lmignon]

Note
I finally made my PoC using a module design and not integrating the code in the current oca lib https://github.com/OCA/oca-decorators because I was more comfortable with this type of design, and that the reasons for using a library seems obsolete in recent versions of Odoo.

IMO providing this functionality through a module is not appropriate for this kind of feature. It's not a per database functionality and once the addon is into the path (I think it's not required to install-it), the decorator is available for the whole runtime.

[NL66278]

Seems very interesting. Would help a lot to make functions more secure in an easy way. Some tests would definitely be helpful. I do not know how to evaluate the performance impact, except by testing it in practice. But certainly for some methods, checking the groups has to be done anyway.

[hparfr]

groups= in view should only be reserved to hide elements you have right to see / activate. And this PR make this possible so 👍

I have concerns about how it behave when :

• somewhere in the inheritance chain the decorator is missing
• some groups are added (limit rights)
• some groups are added (increase rights)

[legalsylvain]

Hi @lmignon.

Regarding the lib vs module design.

1. security issue :

once the addon is into the path (I think it's not required to install it), the decorator is available for the whole runtime.

You was right, but this point is obsolete in recent Odoo version. The code of the module decorator_allowed_groups is executed only if the module decorator_allowed_groups is installed. (or to be exact, present in the server_wide_modules list).
You can test this point with this test PR legalsylvain#8

• as you can see, the module decorator_allowed_groups_for_test_only depends only on purchase module, and if you try to install it, you'll have an error.

    File "/src_code/server-tools/decorator_allowed_groups_for_test_only/models/__init__.py", line 1, in <module>
        from . import purchase_order
    File "/src_code/server-tools/decorator_allowed_groups_for_test_only/models/purchase_order.py", line 8, in <module>
        class PurchaseOrder(models.Model):
    File "/src_code/server-tools/decorator_allowed_groups_for_test_only/models/purchase_order.py", line 11, in PurchaseOrder
        @api.allowed_groups("base.group_multi_company")
    AttributeError: module 'odoo.api' has no attribute 'allowed_groups'

• if you manuallly add a dependency to decorator_allowed_groups module, it will works.

You can read a more detailled explanation here : OCA/OpenUpgrade#2440 (comment)

2. Also I see it more difficult to maintain in a library that doesn't depends on the version of Odoo. This module overload the function ir.ui.view. the postprocess function can change over the version, making the code harder to maintain with some if version > 12.0 and version <= 14.0 ...

3. It has to be said that the project oca-decorators is not very active. See 8 commits between October 2017 and December 2017. For now, 2 of of 4121 OCA modules are using it :

[StefanRijnhart]

Maybe emit a warning if groups is already set and not equivalent?

[legalsylvain]

I thought about that point, and I'm not sure.

Consider you have an Odoo / OCA module with a def my_action(self) defined, and a button defined in a view with groups="module_1._group_A".

In a custom module, you want to change the group, so you simply write

    @api.allowed_groups("module_2.group_B")
    def my_action(self):
        return super().my_action(self)

If you do so, it will raise a false warning, forcing developpers to overload the view in the custom module, to remove the group.

I so just added in the USAGE.rst file this text :

The groups defined in the decorators take precedence over the groups that would be defined in the existing views.

What do you think ?

[StefanRijnhart]

My preference would be to emit the warning. It seems to me that the value of this module is mainly for methods introduced in modules that use this decorator in the first place, rather than overriding existing methods, and in that case, aligning the views is the proper thing to do to avoid confusion. But it's just my preference.

[NL66278]

What about combining the groups in the button and in the decorator, so you can only limit the authorities by either method, but never grant authorities?

[StefanRijnhart]

I would say that is not the way I would expect and want such a mechanism to work. I guess it depends on whether you take a security perspective or a usability perspective.

[NL66278]

It is what I would expect. But I think I expressed myself poorly. Combining should be interpreted as: when groups are defined on the button and in the decorator, only the groups that are valid in both should be shown. When this leaves no valid groups, at least a warning should be shown, or maybe even an exception thrown.

[legalsylvain]

Maybe emit a warning if groups is already set and not equivalent?

You're right. 👍
I added this point to the roadmap. Better to add a warning. We can remove it in a second time, if we consider later, that it is not relevant in some cases.

when groups are defined on the button and in the decorator, only the groups that are valid in both should be shown

do you mean :

• if groups="base.group_A,base.group_B"
• and @api.allowed_groups("base.group_B", "base.group_C")
  -> the result should groups="base.group_B" ?

[StefanRijnhart]

only the groups that are valid in both should be shown

Ah indeed, I misunderstood. This would improve the usability as well as the security. 👍

[StefanRijnhart]

I believe applying format on user controlled input (such as a translatable string) has security implications (except that format is applied here to the translation source instead of the translation value but that is another thing).

[legalsylvain]

applied here to the translation source instead of the translation value but that is another thing

Thanks ! Fixed.

applying format on user controlled input (such as a translatable string) has security implications

I didn't know. I replaced by %s. Is it better ? ;-)

[StefanRijnhart]

Yes, see OCA/pylint-odoo#302

[StefanRijnhart]

Do you know if decorators like this have to be repeated when their methods are overridden?

[sbidoul]

nice!

[legalsylvain]

Regarding inheritance,

@hpar said :

I have concerns about how it behave when :

• somewhere in the inheritance chain the decorator is missing
• some groups are added (limit rights)
• some groups are added (increase rights)

and @StefanRijnhart said :

Do you know if decorators like this have to be repeated when their methods are overridden?

Well, it was not correctly handled, but I refactored a little bit the code to make inheritance working. here is the text added in the USAGE.rst file.

Inheritance mechanisms

it is possible to change accreditation level in custom module.

For exemple, if a module A defines a function like this:

@api.allowed_groups("purchase.group_purchase_manager")
def my_action(self):
    pass

In a custom module B, that depends on module A :

1. You can overwrite accreditation level by writing

    # Now checks if user is member of 'purchase.group_purchase_user'
    @api.allowed_groups("purchase.group_purchase_user")
    def my_action(self):
        return super().my_action()

2. You can remove checkes, by writing

    # No longer performs checks
    @api.allowed_groups()
    def my_action(self):
        return super().my_action()

3. Or you can only overload the function, without changing the accreditation level by writing

    # the user must always be a member of 'purchase.group_purchase_manager'
    def my_action(self):
        # Custom code ...
        return super().my_action()

I wrote some dummy code, available here. you can play with that code grap/odoo-addons-extra#1, if you want to test.

[StefanRijnhart]

Thank you for your clarification regarding overrides.

Or you can only overload the function, without changing the accreditation level by writing

Just to be clear, is it the case that the check is executed when super() is called?

[legalsylvain]

Thank you for your clarification regarding overrides.

In fact, writing test, It seems that it doesnt work exactly as I have described it. But that is the objective !

Just to be clear, is it the case that the check is executed when super() is called?

I'll test it, but I guess, yes.

[lmignon]

    File "/src_code/server-tools/decorator_allowed_groups_for_test_only/models/__init__.py", line 1, in <module>
        from . import purchase_order
    File "/src_code/server-tools/decorator_allowed_groups_for_test_only/models/purchase_order.py", line 8, in <module>
        class PurchaseOrder(models.Model):
    File "/src_code/server-tools/decorator_allowed_groups_for_test_only/models/purchase_order.py", line 11, in PurchaseOrder
        @api.allowed_groups("base.group_multi_company")
    AttributeError: module 'odoo.api' has no attribute 'allowed_groups'

For sure the decorator will not be available in the odoo.api package since your patch is no executed while the addon is not imported by the addons loader. BUT the best practice is not to import the new decorator from odoo.api but from the its real source package from odoo.addons.decorator_allowed_groups.models.decorator import allowed_groups. In such a case, I'm pretty sure that you don't need to install the addon to use it once it's available into the addons_path. If you add an addon to server_wide_modules it's available for all the databases even these where it's not into the dependency graph.

My comment is not a blocking comment, it's just my thought about the right way to provide such a functionality.

[lmignon]

The decorator is not a model. IMO it should be moved into decorator_allowed_groups/api.py

[lmignon]

@legalsylvain IMO it's not a good practice to import the decorator from odoo.api this will only works if you add the addon into the server_wide_modules list and will require to modify the configuration file each time this addon is used to avoid strange error at server start. (Could become a pain in OCA repos). from odoo.addons.decorator.allowed_groups.api import allowed_groups will work in every case without requiring to add the addon into the server_wide_modules list

[thomaspaulb]

@legalsylvain Will you still be working on this?

[legalsylvain]

@legalsylvain Will you still be working on this?

Hi @thomaspaulb. Thanks for your interest.

My PoV :

I still think that :

• this feature could be implemented. (Less code / avoid security by obscurity)
• put the code in the external library oca-decorators is not a good idea. (In fact there is not a single commit on that library since 7 years). The code should be put in an odoo module.

But, now, I think that creating a dedicated OCA module is not a good idea. This feature should be put in the odoo base module to be used widely.
In fact, "good decorators" have been widely used because there are in the core. (like the "new" @api.model_create_multi that improves performance)

At the last Open Days 2023, I spoke briefly about this subject with @rco-odoo. Perhaps the idea will finally be taken up by the Odoo Core team...

[github-actions]

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.