pengyuthon/odoo_source
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[FW][FIX] knowledge: be explicit about ACLs checks

Browse Source 
In this commit we add some explicit checks in methods managing members when
they are intended to be used directly. Some internal methods do not check
for ACLs / access, in which case a note is added in docstring to be clear
about it.

Task-2859616

X-original-commit: odoo/odoo@b2b1aeb099
Part-of: odoo/odoo#95471
This commit is contained in:
Thibault Delavallée
2022-07-07 17:31:04 +02:00
parent 32dd65d059
commit 20008366cb
1 changed files with 65 additions and 29 deletions
Download Patch File Download Diff File Expand all files Collapse all files
addons/knowledge/models/knowledge_article.py
+65 -29
View File
@@ -909,13 +909,19 @@ class Article(models.Model):
def restore_article_access(self):
""" Resets permissions based on ancestors. It removes all members except
members on the articles that are not on any ancestor or that have higher
permission than from ancestors. """
permission than from ancestors.
Security note: this method checks for write access on current article,
considering it as sufficient to restore access and members.
"""
self.ensure_one()
if not self.parent_id:
return False
if not self.env.su and not self.user_has_write_access:
raise AccessError(_('You cannot restore the article %(article_name)s',
article_name=self.display_name))
raise AccessError(
_('You have to be editor on %(article_name)s to restore it.',
article_name=self.display_name))
member_permission = (self | self.parent_id)._get_article_member_permissions()
article_members_permission = member_permission[self.id]
parents_members_permission = member_permission[self.parent_id.id]
@@ -982,6 +988,11 @@ class Article(models.Model):
downgrade = not self.is_desynchronized and self.parent_id and \
ARTICLE_PERMISSION_LEVEL[self.parent_id.inherited_permission] > ARTICLE_PERMISSION_LEVEL[permission]
if downgrade:
# desync is done as sudo, explicitly check access
if not self.env.su and not self.user_has_write_access:
raise AccessError(
_('You have to be editor on %(article_name)s to change its internal permission.',
article_name=self.display_name))
return self._desync_access_from_parents(force_internal_permission=permission)
values = {'internal_permission': permission}
@@ -1002,12 +1013,20 @@ class Article(models.Model):
the article is desynchronized form its parent;
Else we add a new member with the higher permission;
Security note: this method checks for write access on current article,
considering it as sufficient to modify members permissions.
:param <knowledge.article.member> member: member whose permission
is to be updated. Can be a member of 'self' or one of its ancestors;
:param str permission: new permission, one of 'none', 'read' or 'write';
:param bool is_based_on: whether rights are inherited or through membership;
"""
self.ensure_one()
if not self.env.su and not self.user_has_write_access:
raise AccessError(
_('You have to be editor on %(article_name)s to modify members permissions.',
article_name=self.display_name))
if is_based_on:
downgrade = ARTICLE_PERMISSION_LEVEL[member.permission] > ARTICLE_PERMISSION_LEVEL[permission]
if downgrade:
@@ -1015,7 +1034,9 @@ class Article(models.Model):
else:
self._add_members(member.partner_id, permission)
else:
member.sudo().write({'permission': permission})
member.article_id.sudo().with_context(knowledge_member_skip_writable_check=True).write({
'article_member_ids': [(1, member.id, {'permission': permission})]
})
def _remove_member(self, member):
""" Removes a member from the article. If the member was based on a
@@ -1023,6 +1044,12 @@ class Article(models.Model):
We also ensure the partner to remove is removed after the desynchronization
if was copied from parent.
Security note
* when removing themselves: users need only read access on the article
(automatically checked by access on self);
* when removing someone else: write access is required on the article
(explicitly checked);
:param <knowledge.article.member> member: member to remove
"""
self.ensure_one()
@@ -1031,36 +1058,38 @@ class Article(models.Model):
# belongs to current article members
current_membership = self.article_member_ids.filtered(lambda m: m == member)
current_user_partner = self.env.user.partner_id
remove_self = member.partner_id == current_user_partner
# If remove self member, set member permission to 'none' (= leave article) to hide the article for the user.
if remove_self:
if member.partner_id == current_user_partner:
if current_membership:
members_command = [(1, current_membership.id, {'permission': 'none'})]
else:
members_command = [(0, 0, {'partner_id': current_user_partner.id, 'permission': 'none'})]
self.sudo().write({'article_member_ids': members_command})
# member to remove is on the article itself. Simply remove the member.
elif current_membership:
else:
if not self.env.su and not self.user_has_write_access:
raise AccessError(
_("You cannot remove the member %(member_name)s from article %(article_name)s",
member_name=member.display_name,
article_name=self.display_name
))
self.sudo().write({'article_member_ids': [(2, current_membership.id)]})
# Inherited rights from parent
else:
self._desync_access_from_parents(force_partners=self.article_member_ids.partner_id)
current_membership = self.article_member_ids.filtered(lambda m: m.partner_id == member.partner_id)
_("You have to be editor on %(article_name)s to remove or exclude member %(member_name)s.",
article_name=self.display_name,
member_name=member.display_name))
# member is on current article: remove member
if current_membership:
self.sudo().write({'article_member_ids': [(2, current_membership.id)]})
# inherited rights from parent: desync and remove member
else:
self._desync_access_from_parents(force_partners=self.article_member_ids.partner_id)
current_membership = self.article_member_ids.filtered(lambda m: m.partner_id == member.partner_id)
if current_membership:
self.sudo().write({'article_member_ids': [(2, current_membership.id)]})
def _add_members(self, partners, permission, force_update=True):
""" Adds new members to the current article with the given permission.
If a given partner is already member permission is updated instead.
Security note: this method checks for write access on current article,
considering it as sufficient to add new members.
:param <res.partner> partners: recordset of res.partner for which
new members are added;
:param string permission: member permission, one of 'none', 'read' or 'write';
@@ -1070,7 +1099,8 @@ class Article(models.Model):
self.ensure_one()
if not self.env.su and not self.user_has_write_access:
raise AccessError(
_("You cannot give access to the article '%s' as you are not editor.", self.name))
_("You have to be editor on %(article_name)s to add members.",
article_name=self.display_name))
members_to_update = self.article_member_ids.filtered_domain([('partner_id', 'in', partners.ids)])
partners_to_create = partners - members_to_update.mapped('partner_id')
@@ -1090,9 +1120,12 @@ class Article(models.Model):
def _desync_access_from_parents(self, force_internal_permission=False,
force_partners=False, force_member_permission=False):
""" Copies copy all inherited accesses from parents on the article and
de-synchronize the article from its parent, allowing custom access management.
We allow to force permission of given partners.
""" Copies all inherited accesses from parents on the article and desynchronize
the article from its parent, allowing custom access management. We allow
to force permission of given partners.
Security note: this method does not check accesses. Caller has to ensure
access is granted, depending on the business flow.
:param string force_internal_permission: force a new internal permission
for the article. Otherwise fallback on inherited computed internal
@@ -1166,6 +1199,9 @@ class Article(models.Model):
invisible nodes in it (A-B-C where B is not achievable). You might
archive / privatize articles and break hierarchy without knowing it.
Security note: this method does not check accesses. Caller has to ensure
access is granted, depending on the business flow.
:return <knowledge.article> children: the children articles which were
not detached, meaning that current user has write access on them """
all_descendants_sudo = self.sudo()._get_descendants()
@@ -1182,19 +1218,19 @@ class Article(models.Model):
# create new root articles and reset desync: direct children of these articles +
# the writable descendants. Indeed they are going to be modified the same way
# as "self" (archived / moved to private) -> all their children should be detached
new_roots_woperm = other_descendants_sudo.filtered(
new_roots_woperm_sudo = other_descendants_sudo.filtered(
lambda article: article.parent_id in (self + writable_descendants_sudo) and not article.internal_permission)
new_roots_wperm = other_descendants_sudo.filtered(
new_roots_wperm_sudo = other_descendants_sudo.filtered(
lambda article: article.parent_id in (self + writable_descendants_sudo) and article.internal_permission)
if new_roots_wperm:
new_roots_wperm.write({
if new_roots_wperm_sudo:
new_roots_wperm_sudo.write({
'is_desynchronized': False,
'parent_id': False
})
for new_root in new_roots_woperm:
new_root.write({
for new_root_sudo in new_roots_woperm_sudo:
new_root_sudo.write({
'is_desynchronized': False,
'internal_permission': new_root.inherited_permission,
'internal_permission': new_root_sudo.inherited_permission,
'parent_id': False,
})