Skip to content
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

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

Open
wants to merge 12 commits into
base: 12.0
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions decorator_allowed_groups/README.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
==========================
Decorator - Allowed Groups
==========================

1 change: 1 addition & 0 deletions decorator_allowed_groups/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
from . import models
17 changes: 17 additions & 0 deletions decorator_allowed_groups/__manifest__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# Copyright (C) 2021-Today: GRAP (<http://www.grap.coop/>)
# @author: Sylvain LE GAL (https://twitter.com/legalsylvain)
# License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl.html).

{
"name": "Decorator - Allowed Groups",
"summary": "Decorator that checks if user belong to given groups",
"author": "GRAP, Odoo Community Association (OCA)",
"website": "https://github.com/OCA/server-tools/",
"category": "Technical",
"version": "12.0.1.0.0",
"license": "AGPL-3",
"depends": [
"base",
],
"installable": True
}
30 changes: 30 additions & 0 deletions decorator_allowed_groups/i18n/fr.po
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
# Translation of Odoo Server.
# This file contains the translation of the following modules:
# * decorator_allowed_groups
#
msgid ""
msgstr ""
"Project-Id-Version: Odoo Server 12.0\n"
"Report-Msgid-Bugs-To: \n"
"POT-Creation-Date: 2021-02-19 07:52+0000\n"
"PO-Revision-Date: 2021-02-19 07:52+0000\n"
"Last-Translator: <>\n"
"Language-Team: \n"
"MIME-Version: 1.0\n"
"Content-Type: text/plain; charset=UTF-8\n"
"Content-Transfer-Encoding: \n"
"Plural-Forms: \n"

#. module: decorator_allowed_groups
#: code:addons/decorator_allowed_groups/models/decorator.py:45
#, python-format
msgid "To execute the function %s, you should be member of one of the following groups:\n"
" %s"
msgstr "Pour executer cette fonction %s, vous devez être membre de l'un des groupes suivants:\n"
" %s"

#. module: decorator_allowed_groups
#: model:ir.model,name:decorator_allowed_groups.model_ir_ui_view
msgid "View"
msgstr "Vue"

2 changes: 2 additions & 0 deletions decorator_allowed_groups/models/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
from . import decorator
from . import ir_ui_view
55 changes: 55 additions & 0 deletions decorator_allowed_groups/models/decorator.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
# Copyright (C) 2021-Today: GRAP (<http://www.grap.coop/>)
# @author: Sylvain LE GAL (https://twitter.com/legalsylvain)
# License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl.html).

from odoo import _, api
from odoo.exceptions import AccessError


def allowed_groups(*group_xml_ids):
Copy link
Contributor

@lmignon lmignon Feb 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

""" Return a decorator that specifies group(s)
to which the user must belong in order to perform
this function.
- if the user does not belong to any group,
an AccessError will be raised if the function is called.
- Also in the generated views, the according button
will be hidden if the user doesn't belong to the group(s).

@api.allowed_groups(
legalsylvain marked this conversation as resolved.
Show resolved Hide resolved
"purchase.group_purchase_manager",
"sale.group_sale_manager"
)
def my_secure_action(self):
pass
"""

def decorator(method):

Check warning on line 26 in decorator_allowed_groups/models/decorator.py

View check run for this annotation

Codecov / codecov/patch

decorator_allowed_groups/models/decorator.py#L26

Added line #L26 was not covered by tests

def secure_method(*args, **kwargs):

Check warning on line 28 in decorator_allowed_groups/models/decorator.py

View check run for this annotation

Codecov / codecov/patch

decorator_allowed_groups/models/decorator.py#L28

Added line #L28 was not covered by tests

_self = args[0]
_final_method = getattr(_self, method.__name__)
_group_xml_ids = getattr(_final_method, "_allowed_groups", [])

Check warning on line 32 in decorator_allowed_groups/models/decorator.py

View check run for this annotation

Codecov / codecov/patch

decorator_allowed_groups/models/decorator.py#L30-L32

Added lines #L30 - L32 were not covered by tests

