Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
base: 12.0
Are you sure you want to change the base?
[12.0][add] New decorator @api.allowed_groups #2026
Changes from 5 commits
e834c4c
e319f45
07b4599
ece8455
5d97cc1
3a201ec
5460029
c78369f
03fc87d
314ab14
6332db6
4228761
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe applying
format
on user controlled input (such as a translatable string) has security implications (except thatformat
is applied here to the translation source instead of the translation value but that is another thing).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks ! Fixed.
I didn't know. I replaced by
%s
. Is it better ? ;-)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, see OCA/pylint-odoo#302
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 withgroups="module_1._group_A"
.In a custom module, you want to change the group, so you simply write
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
do you mean :
groups="base.group_A,base.group_B"
@api.allowed_groups("base.group_B", "base.group_C")
-> the result should
groups="base.group_B"
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah indeed, I misunderstood. This would improve the usability as well as the security. 👍
There was a problem hiding this comment.
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 theserver_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 theserver_wide_modules
list