# Check if current user is member of correct groups
if _self.user_has_groups(','.join(_group_xml_ids)):

Check warning on line 35 in decorator_allowed_groups/models/decorator.py

View check run for this annotation

Codecov / codecov/patch

decorator_allowed_groups/models/decorator.py#L35

Added line #L35 was not covered by tests
# If it's OK, return the original method
return method(*args, **kwargs)

Check warning on line 37 in decorator_allowed_groups/models/decorator.py

View check run for this annotation

Codecov / codecov/patch

decorator_allowed_groups/models/decorator.py#L37

Added line #L37 was not covered by tests
else:
# If it's KO, raise an error.
# We raise a technical message (with function name
# and xml_ids of the groups, because this message
# will be raised only in XML-RPC call.
raise AccessError(_(

Check warning on line 43 in decorator_allowed_groups/models/decorator.py

View check run for this annotation

Codecov / codecov/patch

decorator_allowed_groups/models/decorator.py#L43

Added line #L43 was not covered by tests
"To execute the function '%s', you should be member"
" of one of the following groups:\n '%s'") % (
method.__name__,
', '.join(_group_xml_ids)
))
setattr(secure_method, "_allowed_groups", group_xml_ids)
return secure_method

Check warning on line 50 in decorator_allowed_groups/models/decorator.py

View check run for this annotation

Codecov / codecov/patch

decorator_allowed_groups/models/decorator.py#L49-L50

Added lines #L49 - L50 were not covered by tests

return decorator

Check warning on line 52 in decorator_allowed_groups/models/decorator.py

View check run for this annotation

Codecov / codecov/patch

decorator_allowed_groups/models/decorator.py#L52

Added line #L52 was not covered by tests


api.allowed_groups = allowed_groups
20 changes: 20 additions & 0 deletions decorator_allowed_groups/models/ir_ui_view.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# Copyright (C) 2021-Today: GRAP (<http://www.grap.coop/>)
# @author: Sylvain LE GAL (https://twitter.com/legalsylvain)
# License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl.html).

from odoo import api, models


class IrUiView(models.Model):
_inherit = "ir.ui.view"

@api.model
def postprocess(self, model, node, view_id, in_tree_view, model_fields):
if node.tag in ["a", "button"] and node.get("name", False):
func = getattr(self.env[model], node.get("name"), False)
if func:
group_xml_ids = getattr(func, "_allowed_groups", False)
if group_xml_ids:
node.set("groups", ",".join(group_xml_ids))

Check warning on line 18 in decorator_allowed_groups/models/ir_ui_view.py

View check run for this annotation

Codecov / codecov/patch

decorator_allowed_groups/models/ir_ui_view.py#L18

Added line #L18 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@legalsylvain legalsylvain Feb 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return super().postprocess(
model, node, view_id, in_tree_view, model_fields)
1 change: 1 addition & 0 deletions decorator_allowed_groups/readme/CONTRIBUTORS.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* Sylvain LE GAL (https://twitter.com/legalsylvain)
18 changes: 18 additions & 0 deletions decorator_allowed_groups/readme/DESCRIPTION.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
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
---------

It makes the application more secure and more concise, 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 : https://github.com/odoo/odoo/pull/66505
5 changes: 5 additions & 0 deletions decorator_allowed_groups/readme/ROADMAP.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
* Write tests.

* Add a warning, if groups defined in decorator are not the same as
group defined in the view.
(remarks https://github.com/OCA/server-tools/pull/2026#discussion_r579102673)
86 changes: 86 additions & 0 deletions decorator_allowed_groups/readme/USAGE.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
Once installed, the following code:

.. code-block:: xml

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

.. code-block:: python

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

can be replaced by:

.. code-block:: xml

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

.. code-block:: python

@api.allowed_groups("purchase.group_purchase_manager")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

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.

.. code-block:: python

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

.. code-block:: python

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

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

* You can overwrite accreditation level by writing

.. code-block:: python

# 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()

* You can remove checkes, by writing

.. code-block:: python

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

* You can only overload the function, without changing the accreditation level by writing

.. code-block:: python

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